serve: build request.url through one function on every transport - #37635
serve: build request.url through one function on every transport#37635alii wants to merge 11 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughHTTP request URL construction now validates ChangesHTTP request URL normalization
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:05 PM PT - Aug 14th, 2026
❌ @alii, your commit 1dd250d has some failures in 🧪 To try this PR locally: bunx bun-pr 37635That installs a local version of the PR into your bun-37635 --bun |
![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 changes security-sensitive Host-header handling in the HTTP server request-URL path, a human look would still be worthwhile.
What was reviewed:
set_url_from_targetapplies the same three steps asensure_url(is_valid_host_headerbyte-set filter,request_target_pathabsolute-form reduction,href_from_stringcanonicalization) — the bare-path fallback still yields an empty href so the raw path is kept, matching H1.- The H3 call site hardcodes
https: true, which is correct sincehttp3: truerequires TLS. - The test's
evil.example/x#host is rejected by the byte-set (contains/and#), andEXAMPLE.com:443exercises WHATWG canonicalization on both transports.
Extended reasoning...
Overview
The PR closes a transport-parity gap in Bun.serve's server-side request.url synthesis. HTTP/1 requests lazily build req.url in Request::ensure_url, which filters the Host header through is_valid_host_header (RFC 3986 authority byte set), reduces an absolute-form request target to its path via request_target_path, and runs the result through the WHATWG URL parser. The HTTP/3 path in prepare_js_request_context_for (which must populate the URL eagerly because the lsquic request goes away) previously did none of these — it formatted https://{host}{path} verbatim from :authority. The PR extracts the H1 policy into a new Request::set_url_from_target and calls it from the H3 path. Files touched: src/runtime/server/server_body.rs (call site, -20/+4), src/runtime/webcore/Request.rs (+26 for the helper), test/js/bun/http/serve-http3.test.ts (+25 for the parity test).
Security risks
This is itself a security fix: without it, an H3 client controls everything before the path in req.url, so new URL(req.url).pathname can disagree with the routed path (the exact class #33072 fixed for H1). I checked that the new helper reuses the existing is_valid_host_header and request_target_path unchanged, and that the fallback when Host is invalid or the target is not slash-prefixed leaves req.url as the bare path (href_from_string returns empty for a relative path, so the raw value is kept). The hardcoded https: true at the call site is correct because Bun.serve rejects http3: true without TLS. I did not find a way for the new code to accept a Host that ensure_url would reject.
Level of scrutiny
High — this is Host-header injection surface in the production HTTP server. The change is small and mechanically sound (it moves H3 onto the already-validated H1 code path rather than adding new parsing), but URL synthesis from client-controlled bytes is exactly the kind of thing a maintainer should sign off on.
Other factors
The new test asserts both the negative case (evil.example/x# never appears in req.url and H3 matches H1's /admin/secret) and the positive canonicalization case (EXAMPLE.com:443 → https://example.com/p on both transports), which is a good invariant test rather than a snapshot. No prior human review comments; CI is still building.
6e15cc8 to
66637cb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/Request.rs`:
- Around line 914-932: Update set_url_from_target to preserve the extracted path
when bun_url::href_from_string rejects the synthesized authority URL, and apply
the same fallback behavior in ensure_url so invalid absolute URLs are not
retained. Add HTTP/1 and HTTP/3 coverage for malformed ports such as
example.com:abc and invalid percent encodings in the authority.
🪄 Autofix
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: 01f99486-aa56-4c38-87c2-232540be020f
📒 Files selected for processing (3)
src/runtime/server/server_body.rssrc/runtime/webcore/Request.rstest/js/bun/http/serve-http3.test.ts
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes how server-side request.url is synthesized from client-controlled :authority/Host bytes over HTTP/3 — a security-sensitive path — a human look would still be worthwhile.
What was reviewed:
set_url_from_targetmirrorsensure_url's three steps (Host byte-set filter, absolute-form target reduction, WHATWG canonicalization) using the existingis_valid_host_header/request_target_pathhelpers.- Hardcoded
https: trueat the H3 call site is correct — H3 requires TLS (matchesSSLpassed toRequest::inittwo lines up). OwnedStringCell::setderefs the replaced value, so the two-stepset(raw)→set(href)doesn't leak;bun_fmtimport inserver_body.rsis still used elsewhere.- Test asserts H3 == H1 for both a rejected Host (
evil.example/x#) and a canonicalized one (EXAMPLE.com:443→https://example.com/p).
Extended reasoning...
Overview
The PR closes an HTTP/3-only Host injection in Bun.serve. HTTP/1's lazy Request::ensure_url already filters the client Host through is_valid_host_header (RFC 3986 authority byte set), reduces absolute-form targets to their path, and runs WHATWG canonicalization. The HTTP/3 eager path in prepare_js_request_context_for (server_body.rs:3266) did none of that — it formatted https://{host}{path} from :authority verbatim, so a Host of evil.example/x# on GET /admin/secret produced req.url === "https://evil.example/x#/admin/secret", whose new URL(...).pathname is /x while the router matched /admin/secret.
The fix adds Request::set_url_from_target(host, target, https) in Request.rs, which reuses the existing is_valid_host_header and request_target_path helpers and finishes with bun_url::href_from_string — the same three-step policy ensure_url applies for H1. The H3 site now copies the Host header out (the uWS header buffer is aliased) and calls the helper with https: true. A new test in serve-http3.test.ts sends the same two Host values over both transports and asserts identical req.url output.
Security risks
This is the security-sensitive part of the change: server-side request.url is built from untrusted client bytes. The PR tightens the H3 path to match H1's existing defense (added in #33072). I checked that a rejected Host falls back to the bare path (matching H1), that an empty/invalid href_from_string result keeps the raw string rather than clearing req.url, and that the new helper only ever runs on the H3 branch (if Ctx::IS_H3). The hardcoded https: true is safe because HTTP/3 requires TLS (serve throws otherwise). No new attack surface is introduced; the change makes two transports agree on the stricter behavior.
Level of scrutiny
High — this is production request handling that turns adversary-controlled bytes into the URL applications parse for routing/auth decisions. The fix itself is small and mechanical (reuses existing validated helpers rather than adding new parsing), which lowers the risk, but the blast radius of getting server-side req.url wrong is large.
Other factors
- The test asserts cross-transport equality (
h3 === h1) and an exact canonicalized value, and the PR description confirms it fails on the current release. It's placed alongside existing H3 coverage. - Memory:
OwnedStringCell::setcalls.deref()on the replaced value (bun_core/string/mod.rs:2395), so the two consecutiveself.url.set(...)calls don't leak.href_from_stringtakes&Stringand bit-copies (String: Copy), same patternensure_urlalready uses. - The removed
HostFormatterwas a no-op withport: None(per the comments inensure_url/size_of_url), so straight byte-extend is equivalent.bun_fmtand other imports inserver_body.rsremain used. - No prior human review comments to address; CI is building.
|
Confirmed on 1.4.0 over HTTP/1 and on this branch over HTTP/3; fix and tests are in this branch (head 76297d6). Every CI lane that ran is green; the two darwin 14 test shards expired unstarted (CI capacity) and need a Buildkite retry, then a maintainer approval. |
…hority fallback (#37683) ### Problem - Test-only. e47da94 on the base branch (#37635) makes `req.url` fall back to the bare request-target when a Host header passes the byte check but does not parse as a URL authority. - With that fallback in place, the per-byte Host matrix in `test/js/bun/http/request-smuggling.test.ts` fails for the 33-64 and 65-96 byte ranges. - Cause: the matrix still expects `http://a%b/p`, `http://a:b/p`, `http://a[b/p` and `http://a]b/p` for `%`, `:`, `[` and `]`; those hosts now come back as `/p` like the other rejected bytes. - Related: #17348 tracks this symptom class. The code change is in #37635 (and #33721), not here. ### Fix - The matrix's expected-URL predicate drops `%`, `:`, `[` and `]`, with a comment on why those four differ from the rest of the RFC 3986 byte set. - This is the right expectation because `a%b`, `a:b`, `a[b` and `a]b` are inside the RFC 3986 host byte set but are not authorities the URL parser accepts, so the fallback applies to them too. - Three more unparseable Host values sent over a raw socket: `1.2.3.4.5`, an unclosed `[::1`, and a 130-byte label with a bad port. The last one is long enough to take the heap path in `ensure_url`; the existing cases only reach the 128-byte stack buffer. - Verification: against a build of e47da94, the unmodified file fails those two ranges and the updated file passes in full (86 tests). The three new cases fail on the 1.4.0 release build. ### Background - For HTTP/1 requests, `Bun.serve` synthesizes `req.url` by joining the Host header and the request-target: `Host: x` plus `GET /p` becomes `http://x/p`. - Two checks gate that join. `Request::is_valid_host_header` accepts only the bytes RFC 3986 allows in `uri-host [ ":" port ]`, and the joined string then has to pass the URL parser. If either fails, `req.url` is the bare request-target (`/p`); the request is served either way. - The per-byte matrix sends `Host: a<byte>b` for each byte over HTTP/1.1 and HTTP/1.0, in batches, and checks which bytes produce a full URL. 33-64 and 65-96 are two of those batches. - `ensure_url` assembles the URL in a 128-byte stack buffer and moves to the heap when the host is longer. The 130-byte case exists to reach that second path. <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 2 · 1 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/js/bun/http/request-smuggling.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/bun/http/request-smuggling.test.ts bun test v1.4.0 (252c963) test/js/bun/http/request-smuggling.test.ts: (pass) rejects multiple Transfer-Encoding headers with chunked [478.38ms] (pass) rejects Transfer-Encoding with chunked not last [121.83ms] (pass) rejects duplicate chunked in Transfer-Encoding [63.24ms] (pass) rejects Transfer-Encoding + Content-Length [55.39ms] (pass) rejects conflicting duplicate Content-Length headers [55.69ms] (pass) accepts duplicate Content-Length headers with identical values [64.57ms] (pass) rejects empty-valued Content-Length followed by smuggled Content-Length [60.60ms] (pass) accepts valid Transfer-Encoding: chunked [78.27ms] (pass) rejects Transfer-Encoding: gzip, chunked [56.96ms] (pass) accepts Transfer-Encoding with whitespace around chunked [60.64ms] (pass) rejects malformed Transfer-Encoding with chunked-false [58.19ms] (pass) prevents request smuggling attack [53.43ms] (pass) rejects split Transfer-Encoding headers gzip + chunked [49.47ms] (pass) Transfer-Encoding lists with codings other than a single chunked > Bun.serve rejects Transfer-Encoding: x, chunked [61.30ms] (pass) Transfer-Encoding lists with codings other than a single chunked > Bun.serve rejects Transfer-Encoding: chunked, chunked [31.93ms] (pass) Transfer-Encoding lists with codings other than a single chunked > Bun.serve rejects Transfer-Encoding: gzip,\tchunked [33.47ms] (pass) Transfer-Encoding lists with codings other than a single chunked > Bun.serve rejects Transfer-Encoding: identity, chunked [46.69ms] (pass) Transfer-Encoding lists with codings other than a single chunked > Bun.serve rejects a real gzip,chunked body instead of delivering it raw [48.69ms] (pass) Transfer-Encoding lists with codings other than a single chunked > node:http accepts Transfer-Encoding: gzip, chunked (llhttp compat) [318.04ms] (pass) Transfer-Encoding lists with codings other th ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/js/bun/http/request-smuggling.test.ts | 36 ++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) ``` </details> **gate history** · 1 passed · 2 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/js/bun/http/request-smuggling.test.ts 0 0 0 ``` </details> <!-- robobun:evidence:end --> <details> <summary>Original description</summary> Test-only follow-up to #37635 (base is `ali/h3-request-url-host-validation`). The first version of this PR also carried the `Request.rs` fallback; e47da94 on the base branch now does that, so this is down to the test file that commit did not touch. ### Problem With the parse-failure fallback from e47da94, the per-byte matrix in `test/js/bun/http/request-smuggling.test.ts` ("HTTP/1.1 and HTTP/1.0 requests synthesize request.url from the same Host bytes") fails for the 33-64 and 65-96 ranges. It expects `req.url` to be `http://a%b/p`, `http://a:b/p`, `http://a[b/p` and `http://a]b/p` for those four bytes, which is the unparseable-URL behavior the fallback removes; they now come back as `/p` like the rest of the rejected bytes. Verified against a build of e47da94: the unmodified file fails those two cases, the updated file passes in full (86 tests). ### Change - The matrix's expected-URL predicate drops `%`, `:`, `[`, `]`, with a comment saying why those four differ from the rest of the RFC 3986 byte set. - Three more parse-failure shapes for HTTP/1 over a raw socket, complementing the `example.com:abc` / `example.com:99999` / `exa%zzmple.com` cases already on the base branch: `1.2.3.4.5`, an unclosed `[::1`, and a 130-byte label with a bad port. The last one is long enough to take the heap path in `ensure_url` (the 128-byte stack buffer is the only path the existing cases reach). All three fail on the 1.4.0 release build. Related: #17348 is the issue for this symptom class; the code change that addresses it is in #37635 (and, more broadly, #33721), not here. </details>
…'t parse, on both transports
…hority fallback (#37683) ### Problem - Test-only. e47da94 on the base branch (#37635) makes `req.url` fall back to the bare request-target when a Host header passes the byte check but does not parse as a URL authority. - With that fallback in place, the per-byte Host matrix in `test/js/bun/http/request-smuggling.test.ts` fails for the 33-64 and 65-96 byte ranges. - Cause: the matrix still expects `http://a%b/p`, `http://a:b/p`, `http://a[b/p` and `http://a]b/p` for `%`, `:`, `[` and `]`; those hosts now come back as `/p` like the other rejected bytes. - Related: #17348 tracks this symptom class. The code change is in #37635 (and #33721), not here. ### Fix - The matrix's expected-URL predicate drops `%`, `:`, `[` and `]`, with a comment on why those four differ from the rest of the RFC 3986 byte set. - This is the right expectation because `a%b`, `a:b`, `a[b` and `a]b` are inside the RFC 3986 host byte set but are not authorities the URL parser accepts, so the fallback applies to them too. - Three more unparseable Host values sent over a raw socket: `1.2.3.4.5`, an unclosed `[::1`, and a 130-byte label with a bad port. The last one is long enough to take the heap path in `ensure_url`; the existing cases only reach the 128-byte stack buffer. - Verification: against a build of e47da94, the unmodified file fails those two ranges and the updated file passes in full (86 tests). The three new cases fail on the 1.4.0 release build. ### Background - For HTTP/1 requests, `Bun.serve` synthesizes `req.url` by joining the Host header and the request-target: `Host: x` plus `GET /p` becomes `http://x/p`. - Two checks gate that join. `Request::is_valid_host_header` accepts only the bytes RFC 3986 allows in `uri-host [ ":" port ]`, and the joined string then has to pass the URL parser. If either fails, `req.url` is the bare request-target (`/p`); the request is served either way. - The per-byte matrix sends `Host: a<byte>b` for each byte over HTTP/1.1 and HTTP/1.0, in batches, and checks which bytes produce a full URL. 33-64 and 65-96 are two of those batches. - `ensure_url` assembles the URL in a 128-byte stack buffer and moves to the heap when the host is longer. The 130-byte case exists to reach that second path. <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 2 · 1 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/js/bun/http/request-smuggling.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/bun/http/request-smuggling.test.ts bun test v1.4.0 (252c963) test/js/bun/http/request-smuggling.test.ts: (pass) rejects multiple Transfer-Encoding headers with chunked [478.38ms] (pass) rejects Transfer-Encoding with chunked not last [121.83ms] (pass) rejects duplicate chunked in Transfer-Encoding [63.24ms] (pass) rejects Transfer-Encoding + Content-Length [55.39ms] (pass) rejects conflicting duplicate Content-Length headers [55.69ms] (pass) accepts duplicate Content-Length headers with identical values [64.57ms] (pass) rejects empty-valued Content-Length followed by smuggled Content-Length [60.60ms] (pass) accepts valid Transfer-Encoding: chunked [78.27ms] (pass) rejects Transfer-Encoding: gzip, chunked [56.96ms] (pass) accepts Transfer-Encoding with whitespace around chunked [60.64ms] (pass) rejects malformed Transfer-Encoding with chunked-false [58.19ms] (pass) prevents request smuggling attack [53.43ms] (pass) rejects split Transfer-Encoding headers gzip + chunked [49.47ms] (pass) Transfer-Encoding lists with codings other than a single chunked > Bun.serve rejects Transfer-Encoding: x, chunked [61.30ms] (pass) Transfer-Encoding lists with codings other than a single chunked > Bun.serve rejects Transfer-Encoding: chunked, chunked [31.93ms] (pass) Transfer-Encoding lists with codings other than a single chunked > Bun.serve rejects Transfer-Encoding: gzip,\tchunked [33.47ms] (pass) Transfer-Encoding lists with codings other than a single chunked > Bun.serve rejects Transfer-Encoding: identity, chunked [46.69ms] (pass) Transfer-Encoding lists with codings other than a single chunked > Bun.serve rejects a real gzip,chunked body instead of delivering it raw [48.69ms] (pass) Transfer-Encoding lists with codings other than a single chunked > node:http accepts Transfer-Encoding: gzip, chunked (llhttp compat) [318.04ms] (pass) Transfer-Encoding lists with codings other th ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/js/bun/http/request-smuggling.test.ts | 36 ++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) ``` </details> **gate history** · 1 passed · 2 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/js/bun/http/request-smuggling.test.ts 0 0 0 ``` </details> <!-- robobun:evidence:end --> <details> <summary>Original description</summary> Test-only follow-up to #37635 (base is `ali/h3-request-url-host-validation`). The first version of this PR also carried the `Request.rs` fallback; e47da94 on the base branch now does that, so this is down to the test file that commit did not touch. ### Problem With the parse-failure fallback from e47da94, the per-byte matrix in `test/js/bun/http/request-smuggling.test.ts` ("HTTP/1.1 and HTTP/1.0 requests synthesize request.url from the same Host bytes") fails for the 33-64 and 65-96 ranges. It expects `req.url` to be `http://a%b/p`, `http://a:b/p`, `http://a[b/p` and `http://a]b/p` for those four bytes, which is the unparseable-URL behavior the fallback removes; they now come back as `/p` like the rest of the rejected bytes. Verified against a build of e47da94: the unmodified file fails those two cases, the updated file passes in full (86 tests). ### Change - The matrix's expected-URL predicate drops `%`, `:`, `[`, `]`, with a comment saying why those four differ from the rest of the RFC 3986 byte set. - Three more parse-failure shapes for HTTP/1 over a raw socket, complementing the `example.com:abc` / `example.com:99999` / `exa%zzmple.com` cases already on the base branch: `1.2.3.4.5`, an unclosed `[::1`, and a 130-byte label with a bad port. The last one is long enough to take the heap path in `ensure_url` (the 128-byte stack buffer is the only path the existing cases reach). All three fail on the 1.4.0 release build. Related: #17348 is the issue for this symptom class; the code change that addresses it is in #37635 (and, more broadly, #33721), not here. </details>
5564df5 to
34213f5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/bun/http/serve-http3.test.ts`:
- Around line 239-242: Update the test around viaH3 and viaH1 to assert that h3
exactly equals the bare target "/admin/secret" before checking HTTP/1 parity.
Keep the existing evil.example exclusion and transport equality assertions
unchanged.
🪄 Autofix
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: 87d66d36-20c8-4b97-a061-76e29706c461
📒 Files selected for processing (5)
src/runtime/server/server_body.rssrc/runtime/webcore/Request.rstest/js/bun/http/request-smuggling.test.tstest/js/bun/http/serve-http3.test.tstest/js/bun/http/serve.test.ts
|
@robobun get this mergeable |
|
Pushed 5b3d78f to this branch: merged current main in (the only blocker on the last run was the two darwin 26 aarch64 shards that never got an agent, so this is a fresh run on an up to date base) and took the two open nits (both transports now assert the exact Build for the new head is https://buildkite.com/bun/bun/builds/94839. Once it is green the only thing left is an approval, which has to come from a maintainer. |
alii
changed the title
|
Checked dcb31ab locally (debug build): request-smuggling.test.ts 90/90, serve-http3.test.ts 52/52, the serve.test.ts request.url cases 9/9, so the unification keeps the behavior the tests pin on both transports. Build for it is https://buildkite.com/bun/bun/builds/95223; all four review threads are still resolved, so after that the only thing left is an approval. One FYI, not a blocker: for joined URLs over 128 bytes, |
There was a problem hiding this comment.
I re-reviewed the rewritten version and didn't find any bugs — both of my earlier comments (the request-smuggling per-byte matrix and the is_valid_host_header doc wording) are addressed in the current diff. Because this changes the request.url fallback semantics on the Host-header path across both transports, a maintainer sign-off is still worthwhile.
What was reviewed:
synthesize_urlcollapses the three oldensure_urlbranches and the H3 copy into one path; stack/heap join,href_from_stringsame-pointer clone, and the new bare-path fallback all match the old short-URL branch's shape.- Checked that dropping the old long-URL
create_uninitialized_latin1fast path doesn't change output — both went throughhref_from_stringand kept the canonical href. - The updated
formsUrlAuthorityregex in request-smuggling.test.ts drops%:\[\]so the byte-matrix expectations line up with the new fallback; the added heap-path (>128B host) and[::1/1.2.3.4.5cases cover what the old suite didn't.
Extended reasoning...
Overview
Consolidates request.url synthesis into one function pair (url_parts + synthesize_url) shared by HTTP/1's lazy ensure_url and HTTP/3's eager set_synthesized_url. Previously ensure_url had three near-duplicate branches (stack buffer <128, heap latin1 all-ASCII, heap utf8) and the HTTP/3 path in server_body.rs had its own copy that skipped the is_valid_host_header check entirely. The refactor is a net -70 lines in Request.rs. On top of the consolidation there is one intentional behavior change: when a Host value passes the byte-set check but the WHATWG URL parser still rejects the joined URL (e.g. example.com:abc, exa%zzmple.com, unclosed [::1), req.url now falls back to the bare request-target instead of the unparsable absolute string. Tests are updated in three files to cover both transports and the >128-byte heap path.
Security risks
Host-header handling on the server request-URL path is security-relevant (host injection into request.url can shift routing decisions or feed downstream new URL() consumers). The change here is a strict tightening: HTTP/3 now applies the same is_valid_host_header gate HTTP/1 already had, and the parser-reject fallback stops handing user code a string that new URL() throws on. The byte set already excludes / ? # @ \\, so client Host bytes cannot land in the path/query/userinfo positions; that invariant is unchanged. I don't see a way this widens the attack surface.
Level of scrutiny
Medium-high. It is per-request hot-path code in Bun.serve, it changes an observable output (req.url), and the request-smuggling test file is where the Host-injection guarantees live. The refactor itself is mechanical and well-covered, but the fallback-semantics change is a policy decision on a security-adjacent surface — the kind of thing a maintainer should explicitly ack rather than have auto-approved.
Other factors
This PR has already been through several review rounds: CodeRabbit flagged the parser-reject fallback (fixed), robobun independently confirmed and supplied the extra test cases (folded in), my earlier run flagged the request-smuggling byte-matrix break (fixed — file is now in the changed set with formsUrlAuthority) and the stale doc comment (fixed). All four inline threads are marked resolved. robobun's last comment explicitly says the only thing left is a maintainer approval, which matches my read. The bug-hunting pass on the current head found nothing.
|
Build 95223 on dcb31ab failed Pushed 774aa58 here: The other red in 95223, |
synthesize_url joined the URL into a scratch buffer and then had the parser copy it, so every request that materialized req.url paid a second, target-sized allocation. With a 15 KB target that is what req-url-leak.test.ts measures on the ASAN lane (RSS 283 -> 579 MB over the run, against 267 -> 302 MB before the refactor; LeakSanitizer reports no leak). Join into the string that becomes req.url instead: parsing a WTF string is a refcount bump and an already-canonical URL comes back as the same string, so it is one allocation per request again. Non-ASCII targets keep the clone_utf8 path they had before.
There was a problem hiding this comment.
I reviewed the final revision (76297d6) and didn't find any bugs — all four earlier review threads are resolved and the last nit (stale test comment) was addressed. Because this reshapes how request.url is synthesized from the client-controlled Host header on every request across both transports, and touches a per-request hot path whose allocation profile already tripped req-url-leak.test.ts once during the PR, a maintainer sign-off is still worth having.
What was reviewed:
url_parts/synthesize_urlagainst the three deletedensure_urlbranches — same host byte-set filter, same origin-form gate, same ASCII/non-ASCII join split; only the parse-reject fallback changed (raw absolute → bare target), which the tests now pin.- Refcount balance on the
create_uninitialized_latin1join:href_from_stringbumps the WTF impl,joined.deref()releases the join's +1 on both the canonical and reject paths, and the Deadhrefon reject needs no deref. - The HTTP/3 call site in
server_body.rs:hostis copied to aVecbefore the second&mut reqborrow forurl(), so the uWS buffer aliasing note still holds. - The per-byte Host matrix and the new parse-reject cases in
request-smuggling.test.ts/serve-http3.test.ts— assertions match the new fallback on both H1 and H3.
Extended reasoning...
Overview
The PR collapses four slightly divergent request.url builders (three arms of ensure_url for HTTP/1 plus a separate inline block in server_body.rs for HTTP/3) into one decision function Request::url_parts and one builder Request::synthesize_url. Both ensure_url (lazy, HTTP/1) and the new set_synthesized_url (eager, HTTP/3) call the same builder. Net: ~90 lines deleted from Request.rs, ~60 added; ~24 lines deleted from server_body.rs, 4 added. Tests in request-smuggling.test.ts, serve-http3.test.ts, and serve.test.ts are extended to pin the new parse-reject fallback and H1/H3 parity.
The one intentional behavior change: a Host that passes is_valid_host_header's byte-set check but that the WHATWG URL parser still rejects (example.com:abc, exa%zzmple.com, 1.2.3.4.5, unclosed [::1) now yields the bare request-target instead of an absolute string new URL() throws on. This replaces two // TODO: what is the right thing to do sites in the old code.
Security risks
This is squarely security-relevant: request.url is assembled from a client-controlled Host / :authority header, and mistakes here are the mechanism behind Host-header-injection and request-smuggling-adjacent path confusion. The change is a tightening on both fronts — HTTP/3 previously pasted the Host into the URL without is_valid_host_header at all (the deleted server_body.rs block only ran the byte-set check, not the URL-parser round-trip, and before #37669 not even that), and HTTP/1 previously handed out unparsable absolute URLs on parser rejection. The byte set still excludes /, ?, #, @, \, so client bytes cannot escape the host position; the new parse-reject fallback is strictly more conservative than what it replaces. I did not find a way for a Host value to influence anything other than the authority component.
Level of scrutiny
High. This runs once per served request on a production hot path, and the PR itself demonstrates how easy it is to regress here: an intermediate revision (dcb31ab) added a second target-sized allocation per request that failed req-url-leak.test.ts under ASAN, and 774aa58 fixed it by joining directly into a WTF string. The final shape is one allocation per request (WTF impl), same as the pre-refactor >=128 path. size_of_url() is now an estimate rather than exact (the doc comment says so), which is fine for its only caller (calculate_estimated_byte_size → JSC extra-memory reporting). The create_uninitialized_latin1 OOM path (returns (dead, &mut []), so split_at_mut(protocol.len()) would panic) is unchanged from the old code and is the standard OOM-panic behavior in this file.
Other factors
The PR has been through four rounds of review (CodeRabbit, robobun, and two prior automated-review passes from me), each of which found something real that was then fixed: the parse-reject fallback, the stale per-byte-matrix assertions, the exact-value H3 assertion, the is_valid_host_header doc comment, the extra allocation, and the stale 128-byte test comment. All threads are resolved. Test coverage is good: raw-socket H1 probes for the byte-set-passes-but-parse-fails cases, an H1/H3 parity test with exact expected values, and the existing per-byte matrix updated. Given the security surface and the hot-path allocation change, I'm deferring rather than approving so a maintainer can confirm the fallback policy (bare target vs. the #33721 configured-authority approach robobun mentioned) and the single-allocation claim.
|
Build 95301 on 76297d6: 177 of 179 jobs passed, including the x64-asan lane that failed req-url-leak before 774aa58, and everything in the annotations is retry-passed flake outside this PR's files. The build is red only because both darwin 14 aarch64 test shards expired without ever starting (queued 01:48, expired 08:05, no agent); that pool is oversubscribed across the pipeline right now (since yesterday evening about 85 darwin 14 shards expired vs 51 run), so I have not pushed an empty commit to re-roll, since it would requeue all 179 jobs into the same backlog. Retrying just those two jobs in Buildkite once the pool catches up is the cheap fix; my token cannot do that. No review threads are open, so after that it only needs an approval. If you would rather I re-roll, say so and I will. |
There was a problem hiding this comment.
Reviewed again after 1dd250d — the pub(crate) nit is addressed and no issues remain from this pass. Deferring the approval to a maintainer since this reshapes a security-adjacent per-request hot path (Host → request.url authority) and changes the fallback semantics on both transports.
What was reviewed:
synthesize_url/url_partsvs. the three deletedensure_urlbranches and the old H3 block inserver_body.rs— same decisions, plus the new bare-target fallback whenhref_from_stringrejects.- The one-allocation join path (
create_uninitialized_latin1→href_from_string→joined.deref()) is balanced on every exit; the H3 caller's small host.to_vec()is the only added allocation and is bounded. size_of_urlnow usesurl_partsso the estimate stays in step with the actual builder.- Test updates: the per-byte Host matrix now excludes
% : [ ], and the new H1/H3 cases pin the exact bare-target fallback.
Extended reasoning...
Overview
The PR collapses four slightly-different request.url builders — three arms inside HTTP/1's lazy ensure_url (stack buffer <128B, latin1 heap, UTF-8 heap) and HTTP/3's separate eager block in server_body.rs — into one decision function url_parts and one builder synthesize_url, both in Request.rs. ensure_url and the new set_synthesized_url (H3) call the same builder. About 40 lines added in Request.rs, ~90 deleted; the H3 site in server_body.rs shrinks to a two-liner. Tests in request-smuggling.test.ts, serve-http3.test.ts, and serve.test.ts are updated/added to pin the new fallback.
Security risks
This is exactly the code that decides whether client-controlled Host / :authority bytes appear in request.url, so it sits on the request-smuggling / SSRF-adjacent boundary. The change is a strict tightening: HTTP/3 now runs the same is_valid_host_header byte-set check the H1 path already had (previously it did not canonicalize at all), and both transports now fall back to the bare request-target when the joined URL fails the WHATWG parser instead of handing user code a string new URL() rejects. url_parts still gates on origin-form (path.first() == Some(&b'/')) and the byte set still excludes / ? # @ \\, so client bytes cannot land anywhere but the host position. I did not find a way for the refactor to widen what the old code accepted.
Level of scrutiny
High. It is a per-request hot path with an explicit no-extra-allocation constraint (an intermediate revision failed req-url-leak.test.ts on the ASAN lane and was reworked), and it changes user-visible behavior for a class of Host headers. The PR went through several iterations with three prior nits from this reviewer and one from coderabbit, all addressed; the most recent commit is the trivial pub(crate) → private visibility fix I asked for. That is the right level of iteration for this kind of change, but it is also why a maintainer sign-off makes sense rather than a shadow approval.
Other factors
CI on the previous head (76297d6) had 177/179 green with the two red jobs being darwin-14 shards that expired without an agent — infrastructure, not code. The head has since picked up a merge of main and the one-line visibility change. Test coverage is good: the per-byte Host matrix, the new in-byte-set-but-parser-rejects cases (1.2.3.4.5, unclosed [::1, 130-byte host), and the H3 parity test all assert exact values. The remaining open question a maintainer might weigh is whether the new behavior (bare path on parser-reject) is the policy they want vs. the #33721 direction (configured-host authority); robobun called that out earlier as a separate decision that does not block this.
2 participants
request.urlinsideBun.servehandlers was assembled in three slightly different ways for HTTP/1 and a fourth for HTTP/3, and the HTTP/3 copy skipped the Host header checks the others had.Now one function decides whether the Host header may be the URL's authority and what the path is, one function builds the string, and both transports call them. About 40 lines; the rest of the diff is deletion.
What changes for a user, on top of the Host validation #37669 already shipped: a Host that passes the byte check but that the URL parser still rejects (
example.com:abc, bad percent-encoding) now yields the bare path rather than arequest.urlthatnew URL()throws on, and HTTP/3 canonicalises exactly like HTTP/1. Everything else is byte for byte what it was, compared against the release binary for short, long and non-ASCII targets. request-smuggling, serve-http3 and the url/host tests in serve.test.ts pass on a debug build.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/bun/http/serve.test.ts