node:http2: validate request() headers and options before the session state

[robobun]

Problem

• ClientHttp2Session#request(headers, options) reports the wrong error for an invalid call. request({ "bad name": "x" }, { endStream: 1 }) throws ERR_INVALID_ARG_TYPE, node v26.3.0 throws ERR_INVALID_HTTP_TOKEN.
• A destroyed or closed session hides header errors. request({ "bad name": "x" }) does not throw on a destroyed session, and throws ERR_HTTP2_GOAWAY_SESSION on a closed one.
• The cause is the order in request() (src/js/node/http2.ts:5892): options fields, session state, header block. Node validates the headers, then options, and acts on the session state last.

Fix

• Move the options copy and checks below the header validation. Move the options.signal check and the session-state checks below them, but above the deferred :path and content-length rejections.
• As in node, the stream that a destroyed session fails is set up like any other request: default headers in sentHeaders, endStream applied, signal attached.
• A non-string :method is sent as String(value), as in node. It threw a TypeError, which a destroyed session would now reach. The GOAWAY error comes from $ERR_HTTP2_GOAWAY_SESSION().
• Verified: test/js/node/http2/node-http2-request-validation.test.ts, 11 new tests. Stock bun fails 10, node passes all. Also test/js/node/http2/ and the vendored test-http2-*.js files. Self-reviewed: 13 concerns raised, 8 addressed.

Background

• An invalid argument makes request() throw. A destroyed session returns a stream that fails on the next tick with ERR_HTTP2_INVALID_SESSION.
• A session is closed after close() or a graceful GOAWAY, while streams are in flight.
• nghttp2 rejects some requests at send time (a :path with a space). Bun reports these from request() as a deferred stream error. Node gets there after the session-state check.

Notes

Repro, run on an open session, then after c.destroy():

c.request({ "bad name": "x" }, { endStream: 1 }); // open
c.request({ "bad name": "x" });                   // destroyed
c.request({ ":method": "CONNECT" });              // destroyed

call	node v26.3.0	bun before	bun after
open, bad name + endStream: 1	ERR_INVALID_HTTP_TOKEN	ERR_INVALID_ARG_TYPE	ERR_INVALID_HTTP_TOKEN
destroyed, bad name	ERR_INVALID_HTTP_TOKEN	no throw, stream fails with ERR_HTTP2_INVALID_SESSION	ERR_INVALID_HTTP_TOKEN
destroyed, CONNECT without :authority	ERR_HTTP2_CONNECT_AUTHORITY	no throw	ERR_HTTP2_CONNECT_AUTHORITY
closed, bad name	ERR_INVALID_HTTP_TOKEN	ERR_HTTP2_GOAWAY_SESSION	ERR_INVALID_HTTP_TOKEN
destroyed, signal: {}	ERR_INVALID_ARG_TYPE	no throw	ERR_INVALID_ARG_TYPE
{ endStream: 1, parent: "x" }	names options.parent	names options.endStream	names options.parent
reads of an options getter per call, invalid / valid headers	0 / 1	3 / 3	0 / 1
destroyed, GET: writableEnded, events	true, error close	false, aborted error close	true, error close
destroyed, signal aborted in the same tick	AbortError	ERR_HTTP2_INVALID_SESSION	AbortError
open, { ":method": 1 }	server sees :method: 1	TypeError: method.toUpperCase is not a function	server sees :method: 1
closed, valid call: String(error)	Error [ERR_HTTP2_GOAWAY_SESSION]: ... (on the stream)	Error: ... (thrown)	Error [ERR_HTTP2_GOAWAY_SESSION]: ... (thrown)

I ran a matrix of 15 header values, 10 option values and 4 session states (open, destroyed, closed, closed by a GOAWAY) on node v26.3.0 and on bun: 224 calls. 136 differed before this change. 54 differ after it. None of the 54 is an order difference between the header block, options and the session state.

Tests. They are in their own file, like the other focused http2 suites (node-http2-client-close.test.ts). node-http2.test.js takes more than 3 minutes on a debug build and some of its tests time out there. The new file takes 5 s. Ten of the eleven new tests fail on stock bun. The other one (a destroyed session fails every request that has valid arguments) passes before and after. That test and its two closed-session siblings fail when the session-state checks move below the :path and content-length rejections. I checked that with the mutation: the streams then fail with ERR_HTTP2_STREAM_CANCEL and ERR_HTTP2_STREAM_ERROR. I ran the same tests on node v26.3.0 through a small describe/it/expect shim: 11 of 11 pass. failureOf() in the tests accepts a thrown error and a stream error, because of the first item below.

