Privacy Export Personal Data: JSON encoding failure generates invalid JSON in export file

[hellofromTonya]

When exporting personal data, if the JSON encoding of the groups fails in wp_privacy_generate_personal_data_export_file(), invalid JSON is generated in the generated export.json file:

"{Personal Data Export for tester@test.com:}"

See it in action here ​https://3v4l.org/bpmqG.

Notice:

• The invalid JSON (as there's no value after the colon :
• No error, warning, or notice is generated for PHP 5.6 to 8.0.3. This makes it difficult to debug as no information is provided as to why the JSON is invalid

Expected Behavior

Either:

• Generate an error via wp_send_json_error to alert of the problem and bail out of generating the file
• Or generate valid JSON in the file, e.g. '"false", with a _doing_it_wrong or notice in the error log

What causes JSON encoding to fail?

Found this issue when working on PHPStan type mismatches for when false passed to fwrite. ​`fwrite` expects a string as it's 2nd argument. This can happen when/if wp_json_encode returns false (i.e. JSON encoding fails).

json_encode returns false when:

• attempting to encode a non-UTF-8 string

WordPress automatically remedies the encoding with _wp_json_sanity_check where it converts string(s) to UTF-8.

• a resource is given

A resource could exist an element of the groups when the post meta data is filtered. Using ​this test script reproduces the failed JSON encoding and invalid JSON report.

How to Reproduce

1. Add ​this script as a must-use file
2. Go to "Tools" > "Export Personal Data" and add a test user in the "Username or email address" field to generate a request.
3. Hover over the email and click on "Download personal data"
4. Then unzip the downloaded zip file
5. Open export.json and notice no value exists in the JSON. For example, if the user's email is tester@test.com, the export.json file would contain:

   "{Personal Data Export for tester@test.com:}"
   

Additional Info

It is a type mismatch with fwrite, as a string is expected for the 2nd argument. Fixing this problem guards against future PHP type strictness.

Trac ticket: https://core.trac.wordpress.org/ticket/52892

Function: wp_privacy_generate_personal_data_export_file()

PHPStan flagged a type mismatch if false is passed into fwrite for groups JSON:
{{{php
fwrite( $file, $groups_json );
}}}

The groups JSON is assigned from wp_json_encode:
{{{php
$groups_json = wp_json_encode( $groups );
}}}

The ​return signatures of `wp_json_encode` are string or false:

_(string|false)_ The JSON encoded string, or false if it cannot be encoded.

### Is Passing false a problem?
See it in action here ​https://3v4l.org/bpmqG

• No errors, warnings, or notices happen in PHP 5.6 to 8.0.3
• Generates invalid JSON as false writes nothing into the file:

{{{json
"{Personal Data Export for tester@…:}"
}}}

This is the current behavior 👆, where generating invalid JSON is a bug.

### What can cause JSON encoding to fail?
json_encode returns false when:

• attempting to encode a non-UTF-8 string
• a resource is given

WordPress automatically remedies the encoding with _wp_json_sanity_check where it converts string(s) to UTF-8.

A resource could exist an element of the groups _when_ the post meta data is filtered. Using this ​test script reproduces the failed JSON encoding and invalid JSON report.

[hellofromTonya]

Moving into 5.8 milestone for visibility. Removed has-patch as the PR is currently a draft.

Proposing 2 different solutions:

Fix 1:
Send a JSON error

// Convert the groups to JSON format.
$groups_json = wp_json_encode( $groups );
if ( false === $groups_json ) {
        wp_send_json_error( __( 'Unable to encode the export file (JSON report).' ) );
}

This fix is breaking change as the files will not be generated or downloaded.

Fix 2:

Fix the type mismatch and then either trigger a notice or warning (to alert of the problem) or _doing_it_wrong.

// Convert the groups to JSON format.
$groups_json = wp_json_encode( $groups );
if ( false === $groups_json ) {
        $groups_json = '"false"';
        trigger_error( __( 'Unable to encode the export file (JSON report).' ), E_USER_NOTICE );
}

or

// Convert the groups to JSON format.
$groups_json = wp_json_encode( $groups );
if ( false === $groups_json ) {
        $groups_json = '"false"';
        _doing_it_wrong(
                __FUNCTION__,
                __( 'Unable to encode the export file (JSON report).' ),
                '5.8.0'
        );
}

[hellofromTonya]

Borrowing from REST ​per conversation in slack, improved Fix 2 to grab the error message and generate a _doing_it_wrong error. PR is updated and ready for review.

[hellofromTonya]

@SergeyBiryukov the patch for this ticket via ​PR 1115 is ready for commit.

BTW @jrf and I have reviewed it thoroughly through pair programming sessions. We decided to generate an error instead of fixing the invalid JSON value. Why? Fixing the JSON creates an empty export without knowing why it's empty or what happened. The error provides more context, i.e. the JSON encoding failed.

[SergeyBiryukov]

Thanks for the PR! This mostly looks good to me, I only have a couple notes:

• It's not quite clear to me why we only append json_last_error_msg() if there is no error code. In my testing, that leads to not getting any additional information about the error, while the actual error in my case was "Malformed UTF-8 characters, possibly incorrectly encoded". If json_last_error() is JSON_ERROR_NONE or empty, I don't think json_encode() would have returned false in the first place, unless I'm missing something. So I think it would make sense to always include json_last_error_msg() here. Something like:

  $error_message = sprintf(
  	/* translators: %s: Error message. */
  	__( 'Unable to encode the personal data for export. Error: %s' ),
  	json_last_error_msg()
  );
  

• Adding a remove_filter() call to the ::tearDown() method is not needed here. I understand that from a consistency point of view it seems like that if we add a filter somewhere, we should remove it as well, however the base WP_UnitTestCase_Base::tearDown() method already handles that for us via ::_restore_hooks(), see [29251] / #28535 for more details. So removing the filter ourselves is redundant, similar instances were previously removed in [50463] / #52625.

52892.diff​ includes these suggestions.

[hellofromTonya]

@SergeyBiryukov great points! You're right on both. Including the last message does provide more context for debugging. Thanks. PR updated though I see the diff you supplied has the same changes.

[davidbaumwald]

In 50713:

Privacy: Ensure "Export Personal Data" does not generate invalid JSON.

Previously, when exporting personal data, if the JSON encoding of the data failed, the invalid JSON was still written to export.json. This change captures the JSON encoding failure and adds a notice to the UI.

Props hellofromTonya, jrf, SergeyBiryukov.
Fixes #52892.

Committed with changeset https://core.trac.wordpress.org/changeset/50713