$allowed_tags and $allowed_protocols in wp_privacy_generate_personal_data_export_group_html not filterable.

[TZ Media]

In order to allow other tags (e.g. <img>) and other protocols in the generated output html file, the $allowed_tags and $allowed_protocols variables in the function wp_privacy_generate_personal_data_export_group_html() should be filterable.

[TZ Media]

I couldn't figure out how to document the parameters for the filter hook wp_privacy_personal_data_export_allowed_tags. This needs to be added.

[desrosj]

Moving to the new Privacy component.

[desrosj]

Marking privacy bugs as introduced in 4.9.6.

[desrosj]

Moving all tickets in 4.9.7 to 4.9.8.

[desrosj]

@TZ Media good catch on this. I am wondering if massaging this to use wp_kses_allowed_html() and wp_allowed_protocols() is a better approach than introducing two new filters.

I think the the $allowedtags list in wp_kses_allowed_html() is a pretty basic list we may be able to utilize. Allowing basic formatting tags could potentially open the door for plugins to style the export files, and these tags could indicate important structural aspects of the data (acronym, cite, or abbr, for example) and may be better left in the export.

Incoming patch with this approach for thoughts and testing.

[desrosj]

Removing the GDPR keyword. This has been replaced by the new Privacy component and privacy focuses in Trac.

[pbiron]

question: this seems more like an enhancement rather than a defect. If it is truly an enhancement, then it needs to be committed before beta (currently scheduled for July 17) if it's going to land in 4.9.8.

[desrosj]

Replying to pbiron:

question: this seems more like an enhancement rather than a defect. If it is truly an enhancement, then it needs to be committed before beta (currently scheduled for July 17) if it's going to land in 4.9.8.

I had thought about this when I updated the ticket last. I could see either way (bug or enhancement). But, since it is changing the markup allowed, let's classify as an enhancement.

[pento]

44044.diff​ is a good approach. It can be simplified further, though. Remove the $allowed_tags and $allowed_protocols variables entirely, and change the wp_kses() call to be wp_kses( $value, 'personal_data_export' ). KSES will take care populating the actual HTML and protocol arrays.

[pento]

4.9.8 has hit RC, moving to 4.9.9.

[desrosj]

44044.2.diff​ let's KSES handle populating allowed HTML and protocol arrays. I did not add unit tests because this function currently does not have any. #44233 will add complete tests for this function, so I think it is better to just add a test method there ensuring disallowed HTML gets stripped appropriately. I'll follow up on that ticket to make sure these tests get added.

[desrosj]

Actually, changed my mind about attaching tests here. Looked at adding them to #44233 and realized that wp_privacy_generate_personal_data_export_group_html(), the function that generates the markup that is relevant here, is not included.

Incoming patch with tests.

[birgire]

44044.4.diff​:

• Splits up test_disallowed_html_is_stripped() into a new one test_allowed_html_not_stripped(), so we have one test method for allowed HTML and another one for disallowed HTML.
• Adds missing link tag in $data['items'][0]['links']['value'].
• Change site.com to example.com, in $data['items'][0]['images']['value'] as the former is a company's webpage.
• Adds "is allowed" in $data['items'][0]['links']['name'].
• Adds a full-stop to the inline comment in wp_privacy_generate_personal_data_export_group_html().

@desrosj I mark this for commit.

[garrett-eclipse]

Thanks @birgire the patch applies cleanly and tests run successfully.

I wonder if we can remove the empty $group_html instantiation and just create the variable on the following line here;
$group_html = '<h2>' . esc_html( $group_data['group_label'] ) . '</h2>';

Aside from that things are looking good, just applying refresh as the unit test versions will need to be updated when this gets milestoned.

[garrett-eclipse]

Hi @birgire I'm going to slate this into 5.2 as it mostly just needs the refresh for version change. If you're able to tackle that update would be appreciated.

As well wondering if we can account for this in the refresh as well.

I wonder if we can remove the empty $group_html instantiation and just create the variable on the following line here;
$group_html = '<h2>' . esc_html( $group_data['group_label'] ) . '</h2>';

Thank you

[birgire]

Thanks for the testing and suggestion @garrett-eclipse

44044-5.diff​ is a refresh for 5.2.0 and also with the suggestion by @garrett-eclipse remove the empty $group_html instantiation.

[garrett-eclipse]

Thanks @birgire for the refresh, this applies and unit test run smoothly.

As we discussed in DM, the only outstanding thought was potentially testing the edge cases of the switch to wp_kses here as it's a bit of a different approach. I've seen some tests that use kses_init_filters like here;
​https://github.com/aaronjorbin/develop-wordpress-verbose/blob/master/tests/phpunit/tests/post/output.php#L126-L172
But haven't seen any testing specifically using a key like personal_data_export.

I was going to move this forward to add 'commit' but am hesitant. @pento or @desrosj would there be need to setup unit tests for the change to wp_kses here? And if so what would be the approach.

Appreciated

[desrosj]

@birgire Thanks for the refresh. I can't reproduce the test failures that I was seeing previously when I reached out in Slack, so this is looking good.

I can't think of any edge cases we need to be overly concerned with.

In the current state, here is the sequence of events:

• An explicit list of allowed markup and attribute combinations are passed to wp_kses() in wp_privacy_generate_personal_data_export_group_html().
• The string is passed through the pre_kses filter with the list of allowed HTML and protocols.
• No further filters.

With the proposed changes:

• The wp_kses_allowed_html filter would be run on the list of allowed HTML tags after pre_kses with the context of personal_data_export.

The only edge case that I can think of is if someone was explicitly checking for the specific list of tags and attributes passed in wp_privacy_generate_personal_data_export_group_html() with pre_kses. But, this scenario would have been affecting plugins and themes that happened to be passing the same list of allowed tags.

This change is beneficial because it allows the tags allowed in this specific context is now possible. Which, should be the encouraged way to filter the allowed tags.

[desrosj]

In 44824:

Privacy: Be less restrictive of the HTML tags allowed in user data exports.

Previously, only a and br tags were allowed in the value table cell for each field included in the HTML file generated when a user is exporting their personal data. Instead of relying on a hardcoded list of allowed tags, the wp_kses() call in wp_privacy_generate_personal_data_export_group_html() will now fallback to the default list of allowed tags (which includes i, strong, em, and other basic HTML formatting tags).

Also, a new context of personal_data_export will now be passed to the wp_kses() call. As a result, the list of HTML tags and attributes allowed in the export file can now be filtered using the wp_kses_allowed_html filter and checking for the personal_data_export context.

Fixes #44044.
Props tz-media, desrosj, pento, birgire, garrett-eclipse.

[desrosj]

Dev note: ​https://make.wordpress.org/core/2019/04/24/developer-focused-privacy-updates-in-5-2/