WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 6 months ago

Last modified 4 months ago

#44044 closed enhancement (fixed)

$allowed_tags and $allowed_protocols in wp_privacy_generate_personal_data_export_group_html not filterable.

Reported by: TZ Media Owned by: garrett-eclipse
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch has-unit-tests has-dev-note
Focuses: Cc:

Description

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.

Attachments (6)

44044.patch (1.5 KB) - added by TZ Media 16 months ago.
44044.diff (771 bytes) - added by desrosj 15 months ago.
44044.2.diff (1.0 KB) - added by desrosj 14 months ago.
44044.3.diff (6.6 KB) - added by desrosj 14 months ago.
44044.4.diff (7.5 KB) - added by birgire 13 months ago.
44044-5.diff (7.6 KB) - added by birgire 7 months ago.

Download all attachments as: .zip

Change History (43)

@TZ Media
16 months ago

#1 @TZ Media
16 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

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.

This ticket was mentioned in Slack in #gdpr-compliance by tz-media. View the logs.


16 months ago

#3 @desrosj
16 months ago

  • Milestone changed from Awaiting Review to 4.9.7

#4 @desrosj
16 months ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

#5 @desrosj
16 months ago

  • Version set to 4.9.6

Marking privacy bugs as introduced in 4.9.6.

#6 @desrosj
16 months ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#7 @desrosj
15 months ago

@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
15 months ago

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


15 months ago

#9 @desrosj
15 months ago

  • Keywords 2nd-opinion added

#10 @desrosj
15 months ago

  • Keywords gdpr removed

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

#11 follow-up: @pbiron
14 months ago

  • Keywords changed from has-patch, needs-testing, 2nd-opinion to has-patch needs-testing 2nd-opinion

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.

#12 in reply to: ↑ 11 @desrosj
14 months ago

  • Type changed from defect (bug) to enhancement

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.

This ticket was mentioned in Slack in #core by joshuawold. View the logs.


14 months ago

This ticket was mentioned in Slack in #core by pbiron. View the logs.


14 months ago

This ticket was mentioned in Slack in #core by pbiron. View the logs.


14 months ago

#16 @pento
14 months ago

  • Keywords needs-refresh added; 2nd-opinion removed

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.

This ticket was mentioned in Slack in #core by joshuawold. View the logs.


14 months ago

#18 @pento
14 months ago

  • Milestone changed from 4.9.8 to 4.9.9

4.9.8 has hit RC, moving to 4.9.9.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


14 months ago

@desrosj
14 months ago

#20 @desrosj
14 months ago

  • Keywords needs-testing needs-refresh removed

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.

#21 @desrosj
14 months ago

  • Keywords has-unit-tests added

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.

@desrosj
14 months ago

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


13 months ago

@birgire
13 months ago

#23 @birgire
13 months ago

  • Keywords commit added

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.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


12 months ago

#25 @pento
12 months ago

  • Milestone changed from 4.9.9 to Future Release

This ticket was mentioned in Slack in #core-privacy by webdevlaw. View the logs.


11 months ago

#27 follow-up: @garrett-eclipse
8 months ago

  • Keywords needs-refresh added; commit removed

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.

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


8 months ago

#29 in reply to: ↑ 27 @garrett-eclipse
7 months ago

  • Milestone changed from Future Release to 5.2
  • Owner set to garrett-eclipse
  • Status changed from new to assigned

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
7 months ago

#30 @birgire
7 months ago

  • Keywords needs-refresh removed

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.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


7 months ago

#32 @garrett-eclipse
6 months ago

  • Keywords dev-feedback added

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

#33 @desrosj
6 months ago

  • Keywords commit added; dev-feedback removed

@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.

#34 @desrosj
6 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#35 @garrett-eclipse
6 months ago

  • Keywords needs-dev-note added

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


6 months ago

#37 @desrosj
4 months ago

  • Keywords has-dev-note added; commit needs-dev-note removed
Note: See TracTickets for help on using tickets.