[robobun]

Problem

• A hostile HTTP/2 origin can pin a fetch() forever: it answers with HEADERS without END_HEADERS, then streams zero-length CONTINUATION frames. The promise never settles and the client burns CPU parsing 9-byte frame headers (CVE-2024-28182 class; the node:http2 side is node:http2: cap CONTINUATION frames per header block (CVE-2024-28182) #36410).
• src/http/h2_client/dispatch.rs, FT_CONTINUATION arm: the only bound on a header block is LOCAL_MAX_HEADER_LIST_SIZE on accumulated bytes. An empty payload never advances that, so nothing ends the sequence.

Fix

• Add continuation_count: u8 to ClientSession, zeroed at the top of the FT_HEADERS arm and incremented in FT_CONTINUATION; past LOCAL_MAX_CONTINUATIONS (8) the connection fails with HTTP2EnhanceYourCalm, which error_code_for already maps to ENHANCE_YOUR_CALM on the GOAWAY.
• The value and behaviour match nghttp2 (NGHTTP2_DEFAULT_MAX_CONTINUATIONS) and node 22: 8 frames accepted, the 9th rejected even when it carries END_HEADERS.
• One reset at the top of FT_HEADERS is sufficient because dispatch_frame already rejects any non-CONTINUATION frame while a block is open, so every HEADERS that reaches the arm starts a new block; this covers the tracked-stream and orphan (already-RST'd stream) branches alike.
• 8 x 16 KiB (default MAX_FRAME_SIZE) is 128 KiB of compressed headers, below the existing 256 KiB byte cap, so the pre-existing CONTINUATION flood is bounded test now sees HTTP2EnhanceYourCalm; its assertion is updated.
• Verified in test/js/web/fetch/fetch-http2-adversarial.test.ts (20/20 with the fix; 3 fail on the released binary):
  • zero-length CONTINUATION flood is rejected: the repro, times out without the fix.
  • header block split across the full CONTINUATION budget is accepted: 8 frames deliver a 200.
  • 9th CONTINUATION is rejected even if it ends the block: pins the cap at exactly 8 and the comparison at >.
  • CONTINUATION budget resets for each header block on a stream: a 103 block and the final 200 block on one stream each use all 8 frames; fails with the reset removed (checked by removing it and rebuilding).
• fetch-http2-client.test.ts still 62/62.

Background

• An HTTP/2 header block starts in a HEADERS frame and, when END_HEADERS is not set, continues in CONTINUATION frames on the same stream; until one arrives with END_HEADERS, no other frame is allowed on the connection (RFC 9113 4.3, 6.10). A response may carry several blocks on one stream: 1xx informational blocks, the final response, and trailers.
• HPACK state is connection-wide, so the fetch client also assembles and decodes blocks for streams it has already reset (the "orphan" path in dispatch.rs) to keep the decoder in sync; the cap has to apply there too.
• ENHANCE_YOUR_CALM is the HTTP/2 error code for "peer is generating excessive load"; HTTP2EnhanceYourCalm is the fetch() error the client already uses for its other flood limits.

---

no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch-http2-adversarial.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d669ea9c-d961-42b4-8f52-63b29731bf9d

📥 Commits

Reviewing files that changed from the base of the PR and between 74c2457 and d6923c8.

📒 Files selected for processing (4)

• src/http/H2Client.rs
• src/http/h2_client/ClientSession.rs
• src/http/h2_client/dispatch.rs
• test/js/web/fetch/fetch-http2-adversarial.test.ts

---

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

[robobun]

Status: Ready to merge. Rebased onto main; all review feedback addressed (field is pub(crate), reset hoisted to the top of FT_HEADERS, two boundary tests added); @alii has signed off on d6923c8. All review threads resolved.

Verified with:

USE_SYSTEM_BUN=1 bun test test/js/web/fetch/fetch-http2-adversarial.test.ts -t CONTINUATION   # 3 fail (flood times out, 9th-frame and byte-flood see the wrong error)
bun bd test test/js/web/fetch/fetch-http2-adversarial.test.ts                                 # 20/20 pass
bun bd test test/js/web/fetch/fetch-http2-client.test.ts                                      # 62/62 pass

The reset test was additionally checked by removing the reset, rebuilding, and confirming it fails.

CI (build 93752 at d6923c8): 179 of 181 jobs passed; fetch-http2-adversarial.test.ts passed on every lane that ran. The build is red only because the two darwin 26 aarch64 - test-bun jobs were canceled by the runner (no test output), and the four flaky entries (child_process_ipc_handle, test-quic-session-initial-rtt, sourcetextmodule-leak, dlopen-duplicate-load) all passed alone or on retry. None of them touch src/http/.

[claude]

LGTM — the earlier nit (tighten toContain → toBe) is addressed in 579d0a5, and the comment-cop trims landed in e5f448b.

What was reviewed:

• Counter reset at both expecting_continuation arm sites (tracked + orphan HEADERS branches), so trailers and RST'd-stream CONTINUATION sequences each start from zero.
• Count check runs before the stream-map lookup, so the orphan-block path is bounded too.
• Boundary test confirms exactly 8 CONTINUATION frames are accepted (> 8 check, not >= 8); flood test confirms the 9th fails with HTTP2EnhanceYourCalm; error_code_for already maps that to ENHANCE_YOUR_CALM for the GOAWAY.

Extended reasoning...

Overview

Adds a per-header-block CONTINUATION frame counter to the fetch() HTTP/2 client (src/http/h2_client/) to close CVE-2024-28182: a server sending unbounded zero-length CONTINUATION frames could spin the parser indefinitely without ever tripping the byte-size cap. The change is a new u32 constant (LOCAL_MAX_CONTINUATIONS = 8, matching nghttp2), a new continuation_count field on ClientSession, two reset sites in the FT_HEADERS arm of dispatch_frame, and one increment+threshold check at the top of the FT_CONTINUATION arm. Two new tests plus one tightened assertion in fetch-http2-adversarial.test.ts.

Security risks

This is a DoS-hardening fix that only adds a failure path — it cannot relax any existing validation. The constant matches nghttp2's upstream CVE fix, so 8 frames × 16 KiB default MAX_FRAME_SIZE = 128 KiB compressed headers is a well-trodden bound. No auth, crypto, or permission surface is touched.

Level of scrutiny

Protocol-parser code warrants careful review, but the change is mechanically minimal (~10 lines of runtime code) and strictly additive. I traced:

• Both expecting_continuation arm sites reset the counter (dispatch.rs:340, :377), so a second HEADERS on the same stream (trailers) and the orphan-HEADERS branch both start clean.
• The increment+check at dispatch.rs:385-389 runs before the streams.get() lookup, so a CONTINUATION flood on an already-RST'd stream (orphan path) is bounded identically.
• remove_stream mid-CONTINUATION preserves expecting_continuation and moves the fragment to orphan_header_block; the counter carries over correctly since it's per-block, not per-stream.
• fatal_error = HTTP2EnhanceYourCalm flows through on_data → fail_all → GOAWAY(ENHANCE_YOUR_CALM) via the existing error_code_for mapping.

Other factors

• My prior review nit (weakened toContain assertion) was fixed in 579d0a5; the assertion is now toBe("HTTP2EnhanceYourCalm") with a corrected comment.
• comment-cop flagged verbose doc comments; trimmed in e5f448b, and the remaining two-line doc comment on the constant is consistent with neighbours.
• Tests cover the attack (10k empty frames → reject), the exact boundary (8 frames → accept), and the pre-existing large-payload flood now hitting the frame-count cap deterministically. Author verified fail-before with USE_SYSTEM_BUN=1 and pass-after with bun bd; full adversarial + client suites pass locally.
• All review threads are resolved; the one CI failure noted is an unrelated Worker exception-scope assertion.

[Jarred-Sumner]

this should be a u8 and not u32 since the number is small

[robobun]

Done in 7bc1dab: continuation_count and LOCAL_MAX_CONTINUATIONS are both u8 now. 17/17 adversarial tests pass.

[claude]

LGTM — the earlier nit (tighten toContain → toBe) and comment-cop feedback are addressed; the u8 narrowing in 7bc1dab is safe.

What was reviewed:

• continuation_count += 1 cannot overflow: fatal_error is set at 9 and parse_frames breaks on it, so the count never advances past 9.
• Counter reset covers every arm-site of expecting_continuation (tracked-stream and orphan HEADERS branches); remove_stream's mid-CONTINUATION orphan hand-off correctly does not reset it — same header block, cap still applies.
• Boundary test exercises exactly 8 frames; flood test hangs on main per the fail-before evidence.

Extended reasoning...

Overview

Adds a per-header-block CONTINUATION-frame counter to the fetch() HTTP/2 client (src/http/h2_client/) to close CVE-2024-28182: a server sending unbounded zero-length CONTINUATION frames after a HEADERS without END_HEADERS could spin the client's frame parser indefinitely, since the existing LOCAL_MAX_HEADER_LIST_SIZE byte cap never advances on empty payloads. Runtime change is ~16 lines: a u8 constant (LOCAL_MAX_CONTINUATIONS = 8, matching nghttp2's fix), a u8 field on ClientSession, two reset points where expecting_continuation is armed, and an increment+check in the FT_CONTINUATION arm. Test file gains a repro (10 000 empty frames → HTTP2EnhanceYourCalm) and a boundary test (exactly 8 frames → 200 OK), and the pre-existing byte-flood test's expected error code is updated to the now-deterministic HTTP2EnhanceYourCalm.

Security risks

This is DoS hardening — it tightens an inbound resource limit and fails closed via fatal_error → fail_all → GOAWAY(ENHANCE_YOUR_CALM). No auth, crypto, or trust-boundary logic is touched. The over-rejection risk (a legitimate server fragmenting response headers into >8 CONTINUATION frames) is mitigated by matching nghttp2's constant — any such server is already broken against the majority of deployed HTTP/2 clients — and by the explicit boundary test at exactly 8.

Level of scrutiny

Moderate. HTTP/2 frame dispatch is a hot path and protocol-correctness matters, but the change is a mechanical counter with a well-known reference implementation (nghttp2's CVE-2024-28182 fix). I traced the counter's lifecycle: reset at both expecting_continuation = stream_id sites in FT_HEADERS (tracked and orphan), incremented once per FT_CONTINUATION, and the > 8 check runs before the byte-size check so the 9th frame always trips it. The u8 type cannot overflow because parse_frames breaks on fatal_error.is_some() immediately after dispatch_frame returns, capping the count at 9. The remove_stream → orphan hand-off (stream RST'd mid-CONTINUATION) intentionally preserves the running count, which is correct — it's the same header-block sequence.

Other factors

All prior review threads are resolved: my earlier toContain → toBe nit was applied in 579d0a5, comment-cop's doc-comment length flags were trimmed in e5f448b, and the field was narrowed to u8 in 7bc1dab. The PR carries fail-before/pass-after evidence for both debug+ASAN and release, and the author reports the touched test file green on every CI lane across two builds. No CODEOWNERS covers src/http/h2_client/. The change follows the sibling node:http2 fix in #36410 for the same CVE.

[claude]

No issues found — the fix is small and looks correct, but since this is a CVE mitigation in the HTTP/2 frame parser I'd like a human to give it a final glance.

What was reviewed:

• continuation_count reset covers both FT_HEADERS branches (tracked stream + orphan block) where expecting_continuation is armed; remove_stream mid-CONTINUATION correctly does not reset (same header block continues as orphan).
• u8 overflow ruled out: parse_frames breaks on fatal_error after dispatch_frame returns, so the counter stops at 9.
• Boundary test (8 frames accepted) matches the > LOCAL_MAX_CONTINUATIONS check; prior nit on the toContain assertion was addressed in 579d0a5.

Extended reasoning...

Overview

Adds a per-header-block CONTINUATION frame count cap to the fetch() HTTP/2 client (src/http/h2_client/) to close CVE-2024-28182: a new LOCAL_MAX_CONTINUATIONS: u8 = 8 constant, a continuation_count: u8 field on ClientSession, two reset sites in the FT_HEADERS arm of dispatch_frame, and one increment+check in the FT_CONTINUATION arm. Two new adversarial tests (zero-length flood + exact-boundary acceptance) plus one existing test's assertion updated to the now-deterministic error code. ~12 lines of runtime code.

Security risks

This is the security-sensitive part. The change is pure hardening — it adds a fail-closed bound where none existed for zero-length payloads. Over-rejection risk: 8 × 16 KiB default MAX_FRAME_SIZE = 128 KiB of compressed headers, matching nghttp2's NGHTTP2_DEFAULT_MAX_CONTINUATIONS from the same CVE fix, and the boundary test guards against off-by-one. The counter cannot overflow: parse_frames checks session.fatal_error after every dispatch_frame and breaks the loop, so the u8 never advances past 9. can_pool() already requires expecting_continuation == 0, so a stale count on a pooled session is irrelevant (next FT_HEADERS resets it anyway).

Level of scrutiny

High — HTTP/2 wire protocol parsing on untrusted server input, and a named CVE. The diff itself is mechanically simple and the mechanism is well-understood (mirrors nghttp2), but a human should sign off on any change to the frame dispatch loop.

Other factors

All prior review threads are resolved: my earlier toContain→toBe nit was fixed in 579d0a5, comment-cop trimming in e5f448b, and the u8 narrowing in 7bc1dab. Fail-before/pass-after verified (USE_SYSTEM_BUN=1 times out on the flood test); CI green on the touched test file across debug/ASAN/release lanes per the author's status comment. The reset placement was checked against the third path that touches expecting_continuation (ClientSession::remove_stream) — that one is a mid-block stream detach where the orphan continues the same header block, so not resetting there is correct.

[alii]

Needs a rebase and two more tests before it lands. The cap itself matches nghttp2 and node (8 accepted, 9 rejected even when the 9th carries END_HEADERS, checked against node 22).

• ClientSession.rs conflicts with main since #36184; make the new field pub(crate) when you rebase.
• Nothing rejects a 9th CONTINUATION, so the tests only pin the cap to somewhere in 8..16.
• Both continuation_count resets are unobserved by any test; a response with two split header blocks pins them.
  Same two test gaps apply to #36410 since they land together.

[robobun]

^{Updated 9:28 PM PT - Aug 12th, 2026}

❌ @robobun, your commit d6923c8 has some failures in Build #93752 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 36411

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

bun-36411 --bun

[robobun]

Rebased and addressed all three points (d6923c8): field is pub(crate), the reset moved to the top of FT_HEADERS, and both tests are in (9th frame with END_HEADERS rejected; 103 + 200 blocks each using the full budget, confirmed to fail with the reset removed). PR body updated to describe the current state.

[claude]

LGTM — small, well-bounded DoS hardening with the human review feedback fully addressed.

What was reviewed:

• Counter reset at the top of FT_HEADERS covers both the tracked-stream and orphan branches; the increment+check in FT_CONTINUATION runs before either branch so both are capped.
• u8 overflow ruled out: the check fires at 9 and returns, so the counter never climbs further.
• Boundary tests pin the cap at exactly 8 in both directions (1c accepts 8, 1d rejects 9); test 1e verifies the per-block reset; the updated assertion in test 1 is deterministic for that payload shape.
• Confirmed #37637's HeaderBlockInFlight refactor is on the node:http2 side (src/runtime/api/bun/h2/), not the fetch client, so the flat continuation_count field alongside expecting_continuation is consistent here.

Extended reasoning...

Overview

Adds a per-header-block CONTINUATION frame cap to the fetch() HTTP/2 client (CVE-2024-28182 mitigation). Runtime change is 3 lines of logic in dispatch.rs (reset in FT_HEADERS, increment+check in FT_CONTINUATION), one u8 field on ClientSession, and one u8 constant in H2Client.rs. Four new adversarial tests plus one updated assertion.

Security risks

This is defensive hardening that closes a DoS vector (a hostile origin streaming endless zero-length CONTINUATION frames). It fails closed — excess frames trigger HTTP2EnhanceYourCalm and the connection is torn down via the existing fatal_error → fail_all → GOAWAY path. No new parsing of untrusted data, no new allocation, no user-facing API change. The cap value (8) and behaviour (reject the 9th even with END_HEADERS) match nghttp2 and Node 22.

Level of scrutiny

Moderate: touches the HTTP/2 frame dispatch hot path, but the change is a monotonic counter with a hard bound and no interaction with HPACK decoding, flow control, or stream lifecycle beyond setting fatal_error. Behaviour on legitimate traffic is unchanged (8 × 16 KiB = 128 KiB of compressed headers is still permitted, below the existing 256 KiB byte cap). The reset placement is provably sufficient because dispatch_frame already rejects any non-CONTINUATION frame while expecting_continuation != 0, so every HEADERS that reaches the arm starts a fresh block.

Other factors

• alii reviewed on 2026-08-12 with three substantive requests (rebase + pub(crate); add a 9th-frame rejection test; hoist the reset and add a two-block test). All three were implemented exactly as requested and every thread is resolved.
• My earlier nit (toContain → toBe) was addressed.
• Author verified: USE_SYSTEM_BUN=1 fails 3 tests, debug build passes 20/20, fetch-http2-client.test.ts still 62/62, and the reset test was confirmed to fail with the reset removed.
• Checked that #37637's Option<HeaderBlockInFlight> refactor lives in src/runtime/api/bun/h2/connection.rs (the node:http2 engine), not this fetch client, so the flat-field approach here remains consistent with its neighbours.

[alii]

Looks ready to merge from this side. All three points are addressed at d6923c8: the field is pub(crate) and the branch merges clean against main, the single reset at the top of the FT_HEADERS arm is correct given the expecting_continuation guard at the top of dispatch_frame, and the two new tests pin what was missing (1d fails with a 200 on a build without the fix, 1e passes there and only the reset keeps it green with the fix; the with-fix run is CI's, not verified locally).