Skip to content

node:http2: cap CONTINUATION frames per header block (CVE-2024-28182) - #36410

Open
robobun wants to merge 1 commit into
mainfrom
claude/farm-8692caa2-h2-continuation-count-guard
Open

robobun wants to merge 1 commit into
mainfrom
claude/farm-8692caa2-h2-continuation-count-guard

Conversation

@robobun

@robobun robobun commented Jul 29, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • node:http2 clients and servers accept any number of CONTINUATION frames for one header block. A peer that sends HEADERS without END_HEADERS, then empty CONTINUATION frames, holds the session with no error (CVE-2024-28182).
  • Cause: handle_continuation in src/runtime/api/bun/h2/connection.rs limits a block by its bytes only.

Fix

  • HeaderBlockInFlight counts the CONTINUATION frames of its block. Above MAX_CONTINUATIONS (8, the limit node uses) the connection ends.
  • The error is the one node gives: GOAWAY(INTERNAL_ERROR), ERR_HTTP2_ERROR, errno -905, Too many CONTINUATION frames following a HEADER frame.
  • Verified: test/js/node/http2/node-http2.test.js and test/js/node/http2/h2-conformance.test.ts (8 frames accepted, the 9th refused). Also node-http2-continuation.test.ts and h2-push-refusal-staged.test.ts.

Background

  • A header block larger than one HEADERS or PUSH_PROMISE frame continues in CONTINUATION frames until one carries END_HEADERS. No other frame is allowed in between.
  • HeaderBlockInFlight is the engine's record of an open block. HEADERS and PUSH_PROMISE build it at one site, so each block starts at zero.
  • Considered a limit that grows with maxHeaderListSize. It keeps large blocks, but it differs from node and is weaker where the size was raised.

Downsides

  • A block of more than 8 CONTINUATION frames (over 147,456 bytes at the default frame size) now ends the session. Main delivers it if the receiver raised maxHeaderListSize. Node has the same limit.
  • A peer can still send HEADERS without END_HEADERS and then nothing. The request then stays pending, as in node.
  • Cost: +8 bytes for each session (Connection) and +67 bytes of text in the release binary. Each CONTINUATION frame costs one add and one compare.
Notes

Measurements. Release builds of the merge base (017b243) and of this PR (66f2145), and node v26.3.0. A raw TCP peer sends the frames.

input from the peer merge base this PR node v26.3.0
HEADERS + 8 empty CONTINUATION, END_HEADERS on the 8th response delivered response delivered response delivered
HEADERS + 9 empty CONTINUATION, END_HEADERS on the 9th response delivered errno -905 errno -905
HEADERS + 1000 empty CONTINUATION, no END_HEADERS request pending errno -905 errno -905
HEADERS + 8 CONTINUATION, 300 ms apart, END_HEADERS on a 9th response delivered errno -905 at the 9th frame errno -905 at the 9th frame
HEADERS without END_HEADERS, then nothing for 3 s request pending request pending request pending
HEADERS + 8 CONTINUATION, then a PING errno -505 errno -505 errno -905
HEADERS + 7 CONTINUATION, then a PING errno -505 errno -505 errno -505

The row with 8 CONTINUATION and a PING is a known difference. RFC 9113 (6.2, 6.10) requires PROTOCOL_ERROR for a frame of another type while a block is open, which is what Bun sends. nghttp2 counts the frame before it reads the frame type.

Large header blocks (the first Downsides bullet). A server answers with N header fields of 5,000 bytes each. The client sets maxHeaderListSize: 1048576. A second stream is open on the same session.

response header block merge base this PR node v26.3.0 (1)
35 fields, about 142,000 bytes, HEADERS + 8 CONTINUATION delivered delivered delivered
40 fields, about 162,000 bytes, HEADERS + 9 CONTINUATION delivered, the second stream stays open errno -905 on the request, on the second stream and on the session errno -905 on both streams
the same 40 fields, the client also sets maxFrameSize: 1048576 delivered delivered errno -905 on both streams

