Repository navigation
Conversation
pushStream() filled the PUSH_PROMISE :authority only from the request's :authority. A request that carried host and no :authority (RFC 9113 8.3.1) produced a PUSH_PROMISE with no authority at all, which nghttp2 clients reject. A push that carried its own host also got the request's :authority next to it. Use getAuthority() for both the check on the push headers and the value read from the request headers, like node's ServerHttp2Stream.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 2 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 2 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 19 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 10:19 AM PT - Sep 18th, 2026
❌ @robobun, your commit b2306e4 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 43298That installs a local version of the PR into your bun-43298 --bun |
|
Status: the fix is at CI
Reproduction
Tests
Not in this PR: the client has the same gap for the origin that a 421 removes from Known difference from node that stays: node reads the authority of the request when it creates the stream. Bun reads it at push time, as bun 1.4.3 already does for |
There was a problem hiding this comment.
LGTM — a two-line change that makes pushStream()'s :authority default match node's core.js line for line, with tests for both new cases.
What was reviewed:
- Compared the new condition against node v26.3.0
core.js(getAuthority(headers) === undefined→getAuthority(this[kHeaders])); the BungetAuthorityhelper at src/js/node/http2.ts:456 has the same:authority-then-hostsemantics. - The "no
:authorityon a push that carrieshost" case (RFC 9113 8.4.1) is node parity, and the base branch previously sent both:authorityandhostthere — this is not a regression. - Existing
pushedHeaderBlockscallers pass at most two arguments, so the new defaulted options object leaves their behavior unchanged; the two new tests wire every error/close path to reject and assert whole header objects withtoEqual.
Extended reasoning...
Overview
The PR touches two files. In src/js/node/http2.ts the :authority default in ServerHttp2Stream.pushStream (around line 3247) changes from reading :authority alone on both the push headers and the parent request headers to routing both through the existing getAuthority() helper, which returns :authority if defined, else host. A short comment cites the node source line being mirrored. In test/js/node/http2/node-http2.test.js, the pushedHeaderBlocks helper gains a defaulted third options argument (requestHeaders, pseudoHeaders), a new pushedAuthority helper captures both the server's push.sentHeaders and the client-received PUSH_PROMISE headers, and two tests cover the two new behaviors (request with only host supplies the default; push with host gets no :authority added).
Security risks
None identified. The change only affects which header value is copied into an outbound PUSH_PROMISE header block on the server side; values still pass through the existing header validation and native serialization (pushPromise). It does not touch TLS, auth, or input parsing, and reduces rather than increases malformed frames (a PUSH_PROMISE with neither :authority nor host is rejected by nghttp2 and by Bun's own inbound engine).
Level of scrutiny
Low-to-moderate. The logic is two lines and is a direct port of node's core.js (if (getAuthority(headers) === undefined) headers[HTTP2_HEADER_AUTHORITY] = getAuthority(this[kHeaders])). I confirmed Bun's getAuthority at line 456 matches node's semantics. The one edge case worth thinking about — the parent request having neither :authority nor host, leaving an own-property :authority: undefined — is unchanged from the base branch (the old code also assigned parentRequestHeaders[':authority'] unconditionally) and node behaves the same way; the native header walk skips undefined values. The single candidate issue investigated (dropping :authority from a push that already carries host) is deliberate node parity, and the base branch previously sent both fields, which was the actual divergence from node.
Other factors
The test helper's signature change is backward compatible: I grepped every existing pushedHeaderBlocks( call site and none passes a third argument, so their captured blocks are unchanged. The new tests follow the file's conventions (Promise.withResolvers, every error/callback failure wired to reject, toEqual on whole header objects, no timeouts or sleeps). The bug hunt exited on dry_streak with no findings, no CODEOWNERS entry covers src/js/node/http2.ts or the test file, and there are no third-party review objections in the timeline. The change is small, self-contained, and behavior-verified against node, so a human look is not required.
|
One correction to the review above. The change does not match node line for line. The difference is the time at which the request side is read.
I ran a listener that changes the object before it calls
The second row shows that the late read is already in bun 1.4.3. This PR extends the same read to |
Keep the node permalink. The helper name and the comment a few lines above already say the rest.
Problem
stream.pushStream()sends a PUSH_PROMISE with neither:authoritynorhostwhen the request carriedhostand no:authority. A node client resets each push. After 1001 requests its session fails withERR_HTTP2_TOO_MANY_INVALID_FRAMES.:authoritydefault inpushStream()(src/js/node/http2.ts:3247) reads only:authority, on the push headers and the request headers.hostalso gets the request's:authority. Node sends onlyhost.Fix
getAuthority()helper (:authority, elsehost) on both sides of the default, like node (core.js#L2929-L2930). Node reads the request side at stream creation. Bun still reads it at push time.:authorityin every PUSH_PROMISE, and node sends only the user'shost.test/js/node/http2/node-http2.test.js(two new tests, both fail on bun 1.4.3). Alsoh2-conformance.test.ts,node-http2-client-close.test.ts, and 15 node parallelpushStream()tests.hostfor the origin that a 421 removes fromoriginSet(node:http2: a 421 response removes the session's origin from originSet when the request usedhost#43294).Background
:authorityor withhost. An RFC 7540 8.1.2.3 gateway sends onlyhost. So does a node or Bun client when the user passeshost.src/runtime/api/bun/h2/connection.rs:1276).Notes
Repro. The client half always runs under node, so only the server differs:
node v26.3.0 server:
bun 1.4.3 server (same on main):
With this change:
One session, 1200 sequential requests with
hostonly, one push per request:ERR_HTTP2_TOO_MANY_INVALID_FRAMESERR_HTTP2_SESSION_ERROR, code 11I did not trace the bun-to-bun row (which side sends code 11). This change removes the malformed pushes that start it. It does not change how either side handles a run of reset pushes.
Authority of the PUSH_PROMISE, same-process server and client (
<port>is the listen port):hostonly:authority: example.test:authority: example.testhostonlyhost: push.testhost: push.testhost: push.test(plus an:authority: undefinedkey onsentHeaders)host: push.test:authorityhost: push.testhost: push.test:authority: 127.0.0.1:<port>andhost: push.testhost: push.test:authority:authority: 127.0.0.1:<port>:authorityandhost:authorityhostonly:authority: push.test:authority: push.testHow a request with
hostonly arises. The object formrequest({ host })sends onlyhoston node and on Bun. The raw-array formrequest([":path", "/", "host", "example.test"])sends onlyhoston Bun. Node also sends the session's:authoritythere.Not changed here.
getAuthority(headers)when it creates the stream (core.js#L2887). Bun reads the stored request headers object at push time, and that object is the one the'stream'listener receives. If the listener setsheaders.host = "mutated.test"before it callspushStream(), node pushes:authority: example.testand this branch pushes:authority: mutated.test. Bun 1.4.3 already does this for:authority(a listener that setsheaders[":authority"]changes the push). This change extends the same read tohost. No issue tracks this yet.host#43294):ClientHttp2Session.request()setsreq.authorityfrom:authority, else from the session.removeOriginFromSet()uses it after a 421. A fix needs a decision on the raw-array form and on empty values, so it is not part of this change.:method,:path,:scheme,:authorityhere,:method,:authority,:scheme,:pathin node).undefined, as in node. The native header walk inpush_promiseskipsundefinedvalues (src/runtime/api/bun/h2_frame_parser.rs:6096). This was already the behavior. The only way there is ahostvalue that strict field whitespace validation drops. nghttp2 rejects that whole request.Test helper.
pushedHeaderBlocks()gets an options argument (requestHeaders,pseudoHeaders). The shape of its blocks does not change, so other tests that compare whole blocks are not affected.#43206 changes the adjacent line 3241 (where
parentRequestHeaderscomes from). It does not touch these lines.[human-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