WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#37698 new defect (bug)

wp_kses_split global variable pollution

Reported by: xknown Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords: dev-feedback
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 (3)

37698.diff (2.1 KB) - added by xknown 3 months ago.
37698.2.diff (3.1 KB) - added by xknown 3 months ago.
Move _wp_kses_split_callback to deprecated.php
37698.3.diff (3.3 KB) - added by xknown 3 months ago.
Add documentation for the related unit test

Download all attachments as: .zip

Change History (9)

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


3 months ago

#2 @markparnell
3 months 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
3 months ago

@xknown Is this still relevant?

#4 in reply to: ↑ 3 @xknown
3 months 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
3 months ago

#5 follow-up: @peterwilsoncc
3 months 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
3 months ago

Move _wp_kses_split_callback to deprecated.php

#6 in reply to: ↑ 5 @xknown
3 months 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
3 months ago

Add documentation for the related unit test

Note: See TracTickets for help on using tickets.