Bun.serve: use the absolute-form request-target's authority for request.url - #33002
Bun.serve: use the absolute-form request-target's authority for request.url#33002robobun wants to merge 2 commits into
Conversation
RFC 9112 section 3.2.2: when the request-target is in absolute-form, an origin server must use the request-target's authority and ignore the Host field. Bun.serve did the opposite: it stripped the target's scheme and authority and rebuilt request.url from the Host header, so the real target was unrecoverable by the handler. request.url is now the request-target itself (normalized) when the target is absolute-form. Origin-form requests are unchanged, routing still matches absolute-form targets by path, and the Host header stays visible via request.headers. request_target_path() had no other callers and is removed.
|
Warning Review limit reached
More reviews will be available in 7 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 6:06 AM PT - Jun 28th, 2026
❌ @robobun, your commit 0c601c0 has some failures in 🧪 To try this PR locally: bunx bun-pr 33002That installs a local version of the PR into your bun-33002 --bun |
|
Status: reproduced on 1.4.0 and on main (18e6d8d). A raw CI on 0c601c0: 281 jobs passed. The one failed job (darwin 26 aarch64 - test-bun) is a Buildkite artifact-download timeout ("artifact download timed out after 120s for step 'darwin-aarch64-build-bun'"), not a test failure, and is unrelated to this change. The diff is ready for review. |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
#17348 is related but not fixed here, so I am not adding a closing reference. That issue is about origin-form requests, where the authority of |
There was a problem hiding this comment.
I didn't find any bugs, but this is a security-relevant behavioral change that partially reverts #31495's deliberate choice to derive request.url from Host — worth a human confirming the threat-model trade-off before it lands.
Extended reasoning...
Overview
This PR changes Request::ensure_url / size_of_url in src/runtime/webcore/Request.rs so that when an HTTP/1.1 request arrives with an absolute-form request-target (e.g. GET http://target-b/abs HTTP/1.1), request.url exposes the request-target's own scheme and authority instead of substituting the Host header. The request_target_path helper that stripped scheme+authority is deleted, and absolute-form targets are now normalized via href_from_string. The accompanying test in bun-serve-routes.test.ts is rewritten to assert the new behavior. Origin-form (/path) handling is unchanged.
Security risks
This is squarely in host-confusion territory. The PR's own description frames it as such: a target/Host disagreement is the ingredient of cache-poisoning and vhost/origin-check bypass chains. The RFC 9112 §3.2.2 argument (and Node/Go/nginx parity) is well-made, and the change does make the previously-discarded target authority observable to handlers. But it also means new URL(req.url).host becomes fully attacker-controlled for any client that sends an absolute-form request line — applications that were (perhaps unknowingly) relying on the post-#31495 behavior to get a Host-derived authority would now see whatever the client put on the request line. Whether that's a net security improvement or regression depends on how Bun users typically consume request.url, which is a judgment call for maintainers.
Level of scrutiny
High. This is a semantic change to the request URL surface of Bun.serve, it explicitly reverses one half of a prior merged decision (#31495), and the trade-off is about which of two attacker-influenceable inputs (Host vs. request-line authority) wins. Even though the diff is small and the implementation looks correct, the policy it encodes deserves a maintainer's sign-off rather than bot approval.
Other factors
The implementation itself is tidy: the Cow import and helper are cleanly removed, the &req_url → req_url borrow adjustments are mechanical, size_of_url() stays consistent with ensure_url() for the debug assertion, and the new normalization step mirrors what the origin-form path already does. Test coverage is good (mismatched authority, matching authority, origin-form, query-only absolute-form). No prior human reviews on the timeline yet; CI build was just kicked off.
|
Agreed that the trade-off is the thing that needs a human call; that is why the description spells out exactly which half of #31495 this reverts. Two facts that bound the blast radius for whoever makes that call:
The two choices are also not symmetric in what they let an application do. With this PR, a handler that wants to trust |
What
For an HTTP/1.1 request whose request-target is in absolute-form,
Bun.servereplaced the target's authority with theHostheader when buildingrequest.url. The real target was unrecoverable from the handler.Bun.servetoday (1.4.0 and main)http://host-a/abs?x=1Bun.servewith this PRhttp://target-b/abs?x=1http.createServer(req.url)http://target-b/abs?x=1node:httpserver (req.url)http://target-b/abs?x=1Why
RFC 9112, section 3.2.2:
The same rule is in RFC 7230 (section 5.5) and RFC 2616 (section 5.2: "Any Host header field value in the request MUST be ignored"). Other implementations follow it: Node puts the raw target in
req.url(verified against v26), Go'snet/httptakesreq.Hostfrom the request line when it is absolute, and nginx's$hostprefers the "host name from the request line".Absolute-form targets are what proxies emit, and a target/Host disagreement is the ingredient of host-confusion chains (cache poisoning, vhost/origin-check bypass). Because every conforming component resolves that disagreement in favor of the request-target, Bun resolving it in favor of
Hostis what creates the split, and the handler cannot even detect it today since the target's authority is gone.Relation to #31495
#31495 changed two things for absolute-form targets: route them by path (before that they never matched
routes), and deriverequest.urlfromHost. This PR keeps the routing fix and reverts only the authority choice, which is the part RFC 9112 forbids. Before #31495request.urlwas already the raw request-target for absolute-form, so this restores that behavior, now normalized through the same URL serializer the origin-form path uses.A handler that wants to reject a target/Host mismatch still can:
request.headers.get("host")is unchanged.Implementation
Request::ensure_url/size_of_url(src/runtime/webcore/Request.rs) no longer strip the scheme and authority off the target before prefixingHost; origin-form targets (starting with/) keep theprotocol + Host + pathconstruction, anything else is used as-is, and absolute-form targets are additionally normalized viahref_from_string.request_target_pathhad no other callers and is deleted. Routing (getUrlForRoutingin uWS) is untouched.Tests
Updated the absolute-form test #31495 added in
test/js/bun/http/bun-serve-routes.test.tsto assert the RFC behavior: the target's authority (and scheme) survive intorequest.url, theHostheader stays visible, path routing still matches, and origin-form plus query-only absolute-form targets are covered.now yields
request.url === "https://target.example/admin/secret"(washttp://127.0.0.1:<port>/admin/secret). The updated test fails on main and passes with this change;serve.test.ts,bun-serve-routes.test.ts, andhspec.test.tspass locally.