fix(CookieMap): skip symbol properties in constructor to prevent crash #25010
Conversation
WalkthroughUse identifierToUSVString when deriving property keys for CookieMap construction, add an exception check after conversion, and use the converted key string when setting records. Add regression tests constructing Bun.CookieMap from objects with symbol properties and numeric-string keys to prevent crashes and validate string-key behavior. Changes
Suggested reviewers
Pre-merge checks✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (10)📚 Learning: 2025-10-01T21:59:54.571ZApplied to files:
📚 Learning: 2025-10-01T21:49:27.862ZApplied to files:
📚 Learning: 2025-10-25T17:20:19.041ZApplied to files:
📚 Learning: 2025-10-18T05:23:24.403ZApplied to files:
📚 Learning: 2025-10-19T02:44:46.354ZApplied to files:
📚 Learning: 2025-09-20T00:58:38.042ZApplied to files:
📚 Learning: 2025-09-20T00:57:56.685ZApplied to files:
📚 Learning: 2025-10-08T13:48:02.430ZApplied to files:
📚 Learning: 2025-10-01T22:13:08.081ZApplied to files:
📚 Learning: 2025-09-20T03:39:41.770ZApplied to files:
🔇 Additional comments (3)
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/bindings/webcore/JSCookieMap.cpp(1 hunks)test/regression/issue/ENG-21750.test.ts(1 hunks)
🔇 Additional comments (1)
src/bun.js/bindings/webcore/JSCookieMap.cpp (1)
161-163: LGTM! Defensive guard against symbol properties.The explicit symbol check correctly prevents the crash. Note that line 156 already specifies
PropertyNameMode::Strings, which should theoretically filter out symbols. This additional guard serves as a defensive safeguard, which is appropriate given the crash involved undefined behavior.
| import { expect, test } from "bun:test"; | ||
|
|
||
| // https://linear.app/oven/issue/ENG-21750 | ||
| // CookieMap constructor should not crash when passed an object with symbol properties | ||
| test("CookieMap should handle objects with symbol properties without crashing", () => { | ||
| // This should not crash - Bun object has symbol properties | ||
| const cookieMap = new Bun.CookieMap(Bun); | ||
| expect(cookieMap).toBeInstanceOf(Bun.CookieMap); | ||
|
|
||
| // Also test with a custom object that has symbol properties | ||
| const obj = { | ||
| foo: "bar", | ||
| [Symbol.for("test")]: "value", | ||
| [Symbol("local")]: "another", | ||
| }; | ||
| const cookieMap2 = new Bun.CookieMap(obj); | ||
| expect(cookieMap2).toBeInstanceOf(Bun.CookieMap); | ||
| // Symbol properties should be skipped, but string properties should work | ||
| expect(cookieMap2.get("foo")).toBe("bar"); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Comprehensive regression test.
The test effectively covers both the crash scenario from the bug report (line 7) and a controlled case with explicit symbol properties (lines 11-19). The assertions verify that construction succeeds and string properties remain accessible.
Optional enhancement: Consider adding an explicit assertion that symbol properties are not retrievable, e.g.:
expect(cookieMap2.get(Symbol.for("test") as any)).toBeNull();This would document the expected behavior more clearly, though the current test is sufficient.
🤖 Prompt for AI Agents
In test/regression/issue/ENG-21750.test.ts around lines 1 to 20, add an explicit
assertion that symbol-keyed properties are not retrievable from the CookieMap to
document expected behavior; update the test to call cookieMap2.get with the
symbol keys used (e.g., Symbol.for("test") and Symbol("local")) and assert they
return null/undefined (use the same null/undefined expectation style as other
tests), keeping the existing assertions intact.
robobun
force-pushed
the
claude/fix-cookiemap-symbol-crash
branch
from
9edfdd6 to
ba24636
Compare
When creating a CookieMap from an object, calling propertyName.string() directly could cause a null pointer reference crash with UBSAN. This change uses identifierToUSVString() which properly handles the conversion of property names to strings, following the same pattern used in JSDOMConvertRecord.h. Fixes: ENG-21750 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
robobun
force-pushed
the
claude/fix-cookiemap-symbol-crash
branch
from
ba24636 to
c251c72
Compare
There was a problem hiding this comment.
These tests do not appear to reproduce the crash. Make sure the tests crash with production bun (or a debug build of bun without your fix)
Summary
CookieMapfrom an object with symbol propertiesTest plan
test/regression/issue/ENG-21750.test.tsFixes: ENG-21750
🤖 Generated with Claude Code