#54160 closed defect (bug) (fixed)
sanitize_key() / _wp_customize_include() is not able to handle non-scalar values
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.9 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Formatting | Keywords: | has-patch has-unit-tests commit |
Focuses: | Cc: |
Description (last modified by )
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)
Change History (32)
#4
in reply to:
↑ 3
@
4 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' );
)
#5
@
4 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.
This ticket was mentioned in PR #1982 on WordPress/wordpress-develop by costdev.
3 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.
3 years ago
This ticket was mentioned in PR #1984 on WordPress/wordpress-develop by costdev.
3 years ago
#9
Trac ticket:
#10
@
3 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.
hellofromtonya commented on PR #1982:
3 years ago
#12
Committed via https://core.trac.wordpress.org/changeset/52292.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
3 years ago
#14
@
3 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
@
3 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 = ''; }
#17
@
3 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
@
3 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.
3 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
@
3 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
@
3 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:
↓ 23
@
3 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:
↓ 24
@
3 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
@
3 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, aE_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
@
3 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.
3 years ago
#26
- Keywords has-patch has-unit-tests added; needs-patch removed
Trac ticket: https://core.trac.wordpress.org/ticket/54160
#27
follow-up:
↓ 28
@
3 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
@
3 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 ofis_string()
. Thoughsanitize_type()
is for only string keys,strtolower()
handles non-string scalar types without throwing an error or deprecation notice.
Thanks so much.
hellofromtonya commented on PR #2042:
3 years ago
#30
Committed via https://core.trac.wordpress.org/changeset/52370.
@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!).