http(h3): reject DATA before final response HEADERS in the HTTP/3 client - #32678
http(h3): reject DATA before final response HEADERS in the HTTP/3 client#32678robobun wants to merge 6 commits into
Conversation
WalkthroughAdds lsquic support for peer application error codes, updates the usockets reset API to pass explicit error codes, rejects HTTP/3 DATA before response HEADERS in the client, and adds a debug-only adversarial test for that protocol failure. ChangesHTTP/3 DATA-before-HEADERS protocol violation
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
|
Updated 8:45 AM PT - Jul 11th, 2026
❌ @robobun, your commit 2db819f has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32678That installs a local version of the PR into your bun-32678 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks — the #ifdef BUN_DEBUG gating addresses my concern. The client-side guard looks correct and mirrors the H2 path, but since this still embeds a routing-bypass test hook in the H3 server request path (debug builds only) and touches protocol-error handling, I'd like a human to sign off on the approach.
Extended reasoning...
Overview
This PR adds a guard in the HTTP/3 client's on_stream_data callback (src/http/h3_client/callbacks.rs) that rejects the stream with HTTP3ProtocolError if any DATA bytes arrive before a final (non-1xx) response HEADERS frame, preventing unbounded growth of stream.body_buffer from a hostile peer. It mirrors the existing HTTP/2 guard at src/http/h2_client/dispatch.rs:432. To test it, the PR adds a debug-only adversarial hook in Http3Context.h / Http3Response.h triggered by an x-bun-test-100-then-data request header, plus a test.skipIf(!isDebug) test in fetch-http3-adversarial.test.ts.
Security risks
My earlier review flagged that the server hook ran unconditionally in release builds, letting any remote client bypass user routing/auth on a production Bun.serve({http3: true}) instance. Commit 175ce88 gates both the hook and endAfterInformational behind #ifdef BUN_DEBUG; I confirmed BUN_DEBUG=1 is set only when c.debug is true in scripts/build/flags.ts:770, matching the existing pattern in quic.c / libusockets.h / BunProcess.cpp. Release builds no longer compile the hook at all, so the production routing-bypass is resolved. The client-side change is purely defensive — it adds a rejection path, never relaxes one — and the len > 0 condition correctly lets a bare-FIN (null,0,fin=1) callback through to deliver().
The residual surface is that debug builds of Bun still honor the magic header on every H3 request before routing. That's acceptable for a non-shipped build, but it's test-only behavior living in the server request path rather than in a test fixture, which CLAUDE.md generally discourages.
Level of scrutiny
Moderate-to-high. The Rust change is small and pattern-matched against H2, but it sits in network protocol error handling (an lsquic FFI callback) where subtle ordering/borrow issues matter. The C++ change introduces a deliberate RFC-9114-violating code path in the server, gated to debug builds — that's a design/policy call (debug-only hook vs. a raw QUIC test fixture or an additional env-var opt-in) that a maintainer should ratify rather than a bot.
Other factors
No CODEOWNERS cover the touched paths. The bug-hunting pass on the revised commit found nothing. The PR description reports all 28+52+45 tests in the three H3 suites passing. Given the security-sensitive area and the embedded test hook design choice, I'm deferring rather than auto-approving.
|
Addressed the review feedback in 5ee206f:
Verified on a debug build: While writing the positive test I found a pre-existing server-side bug: |
175ce88 to
5ee206f
Compare
There was a problem hiding this comment.
Thanks — the test hook is now properly gated (#[cfg(bun_debug)] / #ifdef BUN_DEBUG at all three layers) and moved out of the uws fork, so my earlier concern is resolved. Deferring to a human for the HTTP/3 client/server protocol-path changes themselves.
Extended reasoning...
Overview
This PR adds a 7-line guard at the top of on_stream_data in the HTTP/3 client (src/http/h3_client/callbacks.rs) that rejects DATA frames arriving before a final (non-1xx) response HEADERS with HTTP3ProtocolError, closing an unbounded-buffer-growth vector where a hostile server sends HEADERS(100) followed by an endless DATA stream. To test it, a debug-only server hook is added across three layers: on_h3_request in src/runtime/server/server_body.rs (#[cfg(bun_debug)]), a Rust FFI binding in src/uws_sys/h3.rs (#[cfg(bun_debug)]), and the C++ shim uws_h3_res_test_end_after_informational in src/uws_sys/libuwsockets_h3.cpp (#ifdef BUN_DEBUG). Two tests are added to fetch-http3-adversarial.test.ts: a debug-gated negative test for the malformed sequence, and an all-builds positive test that a legitimate 100-continue → 200 sequence delivers only the final response.
Security risks
My earlier review flagged that the original revision exposed the x-bun-test-100-then-data hook unconditionally in release builds, letting any remote client bypass the user's request handler. That has been fully addressed: the hook is now compiled out of release builds at every layer (Rust server entrypoint, Rust FFI binding, C++ shim), and the test is skipIf(!isDebug). In release builds the header is inert — it falls through to normal routing. I see no remaining security concern with the current revision; the client-side guard itself is a hardening change that strictly reduces attack surface.
Level of scrutiny
This is production-critical networking code: the HTTP/3 client's per-DATA-frame callback and the HTTP/3 server's per-request entrypoint. The core client guard is small, mirrors the existing HTTP/2 equivalent, and is well-justified by RFC 9114 §4.1 / RFC 9110 §15.2. But it sits in a hot path where ordering against deliver(), the receive-pause logic from #29831, and stream/session lifetime all interact, and the debug-only server hook directly manipulates Http3ResponseData state flags and the backpressure buffer. That is the kind of change a human reviewer with HTTP/3 / lsquic context (cirospaciari is suggested) should sign off on rather than a bot.
Other factors
The author addressed my prior feedback thoroughly — moved the hook out of the tracked packages/bun-uws fork, added positive coverage, documented fail-before output for both tests, and verified the existing h3 suites still pass. The PR also surfaces (but intentionally does not fix) a pre-existing server-side QPACK bug around expect: 100-continue on a fresh connection, which the positive test works around with a documented warm-up request; that workaround and the debug-gated test-hook design are both reasonable but worth a human confirming they're acceptable trade-offs.
|
CI status (build 71934, latest, on 2db819f which is rebased onto main and has all review rounds applied): 283/286 passed. All three red lanes are main breaks unrelated to this PR and are each now owned by a dedicated fix session:
Every HTTP/3 test file ( |
|
@robobun bump — pause block confirmed intact after rebase (good). One spec fix before this can land: the guard does a graceful close ( |
|
Thanks, on it. Two things I ran into while digging: 1. 2. The peer can't observe the code without a second small lsquic patch. Both are tiny (~15 lines, one new
Will also file the |
|
Done in 1785c39. Both points addressed, plus the issue filed: #33082 is the 100-continue cold-connection bug, now referenced from the positive 1xx test's warmup comment. The stream error. Two things I found getting there that are worth your eyes:
Fail-before is now exactly the property you asked for. With the wire reset removed (the pre-review behavior) and everything else in place, the server observes Verified with All of |
![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 `@packages/bun-usockets/src/quic.c`:
- Around line 1010-1016: Shorten the existing explanatory comment in the QUIC
stream error handling block so it fits the 3-line comment limit while preserving
the key rationale. Update the comment near the RESET_STREAM/STOP_SENDING
handling in quic.c to keep only the essential explanation: signal an HTTP/3
stream error instead of a clean FIN, and note that RESET_STREAM is used when
canceling the send, with STOP_SENDING carrying the code if the send half is
already closed.
In `@patches/lsquic/stream-error-code.patch`:
- Around line 70-77: The added explanatory comments in the lsquic stream error
code patch exceed the 3-line comment limit and need to be compressed. Shorten
the comment near the STOP_SENDING handling to a compact 3-line version that
still preserves the key invariant: the send half is already closed, only
STOP_SENDING can carry the caller’s error code, and the first error must win;
apply the same tightening to the matching comment block later in the patch as
well.
🪄 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: cb9a8a38-c275-42c9-bce2-119d3f43880e
📒 Files selected for processing (4)
packages/bun-usockets/src/quic.cpackages/bun-usockets/src/quic.hpatches/lsquic/stream-error-code.patchscripts/build/deps/lsquic.ts
RFC 9114 §4.1 requires the final (non-1xx) response HEADERS frame before any DATA. lsquic's client-side frame filter only checks that some HEADERS frame preceded DATA, so a server that sends HEADERS(:status 1xx) followed by DATA reaches on_stream_data with status_code still 0. deliver() then returns without draining body_buffer, letting a hostile peer grow it without bound. Reject the stream with HTTP3ProtocolError on the first such DATA byte, matching the existing HTTP/2 client guard in h2_client/dispatch.rs. A conformant h3 server cannot produce this sequence and there is no raw QUIC framer in the test harness, so a server-side test hook (x-bun-test-100-then-data in Http3Context.h) emits HEADERS(100) + DATA + FIN with no final response. Without the client-side guard the fetch rejects with HTTP3StreamReset only after buffering the DATA and retrying once; with the guard it rejects with HTTP3ProtocolError on the first byte.
The x-bun-test-100-then-data hook in Http3Context.h ran on every request in release builds, letting any remote client bypass the application handler and force an RFC 9114 §4.1-violating frame sequence. Compile it (and Http3Response::endAfterInformational) out of release builds with #ifdef BUN_DEBUG, matching the existing quic.c debug-logging gate, and skip the corresponding test on non-debug builds.
…rim responses The x-bun-test-100-then-data hook lived in the tracked uws fork (Http3Context.h, Http3Response.h), where the next upstream sync would drop it. The response primitive is now an exported entrypoint in Bun's own shim (uws_sys/libuwsockets_h3.cpp, debug builds only) and the request-header routing happens in on_h3_request on the Rust side, so nothing in packages/bun-uws changes. Also adds the positive counterpart to the DATA-after-1xx rejection test: HEADERS(100) -> HEADERS(200) -> DATA -> FIN resolves with the final response. It fails with status 100 if the 1xx skip in on_stream_headers is removed.
…SAGE_ERROR RFC 9114 §4.1.2 requires a malformed response be rejected with a stream error of type H3_MESSAGE_ERROR (0x010E). The client previously tore down the stream with abort()+close(), which is a graceful FIN + STOP_SENDING that a peer cannot distinguish from normal completion. The existing qs.reset() in ClientSession::detach() never corrected this: abort() runs first and sets lsquic's STREAM_U_WRITE_DONE, which trips lsquic_stream_maybe_reset's guard and turns the reset into a no-op. Add ClientSession::fail_malformed, which puts the stream error on the wire (before anything closes the send half) and then fails, and use it at all three malformed-response sites: a connection-specific response field, a missing :status, and DATA before the final response HEADERS. Mirrors h2_client's rst_stream(PROTOCOL_ERROR). lsquic cannot express this with its public API, so patches/lsquic/ stream-error-code.patch fills two gaps: the already-closed-write branch of lsquic_stream_maybe_reset now records the caller's error code (first error wins) so the STOP_SENDING frame carries it instead of NO_ERROR, and the peer's incoming RESET_STREAM / STOP_SENDING code, which lsquic parses and then drops, is stored and exposed via a new lsquic_stream_peer_error_code() accessor. Every internal lsquic caller passes 0, so existing behavior is unchanged. us_quic_stream_reset now takes the application error code explicitly; the one existing caller (detach) keeps H3_REQUEST_CANCELLED. An RFC 9114 §8.1 ErrorCode newtype mirrors the H2 wire::ErrorCode. The test asserts the peer observed 0x010E via a debug-only observer installed by the x-bun-test-100-then-data hook, which now leaves the stream open so only the client's stream error can close it (previously the server's markDone() shut both halves down and raced the assertion). Without the reset the observed code is H3_REQUEST_CANCELLED (0x010C): error: expect(received).toBe(expected) Expected: "10e" Received: "10c" Also files the Bun.serve 100-continue cold-connection bug the positive 1xx test works around: #33082
927cbe2 to
f23d4ad
Compare
2 participants
Problem
Two HTTP/3 client conformance gaps, both in how it handles a malformed response.
1. Unbounded buffering before the final HEADERS (DoS).
on_stream_dataappended every incoming DATA chunk tostream.body_bufferunconditionally and then calleddeliver(). Whenstream.status_code == 0(no final response HEADERS yet),deliver()returned early without draining the buffer. lsquic's frame filter rejects DATA before any HEADERS, but accepts it after a 1xx HEADERS because it only tracks that some HEADERS preceded DATA. A hostile server could sendHEADERS(:status 100)followed by an unbounded stream of DATA frames; each chunk grewbody_bufferwith no consumer and no cap.2. Malformed responses were torn down with a graceful close. RFC 9114 §4.1.2 requires a malformed message be a stream error of type
H3_MESSAGE_ERROR(0x010E). The client'sfail()path didabort()→close(), which puts a FIN +STOP_SENDING(NO_ERROR)on the wire: to the peer that is indistinguishable from normal completion. Theqs.reset()already inClientSession::detach()never corrected this, becauseabort()runs first and sets lsquic'sSTREAM_U_WRITE_DONE, which tripslsquic_stream_maybe_reset's guard and turns the reset into a no-op.Fix
on_stream_datarejects DATA that arrives whilestatus_code == 0, mirroring the existing HTTP/2 guard atsrc/http/h2_client/dispatch.rs:430:ClientSession::fail_malformedputsH3_MESSAGE_ERRORon the wire before anything FINs the send half, then fails the request. It is used at all three malformed-response sites (a connection-specific response field, a missing:status, and DATA before the final HEADERS), not only the new one. It mirrorsh2_client'srst_stream(PROTOCOL_ERROR):The lsquic patch
lsquic's public API cannot express an HTTP/3 stream error once the send half is FIN'd (which every GET already is), and it discards the peer's incoming error code before the application can observe it.
patches/lsquic/stream-error-code.patch(~15 lines) fills both gaps:lsquic_stream_maybe_reset's already-closed-write branch records the caller's error code so theSTOP_SENDINGframe carries it instead of a hardcodedNO_ERROR. First error wins, sodetach()'s laterH3_REQUEST_CANCELLEDcannot overwrite it through lsquic's deferred frame generation.RESET_STREAM/STOP_SENDINGapplication error code, whichlsquic_stream_rst_in/lsquic_stream_stop_sending_inparse and then drop, is stored and exposed via a newlsquic_stream_peer_error_code()accessor.Every internal lsquic caller passes an error code of
0, which preserves theNO_ERRORdefault, so no existing behavior changes.us_quic_stream_resetgains an explicit error-code parameter (the one existing caller keepsH3_REQUEST_CANCELLED), and an RFC 9114 §8.1ErrorCodenewtype mirrors the existing H2wire::ErrorCode.Tests
Because a conformant HTTP/3 server cannot emit
HEADERS(1xx)followed by DATA with no final HEADERS, and there is no raw QUIC framer in the test harness (unlike the raw-TCP servers the HTTP/2 equivalents use), a debug-only server hook (x-bun-test-100-then-data, compiled out of release builds) produces the sequence. The hook deliberately leaves its stream open, so the only thing that can close it is the client's stream error: that makes the error-code assertion deterministic and exercises a trueRESET_STREAM(0x010E)rather than just aSTOP_SENDING. Verified on the wire withBUN_DEBUG_lsquic=1:The test asserts (a) the fetch rejects with
HTTP3ProtocolError, and (b) the server observed0x010E. With the wire reset reverted (the previous behavior) the server instead observesH3_REQUEST_CANCELLED:A positive counterpart verifies a legitimate
HEADERS(100) -> HEADERS(200) -> DATA -> FINstill delivers only the final response. Its warmup request works around a separate, pre-existingBun.serveHTTP/3 server bug with 100-continue on a fresh QUIC connection, filed as #33082.test/js/web/fetch/fetch-http3-adversarial.test.ts(29),fetch-http3-client.test.ts(52), andtest/js/bun/http/serve-http3.test.ts(45) all pass locally.[review] gate passed · iteration 11 · 16 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 11
evidence per changed file