JSPropertyIterator: enumerate string keys only, never symbols

[robobun]

What

Symbol-keyed properties surface through every JSPropertyIterator consumer under the symbol's description, as if they were string keys:

const s = Symbol("SYMK");
Bun.YAML.stringify({ [s]: 1, a: 2 });  // "{a: 2,SYMK: 1}"
Bun.TOML.stringify({ [s]: 1, a: 2 });  // "a = 2\nSYMK = 1\n"
Bun.JSON5.stringify({ [s]: 1, a: 2 }); // "{a:2,SYMK:1}"
Bun.spawnSync({ cmd: ["env"], env: { [s]: "1" } }); // child sees SYMK=1

JSON.stringify skips symbol keys, as do js-yaml, smol-toml, @iarna/toml, and the json5 reference implementation. Node skips them in child_process env and console.table.

The worst case is node:http2: every request using the documented never-index API carries [http2.sensitiveHeaders]: [...] on its headers object, and the native header walk serialized it, so Bun sent a literal nodejs.http2.sensitiveheaders header on the wire:

client.request({ ":path": "/", cookie: "secret", [http2.sensitiveHeaders]: ["cookie"] });
// server receives: ["cookie", "nodejs.http2.sensitiveheaders"]  (node: ["cookie"])

The comments in src/js/node/http2.ts already assume "the native header walk skips symbol keys".

Cause

Bun__JSPropertyIterator__create builds its PropertyNameArrayBuilder with PropertyNameMode::StringsAndSymbols, and getNameAndValue returns Bun::toString(prop.impl()), which for a symbol is its description. Property names cross the FFI as plain strings, so no consumer can even distinguish Symbol("a") from the string key "a".

The JSC C API this binding replaced in #10161 (JSObjectCopyPropertyNames) enumerates with PropertyNameMode::Strings; the mode changed silently in the port, so this is a regression from 1.1.4.

Fix

Use PropertyNameMode::Strings. A survey of all 20 JSPropertyIterator::init call sites (serializers, spawn/shell env, h2 headers, routes, bundler/transpiler config, ffi symbols, parseArgs, expect, console.table, JSX props, error reporting, valkey, ini, macros) found none that wants symbols: Bun.inspect and the test runner's pretty printer render symbol keys through the separate forEachProperty path, which reports an isSymbol flag and is unaffected (guarded by a new assertion).

Note: #34241 adds an include_symbols option to this iterator to fix the console.table case specifically. With this change the iterator never yields symbols, so that option becomes unnecessary; the rest of that PR (dedup, Set handling, depth, column order) is independent.

Verification

New tests, all failing on 1.4.0 and passing with this change:

• test/js/bun/toml/toml.test.ts, test/js/bun/yaml/yaml.test.ts, test/js/bun/json5/json5.test.ts: stringify skips symbol keys (including well-known symbols)
• test/js/bun/spawn/spawn-env.test.ts: symbol-keyed env entries do not reach the child
• test/js/node/http2/node-http2.test.js: neither http2.sensitiveHeaders nor an arbitrary symbol key is sent on the wire
• test/js/bun/console/console-table.test.ts: symbol-keyed columns are dropped (matches node), plus a guard that Bun.inspect still prints [Symbol(SYMK)]

One existing test (yaml.test.ts "handles symbols") had captured the buggy output; its own comment said symbol keys should not appear. Updated.

Full runs of all touched files are green locally (348 pass in node-http2.test.js; the only failures are pre-existing debug-ASAN 5s timeouts in the TOML deep-nesting and parseArgs stress tests, which pass on release and are unrelated).

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/json5/json5.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e2cd930d-60b4-45d6-8807-a092de2f114d

📥 Commits

Reviewing files that changed from the base of the PR and between 5fa371a and 782886a.

📒 Files selected for processing (7)

• src/jsc/bindings/JSPropertyIterator.cpp
• test/js/bun/console/console-table.test.ts
• test/js/bun/json5/json5.test.ts
• test/js/bun/spawn/spawn-env.test.ts
• test/js/bun/toml/toml.test.ts
• test/js/bun/yaml/yaml.test.ts
• test/js/node/http2/node-http2.test.js

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[claude]