Not changed in this PR:

• A closed session still throws ERR_HTTP2_GOAWAY_SESSION synchronously for a valid call. Node v26.3.0 reports it on the stream (http2: emit session close before stream close nodejs/node#63414). node:http2: rewritten inbound engine, batched write path, server push, +290 node v26.3.0 tests (79% passing) #31584 chose the throw and the code comment says so.
• The order of the checks inside the header block (node checks the CONNECT rules before the names).

Concerns from the self-review that I did not address here. All five exist on main:

• With a plain-object authority and no protocol (http2.connect({ host, port }, { protocol: "http:" })), request() reads options.protocol from the request options for the :scheme default. In the raw-list form that read comes before the header validation. The read is part of a separate bug: bun sends :scheme: https on that cleartext session, node sends http. No PR covers it yet.
• A falsy options.signal (null, 0, "", false) throws from the native request(). Node ignores it. node:http2: free a request's slot when its stream closes on the wire, abort request({ signal }) from JS only #42763 removes that native check.
• A duck-typed { aborted: false } signal. It now throws from addAbortListener on a destroyed session, as in node.
• Node reads the session state at entry and acts on it last. Bun reads it where it acts on it. A getter that destroys the session gives a clean ERR_HTTP2_INVALID_SESSION stream.
• Header values are stringified in the native encoder, so a throwing toString() loses to an options error. node:http2: coerce outbound header values before taking a stream id #37568 moves the coercion.

Other differences in the matrix, each with an owner:

• The headers and options type checks (assertIsObject): a separate change is in progress. Node's place for the options check is right above the options copy.
• Connection-specific headers: node:http2: normalize outbound header objects like node #41614.
• The options.signal message says "of type AbortSignal", node says "an instance of AbortSignal": node errors: render a single class name in ERR_INVALID_ARG_TYPE as "an instance of" like node #38694. The new test reads the property name from the message, not the full text.
• parent: 0 and a negative parent: reported separately.

Suites: the new file passed 5 of 5 runs on the debug build with the final commit. The 62 vendored test-http2-*.js files that touch destroy(), 'aborted' or a signal pass with the final build. All 256 passed before the last one-line change. In this container the full node-http2.test.js has timeouts with and without the change, all in tests that spawn a debug-build subprocess (5 s limit).

---

[human-review] gate passed · iteration 1 · 2 files touched

fails on main (without fix)

ASAN without fix: 10 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/node/http2/node-http2-request-validation.test.ts"
bun test v1.4.3 (367d939d9)

test/js/node/http2/node-http2-request-validation.test.ts:
168 |         for (const [name, value] of invalidOptions) {
169 |           headerErrors[`${label} + options.${name}`] = thrownBy(client, headers(), { [name]: value })?.code;
170 |           expectedHeaderErrors[`${label} + options.${name}`] = code;
171 |         }
172 |       }
173 |       expect(headerErrors).toEqual(expectedHeaderErrors);
                                 ^
