JSPropertyIterator: do not yield Symbol keys as strings (macros, http2 sensitiveHeaders, spawn env)

[robobun]

Problem

• Native code that walks a JS object's properties receives every Symbol key as a string: the symbol's description. A macro returning { [Symbol.for("s")]: 1, v: 2 } is inlined as { v: 2, s: 1 }. An http2 request with [http2.sensitiveHeaders]: ["cookie"] sends a literal nodejs.http2.sensitiveheaders: cookie header on the wire (node sends none). Bun.spawn({ env: { [Symbol("K")]: "v" } }) sets K=v in the child. console.table and Bun.YAML.stringify show the key.
• The cause: Bun__JSPropertyIterator__create (src/jsc/bindings/JSPropertyIterator.cpp) asks JSC for PropertyNameMode::StringsAndSymbols, and getNameAndValue hands prop.impl() to Rust as a BunString. For a Symbol that impl is the description.

Fix

• Add include_symbols to JSPropertyIteratorOptions, default false, which selects PropertyNameMode::Strings. That is Object.keys semantics, which every consumer that turns keys into names wants.
• Two callers keep symbols because they only count or display: expect().toBeEmpty() (jest-extended's equals({}, x) sees enumerable Symbol keys) and the extra-properties block of the uncaught-error printer.
• Verified: test/bundler/transpiler/macro-test.test.ts, test/js/node/http2/node-http2.test.js, test/js/bun/spawn/spawn-env.test.ts, each failing on 1.4.2. test/js/bun/yaml/yaml.test.ts asserted the old output and is updated. Also ran expect, inspect, serve, console-table, json5, toml and xml suites.

Background

• JSPropertyIterator (src/jsc/JSPropertyIterator.rs) wraps a JSC PropertyNameArray. About twenty call sites use it: spawn env, Bun.serve routes, http2 headers, FFI symbol maps, Bun.build config, YAML/TOML/JSON5/XML stringify, macros, console.table, test matchers.
• A JSC Symbol property name wraps a SymbolImpl, a StringImpl whose characters are the description, so Bun::toString(prop.impl()) yields the description.
• console.log prints [Symbol(desc)] keys through a different path (for_each_property with an is_symbol flag) and is unchanged.

Notes

• http2 repro:

  import http2 from "node:http2";
  const server = http2.createServer((req, res) => res.end(JSON.stringify(Object.keys(req.headers))));
  server.listen(0, () => {
    const client = http2.connect(`http://localhost:${server.address().port}`);
    const req = client.request({ ":path": "/", cookie: "a=b", [http2.sensitiveHeaders]: ["cookie"] });
    req.setEncoding("utf8"); let d = ""; req.on("data", c => (d += c));
    req.on("end", () => { console.log(d); client.close(); server.close(); }); req.end();
  });
  // bun 1.4.2: [":path",":method",":authority",":scheme","cookie","nodejs.http2.sensitiveheaders"]
  // node:      [":path",":method",":authority",":scheme","cookie"]

  src/js/node/http2.ts leaves the symbol on the headers object on purpose and notes that "the native header walk skips symbol keys". It only skipped description-less symbols (empty name).
• The uncaught-error printer still shows a Symbol-keyed own property by its description (secret: "x" where node prints [Symbol(secret)]: 'x'). Printing it properly needs an is_symbol signal through the iterator; left as is.

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{On-demand reviews are free for the next 13 days. After that, they cost $0.25 per reviewed file.}

Or wait 18 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 3c8d9150-aed4-40be-8817-8e014ebe1093

📥 Commits

Reviewing files that changed from the base of the PR and between b52d3e5 and 709a284.

📒 Files selected for processing (11)

• src/jsc/JSPropertyIterator.rs
• src/jsc/VirtualMachine.rs
• src/jsc/bindings/JSPropertyIterator.cpp
• src/runtime/api/JSTranspiler.rs
• src/runtime/test_runner/expect.rs
• src/runtime/test_runner/expect/toBeEmpty.rs
• test/bundler/transpiler/macro-test.test.ts
• test/bundler/transpiler/macro.ts
• test/js/bun/spawn/spawn-env.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.}

