Make WordPress Core

Opened 8 years ago

Last modified 3 years ago

#37698 new defect (bug)

wp_kses_split global variable pollution

Reported by: xknown's profile xknown Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch needs-testing early
Focuses: Cc:

Description

In r10339, wp_kses_split was modified so it doesn't longer require the preg_replace with the e (eval) modifier. This implementation uses globals to pass the values of $allowed_html and $allowed_protocols to the _wp_kses_split_callback function.
While in most cases this isn't really a problem, we noticed that a call to wp_kses_split (via a filter) from within _wp_kses_split_callback may have undesirable effects on the next replacements.
The snippet below illustrates this problem, you can see in action in https://3v4l.org/YmYTZ

<?php

function wp_kses_split( $string, $allowed_html, $allowed_protocols ) {
    global $pass_allowed_html, $pass_allowed_protocols;
    $pass_allowed_html = $allowed_html;
    $pass_allowed_protocols = $allowed_protocols;
    return preg_replace_callback( '%((<!--.*?(-->|$))|(<[^>]*(>|$)|>))%', '_wp_kses_split_callback', $string );
}
function _wp_kses_split_callback( $match ) {
    global $pass_allowed_html, $pass_allowed_protocols;
    return wp_kses_split2( $match[1], $pass_allowed_html, $pass_allowed_protocols );
}

function wp_kses_split2($string, $allowed_html, $allowed_protocols) {
    wp_kses_split('', array(), array()); // this overrides the globals.
    print_r( array( $allowed_html, $allowed_protocols ) );
}

wp_kses_split("<a style='color: red;'>I link this</a>", array('a'=>array( 'style' => array() )), array('http') );

One way to fix this would be to use an anonymous function, but I guess that's only available on PHP >= 5.3. Another way is to encapsulate the callback in a class and tie the arguments to an instance of this class.

Attachments (4)

37698.diff (2.1 KB) - added by xknown 4 years ago.
37698.2.diff (3.1 KB) - added by xknown 4 years ago.
Move _wp_kses_split_callback to deprecated.php
37698.3.diff (3.3 KB) - added by xknown 4 years ago.
Add documentation for the related unit test
37698.4.diff (3.6 KB) - added by audrasjb 3 years ago.
In case we still want to close this ticket with 5.7, here is a patch refresh against trunk.

Download all attachments as: .zip

Change History (20)

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


4 years ago

#2 @markparnell
4 years ago

  • Keywords dev-feedback added

Given the minimum PHP version for WP is now 5.6 this might be easier to resolve since an anonymous function is now an option.

#3 follow-up: @whyisjake
4 years ago

@xknown Is this still relevant?

#4 in reply to: ↑ 3 @xknown
4 years ago

Replying to whyisjake:

@xknown Is this still relevant?

I'd say, yes. This bug can still be reproduced on the latest trunk revision. I'll upload the patch we are using in WP.com.

@xknown
4 years ago

#5 follow-up: @peterwilsoncc
4 years ago

As this will be the first closure in core, it's worth referring to the coding standards changes of July 2019.

With these points in mind, a conservative, but practical step is to allow closures as function callbacks, but not as hook callbacks in Core. Ultimately, we should be able to allow any sort of complex callback to be attached to hooks, but the Core APIs aren’t quite ready for it yet.

tl;dr: In this context a closure is dandy.

--

Plugin search results for _wp_kses_split_callback in the WordPress directory returns only false positives.

It would still be good to move the function to deprecated.php

@xknown
4 years ago

Move _wp_kses_split_callback to deprecated.php

#6 in reply to: ↑ 5 @xknown
4 years ago

Replying to peterwilsoncc:

As this will be the first closure in core, it's worth referring to the coding standards changes of July 2019.

With these points in mind, a conservative, but practical step is to allow closures as function callbacks, but not as hook callbacks in Core. Ultimately, we should be able to allow any sort of complex callback to be attached to hooks, but the Core APIs aren’t quite ready for it yet.

tl;dr: In this context a closure is dandy.

--

Plugin search results for _wp_kses_split_callback in the WordPress directory returns only false positives.

It would still be good to move the function to deprecated.php

attachment:37698.2.diff moves _wp_kses_split_callback to deprecated.php.

@xknown
4 years ago

Add documentation for the related unit test

#7 @desrosj
4 years ago

  • Component changed from General to Formatting
  • Keywords has-patch needs-testing added
  • Milestone set to 5.6

Readding a milestone. Let's try to wrap this up in 5.6.

#8 @davidbaumwald
3 years ago

  • Keywords needs-refresh added

Latest patch needs a refresh against trunk.

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


3 years ago

#10 @hellofromTonya
3 years ago

  • Keywords early added
  • Milestone changed from 5.6 to 5.7

We are late into the 5.6 beta cycle. From scrub today, Helen notes:

tl;dr: no chance that’s going into 5.6, that’s 1000000% an early thing

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

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


3 years ago

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


3 years ago

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


3 years ago

#14 @metalandcoffee
3 years ago

  • Keywords dev-feedback removed

@audrasjb
3 years ago

In case we still want to close this ticket with 5.7, here is a patch refresh against trunk.

#15 @audrasjb
3 years ago

  • Keywords needs-refresh removed

#16 @desrosj
3 years ago

  • Milestone changed from 5.7 to Future Release

I'm very hesitant to make this change so far into the cycle. I'm going to move this to Future Release as it's been punted in two previous releases so far. When a committer is willing to own this, it can be re-milestoned.

Note: See TracTickets for help on using tickets.