Conversation
|
Warning Review limit reached
Next review available in: 12 minutes 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 (2)
Comment |
|
Updated 11:14 AM PT - Jul 6th, 2026
❌ @robobun, your commit 821f9b1 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33455That installs a local version of the PR into your bun-33455 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the single-exit wrapper covers every failure path by construction, and the tests exercise each leaking case plus the crash.
Extended reasoning...
Overview
Two files touched. src/router/lib.rs renames the body of Pattern::match_ to a private match_inner and wraps it in a 6-line shim that records params.len() on entry and truncates back to that length whenever the inner matcher returns false. The two ad-hoc params.truncate(0) calls inside the loop are removed as redundant. test/js/bun/util/filesystem_router.test.ts gains three it.each cases asserting the full { name, params } shape for each leaking failure path, plus a subprocess test for the two route trees that previously segfaulted.
Security risks
None. This is pure route-matching bookkeeping over an in-memory scratch Vec; no untrusted input parsing, allocation sizing, or privilege boundaries change. If anything the fix removes a null-deref crash reachable from user-controlled route trees.
Level of scrutiny
Low-to-medium. The Rust change is mechanical: the old body is moved verbatim into match_inner (minus two truncate(0) lines), and the new wrapper is a save-len / call / restore-on-false pattern that is strictly more conservative than what it replaces. The switch from truncate(0) to truncate(params_len) is behaviorally identical because every caller (match_dynamic, the pattern_match unit test) enters with an empty list, and params_len is the semantically correct invariant anyway. match_inner is private so no public surface changes.
Other factors
The PR description includes a clear root-cause trace (which failure paths leaked, why the crash follows) and before/after test output showing all four new tests fail on the unfixed build. Tests follow the file's existing conventions (tempDir, bunExe/bunEnv, concurrent pipe drain, exit-code asserted last) and live in the module's own test file as CLAUDE.md prescribes for non-regressions. No CODEOWNERS entry covers src/router/. The bug-hunting system found nothing.
|
Confirmed both, and added their route trees as test fixtures. Running each issue's reported structure against stock 1.4.0 and against this branch: Both are the duplicate-name form of the leak. In #12206, That also answers the API question in #12206: |
There was a problem hiding this comment.
LGTM — surgical fix that restores the params scratch list on every failure path by construction, with thorough tests covering the reported route trees.
Extended reasoning...
Overview
Two files touched: src/router/lib.rs (~20 net lines) and test/js/bun/util/filesystem_router.test.ts (+110 lines of tests). The Rust change extracts the body of Pattern::match_ into a private match_inner and wraps it with a save-length / truncate-on-false guard, then deletes the two now-redundant truncate(0) calls from the inner body. The old code truncated on only two of six failure exits; the wrapper covers all of them by construction rather than enumeration. Since every call site enters with an empty params list, truncate(params_len) is behaviorally identical to the old truncate(0) on the paths that already had it — and strictly more correct if a future caller ever passes a non-empty list.
Security risks
None. This is filesystem-route pattern matching against a URL path; no auth, crypto, permissions, or untrusted-length arithmetic is touched. The bug produced wrong .params and a null-deref crash; the fix restores the invariant that a failed candidate route leaves the shared scratch list unchanged.
Level of scrutiny
Low-to-moderate. The Rust change is a mechanical refactor into a single-exit wrapper — the kind of shape that is easy to verify by reading (record length → call inner → on false, truncate back). match_inner is private, so no API surface changes. The fix sits at the layer that owns the invariant (the function that pushes into the shared list is responsible for cleaning up on failure). No CODEOWNERS on these paths.
Other factors
Tests are well-structured: an it.each over six fixtures (three failure-path shapes plus the exact repros from #12206 and #15554) asserting the full { name, params } object, and a subprocess test for the two crash-inducing trees so a segfault surfaces as an exit code. The subprocess test follows the file's existing conventions (tempDir, bunExe()/bunEnv, concurrent pipe drain, stdout asserted before exit code). PR description shows before/after test output confirming all new tests fail on the unfixed build. No prior human review comments to address.
…match Pattern::match_ pushes parameters into a scratch list that every candidate route shares, but it only cleared that list on two of its six failure paths. A route that matched part of the path, pushed a parameter and then failed (pattern exhausted with segments remaining, or a catch-all reached with nothing left to consume) left the parameter behind for the next candidate, so the route that eventually matched reported parameters belonging to a route that did not. When the leaked parameter name does not appear in the winning route's name, string_pointer_from_strings resolves it to a zero-length slice, toStringCopy turns that into a null WTF::String, and Identifier::fromString dereferences it: reading .params segfaults. Give match_ a single exit point that truncates the list back to the length it had on entry whenever the match fails.
Both report the same leak: a candidate route is probed, pushes its parameter and fails, and the route that then matches reports that parameter alongside its own. Because the two routes name the parameter the same thing, the two values collapse into an array and .params hands back string[] where the docs promise string.
144e9a9 to
72decbd
Compare
There was a problem hiding this comment.
LGTM — clean structural fix that covers every failure path by construction, with thorough test coverage of both linked issues and the crash case.
Extended reasoning...
Overview
This PR fixes Bun.FileSystemRouter leaking route params from failed candidate matches into the winning match, which manifests as spurious extra params, string[] values where string is expected (#12206, #15554), and a segfault when a leaked param name isn't a substring of the winning route's name. The Rust change in src/router/lib.rs is ~20 lines: Pattern::match_ is refactored into a thin wrapper that records params.len() on entry, delegates to a new match_inner, and truncates back to the entry length on any false return. The two scattered truncate(0) calls in the old body are removed as now-redundant. The test file gains an it.each over six fixtures (three synthetic leak paths plus the exact route trees from #12206 and both variants from #15554) and one subprocess test for the crash case.
Security risks
None. This is pure in-process routing logic over already-validated route names; no auth, crypto, network, or filesystem writes are touched. The fix strictly reduces what reaches the caller (stale params are dropped), so it cannot introduce new exposure.
Level of scrutiny
Low-to-moderate. The change is small, mechanical, and correct by construction: instead of enumerating which of six failure paths need to reset the scratch list, the wrapper resets on every failure. I verified Pattern::match_ has exactly one production caller (Routes::match_dynamic, which always enters with an empty list) plus the in-crate unit test (which truncates before each call), so replacing truncate(0) with truncate(entry_len) is behaviorally identical for existing callers while being more robust. match_inner only ever pushes to params, so truncating back to the entry length exactly restores entry state.
Other factors
- No CODEOWNERS for
src/router/. - The bug hunter found no issues.
- Test coverage is strong: fixtures assert the full
{ name, params }shape viatoEqual(so extra keys fail), and the crash repro runs in a subprocess so a segfault surfaces as an exit code. The new subprocess test'sexpect(stderr).toBe("")matches the pattern already used by several other tests in the same file. - The one CI failure reported by robobun is
test/js/bun/cookie/cookie-map.test.ts, which is entirely unrelated to this change (that file was touched by a separate recent commit on main and this PR doesn't go near cookies). - No outstanding human review comments.
There was a problem hiding this comment.
LGTM — clean single-exit wrapper that restores the params invariant on every failure path, with tests covering both linked issues and the crash.
Extended reasoning...
Overview
Two files changed. In src/router/lib.rs, Pattern::match_ is refactored into a thin wrapper that records params.len() on entry, delegates to a new private match_inner, and truncates params back to the entry length whenever the inner matcher returns false. The two ad-hoc params.truncate(0) calls (which covered only 2 of 6 failure paths) are removed. In test/js/bun/util/filesystem_router.test.ts, an it.each over six route-tree fixtures (three synthetic leak paths plus the exact trees from #12206 and #15554) asserts the full { name, params } shape, and a subprocess test covers the two trees that previously segfaulted so a crash surfaces as an exit code.
Security risks
None. This is pure route-matching bookkeeping over filesystem-derived route names and a request path — no auth, crypto, permissions, or untrusted deserialization is touched. The change strictly narrows what ends up in params (removing leaked entries), and Vec::truncate to a saved length is a no-op when nothing was pushed.
Level of scrutiny
Low-to-medium. The Rust diff is ~15 net lines and mechanical: save length → call original body → restore on false. The old truncate(0) and the new truncate(params_len) are equivalent for the sole production caller (Routes::match_dynamic, which always enters with an empty list), and truncate(params_len) is the semantically correct contract for any future caller. Every existing return path in match_inner is unchanged; only the two now-redundant truncate calls were dropped.
Other factors
The fix lives at the layer that owns the invariant (the function that mutates params), covers the whole bug class by construction rather than by enumerating exit sites, and the PR description demonstrates the new tests fail on the unfixed build and pass on the fixed one. No CODEOWNERS entry covers src/router/, no human reviewer has left outstanding comments, and the bug-hunting system found nothing. The subprocess test's expect(stderr).toBe("") matches the pattern already used by several neighboring tests in the same file.
|
Status for a reviewer: the diff is green, and each red build had a different unrelated cause. This PR's test passes on CI. From the latest build on That build finished 267 passed, 3 failed. All three failures are on darwin and none of them executed a router test:
The two earlier red builds:
I have used my one re-trigger, so I am not going to keep pushing empty commits at this. The change itself is 21 lines in |
1 participant
Fixes #12206
Fixes #15554
Bun.FileSystemRouterreports parameters belonging to routes that did not match, and segfaults on.paramsfor a route tree containing both a[x]and a[...x]route.Repro
On 1.4.0 and on
main:Under a debug build the same input trips the invariant the crash violates:
Cause
Routes::match_dynamicwalks the candidate routes in sort order and hands each one the sameparamsscratch list.Pattern::match_pushes into that list as it consumes segments, but it only truncated the list on two of its six failure paths:The other four leave whatever was pushed in place. The repro hits the one where the pattern runs out while path segments remain:
[user]/settingsconsumeshelpasuser, matchessettings, still has/aleft over, falls out of the loop and returnsfalsewithuser=helpstill in the list.help/[...usertopic]then matches and inherits it. A catch-all reached with nothing left to consume ([a]/b/[...rest]against/x/b) leaks the same way.This is also what #12206 and #15554 report. When the losing route and the winning
route happen to name their parameter the same thing, the two values collapse into
one key and
.paramshands back astring[]where the docs promise astring:/admin/[businessId]/providers/creatematched against/admin/6679fbe17b41431a977163fd/providers/createyields{ businessId: ["6679fbe17b41431a977163fd", "6679fbe17b41431a977163fd"] }.The crash is the downstream consequence.
PathnameScannerresolves each parameter's name against the winning route's name, so a leaked name that the winner does not contain resolves to a zero-lengthStringPointer.toStringCopyturns that into a nullWTF::String, andJSC__JSObject__putRecordpasses it toIdentifier::fromString, which dereferences the nullStringImpl. When the leaked name happens to be a substring of the winner's name (userinside[...usertopic]) there is no crash, just a bogus parameter.Fix
Pattern::match_becomes a single-exit wrapper that recordsparams.len()on entry and truncates back to it whenever the inner matcher returnsfalse. The two scatteredtruncate(0)calls go away, and every failure path is covered by construction rather than by enumeration.This has been there since the original implementation (the Rust port is faithful to the Zig
Pattern.match), so it is not a regression; the test lives in the module's own test file.Verification
test/js/bun/util/filesystem_router.test.tsgains four cases, all of which fail on the unfixed build:{ name, params }shape: one per leaking failure path (pattern exhausted with distinct names, pattern exhausted with the same name, catch-all reached empty), plus the route trees reported in Bun.FileSystemRouter router match unknown behaviour #12206 and both of the ones in Array instead of string inMatchedRoutewhen specific file structure #15554;before / after