fix: relative path resolution for the assets directory#76
Open
yuriiShmal wants to merge 6 commits into
Open
Conversation
WalkthroughThe static plugin now computes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/index.test.ts (1)
488-503: Mark this test with.serial()to future-proof against concurrent execution. ♡Bun runs tests sequentially by default, so the try/finally cleanup handles cwd restoration just fine~ But if someone runs tests with
--concurrentflag, this test will flake without explicitly marking it as.serial(). Your cleanup is solid, but addit.serial(...)to make the intent clear and prevent accidents when the runner configuration changes~ (＾▽＾)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 488 - 503, Update the test named "serves files when cwd differs from project root" to be run serially to avoid flakiness under concurrent test runs: change the test declaration from it(...) to it.serial(...) so the try/finally cwd swap (process.chdir(originalCwd)) cannot race with other tests; locate the test by its description string and adjust the test invocation accordingly (the rest of the test body, including uses of process.chdir, Elysia, staticPlugin, and req('/public/takodachi.png'), should remain unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/index.test.ts`:
- Around line 488-503: Update the test named "serves files when cwd differs from
project root" to be run serially to avoid flakiness under concurrent test runs:
change the test declaration from it(...) to it.serial(...) so the try/finally
cwd swap (process.chdir(originalCwd)) cannot race with other tests; locate the
test by its description string and adjust the test invocation accordingly (the
rest of the test body, including uses of process.chdir, Elysia, staticPlugin,
and req('/public/takodachi.png'), should remain unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a866fe0-bd91-4792-9699-3c3df17e7267
📒 Files selected for processing (3)
src/index.tssrc/utils.tstest/index.test.ts
Contributor
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
342-349:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBlock
..escapes before touching the filesystem, baka~ (￢‿￢ )♡This still lets a request like
/public/../package.jsonresolve outsideassetsDirin dynamic mode, so arbitrary files can be served. Resolve the candidate path first, then reject anything whose relative path escapes the assets root.♨️ Tight fix
- const pathName = normalizePath( - path.join( - assetsDir, - decodeURI - ? (fastDecodeURI(params['*']) ?? params['*']) - : params['*'] - ) - ) + const requestedPath = decodeURI + ? (fastDecodeURI(params['*']) ?? params['*']) + : params['*'] + const resolvedPath = path.resolve(assetsDir, requestedPath) + const relativeToAssets = path.relative(assetsDir, resolvedPath) + + if ( + relativeToAssets.startsWith('..') || + path.isAbsolute(relativeToAssets) + ) + throw new NotFoundError() + + const pathName = normalizePath(resolvedPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 342 - 349, The current construct builds pathName before resolving and allows `/public/../package.json` to escape assetsDir; change the logic in the block around normalizePath/path.join so you first compute the candidate string (using the existing decodeURI ? fastDecodeURI(params['*']) ?? params['*'] : params['*']), then call path.resolve(assetsDir, candidate) to get an absoluteCandidate, then compute path.relative(assetsDir, absoluteCandidate) and reject/return a 403 if that relative path starts with '..' or is equal to '..' (i.e., escapes the root); only after that safe-check call normalizePath on absoluteCandidate and continue—this involves the same symbols: assetsDir, params['*'], fastDecodeURI, path.resolve, path.relative, normalizePath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/index.test.ts`:
- Around line 540-542: The test hard-codes "/tmp" which is OS-dependent and may
accidentally contain a package.json; change the spec to create an isolated
temporary directory (e.g., using fs.mkdtemp or fs.promises.mkdtemp with
os.tmpdir()), build a path like path.join(tmpDir, 'index.ts') (optionally create
the file), call findProjectRoot(tmpIndexPath) and assert null, then remove the
temp directory; update the test case around findProjectRoot to use the temporary
directory helper instead of "/tmp/no-such-project/index.ts".
- Around line 505-507: The test "serves files when assets is an absolute path"
constructs absoluteAssets using process.cwd(), making the test cwd-dependent;
change the resolution to use the test file directory (e.g., import.meta.dir or
__dirname) instead of process.cwd() so absoluteAssets is stable across
environments—update the path.resolve call that assigns absoluteAssets (used when
creating new Elysia().use(staticPlugin({ assets: absoluteAssets }))) to base off
import.meta.dir/__dirname.
---
Outside diff comments:
In `@src/index.ts`:
- Around line 342-349: The current construct builds pathName before resolving
and allows `/public/../package.json` to escape assetsDir; change the logic in
the block around normalizePath/path.join so you first compute the candidate
string (using the existing decodeURI ? fastDecodeURI(params['*']) ?? params['*']
: params['*']), then call path.resolve(assetsDir, candidate) to get an
absoluteCandidate, then compute path.relative(assetsDir, absoluteCandidate) and
reject/return a 403 if that relative path starts with '..' or is equal to '..'
(i.e., escapes the root); only after that safe-check call normalizePath on
absoluteCandidate and continue—this involves the same symbols: assetsDir,
params['*'], fastDecodeURI, path.resolve, path.relative, normalizePath.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd2c11d3-f289-4d1e-bfb0-e895583c9509
📒 Files selected for processing (3)
src/index.tssrc/utils.tstest/index.test.ts
Comment on lines
+505
to
+507
| it('serves files when assets is an absolute path', async () => { | ||
| const absoluteAssets = path.resolve(process.cwd(), 'public') | ||
| const app = new Elysia().use(staticPlugin({ assets: absoluteAssets })) |
There was a problem hiding this comment.
Use the fixture location, not process.cwd(), dummy~ (￣^￣)ゞ
This “absolute path” regression still derives the path from the current working directory, so it can fail before it even exercises the behavior under test. Resolve from import.meta.dir / __dirname instead so the test stays invariant to cwd.
♻️ Small cleanup
- const absoluteAssets = path.resolve(process.cwd(), 'public')
+ const absoluteAssets = path.resolve(__dirname, '../public')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('serves files when assets is an absolute path', async () => { | |
| const absoluteAssets = path.resolve(process.cwd(), 'public') | |
| const app = new Elysia().use(staticPlugin({ assets: absoluteAssets })) | |
| it('serves files when assets is an absolute path', async () => { | |
| const absoluteAssets = path.resolve(__dirname, '../public') | |
| const app = new Elysia().use(staticPlugin({ assets: absoluteAssets })) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/index.test.ts` around lines 505 - 507, The test "serves files when
assets is an absolute path" constructs absoluteAssets using process.cwd(),
making the test cwd-dependent; change the resolution to use the test file
directory (e.g., import.meta.dir or __dirname) instead of process.cwd() so
absoluteAssets is stable across environments—update the path.resolve call that
assigns absoluteAssets (used when creating new Elysia().use(staticPlugin({
assets: absoluteAssets }))) to base off import.meta.dir/__dirname.
Comment on lines
+540
to
+542
| it('returns null when no package.json is found above the entrypoint', async () => { | ||
| const result = await findProjectRoot('/tmp/no-such-project/index.ts') | ||
| expect(result).toBeNull() |
There was a problem hiding this comment.
Don’t hard-code /tmp for the “no project root” case, brat~ (◕‿◕✿)♡
This assumes a POSIX-ish host and also assumes no ancestor like /tmp/package.json exists. That makes the test environment-dependent. A fresh temp directory created during the test would keep this deterministic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/index.test.ts` around lines 540 - 542, The test hard-codes "/tmp" which
is OS-dependent and may accidentally contain a package.json; change the spec to
create an isolated temporary directory (e.g., using fs.mkdtemp or
fs.promises.mkdtemp with os.tmpdir()), build a path like path.join(tmpDir,
'index.ts') (optionally create the file), call findProjectRoot(tmpIndexPath) and
assert null, then remove the temp directory; update the test case around
findProjectRoot to use the temporary directory helper instead of
"/tmp/no-such-project/index.ts".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Attempts to resolve #58.
Main issue
Relative assets paths were previously resolved from
process.cwd().That worked when the server was started from the project root, but failed when the same entrypoint was started from another working directory, for example:
In that case, assets:
publicwas resolved as../publicinstead ofproject/public, causing static asset requests to fail withENOENT.Changes
assetsDirconsistently across:package.jsonis found.package.jsoncan be found.process.cwd()when Bun entrypoint metadata is unavailable.shouldIgnoreso ignore string patterns continue to work after asset paths are resolved to absolute paths.Additional test coverage added by JoshuaNam
Added regression coverage for relative assets path resolution:
Added unit coverage for findProjectRoot:
package.jsonfrom a deep entrypointpackage.jsonfrom a flat-layout entrypoint at the project rootpackage.jsonfrom a deeper app-style layoutpackage.jsonexists above the entrypointAdded ignore-pattern regression coverage:
Notes
Notes on project-root resolution
The fix now prefers the nearest package.json above the Bun entrypoint, which is more robust than assuming a fixed directory depth from bun.main.
This avoids resolving assets relative to the wrong cwd while still supporting different project layouts, including flat entrypoints and deeper src/app structures.
Notes on
shouldIgnoreThe
shouldIgnoreupdate is required becauseassetsDirmay now be absolute internally. Without adjusting the ignore matching behavior, existing string ignore patterns could stop matching user-provided relative-style patterns.Testing
bun test
Manual verification
I also manually tested the startup path behavior by temporarily adding:
at the end of
src/index.ts, then starting the server from different current working directories.With the current changes, relative assets paths no longer produced
ENOENTwhen the entrypoint was launched from outside the project root.Compatibility
This preserves the existing public API:
No user configuration changes should be required.
Summary by CodeRabbit
Bug Fixes
Tests