fix: ResponseCookies deduplicate Set-Cookie headers and add missing API surface#761
Conversation
…PI surface
ResponseCookies.set() unconditionally appended Set-Cookie headers, causing
duplicate headers when the same cookie name was set twice and get() returning
stale values. Rewritten to use an internal Map<name, {serialized, entry}>
as the single source of truth, matching @edge-runtime/cookies.
Also adds missing Next.js API surface:
- set() object form: set({ name, value, httpOnly, ... })
- get() object form: get({ name })
- getAll(name?) / getAll({ name }) filtering
- delete({ name, path, domain }) object form with path/domain propagation
- Constructor hydrates from existing Set-Cookie headers
- delete() now uses Expires=epoch (matching edge-runtime) instead of Max-Age=0
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a2338ff9c
ℹ️ 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".
| const entries: CookieEntry[] = []; | ||
| for (const header of this._headers.getSetCookie()) { | ||
| // Hydrate internal map from any existing Set-Cookie headers | ||
| for (const header of headers.getSetCookie()) { |
There was a problem hiding this comment.
ResponseCookies now calls headers.getSetCookie() unconditionally in its constructor, and NextResponse always constructs ResponseCookies, so environments where Headers does not implement getSetCookie will now throw during new NextResponse(...) even if no cookie API is used. This is a regression from the previous behavior where only cookie-access paths required that method; use a guarded fallback (getSetCookie?.() / get('set-cookie')) to avoid breaking basic response creation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The old code already called headers.getSetCookie() unconditionally in get(), getAll(), and [Symbol.iterator]() — so this isn't a regression, it's just eagerness (constructor vs first access). getSetCookie() is also used unconditionally throughout the codebase (worker-utils.ts:57, prod-server.ts:173), and vinext targets Node 18+ / Cloudflare Workers which both support it. Adding a silent fallback would mask a broken environment instead of failing fast.
packages/vinext/src/shims/server.ts
Outdated
| ): this { | ||
| const [name, opts] = | ||
| typeof args[0] === "string" ? [args[0], undefined] : [args[0].name, args[0]]; | ||
| return this.set({ name, value: "", expires: new Date(0), ...opts }); |
There was a problem hiding this comment.
delete() currently builds the cookie as { name, value: "", expires: new Date(0), ...opts }, which lets user-provided properties in opts override value/expires. In JavaScript (or TypeScript with widened variables), passing an object that contains expires can produce a non-expired Set-Cookie, so the delete operation silently fails. Spread opts first (or only forward path/domain) so tombstone semantics are always enforced.
Useful? React with 👍 / 👎.
Explicitly extract path and domain instead of spreading the full opts object, preventing callers from accidentally overriding the tombstone expires/value fields. Matches @edge-runtime/cookies behavior.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| delete( | ||
| ...args: | ||
| | [name: string] | ||
| | [options: Omit<CookieOptions & { name: string }, "maxAge" | "expires">] |
There was a problem hiding this comment.
The Omit type removes maxAge and expires (good), but still admits httpOnly, secure, and sameSite at the type level. The runtime on lines 641–642 silently drops them. This is a parity gap with @edge-runtime/cookies, where delete() passes those attributes through to the Set-Cookie header — which matters because a browser won't delete a cookie unless Secure/Path/Domain match the original.
Two options:
- Narrow the type to only allow
pathanddomain(matches the current runtime). - Forward
httpOnly/secure/sameSitetoo (matches edge-runtime).
I'd lean toward option 2 for parity:
| | [options: Omit<CookieOptions & { name: string }, "maxAge" | "expires">] | |
| | [options: Omit<CookieOptions & { name: string }, "maxAge" | "expires" | "value">] |
and then forward the extra attributes:
return this.set({
name,
value: "",
expires: new Date(0),
path: opts?.path,
domain: opts?.domain,
httpOnly: opts?.httpOnly,
secure: opts?.secure,
sameSite: opts?.sameSite,
});There was a problem hiding this comment.
Fixed in 6dce232 — delete() now forwards httpOnly, secure, and sameSite through to set(), matching edge-runtime behavior.
| validateCookieName(name); | ||
|
|
||
| const parts = [`${name}=${encodeURIComponent(value)}`]; | ||
| const path = opts?.path ?? "/"; |
There was a problem hiding this comment.
Previously set() without a path option would NOT emit Path= at all. Now it always defaults to Path=/. This matches @edge-runtime/cookies's normalizeCookie, so it's correct — but it is a subtle behavioral change beyond the bugs listed in the PR description. Worth a one-liner callout in the PR body so reviewers know it's intentional.
There was a problem hiding this comment.
Added a callout in the PR description.
| if (opts?.secure) parts.push("Secure"); | ||
| if (opts?.sameSite) parts.push(`SameSite=${opts.sameSite}`); | ||
|
|
||
| this._parsed.set(name, { serialized: parts.join("; "), entry: { name, value } }); |
There was a problem hiding this comment.
The internal map stores { serialized, entry } where entry is { name, value } — it doesn't preserve the CookieOptions (httpOnly, secure, sameSite, path, domain, maxAge, expires). The edge-runtime stores the full ResponseCookie object in its map, which lets callers inspect all attributes via get().
This isn't a regression (the old code didn't store options either), but it means get("session") returns { name, value } whereas edge-runtime returns { name, value, httpOnly: true, path: "/", ... }. If this is a known gap, fine — just flagging it as a potential follow-up since you're reworking this class anyway.
There was a problem hiding this comment.
Known gap — tracking as a follow-up.
| const entries: CookieEntry[] = []; | ||
| for (const header of this._headers.getSetCookie()) { | ||
| // Hydrate internal map from any existing Set-Cookie headers | ||
| for (const header of headers.getSetCookie()) { |
There was a problem hiding this comment.
Agree with the author's response to the Codex review here — getSetCookie() was already used unconditionally and the target runtimes (Node 18+, Workers) all support it. Eagerly hydrating in the constructor is the right call since you need the map to be populated before any method is called.
|
|
||
| // has() should work with internal map | ||
| it("has() returns false after delete()", async () => { | ||
| const { ResponseCookies } = await import("../packages/vinext/src/shims/server.js"); |
There was a problem hiding this comment.
Good test — this is the most important behavioral assertion. After delete(), the entry still exists in the map with an empty value and epoch expiry, so has() returning true is correct and matches edge-runtime. The comment explaining this is helpful.
packages/vinext/src/shims/server.ts
Outdated
| return [args[0], args[1] as string, args[2] as CookieOptions | undefined]; | ||
| } | ||
| const { name, value, ...opts } = args[0]; | ||
| return [name, value, Object.keys(opts).length > 0 ? (opts as CookieOptions) : undefined]; |
There was a problem hiding this comment.
Nit: the Object.keys(opts).length > 0 check means that set({ name: "x", value: "y" }) passes undefined as options, which then triggers the opts?.path ?? "/" default in set(). This works, but it adds a subtle code path — if someone later adds a property with a falsy default to CookieOptions, the empty-check could mask it.
Simpler alternative: always return opts as CookieOptions (an empty object with no keys is harmless since every property is optional).
| return [name, value, Object.keys(opts).length > 0 ? (opts as CookieOptions) : undefined]; | |
| return [name, value, opts as CookieOptions]; |
There was a problem hiding this comment.
Fixed in 6dce232 — always returns opts as CookieOptions now.
|
Review posted. Here's a summary of the feedback: Overall: Good fix. The core deduplication bug was real and the Key findings:
|
Address review feedback: - delete() now forwards httpOnly, secure, and sameSite attributes to match @edge-runtime/cookies behavior (browsers need matching Secure flag to actually delete a cookie) - Simplify parseCookieSetArgs: always return opts (empty object is harmless since all CookieOptions properties are optional)
|
/bigbonk review |
There was a problem hiding this comment.
Review
This is a well-executed fix. The Map-as-source-of-truth architecture correctly solves the header duplication bug and matches @edge-runtime/cookies. The API surface additions (object forms, getAll filter, delete with attribute forwarding) are all correct per edge-runtime parity. Tests are thorough and cover the important behavioral cases.
All 31 ResponseCookies tests pass. One minor issue below — the test description/assertion mismatch. The rest looks good to merge.
| }); | ||
|
|
||
| // has() should work with internal map | ||
| it("has() returns false after delete()", async () => { |
There was a problem hiding this comment.
The test description says "has() returns false after delete()" but the assertion on line 4172 expects true (because delete() calls set() under the hood, so the entry stays in the map). The inline comment explains the behavior correctly, but the test name is misleading — someone scanning test names would think has() returns false after deletion.
| it("has() returns false after delete()", async () => { | |
| it("has() returns true after delete() (entry persists as expired tombstone)", async () => { |
| delete( | ||
| ...args: | ||
| | [name: string] | ||
| | [options: Omit<CookieOptions & { name: string }, "maxAge" | "expires">] |
There was a problem hiding this comment.
Nit (not blocking): The Omit removes maxAge and expires from the type, which is correct. Worth noting that value is not in CookieOptions so it's implicitly excluded — this is fine but could confuse someone reading the type who expects to see "value" in the omit list.
Also, the Codex review flagged that spreading opts could override tombstone fields — but the current implementation correctly avoids this by explicitly listing each property on lines 640-644 rather than using ...opts. Good.
|
Review submitted. Here's a summary of my findings: Approved — the core fix is solid. The Two comments posted:
All 31 ResponseCookies tests pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dce2329cb
ℹ️ 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".
| this._parsed.set(name, { serialized: parts.join("; "), entry: { name, value } }); | ||
| this._syncHeaders(); |
There was a problem hiding this comment.
Using name as the sole key in ResponseCookies causes cookies that legitimately share a name but differ by Path/Domain to overwrite each other, and _syncHeaders() then rewrites headers with only the last variant. In practice, if a response already has Set-Cookie: sid=...; Path=/ and Set-Cookie: sid=...; Path=/app, then any later cookies.set()/cookies.delete() call will silently drop one scope, which can break auth/session behavior for multi-path apps.
Useful? React with 👍 / 👎.
|
|
||
| get(...args: [name: string] | [options: { name: string }]): CookieEntry | undefined { | ||
| const key = typeof args[0] === "string" ? args[0] : args[0].name; | ||
| return this._parsed.get(key)?.entry; |
There was a problem hiding this comment.
get() returns the internal entry object directly from _parsed, so user mutation (for example, mutating cookies.get("x")!.value) changes internal in-memory state without updating the serialized Set-Cookie header string. This creates a desynchronized state where future reads differ from emitted headers, whereas the previous implementation always returned freshly parsed snapshots.
Useful? React with 👍 / 👎.
2 participants
Summary
ResponseCookies.set()unconditionally appended Set-Cookie headers — setting the same cookie name twice produced duplicate headers, andget()returned the stale first match. Rewritten to use an internalMap<name, {serialized, entry}>as the single source of truth (matching@edge-runtime/cookies's architecture). The_syncHeaders()method deletes all Set-Cookie headers and re-appends from the map on every mutation.set({ name, value, httpOnly, ... })matching Next.js docs and middleware patterns.getAll(name?)andgetAll({ name }).delete({ name, path, domain })with path/domain propagation.get()accepts object formget({ name })matching edge-runtime.delete()now usesExpires=epoch(matching edge-runtime) instead ofMax-Age=0.delete()now forwardshttpOnly,secure, andsameSiteattributes (matching edge-runtime — browsers need matchingSecureto actually delete a cookie).set()now defaultsPath=/when no path option is given (previously omittedPathentirely). This matches@edge-runtime/cookies'snormalizeCookiebehavior.Test plan