I didn't find any issues — the one-token change to PropertyNameMode::Strings is well-justified (property names cross the FFI as plain strings, so callers couldn't distinguish symbols anyway) and restores the pre-#10161 JSObjectCopyPropertyNames semantics that src/js/node/http2.ts already assumes in five separate comments. Deferring to a human because the blast radius is ~20 call sites across serializers, spawn/shell env, HTTP/2 wire encoding, FFI, bundler config, and expect matchers.

What was reviewed:

• Verified src/js/node/http2.ts already documents "the native header walk skips symbol keys" at 5 sites — this PR makes that true.
• Spot-checked h2_frame_parser.rs, expect.rs (expect.extend), toBeEmpty.rs, pretty_format.rs — none want symbol keys; Bun.inspect's symbol rendering goes through forEachProperty, not this iterator (guarded by the new console-table test).
• The updated yaml.test.ts "handles symbols" case had captured the buggy output; its own comment agreed the new behavior is correct.

Extended reasoning...

Overview

A single-token change in src/jsc/bindings/JSPropertyIterator.cpp: PropertyNameMode::StringsAndSymbols → PropertyNameMode::Strings. Six test files add coverage for TOML/YAML/JSON5 stringify, spawn env, HTTP/2 headers, and console.table, plus a fix to one existing yaml test whose snapshot had captured the buggy output.

Security risks

The bug being fixed is itself the security-adjacent behavior: http2.sensitiveHeaders (a symbol-keyed marker for never-index HPACK headers) was being serialized onto the wire as a literal nodejs.http2.sensitiveheaders header. The fix removes that leak. No new attack surface is introduced — the change strictly narrows what property names are enumerated, and the FFI already couldn't represent symbols (they crossed as their description string).

Level of scrutiny

Medium-high. The native diff is trivial, but JSPropertyIterator is consumed by ~20 Rust call sites (serializers, spawn/shell env, h2 headers, routes, bundler/transpiler config, ffi symbols, parseArgs, expect, console.table, JSX props, valkey, ini, macros). The PR description surveys all of them and I spot-checked several: h2_frame_parser.rs (the wire encoder — matches the http2.ts assumption), expect.rs (expect.extend — matcher names are strings), toBeEmpty.rs (jest-extended uses Object.keys, so string-only is the correct semantics), and pretty_format.rs (its symbol handling goes through the separate forEachProperty callback with an is_symbol flag, not this iterator). The PR also correctly notes this restores the behavior of the JSC C API (JSObjectCopyPropertyNames) that #10161 replaced.

Other factors

The added tests are well-constructed (each asserts an exact value, drains subprocess pipes concurrently, and would fail on the unfixed build). One existing test was updated, and its own inline comment already said the old expected value was wrong. src/js/node/http2.ts contains five separate comments stating "the native header walk skips symbol keys", so the codebase already assumed this fix. Given the breadth of affected subsystems and the wire-protocol implications, a human should confirm the call-site survey before merging.

[dylan-conway]

are there any places we want JSPropertyIterator to include Symbols? or we always want them skipped? maybe make this an option our property iterator accepts?

[robobun]

None today. I went through all 20 JSPropertyIterator::init call sites before making it unconditional, and every one of them wants string keys:

• Serializers (TOML/YAML/JSON5, the ini writer): JSON.stringify semantics; js-yaml, smol-toml, @iarna/toml, and the json5 reference implementation all skip symbol keys too
• Bun.spawn env and shell env: node builds the child env from ObjectKeys(env), so symbol entries never reach the child
• node:http2 header walks: must skip symbols or http2.sensitiveHeaders lands on the wire; the comments in http2.ts already assume they are skipped
• Config readers (Bun.serve routes, bundler define/loader/files, transpiler define/replace, bun:ffi symbols, parseArgs, valkey hset, macro object-to-AST): keys are names or paths by contract, so a symbol description surfacing as one is always a bug
• console.table columns/rows: node derives columns from ObjectKeys, symbol keys produce no column
• expect.extend and toBeEmpty: jest iterates matchers by string name, and jest-extended's toBeEmpty is Object.keys(value).length === 0
• Error-property printing in VirtualMachine.rs and JSX props in inspect/pretty_format: display only, and their skip-lists (message, name, stack, children) compare against string keys

