fix(http)!: strip non-allowlisted Set-Cookie instead of blocking storage (#61)

[pi0x]

Fixes #61.

Problem

HTTP request coalescing deduplicates concurrent same-key requests onto a single handler resolution. When a handler mints a per-request Set-Cookie (e.g. a session id), the leader's cookie was replayed to every deduplicated peer — users 2..N received someone else's session identifier. A cross-user session leak.

The prior _blockSetCookie guard only refused storage. It ran after coalescing had already handed every peer the leader's serialized response (cookie included), so it prevented the entry being cached for future requests but did nothing about the concurrent in-flight peers already sharing the leader's response.

Reproduction (was failing, now passing):

const handler = defineCachedHandler(
  () => new Response("ok", { headers: { "set-cookie": `sid=${crypto.randomUUID()}` } }),
  { maxAge: 10 },
);
const [a, b] = await Promise.all([handler(ev()), handler(ev())]);
a.headers.get("set-cookie") === b.headers.get("set-cookie"); // was true (leak)

Fix

Strip every non-allowlisted Set-Cookie in the serialize hook, before the response headers are serialized. serialize runs once for the shared resolution, so:

• no coalesced peer (nor a future cache hit) can observe another caller's cookie — the leak is closed by construction, upstream of storage
• the rest of the response is cached normally

This mirrors how CDNs / Varnish drop Set-Cookie on cacheable responses, and the existing Cookie-request-header stripping on the way in — making the secure default symmetric in both directions.

• allowCookies: string[] still opts specific names back in: those survive on the response (and vary the key), others are stripped and the rest is still cached.
• On runtimes without getSetCookie (can't enumerate individual cookies), all Set-Cookies are stripped — fail safe.
• validate keeps its read-side check as defense-in-depth for pre-existing/foreign stored entries written before the strip existed.

Breaking change

A Set-Cookie is no longer returned to its direct caller by default. Handlers that mint per-request cookies must either allowlist them via allowCookies, or serve from a bypassed (non-GET/HEAD) route (which passes through untouched).

Tests

• New regression test: concurrent coalesced callers never share a minted Set-Cookie.
• Updated the four existing cookie tests to the strip-and-cache behavior (default strips + caches; allowCookies keeps allowed, strips the rest; no-getSetCookie runtime strips conservatively).
• Full suite: 177 passing, lint + typecheck clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Cookie handling in cached responses now supports an allowlist, letting selected cookies be preserved and shared through cache hits.

• Bug Fixes

  • Cookies are now excluded from caching by default, preventing unexpected request variation and removing Set-Cookie headers from cached responses.
  • Cached entries with disallowed cookies are safely rejected on read.
  • Concurrent requests no longer leak per-request cookies from shared cached responses.

• Documentation

  • Clarified how cookie allowlisting works and when cookies are safe to cache.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes replace the prior _blockSetCookie flag mechanism in defineCachedHandler with a default behavior that strips request Cookie and response Set-Cookie headers before caching, filtering by an allowCookies allowlist. Documentation in AGENTS.md, README.md, and src/types.ts is updated, along with corresponding tests.

Changes

Cookie caching behavior update

Layer / File(s)	Summary
allowCookies contract and documentation src/types.ts, AGENTS.md, README.md	JSDoc and docs updated to describe default cookie stripping (request Cookie and response Set-Cookie) and allowCookies allowlist semantics superseding varies: ["cookie"], including a caveat that allowlisted cookies are shared across callers.
serialize/validate implementation change src/http.ts	serialize now filters Set-Cookie by allowCookies (using getSetCookie() when available, else stripping fail-closed) before storing; validate no longer relies on the removed _blockSetCookie flag, using stored-header allowlist checks instead.
Test coverage for default stripping and allowlist behavior test/index.test.ts	Tests updated to verify default Set-Cookie stripping, coalesced concurrent requests not sharing minted cookies, allowlist preservation and mixed stripping, no-getSetCookie fallback, and pre-upgrade entry regression comment.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related issues

• http: Vary header, server-only caching, and Set-Cookie caching guard #49: The Set-Cookie caching guard in src/http.ts implements an allowlist-based opt-in variant of this issue's requested blanket-reject caching guard.

Suggested reviewers: pi0

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: stripping non-allowlisted Set-Cookie values instead of blocking storage.
Linked Issues check	✅ Passed	The changes address #61 by preventing leaked cookies from shared/coalesced responses while preserving caching for allowlisted cookies.
Out of Scope Changes check	✅ Passed	The code, docs, and tests all relate directly to the Set-Cookie caching fix and its documented behavior.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/cookie-coalesciding

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/http.ts (1)

40-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update stale comment: "Set-Cookie responses are refused storage" no longer matches behavior.

The comment at lines 40-44 says Set-Cookie responses are refused storage, but the new behavior implemented at lines 185-207 strips disallowed Set-Cookie headers and caches the rest of the response. This comment should be updated to reflect the stripping behavior, e.g. "Set-Cookie headers are stripped before storage."

📝 Proposed fix for stale comment

   // Allowlist of cookie names that may participate in caching. `undefined` means
   // "no cookies allowed": the Cookie request header is stripped before the handler
-  // runs, cookies never vary the key, and Set-Cookie responses are refused storage.
+  // runs, cookies never vary the key, and Set-Cookie headers are stripped before
+  // storage so the rest of the response is cached.
   // Names are trimmed/deduped; an empty (or whitespace-only) list normalizes to the
   // "no cookies allowed" default.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/http.ts` around lines 40 - 44, Update the stale documentation comment in
the HTTP caching allowlist section so it matches the behavior in the response
storage path. The comment near the allowlist definition currently says
“Set-Cookie responses are refused storage,” but the logic in the response
handling flow now strips disallowed Set-Cookie headers and still caches the
remaining response. Adjust the wording in that comment to reflect the
stripping-before-storage behavior, using the relevant caching/response storage
symbols in src/http.ts as the reference point.

🧹 Nitpick comments (1)

test/index.test.ts (1)

2345-2367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add callCount assertion to verify coalescing actually occurred.

The test claims "Two concurrent requests collapse onto one resolution" but doesn't verify it. Without expect(callCount).toBe(1), the test passes even if both requests invoke the handler independently — both would still have set-cookie stripped. Asserting coalescing ensures the test genuinely exercises the issue #61 scenario.

✅ Proposed addition

     expect(a.headers.get("set-cookie")).toBeNull();
     expect(b.headers.get("set-cookie")).toBeNull();
+    expect(callCount).toBe(1);
   });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/index.test.ts` around lines 2345 - 2367, The coalescing test in
defineCachedHandler only checks that both responses drop set-cookie, but it
never verifies the requests were actually collapsed into one execution. After
the Promise.all in the test block, add an assertion on callCount to confirm the
handler ran once, so the issue `#61` case is genuinely exercised rather than
passing if both requests execute independently.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/http.ts`:
- Around line 185-207: The cookie filtering in the response handling code
preserves allowed Set-Cookie values in memory, but the cache serialization via
Object.fromEntries(res.headers.entries()) collapses duplicate set-cookie headers
to one. Update the header capture/reconstruction path in src/http.ts (the logic
around res.headers.getSetCookie and the response headers serialization) so
set-cookie is stored separately or handled as a multi-value header, then restore
all allowed cookies when rebuilding the cached response.

---

Outside diff comments:
In `@src/http.ts`:
- Around line 40-44: Update the stale documentation comment in the HTTP caching
allowlist section so it matches the behavior in the response storage path. The
comment near the allowlist definition currently says “Set-Cookie responses are
refused storage,” but the logic in the response handling flow now strips
disallowed Set-Cookie headers and still caches the remaining response. Adjust
the wording in that comment to reflect the stripping-before-storage behavior,
using the relevant caching/response storage symbols in src/http.ts as the
reference point.

---

Nitpick comments:
In `@test/index.test.ts`:
- Around line 2345-2367: The coalescing test in defineCachedHandler only checks
that both responses drop set-cookie, but it never verifies the requests were
actually collapsed into one execution. After the Promise.all in the test block,
add an assertion on callCount to confirm the handler ran once, so the issue `#61`
case is genuinely exercised rather than passing if both requests execute
independently.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1b07740d-4b3d-4fad-b92c-1ce5d9baf50b

