node:http2.createServer — fix h2c compatibility with strict peers#29075
node:http2.createServer — fix h2c compatibility with strict peers#29075
Conversation
ServerHttp2Session's initial SETTINGS frame advertised ENABLE_PUSH=1,
and Http2ServerResponse.end(body) emitted an extra empty DATA frame
followed by a zero-length trailer HEADERS frame. Strict peers (curl's
nghttp2, Node's http2 client) rejected the connection with
NGHTTP2_ERR_CALLBACK_FAILURE — hence 'Empty reply from server' from
curl --http2-prior-knowledge.
RFC 9113 §7.2.2: a server MUST NOT send SETTINGS_ENABLE_PUSH with a
value other than 0 — clients treat nonzero as PROTOCOL_ERROR. Default
the server's initial settings to enablePush: false (user settings
still override).
The compat Http2ServerResponse always passes waitForTrailers: true to
stream.respond() and then unconditionally calls sendTrailers({}) from
onStreamTrailersReady. Track waitForTrailers in a stream-local symbol
set from respond(); in _final, drive the wantTrailers path directly
instead of writing a bare empty DATA frame first. Also short-circuit
Http2Stream.sendTrailers to the native noTrailers path when the
header object is empty so that the stream terminates with a single
empty DATA + END_STREAM, matching Node's wire output.
Fixes #29073
|
Updated 6:14 AM PT - Apr 9th, 2026
❌ @robobun, your commit 9b14d91 has 3 failures in
Add 🧪 To try this PR locally: bunx bun-pr 29075That installs a local version of the PR into your bun-29075 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
WalkthroughAdd per-stream trailer coordination for HTTP/2, treat empty trailer objects as "no trailers", adjust stream finalization to wait for trailers when requested, set StreamResponded before emitting stream events, force-disable server push in parser/settings, and add five h2c regression tests validating on-the-wire framing and 204 behavior. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- respond(): only set bunHTTP2WaitForTrailers when the HEADERS frame
did not already close the stream. Status codes 204/205/304 and HEAD
requests force endStream=true earlier and the native request() writes
END_STREAM on the HEADERS frame; driving the wantTrailers path from
_final on such a stream would call noTrailers/emit("wantTrailers") on
an already-half-closed stream. Guard with '!endStream'.
- ServerHttp2Session: move 'enablePush: false' to the end of the
settings spread so user-supplied settings cannot re-enable push.
RFC 9113 §7.2.2 is unconditional — a server MUST NOT advertise
SETTINGS_ENABLE_PUSH != 0.
- 29073 test: parse frames incrementally and resolve on the stream's
terminating DATA/HEADERS frame with END_STREAM instead of waiting
3s for a socket close that never comes (HTTP/2 keeps the TCP
connection open after a stream ends). First test drops from ~4.1s
to ~1.1s.
- Add a 204 No Content regression test exercising the respond()
guard above.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29073.test.ts`:
- Around line 107-155: Add a second regression test that constructs the server
with a caller-supplied settings object to ensure the server still advertises
SETTINGS_ENABLE_PUSH=0 even when the user passes enablePush: true; specifically,
duplicate the existing test "http2.createServer serves h2c response with
well-formed frames (`#29073`)" but call http2.createServer({ settings: {
enablePush: true } }, ...) and reuse rawH2cRequest to inspect frames, then
assert (using the same logic around serverSettings and checking setting id 0x02)
that the server's SETTINGS frame reports ENABLE_PUSH value 0; place this new
test alongside the existing one so it validates the ServerHttp2Session path that
previously had the spread-order bug.
🪄 Autofix (Beta)
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: 17462fef-cb7e-4ca8-b6ab-54c7adc57171
📒 Files selected for processing (2)
src/js/node/http2.tstest/regression/issue/29073.test.ts
There was a problem hiding this comment.
LGTM — all previously raised issues are fixed; the remaining nit about #sentTrailers assignment in the direct noTrailers path of _final() is minor and limited to low-level ServerHttp2Stream consumers.
Extended reasoning...
Overview
The PR fixes two root-cause bugs in src/js/node/http2.ts that caused strict HTTP/2 peers (curl/nghttp2, Node's own http2.connect) to reject connections from Bun's http2.createServer:
ServerHttp2SessionadvertisedSETTINGS_ENABLE_PUSH=1— an RFC 9113 §7.2.2 PROTOCOL_ERROR.- The compat
Http2ServerResponsepath emitted an extra empty DATA frame plus an empty trailer HEADERS frame instead of a single empty DATA+END_STREAM.
A new regression test file (test/regression/issue/29073.test.ts) exercises both the raw TCP/frame-level path and the http2.connect client path, including a 204 response test added in the follow-up commit.
Security risks
No security-sensitive code (auth, crypto, permissions) is touched. The changes are protocol-compliance fixes in the HTTP/2 transport layer with no credential or access-control implications.
Level of scrutiny
Moderate — HTTP/2 protocol implementation is subtle, but the three changes are narrow and well-motivated:
enablePush: falselast in the spread (RFC-unconditional override) ✓if (options.waitForTrailers && \!endStream)guard prevents callingnoTrailerson already-closed streams for 204/205/304/HEAD ✓sendTrailers({})short-circuits tonoTrailersto avoid zero-length trailer HEADERS ✓
The test is event-driven (END_STREAM detection, no setTimeout) and covers the fixed paths directly.
Other factors
All three bugs I flagged in prior review rounds were addressed. The one remaining nit (inline comment on this run) is that this.#sentTrailers is not assigned when _final() takes the no-listener noTrailers() shortcut, leaving stream.sentTrailers returning undefined instead of {} and theoretically allowing a redundant noTrailers() call if sendTrailers() is invoked after end(). This only affects direct consumers of the low-level ServerHttp2Stream API (the compat layer is unaffected) and requires an unusual post-end() sendTrailers() call to trigger the double-call path — acceptable as a follow-up.
- _final()'s direct native.noTrailers() path (no wantTrailers listener)
now assigns this.#sentTrailers = {} so a later stream.sendTrailers()
call trips the ERR_HTTP2_TRAILERS_ALREADY_SENT guard instead of
calling native.noTrailers() a second time on an already-half-closed
stream. The emit("wantTrailers") path already ends up in
sendTrailers({}) which assigns #sentTrailers itself, so this just
makes the two branches consistent. Only affects direct consumers of
the low-level ServerHttp2Stream API; the compat Http2ServerResponse
always registers a wantTrailers listener.
- Add a regression test exercising createServer({ settings: {
enablePush: true } }, ...) to lock in the spread-order fix: the
server must still advertise SETTINGS_ENABLE_PUSH=0 regardless of
caller input (RFC 9113 §7.2.2 is unconditional).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/http2.ts`:
- Around line 3121-3127: The Server may still send ENABLE_PUSH=1 when a caller
later invokes ServerHttp2Session.settings(...); update the
ServerHttp2Session.settings method to coerce/override any user-supplied
enablePush value to false before emitting the SETTINGS frame or merging into the
session's settings (i.e., ensure enablePush: false is applied last when building
the outgoing settings object, similar to the initial settings merge shown in the
settings: { ...options, ...options?.settings, enablePush: false } line); locate
the ServerHttp2Session.settings function and make sure any path that forwards
settings to the wire or internal store forcibly sets enablePush to false.
🪄 Autofix (Beta)
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: efab157e-8fc7-4bbc-a58c-e3d868d873e7
📒 Files selected for processing (2)
src/js/node/http2.tstest/regression/issue/29073.test.ts
The initial SETTINGS frame is fixed at construction, but a later
session.settings({ enablePush: true }) on a ServerHttp2Session would
still forward ENABLE_PUSH=1 to the wire mid-connection. RFC 9113
§7.2.2 applies to every SETTINGS frame a server sends, not just the
initial one. Force-override enablePush=false in the server's
settings() method before forwarding to the parser (ClientHttp2Session's
settings() is untouched — clients are allowed to advertise enablePush).
Add a regression test that installs a 'session' handler which calls
session.settings({ enablePush: true }) as soon as a session comes up,
then asserts every non-ACK SETTINGS frame on the wire reports
ENABLE_PUSH=0.
The server/client StreamResponded bit was set via stream[bunHTTP2StreamStatus] = status | StreamState.StreamResponded where `status` was captured BEFORE synchronously dispatching the 'stream' / 'response' event to user code. If the user handler sets any other bits on the same stream (via stream.respond/stream.end, both call paths set WantTrailer/FinalCalled/EndedCalled/WritableClosed), the later write back of the stale `status` clobbers them. In the async-wantTrailers path this manifests as ERR_HTTP2_TRAILERS_NOT_READY when the user listener finally calls sendTrailers after awaiting I/O. Switch both call sites to `stream[bunHTTP2StreamStatus] |= StreamState.StreamResponded` (atomic read-modify-write) and set the bit BEFORE dispatching the event so the stored value reflects any bits the user handler adds.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/node/http2.ts:3118-3130— The PR correctly forcesenablePush: falsein the wire SETTINGS frame (constructor spread at line 3127), butServerHttp2Session.#localSettingsis still initialized withenablePush: true(the shared default), sosession.localSettings.enablePushreturnstruein asessionevent handler even though the wire already advertisedENABLE_PUSH=0. Fix: change the#localSettingsfield initializer inServerHttp2SessionfromenablePush: truetoenablePush: falseto match the actual wire behavior from the start.Extended reasoning...
The bug and how it manifests
ServerHttp2Sessionshares the same#localSettingsfield initializer asClientHttp2Session, which defaults toenablePush: true. Before this PR, both the wire SETTINGS frame and#localSettingsagreed onenablePush: true— wrong per RFC 9113 §7.2.2 but internally consistent. This PR correctly fixes the wire by placingenablePush: falselast in the H2FrameParser settings spread (line 3127), so the initial SETTINGS frame now advertisesENABLE_PUSH=0. However,#localSettingsis never given a corresponding server-side override, so it still starts asenablePush: true. The PR therefore introduces a new inconsistency: the wire is correct, but the JS API disagrees until the client's SETTINGS ACK arrives and thelocalSettingshandler at line 2951 () overwrites the stale value.The specific code path that triggers it
sessionevent handlers run viaprocess.nextTick(line 3137:process.nextTick(emitConnectNT, this, socket)), which fires before any network I/O. At that point#localSettingsholds its initial value. Any code that readssession.localSettings.enablePushinside asessionhandler will observetruewhile the wire has already sentENABLE_PUSH=0:server.on("session", session => { console.log(session.localSettings.enablePush); // true — wrong, wire sent 0 });
Why existing code does not prevent it
The
localSettingscallback that corrects#localSettingsonly fires once the SETTINGS ACK round-trip completes. There is no mechanism to pre-seedServerHttp2Session.#localSettingswith server-specific defaults separate from the shared class initializer. Node.js initializes its server-sessionlocalSettingswithenablePush: falsefrom the very start.What the impact would be
The window from session creation until the first
localSettingsevent is narrow but real. It encompasses thesessionevent handler, which is the canonical place to inspect or reconfigure a newly established session. Code comparingsession.localSettings.enablePushagainstfalse(e.g., to guard push-related logic) would get the wrong answer during this window, diverging from Node.js behavior.How to fix it
Change the
#localSettingsfield initializer inServerHttp2SessionfromenablePush: truetoenablePush: false. This mirrors what Node.js does and ensures the JS API is consistent with the wire from the very first tick. ThelocalSettingshandler will continue to update#localSettingson SETTINGS ACK as before.Step-by-step proof
- Client connects; Bun creates a
ServerHttp2Sessionwhose#localSettings.enablePushistrue(the shared default). - The H2FrameParser constructor sends the initial SETTINGS frame with
ENABLE_PUSH=0(from the fixed spread{ ...options, ...options?.settings, enablePush: false }). process.nextTick(emitConnectNT, this, socket)queues thesessionevent before any I/O.- On the next tick the
sessionevent fires.session.localSettings.enablePushreads#localSettings→true. - Much later, the client sends a SETTINGS ACK. The
localSettingshandler fires and sets#localSettings = { ..., enablePush: false }. - After step 5,
session.localSettings.enablePushcorrectly returnsfalse— but the staletruewas observable in step 4.
- Client connects; Bun creates a
| if (options.waitForTrailers && !endStream) { | ||
| this[bunHTTP2WaitForTrailers] = true; | ||
| } |
There was a problem hiding this comment.
🔴 The PR introduces if (options.waitForTrailers && !endStream) inside the else branch of ServerHttp2Stream.respond() but provides no null guard — passing null as options enters that branch (typeof null === 'object', not 'undefined') and options.waitForTrailers throws TypeError: Cannot read properties of null. The fix is options?.waitForTrailers && !endStream.
Extended reasoning...
What the bug is and how it manifests
The PR adds a new property access at line 2692 inside the else branch of ServerHttp2Stream.respond():
} else {
session[bunHTTP2Native]?.request(this.id, undefined, headers, sensitiveNames, options);
if (options.waitForTrailers && !endStream) { // <-- bug: no null guard
this[bunHTTP2WaitForTrailers] = true;
}
}
The else branch runs when typeof options !== 'undefined'. Since typeof null === 'object', calling respond(headers, null) on a non-204/205/304/HEAD response (where endStream stays false) reaches options.waitForTrailers on a null value, throwing TypeError: Cannot read properties of null (reading 'waitForTrailers').
The specific code path that triggers it
Calling stream.respond({ ':status': 200 }, null): (1) options?.endStream uses optional chaining safely, endStream = false; (2) the 204/205/304/HEAD force-endStream block does not run, options remains null; (3) options?.sendDate uses optional chaining safely; (4) typeof null === 'object', so typeof options !== 'undefined' is true, the else branch is entered; (5) options.waitForTrailers throws TypeError.
Why existing code does not prevent it
Before this PR, the else branch only called native.request(..., options) and never accessed any property of options directly. The PR introduces the first direct property access on options in that branch without a null guard. The optional-chaining pattern used earlier in the same method (options?.endStream, options?.sendDate) is not applied here.
What the impact would be
Any direct caller of the low-level ServerHttp2Stream.respond() API who passes null as the second argument on a non-204/304 response gets an unhandled TypeError that crashes the stream handler. The compat Http2ServerResponse layer always supplies a proper options object (lines 1089-1094), so compat users are unaffected. This is a new crash path introduced by this PR that did not exist before it.
How to fix it
Change line 2692 to use optional chaining, consistent with the rest of the method:
if (options?.waitForTrailers && !endStream) {
This is a one-character fix that restores tolerance for null options while correctly gating the WaitForTrailers flag.
Step-by-step proof
- stream.respond({ ':status': 200 }, null) is called.
- endStream = null?.endStream ?? false -> false (optional chaining is safe here).
- Status 200 does not match 204/205/304/HEAD — force-endStream block skipped, options stays null.
- typeof null === 'object' -> typeof options !== 'undefined' -> true -> else branch executes.
- options.waitForTrailers -> null.waitForTrailers -> TypeError: Cannot read properties of null (reading 'waitForTrailers').
| settings = { ...settings, enablePush: false }; | ||
| validateSettings(settings); | ||
| this.#pendingSettingsAck = true; |
There was a problem hiding this comment.
🔴 In ServerHttp2Session.settings(), the PR adds 'settings = { ...settings, enablePush: false }' BEFORE validateSettings(settings) (lines 3275-3277). JavaScript's spread operator converts any value to a plain object — so the type guards in validateSettings (checking typeof settings !== 'object', settings === null, isArray(settings)) always pass on the reassigned value. session.settings(null), session.settings([]), and session.settings(42) no longer throw ERR_INVALID_ARG_TYPE as Node.js requires — they silently proceed and send a SETTINGS frame containing only enablePush: false. Fix: swap the two lines so validateSettings(settings) runs first, then apply the RFC override.
Extended reasoning...
What the bug is and how it manifests
The PR's fix for mid-connection RFC 9113 compliance adds this block to ServerHttp2Session.settings():
settings = { ...settings, enablePush: false };
validateSettings(settings);
In JavaScript, the spread operator is defined to call ToObject() on the operand before copying own enumerable properties. As a result:
- { ...null, enablePush: false } => { enablePush: false } (null is silently ignored)
- { ...[], enablePush: false } => { enablePush: false } (array with no own string keys)
- { ...42, enablePush: false } => { enablePush: false } (primitives have no own enumerable properties)
The reassigned settings is now always a non-null, non-array plain object, so every branch in validateSettings (lines 121-123: typeof settings !== 'object' || settings === null || isArray(settings)) evaluates to false, and no ERR_INVALID_ARG_TYPE is thrown.
The specific code path that triggers it
A caller that does session.settings(null), session.settings([]), or session.settings(42) on a ServerHttp2Session instance will hit the spread-then-validate order. Each such call silently produces a SETTINGS frame on the wire containing only { enablePush: false } instead of throwing ERR_INVALID_ARG_TYPE.
Why existing code does not prevent it
validateSettings (around line 121) opens with:
if (typeof settings \!== 'object' || settings === null || isArray(settings)) {
throw new ERR_INVALID_ARG_TYPE(...)
}
This guard is correct on its own, but the reassignment placed before it means it can never fire for the invalid input types listed above — the guard only ever sees a plain object. Contrast with ClientHttp2Session.settings() (around line 3719), which calls validateSettings(settings) first and makes no spread reassignment, so it correctly throws for invalid inputs.
What the impact would be
This is a Node.js compatibility regression. Production code that relies on ERR_INVALID_ARG_TYPE being thrown for bad settings arguments on a server session will silently swallow the error, potentially sending unexpected SETTINGS frames and masking programmer mistakes with no observable error.
How to fix it
Swap the two lines:
validateSettings(settings); // validate the original input first
settings = { ...settings, enablePush: false }; // then apply the RFC override
This preserves all existing validation behaviour while still ensuring enablePush: false is forwarded to the parser on every code path.
Step-by-step proof
- Caller invokes session.settings(null).
- PR code: settings = { ...null, enablePush: false } => settings is now { enablePush: false }.
- validateSettings({ enablePush: false }): typeof settings === 'object' (pass), not null (pass), not array (pass) — guard passes with no error thrown.
- this.#parser?.settings({ enablePush: false }) sends a SETTINGS frame to the wire.
- Node.js behaviour: validateSettings(null) runs first => typeof null === 'object' && null === null => throws ERR_INVALID_ARG_TYPE. No SETTINGS frame is sent.
1 participant
Fixes #29073.
Repro
curl --http2-prior-knowledge http://localhost:8000against Bun:Cause
Two distinct bugs on Bun's server side, either of which is enough for strict h2 peers (curl's nghttp2, Node's
http2.connect) to reject the connection. They were only visible against strict peers — Bun's ownhttp2.connectclient tolerated both, which is why Bun-to-Bun tests andBun.serve({ http2: true })worked.1. Server SETTINGS advertised
ENABLE_PUSH=1.FullSettingsPayloaddefaultsenablePush = 1for both client and server, andServerHttp2Sessionnever overrode it. RFC 9113 §7.2.2:nghttp2 reports this as
NGHTTP2_ERR_CALLBACK_FAILUREon the first frame after the preface — exactly what curl surfaces.2.
res.end(body)emitted an extra empty DATA frame + a zero-length trailer HEADERS frame. The compat layer always setswaitForTrailers: trueonstream.respond()and thenonStreamTrailersReadyunconditionally callssendTrailers(this[kResponse][kTrailers])with the response's (empty) trailers object. Bun's wire output was:Node's wire output is:
An empty trailer HEADERS frame has no valid HTTP fields, which nghttp2 rejects; even when that's fixed, emitting an extra empty DATA frame without END_STREAM followed by another empty DATA with END_STREAM trips nghttp2's strict state machine.
Fix
All changes are in
src/js/node/http2.ts:ServerHttp2Sessionconstructor now defaults its initial parser settings toenablePush: false. User-provided settings still override ({ enablePush: false, ...options, ...options?.settings }).ServerHttp2Stream.respond()tags the stream with a privatebunHTTP2WaitForTrailerssymbol when called withwaitForTrailers: true.Http2Stream._final()checks that flag. When set, it skipsnative.writeStream(id, "", "ascii", true, cb)(which would emit the extra empty DATA) and instead drives the wantTrailers path directly — either emittingwantTrailers(so user handlers can callsendTrailers(realTrailers)) or calling nativenoTrailersif no listener. The compat layer'sonStreamTrailersReadystill callssendTrailers({})as before; the short-circuit below routes that to the single-empty-DATA-END_STREAM path.Http2Stream.sendTrailers()short-circuits to nativenoTrailerswhen the processed header object has zero keys, matching Node's wire output (single empty DATA + END_STREAM, no trailer HEADERS frame). Non-empty trailers still go through the normalsendTrailersnative binding.Client streams are unaffected — the
bunHTTP2WaitForTrailersflag is only set fromrespond(), which clients never call.Verification
After the fix, Bun's wire against the same curl command:
curl --http2-prior-knowledgereturnsok.Regression test —
test/regression/issue/29073.test.tsTwo tests:
ENABLE_PUSH != 0entry in the server's initial SETTINGS,ok,http2.connectclient against the same server reads the response body end-to-end.Existing
test/js/node/test/parallel/test-http2-trailers*tests (non-empty trailers) andtest-http2-compat-serverresponse-{end,close,finished}.jsall continue to pass under the debug build.Likely related issues
@grpc/grpc-jsserver on Bun emits repeated empty HTTP/2 DATA frames; Envoy aborts withPROTOCOL_ERROR/ "Too many consecutive frames with an empty payload". The extra-empty-DATA bug (_finalalways writing an empty close frame even withwaitForTrailers: true) matches the symptom precisely — Envoy's empty-frame guardrail is exactly what this PR removes.ERR_HTTP2_PROTOCOL_ERROR 200for browser-served assets in a docker container. TheENABLE_PUSH=1fix is a plausible match if HTTP/2 is in play (reporter's stack is express serving a vite bundle, so http/2 isn't guaranteed), but without a repro I'm not auto-closing it.Not using
Fixes #on either — leaving it to the reporters / verifiers to confirm on canary once this merges.