[claude]

I reviewed this PR and didn't find any bugs. The fix is at the right layer and the FFI plumbing checks out, but flipping the iterator's default from StringsAndSymbols to Strings fans out to ~20 native call sites (spawn env, http2 headers, Bun.serve routes, FFI symbol maps, YAML/TOML/JSON5/XML stringify, macros, console.table, Bun.build config), so a human sanity-check on the opt-in/opt-out choices is still worthwhile.

What was reviewed:

• FFI signature: Bun__JSPropertyIterator__create gains a trailing bool include_symbols on both the Rust extern "C" and the C++ definition — parameter order and types match.
• Struct-literal call sites: the four full literals (JSTranspiler const, expect.extend, VirtualMachine error printer, toBeEmpty) are updated; every other caller uses ..Default::default() or ::new(), so they compile and pick up include_symbols: false.
• Opt-ins: toBeEmpty keeps symbols (jest-extended equals({}, x) sees enumerable Symbol keys) and the uncaught-error extra-properties printer keeps symbols for display — both justified in comments.
• Existing assertions on the old behavior: grepped the serializer/console/http2 suites; only yaml.test.ts asserted a Symbol-key leak, and it's updated here.

Extended reasoning...

Overview

The PR adds an include_symbols: bool field to JSPropertyIteratorOptions (default false), threads it through the Rust FFI wrapper into Bun__JSPropertyIterator__create in src/jsc/bindings/JSPropertyIterator.cpp, and uses it to select PropertyNameMode::Strings vs StringsAndSymbols when building the JSC PropertyNameArray. Previously the mode was unconditionally StringsAndSymbols, and because the iterator hands prop.impl() back to Rust as a BunString, a Symbol key surfaced as its description string — so [Symbol("K")]: v on a spawn env object became env var K=v, [http2.sensitiveHeaders] became a literal nodejs.http2.sensitiveheaders header on the wire, and macro-inlined objects gained string keys named by the symbol description. Four full struct-literal sites are updated explicitly; the remaining ~16 callers use ..Default::default()/::new() and inherit the new false default. Two callers deliberately opt in: toBeEmpty (jest-extended semantics count enumerable Symbol keys) and the VirtualMachine uncaught-error extra-properties dump (display only). Regression tests are added to the existing spawn-env, http2, and macro test files, and the one YAML test that asserted the old leak is corrected.

Security risks

Low, and in the direction of tightening. The prior behavior could leak Symbol-keyed values into child-process environments and onto the wire as HTTP/2 headers named by the symbol description — this change stops that. No new attack surface is introduced; the FFI change is a trailing bool on an internal binding, and the C++ side only uses it to pick a JSC enum value.

Level of scrutiny

Medium-high. The fix itself is small and mechanically clean, but it flips a default on a shared helper with roughly twenty consumers spanning Bun.spawn env, node:http2 header encoding, Bun.serve route config, FFI symbol maps, four serializers, macro inlining, console.table, Bun.build config, and test matchers. Per REVIEW.md ("before changing anything shared, enumerate every consumer"), each of those callers now silently gets Object.keys semantics instead of Reflect.ownKeys. That is almost certainly what every key-as-name consumer wants — a Symbol description is not a property key — but the breadth of the default flip is the reason to keep a human in the loop rather than auto-approve.

Other factors

I verified the FFI extern matches parameter-for-parameter across Rust and C++ (the REVIEW.md hard rule for cross-language ABI). Rust's exhaustive struct initialization guarantees no full-literal call site was missed. The new tests follow harness conventions (bunExe/bunEnv, await using, Promise.all on stdout/exited, stdout asserted before exit code, listen(0), error events wired to reject, cleanup in finally). I grepped for other tests asserting the old Symbol-leak output across TOML/JSON5/XML/console.table and found none beyond the updated YAML case. The PR description's acknowledged leftover — the error printer still shows a Symbol key by its description rather than [Symbol(desc)] — is pre-existing behavior explicitly preserved via include_symbols: true, not a regression. No CODEOWNERS cover the changed paths, and the bug hunt ran to a dry streak with no findings.