Repository navigation
Conversation
|
Status Reproduced on bun 1.4.3-canary.1+367d939d9 (Linux x64): CI Build 134902 passed. The The next step is a decision by a maintainer on the route change (the PR description has the choices), then a review. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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 the new base-path loop in src/js/internal/html.ts for termination and the single-entry path: base shrinks one directory per iteration and startsWith("") is always true, so it cannot spin, and on POSIX every resolved page starts with / so base never drops below /; a single entry still gets servePaths[0] = "*" afterwards. The docs/bundler/html-static.mdx example output still matches the new behaviour, there is no ahead-of-time/ copy of internal/html.ts to keep in sync, and the diff only adds tests without altering existing ones.
Extended reasoning...
The change replaces the character-wise longest-common-prefix base path in the bun ./a.html ./b.html dev server with a directory-boundary base, plus a docs sentence and new route tests; no security-sensitive surface is touched. Inline findings already cover the unannounced route changes for prefix-sharing page names, the untested Windows two-root cases, and the serial test spawns, so a human decision on the breaking-change policy is still needed before merge.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/js/internal/html.ts— Users whose page names all begin with:get one page silently unreachable after merging, where the base branch served both. With the base trimmed at a directory boundary at src/js/internal/html.ts:117-122,./:1.html ./:2.htmlbecome routes/:1and/:2at html.ts:170, and Bun.serve treats both as a one-segment parameter route, so whichever registers first answers every single-segment request. On the base branch the shared:was cut and the routes were/1and/2. Fix: percent-encode or otherwise escape route-syntax characters (:at segment start,*) in routeKey or before html.ts:170, or reject such names with a message, so a filename can never become a parameter or wildcard route.Why this was flagged
Trigger: any page set where every basename starts with
:(or whose common directory makes a:-leading segment the first segment), e.g.bun ./:1.html ./:2.htmlon POSIX. Entry: start() in src/js/internal/html.ts. html.ts:130-146 yields servePath:1and:2; html.ts:170 registersroutes["/:1"]androutes["/:2"], both HTMLBundle values. ServerConfig.rs:756 and :789 only run validate_route_name for callable routes, so the HTML bundles bypass the parameter check and are handed to the router as two parameter routes at the same depth; one shadows the other and that page answers 404 or the wrong page everywhere. The base branch's character prefix cut the shared:so it served/1and/2(the PR table confirms this row). The dismissing finder called it too_minor because:names are invalid on Windows; on POSIX and macOS they are legal and the regression is a page the user passed on the command line disappearing without any message. Remedy: encode:and*at segment start in routeKey (serve_cli.ts:137) so routes are literal, covering the UNC and*siblings too.Verification: Trigger: a page set whose basenames all begin with
:. src/js/internal/html.ts:117-122 setsbaseto the directory, so line 170 registers/:1and/:2;routeKey(src/js/internal/serve_cli.ts:134-138) does not encode:. On the base branch routes were/1and/2, both reachable. uWS treats a:-leading segment as a parameter, so one page is unreachable with no error. -
🟣
src/js/internal/html.ts— Windows users with a UNC page beside a drive-letter page get a route whose key begins with//, which Bun.serve registers but browsers and the banner read as a host, so the page is unreachable from the printed link. When no directory contains every page, html.ts:116-122 leavesbaseempty, so//nas/shared/legal.htmlkeeps its whole path at html.ts:130. The strip at html.ts:144-145 removes exactly one leading/, and html.ts:170 prepends one again, giving//nas/shared/legal. Fix: whenbaseis empty, strip every leading/(and the drive or UNC root) from each page before building the route, or fall back to a per-root base so UNC and drive pages get rooted routes.Why this was flagged
Trigger: on win32, pages whose resolved paths share no directory, e.g.
C:\proj\index.htmland\\nas\shared\legal.html, through the HTML entry CLI. html.ts:113 normalizes to//nas/shared/legal.html; the loop at html.ts:118-122 reducesbaseto the empty string; html.ts:130 leaves the whole path; html.ts:136-138 strips.html; html.ts:144-145 strips only one of the two leading slashes; html.ts:170 calls routeKey("/" + "/nas/shared/legal") so the route key is//nas/shared/legal. A relative resolution of that key against the server URL in the banner yields hostnas, and a browser link//nas/shared/legalgoes to another host, so the user cannot reach the page from what Bun prints. The base branch's result depended on sort order and for some orderings gave the drive-letter pages short usable routes, which the new code never does. Windows-only, but the new code path at html.ts:116 explicitly designs for the empty-base case and leaves this consequence in place.Verification: pre-existing. Trigger: win32, pages whose resolved paths share no root (a UNC page such as
\\nas\shared\legal.htmlbesideC:\proj\index.html). In /home/claude/bun/src/js/internal/html.ts the loop at 118-122 popsbasedown to""; 144-145 removes exactly one slash; line 170 prepends one, so the key is//nas/shared/legal. The base branch produces the identical key by the identical route.
… them all The base path was the longest common prefix of the page paths in characters, so it could end in the middle of a name: pub/a.html and pubx/b.html were served as /a and /x/b. The base path is now the deepest directory that contains every page.
88d560f to
bcd5df2
Compare
|
Replies to the two findings of the review that are outside the diff lines. Page names that all start with A UNC page beside a page on a drive. The key |
Stacked on #47279.
@Jarred-Sumner: this moves routes that answer on 1.4.3, with no flag (as #38732, which #46021 took out). Notes has the choices.
Problem
bun ./index.html ./info.htmlserves/and/fo.bun createReact template runsbun './**/*.html':dashboard.htmland the builtdist/dashboard.htmlget/ashboardand/ist/dashboard.longestCommonPath,src/js/internal/html.ts:121) is the longest common prefix of the page paths in characters, so it can end inside a name.Fix
path.relativebranch, the empty-remainder branch, and theprocess.cwd()push and pop.test/js/bun/http/bun-serve-html-entry.test.ts, 6 of 8 new tests that run on Linux fail on the base branch. Self-reviewed: 32 concerns raised, 29 addressed, 3 rejected (Notes).Background
.htmland a lastindex./: it keeps the wrong loop. The nativebun_pathshelper has the defect too:bun build ./pub/a.html ./pubx/b.htmlwrites_.._/pubx/b.html.Downsides
page-home.html+page-about.htmlserved/homeand/about, and now serve/page-homeand/page-about. No user reported the cut.:1.html+:2.htmlserved/1and/2. Now/:1and/:2are one parameter route forBun.serve().Notes
The decision
landing-prs.mdcalls a changed default a breaking change, and Hardening: fix crashes across the runtime, bundler, dev server, shell and CLI #46021 kept what 1.4.3 does ("What worked on 1.4.3 still works"). The choices: land it as it is, land it with a release that announces it, or print anote:at start for each route that differs from the one 1.4.3 gives.note:is not in this PR. To name the old route, each start must compute the old prefix again, which is the loop over every page that this PR removes./that they share. In a replay of 300,000 random page sets, 0 of the 285,781 sets that main names right change a route.page-home.html+page-about.htmlserve/homeand/about. Addindex.html, and main serves/page-homeand/page-about.é1.html+é2.htmlneed the percent-encoded key.Behaviour changes (Linux x64, main is bun 1.4.3-canary.1+367d939d9, whose route block is the one of main)
bun ./index.html ./info.html/,/fo/,/infobun ./home.html ./help.html/ome,/elp/home,/helpbun ./pub/a.html ./pubx/b.html/a,/x/b/pub/a,/pubx/bbun ./pub.html ./pub/a.html/,/a/pub,/pub/abun ./x/a.html ./xa.html/afor both,x/a.htmlhas no URL/x/a,/xabun './**/*.html'withdashboard.html,dist/dashboard.html/ashboard,/ist/dashboard/dashboard,/dist/dashboardbun ./page-home.html ./page-about.html/home,/about/page-home,/page-aboutbun ./😀.html ./😁.htmlTypeError: Invalid route "/�"/😀,/😁bun ./:1.html ./:2.html/1,/2/:1,/:2, one parameter routebun ./site/a.html ./site/b.html/a,/bbun ./index.html/*The
dashboardrow is the tree thatbun create ./dashboard.tsxand itsbuildscript leave (src/runtime/cli/create/projects/react-spa/package.json). I made the two files by hand and did not runbun create.No directory in common (Windows). The test
bun with pages on two drives(substDrive, three pages) runs on Windows only. It passes on the two Windows lanes of CI (2019 x64, 11 aarch64). The cases below are simulated withpath.win32on Linux.C:\a\x.html,D:\b\y.html,D:\b\z.html: main gives/C:/a/x,/D:/b/yand/(forz.html). Now/C:/a/x,/D:/b/y,/D:/b/z.C:\proj\index.html,about.html,pricing.htmland\\nas\shared\legal.html: main gives/,/about,/pricing,//nas/shared/legal. Now/C:/proj,/C:/proj/about,/C:/proj/pricing,//nas/shared/legal.if (!acc) return curr).url:line names a UNC page, it is wrong://nas/...is read as a host. Main prints/nas/...there, which is not a route either.Not fixed here
bun build ./pub/a.html ./pubx/b.html --outdir outwritesout/a.htmlandout/_.._/pubx/b.html, on 1.4.3 and on this branch. That is the native helper, which this PR does not use. Bun.build, bun build: use the shared directory of 9 or more entry points as the default root #38732 changes its case of 9 or more entry points.docs.htmlbesidedocs/index.html: both get/docs, and one replaces the other with no message.:or*is a parameter or a wildcard route, on main too when the name is not cut. Related: Bun.serve: register the files of an HTML import as literal routes #46009./about.html,/blog/and/blog/index.htmlanswer 404. The routes are/aboutand/blog.bun run build, itsdevglob also finds the built page. Then there are two pages, and/answers 404../is resolved as a module specifier first (html.ts:87). The new tests write./.Measurements (Linux x64, this branch against its base #47279)
html.ts, built from the source text withnew Function, page imports as plain objects, on bun 1.4.3.bun:jscheapStats().objectTypeCounts, 3 runs, identical): 1 page 36 to 31. 2 pages 47 to 45. 100 pages 736 to 633.node:pathbuiltins wrapped, pages in one directory): 1 page 18 host and 4 closure calls to 13 and 3. 2 pages 23 and 9 to 22 and 8. 100 pages 905 and 499 to 806 and 400. 1,000 pages 9,005 and 4,999 to 8,006 and 4,000.perf_event_opencounter, 5 runs,perfis not installed): 1 page 868,663..869,295 to 669,229..669,684. 2 pages 1,070,290..1,070,744 to 862,097..863,855. 100 pages 5,722,221..9,549,736 to 2,196,848..2,587,960.getcwdcalls until the banner (anLD_PRELOADcounter,straceis not installed): 2 with one page and 2 with two pages, before and after.process.cwd()is cached.internal/html.js5,152 to 4,724 bytes.InternalModuleRegistryConstants.bin2,752,734 to 2,752,306.Replay against a whole-segment oracle (segment arrays, its own percent-encoding)
Verification
bun bd test test/js/bun/http/bun-serve-html-entry.test.ts: 17 pass, 1 skipped (the Windows test). Withsrc/of the base branch: 6 fail (./pub/a.html ./pubx/b.html, nine pages inpub/x/andpubx/,./pub.html ./pub/a.html,./index.html ./info.html, thedashboardtree,./😀.html ./😁.html).bun test --debug-aoton the test file: 17 pass.Self-review
The review read #47279 and this change as one diff and asked for two PRs. Rejected:
:: the route is a parameter route on main too when the name is not cut. The case is in Downsides.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/http/bun-serve-html-entry.test.ts