Repository navigation
Conversation
The request-line parser only accepted targets beginning with "/" (origin-form), plus CONNECT's authority-form and proxy-style absolute URLs, so `OPTIONS * HTTP/1.1` was rejected with 400 Bad Request before it ever reached a handler. Accept "*" as the start of a request-target alongside "/". RFC 9112 3.2.4 defines asterisk-form for server-wide OPTIONS requests, and Node.js (llhttp) delivers such requests verbatim (`req.url === "*"`) for any method, so Bun does the same. Routing and Request.url construction already handle non-slash targets the same way they do for CONNECT's authority-form.
|
Warning Review limit reached
More reviews will be available in 23 minutes and 24 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 11:29 PM PT - Jun 26th, 2026
❌ @robobun, your commit 924e633 has some failures in 🧪 To try this PR locally: bunx bun-pr 32811That installs a local version of the PR into your bun-32811 --bun |
HttpRouter assumes an origin-form target and strips the leading byte, so routing a verbatim "*admin" target segmented it identically to "/admin" and matched the static Bun.serve route. getUrlForRouting() now returns "/" for asterisk-form targets; getFullUrl() still exposes them verbatim to node:http. Check isConnect before isAsteriskForm so `CONNECT * HTTP/1.1` is still classified as a CONNECT tunnel rather than as asterisk-form.
There was a problem hiding this comment.
Thanks — 924e633 addresses both issues I raised (asterisk-form now routes as / so it can't alias static routes, and CONNECT is detected before the asterisk-form check), with regression tests for each. I'd still like a human to sign off given this is the request-line parser.
Extended reasoning...
Overview
This PR teaches consumeRequestLine in packages/bun-uws/src/HttpParser.h to accept the RFC 9112 §3.2.4 asterisk-form request-target (OPTIONS * HTTP/1.1), which was previously rejected with 400 before reaching any handler. It also adds a guard in getUrlForRouting() so asterisk-form targets route as / (only the catch-all matches), and reorders the request-line classification so CONNECT is detected before asterisk-form. A new test file covers OPTIONS * / GET * / *?a=1 on node:http, CONNECT * tunnel-mode, and the Bun.serve static-route-aliasing case.
Prior feedback now addressed
My earlier review on 770b131 flagged (1) a route-confusion vector where GET *admin would alias a static /admin route in Bun.serve, and (2) a regression where CONNECT * lost tunnel mode. Commit 924e633 fixes both exactly as suggested — getUrlForRouting() returns "/" for any *-prefixed target, and isConnect is computed before isAsteriskForm so the short-circuit no longer mis-classifies CONNECT *. Both fixes have dedicated regression tests in the new test file. The bug-hunting pass on this revision found nothing further.
Security risks
The HTTP request-line parser is the front door for every request to node:http and Bun.serve; mistakes here become request-smuggling, route-confusion, or ACL-bypass vectors. The earlier revision of this very PR introduced a route-aliasing path that is now closed, which is exactly why I'd like human eyes on the final shape before it lands.
Level of scrutiny
High. packages/bun-uws/src/HttpParser.h is shared by every HTTP server path in Bun and the change widens the set of accepted request targets. The diff is small and the logic now reads cleanly (origin-form → CONNECT → asterisk-form → absolute-form, mutually exclusive), but parser changes of this kind warrant a maintainer sign-off rather than a bot approval.
Other factors
No CODEOWNERS entry covers this path. CI on the previous push (770b131) reported failures and I don't see a status for 924e633 yet, so it's worth confirming the suite is green before merge.
|
CI status on 924e633: this PR's tests (
Not pushing a retrigger, since the sourcemap failure is deterministic on Windows rather than a flake. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-27, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Repro
OPTIONS * HTTP/1.1(the asterisk-form request target, RFC 9112 3.2.4) is answered with400 Bad Requestby the parser and never reaches the request handler. Node.js delivers it withreq.url === "*".Bun.serverejects it the same way (same parser).Cause
consumeRequestLineinpackages/bun-uws/src/HttpParser.honly admits three target shapes: origin-form (a target beginning with/), CONNECT's authority-form, and proxy-style absolute URLs (http://.../https://...). Anything else falls through toHTTP_HEADER_PARSER_ERROR_INVALID_REQUESTand a 400, so asterisk-form never parses.Fix
Two parts.
consumeRequestLineaccepts*as the first character of a request-target, as a fourth form in the existing cascade, checked after CONNECT so thatCONNECT * HTTP/1.1keeps its tunnel classification. Like Node (llhttp), any method may carry an asterisk-form target and it is delivered verbatim. The origin-form fast path is unchanged.getUrlForRouting()normalizes asterisk-form targets to/before routing.HttpRouter::getUrlSegmentassumes an origin-form URL and unconditionally strips the leading byte, so routing a verbatim*adminwould segment it identically to/adminand match a staticBun.serveroute registered as/admin, a route-confusion vector. That aliasing was verified against an intermediate build carrying only the parser change (GET *admininvoked the/adminhandler, where it was 400-rejected before); with the normalization an asterisk-form target can only route as the root. Nothing else readsgetUrlForRouting(): bothnode:http'sreq.urlandBun.serve'sRequest.urlcome fromgetFullUrl(), so they stay verbatim. Credit to review for catching this: an earlier revision routed the target verbatim.GET /admin, absolute-form, CONNECT authority-form, and non-asterisk invalid targets (GET badtarget) are all unchanged.Verification
test/js/node/http/node-http-asterisk-form-target.test.ts:OPTIONS *andGET *on anode:httpserver assertreq.url === "*"and the200 OKstatus line on the wire;*?a=1is delivered verbatim.CONNECT * HTTP/1.1asserts theconnectevent fires withreq.url === "*"and that bytes written after the handshake arrive as opaque tunnel data rather than being parsed as a second request.OPTIONS *reaches aBun.servefetch()handler.GET *,GET *admin, andGET */adminagainst a server with a static/adminroute all reach the/*fallback, never/admin.On an unfixed build 5 of 6 fail immediately with
Received: "HTTP/1.1 400 Bad Request\r\nConnection: close\r\n\r\n"; theCONNECT *test guards the cascade ordering and already passes on main. All 6 pass with the fix. The existing proxy-style, CONNECT, malformed-method, and header-overflow parser tests are unaffected.