Repository navigation
Conversation
|
Updated 4:29 AM PT - Sep 8th, 2026
❌ @robobun, your commit 5f019a6 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 41614That installs a local version of the PR into your bun-41614 --bun |
…ees it sendTrailers, additionalHeaders, respond, pushStream and request validated each field and encoded it into the shared HPACK dynamic table one at a time. A later field that failed validation threw after the earlier fields were already in the table and never sent, so the peer's table fell behind and every later header block on the connection decoded against the wrong entries. The JS walk now collects the validated fields into a HeaderList and the encode runs once the whole object is known. request() also checks maxSendHeaderBlockLength against the pre-compression bound, as nghttp2 does, and runs the option and memory-limit checks before the encode, so a refused block never touches the table either. A client request() that throws no longer emits a session 'error' as well.
…mitErrorNT helper
… bound like nghttp2 Also complete a pending close() when the last request throws during submission, and drop the undefined-value test rows that #37953 changes.
f7dc184 to
90f334a
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/jsc/JSPropertyIterator.rs— nit: addinginclude_symbolsleaves three doc comments counting the wrong number of options — sweep:remaining three|32 instantiations. Line 28 ("the remaining three options take the [Default] values") and line 55 ("the remaining three options") now describe four defaulted options, and line 12's "32 instantiations" would be 64 with six bool fields. Fix: update each count (three→four, 32→64) or reword to avoid the literal count.Extended reasoning...
JSPropertyIteratorOptionsgrew from five to six bool fields.JSPropertyIteratorOptions::new(line 29) and theFrom<PropertyIteratorOptions>shorthand (line 63) take two fields and default the rest — the doc comments at lines 27-28 and 55 still say "remaining three", and the rationale comment at line 12 still cites the 2^5=32 monomorphization fan-out. Purely a documentation drift; no runtime effect.Verification: nit — documentation drift only, no runtime effect.
JSPropertyIteratorOptionsnow has six bool fields (lines 16-23:skip_empty_name,include_value,own_properties_only,observable,only_non_index_properties,include_symbols). The PR updated theDefaultdoc at line 10 to listinclude_symbols = true, but left three literal counts stale: - Line 12: "the monomorphization fan-out… -
🟣
src/js/node/http2.ts— pre-existing: the JS name-validation loop inClientHttp2Session.request()still validates every key regardless of value, soclient.request({':path':'/','bad name':undefined})throwsERR_INVALID_HTTP_TOKEN(and{':bogus':undefined}throwsERR_HTTP2_INVALID_PSEUDOHEADER) where Node skips the field — the same undefined-skip this PR adds to the native walk and toassertNoConnectionHeadersis missing here. Fix: skip keys whose value isundefinedbefore callingassertValidPseudoHeader/checkIsHttpToken, matching theif (value === undefined) continuethat Node'smapToHeadersapplies before any name check.Extended reasoning...
Node's
mapToHeaders(lib/internal/http2/util.js) doesif (value === undefined || key === '') continue;before lowering the key or running any pseudo-header/token/connection check, so an undefined-valued field is invisible. This PR aligns the native encoders (is_undefined()skip at h2_frame_parser.rs:5781/6223/6763) andassertNoConnectionHeaders(http2.ts:3770) with that rule, but the synchronous pre-validation loop at http2.ts:6048–6062 still iteratesObjectKeys(headers)and callsassertValidPseudoHeader(name)/checkIsHttpToken(name)without readingheaders[name]. On base this path already threw; after this PR it is the last remaining place a{':path':'/', 'x invalid': undefined}request diverges from Node — the native fix that would now let it pass never runs because the JS guard fires first. REVIEW.md: "Fix the whole class in the same PR … every sibling site sharing the pattern".Verification: pre-existing — the loop at src/js/node/http2.ts:6048–6062 is byte-identical on the base branch (verified via
git show f7dc184a…:src/js/node/http2.ts), so the base already throws on{'bad name': undefined}/{':bogus': undefined}by the same route; merging makes nothing worse. The defect is real: the loop does `const name = headerNames[i]; if (name === "") continue; if (name.charCodeAt(0)…
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // RFC 9113 §8.2.2, checked the way node's mapToHeaders does: before anything is encoded, an | ||
| // undefined value is not a field, a one-element array is its element, and `te` is legal only | ||
| // as `te: trailers`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // nghttp2 refuses a nonzero content-length on a request whose HEADERS carry END_STREAM | ||
| // (no payload can follow): reset with PROTOCOL_ERROR after creation (an async stream | ||
| // error, not a throw). `content-length: 0` matches the empty payload and goes through. | ||
| // An explicit endStream:false keeps the body legal, so only the ended case rejects. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// When false, symbol-keyed properties are not visited (the `Object.keys` view). When | ||
| /// true, a symbol key is yielded under its description string. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Validated outbound header fields, collected before any of them reach the HPACK encoder. | ||
| /// An encoded field that is never sent desyncs the peer's dynamic table for the rest of the | ||
| /// connection, so a validation throw must happen before the first `encode()`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Every field was skipped (undefined values): nghttp2 peers reject a zero-length header | ||
| // block, so end the stream the way noTrailers() does, with an empty DATA frame. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Checked against the pre-compression bound like nghttp2 (which always counts the | ||
| // priority fields for HEADERS), before the encoder is touched. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The three points from the earlier review are addressed by 045a254 — verified the new pending.fields.is_empty() → empty-DATA branch in send_trailers, the hoisted value.length === 0 skip now runs before the kForbiddenConnectionHeaders lookup so {connection: []} passes, and the error is routed through $ERR_HTTP2_INVALID_CONNECTION_HEADERS. Also checked the added headers[name] === undefined skip in the request() pseudo-header pre-scan against Node's mapToHeaders ordering (undefined-before-name-check) — matches.
Extended reasoning...
Re-read the delta since the prior review (589891f..045a254) and confirmed each of the three earlier inline threads is fixed in code with a covering test added to node-http2.test.js. The one new inline finding this run is a side effect of that same fix commit — hoisting the array-unwrap out of the te-only branch also hoisted the unconditional template-stringify — so noting that the rest of 045a254 checks out is the useful non-redundant context here.
| // RFC 9113 §8.2.2 with node's mapToHeaders field rules. Only a `te` value is read, so a user | ||
| // toString() runs once, in the native walk. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
Re-reviewed after 14d1c28 — all four earlier points are addressed (empty-array skip hoisted before the forbidden-name check, $ERR_HTTP2_INVALID_CONNECTION_HEADERS now used, te is the only value stringified so non-te header values are coerced once, and the all-undefined trailer object takes the noTrailers DATA+END_STREAM path). Nothing new found on this push.
What was reviewed: the reworked assertNoConnectionHeaders — {connection: []} and {te: ["trailers"]} now pass, {te: ["a","b"]} still throws; the $ERR_* call matches the existing full-message convention at src/js/internal/quic/http2util.ts:158; the new pending.fields.is_empty() branch in send_trailers mirrors the existing no_trailers write. Also looked at writeEarlyHints still throwing on undefined values — that's the pre-existing assertValidHeader contract, not a drift this PR introduces.
Given the FFI signature change to Bun__JSPropertyIterator__create and the HPACK encode-ordering rework, a maintainer sign-off is still the right bar.
Extended reasoning...
Overview
This is the third pass over this PR. Since the second review, one commit landed (14d1c28) that reshapes assertNoConnectionHeaders in src/js/node/http2.ts: the empty-array skip now runs before the forbidden-name lookup (so {connection: []} no longer throws), the hand-rolled TypeError + .code assignment is replaced with $ERR_HTTP2_INVALID_CONNECTION_HEADERS(...), and value stringification is deferred to the te branch only, so a header value with an observable toString is coerced once (in the native walk) rather than twice. The undefined-value skip was also added to the client request() pseudo-header pre-scan. Together with 045a254 (the empty-pending → noTrailers path in h2_frame_parser.rs), every point raised in the two earlier reviews is closed.
Security risks
No new attack surface beyond what earlier passes covered. The change is a Node-compat normalization of outbound HTTP/2 headers plus a validate-before-encode reordering that removes an HPACK table desync. The $ERR_* call passes the full formatted message, which matches the two-element [code, TypeError] registration in ErrorCode.ts and the existing call shape at src/js/internal/quic/http2util.ts:158.
Level of scrutiny
High — unchanged from earlier passes. The PR threads a new boolean through a C++/Rust FFI boundary (Bun__JSPropertyIterator__create), reorders when the shared HPACK encoder is mutated, and touches five outbound-header entry points. That is protocol-state code where a mistake corrupts every subsequent stream on a connection, so it warrants a maintainer look regardless of automated findings.
Other factors
The writeEarlyHints path still rejects undefined/null values via assertValidHeader — flagged as pre-existing and out of scope for this normalization PR (Node's writeEarlyHints also validates through validateHeaderValue, so this is not obviously a drift). The github-actions bot left six inline comments on 045a254 whose content is not visible here; 14d1c28 post-dates them and may have addressed them. No CHANGES_REQUESTED reviews are outstanding.
|
Status: all review findings are in.
CI on 14d1c28: test/js/node/http2/ is green on every lane. The red lanes are binary-size (the same android, freebsd and musl deltas show on the base branch #41520 against the same canary baseline) and test-crypto-dh-leak.js, which is red on main. Two of the earlier runs lost unrelated test files to a crashed parallel-batch worker. Verified locally with the debug build: test/js/node/http2/ (534 pass) and node's 256 test-http2-*.js files. |
…2 sensitiveHeaders, spawn env) (#41910) ### 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. <details><summary>Notes</summary> - http2 repro: ```js 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. </details> (cherry picked from commit 26fba3a)
The native header walks (request, pushPromise, sendTrailers) skip an undefined value and stringify null and array items the way node's mapToHeaders does. A trailer object whose every value is undefined ends the stream with an empty DATA frame instead of a zero-length header block. Connection-specific headers throw ERR_HTTP2_INVALID_CONNECTION_HEADERS from request(), respond(), additionalHeaders() and sendTrailers() with node's te rules. A zero content-length on an ended request is sent. writeEarlyHints validates its fields like node.
14d1c28 to
5f019a6
Compare
| /// `observable = true`, `only_non_index_properties = false`, | ||
| /// `include_symbols = false`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Also yield Symbol-keyed properties. The yielded name is the symbol's | ||
| /// description, not a property key, so this is only for callers that | ||
| /// count or display properties. Off by default, like `Object.keys`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The iterator yields every name as a string. A Symbol key has no string | ||
| // name: its impl() is the description, so `Symbol.for("s")` would come | ||
| // out as the key "s". Only callers that merely count or display | ||
| // properties opt in to symbols. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // jest-extended compares against `{}` with an equality | ||
| // that sees enumerable Symbol keys, so count them too. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Restacked after #41910 landed on main (the Verified with the debug build at 5f019a6: |
There was a problem hiding this comment.
All four points from my earlier passes are now addressed — the empty-trailers path sends DATA+END_STREAM, assertNoConnectionHeaders skips empty-array values before the forbidden check and throws via $ERR_HTTP2_INVALID_CONNECTION_HEADERS, and the te-only coercion avoids the double toString(). I found nothing new this round; given the FFI signature change and the stacked base, a maintainer sign-off is still the right next step.
What was reviewed
JSPropertyIteratorinclude_symbolsplumbing: Rust extern signature matches the C++ definition parameter-for-parameter; opt-in sites (VirtualMachinedump,toBeEmpty) preserve prior behavior.assertNoConnectionHeadersrewrite against Node'smapToHeadersordering (undefined/empty-array skip → forbidden-name →tevalue); confirmed value coercion is scoped toteonly.- The
pending.fields.is_empty()early-return insend_trailers— mirrors the existingnoTrailers()shape and is covered by the new "every value is undefined" test.
Extended reasoning...
Overview
This PR does two things. First, it threads an include_symbols flag through JSPropertyIterator (Rust FFI + C++), defaulting to PropertyNameMode::Strings so Symbol keys stop leaking as their description string into HTTP/2 headers, Bun.spawn env, YAML output, and macro inlining; the two callers that legitimately count symbols opt in. Second, it aligns node:http2 outbound header normalization with Node's mapToHeaders: undefined values are skipped, null and array items stringify, connection-specific headers throw ERR_HTTP2_INVALID_CONNECTION_HEADERS up front (before HPACK encoding), content-length: 0 on an END_STREAM GET is allowed, and writeEarlyHints validates header names. The change is stacked on #41520.
Security risks
No new attack surface. The change tightens header validation on the send side (rejecting connection-specific headers synchronously) and stops Symbol-keyed properties from reaching the wire or a child process's environment — both are strict reductions in what leaves the process. The FFI signature change adds one bool at the end of an extern "C" call; the Rust and C++ declarations were checked to match. No auth, crypto, or path-traversal surface is touched.
Level of scrutiny
Moderate-to-high. The JSPropertyIterator default flip is a cross-cutting behavior change with ~40 call sites, and the HTTP/2 header walks sit directly in front of a shared HPACK dynamic table where a mid-encode throw desyncs the connection. The PR is also stacked on an unmerged base. That combination warrants a maintainer look even with clean automated review — particularly to confirm the include_symbols: false default is what the other iterator consumers (not touched here) actually want, and that the base PR's HeaderList collect-then-encode contract holds for the new skip paths.
Other factors
This is my fourth pass. The two commits pushed since the last review (97b6428, 5f019a6) directly address every inline finding I raised: the all-undefined-trailers zero-length-HEADERS bug, the empty-array connection false rejection, the hand-rolled error construction, and the double toString() coercion. Each fix has a corresponding new test in node-http2.test.js. Nothing new surfaced this run; I'm deferring rather than approving only because of the FFI/compat scope and the stacked-PR merge order, not because of any open concern with the code as it stands.
|
CI on 5f019a6 is green on this diff: the only red lane is test/js/node/test/parallel/test-crypto-dh-leak.js, which is red on main (an RSS assertion under ASAN), plus one flaky fetch-leak retry. test/js/node/http2/ passes on every lane. The review findings from the earlier passes are all in (empty-trailer DATA path, empty-array connection header, $ERR_ codegen, te-only coercion). Ready for a maintainer. Merge order: #41520 first, then this. |
|
Final CI state for 5f019a6 (build 112735): two red jobs, neither touches this diff.
test/js/node/http2/ is green on every lane. Ready for a maintainer; merge #41520 first. |
90f334a to
57fa470
Compare
Stacked on #41520 (the base branch of this PR). Merge #41520 first. The first commit is a cherry-pick of #41910 (already on main) and disappears once the base branch is rebased past it.
Problem
src/runtime/api/bun/h2_frame_parser.rs(request,push_promise,send_trailers) read the user's header object field by field. Node normalizes the whole object in JS first (mapToHeaders). Three results differ from node v26: anundefinedvalue throwsERR_HTTP2_INVALID_HEADER_VALUE(node skips the field),nullthrows (node sends"null"), and an array item that isundefinedornullthrows (node sends"undefined"or"null"). A node client getsERR_HTTP2_INVALID_HEADER_VALUEor a 500 where node to node gets a 200.connection,te: gzip) go on the wire and the peer resets the stream (node throwsERR_HTTP2_INVALID_CONNECTION_HEADERSat once), a client request withcontent-length: 0is reset before it is sent (http2.tsrejected every content-length on an ended request, nghttp2 rejects only a nonzero one), andwriteEarlyHintsacceptedundefinedvalues and invalid tokens.Fix
undefinedvalue and stringifynulland array items the way${value}does. A trailer object whose every value isundefinedends the stream with an empty DATA frame (thenoTrailers()shape), because nghttp2 peers reject a zero-length header block.assertNoConnectionHeadershas node'sterules (an empty array is no field, a one-element array is its element, only atevalue is read) and runs fromrequest(),respond(),additionalHeaders()andsendTrailers(). The content-length check reads the value.writeEarlyHintsvalidates like node v26.test/js/node/http2/node-http2.test.js(8 new tests, all fail on 1.4.3). Also the rest oftest/js/node/http2/,h2-conformance.test.ts, and node's 256test-http2-*.jsfiles.Background
mapToHeaders(lib/internal/http2/util.jsin node) is the one JS function that turns a header object into the flat list nghttp2 encodes. It skipsundefinedand empty arrays, stringifies everything else, and rejects connection-specific fields (RFC 9113 §8.2.2) before anything reaches the encoder.http2.sensitiveHeadersis a symbol key on the header object. JSPropertyIterator: do not yield Symbol keys as strings (macros, http2 sensitiveHeaders, spawn env) #41910 keeps symbol keys off the wire, so this PR no longer carries its own iterator option.Supersedes #37953 (undefined values) and #33458 (connection headers), both closed. This PR also records the
nulldecision:nullis sent as"null", like node, which retires the pushStream exception noted in #37579.Notes
Probe:
stream.respond()/client.request()with each header face, node v26.3.0 against bun 1.4.3 and this branch, bun to bun and bun client to node server.{"x-a": undefined, "x-b": "ok"}x-b: okERR_HTTP2_INVALID_HEADER_VALUEx-b: ok{"x-a": null}x-a: nullx-a: null{"x-a": ["1", undefined, "2"]}1, undefined, 21, undefined, 2{connection: "keep-alive"}ERR_HTTP2_INVALID_CONNECTION_HEADERSERR_HTTP2_STREAM_ERROR{te: "gzip"}{connection: []},{te: []}content-length: 0ERR_HTTP2_STREAM_ERROR(rst 1)content-length: 5ERR_HTTP2_STREAM_ERROR(rst 1)sendTrailers({"grpc-message": undefined})The one remaining difference is a value with CR, LF or NUL on
client.request(): node sends it and the receiving nghttp2 drops the field, bun throwsERR_HTTP2_INVALID_HEADER_VALUE. That is an existing decision (see the pushStream CR/LF test) and this PR keeps it.The pushStream test "reports an unsendable array element through the callback" used
["ok", null]as a second poison. Under node semantics that push succeeds withx-custom: okandx-custom: null, and the test now asserts the pushed block. The"null value"row of the #41520 poison matrix is removed for the same reason.The raw
[name, value, ...]header form ofrequest()keeps its current rule (anundefinedornullvalue skips the pair). Node has no raw form forrequest().Review history: an all-undefined trailer object produced a zero-length HEADERS frame (now the empty DATA path),
{connection: []}threw (an empty array is now no field for any name), the request() name pre-check ran on undefined values (now skipped first, like mapToHeaders), the error bypassed the$ERR_codegen (now uses it), and the JS check coerced every value so a usertoString()ran twice (now only atevalue is read;h2-conformance.test.tscovers the single call).Earlier shape: this branch first carried its own
include_symbolsiterator option and a duplicate of the collect-then-encode change. Both were dropped in favour of #41910 and #41520.Suites run with the debug build:
bun bd test test/js/node/http2/and node'stest/js/node/test/parallel/test-http2-*.js(256 pass).