sanitize_key() / _wp_customize_include() is not able to handle non-scalar values

[dd32]

On WordPress.org it's common to see PHP Warnings such as the following:

E_WARNING: strtolower() expects parameter 1 to be string, array given in wp-includes/formatting.php:2140

This is ultimately being triggered by a request similar to ​https://example.org/?customize_changeset_uuid[]=junk

This query variable is not intended on containing an array, and the above warning is triggered by _wp_customize_include() calling sanitize_key( array( ... ) ).

Either _wp_customize_include() should validate the input, or sanitize_key() should validate the input to the function. Normally I would lean towards the former, but in this case I think it might be better for the latter for where it's called in other contexts.

[jrf]

@dd32 This is a known issue which is also causing PHP 8.1 deprecation notices when null would be passed.

@hellofromTonya and me recently discussed the notorious lack of input validation in WordPress during our livestream: ​https://www.youtube.com/watch?v=rrU15JwjBZ8

Would be great to have you join this conversation to get to a point where we can architect a more structural solution for all such issue in WP (and there are many!).

[dd32]

Replying to jrf:

Would be great to have you join this conversation to get to a point where we can architect a more structural solution for all such issue in WP (and there are many!).

Oh I know there are a lot of cases :) #17737 has caused me to effectively ​add a "firewall" plugin to WordPress.org to limit the amount of notices/warnings we get from vulnerability scanners.

This ticket is just yet another cause of the same thing - core code that doesn't sanitize that a value is remotely acceptable before using it, and that's just in Core code, not even mentioning plugins.

I don't know what the ideal solution is here, but there's probably something in #18322 or #22325 (eg WP::GET( 'customize_changeset_uuid', 'string' ) (WP::GET( $var, $expected_type = 'any' );)

[dd32]

After thinking about a few ideas, I still think 54160.diff​ should be added, but in addition, 54160.2.diff​ which applies the stricter sanitization should be added.

Trac ticket:

Trac ticket:

[hellofromTonya]

@costdev added unit tests and improvements to 54160.diff​ in ​PR 1982. Marking the PR for commit.

Unfortunately, ran out of time to test and add tests for 54160.2.diff​. Will need to move that portion of the work to 6.0.

[hellofromTonya]

In 52292:

Formatting: Handle non-scalar types passed to sanitize_key().

sanitize_key() expects a string type for the given key. Passing any other data type to strtolower() can result in E_WARNING: strtolower() expects parameter 1 to be string, array given.

A check is added that if the key is not a string, the key is set to an empty string. For performance, the additional string processing is skipped if the key is an empty string.

This change maintains backwards-compatibility for valid string keys while fixing the bug of non-string keys.

Props costdev, dd32.
Fixes #54160.

Committed via https://core.trac.wordpress.org/changeset/52292.

[hellofromTonya]

Reopening to evaluate the _wp_customize_include().

[wppunk]

Unfortunately, but the 5.9-beta2 broken the next code:

sanitize_key( 1 ); // The result was '1' in 5.8
sanitize_key( 1 ); // The result is '' in 5.9

Probably, we should change the code:

if ( ! is_string( $key ) ) {
	$key = '';
}

on:

if ( is_array( $key ) || is_object( $key ) ) {
	$key = '';
}

[hellofromTonya]

@wppunk As documented, sanitize_key() expects the key given to be a string data type. The data type check is part of the input validation to ensure the incoming value is of string before passing it to native PHP functions that expect and require a string value.

Question: Can you share the use case where an integer value needs to be sanitized?

[wppunk]

@hellofromTonya I used to in my code dynamic attribute that sometimes can be a digit and sometimes a string. An example:

foreach( $choices as $key => $value ) {
	$field_name = sprintf( 'field[%s]', sanitize_key( $key ) );
	printf(
		'<input name="%s" value="1"/>'
		esc_attr( $field_name )
	);
}

And the $choices variable can be as:

$choices = [
	esc_html__( 'Red', 'great-example' ),
	esc_html__( 'Blue', 'great-example' ),
	esc_html__( 'Green', 'great-example' ),
];

Here the $key will be 0, 1, or 2.

And as:

$choices = [
	'red'   => esc_html__( 'Red', 'great-example' ),
	'blue'  => esc_html__( 'Blue', 'great-example' ),
	'green' => esc_html__( 'Green', 'great-example' ),
];

And here, the $key will be red, blue, or green.

I can use the (string) $key in my code, but what about other plugins and themes?

