fix(keys): encode names and values that could be read as key structure by derrickmehaffy · Pull Request #251 · strapi-community/plugin-rest-cache

derrickmehaffy · 2026-10-13T01:28:19Z

What changed and why

The failure mode

A cache key is built as

${path}?${querySuffix}&${headersSuffix}&${authSuffix}

where querySuffix and headersSuffix are name=value pairs joined with ,. Nothing inside a name or a value was escaped, so one request could spell out another request's key. In every case below, whichever request arrives first is stored and then served to the other until maxAge. Under the default hitpass, any anonymous caller can do this.

A query value forging a second parameter.

GET /api/categories/slug/news?locale=x,populate=articles   -> ...?locale=x,populate=articles&&
GET /api/categories/slug/news?locale=x&populate=articles   -> ...?locale=x,populate=articles&&

The first request has one parameter and populates nothing. Its response poisons the second URL.

A string forging a nested value. Nested query values go into the key as JSON.stringify output, so a string that looks like that JSON collides with it:

?q={"a":"b"} collides with ?q[a]=b.
?q=["1","2"] collides with ?q=1&q=2.

A bare parameter colliding with the string "null". Strapi's strapi::query middleware parses with qs and strictNullHandling: true, so a bare ?q arrives as null. typeof null is "object", which sent it down the JSON branch and out as q=null. That is the key of the literal ?q=null. Verified end to end: ?q cached, then ?q=null was a HIT serving {"q":null}.

A header value forging a second header. With useHeaders: ["x-a", "x-b"], a request sending x-a: fr,x-b=1 collides with one sending x-a: fr and x-b: 1.

An & moving the boundary between the query and header parts. ?q=1%26x-a%3D2 with x-a: 3, and ?q=1 with x-a: 2&x-a=3, both key as ...?q=1&x-a=2&x-a=3&up:public.

Auth was not spoofable. The auth component is always last, and a caller's own identity is always appended after anything they control. After this change it no longer depends on that ordering argument either: the two & around the header part are now the only bare & in any key.

The fix

New server/src/utils/keys/escapeKeyPart.ts, used by generateQueryParamsKey and generateHeadersKey. The rule is all-or-nothing per string:

