From ad93e6387c0c854119ce09cc2ebf0b86fde3cc94 Mon Sep 17 00:00:00 2001 From: Sergey Biryukov Date: Wed, 3 Aug 2022 14:34:58 +0000 Subject: [PATCH] Cache API: Validate cache key in `WP_Object_Cache` methods. Some plugins may call the `wp_cache_*()` functions with an empty string, `false`, or `null` as cache key, usually as a result of not checking the return value of another function that's used as the key. Previously, this was silently failing, leading to odd behavior at best and often breakage due to key collisions. A valid cache key must be either an integer number or a non-empty string. This commit introduces a quick type check on the given cache keys and adds a `_doing_it_wrong()` message that should help plugin developers to notice these issues quicker. Includes: * A check in `update_user_caches()` and `clean_user_cache()` to make sure user email is not empty before being cached or removed from cache, as it is technically possible to create a user with empty email via `wp_insert_user()`. * Some minor cleanup in unit tests where the email was passed to `wp_insert_user()` incorrectly or was unintentionally reset. Props tillkruess, malthert, dd32, spacedmonkey, flixos90, peterwilsoncc, SergeyBiryukov. Fixes #56198. git-svn-id: https://develop.svn.wordpress.org/trunk@53818 602fd350-edb4-49c9-b593-d223f7449a82 --- src/wp-includes/class-wp-object-cache.php | 68 ++++++++++++++++++++++- src/wp-includes/user.php | 10 +++- tests/phpunit/tests/user.php | 1 + tests/phpunit/tests/user/getUserCount.php | 6 +- tests/phpunit/tests/user/slashes.php | 4 +- 5 files changed, 81 insertions(+), 8 deletions(-) diff --git a/src/wp-includes/class-wp-object-cache.php b/src/wp-includes/class-wp-object-cache.php index b0c1cc36f5..c22fd6298d 100644 --- a/src/wp-includes/class-wp-object-cache.php +++ b/src/wp-includes/class-wp-object-cache.php @@ -129,6 +129,43 @@ class WP_Object_Cache { unset( $this->$name ); } + /** + * Serves as a utility function to determine whether a key is valid. + * + * @since 6.1.0 + * + * @param int|string $key Cache key to check for validity. + * @return bool Whether the key is valid. + */ + protected function is_valid_key( $key ) { + if ( is_int( $key ) ) { + return true; + } + + if ( is_string( $key ) && trim( $key ) !== '' ) { + return true; + } + + $type = gettype( $key ); + + if ( ! function_exists( '__' ) ) { + wp_load_translations_early(); + } + + $message = is_string( $key ) + ? __( 'Cache key must not be an empty string.' ) + /* translators: %s: The type of the given cache key. */ + : sprintf( __( 'Cache key must be integer or non-empty string, %s given.' ), $type ); + + _doing_it_wrong( + sprintf( '%s::%s', __CLASS__, debug_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS, 2 )[1]['function'] ), + $message, + '6.1.0' + ); + + return false; + } + /** * Serves as a utility function to determine whether a key exists in the cache. * @@ -163,6 +200,10 @@ class WP_Object_Cache { return false; } + if ( ! $this->is_valid_key( $key ) ) { + return false; + } + if ( empty( $group ) ) { $group = 'default'; } @@ -216,6 +257,10 @@ class WP_Object_Cache { * @return bool True if contents were replaced, false if original value does not exist. */ public function replace( $key, $data, $group = 'default', $expire = 0 ) { + if ( ! $this->is_valid_key( $key ) ) { + return false; + } + if ( empty( $group ) ) { $group = 'default'; } @@ -245,14 +290,19 @@ class WP_Object_Cache { * more for cache plugins which use files. * * @since 2.0.0 + * @since 6.1.0 Returns false if cache key is invalid. * * @param int|string $key What to call the contents in the cache. * @param mixed $data The contents to store in the cache. * @param string $group Optional. Where to group the cache contents. Default 'default'. * @param int $expire Optional. Not used. - * @return true Always returns true. + * @return bool True if contents were set, false if key is invalid. */ public function set( $key, $data, $group = 'default', $expire = 0 ) { + if ( ! $this->is_valid_key( $key ) ) { + return false; + } + if ( empty( $group ) ) { $group = 'default'; } @@ -310,6 +360,10 @@ class WP_Object_Cache { * @return mixed|false The cache contents on success, false on failure to retrieve contents. */ public function get( $key, $group = 'default', $force = false, &$found = null ) { + if ( ! $this->is_valid_key( $key ) ) { + return false; + } + if ( empty( $group ) ) { $group = 'default'; } @@ -368,6 +422,10 @@ class WP_Object_Cache { * @return bool True on success, false if the contents were not deleted. */ public function delete( $key, $group = 'default', $deprecated = false ) { + if ( ! $this->is_valid_key( $key ) ) { + return false; + } + if ( empty( $group ) ) { $group = 'default'; } @@ -416,6 +474,10 @@ class WP_Object_Cache { * @return int|false The item's new value on success, false on failure. */ public function incr( $key, $offset = 1, $group = 'default' ) { + if ( ! $this->is_valid_key( $key ) ) { + return false; + } + if ( empty( $group ) ) { $group = 'default'; } @@ -455,6 +517,10 @@ class WP_Object_Cache { * @return int|false The item's new value on success, false on failure. */ public function decr( $key, $offset = 1, $group = 'default' ) { + if ( ! $this->is_valid_key( $key ) ) { + return false; + } + if ( empty( $group ) ) { $group = 'default'; } diff --git a/src/wp-includes/user.php b/src/wp-includes/user.php index 3c0f917bbc..416db2b86f 100644 --- a/src/wp-includes/user.php +++ b/src/wp-includes/user.php @@ -1850,8 +1850,11 @@ function update_user_caches( $user ) { wp_cache_add( $user->ID, $user, 'users' ); wp_cache_add( $user->user_login, $user->ID, 'userlogins' ); - wp_cache_add( $user->user_email, $user->ID, 'useremail' ); wp_cache_add( $user->user_nicename, $user->ID, 'userslugs' ); + + if ( ! empty( $user->user_email ) ) { + wp_cache_add( $user->user_email, $user->ID, 'useremail' ); + } } /** @@ -1878,9 +1881,12 @@ function clean_user_cache( $user ) { wp_cache_delete( $user->ID, 'users' ); wp_cache_delete( $user->user_login, 'userlogins' ); - wp_cache_delete( $user->user_email, 'useremail' ); wp_cache_delete( $user->user_nicename, 'userslugs' ); + if ( ! empty( $user->user_email ) ) { + wp_cache_delete( $user->user_email, 'useremail' ); + } + /** * Fires immediately after the given user's cache is cleaned. * diff --git a/tests/phpunit/tests/user.php b/tests/phpunit/tests/user.php index 8a3c59eeb4..60c388b71c 100644 --- a/tests/phpunit/tests/user.php +++ b/tests/phpunit/tests/user.php @@ -2010,6 +2010,7 @@ class Tests_User extends WP_UnitTestCase { $update_data = array( 'ID' => $create_user, 'user_login' => 'test_user', + 'user_email' => 'user@example.com', 'meta_input' => array( 'test_meta_key' => 'test_meta_updated', 'custom_meta' => 'updated_value', diff --git a/tests/phpunit/tests/user/getUserCount.php b/tests/phpunit/tests/user/getUserCount.php index 2ccfea78c9..f6a94d5624 100644 --- a/tests/phpunit/tests/user/getUserCount.php +++ b/tests/phpunit/tests/user/getUserCount.php @@ -45,7 +45,7 @@ class Tests_User_GetUserCount extends WP_UnitTestCase { $current_network_user_count = get_user_count(); // Add another user to fake the network user count to be different. - wpmu_create_user( 'user', 'pass', 'email' ); + wpmu_create_user( 'user', 'pass', 'user@example.com' ); wp_update_network_user_counts( $different_network_id ); @@ -68,7 +68,7 @@ class Tests_User_GetUserCount extends WP_UnitTestCase { wp_update_network_counts(); $start_count = get_user_count(); - wpmu_create_user( 'user', 'pass', 'email' ); + wpmu_create_user( 'user', 'pass', 'user@example.com' ); // No change, cache not refreshed. $count = get_user_count(); @@ -132,7 +132,7 @@ class Tests_User_GetUserCount extends WP_UnitTestCase { wp_update_user_counts(); $current_network_user_count = get_user_count(); - $u1 = wpmu_create_user( 'user', 'pass', 'email' ); + $u1 = wpmu_create_user( 'user', 'pass', 'user@example.com' ); $user_count = get_user_count(); diff --git a/tests/phpunit/tests/user/slashes.php b/tests/phpunit/tests/user/slashes.php index a45f7cfbf9..40eb2b176b 100644 --- a/tests/phpunit/tests/user/slashes.php +++ b/tests/phpunit/tests/user/slashes.php @@ -148,7 +148,7 @@ class Tests_User_Slashes extends WP_UnitTestCase { array( 'user_login' => 'slash_example_user_3', 'role' => 'subscriber', - 'email' => 'user3@example.com', + 'user_email' => 'user3@example.com', 'first_name' => self::SLASH_1, 'last_name' => self::SLASH_3, 'nickname' => self::SLASH_5, @@ -169,7 +169,7 @@ class Tests_User_Slashes extends WP_UnitTestCase { array( 'user_login' => 'slash_example_user_4', 'role' => 'subscriber', - 'email' => 'user3@example.com', + 'user_email' => 'user4@example.com', 'first_name' => self::SLASH_2, 'last_name' => self::SLASH_4, 'nickname' => self::SLASH_6,