I would recommend due to backward compatibility update PHPDoc for the $key variable:

 * @param int|float|string $key String key`

I fixed the sanitize_key function due to backward compatibility with oldest WordPress versions.

Trac ticket: [](https://core.trac.wordpress.org/ticket/54160)

[hellofromTonya]

@wppunk Thank you for providing a sample of the code. An integer, i.e. the index of an array, does not need to be sanitized.

Here's an example of how to adjust your dynamic code ​https://3v4l.org/9sgAf.

As I previously noted, sanitize_key() expects a string key. See the documentation here ​https://developer.wordpress.org/reference/functions/sanitize_key/. Notice that it says "Sanitizes a string key."

Why did it work in 5.8, but doesn't now?

Improvements have been made in 5.9 to validate the values given to functions to ensure they are of the documented data type. Passing anything other than a string value is considered a bug, i.e. doing it wrong.

[hellofromTonya]

Moving this back to 6.0.

Why? Passing anything other than a string key is doing it wrong. There's not a backwards-compatibility break. Rather, the improvement in 5.9 fixed a bug of allowing non-strings to pass through it.

[wppunk]

@hellofromTonya It shouldn't work in this way if this code worked for the int, float, bool types. That means the documentation was incorrect.

If you want to change this behavior and support only string values:

• Firstly, show a deprecation message and support undocumented variables.
• A few releases later, it is possible to remove support undocumented types.

Why do I insist on this? Because this bug is floating, it is pretty challenging to find this floating bug. For example, in my code, all checkboxes are deleted after saving. Let's come up with a blueprint for a smooth deprecation process.

[hellofromTonya]

Replying to wppunk:

It shouldn't work in this way if this code worked for the int, float, bool types. That means the documentation was incorrect.

The function is clear that sanitizes a string key. The documentation is not incorrect. The usage is incorrect.

I appreciate that it adds work on your end to adjust your code to only pass a string key to it.

Because this bug is floating, it is pretty challenging to find this floating bug.

If the data type is scalar but not a string, then you're right, it does pass through without an error; else, a PHP Warning is thrown for Warning: strtolower() expects parameter 1 to be string. In this case, a E_USER_NOTICE could be triggered to alert of an incorrect data type passed to it.

[wppunk]

The function is clear that sanitizes a string key. The documentation is not incorrect. The usage is incorrect.

Not all WordPress developers will check PHPDoc before usage. To understand that the function works only with strings, the function name isn't enough.

If the data type is scalar but not a string, then you're right, it does pass through without an error; else, a PHP Warning is thrown for Warning: strtolower() expects parameter 1 to be string. In this case, a E_USER_NOTICE could be triggered to alert of an incorrect data type passed to it.

It worked for scalar and not string types without any messages.

I have the following logic:

• It worked with numbers a few years.
• PHP isn't a strict typing language (at least PHP5.6 which WordPress supports).

To wrap up. I'm sure that a deprecation message should be added for scalar but not string types. In such a way, the strict typing implementation will be smooth.

[hellofromTonya]

Capturing notes:

PHP is getting more and more strict with documented data types being passed into native PHP functions where deprecation notices are thrown in PHP 8.1, but will be fatal errors in PHP 9. However, currently strtolower() still handles scalars, though it's documented as only accepting strings. It's probable that a PHP 8.x version will add a deprecation to fix the bug internal to strtolower(). This is not the case today or in WordPress 5.9.

A choice needs to be made. Deprecate non-strings scalars now, or allow them until PHP deprecates it in strtolower().

sanitize_key() is clearly identified to only work with string keys and has been since it was introduced. However, it had a bug in it that allowed non-strings to pass through. Non-scalars would throw a PHP Warning, whereas a non-string scalar would be handled by strtolower() and converted into a string.

What input validation and error messaging should happen here?

I agree with @wppunk that silently passing it through makes it difficult to debug.

I'm also leaning towards allowing non-string scalars. If in the future PHP deprecates passing non-strings to strtolower(), then go further with the input validation. For non-scalars, a E_USER_WARNING needs to be thrown to avoid silently passing an issue through the function.

Pulling this back into 5.9 to make adjustments. Patch with tests are coming.

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

[hellofromTonya]

Marking ​PR 2042 for commit.

Props to @wppunk for pointing out the impact of silently setting the key to an empty string for non-string scalar types. This PR switches to use is_scalar() instead of is_string(). Though sanitize_type() is for only string keys, strtolower() handles non-string scalar types without throwing an error or deprecation notice.

[wppunk]

Replying to hellofromTonya:

Marking ​PR 2042 for commit.

Props to @wppunk for pointing out the impact of silently setting the key to an empty string for non-string scalar types. This PR switches to use is_scalar() instead of is_string(). Though sanitize_type() is for only string keys, strtolower() handles non-string scalar types without throwing an error or deprecation notice.

Thanks so much.

[hellofromTonya]

In 52370:

Formatting: Use is_scalar() in sanitize_key().

This is a follow-up to [52292] which introduced is_string() to check the given key is a string to be sanitized, else the key is set to an empty string.

sanitize_key() is clearly identified (in the documentation) to only work with string keys. However, it had a bug in it that allowed non-strings to pass through it:

• A non-scalar "key" would throw a PHP Warning (which was resolved in [52292].
• A non-string scalar "key" was handled by the PHP native strtolower() which converted it into a string.

While is_string() is valid, non-string scalar types passed as the key to be sanitized were being set to an empty string. Given that strtolower() handles these without error or deprecation as of PHP 8.1, is_scalar() protects the website from issues while retaining the past behavior of converting integer keys (for example) into a string.

Changes include:

• Using is_scalar() instead of is_string()
• Refactor for readability and less code
• More tests

Please note, this does not change the behavior of the function, nor redefine it to now accept non-string scalars.

References:

• ​https://developer.wordpress.org/reference/functions/sanitize_key/
• ​https://www.php.net/manual/en/function.strtolower.php

Follow-up [52292].

Props wppunk, hellofromTonya, costdev, jrf.
Fixes #54160.

Committed via https://core.trac.wordpress.org/changeset/52370.