A string goes into the key unchanged unless it contains %, & or = (or ,, for a name), or starts with {, [ or ". Otherwise the whole string goes through encodeURIComponent.

Nested values are still JSON.stringify'd, as before. The rule is applied first to every string inside them, keys included.

A top-level null (a bare ?q) keys as a lone % (KEY_NULL). It is tested before the JSON branch.

This gives four invariants. They are written in the file's header comment, and each can be checked by reading the rule:

A name never contains %, &, , or =.
A raw value never contains %, and an encoded value always does, so the two can never be equal. encodeURIComponent is injective. The one exception is KEY_NULL, a lone %. No raw value holds a %, and in an encoded value every % starts a %XX escape, so KEY_NULL equals neither.
A value never contains & or =. Together with (1):
- every = in a part ends a name;
- each name starts after the last , before its =;
- so a part splits into pairs exactly one way;
- and every & in a key is a separator.
A raw scalar never starts with {, [ or ". Nested JSON always starts with { or [, so a scalar and a nested value can never be equal. Inside the JSON every string obeys (2) and (3), and JSON adds no =, & or % of its own. A nested null is the bare word null while a nested string is quoted, so {"a":null} and {"a":"null"} stay distinct.

What qs can produce. With Strapi's options, qs yields only strings, arrays, objects and null. I checked this directly against qs 6.16 with strictNullHandling: true: ?q=true, ?q=1, ?q=[] and ?q={} are all strings, and ?q, ?q[], ?q[a] and ?q&q=b give null, [null], {"a":null} and [null,"b"]. So a string that reads as a JSON literal ("true", "1", "null") can never meet a non-string top-level value of the same spelling. The only such value was top-level null, and it now has its own form. Strings starting with [ or { are encoded under (4).

encodeURIComponent throws on a lone surrogate. Lone surrogates are replaced before encoding, so a request cannot turn that into a 500. In practice none arrive, because qs leaves undecodable input as a literal %XX.

Breaking change? Which keys move

The rule in AGENTS.md is that keys must not move for existing configurations. Values keep their commas. That is safe because of invariant (3), and it means comma lists are untouched. These keys are byte-identical to main, and the spec pins each one:

/api/articles?filters={"title":{"$ne":"a,b"}},populate=category,author,sort=title:asc,id&&
/api/articles?populate={"author":"true"}&&
/api/categories/slug/news?locale=en,populate=articles&&
/api/categories?&accept-encoding=gzip, deflate, br&up:public
/api/categories/probe/vary?q=null&&up:public
/api/categories/probe/vary?q=true&&up:public
/api/categories/probe/vary?q=1&&up:public
/api/categories/probe/vary?q=&&up:public

The exact rule for which keys move. A key moves when any of these is true:

a query value or useHeaders value contains %, & or =;
a query value or useHeaders value starts with {, [ or ";
a string or object key nested inside a query parameter (for example in filters[...]) meets either of the two conditions above;
a parameter name would be escaped on the same terms, or contains ,;
the request has a bare parameter (?q), which moves from q=null to q=%.

That is wider than "q-weighted headers". Real query values hit it too:

a padded base64 cursor, cursor=eyJpZCI6MX0=;
50%;
Tom & Jerry;
[draft];
a value that starts with ";
a URL value carrying its own =.

On the header side, a q-weighted Accept-Language: en-US,en;q=0.9 now keys as en-US%2Cen%3Bq%3D0.9. No header is part of a key by default (useHeaders: []).

For the null collision I gave null a distinct encoding rather than force-encoding the string "null". That keeps the string rule uniform: a string is raw unless it carries structure. It moves one rare key, a bare parameter's. ?q=null keeps its key.

Each moved value costs one cold entry after the upgrade. The old entries are not orphaned: purge patterns are anchored on the path (^/api/...\?), so they still match and are purged as before, and otherwise they expire at maxAge.

Keeping = raw would require the context-sensitive comma escape this PR first shipped with. That escape fixed the separator collisions, but its proof of uniqueness was much harder to check, and it still left the string-vs-JSON collisions open. For a security fix, I think the simpler invariant is worth the cold entries.

Fixtures

The tests use one new fixture route, /api/categories/probe/vary. It is keyed on useQueryParams: ["q"], useHeaders: ["x-a", "x-b"] and useAuth: true. Its handler echoes all three, so a response served under the wrong key shows up in the body. It is public-readable, like the other category probes.

Before / after

pnpm run test:e2e:memory -- cache-key-separators

Separator and JSON collisions, before (main's generateQueryParamsKey / generateHeadersKey, same spec and fixtures as the first two commits):

      ✓ baseline: the uncached populated response includes articles
      ✕ does not let a comma-bearing value collide with a second parameter
      ✕ does not let a JSON-looking string collide with a nested object
      ✕ does not let a JSON-looking string collide with an array
      ✓ does not let a literal escape collide with an escaped separator
      ✕ does not let a comma-bearing header value collide with a second header
      ✕ does not let an '&' in a query value shift the header part of the key
      ✕ still purges an entry whose key carries escapes
      ✓ is unchanged for comma lists, sort and a nested filter
      ✓ is unchanged for a nested populate
      ✓ is unchanged for a custom route with an allow-list
      ✓ is unchanged for a header value with commas and spaces
      ✕ encodes a value containing '='
Tests:       7 failed, 6 passed, 13 total

All five collision tests are red on main. The last two failures are the expected key-format change. The "literal escape" test cannot fail on main, because main does no escaping. It guards the encoding itself, so I made it fail on purpose: with % dropped from the value rule, it goes red (Expected: "MISS", Received: "HIT").

null collision, before (this branch without the KEY_NULL change, new tests added):

      ✕ does not let a bare parameter collide with the string 'null'
      ✕ does not let the string 'null' collide with a bare parameter
      ✓ does not let a nested null collide with a nested 'null'
      ✓ does not let a JSON-looking string collide with an array holding null
      ...
      ✕ keys a bare parameter apart from every string
    Expected: "MISS"
    Received: "HIT"
    -   "/api/categories/probe/vary?q=%&&up:public",
    +   "/api/categories/probe/vary?q=null&&up:public",
Tests:       3 failed, 19 passed, 22 total

The two guards already pass without the fix, so a green run proves nothing about them. I made each fail on purpose:

with escapeKeyJson turning a nested null into "null", the nested-null test goes red (HIT);
with [ dropped from the leading-character rule, the [null] test goes red (HIT).

After:

      ✓ baseline: the uncached populated response includes articles
      ✓ does not let a comma-bearing value collide with a second parameter
      ✓ does not let a JSON-looking string collide with a nested object
      ✓ does not let a JSON-looking string collide with an array
      ✓ does not let a literal escape collide with an escaped separator
      ✓ does not let a bare parameter collide with the string 'null'
      ✓ does not let the string 'null' collide with a bare parameter
      ✓ does not let a nested null collide with a nested 'null'
      ✓ does not let a JSON-looking string collide with an array holding null
      ✓ does not let a comma-bearing header value collide with a second header
      ✓ does not let an '&' in a query value shift the header part of the key
      ✓ still purges an entry whose key carries escapes
      ✓ is unchanged for comma lists, sort and a nested filter
      ✓ is unchanged for a nested populate
      ✓ is unchanged for a custom route with an allow-list
      ✓ is unchanged for a header value with commas and spaces
      ✓ is unchanged for the string 'null'
      ✓ is unchanged for the string 'true'
      ✓ is unchanged for a number
      ✓ is unchanged for an empty value
      ✓ keys a bare parameter apart from every string
      ✓ encodes a value containing '='
Tests:       22 passed, 22 total

Full memory suite: Test Suites: 24 passed, 24 total / Tests: 157 passed, 157 total.

Checklist

Tests added or updated. For a bug fix: a spec in shared/tests/ that fails before the change.
pnpm run test:e2e:memory passes.
pnpm run test:lint passes.
Touches admin/? Not touched.
Touches a provider or the published package shape? No provider change. pnpm run test:esm passes.
Any duration I touched is milliseconds. None touched.
User-visible change? docs/guide/caching/keys.md documents the encoding rule, the bare-parameter change, and exactly which keys move.
Conventional commit subjects, branched from main.

Notes for the reviewer

Four commits. The second replaces the first commit's context-sensitive comma escape with the all-or-nothing rule. The third keys a bare parameter apart from "null". The fourth corrects the docs on which keys move. Squash on merge.
Redis not run. test:e2e:redis was not run. Key generation is provider-independent, and the redis provider only prefixes keys.
Local test environment. On this branch's lockfile every spec fails at boot with strapi.ai.admin.isEnabled is not a function, from a duplicated Strapi copy. I ran the specs against fix(deps): keep a single Strapi in the workspace #260's lockfile locally. That lockfile is not part of this PR.

A cache key joins query params and headers as `name=value` pairs with "," and the parts with "&", and escaped none of those inside a value. So `?locale=x,populate=articles` keyed exactly like `?locale=x&populate=articles`, and an anonymous caller who sent the first decided what the second URL served until maxAge. A header value such as `fr,x-b=1` could pose as a second configured header the same way, and an "&" in a query value or header could move the boundary between parts. Escaping everything would move the key of nearly every real request - commas are ordinary in populate and sort lists and in Accept-Encoding, "=" in a q-weighted Accept-Language - which would orphan every existing entry and start every route cold after an upgrade. So only what is ambiguous is escaped: "%" and "&" always, a name character outside the token set, and a "," in a value only where it is followed by a name and "=". That is enough for a key to decode one way only, and an ordinary value comes out byte-identical; the new spec pins exact keys for such requests.

The first version escaped a "," only where it was followed by a name and "=". That is correct, but its uniqueness argument was subtle enough that a gap would be a poisoning hole, and it left a JSON-looking string keyed like the object or array qs builds from bracket syntax: `?q={"a":"b"}` and `?q[a]=b`, `?q=["1","2"]` and `?q=1&q=2`, shared a key. Replace it with a rule that is easy to check. A string goes in unchanged unless it contains "%", "&" or "=" (or "," for a name) or starts with "{", "[" or '"'; otherwise the whole string goes through encodeURIComponent. Values then never contain "=" or "&", names never contain "," or "=", and raw, encoded and JSON values are told apart by "%" and by the leading bracket - four invariants, written down in escapeKeyPart.ts. Values keep their commas, which is what keeps `populate=a,b`, `sort=title:asc,id` and `Accept-Encoding: gzip, deflate` byte-identical to before. A value containing "=" does get a new key, which in practice means a q-weighted header listed in useHeaders - no header is keyed by default - and costs one cold entry: old entries are still purged, since purge patterns are anchored on the path.

Strapi's query middleware parses with qs and strictNullHandling, so a bare `?q` arrives as null. typeof null is "object", which sent it down the JSON branch and out as `q=null` - the key of the literal string `?q=null`. Whichever arrived first was served to the other. A top-level null now keys as a lone "%". No raw value contains "%", and in an encoded value every "%" starts a %XX escape, so nothing else can produce it and the invariants in escapeKeyPart.ts still hold. Giving null its own form, rather than force-encoding the string "null", keeps the string rule uniform: a string is raw unless it carries structure. The one key that moves is a bare parameter's (`q=null` to `q=%`); `?q=null` keeps its key. qs yields only strings, arrays, objects and null, so strings such as "true", "1" or "null" cannot meet a non-string top-level value. A null nested in an object or array stays JSON `null` against a quoted "null". Both are now pinned. The two guards that already passed (nested null, and `?q[]` against the string "[null]") were each seen failing under a deliberate mutation before being trusted.

The guide said the only realistic moved keys were q-weighted headers. Query values move too, and they are common enough to matter: a padded base64 cursor, a percentage, "Tom & Jerry", "[draft]", a URL with its own query string. Readers planning an upgrade need the rule itself, not one example of it, plus the bare-parameter change and the reason the old entries are still purged.

Keeps both category probe handlers and their public permissions: echo (purge-pattern fixtures) and vary (#251's key-separator fixtures).

derrickmehaffy added 2 commits October 12, 2026 18:27

derrickmehaffy changed the title ~~fix(keys): escape separators so one request cannot take another's key~~ fix(keys): encode names and values that could be read as key structure Oct 13, 2026

derrickmehaffy mentioned this pull request Oct 13, 2026

fix(recv): do not cache Buffer bodies, which replay as JSON #256

Merged

7 of 8 tasks

derrickmehaffy added 3 commits October 12, 2026 20:49

Merge branch 'main' into fix/cache-key-separators

6ebbddf

derrickmehaffy merged commit 06e792a into main Oct 13, 2026
13 checks passed

derrickmehaffy deleted the fix/cache-key-separators branch October 13, 2026 04:38

github-actions Bot mentioned this pull request Oct 13, 2026

chore: release 5.1.1 #235

Open

derrickmehaffy added a commit that referenced this pull request Oct 13, 2026

Merge origin/main into fix/purge-route-case

d2bf8b4

Keeps both category probe handlers and their public permissions: echo (purge-pattern fixtures) and vary (#251's key-separator fixtures).

Navigation Menu

Repository navigation

fix(keys): encode names and values that could be read as key structure - #251

fix(keys): encode names and values that could be read as key structure#251
derrickmehaffy merged 5 commits into
mainfrom
fix/cache-key-separators

derrickmehaffy commented Oct 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

derrickmehaffy commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed and why

The failure mode

The fix

Breaking change? Which keys move

Fixtures

Before / after

Checklist

Notes for the reviewer

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

derrickmehaffy commented Oct 13, 2026 •

edited

Loading