Repository navigation
Conversation
|
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:
WalkthroughAdds RFC 9113 HTTP messaging validation to the Rust HTTP/2 engine, threads local request method state through the frame parser, changes Node HTTP/2 client session GOAWAY and destroyed-session behavior, and adds conformance tests for the new messaging rules. ChangesRFC 9113 HTTP Messaging Conformance and Session Error Alignment
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:09 PM PT - Jul 5th, 2026
❌ @robobun, your commit 65dabec has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33004That installs a local version of the PR into your bun-33004 --bun |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 4894-4902: The `request()` path in `ClientHttp2Session` is
checking `this.destroyed` before the `this.closed` / `this[kReceivedGoaway]`
GOAWAY state, which causes the wrong session error after graceful close or
GOAWAY. Reorder the guards so the closed/GOAWAY branch is evaluated first, then
fall back to the destroyed branch, keeping the existing
`destroyWithGoawaySessionNT` and `destroyWithInvalidSessionNT` behavior tied to
the correct state.
In `@src/runtime/api/bun/h2/connection.rs`:
- Around line 1088-1096: The CONNECT handling in the response content-length
logic is too broad, since the current LocalRequestMethod::Connect branch
disables validation for all CONNECT responses. Update the logic in the
connection response path to exempt only successful CONNECT responses (2xx) and
keep normal content-length enforcement for non-2xx CONNECT replies, using the
existing method/status checks around content_length.
- Around line 1107-1122: In the BlockCategory::Request handling in
connection.rs, validate the required request pseudo-headers before mutating
stream state: ensure normal requests have :method, :scheme, and :path, allow
:protocol only for extended CONNECT, and reject plain CONNECT when :scheme or
:path are present. Mark these cases malformed before reaching the
self.streams.get_mut(&target) block so headers_received and content_length are
only committed for a fully valid request.
In `@test/js/node/http2/node-http2-http-messaging.test.ts`:
- Around line 475-481: streamErroredAndClosed should fail fast instead of
waiting forever when only one terminal event fires. Update the helper in
node-http2-http-messaging.test.ts to use explicit await/timeout-style assertions
or a rejection path so missing "error" or "close" events immediately reject,
rather than relying on Promise.all([errored, closed]) to settle. Keep the logic
centered around streamErroredAndClosed and its req.on("error") / req.on("close")
wiring so unexpected success or close-only paths become actionable test
failures.
- Around line 406-449: The test in node-http2-http-messaging.test.ts attaches
the server-side error handler too late around the raw HTTP/2 stream setup. Move
the stream.on("error") subscription for the server Http2 stream so it is
registered before writing the invalid DATA frame in the test flow around
gotStream/onStream and rstFromServer. This ensures any protocol error from the
violating request body is captured and the errored promise rejects instead of
hanging.
- Around line 55-80: The rstSentToServer Promise in the HTTP/2 test only
resolves when a RST_STREAM frame is observed, so it should also reject if the
peer closes or errors without sending that frame. Update the server-side setup
in node-http2-http-messaging.test.ts around the Promise.withResolvers and socket
handlers so the missing-RST case fails immediately instead of hanging, and keep
the existing frame parsing logic and onRst resolution path intact.
🪄 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: 3c68c254-b865-4c21-8f94-d14e80f64a25
📒 Files selected for processing (4)
src/js/node/http2.tssrc/runtime/api/bun/h2/connection.rssrc/runtime/api/bun/h2_frame_parser.rstest/js/node/http2/node-http2-http-messaging.test.ts
There was a problem hiding this comment.
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-http-messaging.test.ts`:
- Around line 488-492: Shorten the JSDoc for the helper in
node-http2-http-messaging.test.ts so it fits the repo’s 3-line comment limit.
Update the comment attached to the stream-error resolution helper to keep the
same intent but remove extra wording, preserving the description of resolving
with the emitted error after close and rejecting terminal regression paths. Use
the existing helper context in the http2 messaging test block to locate it.
🪄 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: 6c268d2a-995b-46e2-8566-99956f67fd4e
📒 Files selected for processing (1)
test/js/node/http2/node-http2-http-messaging.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/js/node/http2/node-http2-http-messaging.test.ts (2)
128-132: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winShorten this helper comment to the repo limit.
This JSDoc is still over the 3-line comment cap.
Proposed cleanup
-/** - * Like `exchange`, but the raw server also PUSH_PROMISEs stream 2 (carrying `promisedRequest`) - * on the main request and answers it with the frames `respondPushed` writes. Returns the PUSHED - * stream's events in emission order once it closes. - */ +// Like `exchange`, but also PUSH_PROMISEs stream 2 and returns +// the pushed stream's events once it closes.As per coding guidelines, “Keep code comments to 3 lines max.”
🤖 Prompt for 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. In `@test/js/node/http2/node-http2-http-messaging.test.ts` around lines 128 - 132, Shorten the JSDoc for the helper that documents the push-promise exchange so it fits the repo’s 3-line comment limit. Update the comment above the helper in node-http2-http-messaging.test.ts to keep only the essential description of what the raw server does and what the helper returns, trimming any extra wording while preserving the meaning.Source: Coding guidelines
173-195: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject when the main request finishes without a pushed stream.
pushClosedonly settles fromclient.on("stream")or session close. If the client drops the PUSH_PROMISE but completes the main request normally, this waits for the outer test timeout instead of failing at the missing-push invariant.Proposed fail-fast wiring
const { promise: pushClosed, resolve: onPushClose, reject: onNoPush } = Promise.withResolvers<void>(); + let sawPush = false; client.on("stream", pushed => { + sawPush = true; let body = ""; @@ await once(client, "connect"); const req = client.request({ ":path": "/" }); - req.on("error", () => {}); + req.once("error", err => { + if (!sawPush) onNoPush(err); + }); + req.once("close", () => { + if (!sawPush) onNoPush(new Error("the main request closed before the pushed stream arrived")); + }); req.resume();As per coding guidelines, “Await conditions, wire failures to reject.” Based on learnings, raw HTTP/2 tests should check locally constructed promises for missing terminal rejection paths.
🤖 Prompt for 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. In `@test/js/node/http2/node-http2-http-messaging.test.ts` around lines 173 - 195, The test logic around pushClosed in node-http2-http-messaging.test.ts can hang when the main request completes without ever receiving a pushed stream, because only client.on("stream") and client.on("close") settle it. Update the existing Promise.withResolvers flow and the client request handling so the missing PUSH_PROMISE path rejects immediately with the expected invariant failure instead of waiting for the outer timeout; use the existing client, pushClosed, onPushClose, and onNoPush wiring to add a fail-fast rejection when no pushed stream is observed.Sources: Coding guidelines, Learnings
🤖 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.
Duplicate comments:
In `@test/js/node/http2/node-http2-http-messaging.test.ts`:
- Around line 128-132: Shorten the JSDoc for the helper that documents the
push-promise exchange so it fits the repo’s 3-line comment limit. Update the
comment above the helper in node-http2-http-messaging.test.ts to keep only the
essential description of what the raw server does and what the helper returns,
trimming any extra wording while preserving the meaning.
- Around line 173-195: The test logic around pushClosed in
node-http2-http-messaging.test.ts can hang when the main request completes
without ever receiving a pushed stream, because only client.on("stream") and
client.on("close") settle it. Update the existing Promise.withResolvers flow and
the client request handling so the missing PUSH_PROMISE path rejects immediately
with the expected invariant failure instead of waiting for the outer timeout;
use the existing client, pushClosed, onPushClose, and onNoPush wiring to add a
fail-fast rejection when no pushed stream is observed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c0a7489-9c05-4cb7-9180-7938a34d5877
📒 Files selected for processing (2)
src/runtime/api/bun/h2/connection.rstest/js/node/http2/node-http2-http-messaging.test.ts
Status: the diff is green; the red lanes are CI infrastructureEverything in this PR is finished and reviewed. It needs a maintainer, not more commits from me. Summary of where CI actually stands, because the red X is misleading. The code is validatedBuild #68689 ran the current head
The previous build #68665 ran a byte-identical tree (its head differs only by one comment line, which the bundler strips) and finished 284 of 287 passed, zero test failures, with two of the same three infra faults. Binary size is +0.0 KB on every Linux target. The darwin failure is chronic, not caused by this PRThe
The job exits before running a single test, so it cannot be reporting anything about this diff. The sibling shard on the same OS/arch passes. (An earlier run, #66351, was additionally blocked for hours by an org-wide What was verified locally
Review stateAll 18 review threads from CodeRabbit and What a maintainer needsNothing from me. Either retry the one expired /cc @cirospaciari @Jarred-Sumner — flagged by both review bots as the owners of this subsystem. |
…path The engine enforces §8.1.1/§8.3 on inbound requests only: enforce_content_length returns early for a client, and the commit phase in finish_header_block is gated on is_server. A client therefore delivered a response shorter than its declared content-length as a clean 'end' with rstCode 0 (a truncated body indistinguishable from a complete one, the one signal HTTP/2 has), surfaced a response with no :status to JS as `:status === undefined`, and accepted request pseudo-headers in a response. Extend the same machinery to the response direction, mirroring nghttp2's nghttp2_http_on_response_headers: - content-length not met at the inbound END_STREAM, or exceeded by DATA, is a stream PROTOCOL_ERROR in both directions; the excess is never delivered - a response header section must carry exactly one :status, and it must be three digits, >= 100, and not 101 (§8.3.2) - the pseudo-headers a block may carry are now one allowlist keyed on its category (request / response / trailer), replacing the two partial checks that covered only `:status` inbound on a server and pseudo-headers in a trailer - §8.1.1 no-body exemptions: HEAD, 204/304, CONNECT, and 1xx interim responses HEAD and CONNECT responses need the request's :method, which the engine never sees for a locally-initiated stream (it flows through the legacy encoder), so the Sink trait gains a local_request_method() shim alongside is_local_stream() and request() records the :method. For a server-pushed stream the method instead comes from the PUSH_PROMISE block the engine decodes itself (nghttp2_http_record_request_method). Also: request() on a destroyed or closed session now returns a stream that errors on the next tick (ERR_HTTP2_INVALID_SESSION / ERR_HTTP2_GOAWAY_SESSION) instead of throwing, matching node v26. The write-only kSocketTeardown flag that gated half of that behavior is removed.
964387d to
183fe91
Compare
|
Rebased onto main (force-pushed #33072 ("hardening round 11") landed the same enforcement for the request direction while this PR was open, touching the same function. It added So this PR is not redundant, but it was also not right to merge two parallel implementations of the same rule. I resolved the conflict by rewriting this change on top of #33072's structure:
Net effect: What is left after the rebase: 16 of this PR's 28 tests still fail on current main, and they are exactly the response-direction half plus the Re-verified after the rebase:
The second commit is a separate, pre-existing bug worth calling out: |
There was a problem hiding this comment.
No further findings from me — everything I raised earlier is addressed and the current diff looks correct. That said, this rewires inbound message validation in the core h2 engine (header-block categorization, content-length enforcement on both DATA paths, stream eviction, plus the request()-after-teardown semantics change), so it's worth a look from someone who owns this subsystem before merging.
Extended reasoning...
Overview
This PR adds RFC 9113 §8 HTTP-messaging validation to Bun's from-scratch HTTP/2 engine (src/runtime/api/bun/h2/connection.rs). finish_header_block now categorizes each inbound block as request/response/trailer and enforces :status presence/validity, per-category pseudo-header sets, trailer END_STREAM, and content-length vs. received-DATA agreement (on HEADERS, DATA, streamed DATA, and trailers). A new LocalRequestMethod enum and Sink::local_request_method shim thread the request's :method from the legacy encoder (h2_frame_parser.rs) so HEAD/CONNECT/204/304 exemptions apply; PUSH_PROMISE-decoded methods are recorded on the engine's own Stream. The eviction sweep in replenish_windows() now skips a stream whose header block spans a CONTINUATION. In src/js/node/http2.ts, request() on a destroyed/closed/GOAWAY session now returns a next-tick-erroring stream instead of throwing (matching node v26.3.0), and the dead kSocketTeardown flag is removed. ~680 lines of new raw-frame conformance tests cover 27+ cases.
Security risks
The change tightens validation of peer-supplied wire data — malformed responses that were previously delivered are now reset. parse_status and parse_content_length are strict digit-only parsers with overflow checks; recv_body_bytes uses saturating_add. I don't see new injection, resource-exhaustion, or bounds issues introduced. The one behavioral loosening (request() no longer throws synchronously) is a node-compat alignment, not a security boundary.
Level of scrutiny
High. This is core protocol-engine logic on the hot path of every node:http2 connection, in both directions. It changes what the client accepts from every h2 server and (to a lesser extent) what the server accepts from clients. The header-block state machine, the two DATA delivery paths (buffered and streamed), the pushed-stream method plumbing, and the eviction-guard interaction are each subtle enough that a maintainer familiar with this engine should confirm the shape. This is not a mechanical or config-level change.
Other factors
The PR has been through several review rounds; all 17 threads (CodeRabbit's and mine) are resolved, including a real regression I flagged (pushed HEAD responses incorrectly reset) that was fixed with engine-side local_method recording plus a targeted test. Test coverage is thorough — positive, negative, pushed-stream, server-inbound, and the CONTINUATION-split edge case are all exercised — and the author verified byte-identical exit codes across all 272 upstream test-http2-* parallel tests. CI on the last full run was green modulo an org-wide darwin-14 agent outage. Given the scope and the fact that no human maintainer has reviewed yet, deferring rather than approving.
replenish_windows() removes every Closed engine stream at the end of each receive() batch. A header block that both closes the stream (END_STREAM on a HEADERS with no END_HEADERS) and spans a CONTINUATION therefore lost its stream entry whenever the two frames landed in different batches, before finish_header_block ran: on_stream_end had no entry to dispatch on, so the stream stayed open forever. With the response-direction checks it also loses the message state that validates the block, so a valid pushed HEAD response carrying a content-length was reset with PROTOCOL_ERROR. Skip the stream whose block is still being reassembled in the eviction sweep; it is removed on the next pass once the block completes.
183fe91 to
9df8057
Compare
…WAY_SESSION Both call sites now construct the error through $ERR_HTTP2_GOAWAY_SESSION(), so the parenthetical contradicted the code below it.
There was a problem hiding this comment.
No further findings after the rebase — all prior review threads are resolved and the latest commit only drops a stale comment. Deferring to a human on this one given the scope: ~190 lines of RFC 9113 §8 validation rewritten onto #33072's structure in the HTTP/2 engine core, plus the request()-after-teardown behavior change from throw to async error.
Extended reasoning...
Overview
This PR extends RFC 9113 §8 message-framing enforcement in Bun's HTTP/2 engine from the request direction (landed in #33072) to the response direction, so a node:http2 client now rejects truncated/overlong bodies, missing/invalid :status, and wrong-direction pseudo-headers instead of delivering them as clean successes. It touches four files: src/runtime/api/bun/h2/connection.rs (~190 net lines: LocalRequestMethod, parse_status, per-block-category pseudo-header allowlist, response commit phase, DATA-exceeds-content-length reset, eviction-during-CONTINUATION guard), src/runtime/api/bun/h2_frame_parser.rs (records the outbound :method and implements Sink::local_request_method), src/js/node/http2.ts (request() on a destroyed/closed/GOAWAY session now returns an erroring stream instead of throwing; kSocketTeardown removed as dead; GOAWAY error routed through $ERR_HTTP2_GOAWAY_SESSION), and a new 681-line test file with 28 raw-frame conformance cases.
Security risks
The change tightens validation of untrusted peer input (server responses), which is directionally safer — the pre-fix behavior of silently accepting a truncated body as complete is the actual hazard being closed. The risk to review is false positives: the new validation resetting valid traffic. Two such regressions were caught and fixed during review (pushed HEAD responses; eviction of a stream mid-CONTINUATION losing local_method), and negative-contract tests now cover HEAD/204/304/1xx/CONNECT/pushed-HEAD. No auth, crypto, or filesystem paths are touched.
Level of scrutiny
High. This is native protocol-engine code on the hot path of every HTTP/2 client connection, it replaces two existing pseudo-header checks with a single allowlist (so #33072's server-side conformance suite is also load-bearing here), and it went through a non-mechanical rebase that rewrote the change on top of #33072's fields/helpers. The request()-after-teardown change is a user-visible behavior change (throw → nextTick error) that the author verified against node v26.3.0 and node's own test-http2-client-destroy.js, but it affects every caller that currently try/catches request().
Other factors
The bug-hunting system found nothing this round. All 17 prior review threads (CodeRabbit + mine) are resolved; the author addressed each with a commit and a verified-against-node justification. Test coverage is thorough (28 new cases, 16 fail on main; #33072's 37-case suite still passes; full test/js/node/http2/ run is green modulo a documented pre-existing flake). CI on the pre-rebase head was green except for an org-wide darwin-14 agent outage. The PR description is unusually detailed about scope, out-of-scope items (§8.3.1 request pseudo-headers, status-specific content-length rules), and the rebase resolution. Given the blast radius and the amount of post-review reshaping, a maintainer familiar with the h2 engine (cirospaciari or Jarred-Sumner per CodeRabbit's suggestion) should sign off.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-06, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
The HTTP/2 engine enforces RFC 9113 §8 message framing on inbound requests only.
enforce_content_lengthreturns early for a client, and the commit phase infinish_header_blockis gated onis_server. So thenode:http2client delivers a response shorter than its declaredcontent-lengthas a clean, successful'end'withrstCode 0. HTTP/2 has no other framing signal, which makes a truncated download or JSON body indistinguishable from a complete one. Bun's HTTP/1 side already gets this right, and node errors the stream withNGHTTP2_PROTOCOL_ERROR.Repro
Every case below was confirmed against node v26.3.0 with byte-exact raw frames. Bun before this change:
content-length: 5, 2-byte bodycontent-length: 5, END_STREAM on HEADERScontent-length: 5, END_STREAM on trailerscontent-length: 1, 2-byte body:statuspseudo-header at allresponseeventresponsewith:status === undefined:statusthat is not 3 digits / < 100 /101:method) in a response:statusFix
Streamgainslocal_method; the §8.1.1/§8.3 commit phase infinish_header_blockis extended from the request direction to all three block categories, mirroring nghttp2'snghttp2_http_on_{request,response,trailer}_headers:content-lengthnot met at the inbound END_STREAM (on HEADERS, DATA, or trailers), or exceeded by a DATA chunk, is a stream error of type PROTOCOL_ERROR in both directions; the excess never reaches the application. (enforce_content_lengthloses its!is_serverearly return.):status, and it must be three digits,>= 100, and never101(§8.3.2).wrong_direction, which only rejected:statusinbound on a server, and theis_trailerclause) and additionally rejects request pseudo-headers in a response.HEAD and CONNECT responses need the request's
:method, and the engine has two ways to learn it: for a locally-initiated stream it never sees the outbound request headers (they flow through the legacy encoder), so theSinktrait gains alocal_request_method()transition shim alongside the existingis_local_stream()andrequest()records the:method; for a server-pushed stream the request arrives in the PUSH_PROMISE block the engine decodes itself, so it records that:methodon its ownStream(nghttp2'snghttp2_http_record_request_method) and the response arm prefers it.Second commit:
replenish_windows()evicted everyClosedstream at the end of eachreceive()batch, including one whose header block was still being reassembled across a CONTINUATION. That is a pre-existing bug (a header block that both closes the stream and spans a CONTINUATION lost its entry, soon_stream_endnever fired and the stream stayed open forever); with the response-direction checks it also loses the message state, so a valid pushed HEAD response was reset. The sweep now skips the stream whose block is mid-assembly.This PR also folds in the related
request()-after-teardown fix: node (verified v26.3.0, and node's owntest-http2-client-destroy.js) never throws fromrequest()on a destroyed or closed session; it returns a stream that errors on the next tick withERR_HTTP2_INVALID_SESSION(destroyed) orERR_HTTP2_GOAWAY_SESSION(closed / GOAWAY received). Bun threw synchronously except on one socket-teardown path gated by akSocketTeardownflag; that flag is now write-only dead code and is removed.What the rebase changed
#33072 landed
Stream.content_length/recv_body_bytes/recv_final_headers,parse_content_length,enforce_content_length, the trailer rules, and the 1xx-vs-trailer distinction — all for the request direction (enforce_content_lengthopens withif !self.is_server { return false; }, and its newh2-conformance.test.tscases are all"a request …"). The conflict was resolved by rewriting this change on top of that structure instead of merging two parallel implementations: no duplicated field, no second parser, and the pseudo-header allowlist replaces rather than sits beside the checks it subsumes. Net result is ~190 lines inconnection.rsinstead of the ~270 this PR carried before. 16 of this PR's 28 tests still fail on current main; the other 12 (repeated/non-integercontent-length, the trailer rules, and the negative-contract cases) are now covered by #33072 and are kept as regression cover.Intentionally out of scope
:method/:scheme/:path, §8.3.1).content-lengthfield rules (prohibited on 1xx, restricted on 204).content-length, node tears down the whole session (ERR_HTTP2_ERROR+ GOAWAYINTERNAL_ERROR). Bun surfaces a per-stream PROTOCOL_ERROR, which is what §8.1.1 specifies; matching node's session teardown there would be a regression.Verification
test/js/node/http2/node-http2-http-messaging.test.ts: 28 cases (a raw-frame TCP server driving thenode:http2client, the same driving a server-pushed stream, plus one raw client driving thenode:http2server). 16 fail on current main, all 28 pass with the fix. The negative-contract cases (exact-length body, HEAD/204/304 with acontent-length, 1xx-then-final, a pushed HEAD with acontent-length) prove valid traffic is untouched and that both:methodsources are load-bearing.bun bd test test/js/node/http2/h2-conformance.test.ts(Hardening round 11: input validation, bounds checks, lifetimes #33072's own suite, which the pseudo-header allowlist rewrites): 37 pass, 0 fail.bun bd test test/js/node/http2/: 398 pass, 1 fail. The failure (node-http2-streams-rehash.test.ts, 128 concurrent streams in a subprocess) is a pre-existing 5-second-timeout flake on debug+ASAN builds: it reproduces identically on unmodified main (3/3 runs).cargo clippy -p bun_runtimeandcargo fmt --checkclean; the first commit compiles on its own.