Skip to content

[PLTFRM-2461] Hash memcached keys containing whitespace, control chars, or keys exceeding byte length restrictions.#181

Open
brettshumaker wants to merge 6 commits into
Automattic:masterfrom
brettshumaker:fix/hash-keys-with-invalid-characters
Open

[PLTFRM-2461] Hash memcached keys containing whitespace, control chars, or keys exceeding byte length restrictions.#181
brettshumaker wants to merge 6 commits into
Automattic:masterfrom
brettshumaker:fix/hash-keys-with-invalid-characters

Conversation

@brettshumaker

Copy link
Copy Markdown

Summary

Memcached doesn't allow whitespace or control characters in keys and caps key length at 250 bytes. WP_Object_Cache::key() handled this by stripping whitespace from the generated key:

return preg_replace( '/\s+/', '', "$prefix:$group:$key" );

Stripping characters doesn't address control characters or the length limit at all. This PR makes key generation handle all three constraints without altering distinct keys.

Changes

  • Assemble the key as prefix:group:key, where prefix is internally generated (salt, flush number, blog/global prefix) and only the group and key name come from the caller.
  • If the caller-supplied portion contains whitespace or control characters (\x00-\x1f\x7f), or the full key exceeds 250 bytes, hash that portion (h:md5(...)) so the result is always memcached-safe.
  • The readable, namespaced prefix is preserved, and the group is included in the hashed portion, so keys that don't require hashing are unchanged and remain uniquely scoped.

Tests

  • Added unit/integration coverage for the new key-generation behavior in tests/test-object-cache.php.
  • Run via bin/test.sh (Docker WordPress test runner with memcached); all tests pass.

Comment thread object-cache.php
@rebeccahum rebeccahum changed the title Hash memcached keys containing whitespace, control chars, or keys exceeding byte length restrictions. [PLTFRM-2461] Hash memcached keys containing whitespace, control chars, or keys exceeding byte length restrictions. Jun 10, 2026
@rebeccahum rebeccahum requested a review from Copilot June 10, 2026 14:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates WP_Object_Cache::key() to generate Memcached-safe cache keys without collapsing distinct caller keys, addressing Memcached’s restrictions on whitespace/control characters and the 250-byte key length limit.

Changes:

  • Reworks key assembly to preserve a readable, namespaced prefix and hash only the caller-influenced “tail” (group:key) when needed.
  • Adds hashing behavior for keys containing whitespace/control characters or exceeding Memcached’s 250-byte limit.
  • Adds test coverage to ensure whitespace, long keys, and hashed-key namespacing/collision behavior round-trip correctly.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
object-cache.php Updates key() generation to hash unsafe/overlength key tails while preserving prefix readability.
tests/test-object-cache.php Adds tests for whitespace/long/control-character keys and for hashed vs unhashed key() behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread object-cache.php
Comment thread tests/test-object-cache.php
brettshumaker and others added 2 commits June 10, 2026 10:51
…full key

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@brettshumaker brettshumaker requested a review from rebeccahum June 10, 2026 14:55
Comment thread object-cache.php Outdated
… always forcing those keys down the hashed path.
@brettshumaker brettshumaker requested a review from rebeccahum June 10, 2026 15:25
Comment thread object-cache.php Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment thread object-cache.php
Comment thread object-cache.php Outdated
Comment thread tests/test-object-cache.php
Comment thread object-cache.php
Comment thread object-cache.php Outdated
// the prefix always contains a ':'. A colon-free hash segment therefore
// cannot be produced by the $prefix:$group:$key shape, which keeps the
// hashed and unhashed key namespaces provably disjoint.
if ( strlen( $full_key ) > 250 || preg_match( '/[\s\x00-\x1f\x7f]/', $full_key ) ) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, can we address these separately rather than mixing the two concerns? I’d prefer this PR stay focused on invalid characters + whitespace collisions. Then we avoid handling both tail hashing and whole-key hashing at once. The limit we're not worried about as much b/c of automatic truncations.

Comment thread object-cache.php Outdated

// If the prefix itself makes the key invalid/too long, hash everything.
if ( strlen( $key ) > 250 || preg_match( '/[\s\x00-\x1f\x7f]/', $key ) ) {
$key = 'h' . md5( $full_key );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this now lacks the $prefix this means that it's possible for multiple WP instances to have conflicting cache keys.

This however seems unlikely/impossible to be hit, as if $prefix . ':h' . md5( $tail ); is longer than 250char limit, or $prefix contains non-safe characters, then the configuration (WP_CACHE_KEY_SALT) is invalid at present (and arguably, should remain so) as $prefix should be known to be safe due to being generated by the class itself.

@dd32 dd32 Jun 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally; WP_CACHE_KEY_SALT is not properly documented in the dropin, as it suggests:

define( 'WP_CACHE_KEY_SALT', '...long random string...' );

where we should probably suggest something like:

define( 'WP_CACHE_KEY_SALT', 'ultra-random-unique-string' );
// Note: Memcache has a 250char key length limit, this salt should be as short as possible
//   while remaining unique and non-determinable between sites on a shared memcache server.
//   We recommend at 12-24 characters to allow for most WordPress Multisite cache keys.

@brettshumaker brettshumaker requested a review from rebeccahum June 12, 2026 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants