From d3a851d0d1a58709edbdde59b977986ce7523548 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Tue, 14 Dec 2021 14:59:33 +0000 Subject: [PATCH] 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. git-svn-id: https://develop.svn.wordpress.org/trunk@52370 602fd350-edb4-49c9-b593-d223f7449a82 --- src/wp-includes/formatting.php | 22 ++-- .../phpunit/tests/formatting/sanitizeKey.php | 104 +++++++++++++----- 2 files changed, 86 insertions(+), 40 deletions(-) diff --git a/src/wp-includes/formatting.php b/src/wp-includes/formatting.php index 9c49b97b56..905b07b598 100644 --- a/src/wp-includes/formatting.php +++ b/src/wp-includes/formatting.php @@ -2132,19 +2132,15 @@ function sanitize_user( $username, $strict = false ) { * * @since 3.0.0 * - * @param string $key String key - * @return string Sanitized key + * @param string $key String key. + * @return string Sanitized key. */ function sanitize_key( $key ) { - $raw_key = $key; + $sanitized_key = ''; - if ( ! is_string( $key ) ) { - $key = ''; - } - - if ( '' !== $key ) { - $key = strtolower( $key ); - $key = preg_replace( '/[^a-z0-9_\-]/', '', $key ); + if ( is_scalar( $key ) ) { + $sanitized_key = strtolower( $key ); + $sanitized_key = preg_replace( '/[^a-z0-9_\-]/', '', $sanitized_key ); } /** @@ -2152,10 +2148,10 @@ function sanitize_key( $key ) { * * @since 3.0.0 * - * @param string $key Sanitized key. - * @param string $raw_key The key prior to sanitization. + * @param string $sanitized_key Sanitized key. + * @param string $key The key prior to sanitization. */ - return apply_filters( 'sanitize_key', $key, $raw_key ); + return apply_filters( 'sanitize_key', $sanitized_key, $key ); } /** diff --git a/tests/phpunit/tests/formatting/sanitizeKey.php b/tests/phpunit/tests/formatting/sanitizeKey.php index 7dea43e004..0fb5d905df 100644 --- a/tests/phpunit/tests/formatting/sanitizeKey.php +++ b/tests/phpunit/tests/formatting/sanitizeKey.php @@ -7,38 +7,27 @@ class Tests_Formatting_SanitizeKey extends WP_UnitTestCase { /** - * @ticket 54160 + * @ticket 54160 * @dataProvider data_sanitize_key * - * @param mixed $key The key to sanitize. - * @param mixed $expected The expected value. + * @param string $key The key to sanitize. + * @param string $expected The expected value. */ public function test_sanitize_key( $key, $expected ) { $this->assertSame( $expected, sanitize_key( $key ) ); } + /** + * Data provider. + * + * @return array + */ public function data_sanitize_key() { return array( 'an empty string key' => array( 'key' => '', 'expected' => '', ), - 'a null key' => array( - 'key' => null, - 'expected' => '', - ), - 'an int 0 key' => array( - 'key' => 0, - 'expected' => '', - ), - 'a true key' => array( - 'key' => true, - 'expected' => '', - ), - 'an array key' => array( - 'key' => array( 'Howdy, admin!' ), - 'expected' => '', - ), 'a lowercase key with commas' => array( 'key' => 'howdy,admin', 'expected' => 'howdyadmin', @@ -71,20 +60,81 @@ class Tests_Formatting_SanitizeKey extends WP_UnitTestCase { } /** - * @ticket 54160 + * @ticket 54160 + * @dataProvider data_sanitize_key_nonstring_scalar * - * @covers WP_Hook::sanitize_key + * @param mixed $key The key to sanitize. + * @param string $expected The expected value. */ - public function test_filter_sanitize_key() { - $key = 'Howdy, admin!'; + public function test_sanitize_key_nonstring_scalar( $key, $expected ) { + $this->assertSame( $expected, sanitize_key( $key ) ); + } + /** + * Data provider. + * + * @return array + */ + public function data_sanitize_key_nonstring_scalar() { + return array( + 'integer type' => array( + 'key' => 0, + 'expected' => '0', + ), + 'boolean true' => array( + 'key' => true, + 'expected' => '1', + ), + 'boolean false' => array( + 'key' => false, + 'expected' => '', + ), + 'float type' => array( + 'key' => 0.123, + 'expected' => '0123', + ), + ); + } + + /** + * @ticket 54160 + * @dataProvider data_sanitize_key_with_non_scalars + * + * @param mixed $nonscalar_key A non-scalar data type given as a key. + */ + public function test_sanitize_key_with_non_scalars( $nonscalar_key ) { add_filter( 'sanitize_key', - static function( $key ) { - return 'Howdy, admin!'; - } + function ( $sanitized_key, $key ) use ( $nonscalar_key ) { + $this->assertEmpty( $sanitized_key, 'Empty string not passed as first filtered argument' ); + $this->assertSame( $nonscalar_key, $key, 'Given unsanitized key not passed as second filtered argument' ); + return $sanitized_key; + }, + 10, + 2 ); + $this->assertEmpty( sanitize_key( $nonscalar_key ), 'Non-scalar key did not return empty string' ); + } - $this->assertSame( $key, sanitize_key( $key ) ); + /** + * Data provider. + * + * @return array + */ + public function data_sanitize_key_with_non_scalars() { + return array( + 'array type' => array( + 'key' => array( 'key' ), + 'expected' => '', + ), + 'null' => array( + 'key' => null, + 'expected' => '', + ), + 'object' => array( + 'key' => new stdClass(), + 'expected' => '', + ), + ); } }