Repository navigation
Conversation
…node does
An http2.connect() client accepted nine server-side protocol violations
that RFC 9113 / RFC 7541 require an endpoint to treat as errors and that
node (via nghttp2) rejects:
Connection-level (GOAWAY, session error ERR_HTTP2_ERROR):
* SETTINGS_ENABLE_PUSH with a value of 1 from a server (RFC 9113 8.4)
* DATA / HEADERS / WINDOW_UPDATE on an idle stream (5.1)
* a WINDOW_UPDATE that pushes a stream send window past 2^31-1 (6.9.1;
the check existed but never fired because locally-opened streams
were not in the engine map)
* an HPACK dynamic-table-size-update after the first field in a
header block (RFC 7541 4.2; lshpack accepts one at the start of
every decode() call so the position rule is enforced by the caller)
Stream-level (RST_STREAM(PROTOCOL_ERROR), ERR_HTTP2_STREAM_ERROR):
* a response block without :status or carrying a request
pseudo-header (8.3.2)
The idle-stream detection uses the existing highest_started_stream_id
Sink hook so streams evicted after full close are still read as closed,
not idle.
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChangesThe HTTP/2 engine now enforces additional RFC 9113 rules for idle streams, SETTINGS, flow-control overflow, HPACK updates, pseudo-headers, and response HTTP/2 protocol validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:16 AM PT - Jul 29th, 2026
❌ @robobun, your commit 5027c7e has 1 failures in 🧪 To try this PR locally: bunx bun-pr 36343That installs a local version of the PR into your bun-36343 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the duplicate-PR report: this PR supersedes both #32987 (idle-stream subset) and #33004 (response pseudo-header subset). Both are a month old with merge conflicts against current On comment-cop: the flagged lines are two-line RFC-section citations in the same style this file already uses throughout (see the surrounding |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/runtime/api/bun/h2/connection.rs`:
- Around line 1112-1116: Update the :status parsing logic in the HTTP/2 header
parsing flow near is_response and informational so a 100 informational response
with self.header_end_stream set is marked malformed and rejected instead of
completing the stream. Add the corresponding conformance test covering
END_STREAM on an informational response.
In `@test/js/node/http2/h2-conformance.test.ts`:
- Around line 1063-1077: Update the close handler in the request setup around
reqClosed so streamRstCode is assigned whenever req!.rstCode is present,
including the legitimate value 0, rather than using a truthiness check. Preserve
the existing assignment and close-resolution behavior.
🪄 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: 8cc360cc-3227-4720-852b-413ebf233d14
📒 Files selected for processing (3)
src/runtime/api/bun/h2/connection.rssrc/runtime/api/bun/h2/wire.rstest/js/node/http2/h2-conformance.test.ts
Gate on rstCode presence (not truthiness) in the conformance probe.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… already-sent bytes The idle-stream test compared against the mixed-parity high-water mark so an idle even id numerically below a higher odd id slipped through. is_idle_stream_id now compares against the parity-matching mark: the engine-tracked last_peer_stream_id for peer-parity ids and a new Sink::highest_local_stream_id() for own-parity ids. The WINDOW_UPDATE shim that inserts a locally-opened stream into the engine map seeded its send window at the initial size without the bytes the legacy encoder has already written (still parked in pending_stream_send_consumed). track_local_stream now subtracts those via Sink::take_pending_stream_send_consumed so the overflow check sees the same window the peer does; the DATA and HEADERS shims use the same helper. Also: surface connection-level WINDOW_UPDATE overflow as NGHTTP2_ERR_FLOW_CONTROL for node-message parity with the per-stream path; drop the duplicate settingsPayload helper in the test.
|
Builds #85164 (3723d8a) and #85240 (5027c7e): the new conformance tests pass on every lane. Remaining reds on both runs are unrelated flakes (proxy-stress-protocol, spawn-streaming-stdout, setInterval, fastutf8stream-reopen, svelte, napi, jsonc, etc.) plus the pre-existing All review threads addressed; ready for review. |
… shim handle_rst_stream still used a parity-unaware high-water-mark check and a hand-rolled local-stream shim, so RST_STREAM on an idle even stream below a higher odd id was tolerated. It now uses is_idle_stream_id and track_local_stream like DATA/HEADERS/WINDOW_UPDATE. Also: key the request-side wrong_direction test on is_request instead of is_server so a PUSH_PROMISE block carrying :status is malformed too.
|
The diff is ready; the internal gate is bouncing on build infra rather than the change itself. Both gate ASAN builds (with and without the fix) fail at Local proof with a clean tree:
Buildkite #85164 (at 3723d8a) ran all 14 then-new conformance tests green on every lane. |
There was a problem hiding this comment.
No new issues found after 5027c7e — every prior finding (parity-aware idle test, RST_STREAM routing, connection-level FLOW_CONTROL lib_error, dead highest_started_stream_id, PUSH_PROMISE :status, duplicate settingsPayload) has been addressed and all threads are resolved. Deferring to a human because this is ~150 lines of new inbound-frame validation in the h2 protocol state machine with a deliberate behavioral escalation (per-stream §6.9.1 overflow: stream error → connection error) and a new send-window seeding shim; worth a maintainer's eyes on the state-machine interactions.
What was reviewed:
is_idle_stream_idparity logic vs nghttp2'ssession_detect_idle_stream; all five §5.1 call sites (both DATA paths, HEADERS, WINDOW_UPDATE, RST_STREAM) now route through it andtrack_local_stream.take_pending_stream_send_consumedvs the pre-existingrewrite_readdrain at h2_frame_parser.rs:5549 — bothretain-remove matched entries, so no double-consume.handle_rst_streamrewrite preserves the evicted-after-close tolerance (None => {}) and still only callson_stream_resetfor tracked streams.- Response pseudo-header enforcement (
is_responsegating, missing:status, 1xx+END_STREAM) and the HPACK §4.2 mid-block size-update guard.
Extended reasoning...
Overview
This PR hardens node:http2 client-side inbound frame validation to reject nine RFC 9113/7541 violations that node (via nghttp2) rejects but Bun previously accepted. It touches four files: h2/connection.rs (the protocol state machine — new is_idle_stream_id, track_local_stream, last_peer_stream_id; rewired DATA/HEADERS/WINDOW_UPDATE/RST_STREAM idle checks; response pseudo-header validation; HPACK §4.2 mid-block size-update guard; §6.9.1 overflow escalation), h2/wire.rs (adds lib_error::FLOW_CONTROL), h2_frame_parser.rs (new last_local_stream_id cell, highest_local_stream_id/take_pending_stream_send_consumed Sink impls, drops the dead highest_started_stream_id impl), and 230 lines of new conformance tests.
Security risks
The change is security-positive: it closes cases where a hostile server could deliver malformed response header blocks (missing :status, request pseudo-headers in a response) directly to application 'response' listeners, and where idle-stream frames could open phantom streams. The risk to review is the opposite direction — over-strict validation breaking legitimate servers. The positive-control tests (well-formed response still delivered; WINDOW_UPDATE replenishing already-sent bytes to the 2^31-1 cap accepted) and the 307-pass node-http2.test.js / 254-pass test-http2-* parallel suite give reasonable confidence, but the per-stream §6.9.1 overflow escalation from RST_STREAM to GOAWAY is a deliberate strictness increase for node parity that changes observable behavior.
Level of scrutiny
High. This is production protocol-state-machine code on a hot path (http2.connect), with new state (last_peer_stream_id, last_local_stream_id), new Sink trait methods, and a shim (track_local_stream) that inserts engine stream entries mid-receive with a seeded send window. The interactions between the two pending_stream_send_consumed consumers (the pre-existing rewrite_read drain and the new take_pending_stream_send_consumed) are subtle enough that a maintainer familiar with the legacy-encoder transition should confirm the invariant.
Other factors
The PR has been through five rounds of my own review; every finding was addressed (parity-aware idle test rewritten in 3723d8a, RST_STREAM rewired and PUSH_PROMISE :status fixed in 532dcc8, dead trait method removed in 5027c7e). The current bug-hunt pass found nothing. Test coverage is thorough: 15 new wire-level conformance cases including positive controls and the parity edge case (idle even stream 2 with two open odd requests). CI at #85164 was green on the conformance suite across all lanes. All comment-cop and CodeRabbit threads are resolved. Given the scope and the state-machine subtlety, a human sign-off is still appropriate.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-29, 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. |
2 participants
Problem
http2.connect()accepts nine server-side protocol violations that RFC 9113 / RFC 7541 require an endpoint to treat as errors and that node (via nghttp2) rejects. Every case is deterministic on bun 1.4.0 and errors in node v26.3.0::status'response':method/:path'response'The last three surface attacker-controlled header shapes to application
'response'listeners.Standalone repro (raw hostile h2c server vs
http2.connect)Cause
Inbound frame validation in
src/runtime/api/bun/h2/connection.rswas thorough for a server receiving requests but largely unchecked for a client receiving responses:validate_unitrejects ENABLE_PUSH > 1 but not ENABLE_PUSH = 1 from a server (the §8.4 role rule).handle_window_update's §6.9.1 overflow check only ran for streams in the engine map; a client's own requests are opened by the legacy outbound encoder and were never inserted there by a WINDOW_UPDATE.finish_header_blockenforced the §8.3.1 request pseudo-header rules but had no §8.3.2 response counterpart;wrong_directionwas server-only.lshpack_dec_decodeaccepts one at the start of every call), and the decode loop calls it once per field.Fix
All in
connection.rs:is_idle_stream_id()centralises the §5.1 test (not in either map, above bothlast_stream_idmarks). DATA, HEADERS (client), WINDOW_UPDATE use it to GOAWAY(PROTOCOL_ERROR).finish_header_blockcomputesis_response; request pseudo-headers in a response block arewrong_direction, and a response block missing:statusismalformed. Both take the existing malformed path (RST(PROTOCOL_ERROR), noon_headers_complete).Supersedes #32987, which covered the idle-stream subset of this and has drifted against main.
Verification
bun bd test test/js/node/http2/h2-conformance.test.ts: 69 pass (58 existing + 11 new). The 10 new violation tests fail underUSE_SYSTEM_BUN=1; the well-formed-response control test passes under both.bun bd test test/js/node/http2/node-http2.test.js: 307 pass / 6 skip / 0 fail.test/js/node/test/parallel/test-http2-*.jspass.[review] gate passed · iteration 2 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file