node:http2: send array-valued pushStream() headers as one field per element

[robobun]

Repro

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.

[robobun]

Status: merged as e592637.

Reproduced on bun 1.4.0 and main with the script in the PR body: pushStream() sent x-multi: a,b and a single set-cookie: c1=1,c2=2 field where node v26.3.0 sends one field per element. The five new tests in test/js/node/http2/node-http2.test.js fail on the release build (USE_SYSTEM_BUN=1) and pass with this change.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 510a3901-caa8-4a66-8350-d9efe38b2e44

📥 Commits

Reviewing files that changed from the base of the PR and between 8a97f7d and 3608809.

📒 Files selected for processing (1)

• test/js/node/http2/node-http2.test.js

---

Walkthrough

Changes

HTTP/2 pushStream() now validates restricted headers before stream allocation. PUSH_PROMISE encoding supports array values and per-element validation. Tests cover serialization, sensitive fields, strict and relaxed validation, and callback errors.

Suggested reviewers: cirospaciari, jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: encoding array-valued headers as separate fields in node:http2 pushStream().
Description check	✅ Passed	The description explains the cause, fix, behavior, edge cases, and verification results, although it does not use the template headings.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@test/js/node/http2/node-http2.test.js`:
- Around line 3593-3625: Update the helper around the server listen/client
request flow to wire server error events to the existing rejection path,
ensuring listen or runtime failures reject the promise instead of leaving it
pending. Move client cleanup into a finally block so the client is destroyed or
closed even when await promise rejects, while preserving normal block collection
and server cleanup.

🪄 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: 497f2794-0e35-4bcc-b310-a7734e8f34d8

📥 Commits

Reviewing files that changed from the base of the PR and between 97e21e5 and cfb3fd8.

📒 Files selected for processing (3)

• src/js/node/http2.ts
• src/runtime/api/bun/h2_frame_parser.rs
• test/js/node/http2/node-http2.test.js

[robobun]

^{Updated 1:33 PM PT - Aug 11th, 2026}

✅ @robobun, your commit 3608809665523b675a1cae6363af8c27cd3b14f8 passed in Build #92415! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37579

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

bun-37579 --bun

[coderabbitai]

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 3428-3429: Add tests for pushStream() covering headers with an
empty array and a singleton array, asserting both are accepted. Also verify
multi-value arrays and repeated case-insensitive header names remain rejected
through assertSingleValueHeaders.

In `@test/js/node/http2/node-http2.test.js`:
- Around line 3595-3597: Update the server startup promise around the listening
callback so the temporary "error" listener is removed once the server emits
"listening"; then register subsequent server errors with the promise's active
reject path. Ensure post-start errors cannot invoke the already-settled startup
callback or leave the operation promise pending.

🪄 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: df91ab1c-26d4-48c8-9dd7-396381875e4b

📥 Commits

Reviewing files that changed from the base of the PR and between cfb3fd8 and 8a97f7d.

📒 Files selected for processing (3)

• src/js/node/http2.ts
• src/runtime/api/bun/h2_frame_parser.rs
• test/js/node/http2/node-http2.test.js

💤 Files with no reviewable changes (1)

• src/runtime/api/bun/h2_frame_parser.rs

[claude]

I re-reviewed after 007d643 / 8a97f7d / 3608809 — the client-cleanup nit and the CodeRabbit points are addressed, and I didn't find anything new. Given this is ~90 lines in the native HPACK encoder for PUSH_PROMISE, a human look would still be worthwhile.

What was reviewed:

• push_promise() array/single-value branch matches the send_trailers()/request() pattern (same single_value_index_checked bitset, same per-element is_empty_or_undefined_or_null reject, never_index hoisted once per name).
• assertSingleValueHeaders in pushStream() is placed before the promised id is reserved and gated on kStrictSingleValueFields, same as respond()/request().
• pushedHeaderBlocks helper now closes the client in finally and wires server error/sessionError to the single rejection path for the whole lifetime.

Extended reasoning...

Overview

This PR brings push_promise() in h2_frame_parser.rs to parity with its siblings request() and send_trailers(): array-valued headers now encode as one HPACK field per element, and single-value-header enforcement uses the same strictSingleValueFields-gated bitset. On the JS side, pushStream() gains the same assertSingleValueHeaders() pre-check that respond(), additionalHeaders(), and request() already run, placed before a promised stream id is reserved so the throw is synchronous and reserves nothing. Five new tests in node-http2.test.js assert on the flat wire-decoded field list.

Security risks

None identified. The change tightens validation (rejects CR/LF/NUL per element, rejects duplicated single-value headers) rather than relaxing it. No new user-controlled data reaches an allocation size or index computation; the array iteration and per-element to_js_string coercion follow the same JSC exception-propagation shape (? on every fallible call) as the sibling encoders.

Level of scrutiny

Medium-high. This is native HTTP/2 wire-protocol encoding — an error here corrupts the shared HPACK dynamic table for the session. The change is mechanical (I diffed it against send_trailers() at lines 7907-8050 and the shape is the same), but ~90 lines of new native logic in a Node-compat protocol path is more than I'd approve without a human look, particularly given the PR body itself notes the pre-existing mid-encode HPACK-desync hazard (#33470) that this change is shaped to compose with.

Other factors

Since my previous inline nit, three commits landed addressing all feedback: the test helper now closes the client in finally, wires server error/sessionError to a single rejection promise for the whole lifetime, and races listen() against it; the flagged comments were trimmed; and an empty-array-on-single-value-field case was added. The bug-hunting pass on the latest revision found nothing. Tests cover the variant matrix well (array/empty/singleton, sensitive per-element, three single-value violation shapes, strictSingleValueFields: false, LF/null element via callback). CI build #92415 is in progress.

[claude]

LGTM — brings push_promise() to parity with the sibling encoders.

What was reviewed:

• Compared the new array/single-value branch in push_promise() against send_trailers() (h2_frame_parser.rs:8007-8094) and request() — same is_array()/array_iterator/bitset pattern, same error codes and messages.
• The JS-side assertSingleValueHeaders guard matches the three existing call sites (respond, additionalHeaders, request) with the same kStrictSingleValueFields gate.
• Test helper cleanup: my earlier note about client.close() on the failure path was addressed in 007d643/3608809; all review threads are resolved.

Extended reasoning...

Overview

This PR fixes pushStream() in node:http2 so that array-valued headers are sent as one HPACK field per element, matching Node.js. Three files touched: a one-line guard in src/js/node/http2.ts, a ~90-line restructure of the header-encoding loop in push_promise() in src/runtime/api/bun/h2_frame_parser.rs, and five new tests in test/js/node/http2/node-http2.test.js.

Security risks

None identified. This is outbound HTTP/2 header encoding on the server side; the change tightens validation (single-value enforcement, per-element CR/LF/NUL rejection) rather than relaxing it. No auth, crypto, or permissions code touched.

Level of scrutiny

Moderate — protocol encoding is correctness-sensitive, but the change is mechanical parity with two sibling encoders in the same file. I read the new push_promise() block side-by-side against send_trailers() at lines 8007-8094: identical structure (js_type().is_array() → array_iterator → single_value_index_checked bitset → per-element is_empty_or_undefined_or_null rejection → encode). The one intentional divergence is computing never_index once per header name and capturing it in the closure rather than per-element, which is semantically identical (the lookup depends only on the name) and is called out in the PR description. The JS change is a copy of the guard that respond()/additionalHeaders()/request() already run at lines 3664/3775/6235.

Other factors

• Test coverage is thorough: wire-level assertion on the flat rawHeaders list, empty-array on both ordinary and single-value fields, single-element array on a pseudo-header, sensitive-header never-index for every element, synchronous throw with no stream id reserved (asserts the next push gets id 2), strictSingleValueFields: false opt-out, and per-element invalid-value callback errors. The PR body confirms all five fail with USE_SYSTEM_BUN=1 and pass on the branch.
• All prior review feedback (CodeRabbit and my own inline nit about client cleanup in the test helper's failure path) has been addressed across commits 007d643, 8a97f7d, and 3608809; every thread is marked resolved.
• The pre-existing HPACK-desync-on-mid-encode-throw is explicitly noted as out of scope (tracked by #33470) and this change doesn't alter that behavior — the loop shape is preserved so that PR rebases mechanically.
• The single_value_headers bitset is declared outside the two-pass pseudo-header loop, so a duplicate across passes would still be caught; pseudo and non-pseudo names are disjoint anyway, and this matches how request() structures the same state.