Repository navigation
Conversation
|
Status Reproduced on main: const server = Bun.serve({
port: 0,
routes: { "/*": { GET: () => new Response("star") } },
fetch: req => new Response("fetch saw " + req.method),
});
for (const method of ["GET", "POST", "PROPFIND"]) {
const answer = await fetch(server.url, { method }).then(
async res => `${res.status} ${await res.text()}`,
err => err.code,
);
console.log(method, "->", answer);
}
server.stop(true);Before: With #45396 alone the last line is The stack: #44510, #45396, #45397, #45398, #45399 (draft), #45400 (draft). CI: the one red test in the runs of this stack is |
c58a788 to
f81c8f6
Compare
9cd6431 to
6a5010d
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked whether the new debug_assert!(false, ...) arms in App::method and the two RouteKind::from_method can trip on user input: the only caller passing a user-chosen method is the per-method user route registration in set_routes, and ServerConfig only accepts the 9 method keys that have route functions, so they are unreachable from routes objects. I also checked that the negative-route loop is unchanged apart from the negative_h1 -> default_h1 rename.
Extended reasoning...
The change collapses the per-method "/" fallback registration in src/runtime/server/mod.rs into a single any-method registration for H1 and H2/H3, and adds debug-only assertions in src/uws_sys for methods without a route function; it touches no auth, crypto, or input-parsing surface. Two confirmed behavioral regressions were posted inline (file-route-on-"/" yield path and websocket upgrade under a GET-only "/*" route), so a human should weigh those before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/server/mod.rs— Users withwebsocketand"/*": { GET: fn }get every WebSocket upgrade routed to fetch, or a 403 when there is no fetch, and fn never sees the upgrade. Step 3 registers fn's upgrade route with app.ws at mod.rs:2428 but only an any-method route sets has_any_ws_route_for_star_path (mod.rs:2398-2400). The fallback app.ws("/") at mod.rs:2636-2644 then registers the same method, pattern and HIGH priority, and HttpRouter::add (HttpRouter.h:282) removes fn's route first. Fix: set has_any_ws_route_for_star_path for a GET "/" user route as well, so the fallback ws route is only added when no "/*" upgrade route exists.Why this was flagged
Trigger:
Bun.serve({ routes: { "/*": { GET: (req, server) => server.upgrade(req) ? undefined : new Response("x") } }, websocket: {...} })without a fetch, then a client opensws://host/anything. mod.rs:2427-2434 registers the user's upgrade route via app.ws("/", ud, 1) for the GET route, but only the RouteMethod::Any arm at mod.rs:2398-2400 sets has_any_ws_route_for_star_path. mod.rs:2636 therefore registers the fallback app.ws("/", self_ptr, 0). Both go through onHttp with upgrade=true at HIGH_PRIORITY (HttpContext.h:1160) on the same pattern, and HttpRouter::add calls remove(methods[0], pattern, priority) at HttpRouter.h:282, so the user's handler is culled and only the fallback remains. The fallback runs fetch, or answers 403 when fetch is absent, so fn's server.upgrade never runs. The base branch has the same gap; the diff rewrote this coverage tracking (renaming the flag at mod.rs:2356) and the PR text lists the double registration under 'Not in this PR', so it was examined and left. Population: every server combining websocket with a per-method "/*" route; rate is every upgrade request.Verification: Pre-existing: the base at 6dec63d has the identical ws logic at its lines 2358/2403/2435/2653-2655. mod.rs:2397-2400 sets
has_any_ws_route_for_star_path = trueonly inside theRouteMethod::Anyarm; the GET arm at mod.rs:2425-2434 never sets the flag, so mod.rs:2636-2644 registers a second ws route on the same pattern and HttpRouter.h:280-282add()removes the user route's handler.
f81c8f6 to
71e415c
Compare
6a5010d to
c7df286
Compare
|
On the additional finding of the review ( Today every upgrade request on such a server reaches On any other path the route gets the upgrade request, so the change is consistent. But it can break a server that works today, so it needs its own PR and a maintainer decision. The PR body lists it under "Not in this PR". |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I checked the router semantics this PR leans on in packages/bun-uws/src/HttpRouter.h and Http2Context.h/Http3Context.h: any() registers at LOW_PRIORITY and a node's handlers are sorted by priority, so a per-method "/*" route (MEDIUM) runs before default_h1 and the default only sees what it yields; the remove(methods[0], ..) at the top of HttpRouter::add can only evict an earlier default_h1 from a "/*": false negative route, never a user route. I also confirmed the new debug_assert!(false, ..) arms in App.rs/h2.rs/h3.rs cannot trip from user input, since ServerConfig.rs probes only the 9 method keys that have route functions, and that http_method::Set still has callers after the deletions.
Extended reasoning...
The change replaces per-method registration of the Bun.serve "/*" default handler with a single any-method registration on H1 and the H2/H3 mux routers in src/runtime/server/mod.rs, adds debug-only assertions in src/uws_sys/App.rs, h2.rs and h3.rs, and adds protocol-matrix tests. It touches no auth, crypto or input-parsing surface, but it changes request-dispatch behavior for HTTP/1.1, HTTP/2 and HTTP/3 in ways the author documents as intentional, so it is not a mechanical change suitable for an approve.
c7df286 to
fbde686
Compare
71e415c to
a375993
Compare
fbde686 to
47a7b48
Compare
There was a problem hiding this comment.
I reviewed the latest push and didn't find any bugs; because this changes user-visible routing precedence (HEAD on a non-function GET "/*" route now comes from the route, and any-method routes now run ahead of fetch on HTTP/2 and HTTP/3), a maintainer's look at the intended semantics is still worthwhile.
What was reviewed:
- Read packages/bun-uws/src/HttpRouter.h and the H1/H2/H3
onHttpregistrations: handlers at a node run in priority order (MEDIUM method route before the LOW any route), H1any()fans out to all 36 method names, and H2/H3 consult the "" tree only after the method tree, so the singleany("/*")default is reached after a per-method "/" route yields. - Checked that the new
debug_assert!inApp::method/RouteKind::from_methodis unreachable from user input: route objects accept only the 9 methods listed at src/runtime/server/ServerConfig.rs:814. - Confirmed no leftover readers of the removed coverage variables, that the H1 and H2/H3 branches now use the same handlers as before, and that the earlier dynamic-import nit is addressed by the module-scope import of bun-serve-routes-page.html.
Extended reasoning...
The commit rewrites step 9 of NewServer::set_routes in src/runtime/server/mod.rs so the default handler is one any-method "/" route (H1 and H2/H3) instead of a per-method loop limited to 9 of 36 methods, collapses the per-method coverage set into one bool, and adds debug-only assertions in src/uws_sys/App.rs, h2.rs and h3.rs; it touches no auth, crypto or input-parsing surface. The uWS router semantics the change relies on were verified from HttpRouter.h, Http2Context.h and Http3Context.h rather than from the comments, and the new tests cover the HTTP/1.1, HTTP/2 and HTTP/3 matrix with toEqual on full result tables. It is deferred rather than approved because it intentionally changes answers that requests previously received (HEAD on static "/" GET routes, any-method route precedence over fetch on H2/H3), which is a product decision for a maintainer; the bug hunt exited on dry_streak with no findings and no third-party objections are outstanding.
a375993 to
b9147c1
Compare
47a7b48 to
142d9eb
Compare
There was a problem hiding this comment.
I reviewed the latest push and found no bugs; the two earlier inline items (file-route yield on an any-method "/", dynamic HTML import) are addressed in the code. Because this changes user-visible routing of Bun.serve (HEAD on a non-function GET "/" route, any-route ordering over HTTP/2 and HTTP/3), a human look at the intended behaviour table would still be worthwhile.
What was reviewed:
- Checked the uWS router semantics the fix relies on:
onHttp("*")registers at LOW_PRIORITY under all 36 method names (HttpContext.h:1148-1156) andHttpRouter::routetries the method tree before the "" tree, so a per-method "/" route still wins and a yielding one falls through to the newanydefault. - Checked the collapsed coverage flag against the old paths: no-"/"-route, any-method "/", and DevServer-H1-only cases register the same handlers as before; only the partial-method case changes, and it is covered by the new tables.
- Checked the new
debug_assert!s are unreachable from user input: route objects and HTML/file routes only produce the 9 methods in ServerConfig.rs:814-824 plus GET/HEAD (server_body.rs:650-653).
Extended reasoning...
The PR rewrites the "/" default-handler registration in src/runtime/server/mod.rs (steps 3-9 of route setup) to register one any-method route instead of one route per uncovered method, and adds debug assertions in src/uws_sys/App.rs, h2.rs and h3.rs for methods without a route function. It touches no auth, crypto or input-parsing surface; the user-reachable change is which handler answers a given method/path. Deferring rather than approving because the change intentionally alters observable routing behaviour (HEAD on static "/" GET routes, H2/H3 any-route ordering) that maintainers should sign off on, and the diff is ~360 lines across the core server routing path; no CODEOWNERS entry covers these files and no third-party objection is outstanding.
set_routes registers the default handler (fetch, or the 404) with
any("/*") whenever no any-method "/*" route exists. Before, a server
with a "/*" route for some methods registered the default handler with
App::method() for each other method, and App::method() registers only 9
of the 36 methods.
The set of covered methods is now one flag: an any-method "/*" route is
registered or it is not.
b9147c1 to
eb10891
Compare
142d9eb to
eaf2275
Compare
Problem
"/*": { GET: fn },PROPFIND /xgets the 404 of a router miss and notfetch. Under"/*": { GET: static, TRACE: fn }theTRACEhandler runs.set_routes(src/runtime/server/mod.rs:2677) registers the default handler per other method withApp::method(), which knows 9 of the 36 methods (src/uws_sys/App.rs:227).HEADof a non-functionGETroute.Fix
set_routesregisters the default handler withany("/*"), unless an any-method"/*"route exists. uWS tries it after the routes of the request's method.App::method()asserts in debug builds that the method has a route function.test/js/bun/http/bun-serve-routes.test.ts, 9 fail without the change. 7 tables run over HTTP/1.1, HTTP/2 and HTTP/3.Background
fetch, or the 404.App::method(). Oneany()does the same.Downsides
HEADon"/*": { GET: <not a function> }comes from the route, notfetch. On HTTP/2 and HTTP/3 an any-method route runs beforefetch.Notes
Answers, before and after. One server with
tls,http2: trueandhttp3: true.fetchanswers every request. Each row gets the same answer over HTTP/1.1, HTTP/2 and HTTP/3 after this PR.routes"/*": { GET: fn }PROPFIND /x,ACL /xfetch"/*": { GET: new Response("s") }HEAD /xfetchfetch"/*": { GET: new Response("s"), TRACE: fn }PROPFIND /x,ACL /xTRACErouteTRACEroutefetch"/*": { GET: fn }, "/any": fn2POST /anyfn2fetchfn2/u/:idany-method routePOSTto itfetchPROPFIND /nopefetch"/*": { GET: new Response(Bun.file(missing)) }, "/any": fn2GET /xfetch"/*": { GET: fn }, "/any": fn2, nofetchPOST /anyfn2fn2HEADon aGETroute on"/*"that is not a function, by kind (HTTP/1.1, before then after):Response:fetch, then the route (itsContent-Length, no body).Responseof aBun.file:fetch, then the route. A file that cannot be opened:fetchboth times, because the route yields.{ dir }:fetch, then the route. A miss is a 404 from the route andfetchdoes not run.fetch, then the route.That is what the same route does on any other path.
apply_static_routeregisters the route's ownHEADwhen the method set hasGET. The old loop then registered the default handler forHEADon"/*"over it.A file route whose file cannot be opened falls back to
fetch(bun-serve-file.test.ts, "handles nonexistent files gracefully"). For aGETfile route on"/*"that worked only while the table had no any-method route.Still different on HTTP/2 and HTTP/3. A route for the request's own method runs before an any-method route, also when the any-method route has the more specific path. With
"/*": { GET: fn }, "/any": fn2,GET /anyrunsfn2on HTTP/1.1 andfnon HTTP/2 and HTTP/3. Only HTTP/1.1 registers an any-method route under each method name. This PR does not change that, and its tests do not assert it. No issue tracks it.Measurements.
operator newcalls in a standalone driver over the realHttpRouter.h) to register 100 any-method routes,"/*": { GET: fn }and the default handler: HTTP/1.1 7,665 -> 7,713. HTTP/2 and HTTP/3 365 -> 325. Without a method-specific"/*"route nothing changes. Once perlistenand perreload.llvm-size): text 80,703,827 -> 80,700,499 bytes (Bun.serve: answer 501 to an unknown method and 404 to a router miss, not a closed socket #45396, then this PR, on base f4d755a; measured while Bun.serve: answer 501 to an unknown method and 404 to a router miss, not a closed socket #45396 still had an HTTP/3 check). data and bss do not change.Not in this PR.
App::method()and the twoRouteKind::from_method()still have no route function for 27 of the 36 methods. No caller passes one of them now. Aroutesmethod object still takes 9 keys.websocket, an upgrade request under"/*": { GET: fn }reachesfetchand not the route.set_routesregisters the"/*"upgrade handler a second time for that table."/*"route that yields, such as"/*": new Response(Bun.file(missing)), leaves no place for the default handler. Such a request gets 404 on each transport (HTTP/1.1 since Bun.serve: answer 501 to an unknown method and 404 to a router miss, not a closed socket #45396), notfetch.set_routes.Suites (debug build with ASAN). On this PR:
bun-serve-routes(88 tests),bun-serve-static,bun-serve-file,serve-directory-routes,bun-serve-html. On the last PR of the stack, which has this change: alsoserve-http2,serve-http2-protocol,serve-http3,node-http, andserve.test.ts.