(1) With maxSendHeaderBlockLength: 1048576 on the node server. With the default (65,536) the node server refuses both blocks before it sends a frame: frameError, NGHTTP2_FRAME_SIZE_ERROR, and only that stream fails. Bun has no default send limit (#43474 proposes one), so a Bun sender emits these blocks with no option set. The Bun sender fills each frame to the frame size of the peer, which is why a larger maxFrameSize on a Bun receiver keeps large blocks between two Bun peers. Node splits a block at 16,384 bytes in all cases.

Cost numbers.

  • size on the release binaries: text 106,738,870 (merge base) and 106,738,937 (this PR). Data (116,624) and bss (1,823,264) do not change. The stripped file is 106,911,456 bytes in both.
  • size_of::<Connection>() is 448 on the merge base and 456 on this PR (x86_64 Linux). HeaderBlockInFlight goes from 16 to 20 bytes. One Connection exists for each HTTP/2 session.
  • A HEADERS or PUSH_PROMISE frame with END_HEADERS (the usual case) runs no new code. A frame without it stores one more u32.

Tests.

  • test/js/node/http2/node-http2.test.js (client): a response and its trailers on one stream each use exactly 8 CONTINUATION frames and are delivered. A 9th frame on a response block gives errno -905. A PUSH_PROMISE block with 8 frames is delivered, with 9 it gives errno -905. The two 9th-frame tests fail on main, which delivers the 200 and the push.
  • test/js/node/http2/h2-conformance.test.ts (server, raw client against createServer): 8 CONTINUATION frames dispatch the request with no GOAWAY. The 9th gives GOAWAY(INTERNAL_ERROR), sessionError with errno -905, and no stream event. The 9th-frame test fails on main.
  • Unit tests in the connection.rs test module cover the count for each block (HEADERS) and the PUSH_PROMISE opener. They compile (cargo clippy --all-targets), but they were not executed: cargo test -p bun_runtime does not link in this tree. The JS tests drive the same engine paths.
  • Debug build of this commit: node-http2.test.js 429 pass and 6 skip, h2-conformance.test.ts 86 pass, node-http2-continuation.test.ts 9 pass, h2-push-refusal-staged.test.ts 1 pass. In one run of three files together, two stream-reset flood tests of h2-conformance.test.ts passed the 5 s limit of the debug build. This PR does not touch them (test: make the h2 stream-reset flood tests pass on debug builds #44256 is about that).
  • One review note is open for the next push: the server test for 8 frames waits on once(srv, "stream"), so a regression there ends on the test timeout and not on an assertion.

Related.

Repro. Node prints the -905 error at once. Bun on main prints STILL PENDING.

import net from "node:net";
import http2 from "node:http2";
const fr = (t, fl, sid, p = Buffer.alloc(0)) => {
  const h = Buffer.alloc(9);
  h.writeUIntBE(p.length, 0, 3); h[3] = t; h[4] = fl; h.writeUInt32BE(sid, 5);
  return Buffer.concat([h, p]);
};
const srv = net.createServer(sock => {
  let buf = Buffer.alloc(0), pre = false, started = false;
  sock.on("error", () => {});
  sock.on("data", d => {
    buf = Buffer.concat([buf, d]);
    if (!pre && buf.length >= 24) { pre = true; buf = buf.subarray(24); sock.write(fr(4,0,0)); sock.write(fr(4,1,0)); }
    while (pre && buf.length >= 9) {
      const len = buf.readUIntBE(0, 3); if (buf.length < 9 + len) break;
      const type = buf[3], sid = buf.readUInt32BE(5) & 0x7fffffff; buf = buf.subarray(9 + len);
      if (type === 1 && !started) {
        started = true;
        sock.write(fr(1, 0, sid, Buffer.from([0x88])));            // HEADERS :status 200, no END_HEADERS
        sock.write(Buffer.concat(Array(1000).fill(fr(9, 0, sid)))); // empty CONTINUATION flood
      }
    }
  });
});
await new Promise(r => srv.listen(0, "127.0.0.1", r));
const s = http2.connect(`http://127.0.0.1:${srv.address().port}`);
s.on("error", e => { console.log("guarded:", e.code, e.errno, e.message); process.exit(0); });
s.request({ ":path": "/" }).on("error", () => {});
setTimeout(() => { console.log("STILL PENDING (no guard)"); process.exit(1); }, 4000);

History. The first revision kept the count as fields on Connection with a public max_continuations setting, and it tested 64 frames and 8 frames on the client only. After review the count moved into HeaderBlockInFlight (from #37637), the setting became a private constant, and tests were added for the 9th frame, for the count of each block, for PUSH_PROMISE and for the server side.


[review] gate passed · iteration 3 · 5 files touched

fails on main (without fix)
ASAN without fix: 13 failed, 6 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/http2/h2-conformance.test.ts" "test/js/node/http2/node-http2.test.js"
bun test v1.4.0 (b40d1e902)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > should be able to send a GET request [956.46ms]
(pass) node none > Client Basics > should be able to send a POST request [620.57ms]
(pass) node none > Client Basics > constants [20.31ms]
(pass) node none > Client Basics > getDefaultSettings [8.11ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [24.82ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [6.59ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [5.43ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [6.15ms]
(pass) node none > Client Basics > should be able to send data using end [658.88ms]
(pass) node none > Client Basics > should be able to mutiplex GET requests [646.20ms]
(pass) node none > Client Basics > http2 s
... (truncated)

release without fix: 24 failed, 6 skipped
bun test v1.4.0-canary.1 (da3851e57)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > constants [0.94ms]
(pass) node none > Client Basics > getDefaultSettings [0.16ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [0.41ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [0.11ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [0.03ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [0.08ms]
(pass) node none > Client Basics > is possible to abort request [2.29ms]
(pass) node none > Client Basics > aborted event should work with abortController [0.89ms]
(pass) node none > Client Basics > aborted event should work with aborted signal [0.82ms]
(pass) node none > Client Basics > signal validation matches node: non-signal objects throw, duck-typed { aborted } is accepted [1.37ms]
(pass) node none > Client Basics > headers cannot be bigger than 65536 bytes [46.70ms]
(skip) node none > Client Basics > should not leak memory
(pass) node none > Client Basics > should fail to con
... (truncated)
passes on PR (with fix)
ASAN with fix: 6 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/http2/h2-conformance.test.ts" "test/js/node/http2/node-http2.test.js"
bun test v1.4.0 (b40d1e902)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > should be able to send a GET request [1082.42ms]
(pass) node none > Client Basics > should be able to send a POST request [732.11ms]
(pass) node none > Client Basics > constants [19.99ms]
(pass) node none > Client Basics > getDefaultSettings [8.35ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [25.44ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [6.07ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [3.61ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [5.58ms]
(pass) node none > Client Basics > should be able to send data using end [751.74ms]
(pass) node none > Client Basics > should be able to mutiplex GET requests [730.94ms]
(pass) node none > Client Basics > http2 
... (truncated)

release with fix: 6 skipped
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 1351ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/22] gen generated_host_exports.rs
generated_host_exports.rs: 93 exports (host=3, lazy=10, generic=80, rust=0); 239 extern-C blocks audited
[2/22] gen JS modules (bundle-modules)
Preprocess modules (14006ms)
Bundle modules (573ms)
Postprocesss modules (1879ms)
Bundle Functions (2505ms)
Generate Code (50ms)

[19.06s] Bundled "src/js" for production
  2626 kb
  197 internal modules
  13 native modules
  91 internal functions across 17 files
[2/6] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

^[[1m^[[92m   Compiling^[[0m bun_paths v0.0.0 (/workspace/bun/src/paths)
^[[1m^[[92m   Compiling^[[0m bun_sys v0.0.0 (/workspace/bun/src/sys)
^[[1m^[[92m   Compiling^[[0m bun_url v0.0.0 (/workspace/bun/src/url)
^[[1m^[[92m   Compiling^[[0m bun_http_types v0.0.0 (/workspace/bun/src/http_types)
^[[1m^[[92m   Compiling^[[0m bun_threading v0.0.0 (/workspace/bun/src/threading)
^[[1m^[[92m   Compiling^[[0m bun_perf v0
... (truncated)
diff hotspot
src/js/node/http2.ts                      |   1 +
 src/runtime/api/bun/h2/connection.rs      | 120 ++++++++++++++++++++++-
 src/runtime/api/bun/h2/wire.rs            |   2 +
 test/js/node/http2/h2-conformance.test.ts |  60 ++++++++++++
 test/js/node/http2/node-http2.test.js     | 153 ++++++++++++++++++++++++++++++
 5 files changed, 335 insertions(+), 1 deletion(-)

gate history · 3 passed · 0 rejected · iteration 3

evidence per changed file
file                                       reads  edits  tests
src/js/node/http2.ts                           2      2      0
src/runtime/api/bun/h2/connection.rs          14     17      0
src/runtime/api/bun/h2/wire.rs                 3      3      0
test/js/node/http2/h2-conformance.test.ts      4      2      0
test/js/node/http2/node-http2.test.js          5      4      0

@coderabbitai

coderabbitai Bot commented Jul 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 51b57179-280a-4590-8818-0507f20d0a9c
📥 Commits

Reviewing files that changed from the base of the PR and between 017b243 and 66f2145.

📒 Files selected for processing: 5
  • src/js/node/http2.ts
  • src/runtime/api/bun/h2/connection.rs
  • src/runtime/api/bun/h2/wire.rs
  • test/js/node/http2/h2-conformance.test.ts
  • test/js/node/http2/node-http2.test.js

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

Walkthrough

The HTTP/2 connection now allows up to eight CONTINUATION frames per header block. A ninth frame terminates the connection with GOAWAY and an INTERNAL_ERROR. The error is reported as nghttp2 error -905. Tests cover request headers, response headers, trailers, and PUSH_PROMISE.

Changes

HTTP/2 CONTINUATION limit

Layer / File(s) Summary
Track and enforce the frame limit
src/runtime/api/bun/h2/connection.rs, src/runtime/api/bun/h2/wire.rs, src/js/node/http2.ts
The connection counts CONTINUATION frames per header block and terminates the connection after the eighth allowed frame. The new TOO_MANY_CONTINUATIONS code and its JavaScript error message use error code -905.
Verify connection-level handling
src/runtime/api/bun/h2/connection.rs
Runtime tests cover separate header blocks using the full allowance and rejection of a ninth frame for header and PUSH_PROMISE blocks.
Verify HTTP/2 client and server behavior
test/js/node/http2/h2-conformance.test.ts, test/js/node/http2/node-http2.test.js
Tests cover acceptance at eight frames and errors at nine frames for requests, response headers, trailers, and PUSH_PROMISE.

Suggested reviewers: jarred-sumner

Priority: ⬆️ High

Merge Risk: ⚪ Minimal · up to 66f21

No merge-blocking issue is established; the change is ready for normal merge checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely identifies the affected API, the CONTINUATION frame limit, and the security issue addressed.
Description check Passed The description explains the problem, implementation, behavior, limitations, testing, and verification results. It does not use the exact template headings, but it includes the required change summary…
Full details: Description check

Explanation

The description explains the problem, implementation, behavior, limitations, testing, and verification results. It does not use the exact template headings, but it includes the required change summary and verification information.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@robobun

robobun commented Jul 29, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: rebased on main (head 66f2145), no conflicts. CI passed. Ready for review and merge.

CI: build #127492 passed.

How it was reproduced: the raw-socket script in the PR body. Node fails the request at once with ERR_HTTP2_ERROR, errno -905. Bun on main leaves the request pending. The 9th-frame tests of this PR fail on main: main delivers the response or the push, and a server answers the request.

Review of 4f0cadb: three optional notes, all answered on the PR. One is a measured behavior change: a header block that needs more than 8 CONTINUATION frames (over 147,456 bytes at the default frame size) now ends the session, as in node. The PR body has it under Downsides.

Related: #36411 (merged) made the same change for the fetch() HTTP/2 client.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — small, well-scoped port of nghttp2's CVE-2024-28182 guard.

What was reviewed:

  • Counter reset covers both header-block entry points (handle_headers and handle_push_promise); handle_continuation is the only consumer.
  • Error path reuses local_connection_error identically to the existing FLOODED / BAD_CLIENT_MAGIC guards; -905 message matches nghttp2's nghttp2_strerror exactly.
  • Boundary semantics (> max_continuations → 8 accepted, 9th errors) match nghttp2 and are covered by both the flood test and the exact-budget acceptance test.
  • max_continuations is pub but nothing writes it beyond the constructor default — harmless future knob.
Extended reasoning...

Overview

Adds a per-header-block CONTINUATION frame counter to the node:http2 inbound engine (src/runtime/api/bun/h2/connection.rs), capped at nghttp2's NGHTTP2_DEFAULT_MAX_CONTINUATIONS = 8. Exceeding it triggers local_connection_error(InternalError, -905), which sends GOAWAY and surfaces NghttpError with code ERR_HTTP2_ERROR / errno -905 to JS. Supporting changes: the -905 errno constant in wire.rs, its message string in http2.ts, and two tests in node-http2.test.js.

Security risks

This is a DoS-hardening change that fails closed — it only adds a stricter rejection path, so the risk profile is over-rejection of legitimate traffic, not under-protection. The 8-frame × 16 KiB default budget (>128 KiB compressed) sits above the existing byte cap, and the exact-budget acceptance test guards against regressing legitimate large blocks. No auth, crypto, or permission logic is touched.

Level of scrutiny

Medium. The mechanism is a trivial counter + compare, wired through the same local_connection_error machinery already used for FLOODED (-904) and BAD_CLIENT_MAGIC (-903). I verified the counter is reset at every site that sets continuation_stream (HEADERS at ~line 1004 and PUSH_PROMISE at ~line 1714 — the only two), and that finish_header_block clears continuation_stream so the count is per-block. The > 8 comparison gives the 8-accepted/9th-errors boundary the PR verified against node v26.3.0.

Other factors

Tests follow the existing raw-net-server pattern in the same file (http2utils.Frame / HeadersFrame / SettingsFrame), wire error events to resolve, and assert exact code/errno/message. The author ran the full node-http2.test.js (319 pass), node-http2-continuation.test.ts, and h2-conformance.test.ts suites. The parallel gap in fetch()'s h2 client is explicitly called out as tracked separately, which is reasonable given the different error plumbing.

Comment thread src/runtime/api/bun/h2/connection.rs Outdated
Comment thread src/runtime/api/bun/h2/connection.rs Outdated
Comment thread src/runtime/api/bun/h2/connection.rs Outdated
Comment thread src/runtime/api/bun/h2/wire.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No bugs found. The change is small and follows nghttp2's CVE-2024-28182 fix exactly (counter + local_connection_error with INTERNAL_ERROR/-905, reset at both HEADERS and PUSH_PROMISE block starts), but it's a security-relevant guard in the HTTP/2 inbound frame parser, so worth a maintainer glance — particularly the decision to leave fetch()'s h2 client (src/http/h2_client/dispatch.rs) for a follow-up.

What was reviewed:

  • Counter reset covers both header-block entry points (handle_headers and handle_push_promise); finish_header_block clears continuation_stream so the next block always re-arms via one of those paths.
  • Error path reuses local_connection_error identically to the existing FLOODED/BAD_CLIENT_MAGIC/STREAM_CLOSED sites; -905 wired into kNghttp2ErrorMessages matches nghttp2's string.
  • Boundary test confirms 8 CONTINUATIONs are accepted and the 9th trips, matching node; node-http2-continuation.test.ts large-header cases still pass per the evidence.
Extended reasoning...

Overview

Adds a per-header-block CONTINUATION frame counter to the node:http2 inbound engine (src/runtime/api/bun/h2/connection.rs): two new u32 fields on Connection, reset to 0 when a HEADERS or PUSH_PROMISE opens a header block without END_HEADERS, incremented in handle_continuation, and compared against max_continuations (default 8 = nghttp2's NGHTTP2_DEFAULT_MAX_CONTINUATIONS). Overflow calls local_connection_error(InternalError, -905, ...). wire.rs gains the TOO_MANY_CONTINUATIONS lib-error constant and the default cap; http2.ts maps -905 to the nghttp2 message. Two tests exercise the flood-rejection and the exact 8-frame boundary.

Security risks

This is the security fix — CVE-2024-28182 (CONTINUATION flood). The guard fails closed (connection torn down via GOAWAY, session marked terminated so further input is ignored). I checked that the counter is reset on every path that sets continuation_stream (both handle_headers and handle_push_promise), so a legitimate second header block on the same connection starts from 0. The > max_continuations comparison after a pre-increment gives exactly 8 accepted frames, matching nghttp2/node. No new attack surface is introduced; the only behavior change is earlier teardown on hostile input.

Level of scrutiny

Medium-high: it's a small, mechanical guard that mirrors an upstream reference implementation line-for-line, but it lives in the network-facing HTTP/2 frame dispatch loop and carries a CVE tag. The logic itself is trivially auditable; the reason to defer is that protocol DoS guards are the kind of thing a maintainer should sign off on, and the PR explicitly scopes out the sibling gap in fetch()'s h2 client — that scoping call is reasonable but should be a human's to make.

Other factors

  • Error surfacing reuses the existing local_connection_error helper, so GOAWAY emission, session termination, and NghttpError construction follow the same path as FLOODED (-904) and BAD_CLIENT_MAGIC (-903).
  • max_continuations is pub but nothing sets it in this PR; that mirrors nghttp2's nghttp2_option_set_max_continuations (which node doesn't expose either), so the fixed default is fine.
  • The four comment-cop inline notes are marked resolved; the diff shows the trimmed one-line comments.
  • robobun evidence shows the full node-http2.test.js / node-http2-continuation.test.ts / h2-conformance.test.ts suites pass on both debug+ASAN and release.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — small, self-contained CONTINUATION-count guard matching nghttp2's CVE-2024-28182 fix.

What was reviewed:

  • Counter reset covers both header-block entry points (HEADERS at connection.rs:1002 and PUSH_PROMISE at :1709); dispatch() already gates handle_continuation on continuation_stream != 0, so the count can't advance outside a block.
  • Boundary is > 8 (9th frame trips), error path reuses local_connection_error with InternalError GOAWAY + errno -905, same shape as the existing FLOODED/-904 guard; message string matches nghttp2's nghttp2_strerror.
  • Tests cover both the flood → ERR_HTTP2_ERROR/-905 case and the exactly-8-frames-accepted boundary; robobun confirms the flood test times out on main and the existing continuation/conformance suites still pass.
  • The sibling gap in fetch()'s h2 client is called out and tracked separately.
Extended reasoning...

Overview

Adds a per-header-block CONTINUATION frame counter to the node:http2 inbound engine (src/runtime/api/bun/h2/connection.rs): two new struct fields (continuation_count, max_continuations), reset-to-zero at both block-start sites (HEADERS and PUSH_PROMISE), and a > max_continuations check in handle_continuation that tears down via the existing local_connection_error path. wire.rs gains the TOO_MANY_CONTINUATIONS (-905) lib-error constant and DEFAULT_MAX_CONTINUATIONS = 8; http2.ts gains the -905 → message table entry. Two tests added to node-http2.test.js.

Security risks

This is the security fix — a DoS guard against CVE-2024-28182 (unbounded zero-length CONTINUATION frames pinning the receiver). The change is strictly additive: it can only reject input that was previously accepted, never the reverse, so it fails closed. The over-rejection risk (legitimate large header blocks being dropped) is bounded by the negative test and by the fact that 8 × 16 KiB default frame size ≥ 128 KiB compressed, well above the existing byte cap. No auth/crypto/permission surface is touched.

Level of scrutiny

Medium. The mechanism is a counter with a fixed bound, checked before the byte-cap logic, using the same connection-error helper that FLOODED (-904) and BAD_CLIENT_MAGIC (-903) already route through — so the teardown/GOAWAY/JS-error plumbing is unchanged and already exercised. I traced that continuation_stream is set non-zero at exactly two sites (HEADERS :1001, PUSH_PROMISE :1708) and both reset the counter in the same commit; finish_header_block clears continuation_stream so a stale count between blocks is unreachable. The u32 counter cannot overflow before the guard trips at 9.

Other factors

  • robobun evidence shows fails-on-main (timeout) / passes-with-fix in both debug+ASAN and release, plus node-http2-continuation.test.ts and h2-conformance.test.ts still green.
  • Constants and message string are cited to nghttp2 and match Node v26.3.0's observable shape (errno: -905, code: ERR_HTTP2_ERROR, exact message).
  • comment-cop flags were addressed in commit 580d0ea and are marked resolved.
  • max_continuations is pub but not yet JS-configurable — matches Node, which doesn't expose nghttp2_option_set_max_continuations either.

@alii alii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Engine change looks right and matches node: 8 CONTINUATIONs accepted, the 9th rejected even when it carries END_HEADERS, counted per block, on both client and server (checked against node 22; main accepts the 9th on both sides). The tests need work before this lands, same gaps as #36411:

  • nothing rejects a 9th CONTINUATION, so the two tests only pin the cap to somewhere in 8..63
  • neither continuation_count reset is observed by any test, and the PUSH_PROMISE block start is untested
  • nothing exercises createServer or checks the GOAWAY code
    Minor: max_continuations is a knob nothing sets.

Comment thread test/js/node/http2/node-http2.test.js Outdated
Comment thread src/runtime/api/bun/h2/connection.rs Outdated
Comment thread src/runtime/api/bun/h2/connection.rs Outdated
Comment thread src/runtime/api/bun/h2/connection.rs Outdated
@robobun
robobun force-pushed the claude/farm-8692caa2-h2-continuation-count-guard branch from 580d0ea to 7acc182 Compare August 13, 2026 01:45
Comment thread src/runtime/api/bun/h2/connection.rs Outdated
Comment thread src/runtime/api/bun/h2/connection.rs Outdated
Comment thread src/runtime/api/bun/h2/connection.rs Outdated
Comment thread src/runtime/api/bun/h2/connection.rs Outdated
@robobun

robobun commented Aug 13, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:05 AM PT - Oct 17th, 2026

❌ @robobun, your commit 66f2145 has 1 failures in Build #127492 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 36410

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

bun-36410 --bun

@alii alii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ready to merge from this side. The counter now lives on HeaderBlockInFlight, which has a single construction site shared by HEADERS and PUSH_PROMISE, so the per-block reset is structural; the cap is a private const; the four client cases and the two createServer cases pin exactly 8 accepted / 9th rejected (with the message, errno -905 and GOAWAY INTERNAL_ERROR on the server side), and run unmodified against main the accept cases pass while the reject cases fail with main's old behavior (200 delivered, push delivered, request answered); the same six cases pass against a node 22 peer. The two Rust unit tests read correctly but were not executed here, as the body says; nothing on main has touched these files since the merge-base.

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for re-checking against node 22. Nothing further planned from my side; Buildkite #93855 is running on b40d1e9. The red Source lints check is the dead-code-escape inventory (NodeHTTPResponse.rs dropped its last escape in #37977 without the inventory following); it passes on this branch's own base and is failing on every PR right now, and a fix branch is already up, so it is unrelated to this diff.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Since it's a CVE mitigation in the HTTP/2 protocol engine and alii's earlier round has been addressed but not yet re-reviewed, a human sign-off still makes sense.

What was reviewed:

  • Both HEADERS and PUSH_PROMISE reach the single finish_or_park_header_block construction site, so continuations: 0 is per-block by construction with no reset to forget.
  • Boundary math: increment-then-> 8 check accepts the 8th and rejects the 9th, matching nghttp2 and the tests on both sides.
  • The error path (local_connection_error → GOAWAY INTERNAL_ERROR + errno -905) follows the same pattern as FLOODED/BAD_CLIENT_MAGIC; the -905 message string matches nghttp2's.
Extended reasoning...

Overview

The PR adds a per-header-block CONTINUATION frame counter to the node:http2 engine to close CVE-2024-28182 (CONTINUATION flood). Production changes are small: a continuations: u32 field on HeaderBlockInFlight, a private MAX_CONTINUATIONS = 8 const, an increment-and-check in handle_continuation, a new lib_error::TOO_MANY_CONTINUATIONS (-905) constant in wire.rs, and the matching message-table entry in src/js/node/http2.ts. The rest is tests: two Rust unit tests, four client-side JS tests in node-http2.test.js, and two server-side JS tests in h2-conformance.test.ts.

Security risks

This is itself a security hardening (DoS mitigation). The change is strictly additive on the receive path — it only introduces a new terminal condition; nothing that previously errored now succeeds. I confirmed both openers (handle_headers at connection.rs:1026 and handle_push_promise at :1767) go through the single finish_or_park_header_block site that initializes continuations: 0, so the budget cannot leak across blocks. The take-mutate-put-back pattern in handle_continuation correctly leaves header_block_in_flight = None on the error return since it was already .take()n. The 8-frame limit matches nghttp2's NGHTTP2_DEFAULT_MAX_CONTINUATIONS, and 8 × 16 KiB max-frame-size comfortably exceeds the existing byte cap, so legitimate large-header traffic is not newly rejected.

Level of scrutiny

High — this is protocol-state-machine code in a network-facing engine, and a CVE fix. The change itself is mechanically simple and follows the exact pattern of the neighboring FLOODED / BAD_CLIENT_MAGIC local-connection-error paths, but per the approval guidelines security-sensitive code paths should get a human sign-off rather than bot-only approval.

Other factors

alii left a substantive review on 2026-08-12 (four comments: pin the exact 9th-frame boundary, cover the per-block reset via response+trailers, cover PUSH_PROMISE, cover createServer + GOAWAY code, drop the unused pub knob for a private const). The rework in 7acc182 + b40d1e9 addresses each point directly and all threads are resolved, but alii has not yet re-reviewed. The comment-cop bot flags are all resolved (b40d1e9 shortened comments). Test coverage is thorough — at-limit / one-past on client, server, and PUSH_PROMISE, plus a per-block-budget test that would catch a session-wide counter — and the PR body reports the JS suites pass locally under bun bd test and fail on main. The two Rust unit tests were only compile-checked (cargo test -p bun_runtime doesn't link in the author's tree), but the JS tests exercise the same engine paths end-to-end.

alii pushed a commit that referenced this pull request Aug 15, 2026
…36411)

### 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
#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.

<!-- robobun:evidence:begin -->

---

**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

<!-- robobun:evidence:end -->
@alii

alii commented Aug 15, 2026

Copy link
Copy Markdown
Member

@robobun this conflicts with main now, please rebase and get a fresh CI run so it can be merged.

@robobun
robobun force-pushed the claude/farm-8692caa2-h2-continuation-count-guard branch from b40d1e9 to 5d65ca8 Compare August 15, 2026 03:43
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto main (5d65ca8). The only conflict was in the CONTINUATION describe of h2-conformance.test.ts, where #37961 added a reassembly test in the same spot; both are kept. No changes to the engine diff. Locally on the rebased build: node-http2.test.js 381 pass / 6 skip, h2-conformance + node-http2-continuation + h2-push-refusal-staged 79 pass, source lints pass. Fresh CI run is Buildkite #97258.

@robobun

robobun commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased on main (e4c638f) as one commit, 4f0cadb. The branch has no conflicts now.

  • wire.rs: main made the lib_error constants pub(crate). TOO_MANY_CONTINUATIONS is pub(crate) too.
  • connection.rs: main added the stream reset rate limit at the place where this PR adds MAX_CONTINUATIONS. Both are kept.
  • The engine change and the tests are the same as before the rebase.

Checked on a debug build of this commit:

  • test/js/node/http2/h2-conformance.test.ts: 86 pass.
  • test/js/node/http2/node-http2.test.js: 429 pass, 6 skip.
  • A raw client that sends 9 or 1000 empty CONTINUATION frames gets GOAWAY(INTERNAL_ERROR) and the request handler does not run. With 8 frames the request gets its response.

On a debug build of main the same tests fail where they must: the 9th CONTINUATION test in h2-conformance.test.ts and the two 9th CONTINUATION tests in node-http2.test.js (all 6 variants). Main still answers the request after 9 and after 1000 frames.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟡 src/runtime/api/bun/h2/connection.rs — Bun-to-Bun sessions that carry a header block over 9 x the peer's max frame size now lose the whole session, including every other multiplexed stream, where the base delivered it. send_header_block at connection.rs:1977-1992 splits enc_buf into as many CONTINUATION frames as needed with no limit, so a block over about 147 KiB at the default 16384 emits 9 or more CONTINUATIONs. The upgraded receiver kills the connection at connection.rs:1175 with GOAWAY(INTERNAL_ERROR). Node never emits this: nghttp2 fails the submit locally as a stream-level error. Fix: bound the outbound block at the sender (reject the submit for the one stream, as nghttp2 does) so a large header list fails that stream instead of letting the peer tear down the session; cover HEADERS, trailers and PUSH_PROMISE emission.

    Why this was flagged

    Trigger: a Bun server (or client) with a peer that raised maxHeaderListSize above about 147 KiB sends a response, request, trailer or PUSH_PROMISE block larger than 9 x remote_settings.max_frame_size (connection.rs:1958). The loop at connection.rs:1977-1992 writes one CONTINUATION per max-frame chunk with no count check, so it emits 9+ CONTINUATIONs. On the base branch a Bun receiver accepted that block when its enforced_max_header_list_size (connection.rs:1192) covered it; after this change the receiver trips MAX_CONTINUATIONS at connection.rs:1175 and sends GOAWAY(INTERNAL_ERROR), closing every other stream on the session. nghttp2 refuses to pack a header block beyond its framebuf limit and fails the submit (NGHTTP2_ERR_HEADER_COMP / frame error on the one stream), so node never puts a peer in this position. Remedy: cap the outbound block (or the CONTINUATION count) in send_header_block and surface a stream error instead of emitting more than MAX_CONTINUATIONS frames.

    Verification: The new receive-side guard at src/runtime/api/bun/h2/connection.rs:1174-1184 tears down the whole session on the 9th CONTINUATION of a block. On the base branch the only receive-side bound was the byte cap at connection.rs:1192-1197, let cap = (self.enforced_max_header_list_size as usize).max(65536). Bun's send side has no matching bound.

Comment thread test/js/node/http2/h2-conformance.test.ts
Comment thread src/runtime/api/bun/h2/connection.rs
The inbound engine bounded a header block under reassembly only by its
byte size, so a peer sending HEADERS without END_HEADERS followed by
zero-length CONTINUATION frames could hold the session in header-block
state indefinitely: nothing ever errored, the request stayed pending, and
the receiver spent a core parsing 9-byte frames. This is the bug nghttp2
fixed as CVE-2024-28182; node errors after the 8th CONTINUATION.

Count CONTINUATION frames on the in-flight block record and fail the
connection once a block exceeds nghttp2's NGHTTP2_DEFAULT_MAX_CONTINUATIONS
(8), the same way node does: GOAWAY(INTERNAL_ERROR) on the wire and an
NghttpError with errno -905 ("Too many CONTINUATION frames following a
HEADER frame") on the session. The counter lives on HeaderBlockInFlight,
so every HEADERS or PUSH_PROMISE that opens a block starts from zero and
the budget is per block, not per session. Applies to clients and servers
alike since they share the engine.

Tests pin the exact boundary on both sides (8 accepted, the 9th rejected
even when it carries END_HEADERS), the per-block reset (response and
trailers each using the full budget on one stream), the PUSH_PROMISE
opener, and the GOAWAY error code a server emits.
@robobun
robobun force-pushed the claude/farm-8692caa2-h2-continuation-count-guard branch from 4f0cadb to 66f2145 Compare October 17, 2026 10:55
@robobun

robobun commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator Author

CI on 4f0cadb (build 126527) had two red tests that this PR does not touch: bun-install-windows-rename-retry.test.ts on Windows aarch64 and worker-terminate-lifetime.test.ts on macOS x64. Main fixed both after that run (#45588, #45600). The http2 test files passed. node-http2.test.js needed one retry on macOS aarch64, for a 15 s timeout in a test that sends 10,000 requests and no CONTINUATION frame.

Rebased again on main (017b243) as 66f2145, so that the next run has those two fixes. The rebase had no conflicts, and main did not change a file of this PR. On a debug build of this commit: h2-conformance.test.ts 86 pass, node-http2.test.js 429 pass and 6 skip.

@robobun

robobun commented Oct 18, 2026

Copy link
Copy Markdown
Collaborator Author

Reply to the review note on large header blocks (the finding outside the diff). I measured it. The finding is correct, and the PR body lists it under Downsides.

Setup: a server answers with N header fields of 5,000 bytes each. The client sets maxHeaderListSize: 1048576. A second stream stays open on the same session.

response header block bun 1.4.3 this branch node v26.3.0 (1)
35 fields, about 142,000 bytes, HEADERS + 8 CONTINUATION delivered delivered delivered
40 fields, about 162,000 bytes, HEADERS + 9 CONTINUATION delivered errno -905 on the request, on the second stream and on the session errno -905 on both streams

(1) With maxSendHeaderBlockLength: 1048576 on the node server. With the default (65,536) the node server refuses both blocks before it sends a frame: frameError, NGHTTP2_FRAME_SIZE_ERROR, and only that stream fails.

  • The receive side now does what node does. One block can use 9 frames, which is 147,456 bytes at the default frame size of 16,384.
  • Bun has no default maxSendHeaderBlockLength, so a Bun sender can emit such a block with no option set. node:http2: refuse a header block over the default send limit, like node #43474 proposes node's default. I did not add a send limit in this PR. It changes which blocks a sender accepts, and that is a separate decision.
  • Two Bun peers can keep larger blocks: raise maxFrameSize on the receiver together with maxHeaderListSize. The Bun sender fills each frame to the frame size of the peer. Measured on this branch with maxFrameSize: 1048576: blocks of about 162,000 and 486,000 bytes are delivered. Node splits a block at 16,384 bytes in all cases, so this does not help a node sender.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants