feat(http)!: allowCookies and disallow cookies from caching by default - #58
Conversation
Introduces `allowCookies?: string[]` on `defineCachedHandler` and makes cookies non-participating in caching by default (secure default). BREAKING CHANGE: by default no cookies participate in caching now. - The `Cookie` request header is stripped before the handler runs and never varies the cache key, so a handler can no longer produce cookie-dependent output that gets cached and served to other users. - Any response carrying a `Set-Cookie` header is refused storage — still returned to the caller that triggered it, but never cached and replayed to other requests (closes the session-leak class from #49). Set `allowCookies` to opt specific cookie names back in: only those survive in the handler-visible `Cookie` header and vary the key (sorted, order-independent, like `allowQuery`), and a `Set-Cookie` response is cacheable only when every cookie it sets is allowlisted (#54). The Set-Cookie guard inspects `res.headers.getSetCookie()` (lossless) in the resolver rather than the serialized headers, which collapse multiple Set-Cookie values to the last one. Refs: #49 (3), #54 (2) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Addresses subagent review of #58: - Fail-safe Set-Cookie guard: on runtimes without `Headers.getSetCookie`, fall back to header presence and block conservatively instead of failing open (previously `getSetCookie?.() ?? []` silently allowed storing a Set-Cookie response where the method was unavailable). - `allowCookies` now genuinely supersedes `varies: ["cookie"]` by dropping `"cookie"` from the varied header names when the allowlist is set, so the full raw Cookie header no longer double-varies the key nor strips the allowlisted cookie from the handler. - Normalize `allowCookies` (trim/dedupe names, empty list -> "no cookies"). - Document the custom-`getKey` and request-coalescing caveats. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
pi0
changed the title
allowCookies — disallow cookies from caching by defaultallowCookies and disallow cookies from caching by default
Match the plain-expression style of the adjacent allowedQueryNames. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Closes the cookie-related caching gaps from #49 (item 3) and #54 (item 2) with a single, secure-by-default option.
What
Introduces
allowCookies?: string[]ondefineCachedHandler, and makes cookies non-participating in caching by default.Default (no
allowCookies) — no cookies allowedCookieheader is stripped before the handler runs and never varies the cache key, so a handler can no longer produce cookie-dependent output that gets cached and replayed to other users.Set-Cookierefuses storage — it is still returned to the caller that triggered it, but never cached and replayed to other requests. Closes the session/cookie-leak class described in http: Vary header, server-only caching, and Set-Cookie caching guard #49 (3) (🐛defineCachedEventHandlerresets session unexpectedly — causes user logout and potential security risk nitrojs/nitro#3468).Opt-in (
allowCookies: ["theme"])Cookieheader, and their name/value pairs vary the key — sorted and order-independent, mirroringallowQuery, so only the relevant subset is hashed rather than the entire rawCookieheader. Closes Cache key generation: fix getKey delimiter stripping (#1874) and support cookie-value-based keys (#3061) #54 (2) (How to generate CacheKey based on a Cookie Value nitrojs/nitro#3061).Set-Cookieis cacheable only when every cookie it sets is allowlisted.Cookie names are case-sensitive.
allowCookiessupersedesvaries: ["cookie"].Previously the
Cookierequest header was passed through to the handler andSet-Cookieresponses could be stored and replayed. Both now require an explicitallowCookiesopt-in. Marked with!per Conventional Commits.Implementation notes
Set-Cookieguard inspectsres.headers.getSetCookie()in the resolver (lossless) and flags the entry non-enumerably forvalidateto reject. This avoidsObject.fromEntries(headers.entries())collapsing multipleSet-Cookievalues to the last one — which would be a security gap in the allowlist path.Request/Responsedesign.Tests
5 new tests in
test/index.test.tscovering: default Cookie stripping + non-varying key, allowlisted key scoping (order-independent), handler-visible Cookie filtering, defaultSet-Cookiestorage refusal (with first-caller passthrough), and the all-cookies-allowlisted requirement for cachingSet-Cookie. Full suite: 139 passing; typecheck + lint clean. Docs updated inREADME.mdandAGENTS.md.🤖 Generated with Claude Code