Repository navigation
Conversation
|
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 (2)
WalkthroughChangesHTTP/2 client requests and server push now coerce header values before stream allocation. Pre-coerced wire headers preserve re-entrant ordering, while original header objects remain available for observable state. Conformance tests cover ordering, copying, and synchronous errors. HTTP/2 header coercion
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on main (da3851e) with the raw-server script in the description: bun writes HEADERS for streams CI: the final build (93516) has 179 of 181 jobs passed and no test failures; the remaining two are the darwin 26 aarch64 test jobs, which expired without ever getting an agent (the same lane did that on the two builds before it too). The only entries in its failure list are unrelated tests that passed on retry. Ready for review. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the hot request() path in node:http2 and shifts when pushStream() header-coercion errors surface (callback → synchronous throw), a human look would still be worthwhile.
What was reviewed:
toWireHeaders/coerceHeaderValue: primitives (incl.null/undefined/symbols) pass through untouched so native validation errors are unchanged; arrays are freshly copied; the lazywire = { ...headers }correctly overwrites every non-primitive key and preserves thesensitiveHeaderssymbol.- Call sites:
headersis already a local spread copy in bothrequest()andpushStream(), so re-entrant calls can't mutate the object being iterated;sentHeaders/diagnostics channels still receive the uncoercedheaders. #flushPendingRequests()already readswireHeadersfrom the queued entry, so pre-coerced values flow through the deferred path unchanged.- Common-path cost: one
ObjectKeyswalk with no allocation when all values are primitives; array values always allocate (intentional per the PR, defends against proxies/accessors).
Extended reasoning...
Overview
This PR fixes stream-id ordering when a header value's toString() re-enters request()/pushStream() on the same session. It adds a JS-side toWireHeaders() helper (src/js/node/http2.ts) that coerces object/function header values to strings before getNextStream() allocates an id, applies it at the three call sites (ClientHttp2Session.request() immediate and pending-queue paths, ServerHttp2Stream.pushStream()), and adds seven wire-level tests in test/js/node/http2/h2-conformance.test.ts covering the object/array-element/raw-array header forms, the pre-connect queue, an end-to-end echo, PUSH_PROMISE ordering, and throwing coercions.
Security risks
None identified. The change moves an existing string coercion earlier in the same synchronous call; it does not introduce new user-controlled data paths, and template-literal coercion is semantically equivalent to the native to_js_string it front-runs. The tests use local servers and port: 0.
Level of scrutiny
Medium-high. node:http2 is a Node-compat networking module (flagged in REVIEW.md under Node/Web compat), and request() is on every client call's hot path. The re-entrancy/HPACK reasoning is subtle enough that a maintainer familiar with the h2 rewrite should confirm the perf trade-off (always-copy for array values) and the intentional behavior change where a throwing header coercion in pushStream() now throws synchronously instead of reaching the callback.
Other factors
The PR description is unusually thorough — root cause traced to to_js_string in h2_frame_parser.rs, node's buildNgHeaderString cited as the reference behavior, verified against node v26.3.0, and it explicitly scopes out the respond()/sendTrailers() HPACK half as belonging to #33470. All seven new tests are stated to fail on the unfixed build. #flushPendingRequests() was checked and already destructures wireHeaders from the queued entry, so the pending path needs no further change. I verified ObjectKeys is a captured primordial (line 79) and $isArray is used, so the helper is tamper-safe per the builtins conventions. No prior human review comments on this PR.
…lement (#37579) ## Repro ```js const http2 = require("node:http2"); const server = http2.createServer(); server.on("stream", stream => { stream.pushStream({ ":path": "/p", "x-multi": ["a", "b"], "set-cookie": ["c1=1", "c2=2"] }, (err, push) => { if (err) throw err; push.respond({ ":status": 200 }); push.end(); }); stream.respond({ ":status": 200 }); stream.end(); }); server.listen(0, "127.0.0.1", () => { const client = http2.connect("http://127.0.0.1:" + server.address().port); client.on("stream", (pushed, headers) => { console.log(JSON.stringify({ "x-multi": headers["x-multi"], "set-cookie": headers["set-cookie"] })); pushed.resume(); client.close(); server.close(); }); client.request({ ":path": "/" }).resume(); }); ``` | | output | | --- | --- | | node v26.3.0 | `{"x-multi":"a, b","set-cookie":["c1=1","c2=2"]}` | | bun 1.4.0 / main | `{"x-multi":"a,b","set-cookie":["c1=1,c2=2"]}` | | this PR | `{"x-multi":"a, b","set-cookie":["c1=1","c2=2"]}` | (A receiver joins repeated fields with `", "`, so the missing space in `x-multi` shows a single field was sent; `set-cookie` shows it directly: two cookies collapsed into one invalid cookie.) ## Cause Every outbound header block in bun goes through a native encoder in `h2_frame_parser.rs`. `request()` (used by `session.request()`, `respond()`, `additionalHeaders()`) and `send_trailers()` have an array branch that encodes one field per element. `push_promise()`, which backs `pushStream()`, did not: it called `to_js_string()` on the value, so an array went through `Array.prototype.toString` and was sent as a single field joined with `","`. Node's `buildNgHeaderString` emits one field per element for every block, `pushStream()` included. The same missing branch meant `pushStream()` had no single-value enforcement at all: `{"content-type": ["a", "b"]}` was sent as `content-type: a,b`, `{":path": ["/a", "/b"]}` as `:path: /a,/b`, and `{"content-type": "a", "Content-Type": "b"}` as two fields. Node throws `ERR_HTTP2_HEADER_SINGLE_VALUE` synchronously from `pushStream()` for all three (verified against v26.3.0). ## Fix - `src/runtime/api/bun/h2_frame_parser.rs`: `push_promise()` gets the same value handling as `request()` and `send_trailers()`: an array encodes one field per element (each element validated like a single value, so an element with CR/LF/NUL or a null element is rejected with `ERR_HTTP2_INVALID_HEADER_VALUE` exactly as it is for `respond()`), and single-value fields are tracked with the same `strictSingleValueFields`-gated bitset the other encoders use. The never-index lookup is done once per header name and applied to every element. - `src/js/node/http2.ts`: `pushStream()` runs `assertSingleValueHeaders()` (the check `respond()` / `additionalHeaders()` / `request()` already run) before a promised stream id is reserved, so the violation throws synchronously from `pushStream()` like node, reserves no stream, and never reaches the encoder. Behavior checked against node v26.3.0 with a per-case probe: | `pushStream()` headers | node | main | this PR | | --- | --- | --- | --- | | `"x-multi": ["a", "b"]` | two fields | one field `a,b` | two fields | | `"set-cookie": ["c1=1", "c2=2"]` | two fields | one field `c1=1,c2=2` | two fields | | `"x-none": []` | nothing sent | `x-none: ` (empty value) | nothing sent | | `"x-num": [1, 2]` | `1`, `2` | `1,2` | `1`, `2` | | `"content-type": ["a", "b"]` | throws `ERR_HTTP2_HEADER_SINGLE_VALUE` | sent as `a,b` | throws | | `":path": ["/a", "/b"]` | throws | sent as `/a,/b` | throws | | `"content-type": "a", "Content-Type": "b"` | throws | both sent | throws | | same with `strictSingleValueFields: false` | two fields | one field | two fields | | `"x": ["ok", null]` | sends `null` (stringified) | one field `ok,` | callback `ERR_HTTP2_INVALID_HEADER_VALUE` | The last row is the one deliberate difference from node: rejecting a null element is what bun's `request()` / `respond()` already do for array elements, so `pushStream()` now matches the rest of bun rather than node's `String(null)`. Not changed here: a block that fails validation after some of its fields were already fed to the HPACK encoder still desyncs the encoder table from the peer. That is pre-existing (the same `{"x-a": "fine", "x-bad": "a\nb"}` push kills the session with COMPRESSION_ERROR before and after this change, and it applies to every encoder) and is what #33470 fixes by collecting the block before encoding; this change keeps `push_promise()`'s loop shape so that PR rebases mechanically either way. #37568 (coerce values in JS before taking the id) passes arrays through to the native encoder and is complementary. ## Verification Five tests added to `test/js/node/http2/node-http2.test.js`, asserting on the flat header list the client decodes from the wire (one name/value pair per field): - array value sends one field per element; empty array sends nothing; `[x]` sends one field (also for a pseudo-header) - every element of a `[http2.sensitiveHeaders]` header arrives never-indexed - the three single-value violations throw `ERR_HTTP2_HEADER_SINGLE_VALUE` synchronously, the callback is never invoked, and the next push still gets stream id 2 - `strictSingleValueFields: false` sends each element of a single-value header - an element containing LF, or a null element, is reported through the callback as `ERR_HTTP2_INVALID_HEADER_VALUE` and nothing is pushed All five fail on the current release (`USE_SYSTEM_BUN=1`) and pass with this change. The rest of `node-http2.test.js` (352 tests), `h2-conformance.test.ts`, `h2-push-refusal-staged.test.ts`, and the ported node push tests (`test-http2-server-push-stream*.js`, `test-http2-compat-serverresponse-createpushresponse.js`, `test-http2-respond-file-push.js`, `test-http2-server-push-disabled.js`, `test-http2-single-headers-validation-disabled.js`) pass with the debug build.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
ClientHttp2Session.request() and ServerHttp2Stream.pushStream() took the next stream id (getNextStream) and then handed the header object to the native encoder, which coerces each value to a string while encoding. A value whose toString() called request()/pushStream() again therefore took the next id but had its frame written first, so HEADERS / PUSH_PROMISE went out with decreasing ids (3 then 1; promised 4 then 2), which RFC 9113 5.1.1 forbids and nghttp2 peers ignore, and the two blocks' HPACK table updates were interleaved relative to the wire order. Coerce the values in JS first, the way node's buildNgHeaderString does, and pass that copy to the native encoder. Arrays are always copied so the native walk only reads fresh arrays of primitives; the caller's object still backs sentHeaders. A throwing coercion now throws from the call itself (as in node) instead of leaving a half-encoded block behind.
703b461 to
e1c62a7
Compare
|
Updated 5:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit f7554f2 has some failures in 🧪 To try this PR locally: bunx bun-pr 37568That installs a local version of the PR into your bun-37568 --bun |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/node/http2.ts:6467-6469—onClientStreamCreatedChannel.publish()at line 6467-6469 (andonServerStreamCreatedChannel/StartChannel.publish()at 3459-3464 inpushStream()) still runs synchronous user subscribers betweengetNextStream()andparser.request()/pushPromise(), so a diagnostics_channel subscriber that callssession.request()re-entrantly reproduces the same §5.1.1 wire-order violation ([3,1]) this PR fixes for header-valuetoString(). The PR description's claim that header values were "the remaining place user code could run between the id and the frame" is inaccurate — consider moving thesepublish()calls afterparser.request()/pushPromise(), or noting the exclusion.Extended reasoning...
What the bug is
The PR closes the window where user code can run between
getNextStream()allocating a stream id andparser.request()/parser.pushPromise()writing the frame that carries it, and its description explicitly claims that after this change, "header values were the remaining place user code could run between the id and the frame." That claim is not accurate:diagnostics_channel.publish()still sits inside that window at both call sites.In
ClientHttp2Session.request()(src/js/node/http2.ts:6458-6471):let stream_id: number = this.#parser.getNextStream(); // 6458 — id allocated ... if (onClientStreamCreatedChannel.hasSubscribers) { onClientStreamCreatedChannel.publish({ stream: req, headers }); // 6467-6469 — user subscribers run } if (typeof options === "undefined") { this.#parser.request(stream_id, req, wireHeaders, sensitiveNames); // 6471 — HEADERS written }
And in
ServerHttp2Stream.pushStream()(lines 3450 → 3459-3464 → 3467):getNextStream()reservespushId, thenonServerStreamCreatedChannel.publish()andonServerStreamStartChannel.publish()run, thenparser.pushPromise()writes the PUSH_PROMISE.diagnostics_channel.publish()invokes subscribers synchronously per Node semantics, so a subscriber to'http2.client.stream.created'(or'http2.server.stream.created'/'start') that callssession.request()/stream.pushStream()from its handler runs to completion inside that gap.Step-by-step proof
const dc = require('diagnostics_channel'); let once = false; dc.subscribe('http2.client.stream.created', ({ stream }) => { if (once) return; once = true; stream.session.request({ ':path': '/inner' }).end(); // re-entrant }); client.request({ ':path': '/outer' }).end();
- Outer
request()callsgetNextStream()→stream_id = 1. onClientStreamCreatedChannel.publish()runs the subscriber synchronously.- The subscriber calls
client.request(), which callsgetNextStream()→stream_id = 3, publishes (guarded byonce), thenparser.request(3, ...)writes HEADERS(3). - The subscriber returns; the outer call reaches
parser.request(1, ...)and writes HEADERS(1). - Wire order is
[3, 1]. RFC 9113 §5.1.1 requires ids to increase on the wire; nghttp2 peers ignore HEADERS(1) and the outer request never gets a response.
Unlike the
toString()case, HPACK stays consistent here — each header block is fully encoded by its ownparser.request()call before the next begins, so the dynamic-table order matches the wire order and there is no COMPRESSION_ERROR. Only the id-ordering half of the bug applies.Why Node doesn't hit this
Node publishes
'http2.client.stream.created'before callingnghttp2_submit_request, but nghttp2 allocates the id and queues the frame in the same call — a re-entrantrequest()from the subscriber therefore takes the lower id and queues its HEADERS first, producing[1, 3]. Bun's split (getNextStream()in JS, thenparser.request()for the frame) is what creates the window.Why this belongs in this PR
REVIEW.md, Correctness: the bug class, not the bug: "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep). Grep for every sibling site sharing the pattern... If a site is intentionally excluded, say so in the PR." This is the identical bug class (user code between
getNextStream()and the frame write) at a sibling site in the same two functions, and the PR description asserts completeness without mentioning it.Impact and fix
Impact is low: the trigger — a diagnostics_channel subscriber that makes a re-entrant
request()/pushStream()— is far more contrived than an object header value, and the more severe HPACK-desync half does not apply. This is pre-existing (the PR does not touch thesepublish()calls) and does not regress anything.The fix is trivial: move
onClientStreamCreatedChannel.publish()afterparser.request()(beforeonClientStreamStartChannel.publish(), which is already there), and move the two server-side publishes inpushStream()afterparser.pushPromise(). Alternatively, note the exclusion in the PR description so the completeness claim is accurate. - Outer
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes stream-id allocation ordering and error-throwing semantics in the node:http2 request/pushStream path — and the comment-cop bot is still flagging the toWireHeaders comments on the latest commit — a human look would still be worthwhile.
What was reviewed:
toWireHeaders/coerceHeaderValue: only objects/functions coerced (primitives pass through so native validation errors forundefined/null/symbol are unchanged); arrays always copied; caller's object never mutated (sentHeadersidentity preserved, verified in test).- Ordering:
toWireHeadersruns beforegetNextStream()in bothrequest()andpushStream(), and before the pending-queue push so#flushPendingRequestsat http2.ts:6527 consumes the already-coerced form. respond()at http2.ts:3837 is intentionally left uncoerced (fixed stream id; HPACK half deferred to #33470 per the description).
Extended reasoning...
Overview
The PR adds a toWireHeaders() helper in src/js/node/http2.ts that coerces object/function header values to strings in JS before getNextStream() allocates a stream id, so a re-entrant toString() that calls request()/pushStream() again gets the lower id and reaches the wire first (RFC 9113 §5.1.1). It's wired into ClientHttp2Session.request() (before both the pending-queue check and getNextStream()) and ServerHttp2Stream.pushStream() (before reserving the promised id). The pending queue now stores the already-coerced wireHeaders, and #flushPendingRequests consumes it. Seven new tests in test/js/node/http2/h2-conformance.test.ts cover wire order for the object/array-element/raw-array forms, the pre-connect queued case, an end-to-end run against a real http2.createServer(), PUSH_PROMISE promised-id order, and synchronous throw behavior.
Security risks
None identified. This is outbound-header pre-processing on values the caller already controls; no new parsing of untrusted input, no auth/crypto/permissions surface.
Level of scrutiny
Medium-high. node:http2 request() is a hot, protocol-critical path where ordering mistakes surface as COMPRESSION_ERROR or hung requests against nghttp2 peers. The change also alters user-visible error timing: a throwing header-value coercion now propagates synchronously from request()/pushStream() (matching node v26.3.0) instead of surfacing later as a stream 'error' / push callback error. That's a compat improvement but is a behavior change worth a human sign-off.
Other factors
- The comment-cop bot re-flagged lines 4083 and 6426 on the latest commit (e1c62a7, after the "trim the toWireHeaders comments" commit). The remaining comments look justified (they explain a non-local ordering constraint and cite the RFC/node reference), but the bot nag is still outstanding.
respond()/additionalHeaders()/sendTrailers()are deliberately not covered here (fixed stream id; the HPACK re-entrancy half is deferred to #33470). That scope split reads correctly but is worth confirming.- The lazy
{ ...headers }copy intoWireHeadersruns after earliercoerceHeaderValuecalls, so atoString()that mutates the input object could influence what's spread — butObjectKeyswas captured up front and each iterated key is overwritten with its coerced value, so the wire block is consistent; this is an edge case, not a bug. - Test coverage is thorough (raw-server wire-order assertions, end-to-end against
http2.createServer(),sentHeadersidentity check, session-still-usable after throw), and the description reports the fulltest/js/node/http2/suite plus 274 upstream test files passing.
…written A diagnostics_channel subscriber for http2.client.stream.created or http2.server.stream.created ran between getNextStream() and the native request()/pushPromise() call, so a request()/pushStream() made from the subscriber took the next id and reached the wire first, the same ordering violation the header value coercion fix closes. Publish the client channel at the end of request() and the server push channels once the PUSH_PROMISE is written, which is also where node publishes them. A push whose header block fails validation still publishes 'created' before its teardown, as that is the only handle on the stream, but no longer 'start'. Also cuts the toWireHeaders comments down to one line each.
|
Heads-up on an interaction with #37796, which makes the native raw-pairs walk in
|
|
The diagnostics_channel point from the review is right and is addressed in 1cba97f: |
A subscriber making a request from the channel while the session was still connecting was queued ahead of the request being published, so it took the lower id once the queue drained; node queues first and publishes last.
There was a problem hiding this comment.
Thanks — e24a48f addresses the pending-branch ordering I flagged (publish now follows #pendingRequests.push(), and the subscriber test covers both connected/connecting). No further issues found this run. Given this reorders when user JS runs relative to id allocation and HPACK encoding, plus the noted merge-order interaction with #37796 on the raw-array form, a human look is still worthwhile.
Checked: toWireHeaders leaves primitives untouched so native validation errors are unchanged; the caller's object is never mutated (sentHeaders identity preserved, tested); the common all-primitive path passes the same object through; the pushStream error path still publishes created before silent teardown so the existing "pushStream failure reports only via callback" test keeps its handle.
Extended reasoning...
Overview
Two files: src/js/node/http2.ts adds toWireHeaders() / coerceHeaderValue() to stringify object-typed header values before getNextStream() in ClientHttp2Session.request() and ServerHttp2Stream.pushStream(), and moves the http2.client.stream.created / http2.server.stream.{created,start} diagnostics_channel publishes to after the native request() / pushPromise() (and after #pendingRequests.push() on the queued path, per my prior comment). test/js/node/http2/h2-conformance.test.ts adds a nine-test describe block verifying HEADERS/PUSH_PROMISE wire order, id assignment, end-to-end against a real h2 server, subscriber reentry on both connected and still-connecting sessions, and synchronous-throw semantics.
Prior feedback addressed
My previous inline comment (on 1cba97f) pointed out the pending-request branch still published created before pushing to the queue, so a subscriber's nested request would be queued first. Commit e24a48f moved the publish to after the push (and after setupRequestEndAndSignal), and the subscriber test was extended to test.each over both requestBeforeConnect states. That resolves it.
Security risks
None identified. Header values are user-provided but were already reaching the native encoder; this only moves when they are stringified. toWireHeaders uses $isArray and ObjectKeys (primordials), copies arrays fresh so native never reads caller-owned storage, and never mutates the input.
Level of scrutiny
Medium-high. This is Node-compat protocol code where the fix hinges on RFC 9113 §5.1.1 ordering and shared HPACK encoder state — a subtle reentrancy invariant. The tests are strong (raw wire-level frame observation, verified against Node v26.3.0), but the change is not mechanical: it reorders observable side effects (start now precedes created on the connected path, matching Node), changes throw timing for pushStream(), and has a known interaction with #37796 (raw-array form with an array value slot would be joined by coerceHeaderValue once that PR makes native encode array slots element-wise). robobun already flagged that merge-order concern in the thread.
Other factors
- The comment-cop bot flagged verbose comments; author trimmed them in later commits (all threads resolved).
- Full h2 test suite reported passing (473 tests + 256 upstream test-http2-* + 18 diagnostics-channel-http2-*).
- The behavioral change for a Proxy-wrapped array (now sent per-element via
$isArray, matching Node) is called out in the description.
Deferring rather than approving because the reentrancy/HPACK reasoning and the #37796 interaction warrant a maintainer's eye on merge order.
…reHeaders An array in a value slot of the raw [name, value, ...] form was stringified to its joined form; copy it element-wise like the object form instead, so the encoder still sees an array (the same bytes today, and one field per element once the raw-form encoder handles arrays).
|
Thanks, that was right: an array in a value slot of the raw form went through the scalar coercion and reached the encoder as its joined string. a8da887 gives the raw branch the same element-wise copy as the object branch (shared |
The copy was taken lazily, after the first coercion had already run user code; take it before any coercion and coerce the copy's own keys, so the block handed to the encoder cannot depend on what a coercion did to the object it was read from. The all-primitive path still returns the object as is.
There was a problem hiding this comment.
39da606 addresses my note on the lazy spread — toWireHeaders now copies the object before any coercion runs, so a toString() that mutates headers can't inject an uncoerced value into wire. No further findings from this pass. Given this reorders the http2.client/server.stream.created/start publish points and changes how a throwing coercion surfaces from request()/pushStream(), a human look at the Node-compat and #37796-interaction claims would still be worthwhile.
What was reviewed:
toWireHeaders: primitives pass through untouched (native validator's errors preserved), arrays copied element-wise in both forms, object branch now spread-then-coerce so self-mutation during coercion can't reach native.- Both
request()branches publishcreatedat the end (pending branch after the queue push, per e24a48f), andpushStream()publishes afterpushPromise()returns; the failure path still firescreatedfor the torn-down stream. sentHeadersand the channel payloads keep the caller's uncoerced object; only the native call seeswireHeaders.
Extended reasoning...
Overview
The PR touches src/js/node/http2.ts (ClientHttp2Session.request(), ServerHttp2Stream.pushStream(), and a new toWireHeaders helper) and adds an eleven-test describe block to test/js/node/http2/h2-conformance.test.ts. The fix moves every point where user JS can run (header-value toString(), diagnostics_channel subscribers) to before parser.getNextStream() or after the native request()/pushPromise() call, so a re-entrant request()/pushStream() cannot take a higher stream id and write it first (RFC 9113 §5.1.1) or interleave HPACK encoder state.
Changes since my last review
Commit 39da606 rewrote the object branch of toWireHeaders: instead of a lazy wire = { ...headers } on first divergence (which ran after the first toString() and so could pick up keys that coercion had just added), it now pre-scans with isCoercibleHeaderValue (typeof-only, runs no user code), spreads once before any coercion runs, re-snapshots ObjectKeys(wire), and coerces every key of the copy in place. That closes both the added-key and the read-twice-getter variants I flagged; the copy is a plain data-property object user code has no reference to, so nothing a toString() does can land in what native reads. The all-primitive fast path (return headers) is unchanged.
Security risks
None identified. The header coercion runs in JS before native is entered, which is strictly earlier than before; primitives (including undefined/null/symbol) are passed through so native's existing validation errors are unchanged. The spread creates a plain object without __proto__: null, but the pre-change code already handed the caller's own object straight to native, so no new prototype surface is introduced.
Level of scrutiny
Medium-high. This is built-in node:http2 on the request/push hot path, and it reorders observable diagnostics_channel events (a connected request() now publishes start before created, matching Node) and changes error semantics (a throwing header-value coercion now throws synchronously from request()/pushStream() instead of surfacing later as a stream/callback error). The description says the full test-http2-* and test-diagnostics-channel-http2-* suites pass, and the eleven new tests were verified against Node v26.3.0, but the ordering claims and the interaction with #37796 (raw-array slots reaching the encoder as arrays, not joined strings) are the kind of thing a maintainer should sign off on.
Other factors
Two prior 🟡 findings from this bot were both addressed (e24a48f for the pending-queue publish order, 39da606 for the lazy spread). The comment-cop bot's paragraph-comment complaints were resolved by trimming to one-line comments. Test coverage is thorough (wire-order assertions against a raw server/client, an end-to-end run against http2.createServer(), throwing-coercion recovery, both connected and still-connecting subscriber cases). This is a well-reasoned, well-tested fix — just not simple or mechanical enough to land without a human look.
When nothing in the block needed coercing, the block queued for a request made before connect (or behind the concurrency limit) was the same object the stream exposes as sentHeaders, so a value put there before the queue drained was coerced by the encoder at flush time, after the stream id was taken. Queue a copy in that case; node sends the list it built in request() as well.
There was a problem hiding this comment.
I reviewed this PR (third pass) and found no bugs. Both of my earlier inline concerns were addressed (e24a48f, 39da606), and the new commit 4cbc0e7 correctly snapshots the queued header block so sentHeaders mutations while pending can't reach the encoder. Because this reorders the http2.*.stream.created/start diagnostics_channel publishes relative to parser.request()/pushPromise() and moves header coercion to a new spot on the request/push hot path, a maintainer look would still be worthwhile.
What was reviewed:
toWireHeadersobject/array/raw-array branches — scan runs on the caller's spread copy (data-only props), copy taken before any coercion; arrays always rebuilt element-wise so native never reads caller storage.- Pending-queue snapshot at http2.ts:6458 —
wireHeaders === headersonly when all-primitive object form; raw-array form always returns a fresh list, so the ternary is correct for both. - Channel-publish moves in
request()(both branches) andpushStream()(success + validation-failure paths) against Node's order; the failure path still firescreatedbut notstart. - Primitives/null/undefined/symbol pass through
coerceHeaderValueuntouched, so native validator error messages are unchanged.
Extended reasoning...
Overview
Two files: src/js/node/http2.ts (~90 lines) adds toWireHeaders() + three small helpers, calls it before getNextStream() in ClientHttp2Session.request() and ServerHttp2Stream.pushStream(), moves the http2.client.stream.created / http2.server.stream.{created,start} diagnostics_channel publishes from before the native call to after it (matching Node), and snapshots the queued wire block so sentHeaders mutations while pending don't reach the encoder. test/js/node/http2/h2-conformance.test.ts gains eleven tests (wire-order for four header forms, pending-queue coercion, subscriber re-entry on connected/connecting sessions, pending-snapshot immutability, end-to-end against a real server, PUSH_PROMISE order, subscriber-driven push, throwing coercion).
Security risks
None identified. Header values are user-supplied but the change only moves when they're stringified (earlier, in JS, before an id is taken) — the same coercion Node performs in buildNgHeaderString. No new parsing, no trust-boundary crossing, no allocation sized from untrusted input.
Level of scrutiny
Medium-high. This is Node-compat protocol code on the request()/pushStream() hot path, and it changes two observable behaviors: (1) header toString() now runs synchronously inside request()/pushStream() before the id is taken (throwing coercions now throw synchronously, matching Node), and (2) the created/start channels publish after the frame is queued instead of before. Both are verified against Node v26.3.0 in the description and covered by tests, but the re-entrancy reasoning is subtle enough that a maintainer familiar with the h2 frame parser should confirm nothing else between toWireHeaders() and parser.request() can run user JS.
Other factors
- I reviewed this PR twice before; both findings (pending-branch publish order, lazy spread after first coercion) were fixed with follow-up commits and covering tests.
- The one commit since my last look (4cbc0e7) is small and has a dedicated test ("the block of a request queued before connect is fixed when request() is made").
- The all-primitive fast path adds one
ObjectKeyswalk and no copy, so common-case overhead is minimal. - Tests are hermetic (raw in-process TCP server/client,
port: 0), await observable conditions (frame arrival, PING-ACK barrier), wire failure events to reject, and were verified to fail on the unfixed build. - The comment-cop bot's verbose-comment flags were addressed; remaining comments are one-liners.
There was a problem hiding this comment.
I reviewed this PR and found no issues on the current revision — both inline findings I raised earlier (pending-branch publish ordering, lazy spread after first coercion) have been addressed in e24a48f and 39da606. Because the change reorders observable diagnostics_channel timing and shifts pushStream()'s throwing-coercion error channel from callback to synchronous throw, a human look at those behavior decisions would still be worthwhile.
What was reviewed:
toWireHeadersobject/array/raw-array branches: copy is taken before any coercion runs, arrays stay arrays, primitives pass through untouched so native validator errors are preserved.request()immediate + pending branches andpushStream(): coercion precedesgetNextStream(), channel publishes follow the frame write / queue push on every path.- Pending-queue snapshot (
wireHeaders === headers ? { ...headers } : ...) closes thesentHeadersmutation window; covered by the new "block ... is fixed when request() is made" test. - Interaction with #37796 (raw-form array slots) —
toWireHeaderValuekeeps arrays as arrays so the two compose in either merge order.
Extended reasoning...
Overview
The PR touches src/js/node/http2.ts (~70 lines net) and adds a 12-test describe block to test/js/node/http2/h2-conformance.test.ts. It introduces toWireHeaders() + three small helpers, calls it before parser.getNextStream() in ClientHttp2Session.request() and ServerHttp2Stream.pushStream(), and moves the http2.client.stream.created / http2.server.stream.created / .start diagnostics_channel publishes to after the native request() / pushPromise() returns (or after the pending-queue push). The pending-request queue now stores a snapshot of the header block so a post-request() mutation of sentHeaders cannot reach the encoder.
Security risks
None identified. This is outbound header handling in a built-in JS module; no auth, crypto, or filesystem paths are touched. The re-entrancy hardening is defensive (copy-before-coerce) rather than trust-widening. The ${value} template coercion runs the same user code that native to_js_string was already running — just earlier, before an id or HPACK state is committed.
Level of scrutiny
Medium-high. src/js/node/http2.ts is a hot-path built-in module with tamper-resistance conventions, and the fix involves re-entrancy across the JS/native boundary plus RFC 9113 §5.1.1 stream-id ordering. That said, the change is pure JS with no memory-safety surface, the mechanism is well-explained (coerce-before-id, publish-after-write), and every branch has a wire-level test verified against Node v26.3.0.
Other factors
- Two rounds of my own inline feedback were incorporated: the pending-branch publish now follows the queue push (e24a48f, with a connecting/connected test.each), and
toWireHeaders's object branch now spreads before any coercion runs (39da606). - The PR introduces observable behavior changes that a maintainer should sign off on: (1) a connected
request()now publishesstartbeforecreated(matches Node per the description); (2) a throwing header-value coercion inpushStream()now throws synchronously instead of routing to the callback; (3) a push whose header block fails native validation publishescreatedbut no longerstart. All three are argued as Node-matching, but they change what existing code might have relied on. - The comment-cop bot flagged long comments on earlier revisions; those were trimmed to one-liners in 1cba97f and the current diff's comments are concise.
- Test coverage is thorough: 4 header forms × wire-order, pre-connect coercion, subscriber re-entry on connected + connecting sessions,
sentHeaderssnapshot, end-to-end againsthttp2.createServer(), PUSH_PROMISE order, subscriber-push order, and throwing coercion + session survivability. The evidence block shows all 12 fail on the unfixed build and pass with the fix on both ASAN-debug and release.
|
Re-verified on current main (f05c8c3). The branch merges cleanly with main. With this diff applied on main:
Repro used for the last point: const http2 = require("node:http2");
const srv = http2.createServer(() => {}).listen(0, () => {
const c = http2.connect(`http://127.0.0.1:${srv.address().port}`);
try {
const r = c.request({ ":path": "/", "x-foo": { toString() { throw new Error("boom"); } } });
r.on("error", e => { console.log("stream error:", e.code, e.message); c.close(); srv.close(); });
} catch (e) { console.log("sync throw:", e.code, e.message); c.close(); srv.close(); }
});
// node v26.3.0 and this PR: sync throw: undefined boom
// bun 1.4.1: stream error: undefined boom |
Repro
A header value whose
toString()makes another request on the same session. The raw server below only records the stream id of each HEADERS frame it receives, in wire order:[1,3][3,1]pushStream(), PUSH_PROMISE promised ids[2,4][4,2]http2.createServer()Session closed with error code 9(COMPRESSION_ERROR)request()/pushStream()whose valuetoString()throwsRFC 9113 §5.1.1 requires every stream an endpoint opens or reserves to have a higher id than the ones before it, so nghttp2 (node's server, curl, ...) ignores the HEADERS on stream 1 and the outer request never gets a response. #37563 makes bun's server do the same for inbound HEADERS, after which this hangs against bun servers too.
Cause
ClientHttp2Session.request()callsparser.getNextStream()and then passes the header object to the nativerequest(), which coerces each value to a string while it encodes the block (to_js_stringinh2_frame_parser.rs). Any user code that runs there, such as atoString(), runs after the outer request has taken its id but before its frame is written. A nestedrequest()takes the next id and writes its HEADERS first.ServerHttp2Stream.pushStream()has the identical structure (getNextStream()for the promised id, then the nativepushPromise()coerces).The nested block is also encoded in the middle of the outer block's encoding, so the shared HPACK encoder table is updated in an order that differs from the order the blocks reach the peer; in the repro the nested block references the
:authorityentry the outer block inserted but has not sent yet, which is the COMPRESSION_ERROR in the table above. A throwingtoString()leaves the same half-encoded state behind.Node cannot hit any of this because
buildNgHeaderStringstringifies every value in JS insiderequest()/pushStream()itself, and nghttp2 only assigns the id when it serializes the frame. #31323 already movedrequest()'s options reads ahead of the encode for the same reason (a getter re-enteringrequest()reordered the blocks); header values were one of the two remaining places user code could run between the id and the frame. The other isdiagnostics_channel:request()publishedhttp2.client.stream.createdandpushStream()publishedhttp2.server.stream.created/.startafter taking the id but before the native call, so a subscriber making another request / push from its handler produced the same[3,1]/[4,2]wire order (only the id half; each block is encoded whole there, so HPACK stays consistent). Node publishes these after the frame is queued.Fix
src/js/node/http2.ts:toWireHeaders()builds the block handed to native with every object value (the only kind whose coercion runs user code) turned into a string up front, for the object form, array values (copied element-wise, in both forms, so they still reach the encoder as arrays; this is what lets #37796, which teaches the raw-form encoder about arrays, compose with this change in either merge order), and the raw[name, value, ...]form.request()calls it before the pending-queue check andgetNextStream(), and the pending queue stores the coerced form, so a request made while still connecting is coerced when it is made too (node does the same).pushStream()calls it before reserving the promised id. A request that gets queued (made before connect, or behindSETTINGS_MAX_CONCURRENT_STREAMS) queues a copy of the block when nothing in it needed coercing, because in that case the block would otherwise be the very object the returned stream exposes assentHeadersuntil the queue drains, and a value put there in the meantime would have been coerced by the encoder at flush time, after the id was taken. Node sends the list it built insiderequest()in that situation as well, so a later edit ofsentHeadershas no effect on the wire in either runtime now.The channel publishes move to after the frame is written:
http2.client.stream.createdis published at the end ofrequest()on both branches (node's position as well: a connectedrequest()now publishesstartthencreatedlike node, and a request made by a subscriber while the session is still connecting is queued behind the one being published instead of ahead of it), and the server pushcreated/startpair is published oncepushPromise()has returned, still in that order (the portedtest-diagnostics-channel-http2-server-stream-created-start-timing.jschecks it). A push whose header block fails native validation still publishescreatedbefore its silent teardown, since that is the only handle user code can get on that stream (the existingpushStream failure reports only via callbacktest relies on it), but no longer publishesstartfor a stream that was never announced; node publishes neither.Details that keep the rest of the behavior unchanged:
undefined/null/ symbol values, and numbers still stringify natively.sentHeaders(node keeps the uncoerced values there as well; the test checks identity) and the diagnostics channel payloads.Object.keyswalk over the block and does not copy it. Otherwise the block is copied before any coercion runs and the copy is what gets coerced, so the encoder never sees anything a coercion did to the object it was read from.toString()that destroys the session instead of making a request now leavesrequest()returning a pending stream, which is what node returns there too (before, the stream had already taken an id);pushStream()behaves as before in that case.A throwing coercion now propagates out of
request()/pushStream()synchronously, which is what node does (verified against v26.3.0 for both); before, a pendingrequest()reported it as a later stream'error'andpushStream()routed it to the callback, and in both cases the session was left with a desynced HPACK table. The existingrequest() propagates a throwing header-value toString()test in node-http2.test.js still passes.The encode side of the same problem for
respond()/additionalHeaders()/sendTrailers()(which have a fixed stream id, so only the HPACK half applies) is what #33470 addresses natively by collecting the block before encoding; this PR is the id-allocation half that a native change cannot cover, since the id is taken in JS before native is entered.The other way to close this would be to stop taking the id in JS at all and have the native call allocate it as it writes the frame, the way nghttp2 does. That means creating the stream object without an id and attaching it afterwards (the push path creates the pushed stream from inside the id allocation today), and it would land in the middle of the native
request()that #33470 is restructuring, for no gain over this once the only things left betweengetNextStream()and the write are the stream construction and bookkeeping in this module: header coercion, the channel publishes and the queued block were the inputs user code could reach, and all three are handled here.Verification
Twelve tests in
test/js/node/http2/h2-conformance.test.ts(request()/pushStream() run no user code between taking a stream id and writing its frame): HEADERS wire order plus the ids each stream ends up with for the object, array-element, raw-array and raw-array-with-an-array-slot forms, the same for arequest()made before the session connected, the same for a request made from ahttp2.client.stream.createdsubscriber on a connected and on a still-connecting session (it must come second both times:[1,3]with the outer request on 1) and for a push made from ahttp2.server.stream.createdsubscriber ([2,4]with the outer push on 2), a request queued before connect whosesentHeadersis given a re-entering value before the flush (it must never be coerced, and exactly one HEADERS frame goes out), an end-to-end run against anhttp2.createServer()checking that both requests are answered with their own headers (and thatsentHeaderskeeps the original value), PUSH_PROMISE promised-id order observed by a raw client, and the synchronous throw plus the session staying usable afterwards. The expected values were checked against node v26.3.0 with the same scenarios.I also ran a script sending about 45 value shapes through
request()(numbers, bigint, Date, Buffer and Uint8Array values and elements, boxed strings,Symbol.toPrimitive/valueOf-only objects, functions, nested / holey / empty arrays, Array subclasses, proxies,undefined/null/ symbol values and elements, and the raw array form with numeric, object,nulland symbol slots) against anhttp2.createServer()and compared what the server received or what was thrown on the unfixed and fixed builds. The only difference is a Proxy wrapping an array, which used to be sent as one joined field and is now sent as one field per element, which is what node does (Array.isArraysees through proxies).All twelve fail on the unfixed build (
[3,1]/[4,2]/Session closed with error code 9/returned) and pass with the fix.bun bd test test/js/node/http2/(476 tests), the 256 upstreamtest-http2-*.jsfiles and the 18test-diagnostics-channel-http2-*.jsfiles all pass with the fix.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file