Make WordPress Core

Opened 22 months ago

Closed 21 months ago

Last modified 19 months ago

#56198 closed enhancement (fixed)

Check cache key type

Reported by: tillkruess's profile tillkruess Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version: 6.1
Component: Cache API Keywords: has-patch has-unit-tests needs-dev-note
Focuses: performance Cc:

Description

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.

Change History (30)

This ticket was mentioned in PR #2967 on WordPress/wordpress-develop by tillkruss.


22 months ago
#1

  • Keywords has-patch added

#2 @malthert
22 months ago

#56000 was marked as a duplicate.

#3 @malthert
22 months ago

+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

#4 @SergeyBiryukov
22 months ago

  • Milestone changed from Awaiting Review to 6.1

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.

#5 follow-up: @malthert
22 months ago

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 ) ) ) {

Last edited 22 months ago by malthert (previous) (diff)

#6 in reply to: ↑ 5 @SergeyBiryukov
22 months ago

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.

#7 @TweetyThierry
22 months ago

  • Focuses performance added

#8 @tillkruess
22 months ago

@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)?

Last edited 22 months ago by tillkruess (previous) (diff)

#9 @tillkruess
22 months ago

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.)
Last edited 22 months ago by tillkruess (previous) (diff)

#10 @tillkruess
22 months ago

@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?

#11 @malthert
22 months ago

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

#12 @malthert
22 months ago

What about the trimming of empty groups?

#13 follow-up: @tillkruess
22 months ago

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.

#14 in reply to: ↑ 13 ; follow-up: @malthert
22 months ago

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";
}

#15 in reply to: ↑ 14 ; follow-up: @tillkruess
21 months ago

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'))?

#16 in reply to: ↑ 15 ; follow-up: @malthert
21 months ago

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

#17 in reply to: ↑ 16 @tillkruess
21 months ago

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?

SergeyBiryukov commented on PR #2967:


21 months ago
#18

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:
  • 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 🙂

tillkruss commented on PR #2967:


21 months ago
#19

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

LGTM ✌️

#20 @SergeyBiryukov
21 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

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.

SergeyBiryukov commented on PR #2967:


21 months ago
#21

Thanks for the PR! Merged in r53818.

#22 @SergeyBiryukov
21 months ago

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.

#23 @SergeyBiryukov
21 months ago

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.

This ticket was mentioned in PR #3063 on WordPress/wordpress-develop by tillkruss.


21 months ago
#24

  • Keywords has-unit-tests added

Follow-up to #2967 and a164d642759a6673ad832cdc3330856bb4084be8.

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

#25 @SergeyBiryukov
21 months ago

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.

SergeyBiryukov commented on PR #3063:


21 months ago
#26

Thanks for the PR! Merged in r53834.

#27 @SergeyBiryukov
21 months ago

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.

#28 @milana_cap
20 months ago

  • Keywords add-to-field-guide added

#29 @desrosj
19 months ago

  • Keywords needs-dev-note added

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.

#30 @milana_cap
19 months ago

  • Keywords add-to-field-guide removed
Note: See TracTickets for help on using tickets.