Repository navigation
Conversation
…er element The raw [name, value, ...] form accepted by session.request() and stream.respond() stringified an array value slot, so ["ra", "rb"] went out as a single field "ra,rb". The object form already encodes one field per element; node's buildNgHeaderString does the same for both forms. The native raw-pairs walk in H2FrameParser::request() gets the same array branch as the object walk: one field per element, each element validated like a single value, an empty array sends nothing, and the single-value rule applies to the array as a whole. The sentHeaders object derived from a raw list no longer accumulates a later duplicate into the caller's own array: that array is also what the encoder reads, so ["x", ["a"], "x", "b"] would have sent b twice.
|
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 (1)
WalkthroughHTTP/2 request and response header handling now preserves raw arrays, validates each header occurrence, and encodes array elements as separate fields. Tests cover duplicates, sensitive headers, empty arrays, single-value restrictions, relaxed validation, and invalid values. ChangesHTTP/2 header encoding
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Comment |
|
Updated 2:04 PM PT - Oct 8th, 2026
❌ @robobun, your commit 4ff37ff has 3 failures in
🧪 To try this PR locally: bunx bun-pr 37796That installs a local version of the PR into your bun-37796 --bun |
|
Status: reproduced on a release build of main (bd599f5) and on Node v26.3.0 with the script in the description. This PR is stacked on #44804, which holds the copy of the array values. Merge #44804 first. |
![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 native HTTP/2 header-encoding path and carries a deliberate Node divergence plus noted interactions with #33470 and #37568, a human look is still worthwhile.
What was reviewed:
- The new
encode_valueclosure and array branch in the raw-pairs walk against the existing object-form array handling (single-value rule, never-index, empty-array skip, error paths). - Borrow/provenance of the closure's captures — all methods it calls take
&self, so no aliasing conflict with thesingle_value_headersbookkeeping outside it. rawHeadersToObject: traced the array-then-scalar and scalar-then-array duplicate cases to confirm.slice()prevents the encoder from re-reading a mutated caller array.- Ruled out: session reuse across throwing requests in the single-value test — each throwing case gets its own session or is followed only by the empty-array case.
Extended reasoning...
Overview
This PR fixes node:http2 so that an array in a value slot of the raw [name, value, ...] header form is sent as one field per element, matching Node and Bun's own object-form behavior. It touches three files: the raw-pairs walk in H2FrameParser::request() (h2_frame_parser.rs) gains an array branch that mirrors the existing object-form array handling; src/js/node/http2.ts extracts the duplicated sentHeaders-derivation loop from respond() and ClientHttp2Session.request() into rawHeadersToObject(), adding a .slice() on array values so accumulating later duplicates does not mutate the caller's array (which is what the encoder subsequently reads); and ~290 lines of tests are added to node-http2.test.js covering both directions, queued vs. connected, single-value enforcement, strictSingleValueFields: false, sensitive-header never-indexing, and invalid-element rejection.
Security risks
None identified. The change tightens validation (each array element is now run through is_valid_header_value and null/undefined elements are rejected) rather than relaxing it. No new untrusted-length arithmetic or buffer sizing is introduced; encoding still goes through the existing encode_header_into_list.
Level of scrutiny
Medium-high. This is wire-format code in the HTTP/2 encoder — a subtle mistake here corrupts what peers receive. The Rust change is a careful refactor: the per-value logic is hoisted into a local FnMut closure returning JsResult<Option<JSValue>> so the encode-error early-return path (schedule_header_compression_session_error) can bubble out of both the scalar and per-element call sites. I checked that every captured method (encode_header_into_list, handle_received_stream_id, schedule_header_compression_session_error) takes &self, so the closure's shared borrow of this does not conflict with the single_value_index_checked / single_value_headers[idx] bookkeeping that runs between the closure's definition and its calls. The empty-array continue before the single-value mark is a deliberate (and tested) choice matching Node's buildNgHeaderString.
Other factors
The PR is thorough and self-aware — it documents a deliberate divergence from Node (rejecting null/LF elements, consistent with Bun's object form and #37579), the pre-existing HPACK-desync-on-throw issue (#33470), and a landmine in the concurrently open #37568. Test coverage is strong and asserts on the peer-decoded flat header list rather than sentHeaders alone. That said, the combination of a native protocol-path refactor, a documented compat divergence, and cross-PR ordering concerns is exactly the kind of change a maintainer should sign off on rather than an automated approval.
|
On the two points flagged for a maintainer: the one divergence from node (an array element that is null or contains CR/LF/NUL throws |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The raw-pairs array branch mirrors the object walk's existing shape and the tests cover the wire-visible behaviour end-to-end. Because it restructures the native HPACK header-encoding loop and takes a deliberate Node divergence (rejecting null / CR-LF-NUL array elements rather than stringifying), a maintainer sign-off on that call and on the ordering with #37568 would still be worthwhile.
What was reviewed:
- The new
encode_valueclosure and array/scalar split against the object walk ath2_frame_parser.rs:8448/8484— same single-value bookkeeping, same per-element null check, same never-index propagation. rawHeadersToObject()— theslice()copy prevents the caller-array mutation the tests assert on; both former inline copies (respond/request) are replaced with identical semantics plus the copy.- Empty-array-before-single-value skips the occurrence bit (matches node's
buildNgHeaderString); scalar path is byte-identical to before.
Extended reasoning...
Overview
Three files: src/runtime/api/bun/h2_frame_parser.rs restructures the raw [name, value, ...] header walk inside H2FrameParser::request() so an array in a value slot encodes one HPACK field per element instead of going through Array.prototype.toString; src/js/node/http2.ts extracts the two identical inline sentHeaders-derivation blocks (in ServerHttp2Stream#respond and ClientHttp2Session#request) into rawHeadersToObject() and adds a defensive .slice() so accumulating a later duplicate does not push into the caller's array (which the encoder subsequently reads); ~300 lines of tests in node-http2.test.js covering request/respond, connected/queued, sensitive headers, single-value enforcement, strictSingleValueFields: false, and invalid elements.
Security risks
None identified. Header validation is unchanged for scalars and applied per-element for arrays (the same is_valid_header_value gate and null/undefined rejection the object walk already uses), so this does not open a new injection surface. The single-value rule is enforced on the array as a whole (len > 1 || already_seen), matching the sibling walks.
Level of scrutiny
Medium-high. This is protocol-critical native code on the request/response header path for node:http2, and the diff moves ~40 lines of encoding logic into a closure that is now called from two branches. I compared it line-for-line against the object walk (lines ~8448–8534) and push_promise (lines ~8007–8060), which already have this shape — the new branch is a faithful copy including the Ok(Some(ret)) early-return convention and the compression-error scheduling. The scalar else branch preserves the pre-change behaviour byte-for-byte (single-value check → encode_value(value_js)), so existing callers passing scalars are unaffected.
Other factors
The change carries a deliberate Node.js divergence: null/undefined or CR-LF-NUL-containing array elements throw ERR_HTTP2_INVALID_HEADER_VALUE where Node stringifies them. The PR body argues this matches bun's existing object-form behaviour and #37579's choice for pushStream, which I verified is the pattern at the sibling sites, but per the repo's Node-compat guidance a maintainer should confirm that call. There is also a stated ordering hazard with in-flight #37568 (whose toWireHeaders() would re-join arrays); the new tests would catch it, but landing order matters. The comment-cop bot flags are all resolved (comments were trimmed in a6f9214). Given the native-encoder scope and the compat decision, deferring rather than auto-approving.
They share one event loop, so running them concurrently only stacks every test's wall clock onto the same 5s budget; sequentially each one takes a fraction of a second even under a loaded debug+ASAN build.
…y slot The pre-encode single-value check ran on the object derived from a raw [name, value, ...] list, where a later empty-array slot for a name that already had a value shows up as a second element. Node (and the encoder, which skips an empty array before counting the field) accepts that in either order, so the check now walks the list itself and skips the slots the encoder skips. Tests cover every ordering for request() and respond().
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/http2.ts`:
- Around line 4032-4040: Update noteSingleValueField() to return seen
immediately for empty array values before checking or recording single-value
headers, while preserving validation for non-empty arrays and scalar values.
Apply the identical empty-array rule in the native object-form encoder, and add
request and response tests covering differently cased duplicate header names.
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 8907-8951: Copy the raw header name before any JavaScript
execution can re-enter the runtime: update the header-encoding flow around
name_str.to_slice and the encode_value closure to use an owned slice via
to_slice_clone (or otherwise keep the source values alive) before
sensitive_arg.get_truthy or item.to_js_string runs. Ensure validated_name
remains valid when reused after item.to_js_string, and add a regression covering
a toString() implementation that mutates the raw header array.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96de372d-877a-4216-b442-2c0852c033e1
📒 Files selected for processing (3)
src/js/node/http2.tssrc/runtime/api/bun/h2_frame_parser.rstest/js/node/http2/node-http2.test.js
to_slice() may alias the JSString's storage, and the name is still used after each value's toString() has run, which can drop the list's only reference to that string.
|
Review summary so far, for whoever picks this up:
|
Count the fields a slot will send (0 for [], the length for an array, 1 otherwise) before branching, so the skip and the single-value check exist once instead of once per branch.
The no-payload check called toUpperCase() on whatever :method held, so an array there (a one-element array in either header form, or an empty array following the value in the raw form) threw a TypeError before the block was encoded. Node sends the method the array holds and leaves the stream open; do the same.
|
Heads-up for rebasing: #37945 changes |
…d9e6e94/http2-raw-array-headers This brings in main and the copy of the array values (#44804). The merge takes that tree as it is: the native array arm of this branch and its tests in node-http2.test.js are gone. The next commit adds the change to one field per element in JS, on top of the copy.
… field respond() and request() sent an array in a value slot of a raw [name, value, ...] list as one field, joined with ",". foldRawHeaders() now folds the list for both. For a list with an array it returns a new list to encode: one pair per element, each element coerced with String() as node does. The native encoder does not change and never receives an array. A list that was expanded is checked before the encoder runs. The single-value rule counts each pair, and an element with CR, LF or NUL throws. The encoder throws for such a value after it has encoded a part of the block. A list without an array takes the same checks as before. request() accepts an array in the :method slot, and it builds its list without Array.prototype.concat.
|
The comments of 2026-08-12 and 2026-08-13 on this PR describe its first version: an array arm in the native walk, and a throw for a
|
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.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/js/node/http2.ts— nit: Maintainers keep a write that nothing reads any more: the never-index list re-attached to the sliced copy at src/js/node/http2.ts:3579. The only reader of that symbol on the copy was the oldrawHeadersList[sensitiveHeaders]fold, which this diff replaced withsensitiveNamesForCopyat src/js/node/http2.ts:3602. Fix: delete line 3579 and the "carry it over" clause of the comment at 3576, keepingsensitiveNamesForCopyas the single source forheadersObject[sensitiveHeaders]. foldRawHeaders() never looks at symbol keys and the native array walk ignores them too.Why this was flagged
A caller passes a raw list with
list[http2.sensitiveHeaders] = [...]to respond(). The diff reads that list intosensitiveNamesForCopyat src/js/node/http2.ts:3577, then still writes it back onto the slice at src/js/node/http2.ts:3579. On the base branch that write was load-bearing: the old inline fold readrawHeadersList[sensitiveHeaders]from the slice. After the diff, the slice is only consumed by foldRawHeaders() (src/js/node/http2.ts:3600, which reads indexed slots only) and, when no array value is present, by the native request() whose array_iterator never sees symbol keys; the sentHeaders object gets the list directly fromsensitiveNamesForCopyat src/js/node/http2.ts:3602. No behaviour changes for users; the cost is dead code and a stale comment left by the PR that made it dead, which the repository's review rules ask to be removed in the same PR.Verification: Any respond() call with a raw list carrying
list[http2.sensitiveHeaders]. At HEAD src/js/node/http2.ts:3577-3579 the symbol is written back onto the slice. On the base commit the only consumer of that write was the inline fold at base lines 3608-3609, and the diff replaces that withheadersObject[sensitiveHeaders] = sensitiveNamesForCopyat HEAD 3602-3603. So the write at 3579 has no reader. -
🟣
src/js/node/http2.ts— Clients sending cookies as an array in a raw header list get every short cookie field HPACK-indexed after merging, which nghttp2 (and node) never index. foldRawHeaders at src/js/node/http2.ts:3955 stores the copied array as the object value, and buildSensitiveNames at src/js/node/http2.ts:3906 only applies the short-cookie rule when the value is a string, socookienever enters sensitiveNames while the wire list now carries one shortcookiefield per element. Fix: when the cookie value is an array, apply the < 20 rule to each element (markcookienever-index if any element qualifies) so both the raw and object forms match nghttp2; cover["cookie", ["a=1"]]in the never-index test.Why this was flagged
Trigger: request([":path", "/", "cookie", ["a=1", "b=2"]]) or the same in respond()/push headers, the cookie-splitting form RFC 9113 §8.2.3 recommends. foldRawHeaders (src/js/node/http2.ts:3933-3947) copies the array, pushes one
cookie/element pair per element onto the wire list, and at :3955 stores the array copy as object.cookie. buildSensitiveNames (src/js/node/http2.ts:3904-3906) reads that object value, sees typeof cookie !== "string", and does not set map["cookie"]. The native raw walk (src/runtime/api/bun/h2_frame_parser.rs:6590-6597) decides never_index solely from sensitiveNames, so each short cookie element is encoded with indexing into the shared HPACK table. The comment at :3856-3858 states the rule bun intends to mirror (nghttp2 never indexes cookie values < 20 chars); node gets it from nghttp2 for every element. On the base branch the array was sent as one joined field, so this PR is what first produces the per-element short cookie fields the rule targets. Remedy: apply the length rule to array elements in buildSensitiveNames.Verification: Trigger: a
cookieheader whose value slot is an array.foldRawHeaders(src/js/node/http2.ts:3955-3957) stores the copied array asobject[key];buildSensitiveNames(src/js/node/http2.ts:3904-3906) applies the short-cookie never-index rule only whentypeof cookie === "string", so an array value never setsmap["cookie"]. However the base branch already fails by the same route.
| // desync the shared HPACK table from the peer). | ||
| if (this[kStrictSingleValueFields] !== false) assertSingleValueHeaders(headers); | ||
| if (rawHeadersExpanded) { | ||
| assertExpandedRawHeaders(rawHeadersList!, this[kStrictSingleValueFields] !== false); |
There was a problem hiding this comment.
🟡 (optional) Callers whose raw list holds an array value now get a synchronous TypeError for a :path containing CR, LF or NUL, where the base (and node) deliver an async stream error. assertExpandedRawHeaders at http2.ts:3978-3991 scans every string value, pseudo-headers included, and runs at http2.ts:6187 before the :path control-character check at http2.ts:6274-6288 that turns such a path into a nextTick NGHTTP2_PROTOCOL_ERROR on the returned stream. Fix: keep one contract for :path regardless of list form, e.g. skip :path in the expanded pre-check (or run the 6274 check first) so request([":path", "/a\n", "x", ["b"]]) still returns a stream that errors asynchronously.
Why this was flagged
A caller passes client.request([":path", "/a\n", "x-multi", ["b", "c"]]). foldRawHeaders at http2.ts:3923 expands the list, so rawHeadersExpanded is true and http2.ts:6187 calls assertExpandedRawHeaders. Its second loop at http2.ts:3978-3991 does not exclude pseudo-headers; it finds 0x0a in the ":path" value and throws TypeError with code ERR_HTTP2_INVALID_HEADER_VALUE and message 'Invalid value for header ":path"' synchronously. On the base branch the list reached the check at http2.ts:6274-6288, which for c <= 0x20 creates the ClientHttp2Stream, sets rstCode = NGHTTP2_PROTOCOL_ERROR, schedules emitStreamErrorNT and returns the stream. After the change the same path value is a sync throw only when another slot is an array, and stays async otherwise, so the error contract now depends on an unrelated slot. The single-value guard does not apply; nothing else intercepts the pre-check before it reaches pseudo-header values.
Verification: foldRawHeaders (src/js/node/http2.ts:3923-3960) returns a new wire list whenever a value slot is an array, so rawHeadersExpanded = rawHeadersList !== list (6124) is true and 6186-6187 calls assertExpandedRawHeaders. Its value loop (3978-3991) has no pseudo-header exclusion, so :path = "/a\n" throws synchronously. After the PR the same call throws before 6274 is reached.
| const error = new TypeError(`Invalid value for header "${lower}"`); | ||
| error.code = "ERR_HTTP2_INVALID_HEADER_VALUE"; |
There was a problem hiding this comment.
🟡 nit (optional): Callers of request()/respond() with an expanded raw list get a hand-built TypeError with a hand-assigned .code, bypassing the shared error machinery the rest of this file uses. src/js/node/http2.ts:3987-3988 constructs new TypeError(...) and sets error.code = "ERR_HTTP2_INVALID_HEADER_VALUE" by hand, while the same file throws $ERR_HTTP2_INVALID_HEADER_VALUE(value, name) at http2.ts:618 for the same code. Fix: throw through the $ERR_HTTP2_INVALID_HEADER_VALUE helper (or, if the encoder's shorter message must be preserved, add that variant to ErrorCode.ts) so every ERR_HTTP2_INVALID_HEADER_VALUE thrown from JS carries the same class, code and message shape. [also at: src/js/node/http2.ts:3989 - nit: this hand-builds a new TypeError and assigns error.code instead of going through the $ERR_* ErrorCode machinery that every other throw in this file uses.]
Why this was flagged
A raw header list containing an array value plus a string element with CR, LF or NUL reaches assertExpandedRawHeaders via ClientHttp2Session.request() at src/js/node/http2.ts:6187 or ServerHttp2Stream.respond() at src/js/node/http2.ts:3623. At src/js/node/http2.ts:3987-3989 the code builds a plain TypeError and assigns .code manually instead of using the $ERR_HTTP2_INVALID_HEADER_VALUE helper declared in src/js/builtins.d.ts:461 and already used at src/js/node/http2.ts:618. The helper's message template in src/jsc/bindings/ErrorCode.cpp:1906 is Invalid value "<value>" for header "<name>", so the same error code now has two different message shapes, and the hand-rolled error does not go through the centralized ErrorCode construction. On the base branch this input never threw from JS at all (the native encoder threw mid-encode); the behaviour change itself is intended, only the construction of the error diverges from the file's convention. No safeguard applies; this is a convention/consistency nit, not a runtime failure.
Verification: Any expanded raw header list whose string element contains NUL/CR/LF reaches assertExpandedRawHeaders via respond() (src/js/node/http2.ts:3622-3624) or request() (:6186-6188). src/js/node/http2.ts:3987-3989 throws a plain TypeError with a hand-assigned own .code, while the same file throws $ERR_HTTP2_INVALID_HEADER_VALUE(value, name) at :618. REVIEW.md states: never inline new Error with a hand-assigned .code.
| } | ||
| const existing = object[key]; | ||
| if (existing === undefined) object[key] = value; | ||
| else if ($isArray(existing)) existing.push(value); |
There was a problem hiding this comment.
🟡 (optional) Every raw-list respond() or request() that repeats a header name (the set-cookie case this PR targets) routes its sentHeaders fold through user-overridable Array.prototype.push at src/js/node/http2.ts:3956, while the same new helper uses $arrayPush for the wire list four lines above. Page code that patches Array.prototype.push corrupts or throws from sentHeaders for a duplicate name. Fix: use $arrayPush(existing, value) here, and audit the sibling fold sites in this file for the same call so the builtin stays tamper-proof as src/js/CLAUDE.md requires.
Why this was flagged
Trigger: any raw list whose name appears three or more times, e.g. respond([":status", 200, "set-cookie", "a", "set-cookie", "b", "set-cookie", "c"]), or twice when the first slot is an array. foldRawHeaders at src/js/node/http2.ts:3954-3957 does existing.push(value) on the internal array it created, dispatching through Array.prototype.push, which user or polyfill code can replace; a replaced push that throws or drops arguments makes respond() throw before the HEADERS frame is sent or leaves sentHeaders missing values. The wire list in the same function is built with $arrayPush (:3931, :3941, :3947, :3951), so the diff itself shows the correct primitive. The base inline loop had the same call, but this PR rewrote that loop into a new helper and chose the intrinsic for every other push in it, so the remaining .push is the one tamper-reachable call on a path that runs once per duplicate header on every raw-list response. Remedy: $arrayPush at :3956.
Verification: Pre-existing. Triggering condition: user code has replaced Array.prototype.push and a raw list repeats a name. src/js/node/http2.ts:3954-3957 does existing.push(value) on an internal plain array, so the call runs whatever the user installed. The base branch contains the identical call in both inline fold loops, so merging makes nothing worse than the base.
| if (rawHeadersExpanded) { | ||
| assertExpandedRawHeaders(rawHeadersList!, session[kStrictSingleValueFields] !== false); | ||
| } else if (session[kStrictSingleValueFields] !== false) assertSingleValueHeaders(headers); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: callers who repeat a single-value header with an undefined value slot after the real value still get ERR_HTTP2_HEADER_SINGLE_VALUE, which node accepts, unless some unrelated slot holds an array. At http2.ts:3624 and :6188 a list with no array value is still judged by assertSingleValueHeaders on the folded object, where ["content-type", "text/html", "content-type", undefined] folds to a length-2 array; the same list with "x-a", ["a"] added passes via assertExpandedRawHeaders. Fix: judge every raw list slot by slot (skip undefined and empty-array slots) regardless of whether foldRawHeaders expanded it, so the outcome does not depend on an unrelated slot. [also at: src/js/node/http2.ts:6188 - pre-existing: callers of request()/respond() still get ERR_HTTP2_HEADER_SINGLE_VALUE for a raw list that repeats a single-value name with an undefined second slot, which node accepts, unless some unrelated slot happens to hold an array. src/js/node/http2.ts:6188 and :3624 keep judging a…]
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A caller passes request([":path", "/", "content-type", "text/html", "content-type", undefined]) or the same list to respond(). foldRawHeaders (http2.ts:3923) sees no array, so wire === list and rawHeadersExpanded is false; the folded object has content-type = ["text/html", undefined] (http2.ts:3957). http2.ts:3624 / :6188 then call assertSingleValueHeaders(headers), whose check at http2.ts:3871 sees value.length > 1 and throws ERR_HTTP2_HEADER_SINGLE_VALUE. Node's buildNgHeaderString skips an undefined value slot in the array form and sends one content-type field. If the caller adds any array value elsewhere in the list (test row at node-http2-header-list.test.ts:518), the list is expanded and assertExpandedRawHeaders (http2.ts:3967) skips the undefined slot, so the same two content-type slots are accepted. The base branch throws for this input too (pre-existing), but the test comment at :499-501 claims an undefined slot is 'none, in every slot of the list', which the non-expanded path does not honor.
Verification: Pre-existing. A raw list that repeats a single-value name with an undefined value after the real value and has no array-valued slot folds to ["text/html", undefined] at src/js/node/http2.ts:3957; lines 3624 / 6188 hand it to assertSingleValueHeaders, which throws ERR_HTTP2_HEADER_SINGLE_VALUE at 3871. assertExpandedRawHeaders (3966) skips the slot, and the base revision throws by the same route.
| let list = raw; | ||
| if (additionalPseudoHeaders.length) { | ||
| list = additionalPseudoHeaders; | ||
| for (let i = 0; i < raw.length; i++) $arrayPush(list, raw[i]); | ||
| } | ||
| const headersObject = { __proto__: null }; | ||
| rawHeadersList = foldRawHeaders(list, headersObject, this[kStrictSingleValueFields] !== false); | ||
| rawHeadersExpanded = rawHeadersList !== list; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: A caller that queues request() on a still-connecting session and then mutates its raw header array sees the mutation sent on the wire, unlike node. When the list carries every pseudo-header and no array value, http2.ts:6117 keeps list = raw and foldRawHeaders returns that same caller array, which is stored as wireHeaders at http2.ts:6351 and encoded later at http2.ts:6445. Fix: encode a copy of the caller's list on every request() path (e.g. slice raw as respond() does at http2.ts:3578, or always build a fresh list), while keeping sentHeaders derived from the same copy. [also at: src/js/node/http2.ts:6120 - pre-existing: a request queued on a still-connecting (or stream-limited) session encodes the caller's own list later, so edits the caller makes after request() returns reach the wire; node encodes at the call.; src/js/node/http2.ts:6121 - pre-existing: a caller who passes every pseudo-header in a raw list and mutates that array after request() returns gets the mutated list on the wire when the request was queued.]
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
On a session that has not emitted 'connect', client.request([":method","GET",":scheme","http",":authority",host,":path","/","x-tag","a"]) followed by list.push("x-late","v"). additionalPseudoHeaders is empty at http2.ts:6118, so list is the caller's own array; foldRawHeaders at http2.ts:3922 sees no array value and returns wire === list (http2.ts:3923, 3958). rawHeadersList is the caller's array, pushed into #pendingRequests as wireHeaders at http2.ts:6351 and only handed to parser.request at http2.ts:6445 once the session connects, so the late "x-late" pair goes on the wire while req.sentHeaders (built eagerly at http2.ts:6123) does not show it. Node encodes the block synchronously inside request(), so the late pair is never sent. The base branch has the same : raw branch, so this is pre-existing, but the new test at test/js/node/http2/node-http2-header-list.test.ts:373-386 claims the list is read at the call and only passes because its list contains an array value, which forces the copy. respond() avoids this by headers.slice() at http2.ts:3578.
Verification: pre-existing. request() is queued with a raw list that supplies every pseudo-header and contains no array value, and the caller then mutates that array before the queue flushes. With no array values rawHeadersList === raw, stored as wireHeaders at 6351 and encoded only at 6445/6447. Base line 6038 aliases the caller's array identically, so merging makes nothing worse.
This branch has not been deployed
1 participant
Stacked on #44804. Merge #44804 first.
Problem
stream.respond()andsession.request()accept a flat[name, value, ...]list. An array in a value slot goes out as one field joined with,.respond([":status", 200, "set-cookie", ["a=1", "b=2"]])sendsset-cookie: a=1,b=2. Node v26.3.0 sends one field per element.src/runtime/api/bun/h2_frame_parser.rs:6588).Fix
foldRawHeaders()folds the list for both methods. For a list with an array it returns a new list to encode, with one pair per element. Native code is unchanged.request()accepts an array in:method.test/js/node/http2/node-http2-header-list.test.ts(12 new tests, all fail on node:http2: copy the array values of a raw header list in respond() and request() #44804) and 256 upstreamtest-http2-*.js. Self-reviewed: 8 concerns raised, 8 addressed.Background
content-typegoes out once.nullelement then throws after a partial encode.Downsides
nullelement is sent as text, as in Node. The object form andpushStream()throw for it (node:http2: send array-valued pushStream() headers as one field per element #37579). A maintainer must choose one rule.request()with a bad element now throws from the call, not on the stream.Notes
Repro
The fields after
:status, as the client decoded them:arrayset-cookie: a=1,set-cookie: b=2set-cookie: a=1,b=2set-cookie: a=1,set-cookie: b=2nullx-n: a,x-n: nullx-n: a,x-n: a,x-n: nullcrlfx-session: sid=malletrespond()throwsrespond()throwsafter(the next response)x-session: sid=bobx-session: sid=bobx-session: sid=bobThe
crlfrow is a difference from Node that main has too: Node drops the element, Bun throwsERR_HTTP2_INVALID_HEADER_VALUE.What changed since the first version of this PR
nullelement throws after a part of the block is encoded. The next response on the session then fails with a compression error.git diff origin/main -- src/runtimeis empty.["x-session", ["sid=mallet", "en\r\nx"]]the encoder then threw after it had encodedsid=mallet, and the next response on the session carried the value of another response. Main throws for that list before it encodes a field. The check inassertExpandedRawHeaders()keeps that, and the testan element with CR or LF throws before a field of the block is encodedcovers it.copyHeaderValueArray().node-http2.test.jstonode-http2-header-list.test.ts.nullorundefinedelement is sent as text. The first version threw. The test for that expects the text now.Rules for an expanded list
strictSingleValueFields: falsethe elements are joined into one field, as in Node. With the rule on, the pairs are rejected withERR_HTTP2_HEADER_SINGLE_VALUE.nullvalue is an occurrence, as in Node. Anundefinedvalue and an empty array are not.String()inside the call. AtoString()that throws makesrequest()throw at the call on a connecting session too.respond([":status", "200", ":status", []])now throwsERR_HTTP2_STATUS_INVALID. Main and Node v26.3.0 throwERR_HTTP2_HEADER_SINGLE_VALUE. Each throws before anything is encoded.Measurements
respond()rows and 15request()rows on a connected and on a connecting session.cookiewith an array of short values (Node marks each field never-indexed, Bun decides that from one string value, as on main). 2 rows readsentHeadersand then look at the caller's array (Node has written into it, see node:http2: copy the array values of a raw header list in respond() and request() #44804). 1 row changes a list without an array after a queuedrequest()(Bun sends the change, as on main).BUN_JSC_dumpGeneratedBytecodes=1):respond()+3,request()+3.assertSingleValueHeadersis byte-identical.heapStats().objectTypeCounts, JIT off, GC off): object form 0, raw list without an array 0, raw list with an array 0 (one Array for the new list, one less for the key list of the old check). Arequest()list that gets default pseudo-headers: one Array less, because the list is no longer built withconcat.js/node/http2.js169,374 B to 171,207 B (+1,833 B). No native change, no new host function.valgrind), and wall clock for this version.Not changed here
nullelement in the object form and inpushStream()throws (node:http2: send array-valued pushStream() headers as one field per element #37579). node:http2: normalize outbound header objects like node #41614 changes the object form to Node's rule.[]in the object form counts as an occurrence of a single-value field (node:http2: an empty array value is not an occurrence of a single-value header #37945).request()reads a list without an array, and a header object, when the session connects (related: node:http2: coerce outbound header values before taking a stream id #37568).cookiewith an array of short values is not never-indexed.null. Node sendsnull.Other open PRs on the same lines
git merge-treeof this branch with node:http2: normalize outbound header objects like node #41614, node:http2: coerce outbound header values before taking a stream id #37568, node:http2: prepare the final response headers like node #43526 and node:http2: validate request() headers and options before the session state #43536 reports a conflict insrc/js/node/http2.ts. The PR that lands later needs a merge.Suites run with the debug build
test/js/node/http2/node-http2-header-list.test.ts: 18 pass. With the JS of node:http2: copy the array values of a raw header list in respond() and request() #44804, the 12 tests of this PR fail.test/js/node/test/parallel/test-http2-*.js: 255 of 256 pass.test-http2-forget-closed-streams.jsneeds about 4 minutes with the debug build and passes when it runs alone.describe/test/expectshim): 10 pass. The other 2 expect the throw for an element with CR or LF.[review] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 2
evidence per changed file
root cause · written by the author bot
The raw flat array header path in the native request encoder, which also serves respond(), converted each value slot straight to a string, so an array value was stringified into a single comma-joined field instead of one field per element as the object form and node already produce. The fix makes that walk compute the number of fields a slot will emit once (none for an empty array, the array length, or one for a scalar), skip empty arrays, apply the single-value header check against that count, and convert, validate and HPACK-encode each element as its own field. The JavaScript side now als…