Bun.serve http2: reject the request bytes the HTTP/1 parser rejects - #40676
Conversation
Over HTTP/2 an unknown :method reached the handler as GET, :path accepted control bytes and SP, and field names with trailing whitespace or non-token bytes reached the handler trimmed or verbatim. The HTTP/1 parser on the same port rejects all of these. - :method must be a token. A token Bun.serve has no Method for gets 501 on its stream before the router runs. - :path rejects bytes at or below 0x20. Field values reject control bytes other than HTAB. Field names must be lowercase tokens. - lshpack no longer trims trailing whitespace from literal field names (patches/lshpack/no-name-trim.patch), so the validators see the bytes the peer sent.
|
Warning Review limit reached
On-demand reviews are free for the next 24 days. After that, they cost $0.25 per reviewed file. Or wait 59 seconds for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it tightens request-byte validation on the network-facing HTTP/2 hot path and patches vendored lshpack in a way that also reaches node:http2 and the fetch h2 client, a human look is still worthwhile.
What was reviewed:
validFieldName/validFieldValue/validPseudoHeaderTargetedge cases — empty name, bare":", and CONNECT (empty:path) all still take the intended branch.isKnownMethodgating inhandleHeaderBlock— the 501 path creates and registers the stream beforewriteStatus()->end(), matching the existing 404-after-route lifecycle, andCONNECT/M-SEARCHpass the[A-Z-]prefilter.- The lshpack patch removes only the trailing-whitespace trim; the
len == 0guard is preserved. - New tests follow the file's existing raw-frame pattern and each asserts the connection survives on a follow-up stream.
Extended reasoning...
Overview
This PR tightens HTTP/2 request-header validation in packages/bun-uws/src/Http2Context.h and Utilities.h to match RFC 9113 §8.2.1 and align with what the HTTP/1.1 parser on the same port already rejects: :method must be a non-empty token, :path may not carry bytes ≤ 0x20, field names must be lowercase RFC 9110 tokens, and field values reject all C0 controls except HTAB. A new isKnownMethod check answers 501 for token-valid but unrepresentable methods before the router runs (previously they reached the "any" handler and were reported as GET). A new vendor patch patches/lshpack/no-name-trim.patch (wired into scripts/build/deps/lshpack.ts) removes upstream lshpack's trailing-whitespace trim on decoded literal header names so names like "x-a " reach the validator byte-exact. 26 new cases are added to test/js/bun/http/serve-http2-protocol.test.ts.
Security risks
This is protocol-level input validation on a network-facing server — squarely security-sensitive. The direction of the change is strictly tightening (more inputs rejected, fail-closed via RST_STREAM PROTOCOL_ERROR), which reduces rather than adds attack surface, and the request-smuggling-adjacent aliasing (/sta\ttic → /static, x-a → x-a, unknown method → GET) it closes is a real class. However, the lshpack patch changes decoder behavior seen by every HPACK consumer in the process (Bun.serve h2, node:http2, fetch h2 client), and the PR relies on those other paths already validating names to absorb the behavior change. That cross-subsystem reach is the main reason a human should confirm the other h2 suites still pass.
Level of scrutiny
High. Per the repository's review rules, changes to security-sensitive parsing of untrusted input, hot-path header validation, and vendored-dependency patches each independently warrant human review. The change is not mechanical: it introduces a new user-visible behavior (501 for unknown :method), a new shared helper (isTokenByte), and a permanent divergence from upstream lshpack.
Other factors
No CODEOWNERS entry covers these paths. The bug hunt exited on dry_streak with no findings. Test coverage is thorough — malformed cases each assert both the RST_STREAM and connection survival, the 501 loop covers lowercase/mixed-case/underscore variants, and a positive case pins every tchar and HTAB-in-value as still accepted. I checked the trickier control-flow edges (unsigned i = n && p[0] == ':' handles n==0 and bare ":"; CONNECT skips the new :path byte scan; the 501 branch registers the stream before ending it, mirroring the 404 path) and found nothing wrong, but the combination of protocol-layer semantics and a vendored-dep patch is exactly the kind of change a maintainer should sign off on.
|
On the lshpack patch reaching the other HPACK decoders: the two other consumers validate names after decode, so the only visible change for them is that a peer-sent
Suites run locally with this branch's debug build, all green: The one red lane in CI so far is |
|
Updated 6:14 PM PT - Aug 27th, 2026
❌ @robobun, your commit f5b2098 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 40676That installs a local version of the PR into your bun-40676 --bun |
|
CI on f5b2098: 179 of 181 jobs pass. The two red lanes fail on main as well and do not touch this diff:
|
…body, as HTTP/1.1 does (#40692) ### Problem - Over `http2: true`, `req.url` is `scheme://host` plus the raw `:path`. The HTTP/1.1 path on the same port runs the request target through the WHATWG parser. So `/a/../s`, `/./s` and `/%2e/s` reach the handler as sent over h2 and as `/s` over h1. Anything keyed on `req.url` (a guard, a logger, a cache) sees a different string per transport for the same resource. The cause is the eager URL build for MUX requests in `src/runtime/server/server_body.rs` (`prepare_js_request_context_for`), which skipped the `bun_url::href_from_string` pass that `Request::ensure_url` applies for h1. - A POST, DELETE, OPTIONS or PURGE whose HEADERS frame carries END_STREAM gets an empty `ReadableStream` as `req.body`. HTTP/1.1 gives `null` for a request with no Content-Length and no Transfer-Encoding, including `content-length: 0`. The arming rule was `req_len > 0 || is_te || IS_MUX`: every body-method request over h2 or h3 got a pending body. ### Fix - The MUX URL build now runs the same `href_from_string` normalization as `ensure_url`, with the same fallback to the raw string when the parser rejects the input. - New `uws_h2_res_request_body_ended` (`src/uws_sys/libuwsockets_h2.cpp`): true when the stream is already half-closed by the peer or declared `content-length: 0`. `RespLike::request_body_ended` exposes it, and the arming rule becomes `req_len > 0 || is_te || (IS_MUX && !request_body_ended)`. HTTP/3 answers `false` (the QUIC FIN is only seen by a later read), so its behavior is unchanged. - Correct because a stream that is half-closed by the peer cannot carry DATA, and `content-length: 0` with a later empty DATA frame only completes the stream. The C++ layer already rejects END_STREAM with `content-length > 0`. - Verified: `test/js/bun/http/serve-http2.test.ts`, two new tests over TLS and cleartext (4 cases, all fail on main). Each compares h2 against an HTTP/1.1 request on the same port. Also `serve-http2-protocol`, `serve-http2-lifecycle`, `serve-http3`, `serve-protocols` (367 pass). ### Background - `Request.url` for HTTP/1 is computed lazily from the uWS request in `Request::ensure_url` (`src/runtime/webcore/Request.rs`). HTTP/2 and HTTP/3 requests populate `url` and `headers` eagerly at dispatch because the uWS request handle does not outlive the callback. - `req.body` starts as `BodyValue::Null`. When bytes may arrive, the server installs a `Locked` pending value and arms the transport's onData callback. The JS `Request.body` getter reports `null` only for `Null`. - `Http2Response::remoteClosed` is set from the END_STREAM flag before the router runs, so the information is available at the point the body is armed. <details><summary>Notes</summary> - The uWS routers (h1 and h2) match on the raw path, so `routes["/s"]` still does not match `/a/../s` on either transport. That is unchanged here and identical across transports. - With `content-length: 0` and no END_STREAM, the handler may answer before the empty DATA frame arrives. The stream then ends with RST_STREAM NO_ERROR after the response, the existing early-response path, and the late DATA frame is ignored. - The fixture gained a `/body-null` route that returns `String(req.body === null)`. - Follows #40676 (protocol-level validation). This PR is the app-layer parity part. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 3 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 12 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/bun/http/serve-http2.test.ts" bun test v1.4.1 (65362b5) test/js/bun/http/serve-http2.test.ts: (pass) Bun.serve http2 (TLS + ALPN) > ALPN negotiated h2 [784.52ms] (pass) Bun.serve http2 (TLS + ALPN) > GET through fetch handler [381.37ms] (pass) Bun.serve http2 (TLS + ALPN) > POST body is echoed with status and request headers [94.74ms] (pass) Bun.serve http2 (TLS + ALPN) > POST with END_STREAM on HEADERS (no body) resolves req.text() [33.97ms] (pass) Bun.serve http2 (TLS + ALPN) > 204 has no body [32.41ms] (pass) Bun.serve http2 (TLS + ALPN) > HEAD returns content-length and no body [33.91ms] (pass) Bun.serve http2 (TLS + ALPN) > unknown route is 404 from fetch [27.12ms] (pass) Bun.serve http2 (TLS + ALPN) > routes: params, per-method, static Response, file route [382.14ms] (pass) Bun.serve http2 (TLS + ALPN) > request url and headers reach the handler; :authority becomes host [169.53ms] 188 | ["/%2e/headers", "/headers"], 189 | ['/headers?q=a"b<c>', "/headers?q=a%22b%3Cc%3E"], 190 | ]) { 191 | ... (truncated) release without fix: 2 FAILED bun test v1.4.1-canary.1 (7379514) test/js/bun/http/serve-http2.test.ts: (pass) Bun.serve http2 (TLS + ALPN) > ALPN negotiated h2 [16.94ms] (pass) Bun.serve http2 (TLS + ALPN) > GET through fetch handler [4.70ms] (pass) Bun.serve http2 (TLS + ALPN) > POST body is echoed with status and request headers [1.04ms] (pass) Bun.serve http2 (TLS + ALPN) > POST with END_STREAM on HEADERS (no body) resolves req.text() [0.46ms] (pass) Bun.serve http2 (TLS + ALPN) > 204 has no body [0.42ms] (pass) Bun.serve http2 (TLS + ALPN) > HEAD returns content-length and no body [3.45ms] (pass) Bun.serve http2 (TLS + ALPN) > unknown route is 404 from fetch [0.38ms] (pass) Bun.serve http2 (TLS + ALPN) > routes: params, per-method, static Response, file route [29.64ms] (pass) Bun.serve http2 (TLS + ALPN) > request url and headers reach the handler; :authority becomes host [0.79ms] (pass) Bun.serve http2 (TLS + ALPN) > req.url is normalized the same way HTTP/1.1 normalizes it on the same port [14.05ms] (pass) Bun.serve http2 (TLS + ALPN) > req.body is null for a GET whose HEADERS frame carries END_STREAM [0.40ms] (pass) Bun.serve http2 (TLS + ALPN) > req.body is null for a POST whose HEADE ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/bun/http/serve-http2.test.ts" bun test v1.4.1 (65362b5) test/js/bun/http/serve-http2.test.ts: (pass) Bun.serve http2 (TLS + ALPN) > ALPN negotiated h2 [455.26ms] (pass) Bun.serve http2 (TLS + ALPN) > GET through fetch handler [225.53ms] (pass) Bun.serve http2 (TLS + ALPN) > POST body is echoed with status and request headers [54.93ms] (pass) Bun.serve http2 (TLS + ALPN) > POST with END_STREAM on HEADERS (no body) resolves req.text() [20.66ms] (pass) Bun.serve http2 (TLS + ALPN) > 204 has no body [18.46ms] (pass) Bun.serve http2 (TLS + ALPN) > HEAD returns content-length and no body [19.20ms] (pass) Bun.serve http2 (TLS + ALPN) > unknown route is 404 from fetch [15.38ms] (pass) Bun.serve http2 (TLS + ALPN) > routes: params, per-method, static Response, file route [257.66ms] (pass) Bun.serve http2 (TLS + ALPN) > request url and headers reach the handler; :authority becomes host [79.33ms] (pass) Bun.serve http2 (TLS + ALPN) > req.url is normalized the same way HTTP/1.1 normalizes it on the same port [323.63ms] (pass) Bun.serve ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 80de99f features baseline 23 deps, 131 codegen, 1172 objects in 843ms ninja: Entering directory `/workspace/bun/build/release' [1/143] fetch WebKit (prebuilt) [WebKit] up to date [2/143] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [3/143] gen cpp.rs (cppbind) [4/143] gen JS modules (bundle-modules) Preprocess modules (8418ms) Bundle modules (198ms) Postprocesss modules (762ms) Bundle Functions (876ms) Generate Code (32ms) [10.29s] Bundled "src/js" for production 2595 kb 197 internal modules 13 native modules 50 internal functions across 16 files [4/143] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_runtime v0.0.0 (/workspace/bun/src/runtime) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[9 ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/server/server_body.rs | 32 +++++++++--- src/uws_sys/h2.rs | 5 ++ src/uws_sys/libuwsockets_h2.cpp | 7 +++ test/js/bun/http/serve-http2-fixture.ts | 2 + test/js/bun/http/serve-http2.test.ts | 92 +++++++++++++++++++++++++++++++++ 5 files changed, 132 insertions(+), 6 deletions(-) ``` </details> **gate history** · 5 passed · 0 rejected · iteration 3 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/server/server_body.rs 9 17 0 src/uws_sys/h2.rs 6 6 0 src/uws_sys/libuwsockets_h2.cpp 3 3 0 test/js/bun/http/serve-http2-fixture.ts 2 2 0 test/js/bun/http/serve-http2.test.ts 6 5 0 ``` </details> <!-- robobun:evidence:end -->
2 participants
Problem
http2: true, an unknown:method(BREW, lowercaseget) reaches the handler asreq.method === "GET": the any-method route takes it andRequestContext::createmaps an unknown method to GET (src/runtime/server/RequestContext.rs:1346).:pathaccepts HTAB, SP and control bytes, so/sta\tticreaches the handler andnew URL(req.url).pathnameis/static. lshpack trims trailing whitespace from field names before validation (lshpack.c:1893), sox-aarrives asx-a. Names with(),;={}"and values with control bytes pass.Fix
validPseudoHeaderTarget(packages/bun-uws/src/Utilities.h)::methodmust be a token,:pathhas no byte at or below 0x20.validFieldNameaccepts lowercase token bytes only.validFieldValuerejects control bytes other than HTAB. Each is RST_STREAM PROTOCOL_ERROR, the connection survives.Methodfor gets501 Not Implementedon its stream before the router runs (Http2Context.h,handleHeaderBlock). The known set is the HTTP/1 parser's strict set, case-sensitive.patches/lshpack/no-name-trim.patchremoves the trim. node:http2 and the fetch h2 client already validate names, so they now rejectx-ainstead of seeingx-a.test/js/bun/http/serve-http2-protocol.test.ts(26 new cases, 21 fail on main). Also the other h2, h3, node:http2 and fetch h2 suites.Background
Http2Connection::handleHeaderBlock, then routes through the uWS router. Thefetchhandler sits on the any-method node.Request.methodis a fixed enum. No arbitrary token can reach JS, so an unknown method must be rejected at the protocol layer./sta\tticbecomes/static.Notes
BREW / HTTP/1.1closes the socket with no response (no route for the method);\x01as the method: 400; HTAB, SP or a control byte in the target: 505; trailing SP or a non-token byte in a field name: 400; a control byte in a value: 400. DEL and bytes at or above 0x80 in the target or a value: accepted. h2 accepts those two as well after this change.req.urlover h2 is the raw:path, over h1 it is WHATWG-normalized ("becomes%22). Bytes at or above 0x80 in the path fold to U+FFFD on both.OPTIONS *reaches the handler withreq.url === "*". HTTP/3 (packages/bun-usockets/src/quic.c) validates field names as tokens but has no pseudo-header validation.isKnownMethodis[A-Z-]plusBun__HTTPMethod__from, which is the HTTP/1 parser's strict method check.Method::whichalone acceptsget(fornew Request("get")), so a lowercase wire method would still have been reported asGET.[review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file