wp_kses_split global variable pollution

[xknown]

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.

[markparnell]

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.

[whyisjake]

@xknown Is this still relevant?

[xknown]

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.

[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

[xknown]

Move _wp_kses_split_callback to deprecated.php

[xknown]

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]

Add documentation for the related unit test

[desrosj]

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

[davidbaumwald]

Latest patch needs a refresh against trunk.

[hellofromTonya]

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.

[audrasjb]

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

[desrosj]

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.

Refreshing again 37698.4.diff for testing
Tests below

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

[SirLouen]

Test Report

Description

🟠 This report validates that the indicated patch works as expected.

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/9108.diff

Environment

• WordPress: 6.9-alpha-60093-src
• PHP: 8.2.28
• Server: nginx/1.29.0
• Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
• Browser: Chrome 137.0.0.0
• OS: Windows 10/11
• Theme: Twenty Fifteen 4.0
• MU Plugins: None activated
• Plugins:
  • Classic Editor 1.6.7
  • Hello Dolly 9.7.2
  • Test Reports 1.2.0

Testing Instructions

• Using the info provided in OP, place the code anywhere where it can be executed

Actual Results

1. 🟠 Issue resolved with patch with some caveats (check notes)

Additional Notes

• This patch basically takes advantage of use in lambda functions to avoid having to edit global variables which seem to be very unreliable for this task, deprecating the callback function, that happened to use these globals.

• ⚠️ The problem is that I'm not 100% convinced about how this patch is working. I'm not liking the fact that this assert alone:

public function test_wp_kses_split_global_pollution() {
	$result_inner = '';
	$func         = function ( $attributes ) use ( &$result_inner ) {
		$result_inner = wp_kses_split( '<img src=x style="color: red;" >', array( 'img' => array( 'src' => array() ) ), array() ); 
         	return $attributes;
	};
	add_filter( 'safe_style_css', $func );
	$this->assertEquals( '<img src="x">', $result_inner );
}

Fails, and I'm not sure why.

But this alone

public function test_wp_kses_split_global_pollution() {
	$expected = "<a style='color: red'>I link this</a>";
	$result   = wp_kses_split( "<a style='color: red;'>I link this</a>", array( 'a' => array( 'style' => array() ) ), array( 'http' ) );
	$this->assertEquals( $expected, $result );
}

Passes well as expected

A more detailed explanation would do good for continuing with this report.

cc @xknown