On making it an option: the catch is that this iterator returns the name as a bun.String, so a caller that opted in could not tell Symbol("a") from the string key "a". That ambiguity is precisely how the http2/env/serializer leaks happened. We already have a symbol-aware walk for callers that genuinely need them: forEachProperty / for_each_property_ordered passes is_symbol and is_private_symbol flags, and it is what Bun.inspect and the test runner's pretty printer use to render [Symbol(foo)]: bar.

So my recommendation is to keep this iterator strings-only with no flag, and point future symbol-needing callers at forEachProperty. If we ever do want symbols through this iterator, the option should land together with an is-symbol out-param on next() so the ambiguity cannot come back. Happy to add include_symbols (default false) now instead if you'd rather have the escape hatch in place; it would just be unused until a caller sets it.

Related: #34241 adds an include_symbols flag (default true) with console.table opting out; with this PR that flag becomes unnecessary, while the rest of that PR (dedup, Set handling, depth, column order) is unaffected.

[github-actions]

This PR may be a duplicate of:

1. console.table: match Node's column model (symbol keys, properties dedup, Set, depth, key order) #34241 - Modifies the exact same PropertyNameArrayBuilder line in src/jsc/bindings/JSPropertyIterator.cpp to stop symbol keys from being enumerated as their string description, just plumbed through an opt-in include_symbols flag rather than made unconditional.

🤖 Generated with Claude Code

[robobun]

Not a duplicate. #34241 is a console.table column-model fix; it touches this line by adding an include_symbols option that defaults to true, so symbols keep flowing to every other consumer (spawn env, the TOML/YAML/JSON5 serializers, node:http2 wire encoding). This PR makes the iterator strings-only unconditionally, which fixes those leaks and makes that option unnecessary. Both PRs are cross-linked, and the rest of #34241 (properties dedup, Set handling, depth, column order) is independent of this change.

[dylan-conway]

does this fix any issues?

[robobun]

Not that I can find. I searched open and closed issues for the observable symptoms (a literal nodejs.http2.sensitiveheaders header, symbol keys in YAML/TOML/JSON5 stringify output, symbol env entries reaching spawned children, symbol columns in console.table) and nothing filed tracks them.

Closest hit is #25771 (got with http2: true), but its error comes from http2-wrapper's origin-set check, not from the header leak, so I'm not claiming it here.

A likely reason nobody filed the http2 one: servers ignore unknown headers, so the extra header is invisible unless you capture the wire or the server is strict, even though it leaks the list of which header names the client considers sensitive. The other surfaces need a symbol-keyed object to begin with (rare), and TOML/JSON5 stringify are canary-only. This started from reading the iterator rather than from a user report.

[robobun]

^{Updated 9:05 PM PT - Aug 5th, 2026}

✅ @robobun, your commit 782886a091650fa46ff96484573b2f18af377632 passed in Build #89415! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37010

That installs a local version of the PR into your bun-37010 executable, so you can run:

bun-37010 --bun

[robobun]

Issue #40388 reports the same bug for TOML, JSON5, and YAML stringify, including the TOML round-trip failure (Cannot redefine key). This PR covers it.

[robobun]

Superseded by #41910, which landed the same change: Bun__JSPropertyIterator__create requests PropertyNameMode::Strings by default, with an opt-in include_symbols for the two display-only callers (expect().toBeEmpty() and the uncaught-error printer). It also landed the macro, http2 sensitive headers, Bun.spawn env and YAML tests.

I checked every test case from this PR against a debug build of main at aecbe19. All six pass. The three that main did not cover yet (Bun.TOML.stringify, Bun.JSON5.stringify, console.table) are ported as a test-only change in #41940.

Closing.