bake: report router roots that do not fit the path buffer and skip such route entries instead of panicking - #39196
bake: report router roots that do not fit the path buffer and skip such route entries instead of panicking#39196robobun wants to merge 8 commits into
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughChangesFramework router root resolution
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:46 PM PT - Aug 15th, 2026
✅ @robobun, your commit d697d00d1dd46eccdd999214d19df698ed8369a4 passed in 🧪 To try this PR locally: bunx bun-pr 39196That installs a local version of the PR into your bun-39196 --bun |
|
Status: reproduced on the released binary: an over-long router root aborts through every entry point ( |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the earlier same-class miss (JSFrameworkRouter::constructor) is now fixed and covered by a test, and the comment-cop feedback was addressed.
What was reviewed:
resolve_router_rootusesjoin_abs_string_buf_checkedinto a pooled buffer and copies toBox<[u8]>before the guard drops; theNonearm setshad_errorsso the existing "missing required files" path fires.- All four router-root join sites in
src/runtime/bakenow go through a checked join; the production.rs re-join writes into a pooledPathBufferso a Windows root between 4 KB andMAX_PATH_BYTESthat now survivesresolve()still fits. - Tests spawn subprocesses per entry point (
Bun.serve({app}),routes.dir,bun build --app, internal constructor), drain pipes concurrently, assert the specific error text and exit codes, and pin that a long-as-written root that normalizes down still works.
Extended reasoning...
Overview
Replaces the unchecked resolve_path::join_abs (which normalizes into a fixed 4096-byte thread-local buffer and panics on overflow) with join_abs_string_buf_checked into a pooled PathBuffer at the four sites that resolve a bake router root: both Framework::resolve implementations (via a new shared resolve_router_root helper in bake_body.rs), JSFrameworkRouter::constructor, and the re-join in production.rs. On overflow the framework paths report ENAMETOOLONG and set had_errors (same collection pattern as the sibling resolve_helper), and the JS constructor throws an invalid-arguments error. Six new tests in test/bake/framework-router.test.ts cover every user-reachable entry point plus the normalizing case.
Security risks
None. The change strictly tightens handling of untrusted input: a user-controlled path that previously overflowed a fixed buffer and panicked now fails with a bounded error. No new privilege, filesystem, or network surface.
Level of scrutiny
Moderate. Bake is explicitly experimental (print_warning()), and the behavior change is scoped to over-long router roots that never worked before (they panicked). The fix uses existing in-tree helpers (join_abs_string_buf_checked, path_buffer_pool::get) with the same call shape as the dev-server re-join in DevServer.rs. The pooled-buffer lifetime is correct at each site: the result is copied to a Box<[u8]> (or into the arena) before the PoolGuard drops, and in production.rs the borrowed joined_root is only read on the next line before buf goes out of scope at loop-iteration end.
Other factors
My prior review flagged the missed JSFrameworkRouter::constructor sibling; that is now fixed (1e97fe2) with a dedicated test that throws instead of aborting. The comment-cop notices were addressed by shortening the helper's doc comment to one line (236e4d3). The bun build --app test unsets BUN_JSC_validateExceptionChecks for the child, which mirrors the existing no-validate-exceptions.txt entry for test/bake/dev/production.test.ts (pre-existing unchecked exception in BakeGetDefaultExportFromModule, tracked separately). Tests follow harness conventions: tempDir, bunEnv spread, Buffer.alloc(n, fill).toString(), concurrent pipe draining, describe.concurrent for the subprocess suite.
There was a problem hiding this comment.
I re-reviewed after 1e97fe2/236e4d3/b609492 and found no bugs — the constructor sibling I flagged earlier is now routed through the same checked join, and the comment-cop notes are addressed. Because this touches all four bake router-root resolution paths (dev server, bun build --app, the production re-join, and the internal constructor) and the --app tests need ASAN/exception-check overrides tied to #38949/#38233, a human sign-off would still be worthwhile.
Checked: join_abs_string_buf_checked compares the normalized length against buf.len() (resolve_path.rs:1661), so slicing the pooled buffer to MAX_PATH_BYTES - 1 accepts exactly ≤ MAX_PATH_BYTES - 1 bytes — matches the boundary tests and the read_dir_info limit cited in the description.
Checked: on the None arm both resolve() loops leave fsr.root untouched but set had_errors, so the stale value is never read (fn returns Err(ModuleNotFound)), and entry_client/entry_server still resolve for batch error reporting.
Checked: the production.rs re-join is unchecked but writes into a full pooled PathBuffer; fsr.root is absolute and ≤ MAX_PATH_BYTES - 1 post-resolve(), so the join cannot overflow.
Checked: the pooled PathBuffer guard drops at the end of join_router_root, so no pool leak on either arm.
Extended reasoning...
Overview
This PR replaces a user-reachable panic (range end index N out of range for slice of length 4095) with a recoverable error when a bake fileSystemRouterTypes[n].root resolves to a path that does not fit in a PathBuffer. It introduces join_router_root / resolve_router_root in bake_body.rs (a join_abs_string_buf_checked into MAX_PATH_BYTES - 1 bytes of a pooled buffer) and routes all four call sites through it: both Framework::resolve copies (mod.rs and bake_body.rs), the JSFrameworkRouter::constructor, and the production.rs re-join. ~160 lines of new tests in framework-router.test.ts cover the internal constructor, both Bun.serve shapes, and bun build --app at both sides of the boundary plus a normalizing root.
Security risks
None. This tightens input validation on a user-supplied configuration path — a value that previously crashed the process now surfaces as ENAMETOOLONG and fails the framework load. No new file I/O, no privilege boundaries, no untrusted parsing.
Level of scrutiny
Medium. Bake is experimental but user-facing, and the change threads through four distinct entry points across three platforms with different MAX_PATH_BYTES values (1024 macOS / 4096 Linux / 98302 Windows). The core swap (join_abs → join_abs_string_buf_checked) is mechanical and I verified the checked helper's semantics match the boundary the tests assert. The production.rs re-join now uses a full pooled PathBuffer instead of the 4 KB thread-local one, which on Windows expands the accepted range — a behavioral widening the description calls out.
Other factors
- My earlier inline finding (the
JSFrameworkRouter::constructorsibling) was fixed in 1e97fe2 with a matching test, and both comment-cop notes were addressed in 236e4d3; all threads are resolved. - The
bun build --apptests spreadbunEnvand unsetBUN_JSC_validateExceptionChecks/ appenddetect_leaks=0toASAN_OPTIONS, citing pre-existing #38949/#38233 (same exemptionsproduction.test.tsalready carries). That is a documented workaround rather than a new safety-net removal, but it is the kind of env override a maintainer should sign off on. - Test quality is strong:
describe.concurrent,tempDir, concurrent stdout/stderr/exit drain, exactstdoutassertions, boundary atmaxPathBytes - 1vsmaxPathBytes, and the per-platform limit is asserted in the error text so the test's constant table is checked against the binary. - Given the multi-file surface area and the platform-dependent limit, I am deferring rather than approving so a maintainer can confirm the
MAX_PATH_BYTES - 1threshold choice and the test env overrides.
…buffer instead of panicking Both Framework::resolve implementations joined each user supplied router root with join_abs, which writes into a fixed 4 KB thread-local buffer and panics with "range end index N out of range for slice of length 4095" when the resolved path does not fit. Resolve the roots through one shared helper that uses a checked join into a PathBuffer and reports ENAMETOOLONG through the existing had_errors path, so Bun.serve throws and bun build --app exits with an error. The production build re-joined the resolved root into the same thread-local buffer; join it into a PathBuffer like the dev server does.
…-app child Loading any --app config trips the validator in BakeGetDefaultExportFromModule before the framework is resolved, which is why production.test.ts is excluded from validation; exclude only this child instead of the whole file.
…ng root too Same join into the 4 KB thread-local buffer as the framework router roots; it is a JS constructor, so it throws instead of reporting through had_errors. Also shorten the resolve_router_root doc comment.
…he internal constructor join_router_root resolves into MAX_PATH_BYTES - 1 bytes, the same length from which the resolver's read_dir_info refuses a directory, so every accepted root leaves a byte to spare in the path buffers the dev server and production build re-join it into. The tests now pin that threshold on both sides (one byte below is looked up and skipped like any missing directory, at the limit is reported) for Bun.serve and bun build --app, and report both roots of a two-entry framework.
Every bun build --app leaks its transpilers at exit (#38233), which is what keeps production.test.ts out of LeakSanitizer; the successful build here aborted under it on the ASAN lane.
FrameworkRouter::scan_inner joined every directory and file under an accepted root through the 4 KB thread-local buffer, so a root that passes the new limit but contains an entry whose path does not fit still aborted the scan. Join entries with the same checked join and skip the ones that cannot be opened. The root helper is now resolve_dir_option, strips the trailing separator for every caller, and is what the internal constructor uses directly; the tests read MAX_PATH_BYTES from the harness and add a scan over such a tree.
b609492 to
8cb473b
Compare
robobun
changed the title
There was a problem hiding this comment.
Beyond the inline nit, I also checked whether production.rs:548's unchecked join_abs_string_buf could still overflow after this change — it cannot: fsr.root is absolute and pre-validated to < MAX_PATH_BYTES by resolve_router_root, and the target is a full PathBuffer. Also confirmed that when resolve_router_root returns None, the stale (relative) fsr.root is never read: both resolve() implementations return Err(ModuleNotFound) on had_errors before any consumer sees it.
Extended reasoning...
The one finding this run is a nit-level latent test hang; the inline comment covers it. Recording two adjacent concerns that were examined and ruled out so a later pass does not re-derive them: (1) production.rs still uses an unchecked join, but its input is now guaranteed to fit; (2) the None branch leaves fsr.root un-rewritten, but had_errors short-circuits before use.
A temp dir whose length left exactly one byte before the target produced a zero-length segment, which path.join drops, so the loop never advanced.
There was a problem hiding this comment.
LGTM — both earlier findings (the unchecked JSFrameworkRouter::constructor join and the tree-builder loop's zero-length-segment fixed point) are addressed, and this pass found nothing further.
What was reviewed:
resolve_dir_option/resolve_router_rootand the four call sites: checked join intoMAX_PATH_BYTES - 1, trailing-slash strip, error reported throughOutput::err+had_errorslike the sibling entry-point failures.entry_abs_pathin the scan: verifiedabs_buf_checkedexists and returnsNoneon overflow; both the dir-recurse and file-register arms skip onNoneand the pooled buffer outlives every use ofabs_path.production.rsre-join: now into a pooledPathBuffer, andfsr.rootis already bounded so the uncheckedjoin_abs_string_bufthere cannot overflow.- Tests: boundary at exactly
MAX_PATH_BYTESandMAX_PATH_BYTES - 1for every entry point, the scan-skip case, and the normalizing-root case; theMAX_PATH_BYTESharness constant is asserted against the binary via the error text.
Extended reasoning...
Overview
Converts a user-reachable panic (range end index N out of range for slice of length 4095) in bake's router-root resolution into a reported ENAMETOOLONG error, and makes the route scan skip entries whose absolute paths would overflow the same buffer. Touches four Rust files in src/runtime/bake/ (~70 lines of production code), adds ~190 lines of tests to test/bake/framework-router.test.ts, and adds a MAX_PATH_BYTES constant to test/harness.ts.
Security risks
None. This tightens input validation (bounds-checks a user-provided path before writing into a fixed buffer) rather than loosening any check. No auth, crypto, or trust boundaries are involved.
Level of scrutiny
Medium. Bake is experimental and the change follows an established pattern — replace an unchecked thread-local-buffer join with the existing join_abs_string_buf_checked variant and report on None. The two resolve() implementations, the internal constructor, and the production re-join are the complete set of join_abs/fs.abs call sites on router roots/entries in this module; I grepped for siblings on the first pass and the one I found (the constructor) was fixed.
Other factors
This is my third look at the PR. The first pass flagged the missed constructor sibling (fixed in 1e97fe2) and the second flagged an environment-dependent hang in the test's tree-builder loop (fixed in d697d00 with Math.max(1, …)). Both fixes match what was suggested. Test coverage is thorough: every entry point, both sides of the exact byte limit, the scan-skip case with a real on-disk tree, and a normalizing root that must still be accepted. The bun build --app env overrides (disabling exception-check validation and leak detection) are pre-existing exemptions on production.test.ts with tracking issues named in the comment.
1 participant
Problem
panic: range end index 5010 out of range for slice of length 4095(exit 134).Bun.serve({ routes: { "/*": { dir, style } } })in every release (thedirbecomes a router root and that path has no feature gate), and fromBun.serve({ app: { framework } })andbun build --appon canary or withBUN_FEATURE_FLAG_EXPERIMENTAL_BAKE=1. Thebun:internal-for-testingFrameworkRouterconstructor has the same join on itsrootoption.Framework::resolveimplementations (src/runtime/bake/mod.rs:384for the dev server,src/runtime/bake/bake_body.rs:686forbun build --app) andJSFrameworkRouter::constructor(src/runtime/bake/FrameworkRouter.rs:1800) resolve the root withresolve_path::join_abs, which normalizes into a fixed 4096 byte thread-local buffer (src/paths/resolve_path.rs:15) and indexes past its end when the result does not fit. The root is user input of arbitrary length.FrameworkRouter::scan_innerjoined every directory and file under the root withFileSystem::abs(the same thread-local buffer), so a root that is itself acceptable but contains an entry whose full path does not fit aborted during the scan (panic: range end index 4151 out of range for slice of length 4095). Such entries exist: the OS limits the path you pass to a syscall, not how deep a tree can get.src/runtime/bake/production.rs:548re-joined the resolved root into the same thread-local buffer.Fix
bake_body::resolve_dir_option: ajoin_abs_string_buf_checkedintoMAX_PATH_BYTES - 1bytes of a pooledPathBuffer, with the trailing separator stripped (previously only the constructor did that).MAX_PATH_BYTES - 1is the limitResolver::read_dir_infoalready applies to a directory path (src/resolver/resolver.rs:4191), so a root is rejected exactly when no directory lookup could succeed for it. bake: resolve app.root against the cwd and require it to be a string #39188 resolvesapp.rootand can use the same helper.resolve()implementations call it throughresolve_router_root, which printsENAMETOOLONG: Failed to resolve 'fileSystemRouterTypes[n].root' for framework: the resolved path must be shorter than N bytesand setshad_errors, the same path an unresolvable entry point takes, soBun.servethrows its existing "Framework is missing required files!" error,bun build --appexits 1 with its existing summary line, and every bad root is still reported in one pass. The internal constructor throws an invalid-arguments error instead, since it is a JS constructor rather than a batch resolver.entry_abs_path) and skips entries that do not fit, both for recursing into a directory and for registering a file. Skipping is the right outcome there: nothing could open such an entry, andread_dir_inforeturnsNonefor the directory case already. Everything else under the root is still served.MAX_PATH_BYTES - 1, the re-joins into a fullPathBufferin dev server init (DevServer.rs, unchanged) and the production build (now a pooledPathBufferinstead of the 4 KB thread-local buffer, which is smaller than aPathBufferon Windows) always fit.routes/../routes/...) is still accepted; the old code accepted it too and a test pins that.test/bake/framework-router.test.ts. On the released binary the constructor test fails and the scan test aborts with the second panic above; with the scan test removed, the twoBun.servetests abort with the first panic and the at-limit tests are wrongly accepted. With this change all pass: the internal constructor at exactlyMAX_PATH_BYTES, a scan of a root 200 bytes below the limit holdingindex.tsplus a file and a directory with maximum-length names (onlyindex.tsis discovered; skipped on Windows, whose own path limit is belowMAX_PATH_BYTES), a two-entry framework reporting[0].rootand[1].root, aroutesdir, andBun.serveplusbun build --appwith roots of exactlyMAX_PATH_BYTES(reported) andMAX_PATH_BYTES - 1(looked up and skipped like any missing directory, so the re-joins run on a maximum-length root), plus the normalizing root. The per-platform limit comes from a newMAX_PATH_BYTESexport intest/harness.ts(the same lines pack: report an error instead of panicking when --destination does not fit the path buffer #38749 adds) and is asserted in the error text, so the table is checked against the binary.bun build --appchildren are spawned withoutBUN_JSC_validateExceptionChecksand with leak detection off: loading any--appconfig trips a pre-existing unchecked exception inBakeGetDefaultExportFromModulebefore the framework is resolved, and every successful--appbuild leaks its transpilers at exit. These are the reasonstest/bake/dev/production.test.tsis exempt from both checks; bake: check for exceptions in the production build's module helpers #38949 and bake: drop the production build's transpilers and framework projection #38233 fix them, after which the override can go. The tests stay in this file rather thanproduction.test.tsbecause that file's existing tests run close to the default per-test timeout on a debug build.bun bd test test/bake/dev/bundle.test.tsstill passes (real router roots and trees through the dev server scan); a root written asroutes/is still served.Background
rootdirectory that bake scans for route files. Bake keeps two copies of theFrameworkstruct (mod.rsfor the dev server,bake_body.rsforbun build --app), each with its ownresolve()that turns the user's root into an absolute path; that is why the fix touches two call sites through one helper.FrameworkRouter::scan_inneris the walk over that directory that both of them, and the internal constructor, run afterwards.resolve_path::join_abs(andFileSystem::abs, which wraps it) is bun'spath.resolve: it joins against a base directory and normalizes into a caller-independent 4096 byte thread-local scratch buffer.join_abs_string_buf_checkedis the variant for input of unknown length: it joins into a caller-provided buffer and returnsNonewhen the normalized result would not fit.PathBufferis bun's[u8; MAX_PATH_BYTES]path scratch type (4096 bytes on Linux, 1024 on macOS, 98302 on Windows), drawn from a per-thread pool so it does not live on the stack. A path ofMAX_PATH_BYTESbytes leaves no room for the NUL terminator, which is why the resolver and this change both treat that length as already too long.pagesroot that a project may not have, soread_dir_info_ignore_errorreturningNoneskips the router type. That is the behavior this change deliberately does not extend to over-long roots, and the behavior it does extend to over-long entries inside a root.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bake/framework-router.test.ts