error: expect(received).toEqual(expected)

  {
    "CONNECT with :path": "ERR_HTTP2_CONNECT_PATH",
-   "CONNECT with :path + options.endStream": "ERR_HTTP2_CONNECT_PATH",
-   "CONNECT with :path + options.exclusive": "ERR_HTTP2_CONNECT_PATH",
-   "CONNECT with :path + options.parent": "ERR_HTTP2_CONNECT_PATH",
+   "CONNECT with :path + options.endStream": "ERR_INVALID_ARG_TYPE",
+   "CONNECT with :path + options.exclusive": "ERR_INVALID_ARG_TYPE",
+   "CONNECT with :path + options.parent": "ERR_INVALID_ARG_TYPE",
    "CON
... (truncated)

release without fix: all passed
bun test v1.4.3-canary.1 (f3083d6c5)

test/js/node/http2/node-http2-request-validation.test.ts:
(pass) http2 client.request() validates headers, then options, then looks at the session state > on a session that is open [11.68ms]
(pass) http2 client.request() validates headers, then options, then looks at the session state > on a session that is destroyed [2.55ms]
(pass) http2 client.request() validates headers, then options, then looks at the session state > on a session that is closed [3.63ms]
(pass) http2 client.request() validates headers, then options, then looks at the session state > on a session that is closed by a GOAWAY [1.87ms]
(pass) http2 client.request() validates headers, then options, then looks at the session state > a session that is destroyed fails every request that has valid arguments [1.25ms]
(pass) http2 client.request() validates headers, then options, then looks at the session state > a session that is closed fails every request that has valid arguments [1.30ms]
(pass) http2 client.request() validates headers, then options, then looks at the session state > a session that is closed by a GOAWAY fails every request that has valid arguments [1.1
... (truncated)

passes on PR (with fix)

ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/node/http2/node-http2-request-validation.test.ts"
bun test v1.4.3 (367d939d9)

test/js/node/http2/node-http2-request-validation.test.ts:
(pass) http2 client.request() validates headers, then options, then looks at the session state > on a session that is open [718.54ms]
(pass) http2 client.request() validates headers, then options, then looks at the session state > on a session that is destroyed [145.55ms]
(pass) http2 client.request() validates headers, then options, then looks at the session state > on a session that is closed [319.81ms]
(pass) http2 client.request() validates headers, then options, then looks at the session state > on a session that is closed by a GOAWAY [146.12ms]
(pass) http2 client.request() validates headers, then options, then looks at the session state > a session that is destroyed fails every request that has valid arguments [69.27ms]
(pass) http2 client.request() validates headers, then options, then looks at the session state > a session that is closed fails every request that has valid arguments [6
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 943ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/128] gen ErrorCode+*.h
[2/128] gen generated_host_exports.rs
generated_host_exports.rs: 121 exports (host=5, lazy=10, generic=106, rust=0); 245 extern-C blocks audited
[3/128] gen cpp.rs (cppbind)
[4/128] gen JS modules (bundle-modules)
Preprocess modules (8549ms)
Bundle modules (125ms)
Postprocesss modules (166ms)
Bundle Functions (503ms)
Generate Code (46ms)

[9.40s] Bundled "src/js" for production
  2606 kb
  197 internal modules
  13 native modules
  50 internal functions across 16 files
[4/12] cargo bun_runtime → libbun_runtime.a
�[1m�[33mwarning�[0m�[1m: binary `bun_shim_impl` should have a kebab-case name�[0m
   �[1m�[94m|�[0m
�[1m�[94m 1�[0m �[1m�[94m|�[0m /workspace/bun/build/release/rust-target/.../bun_shim_impl
   �[1m�[94m|�[0m                                              �[1m�[33m^^^^^^^^^^^^^�[0m
   �[1m�[94m|�[0m
   �[1m�[94m= �[0m�[1mnote�[0m: `cargo::non_kebab_case_bins` is set to `warn` by default
�[1m�[96mhelp�[0m: to change the binary name to `bun-shim-impl`, convert `bin.name`
  �
... (truncated)

diff hotspot

src/js/node/http2.ts                               | 145 +++++-----
 .../http2/node-http2-request-validation.test.ts    | 316 +++++++++++++++++++++
 2 files changed, 384 insertions(+), 77 deletions(-)

gate history · 1 passed · 1 rejected · iteration 1

evidence per changed file

file                                                      reads  edits  tests
src/js/node/http2.ts                                         12     15     24
test/js/node/http2/node-http2-request-validation.test.ts      0      1      9

[coderabbitai]

Warning

Review paused — included plan limit reached

Keep your review moving with free on-demand reviews.

• Run this review for free

On-demand reviews are free for one more day.

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Promotion and pricing details

On-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file.

Review limit details

Or wait 46 seconds for your next included review.

Check out review usage here.

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

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: a61fb3e4-b0b7-41b2-8c3f-c8f1a8f2c44a

📥 Commits

Reviewing files that changed from the base of the PR and between 9b7c982 and 15163c2.

📒 Files selected for processing (2)

• src/js/node/http2.ts
• test/js/node/http2/node-http2-request-validation.test.ts

---

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

[robobun]

^{Updated 7:37 PM PT - Sep 19th, 2026}

✅ @robobun, your commit 15163c25015d54055860f3d63ebc2cf9ab39902e passed in Build #118757! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 43536

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

bun-43536 --bun

[robobun]

Status: fix pushed in #43536. Waiting for CI and review.

How I reproduced it. Run this with node (v26.3.0) and with bun (1.4.3-canary, main at 26e7a4b):

import http2 from "node:http2";
const thrown = fn => { try { fn().on("error", () => {}); return "no throw"; } catch (e) { return e.code; } };
const srv = http2.createServer();
srv.listen(0, "127.0.0.1", () => {
  const c = http2.connect("http://127.0.0.1:" + srv.address().port);
  c.on("error", () => {});
  c.on("connect", () => {
    console.log("open     ", thrown(() => c.request({ "bad name": "x" }, { endStream: 1 })));
    c.destroy();
    console.log("destroyed", thrown(() => c.request({ "bad name": "x" })));
    console.log("destroyed", thrown(() => c.request({ ":method": "CONNECT" })));
    srv.close();
  });
});

	node v26.3.0	bun before	bun with this PR
open	ERR_INVALID_HTTP_TOKEN	ERR_INVALID_ARG_TYPE	ERR_INVALID_HTTP_TOKEN
destroyed	ERR_INVALID_HTTP_TOKEN	no throw	ERR_INVALID_HTTP_TOKEN
destroyed	ERR_HTTP2_CONNECT_AUTHORITY	no throw	ERR_HTTP2_CONNECT_AUTHORITY

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked that the moved blocks stay inside the same try so the connectionsCounted bookkeeping in the finally is unaffected, and that the DEP0194 warning now firing on a closed/destroyed session before the call fails matches Node's order (the warning sits in the options copy, which Node also runs before the session-state check).

Extended reasoning...

The reordering keeps every relocated block inside the existing try/finally, and connectionsCounted is only set after the native submission, so no early return introduced by the move can leak or double-decrement the counter. The setupRequestEndAndSignal helper called on the destroyed branch is the same one the pending/submitted paths already use (req.end() when endStream, plus an abort listener disposed on close), so the destroyed stream's shape now matches the other returned streams. The four candidates in the ruled-out list (send-time rejection streams missing the signal wiring, the duck-typed signal TypeError, the already-aborted signal on a closed session, and the earlier DEP0194 warning) are either pre-existing on the base or are exactly the Node ordering the PR cites, and the inline findings already cover what remains.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🟡 src/js/node/http2.ts — nit, pre-existing: callers whose request nghttp2 would reject at send time get a stream with no endStream applied and no abort listener, unlike the destroyed-session stream this PR fixes. The :path rejection at src/js/node/http2.ts:6166-6172 and the content-length rejection at 6182-6187 build the stream and return without calling setupRequestEndAndSignal. Fix: call setupRequestEndAndSignal(req, options, signal) on every early-return stream that is destroyed on the next tick, so writableEnded, the 'aborted' event and options.signal behave the same on all three sites (6137, 6166, 6182).

  Extended reasoning...

  The PR's destroyed-session branch at src/js/node/http2.ts:6136-6141 now calls setupRequestEndAndSignal so the returned stream is ended (endStream) and listening to options.signal before destroyWithInvalidSessionNT runs. The two sibling early returns that immediately follow it were left as they were. request({ ":path": "/a b" }, { signal: controller.signal }) on an open session hits line 6166: a stream is created, emitStreamErrorNT is queued, and the stream is returned with no abort listener and without req.end(). controller.abort() in the same tick therefore does nothing; the stream later fails with ERR_HTTP2_STREAM_ERROR instead of AbortError. For a GET (options.endStream defaulted to true at 6092) the writable side is still open when emitStreamErrorNT destroys the stream, so _destroy at 2568-2574 sets kAborted and emits 'aborted' for a request the peer never saw; on the destroyed-session branch the same GET now correctly has writableEnded true and no 'aborted'. Same for the content-length path at 6182-6187 (rejectNoPayloadContentLengthNT). The base branch behaves the same way on these two…

  Verification: nit, pre-existing. Triggering condition: a caller on an open session passes { endStream: true } and/or { signal } together with a request nghttp2 would reject at send time (a :path containing SP/control/DEL, or a content-length on a no-payload method). Mechanism verified in /home/claude/bun/src/js/node/http2.ts at HEAD. The destroyed-session branch (lines 6136-6141) now calls…

• 🟡 src/js/node/http2.ts — Callers whose request() on a destroyed session carries a non-string :method (for example a number) now get a synchronous plain TypeError; on the base they got a stream that failed on the next tick with ERR_HTTP2_INVALID_SESSION. The destroyed check moved below src/js/node/http2.ts:6087, so method.toUpperCase() now runs on a destroyed session for a value that is truthy but not a string. Fix: validate :method as a string (or coerce with String()) before NoPayloadMethods.has so both open and destroyed sessions report the same $ERR_* error rather than a bare TypeError.

  Extended reasoning...

  On the base, this.destroyed was checked at old line 5901 before any header processing, so a destroyed session returned a stream regardless of the :method value. Now headers are processed first: src/js/node/http2.ts:6050 reads headers[':method'], a truthy non-string such as 1 or an object passes the !method default, and src/js/node/http2.ts:6087 calls method.toUpperCase(), which throws TypeError 'method.toUpperCase is not a function'. The destroyed branch at http2.ts:6136 is never reached. The finder dismissed it as pre-existing because open sessions threw the same TypeError on the base, but the destroyed-session population is new: code that fires a request during teardown and relies on the stream 'error' channel now gets an uncaught synchronous TypeError. The PR description acknowledges a separate fix (#37796) that is not in this checkout, so the regression lands if this merges first. Remedy: validate typeof method === 'string' before the toUpperCase call, ahead of the session-state checks.

  Verification: nit — acknowledged in PR description (not in the diff): "A non-string :method throws a plain TypeError from method.toUpperCase(). A destroyed session now reaches that line too. #37796 fixes the line." The note is accurate. Triggering condition: request() on a destroyed ClientHttp2Session with a truthy non-string :method (e.g. { ":method": 1 }). Mechanism verified in… | nit —…

[robobun]

This PR and #43491 both move blocks in request(), so they conflict.

#43491 adds the headers and options type checks (assertIsObject) to the seven methods that take them. In request(), assertIsObject(options, "options") at the top made 20 doubly invalid calls report the options error where main gives node's answer, for example request({ "bad name": "x" }, 5). So #43491 also moves the options checks and the session-state checks below the header validation. That reorder is the minimum that the type check needs. The reorder here is more complete (the options.signal check, the field order, the stream of a destroyed session).

The two fit together. If this PR merges first, #43491 drops its reorder and keeps two lines in request(): the headers check in the last branch of the header block, and assertIsObject(options, "options") right above the options copy, the place you named in your notes. If #43491 merges first, the moved blocks here shrink to the parts that #43491 does not touch.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[robobun]

Reply to the review on 8914280. Two of the three findings are fixed in 496891c.

• Non-string :method. Fixed. request() skips the no-payload default when :method is not a string, so the value goes to the encoder, which sends String(value). Node does the same. New tests: an open session sends { ":method": 1 } as :method: 1, and a destroyed or closed session fails that request with the session-state error, not a TypeError.
• Hand-built ERR_HTTP2_GOAWAY_SESSION. Fixed at all three sites (the two branches in request(), now one, and streamRejectedByGoawaySession). They throw $ERR_HTTP2_GOAWAY_SESSION(). String(error) is now Error [ERR_HTTP2_GOAWAY_SESSION]: ... like node, and the tests assert that string.
• setupRequestEndAndSignal on the :path and content-length rejections. Not in this PR. Those two paths copy what nghttp2 does at send time. They do not depend on the session state. With the helper they still differ from node: node emits finish, error, close, and bun would emit ready, error, close. That needs its own change with the event order checked on node.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[robobun]

Follow-up to my note above: #43491 is now stacked on this PR. Its base is this branch, and I merged this branch into it, so request() there is this PR's version plus two lines: the headers type check in the last branch of the header block, and assertIsObject(options, "options") right above the options copy. The block move that #43491 had is gone.

On the merged tree, node-http2-request-validation.test.ts passes 11 of 11, node-http2.test.js passes 394 of 394 (6 skipped), and all 261 vendored test-http2-*.js pass. If this branch is rebased or force-pushed, #43491 needs a new merge. A comment on #43491 tells me.