Check cache key type

[tillkruess]

It's unfortunately relatively common for plugins to call the wp_cache_*() methods with an empty-string or false as cache key, usually as a result of not checking the return value of another function that's used as the key.

Currently this is failing silently and thus leads on odd behavior at best and often breakage.

$sessionId = session_id(); // equals "" when session wasn't started
wp_cache_get( $sessionId );

Doing a quick type check on the given cache keys and calling _doing_it_wrong() would help plugins developers to notice these issues quicker.

Trac ticket: https://core.trac.wordpress.org/ticket/56198

[malthert]

#56000 was marked as a duplicate.

[malthert]

+1 as I outlined in the ticket I opened a few weeks ago, those are the bain of my existence, since even plugins with millions of users have this issue

[SergeyBiryukov]

Thanks for the patch! I have a few notes:

• It looks like the conditional is reversed in individual methods, causing unit tests to fail:

  if ( $this->_valid_key( $key ) ) {
  	return false;
  }
  

  This should be:

  if ( ! $this->_valid_key( $key ) ) {
  	return false;
  }
  

• The 'empty string' part should be translatable too, but it's not. I think the notice could be split into two for clarity:
  • For empty strings: Cache key must not be empty.
  • For other types: Cache key must be integer or non-empty string, %s given.
• A translator comment is needed for the string, something like:

  /* translators: %s: The type of the given cache key. */
  

• We don't use debug_backtrace() like that anywhere else in core, I think for performance reasons. We might need to pass the caller method name to ::_valid_key() instead, or move the message to the calling methods.

[malthert]

Cache key must not be empty.

I think this is confusing, because 0 and '0' are valid cache keys but technically (in PHP terms) 'empty'.
I think the single error string that is in the PR now is better.

===

Regarding translation:
Afaik l10n.php might not be loaded yet when this function is called at all. Since the object cache is loaded before l10n.php.
I think a function_exists() check for all translations functions in class-wp-object-cache.php is needed.

===

Regarding debug_backtrace:
adding a limit of 2 and removing args will take care of the performance issues:
debug_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS, 2 )[1]['function'];
This is sufficiently fast (e-6 seconds).

The only other option would be to pass __FUNCTION__ as a 2nd param to _valid_key, which I think is dumb.

===

if ( is_string( $key ) && trim( $key ) !== '' ) {

I think trimming is a good idea to disallow space only keys.
However I would do the same for groups then too, to keep things consistent.

if ( empty( $group ) ) { => if ( empty( trim( $group ) ) ) {

[SergeyBiryukov]

Replying to malthert:

Cache key must not be empty.

I think this is confusing, because 0 and '0' are valid cache keys but technically (in PHP terms) 'empty'.

Good point, I would revise the suggestion then:

• For empty strings: Cache key must not be an empty string.
• For other types: Cache key must be integer or non-empty string, %s given.

I think the single error string that is in the PR now is better.

It may be if we find a way to improve the wording, but we should not insert English phrases like 'empty string' into a translatable string, it results in a mix of languages that is neither English nor properly translated.

Regarding translation:
Afaik l10n.php might not be loaded yet when this function is called at all. Since the object cache is loaded before l10n.php.
I think a function_exists() check for all translations functions in class-wp-object-cache.php is needed.

Good point too! Translation functions might indeed not be included if WordPress is loaded in SHORTINIT mode. We could do a function_exists( '__' ) check similar to the one found in the _doing_it_wrong() function itself:

if ( function_exists( '__' ) ) {
	$message = sprintf( __( 'Cache key must be integer or non-empty string, %s given.' ), $type );
} else {
	$message = sprintf( 'Cache key must be integer or non-empty string, %s given.', $type );
}

Another option might be to skip translation altogether.

[tillkruess]

@SergeyBiryukov @malthert: Thanks for the suggestions, I've implemented them all and tried to make the code as readable as possible given the nesting possibilities.

I've kept the debug_backtrace() call for now, because passing in __METHOD__ like @malthert says seems dumb.

@spacedmonkey asked about floats on GitHub, should we simply disallow them, or cast them to strings (seems risky to me)?

[tillkruess]

5 tests were failing because they are using non-empty strings as cache keys.

This was an issue with update_user_caches() and clean_user_cache(), I've fixed this in ​https://github.com/WordPress/wordpress-develop/pull/2967/commits/c8f38c97b1f88263bf7249ad2d3bdabac18d60e7

This is likely causing issues right now and should be fixed in the next patch release.

1) Tests_User::test_wp_insert_user_with_meta
Unexpected incorrect usage notice for add.
Cache key must not be an empty string. (This message was added in version 6.1.0.)

2) Tests_User_GetUserCount::test_get_user_count_on_different_network
Unexpected incorrect usage notice for add.
Cache key must not be an empty string. (This message was added in version 6.1.0.)

3) Tests_User_GetUserCount::test_enable_live_network_user_counts_filter
Unexpected incorrect usage notice for add.
Cache key must not be an empty string. (This message was added in version 6.1.0.)


