WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 2 months ago

#54160 new defect (bug)

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

Reported by: dd32 Owned by:
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch
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 2 months ago.
54160.2.diff (2.3 KB) - added by dd32 2 months ago.

Download all attachments as: .zip

Change History (7)

@dd32
2 months ago

#1 @dd32
2 months ago

  • Description modified (diff)

#2 @SergeyBiryukov
2 months ago

  • Milestone changed from Awaiting Review to 5.9

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

#5 @dd32
2 months 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.

Note: See TracTickets for help on using tickets.