Repository navigation
Conversation
Over HTTP/3, req.url was assembled as "https://" + :authority + :path with no checks on either value, while the HTTP/1 path (Request::ensure_url) only uses a Host value that passes is_valid_host_header, reduces absolute-form targets to their path, and normalizes the result through the URL parser. So an H3 client could send ":authority: good.com@evil" or "a/b" and get a req.url whose host or pathname disagrees with what the router matched on, and a :path that was not origin-form (an absolute URL, "*", "xfoo") was routed on whatever followed its first byte and handed to the handler verbatim as req.url. Http3Context now answers 400 to any :path that does not start with "/" before routing (RFC 9114 4.3.1 / 4.1.2), which is the layer where HttpParser rejects the HTTP/1 equivalents. The HTTP/1 synthesis is moved out of ensure_url into Request::set_url_from_request_target and the eager HTTP/3 path calls it too, so both transports apply the same authority filter and normalization. The h3 request accessors for url/header take &self so host and path can be borrowed together without copying.
WalkthroughHTTP/3 request handling now validates request targets before routing. Shared URL construction centralizes authority and path handling. URL and header access use immutable request references. New raw QUIC tests cover valid and invalid targets. ChangesHTTP/3 URL validation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on the current release with a |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked that the ensure_url → set_url_from_request_target extraction is a mechanical move (HTTP/1 path unchanged, the two dropped debug_asserts are replaced by the shared url_authority helper), and that the ReqLike::{url,header} signature change to &self matches the existing H1 uws_sys::Request signatures. Given this is security-sensitive input validation on the per-request hot path, a human look is still worthwhile.
Extended reasoning...
This PR closes an HTTP/3 authority/path injection gap and refactors the URL-synthesis hot path. The only finding is a comment-wording nit (RFC 9114 §4.3.1 does permit :path = * for OPTIONS; the rejection is fine for HttpParser parity but the comment overstates the spec). I verified the Request.rs refactor is a byte-for-byte extraction of the old ensure_url body, that size_of_url now shares the same authority selection instead of duplicating it, and that the &mut self → &self change on h3::Request::{url,header} brings it in line with the H1 uws_sys::Request which already used &self. Not approving because this touches security-sensitive validation of hostile network input and the Bun.serve per-request path.
|
Updated 2:39 PM PT - Aug 11th, 2026
✅ @robobun, your commit d26419272f4a237bd5887ccff2e7d0b2d3196122 passed in 🧪 To try this PR locally: bunx bun-pr 37600That installs a local version of the PR into your bun-37600 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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 `@test/js/bun/http/serve-http3.test.ts`:
- Around line 1360-1382: Add a test case alongside the existing HTTP/3 authority
cases that omits :authority while sending a non-CONNECT path, then assert
req.url remains the bare request target and verify the observed host value after
confirming whether uWS exposes it as empty or omitted. Reuse serveRecordingHits,
rawH3, and the existing response/hits assertions.
🪄 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: 5f18231a-fbfb-4d0b-8406-afa8cb27a506
📒 Files selected for processing (5)
packages/bun-uws/src/Http3Context.hsrc/runtime/server/server_body.rssrc/runtime/webcore/Request.rssrc/uws_sys/h3.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; my earlier note on the RFC 9114 §4.3.1 comment was addressed in 3c49b7b, and the comment-cop / CodeRabbit threads are resolved. Because it changes request-target validation on the network boundary, refactors the per-request ensure_url hot path, and overlaps with #33721's approach to the same helper, a human sign-off would still be worthwhile.
What was reviewed:
set_url_from_request_targetis a straight extract of the oldensure_urlbody;size_of_urland the new function now share host selection viaurl_authority, so the removeddebug_asserts no longer guard anything.&mut self→&selfonh3::Request::{url,header}is sound — the C shims only read; the HTTP/1uws_sys::Requestmethods were already&self, so theReqLiketrait relaxation just aligns with them.get_protocol()returnshttps://on the H3 path becauseRequest::initis called withSSL(always true for the H3 context), confirmed by the normalization test.- The 10 new
node:quictests cover invalid:authority, absent:authority, valid-and-normalized, and the four rejected:pathshapes; thehitsarray proves no handler fires on 400.
Extended reasoning...
Overview
Five files: a 9-line guard in Http3Context.h that 400s any :path not starting with / before routing; Request.rs extracts the body of ensure_url into set_url_from_request_target(host, target) and factors the host-selection predicate into url_authority (shared with size_of_url); server_body.rs replaces the H3-only Vec+write! URL assembly with a call to that shared function and relaxes the ReqLike trait so header and url can be borrowed together; uws_sys/h3.rs changes url/header (and their extern decls) to &self/*const; and serve-http3.test.ts gains a 10-case describe driving raw pseudo-headers through node:quic.
Security risks
This is itself a security fix — before it, an HTTP/3 client could put @, /, or ? in :authority and have new URL(req.url).hostname disagree with the router's view, or put a non-/ :path and have HttpRouter strip the first byte and route on the remainder. The fix reuses the exact is_valid_host_header byte-set and request_target_path that HTTP/1 already applies, and the C++ guard mirrors what HttpParser already enforces, so the attack surface converges rather than diverges. I did not find a way for the new code to accept something the old HTTP/1 path rejects.
Level of scrutiny
High — this is untrusted-input validation on the network boundary and a refactor of code that runs once per request. Two things push it toward a human look: (1) the intentional behaviour change that OPTIONS * and CONNECT over H3 now get 400 (documented, matches HTTP/1, but still a user-visible change), and (2) the PR description flags an overlap with #33721, which takes a different approach to the same shared helper (it also changes the HTTP/1 fallback authority). Which of the two lands, or how they reconcile, is a maintainer call.
Other factors
The refactor itself is mechanical: I diffed the old ensure_url body against set_url_from_request_target and the only non-whitespace changes are (a) the two debug_assert!(self.size_of_url() == …) lines dropped — now redundant because both sides go through url_authority — and (b) the // TODO: what is the right thing to do for invalid URLS? comments dropped alongside unchanged behaviour. The &mut → & FFI relaxation is safe because uws_h3_req_get_url/get_header only write to the out-param and read from request-owned storage; the HTTP/1 uws_sys::Request::{url,header} were already &self. Test coverage is strong: 9 of 10 new cases fail-before/pass-after per the PR's evidence block, the hits array asserts the negative (no handler ran on 400), and the surrounding suites the author lists as re-run cover the HTTP/1 ensure_url path that was refactored.
Problem
Bun.serve({ http3: true })builtreq.urlas"https://" + :authority + :pathwithout looking at either value (server_body.rs, theCtx::IS_H3block inprepare_js_request_context_for). The HTTP/1 path (Request::ensure_url) only uses aHostthat passesis_valid_host_header, reduces an absolute-form target to its path, and runs the result through the URL parser. HTTP/3 clients got none of that.Observed with a
node:quicclient against a server withroutes: { "/declared": ... }plus afetchfallback, on the current release:The same requests over HTTP/1 give
req.url = /xfor the first two and400for the last two, becauseHttpParserrejects request-targets that are neither origin-form nor absolute-form.HttpRouterassumes a leading/and strips the first byte unconditionally, which is whyxdeclaredreached the/declaredroute.Fix
packages/bun-uws/src/Http3Context.h: a:paththat does not start with/(absolute URL,*, empty, anything else) gets a400before routing. RFC 9114 4.3.1 defines:pathas the path and query of the target URI (plus*for OPTIONS), so apart from asterisk-form these are malformed requests (4.1.2); this is the same layer where HTTP/1 rejects its equivalents. Side effects:OPTIONS *gets400over HTTP/3 as it already does over HTTP/1 (HttpParseronly accepts origin-form, absolute-form and CONNECT; previously it reached the handler withreq.url === "*"), and a request with no:pathat all (CONNECT) now gets400instead of the router's404.src/runtime/webcore/Request.rs: the body ofensure_urlmoves intoRequest::set_url_from_request_target(host, target), andsize_of_urlshares the host selection throughurl_authority, which replaces the twodebug_asserts that were keeping the two copies in sync. HTTP/1 behavior is unchanged.src/runtime/server/server_body.rs: the HTTP/3 eager path calls the same function, so an:authoritythat cannot be a URL authority is left out ofreq.url(it becomes the bare path, exactly as HTTP/1 does) and a valid one is normalized the same way. This also drops the per-requestVec+core::fmtformatting that server: assemble H3 request URL directly into WTFStringImpl #31055 set out to remove; the shared code uses the stack buffer / direct latin1 paths.src/uws_sys/h3.rs:Request::urlandRequest::headertake&self(the C++ side only reads), so host and path can be borrowed at the same time instead of copying one of them.#33721 also routes HTTP/3 through a shared helper as part of changing the HTTP/1 fallback to the server's configured authority; this PR keeps the current fallback and only closes the HTTP/3 gaps, including the
:pathone that #33721 does not cover.Tests
test/js/bun/http/serve-http3.test.ts, newdescribeusing anode:quicclient so the pseudo-headers can carry arbitrary bytes (the native client always derives them from a parsed URL)::authorityofgood.com@evil,a/b,a?b,a b: served withreq.url === "/x", raw value still visible inreq.headers.get("host"):authorityLOCALHOST:<port>with/x/./y/../z?q=1:req.url === "https://localhost:<port>/x/z?q=1":pathofhttps://other-tenant/declared,xdeclared,*,"":400, no handler invokedAll 9 fail on the unpatched build (verbatim URLs / 200s as in the table above) and pass with the fix. Also ran
serve-http3.test.ts(57 pass),serve.test.ts(the 4 failures there are IPv6 / root-port environment failures that fail identically without this change),request-smuggling.test.ts,bun-serve-routes.test.ts,websocket-server.test.ts,serve-protocols.test.ts,fetch-http3-client.test.tsandfetch-http3-adversarial.test.ts. A 2048-request run with a 15 KB request-target on the debug build measured the same per-request time before and after theensure_urlrefactor (about 23.2 ms vs 23.5 ms, within run-to-run noise).[review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file