Skip to content

FileSystemRouter: fix a panic and wrong route names for an absolute dir with '..' in it - #44342

Merged
dylan-conway merged 3 commits into
mainfrom
claude/router-absolute-dir
Oct 1, 2026
Merged

dylan-conway merged 3 commits into
mainfrom
claude/router-absolute-dir

Conversation

@dylan-conway

@dylan-conway dylan-conway commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

What does this PR do?

Fixes panic: range start index 123 out of range for slice of length 119 in new Bun.FileSystemRouter() (RouteLoader::load, Sentry BUN-564Q and BUN-54X5, 7 reports on 1.4.2), and the wrong route names the same bug gives when it does not panic.

Trigger

An absolute dir with .. in it, which is what import.meta.dir + "/../pages" is:

new Bun.FileSystemRouter({ dir: "/app/other/../pages", style: "nextjs" });
file route on canary 7fe13e1b9 expected
pages/index.tsx / /
pages/api/x.tsx panic /api/x
pages/a-long-directory-name/deeper/page.tsx /ng-directory-name/deeper/page /a-long-directory-name/deeper/page

Cause

A route is named by what follows the router's directory in the directory of its file: &entry_dir[base_dir.len() - 1..]. The router enters each subdirectory by a normalized path, so that is how the directory of a nested file is spelled.

The constructor normalized a relative dir, by joining it to the working directory. An absolute dir was used as written, with only repeated separators collapsed. With .. in it, it is longer than the normalized path, so for a nested file the slice starts too far in: inside a directory name, or past the end when the path below the router's directory is shorter than what .. folds away.

Fix

Both kinds go through the same join, which takes an absolute part as it is and normalizes it. It is the length-checked one, since the string comes from the user. A dir too long for the buffer gets the constructor's Unable to find directory error.

How did you verify your code works?

A new test in filesystem_router.test.ts that compares the routes of both spellings of one directory, with the three files above.

the new test
1.4.2 fails: the child ends with 134 and panic: range start index 35 out of range for slice of length 31
this PR, debug build passes

filesystem_router.test.ts: 36 pass on the debug build.

Relative paths with .., . or doubled separators, absolute paths with doubled separators, and symlinked directories already gave the right routes on canary.

.. after a symlink is folded as text, as it already was for a relative dir. It did not work for an absolute one before. With /app/link a symlink to /srv/x, routes in /srv/pages, and dir: "/app/link/../pages":

canary 7fe13e1b9 this PR the same path written relative, on both
no /app/pages throws ENOENT opening route: /app/pages/index.tsx throws Unable to find directory: /app/pages throws Unable to find directory: /app/pages
an unrelated /app/pages exists throws the same ENOENT the routes of /app/pages the routes of /app/pages

@robobun

robobun commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator
Updated 6:28 PM PT - Sep 30th, 2026

🔄 @dylan-conway, the build for b1f2881b was cancelled — 482bfa10 is building instead in Build #122123. Stay tuned...

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 659bc57b-62b8-4eb7-a68e-ce8ed14f9b54

📥 Commits

Reviewing files that changed from the base of the PR and between b1f2881 and 482bfa1.

📒 Files selected for processing (1)
  • test/js/bun/util/filesystem_router.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.


Walkthrough

The filesystem router constructor now joins absolute and relative directory values against the filesystem top-level directory. A subprocess test compares routes from an absolute path containing .. with routes from its normalized spelling.

Changes

Filesystem router path resolution

Layer / File(s) Summary
Directory path resolution and route validation
src/runtime/api/filesystem_router.rs, test/js/bun/util/filesystem_router.test.ts
The constructor uses checked joining for directory values and reports an “Unable to find directory” error if joining fails. A subprocess test compares routes from equivalent absolute paths.

Suggested reviewers: jarred-sumner

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 482bf

No actionable merge-blocking risk is identified for the path-normalization change. The regression test exercises the intended path spelling; normal build and test checks should complete before merging.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the FileSystemRouter bug, including the panic and incorrect route names caused by an absolute directory containing '..'.
Description check ✅ Passed The description includes both required sections. It clearly explains the bug, cause, fix, trigger, and verification results with relevant test details.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/runtime/api/filesystem_router.rs Resolved
@dylan-conway
dylan-conway merged commit 95690fc into main Oct 1, 2026
9 of 10 checks passed
@dylan-conway
dylan-conway deleted the claude/router-absolute-dir branch October 1, 2026 01:32

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

robobun added a commit that referenced this pull request Oct 30, 2026
…tem does (parked)

Not proposed for merge. This is the rest of the change that #47454 took
Bun.mmap from. It moves a relative Bun.Glob `cwd` and the FileSystemRouter
`dir` from the text reading of `<symlink>/..` to the system's reading, and
makes absolute glob results behind a `..` name the directory that was
listed.

It waits for a decision. #44342 records the text fold for the router's
`dir`, and #45800 pins the same fold for `--cwd` with tests.

Review items that are still open in this commit:
- make the walker check opt-in for Bun.Glob, or own the `bun lint` change
- return the error of the check and do not fall back to the folded text
- do not name the directory with libc realpath
- move the router check into load_router, so that reload() repeats it
- take `\` as a separator at the router, as its join does
- add rows for a literal pattern and for a folded text that does not exist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants