Conversation
WalkthroughThis PR enforces browser ChangesCookie prefix and path validation
Sequence Diagram(s)sequenceDiagram
participant JS as JS Caller
participant JSCookieMap as JSCookieMap.cpp
participant CookieMap as CookieMap::set
participant Cookie as Cookie validation
JS->>JSCookieMap: cookieMap.set(...)
JSCookieMap->>CookieMap: impl.set(cookie)
CookieMap->>Cookie: validateNamePrefix(name, ...)
Cookie-->>CookieMap: ExceptionOr<void>
alt validation fails
CookieMap-->>JSCookieMap: Exception
JSCookieMap-->>JS: propagateException + throw
else validation succeeds
CookieMap->>CookieMap: removeInternal + append
CookieMap-->>JSCookieMap: {}
JSCookieMap-->>JS: success
end
Related PRs: None identified. Suggested labels: javascript, needs-tests-changes, bun:api Suggested reviewers: Jarred-Sumner, cirospaciari 🐰
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/runtime/cookies.mdx`:
- Around line 116-126: Update the CookieMap documentation to mention that
`delete()`/`remove()` can also throw when re-validating prefixed cookies, not
just `set()`. In the `delete()` section, note that `CookieMap.delete` rejects
invalid `__Secure-` and `__Host-` combinations using the same prefix rules, and
reference the existing `CookieMap.remove`/`delete()` behavior so readers know a
`TypeError` can occur during deletion as well.
In `@src/jsc/bindings/CookieMap.cpp`:
- Around line 197-213: The prefix handling in CookieMap::remove is duplicated
and should reuse the shared Cookie prefix logic instead of hard-coding
"__Secure-" and "__Host-". Expose or call the existing Cookie prefix helpers
(such as hasSecurePrefix and hasHostPrefix) from Cookie.cpp/Cookie.h, then use
them when building the expiring cookie in CookieMap::remove so the prefix rules
stay consistent with validateNamePrefix and future changes only need to happen
in one place.
In `@test/js/bun/cookie/cookie-map.test.ts`:
- Around line 466-472: The __Host- delete test in CookieMap is missing the same
post-throw header invariant checked by the sibling domain-invalid case. Update
the failing delete scenario in cookie-map.test.ts to also assert that the map
emits no Set-Cookie headers after the rejected delete, using the existing
CookieMap instance and its toSetCookieHeaders() behavior for consistency with
the other delete validation tests.
- Around line 475-513: The “a rejected set leaves the previous cookie in place”
test in cookie-map.test.ts uses a bare toThrow() on Bun.CookieMap.set, which is
too weak and can pass for unrelated failures. Update that assertion to match the
same exact prefixed-cookie error text used by the other __Host- and __Secure-
tests, so the test verifies the intended validation path rather than any throw.
Keep the rest of the CookieMap behavior checks unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56b15fe0-dc85-42bc-a432-7ee75c62b32d
📒 Files selected for processing (11)
docs/runtime/cookies.mdxpackages/bun-types/bun.d.tssrc/jsc/bindings/Cookie.cppsrc/jsc/bindings/Cookie.hsrc/jsc/bindings/CookieMap.cppsrc/jsc/bindings/CookieMap.hsrc/jsc/bindings/webcore/JSCookie.cppsrc/jsc/bindings/webcore/JSCookieMap.cpptest/js/bun/cookie/cookie-map.test.tstest/js/bun/cookie/cookie.test.tstest/js/bun/util/cookie.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Addressed the review in 17e693c:
|
There was a problem hiding this comment.
The implementation looks solid and my earlier nit is addressed, but this introduces new throw conditions on previously-accepted input (__Secure-/__Host- without secure, and any non-/-leading path), so a maintainer should sign off on the API decision to throw rather than auto-repair.
Extended reasoning...
Overview
This PR enforces RFC 6265bis §4.1.3 cookie name prefix rules (__Secure-, __Host-) and RFC 6265 §5.2.4 path rules on Bun's cookie write path. It touches 11 files: native C++ (Cookie.{cpp,h}, CookieMap.{cpp,h}, JSCookie.cpp, JSCookieMap.cpp), docs, .d.ts, and three test files. Cookie::setSecure and CookieMap::set change from void to ExceptionOr<void>; Cookie::create(CookieInit) and the domain/path/secure setters now validate the prefix rules; CookieMap::remove reorders removeInternal after the fallible step so a rejected delete no longer drops the existing entry.
Security risks
None introduced — the change tightens validation to match browser enforcement, which is strictly safer than the status quo (where a __Host- session cookie set without secure was silently dropped by every browser). The prefix helpers are static string checks with no untrusted parsing.
Level of scrutiny
This warrants human review because it is a breaking API change: inputs that Bun previously accepted and serialized (e.g. new Bun.Cookie("__Host-a", "1"), { path: "../foo/" }) now throw TypeError. The PR moved two entries out of the cookie-package parity suite's valid-paths list into a throws-list. The design choice — throw vs. auto-set secure: true vs. warn — is defensible (it mirrors the CookieStore spec's "set a cookie" algorithm) but is the kind of user-facing API surface decision a maintainer should confirm. There's also a deliberate asymmetry between the two Cookie::create overloads (only the CookieInit one enforces prefixes, so Cookie.parse stays a pure read path) that's worth a second pair of eyes.
Other factors
Test coverage is thorough (constructor, Cookie.from, object form, setters, CookieMap.set/delete, Bun.serve end-to-end, case-insensitive match, negative cases, repair-after-parse). All prior review feedback (mine and CodeRabbit's four nits) was addressed in 17e693c: prefix helpers are now shared statics on Cookie, the docs ## Types mirror is synced, the bare toThrow() is pinned, and delete() throw behavior is documented. No bugs were found by the automated hunt. The C++ changes follow existing patterns (ExceptionOr<void> + propagateException) already used by setDomain/setPath.
|
Flagging the API decision for a maintainer, since the review asked for it. The short version of why this throws rather than auto-repairing:
That leaves Nothing that works today stops working. A The one tightening that goes beyond that: If you would rather have |
RFC 6265bis 4.1.3 requires a user agent to ignore a cookie whose name starts
with __Secure- unless it is Secure, or with __Host- unless it is Secure, has
no Domain, and has Path=/. Bun enforced none of this when creating a cookie,
so `cookies.set("__Host-sid", token)` put a header on the wire that every
browser drops.
Cookie::create(CookieInit) and CookieMap::set() now reject a cookie that
violates its name's prefix, and CookieMap::remove() builds its expiring
cookie through the same path. Cookie.parse() keeps reporting what was on the
wire. A rejected set or delete no longer drops the cookie already in the map.
Also reject a Path that does not start with "/": a user agent ignores such an
attribute, and Bun's own parser drops it, so the cookie could not round-trip.
CookieMap::remove derives the Secure flag from Cookie::hasSecurePrefix / Cookie::hasHostPrefix instead of repeating the prefix strings, so the rule lives in one place. Pin the error message in the rejected-set test, assert no header is emitted after a rejected delete, document that delete() throws for the same combinations set() rejects, and sync the CookieInit mirror in the docs.
17e693c to
59ce445
Compare
|
The red X on build 68973 is a Buildkite scheduling failure, not a test failure. No job in that build is in a
The 14 expired jobs are build steps on every platform at once (darwin, linux, freebsd, windows, android, musl, asan). A source diff cannot cause build jobs to go unscheduled on all of them simultaneously. The build and compile jobs that did get an agent all passed, which is the signal that matters for a C++ change: A re-run should clear it. I have not pushed anything, to avoid stacking empty commits on the branch. Where the PR stands
The one thing still open is a maintainer call, not a code change: whether a prefixed name missing |
Cookie::validateAttributes(name, domain, path, secure, sameSite, partitioned) now holds every RFC 6265bis attribute combination browsers reject: SameSite=None without Secure, Partitioned without Secure, and the __Secure-/__Host- name-prefix rules. It runs from Cookie::create(CookieInit), CookieMap::set, and the domain/path/secure/sameSite/partitioned setters. Cookie.parse keeps reporting what was on the wire. CookieMap::delete normalizes a __Host- tombstone (no Domain, Path=/) instead of throwing, since the browser could only have stored that shape, and it now validates before mutating so a rejected delete leaves the map untouched. Supersedes #33459.
|
The |
1 participant
Repro
Every one of those headers is ignored by every browser, so a
__Host-session cookie set through the documented API silently never sticks, with no error anywhere.Cause
RFC 6265bis 4.1.3 requires a user agent to ignore a cookie whose name begins with
__Secure-unless it isSecure, or with__Host-unless it isSecure, has noDomain, and hasPath=/. Nothing on the write path checked this:Cookie::create,CookieMap::set, and thesecure/domain/pathsetters accepted any combination.CookieMap::removealready forcedSecureonto the deletion cookie, so the rule was half-known.A related case:
path: "x"was accepted and serialized asPath=x, which a user agent ignores (RFC 6265 5.2.4) and which Bun's ownCookie.parsethrows away, so the cookie could not round-trip through Bun.Fix
Cookie::validateNamePrefixholds the rule, split per attribute since a cookie's name is immutable but its attributes are not. It runs at every point where a cookie can reach the wire:Cookie::create(CookieInit), the "set a cookie" entry behindnew Bun.Cookie,Cookie.from, andCookieMap.setCookieMap::set(Cookie), which catches a cookie that came fromCookie.parsewithout being repairedCookieMap::remove, which now builds its expiring cookie through the same entrysecure,domainandpathsetters, sinceCookieMap.setkeeps a reference to theCookieobject and the cookie could otherwise be mutated into an invalid state afterwardsViolations throw a
TypeError, matching the CookieStore "set a cookie" algorithm thatBun.CookieMapmodels.Cookie.parsestill reports what was on the wire without throwing; it is a read path, and a cookie parsed from a broken upstream header can be repaired (cookie.secure = true) and then set.Two smaller things fall out of this:
setordeleteno longer drops the cookie that was already in the map (the removal now happens after the fallible step, not before)paththat does not start with/is aTypeError. This changes two inputs in thecookiepackage parity suite ("../foo/","./") from serialized to rejected; they are moved into a test asserting the throw.Verification
With
src/reverted to main, 19 of the new tests fail.After the fix