📥 Commits

Reviewing files that changed from the base of the PR and between 3996d2e and dc5ad23.

📒 Files selected for processing (5)

• AGENTS.md
• README.md
• src/http.ts
• src/types.ts
• test/index.test.ts

[coderabbitai]

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for test cases that set multiple Set-Cookie headers with allowCookies
rg -nC5 'allowCookies' test/index.test.ts | head -80

Repository: unjs/ocache

Length of output: 3259

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the serialization code around the reported lines.
sed -n '190,240p' src/http.ts

# Inspect the nearby tests that cover allowCookies + Set-Cookie handling.
sed -n '2360,2425p' test/index.test.ts

# Look for any direct tests that exercise multiple allowlisted Set-Cookie headers.
rg -nC3 'set-cookie|getSetCookie|allowCookies' test/index.test.ts

Repository: unjs/ocache

Length of output: 11731

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the cache entry type and the read-side reconstruction.
rg -nC4 'type ResponseCacheEntry|interface ResponseCacheEntry|transform:|createResponse|new Response\(' src/http.ts

# Read the relevant section around the cache entry serialization/replay path.
sed -n '150,290p' src/http.ts

Repository: unjs/ocache

Length of output: 9154

---

Preserve multiple allowlisted Set-Cookie headers in the cache entry. Object.fromEntries(res.headers.entries()) collapses duplicate set-cookie values to the last one, so a response that sets two allowed cookies will replay only one. Store set-cookie separately or special-case it during serialization and reconstruction.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/http.ts` around lines 185 - 207, The cookie filtering in the response
handling code preserves allowed Set-Cookie values in memory, but the cache
serialization via Object.fromEntries(res.headers.entries()) collapses duplicate
set-cookie headers to one. Update the header capture/reconstruction path in
src/http.ts (the logic around res.headers.getSetCookie and the response headers
serialization) so set-cookie is stored separately or handled as a multi-value
header, then restore all allowed cookies when rebuilding the cached response.