Skip to content

fix: RequestCookie behavior and getAll(name)#403

Open
JaredStowell wants to merge 2 commits intocloudflare:mainfrom
JaredStowell:jstowell/fix-request-cookies-parity
Open

fix: RequestCookie behavior and getAll(name)#403
JaredStowell wants to merge 2 commits intocloudflare:mainfrom
JaredStowell:jstowell/fix-request-cookies-parity

Conversation

@JaredStowell
Copy link
Contributor

Align vinext request-cookie behavior with the current Next.js runtime in both request-cookie implementations:

  • packages/vinext/src/shims/headers.ts
  • packages/vinext/src/shims/server.ts

What changed

Fix RequestCookies.getAll(name)

getAll(name) was ignoring its argument and returning the full cookie list.

Before, with:

cookie: a=1; a=2; b=3

vinext returned:

  • get('a') => { name: 'a', value: '2' }
  • getAll('a') => [{ name: 'a', value: '2' }, { name: 'b', value: '3' }]

Now it returns:

  • get('a') => { name: 'a', value: '2' }
  • getAll('a') => [{ name: 'a', value: '2' }]

This now works for both overloads:

  • getAll("a")
  • getAll({ name: "a" })

Missing names now correctly return [].

Align request cookie parsing with current Next.js runtime

While expanding coverage, I found both request-cookie parsers also diverged from current Next.js runtime behavior.

Both shims now match next@16.1.6 for:

  • duplicate names collapsing to the last value
  • percent-decoding cookie values
  • ignoring malformed percent-encoded values
  • treating bare cookie tokens as "true"

Examples:

  • token=abc%3D123 => abc=123
  • bad=%E0%A4%A => skipped
  • flag => { name: "flag", value: "true" }

Keep middleware cookie rewrites consistent

applyMiddlewareRequestHeaders() now rebuilds the cookie map using the same parser, so middleware-rewritten cookie headers behave the same as initially parsed request headers.

Tests

Added targeted regressions in tests/shims.test.ts covering:

  • getAll("name")
  • getAll({ name })
  • missing-name lookups
  • duplicate-name behavior
  • percent-decoding
  • malformed encoded values
  • bare token cookies
  • middleware cookie rewrites with duplicate names

@pkg-pr-new
Copy link

pkg-pr-new bot commented Mar 10, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@403

commit: 8571b3d

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 08d05caf64

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@james-elicx
Copy link
Collaborator

/bigbonk review

Copy link
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

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

Solid fix. The getAll(name) filtering bug was a clear correctness issue, and the cookie parsing alignment with @edge-runtime/cookies' parseCookie is well done. Tests are thorough and all 609 pass.

A few notes:

The Codex review comment about empty values is wrong. The "true" fallback only applies to bare tokens (no = sign). For empty=, pair.slice(splitAt + 1) yields "", and decodeURIComponent("") returns "" — not "true". The tests at lines 337 and 2513 correctly assert this.

Minor observations (non-blocking):

}
}
return cookies;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This is a character-for-character duplicate of the parseCookieHeader in headers.ts (only the local variable name differs: key vs name). Consider extracting it to a shared utility, e.g. shims/cookie-parse.ts, to avoid the two copies drifting apart over time. Not blocking.

if (!key) continue;

try {
cookies.set(key, decodeURIComponent(pair.slice(splitAt + 1).trim()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor: edge-runtime's parseCookie does not trim cookie values (pair.slice(splitAt + 1) without .trim()). The .trim() here is unlikely to matter in practice since RFC 6265 cookie-values shouldn't have leading/trailing whitespace, but noting the difference for completeness. Same applies in server.ts:286.


getAll(): CookieEntry[] {
return [...this._parse().entries()].map(([name, value]) => ({ name, value }));
getAll(nameOrOptions?: string | CookieEntry): CookieEntry[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pre-existing, but worth noting: _parse() re-creates the Map on every get(), getAll(), has(), and iterator call. If cookie access becomes hot in a request, consider caching the parsed result (with invalidation when headers change). Not a regression from this PR.

const headers = new Headers({ cookie: "a=1; a=2; token=abc=def" });
const cookies = new RequestCookies(headers);

expect(cookies.getAll({ name: "a", value: "ignored" })).toEqual([{ name: "a", value: "2" }]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice — this test case explicitly verifies that value in the { name, value } overload is ignored for filtering, which matches @edge-runtime/cookies behavior. Good catch.

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 10, 2026

Review submitted. Here's a summary of my findings:

Verdict: Approved. This is a clean, well-tested fix.

What the PR does right

  • Fixes the getAll(name) bug — it was ignoring the argument and returning all cookies
  • Aligns cookie parsing with @edge-runtime/cookies' parseCookie (bare tokens → "true", percent-decoding, malformed value skipping)
  • Tests are thorough: 197 new lines covering all the edge cases, all 609 tests pass
  • The "preserves explicit empty values" tests correctly assert empty="", not "true"

Key finding: the Codex review was wrong

The automated Codex review claimed that empty= would be coerced to "true". That's incorrect — the "true" fallback only triggers for bare tokens with no = sign. decodeURIComponent("") returns "" as expected.

Non-blocking observations in the review

  1. Code duplicationparseCookieHeader is copy-pasted between headers.ts and server.ts (differs only in a local variable name). Worth extracting to a shared module.
  2. Value trimming — vinext applies .trim() to cookie values; edge-runtime doesn't. Unlikely to matter in practice but noted for completeness.
  3. Re-parsing on every call (pre-existing) — server.ts's _parse() rebuilds the Map on every get()/getAll()/has() call. Not a regression from this PR.

github run

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.

2 participants