Broaden the fix to all control characters, not just NUL

protocol.txt's actual rule is "must not include control characters or
whitespace" -- \s alone missed NUL plus the rest of C0 (0x01-0x08,
0x0E-0x1F) and DEL (0x7F). Switches to \p{Cntrl} (equivalent to POSIX
[:cntrl:], but without the "duplicated range" warning Ruby raises when
\s and [:cntrl:] are combined in one class -- fatal under this suite's
-w run).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: petergoldstein

CHANGELOG.md

@@ -38,11 +38,11 @@ Other changes:
 
 Bug Fixes:
 
-- Base64-encode keys containing embedded NUL bytes (#1148)
-  - `KeyRegularizer.required?` decided whether a key needed base64 encoding using `/\s/`, which does not match NUL -- a key that was otherwise ASCII-only and contained no whitespace (e.g. `"foo\x00bar"`) went out on the wire unencoded
-  - Not a command-injection risk: the text protocol splits commands on CRLF, not NUL. The risk is key confusion -- anything downstream that treats the key as a C string (memcached itself, a proxy, logging) could silently truncate at the NUL and act on a different, shorter key than Dalli believes it sent
-  - A raw NUL byte in a key was never protocol-compliant in the first place: memcached's own spec (`protocol.txt`) states a key "must not include control characters or whitespace," and NUL is a control character. The only sanctioned way to carry binary content in a key is the meta protocol's base64 (`b` flag) path -- the one whitespace and non-ASCII keys already used, and the one NUL-containing keys now use too
-  - **Behavior change:** a key containing a NUL byte now produces different bytes on the wire (base64-encoded, per the meta protocol's `b` flag) than before. Existing cache entries stored under the old, unencoded form of such a key will read as a miss once every reader has upgraded. **During a rolling deploy, old and new Dalli versions disagree about which physical key a NUL-containing logical key maps to** -- not just a one-time cutover, but ongoing inconsistency between the old-version and new-version server pools for the duration of the rollout. Harmless for an ordinary cached value (worst case, extra cache misses); worth accounting for if such a key ever backs something stateful, like a lock or counter. Expected to be rare in practice: embedding a raw NUL byte in a cache key is unusual, and doing so was already outside what the protocol permits
+- Base64-encode keys containing control characters, not just NUL (#1148)
+  - `KeyRegularizer.required?` decided whether a key needed base64 encoding using `/\s/`, which matches most whitespace but none of the C0 control range (0x00-0x1F) or DEL (0x7F) -- a key that was otherwise ASCII-only and contained no whitespace (e.g. `"foo\x00bar"` or a key with an embedded ESC byte) went out on the wire unencoded
+  - Not a command-injection risk: the text protocol splits commands on CRLF, not other control bytes. The risk is key confusion -- anything downstream that treats one of these bytes specially (a C string terminating at NUL, a terminal or log line interpreting an escape byte) could silently act on a different key than Dalli believes it sent
+  - A raw control byte in a key was never protocol-compliant in the first place: memcached's own spec (`protocol.txt`) states a key "must not include control characters or whitespace." The only sanctioned way to carry such content in a key is the meta protocol's base64 (`b` flag) path -- the one whitespace and non-ASCII keys already used, and the one these keys now use too. The check is now `/[\s[:cntrl:]]/`, matching that rule directly rather than special-casing NUL
+  - **Behavior change:** a key containing a control character now produces different bytes on the wire (base64-encoded, per the meta protocol's `b` flag) than before. Existing cache entries stored under the old, unencoded form of such a key will read as a miss once every reader has upgraded. **During a rolling deploy, old and new Dalli versions disagree about which physical key such a logical key maps to** -- not just a one-time cutover, but ongoing inconsistency between the old-version and new-version server pools for the duration of the rollout. Harmless for an ordinary cached value (worst case, extra cache misses); worth accounting for if such a key ever backs something stateful, like a lock or counter. Expected to be rare in practice: embedding a raw control byte in a cache key is unusual, and doing so was already outside what the protocol permits
   - Found while auditing `request_formatter.rb` during the routing-token work in #1130 / #1147; unrelated to that change and predates it
 
 - Retry transient network errors in `get_multi`, `set_multi` and `delete_multi` instead of silently swallowing them (#1149)

lib/dalli/protocol/key_regularizer.rb

@@ -12,15 +12,25 @@ class Meta
       module KeyRegularizer
         module_function
 
-        # \s does not match NUL. A key with an embedded NUL is ASCII-only and
-        # has no whitespace, so it would otherwise be written to the wire
-        # unencoded -- not a protocol-injection risk (the text protocol splits
-        # on CRLF, not NUL), but a downstream consumer that treats the key as
-        # a C string (memcached itself, a proxy, logging) could silently
-        # truncate at the NUL and act on a different, shorter key than Dalli
-        # believes it sent.
+        # protocol.txt requires that a key "must not include control
+        # characters or whitespace" -- \p{Cntrl} is C0 (0x00-0x1F) plus DEL
+        # (0x7F). \s alone misses NUL and the rest of that range: a key
+        # containing one of those bytes but no whitespace is ASCII-only, so
+        # it would otherwise be written to the wire unencoded. Not a
+        # protocol-injection risk (the text protocol splits on CRLF, not
+        # other control bytes), but a downstream consumer that treats the key
+        # specially at one of those bytes (a C string terminating at NUL, a
+        # terminal or log line interpreting an escape byte) could silently
+        # act on a different key than Dalli believes it sent.
+        #
+        # Written as \p{Cntrl} rather than the POSIX [:cntrl:] bracket class:
+        # \s and [:cntrl:] overlap (tab, newline, CR are in both), and Ruby
+        # warns "character class has duplicated range" when they're combined
+        # in one -- fatal here, since this suite's -w run treats warnings as
+        # errors (see test_strict_warnings.rb). \p{Cntrl} matches the same
+        # bytes without the overlap warning.
         def required?(key)
-          !key.ascii_only? || /[\s\0]/.match?(key)
+          !key.ascii_only? || /[\p{Cntrl}\s]/.match?(key)
         end
 
         def encode(key)

test/integration/test_encoding.rb

@@ -25,11 +25,11 @@
         end
       end
 
-      # KeyRegularizer.required? previously missed embedded NUL bytes ('\s'
-      # doesn't match '\0'), so a key like this went on the wire unencoded --
-      # this proves it now round-trips through the base64 path instead, and
-      # (via the sibling key below) doesn't collide with a similar key that
-      # has no NUL.
+      # KeyRegularizer.required? previously missed embedded control bytes
+      # ('\s' doesn't match NUL or most of the rest of the C0 range), so a key
+      # like this went on the wire unencoded -- this proves it now round-trips
+      # through the base64 path instead, and (via the sibling key below)
+      # doesn't collide with a similar key that has no control byte.
       it 'supports keys with an embedded NUL byte, distinct from a similar key without one' do
         memcached_persistent(p) do |dc|
           nul_key = "foo\x00bar"
@@ -42,6 +42,19 @@
           assert_equal 'plain_value', dc.get(plain_key)
         end
       end
+
+      it 'supports keys with a non-NUL control byte (e.g. ESC), distinct from a similar key without one' do
+        memcached_persistent(p) do |dc|
+          esc_key = "foo\x1Bbar"
+          plain_key = 'foobar'
+
+          dc.set(esc_key, 'esc_value')
+          dc.set(plain_key, 'plain_value')
+
+          assert_equal 'esc_value', dc.get(esc_key)
+          assert_equal 'plain_value', dc.get(plain_key)
+        end
+      end
     end
   end
 end

test/protocol/test_key_regularizer.rb

@@ -52,14 +52,35 @@
       refute Dalli::Protocol::Meta::KeyRegularizer.required?(key)
     end
 
-    # \s does not match NUL, so an ASCII-only key with an embedded NUL and no
-    # other whitespace would otherwise slip past this check and reach the
-    # wire unencoded.
+    # \s does not match NUL or most other control bytes, so an ASCII-only key
+    # containing one but no whitespace would otherwise slip past this check
+    # and reach the wire unencoded -- protocol.txt requires that a key "must
+    # not include control characters or whitespace."
     it 'returns true for keys with embedded NUL bytes' do
       key = "foo\x00bar"
 
       assert Dalli::Protocol::Meta::KeyRegularizer.required?(key)
     end
+
+    it 'returns true for keys with a non-NUL control byte (e.g. ESC)' do
+      key = "foo\x1Bbar"
+
+      assert Dalli::Protocol::Meta::KeyRegularizer.required?(key)
+    end
+
+    it 'returns true for keys containing DEL (0x7F)' do
+      key = "foo\x7Fbar"
+
+      assert Dalli::Protocol::Meta::KeyRegularizer.required?(key)
+    end
+
+    it 'returns true for every C0 control byte and DEL' do
+      ((0x00..0x1F).to_a + [0x7F]).each do |byte|
+        key = "foo#{byte.chr}bar"
+
+        assert Dalli::Protocol::Meta::KeyRegularizer.required?(key), "byte 0x#{byte.to_s(16)} was not caught"
+      end
+    end
   end
 
   describe '.encode' do
@@ -133,5 +154,13 @@
 
       assert_equal original_key, decoded_key
     end
+
+    it 'encode then decode returns original key with a non-NUL control byte' do
+      original_key = "foo\x1Bbar"
+      encoded_key = Dalli::Protocol::Meta::KeyRegularizer.encode(original_key)
+      decoded_key = Dalli::Protocol::Meta::KeyRegularizer.decode(encoded_key)
+
+      assert_equal original_key, decoded_key
+    end
   end
 end

test/protocol/test_request_formatter.rb

@@ -557,12 +557,20 @@ def assert_encoded(key)
       assert_encoded 'user:🎉:profile'
     end
 
-    # \s does not match NUL, so an ASCII-only key with an embedded NUL and no
-    # other whitespace previously reached the wire unencoded.
+    # \s does not match NUL or most other control bytes, so an ASCII-only key
+    # containing one but no whitespace previously reached the wire unencoded.
     it 'returns base64 encoded key for keys with an embedded NUL byte' do
       assert_encoded "foo\x00bar"
     end
 
+    it 'returns base64 encoded key for keys with a non-NUL control byte (e.g. ESC)' do
+      assert_encoded "foo\x1Bbar"
+    end
+
+    it 'returns base64 encoded key for keys containing DEL (0x7F)' do
+      assert_encoded "foo\x7Fbar"
+    end
+
     it 'handles empty keys' do
       assert_raw ''
     end