4) Tests_User_GetUserCount::test_get_user_count_update_on_delete_multisite
Unexpected incorrect usage notice for add.
Cache key must not be an empty string. (This message was added in version 6.1.0.)

5) Tests_User_Slashes::test_wp_insert_user
Unexpected incorrect usage notice for add.
Cache key must not be an empty string. (This message was added in version 6.1.0.)

[tillkruess]

@SergeyBiryukov: An alternative to debug_backtrace() could be appending wp_debug_backtrace_summary() to the doing it wrong message. it would also make tracing these errors much easier. Thoughts?

[malthert]

I'm against floats, as this will just introduce all kinds of typecasting issues with external caches then.

[malthert]

What about the trimming of empty groups?

[tillkruess]

I'm against floats, as this will just introduce all kinds of typecasting issues with external caches then.

Agreed.

What about the trimming of empty groups?

We can safeguard against that as well in the patch, if desired. I seem to recall that empty(trim()) doesn't work on PHP 5.6 and it may need a dedicated variable, which would mean more code.

[malthert]

Replying to tillkruess:

What about the trimming of empty groups?

We can safeguard against that as well in the patch, if desired. I seem to recall that empty(trim()) doesn't work on PHP 5.6 and it may need a dedicated variable, which would mean more code.

I tested with PHP 5.6.40 and could not reproduce such an issue. There is no extra variable needed: (same result for PHP 5.6, 7.4 and 8.1)

<?php
$group = '   ';
if ( empty( trim( $group ) ) ) {
        echo "empty";
}

[tillkruess]

Replying to malthert:

I tested with PHP 5.6.40 and could not reproduce such an issue. There is no extra variable needed: (same result for PHP 5.6, 7.4 and 8.1)

I could swear this used to be a thing.

Anyhow, would it not make sense to check the group similar to is_valid_key(), since "0" is technically a valid group, but fail with empty(trim('0'))?

[malthert]

Definitely, is_valid_group() would replace the empty() for fallback to default group - with no error message for groups?

[tillkruess]

Replying to malthert:

Definitely, is_valid_group() would replace the empty() for fallback to default group - with no error message for groups?

I'd also suggest a _doing_it_wrong() as well, any objections?

I've spent some time taking a closer look at the update_user_caches()/clean_user_cache() changes.

Some observations:

• It is not possible to have an empty user_login or user_nicename:
  • If user_login is empty, the user is never saved to the database.
  • If user_nicename is empty, it is created from `user_login`.
• So we only really need to check if user_email is not empty. I considered creating a ticket to disallow an empty email in the same way as an empty user_login is disallowed, but it looks like creating a user with an empty email should actually be possible, see #14834.
• 0 or '0' would not be a valid email, so I think a if ( ! empty( $user->user_email ) ) check would suffice here.
• We have some tests where the email is passed to wp_insert_user() incorrectly or is accidentally reset, might as well fix that.

With that in mind, I've added a few commits to:

• Simplify the checks in update_user_caches()/clean_user_cache().
• Correct some tests where the email is not passed as expected. This is not strictly required here, just some minor cleanup.

I think this looks good. Any feedback on these latest changes appreciated 🙂

I think this looks good. Any feedback on these latest changes appreciated 🙂

LGTM ✌️

[SergeyBiryukov]

In 53818:

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.

Thanks for the PR! Merged in r53818.

[SergeyBiryukov]

In 53821:

Tests: Add a unit test for WP_Object_Cache::is_valid_key().

A valid cache key for wp_cache_*() functions must be either an integer number or a non-empty string.

Follow-up to [53818].

See #56198.

[SergeyBiryukov]

In 53822:

Tests: Add a test case with a float value for WP_Object_Cache::is_valid_key().

A valid cache key for wp_cache_*() functions must be either an integer number or a non-empty string. To avoid potential compatibility issues or key collisions, float values should not be considered a valid cache key.

Follow-up to [53818], [53821].

Props tillkruess, malthert, spacedmonkey.
See #56198.

Follow-up to #2967 and a164d642759a6673ad832cdc3330856bb4084be8.

Trac ticket: https://core.trac.wordpress.org/ticket/56198

[SergeyBiryukov]

In 53834:

Tests: Add more test cases for WP_Object_Cache::is_valid_key().

A valid cache key for wp_cache_*() functions must be either an integer number or a non-empty string. To avoid potential compatibility issues or key collisions, a string that becomes empty after trim() should not be considered a valid cache key.

Follow-up to [53818], [53821], [53822].

Props tillkruess.
See #56198.

Thanks for the PR! Merged in r53834.

[SergeyBiryukov]

In 53835:

Tests: Use named data provider for WP_Object_Cache::is_valid_key() test.

This makes the output when using the --testdox option more descriptive and is helpful when trying to debug which data set from a data provider failed the test.

Follow-up to [53818], [53821], [53822], [53834].

See #56198.

[desrosj]

There are a lot of caching API related changes in 6.1 and a dev note should be written collecting them all. Marking this for inclusion.