fix(cookie): dedup cookies with leading-dot / mixed-case domains#1462
Conversation
Cookie dedup compared the raw `options.domain` of a new cookie against the normalized domain from `parseSetCookie` (leading "." stripped, lowercased). For domains like `.Example.com` the keys never matched, so two `Set-Cookie` headers were emitted instead of a replacement. Normalize the domain (strip leading ".", lowercase) on both sides in `_getDistinctCookieKey`, per RFC 6265 (Domain is case-insensitive and a leading "." is ignored). Co-Authored-By: Claude Fable 5 <[email protected]>
pi0x
left a comment
pi0x
left a comment
There was a problem hiding this comment.
Review verdict: LGTM (approve) — posting as a comment since GitHub blocks self-approval on this account.
Clean, correct, minimal fix.
Correctness
- Normalizing inside
_getDistinctCookieKeyfixes both sides at once: the new-cookie key (options.domain) and the existing-cookie key (fromparseSetCookie) now go through identical normalization, so they match regardless of leading dot or casing. It's the single shared chokepoint, so no missed call sites —setCookieis the only consumer, anddeleteCookie/chunked helpers route through it. (options.domain || "").replace(/^\./, "").toLowerCase()handles empty/undefined domain (→"") and idempotently re-normalizes the already-normalizedparseSetCookieoutput. Path and cookie name are left untouched, which is correct: RFC 6265 treats path and name as case-sensitive, only Domain as case-insensitive with a single leading dot ignored.- The actual emitted
Set-Cookieheader still carries the user's rawDomain=.Example.com— only the dedup key is normalized. Correct: we dedup without mutating the requested cookie.
Verification
- Reverted the source change and confirmed the new test fails as expected (two
Set-Cookieheaders emitted); restored, and all 98 cookie tests pass in both web and node matrix modes.pnpm lint(oxlint + oxfmt) clean. - Test quality is good: covers both leading-dot and mixed-case in one case, asserts exact single-header output.
Matches AGENTS.md conventions and RFC 6265 §4.1.2/§5.1.3.
🤖 Generated with Claude Code
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesCookie deduplication
Estimated code review effort: 2 (Simple) | ~5 minutes Suggested reviewers: Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
2 participants
Cookie dedup in
setCookiekeyed new cookies on the rawoptions.domain, but keyed existing cookies on the normalized domain fromparseSetCookie(leading.stripped, lowercased). For domains like.Example.comthe keys never matched, so twoSet-Cookieheaders were emitted instead of a replacement.This normalizes the domain (strip leading
., lowercase) on both sides in_getDistinctCookieKey, per RFC 6265 where Domain is case-insensitive and a leading.is ignored. Adds a regression test covering setting the same cookie twice withdomain: ".Example.com".🤖 Generated with Claude Code
Summary by CodeRabbit