Fix CookieMap.toJSON() crash with numeric cookie names#28314
Conversation
CookieMap.toJSON() used putDirect to set properties on the result object, which asserts that the property name is not an array index. Cookie names can be numeric strings (e.g. '0', '1', '42'), which parse as array indices and trigger the assertion. Use putDirectMayBeIndex instead, which correctly handles both indexed and named properties.
|
Updated 9:20 AM PT - Mar 20th, 2026
✅ @robobun, your commit 944a26658cfa790358fa90e08309d56c14f17eeb passed in 🧪 To try this PR locally: bunx bun-pr 28314That installs a local version of the PR into your bun-28314 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReworked Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward crash fix replacing putDirect with putDirectMayBeIndex, consistent with how FormData, FetchHeaders, and other bindings handle user-controlled property names.
Extended reasoning...
Overview
This PR fixes a crash in CookieMap::toJSON() when cookie names are numeric strings (e.g., "0", "1", "42"). The fix changes two calls from putDirect to putDirectMayBeIndex in src/bun.js/bindings/CookieMap.cpp, and adds a corresponding test case.
Security risks
None. The change moves from a method that asserts non-index property names to one that correctly handles both indexed and named properties. No new attack surface is introduced.
Level of scrutiny
This is a minimal, low-risk bug fix. The diff is 2 changed lines in C++ plus a 9-line test. The putDirectMayBeIndex pattern is already established across 6 other files in the bindings directory (JSFetchHeaders.cpp, JSDOMFormData.cpp, bindings.cpp, ZigGlobalObject.cpp, NodeHTTP.cpp, SerializedScriptValue.cpp), confirming this is the standard approach for user-controlled property names in Bun.
Other factors
- No CODEOWNERS rules cover the modified files.
- No outstanding reviewer comments.
- The pre-existing bug report about
hasPropertytraversing the prototype chain is valid but is not introduced by this PR — it exists in the unchangedhasPropertycall on line 245 and predates this change. - The test directly exercises the crash scenario from the PR description.
…hain Use getDirectOffset instead of hasProperty when deduplicating original cookies against modified cookies. hasProperty traverses the prototype chain, causing cookies named 'toString', 'constructor', etc. to be silently dropped from the output.
There was a problem hiding this comment.
LGTM — small, focused crash fix that follows established patterns (putDirectMayBeIndex) and also fixes the prototype chain deduplication issue.
Extended reasoning...
Overview
This PR touches two files: CookieMap.cpp (the fix) and cookie-map.test.ts (regression tests). The code change is minimal — replacing putDirect with putDirectMayBeIndex in both loops of toJSON(), and replacing hasProperty with getDirectOffset for own-property-only deduplication. Both changes follow established patterns used elsewhere in the codebase (FormData, FetchHeaders, env vars).
Security risks
None. Cookie names are user-controlled strings, but putDirectMayBeIndex is the correct JSC API for handling arbitrary property names. The switch from hasProperty to getDirectOffset actually improves correctness by avoiding prototype chain traversal.
Level of scrutiny
This is a straightforward 2-line bug fix with clear intent (preventing an assertion failure crash) and good test coverage. The pattern is well-established in the codebase. Low scrutiny is appropriate.
Other factors
The nit-level bug report about getDirectOffset not finding indexed properties for numeric key deduplication is technically valid — if the same numeric cookie name appears twice in m_originalCookies, the second value wins instead of the first. However, this is an extremely rare edge case (duplicate numeric cookie names in a single cookie string), and the PR is still a net improvement since numeric cookie names previously caused a crash. The PR includes two well-targeted regression tests covering both numeric names and Object.prototype property names.
getDirectOffset only searches named property storage, missing numeric keys stored in indexed storage by putDirectMayBeIndex. Use a HashSet for deduplication, consistent with FormData and URLSearchParams.
There was a problem hiding this comment.
LGTM — all previous feedback addressed; the HashSet dedup + putDirectMayBeIndex fix is clean and follows established patterns.
Extended reasoning...
Overview
This PR fixes a crash in CookieMap::toJSON() when cookie names are numeric strings (e.g., "0", "1", "42") by replacing putDirect with putDirectMayBeIndex. It also replaces the hasProperty-based deduplication (which traversed the prototype chain) with a HashSet<String>, fixing silent data loss for cookies named "toString", "constructor", etc. Two files changed: CookieMap.cpp (the fix) and cookie-map.test.ts (regression tests).
Security risks
None. This is a bugfix in cookie serialization that makes the code more robust against unusual but valid cookie names. No auth, crypto, or permission-related code is affected.
Level of scrutiny
Low scrutiny warranted. The change is small (~10 lines of logic), self-contained in a single method, and follows the exact same pattern used by FormData (JSDOMFormData.cpp) and URLSearchParams (JSURLSearchParams.cpp). The regression tests exercise the specific edge cases being fixed.
Other factors
I previously left two detailed comments identifying the hasProperty prototype chain bug and the getDirectOffset indexed storage mismatch. Both have been fully addressed in the final revision — the HashSet<String> approach was exactly what I recommended. The first inline comment is marked as resolved. No CODEOWNERS match for these files. CodeRabbit also found no actionable issues.
There was a problem hiding this comment.
LGTM — clean fix addressing the crash and both previously flagged deduplication issues.
Extended reasoning...
Overview
This PR fixes a crash in CookieMap::toJSON() triggered by numeric cookie names (e.g., "0", "1", "42") that parse as array indices and hit a JSC assertion in putDirect. The fix replaces putDirect with putDirectMayBeIndex in both loops and replaces the hasProperty-based deduplication with a HashSet<String> seenKeys, matching the pattern used by FormData, URLSearchParams, and FetchHeaders elsewhere in Bun. Two regression tests are added covering numeric names and Object.prototype-colliding names.
Security risks
None. The change only affects how cookie key-value pairs are written to a plain JS object during serialization. No auth, crypto, or permission logic is involved.
Level of scrutiny
This is a small, targeted bug fix (2 files, ~20 lines changed) in a non-security-critical serialization path. The fix follows well-established patterns already used throughout the codebase. Low scrutiny is appropriate.
Other factors
I previously raised two issues on this PR: (1) hasProperty traversing the prototype chain, silently dropping cookies named "toString", "constructor", etc., and (2) getDirectOffset failing to detect indexed properties, breaking dedup for numeric cookie names. Both have been addressed by the HashSet<String> approach, and both inline comments are resolved. The test coverage exercises both edge cases. CodeRabbit also found no actionable issues.
CookieMap.toJSON() used `putDirect` to set properties on the result
object, which asserts that the property name is not an array index.
Cookie names can be numeric strings (e.g. `"0"`, `"1"`, `"42"`), which
parse as array indices and trigger the assertion in
`JSObject::putDirectInternal`.
Use `putDirectMayBeIndex` instead, which correctly handles both indexed
and named properties. This is consistent with how other Bun code handles
user-controlled property names (e.g. FormData, FetchHeaders, environment
variables).
**Crash fingerprint:** `JSObjectInlines.h(451)` — `ASSERTION FAILED:
!parseIndex(propertyName)`
**Minimal repro:**
```js
const map = new Bun.CookieMap("0=first; 1=second; 42=answer");
map.toJSON();
```
---
**Verification:** CI building (Buildkite #40296, lint/format passing).
Diff is 2-line fix replacing `putDirect` → `putDirectMayBeIndex` in both
loops of `CookieMap::toJSON`, matching the established pattern used in
ZigGlobalObject.cpp (env vars), JSDOMFormData.cpp, JSFetchHeaders.cpp,
and NodeHTTP.cpp. Regression test in cookie-map.test.ts exercises
numeric cookie names ("0", "1", "42") through toJSON(). No
TODO/FIXME/HACK markers. No unrelated changes.
---
Co-authored-by: Alistair Smith <hi@alistair.sh>
CookieMap.toJSON() used `putDirect` to set properties on the result
object, which asserts that the property name is not an array index.
Cookie names can be numeric strings (e.g. `"0"`, `"1"`, `"42"`), which
parse as array indices and trigger the assertion in
`JSObject::putDirectInternal`.
Use `putDirectMayBeIndex` instead, which correctly handles both indexed
and named properties. This is consistent with how other Bun code handles
user-controlled property names (e.g. FormData, FetchHeaders, environment
variables).
**Crash fingerprint:** `JSObjectInlines.h(451)` — `ASSERTION FAILED:
!parseIndex(propertyName)`
**Minimal repro:**
```js
const map = new Bun.CookieMap("0=first; 1=second; 42=answer");
map.toJSON();
```
---
**Verification:** CI building (Buildkite #40296, lint/format passing).
Diff is 2-line fix replacing `putDirect` → `putDirectMayBeIndex` in both
loops of `CookieMap::toJSON`, matching the established pattern used in
ZigGlobalObject.cpp (env vars), JSDOMFormData.cpp, JSFetchHeaders.cpp,
and NodeHTTP.cpp. Regression test in cookie-map.test.ts exercises
numeric cookie names ("0", "1", "42") through toJSON(). No
TODO/FIXME/HACK markers. No unrelated changes.
---
Co-authored-by: Alistair Smith <hi@alistair.sh>
2 participants
CookieMap.toJSON() used
putDirectto set properties on the result object, which asserts that the property name is not an array index. Cookie names can be numeric strings (e.g."0","1","42"), which parse as array indices and trigger the assertion inJSObject::putDirectInternal.Use
putDirectMayBeIndexinstead, which correctly handles both indexed and named properties. This is consistent with how other Bun code handles user-controlled property names (e.g. FormData, FetchHeaders, environment variables).Crash fingerprint:
JSObjectInlines.h(451)—ASSERTION FAILED: !parseIndex(propertyName)Minimal repro:
Verification: CI building (Buildkite #40296, lint/format passing). Diff is 2-line fix replacing
putDirect→putDirectMayBeIndexin both loops ofCookieMap::toJSON, matching the established pattern used in ZigGlobalObject.cpp (env vars), JSDOMFormData.cpp, JSFetchHeaders.cpp, and NodeHTTP.cpp. Regression test in cookie-map.test.ts exercises numeric cookie names ("0", "1", "42") through toJSON(). No TODO/FIXME/HACK markers. No unrelated changes.