Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#54160 closed defect (bug) (fixed)

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

Reported by: dd32's profile dd32 Owned by: hellofromtonya's profile hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description (last modified by 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.

Attachments (2)

54160.diff (1.1 KB) - added by dd32 3 years ago.
54160.2.diff (2.3 KB) - added by dd32 2 years ago.

Download all attachments as: .zip

Change History (32)

@dd32
3 years ago

#1 @dd32
3 years ago

  • Description modified (diff)

#2 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.9

#3 follow-up: @jrf
3 years ago

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

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

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
2 years ago

#5 @dd32
2 years ago

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.

#6 @hellofromTonya
2 years ago

  • Keywords needs-unit-tests added

This ticket was mentioned in PR #1982 on WordPress/wordpress-develop by costdev.


2 years ago
#7

  • Keywords has-unit-tests added; needs-unit-tests removed

Trac ticket:

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


2 years ago

This ticket was mentioned in PR #1984 on WordPress/wordpress-develop by costdev.


2 years ago
#9

Trac ticket:

#10 @hellofromTonya
2 years ago

  • Keywords commit added
  • Owner set to hellofromTonya
  • Status changed from new to reviewing

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

#11 @hellofromTonya
2 years ago

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

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.

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


2 years ago

#14 @hellofromTonya
2 years ago

  • Keywords needs-unit-tests added; has-unit-tests commit removed
  • Milestone changed from 5.9 to 6.0
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to evaluate the _wp_customize_include().

#15 @wppunk
2 years ago

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 = '';
}

#16 @ocean90
2 years ago

  • Milestone changed from 6.0 to 5.9

#17 @hellofromTonya
2 years ago

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

#18 @wppunk
2 years ago

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

This ticket was mentioned in PR #2040 on WordPress/wordpress-develop by wppunk.


2 years ago
#19

  • Keywords has-unit-tests added; needs-unit-tests removed

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

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

#20 @hellofromTonya
2 years ago

  • Keywords needs-unit-tests added; has-unit-tests removed

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

#21 @hellofromTonya
2 years ago

  • Milestone changed from 5.9 to 6.0

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.

#22 follow-up: @wppunk
2 years ago

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

#23 in reply to: ↑ 22 ; follow-up: @hellofromTonya
2 years ago

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.

#24 in reply to: ↑ 23 @wppunk
2 years ago

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.

#25 @hellofromTonya
2 years ago

  • Keywords needs-patch added; has-patch needs-unit-tests removed
  • Milestone changed from 6.0 to 5.9

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.

This ticket was mentioned in PR #2042 on WordPress/wordpress-develop by hellofromtonya.


2 years ago
#26

  • Keywords has-patch has-unit-tests added; needs-patch removed

#27 follow-up: @hellofromTonya
2 years ago

  • Keywords commit added

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.

#28 in reply to: ↑ 27 @wppunk
2 years ago

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.

#29 @hellofromTonya
2 years ago

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

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:

Follow-up [52292].

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

Note: See TracTickets for help on using tickets.