fix: resolve wildcard path against assetsDir before read#66
Conversation
WalkthroughThe request handler's filesystem target resolution was refactored to use absolute path resolution against Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 302-310: Remove the unnecessary undefined-guard around params['*']
(Elysia guarantees it's a string) and, to prevent symlink escapes, resolve
symlinks before the boundary check: compute and store a realpath for assetsDir
at plugin init (e.g., assetsRealPath) and replace the lexical resolve of joined
with fs.realpathSync(joined) (or async realpath) and compare that real path
against assetsRealPath (use startsWith(assetsRealPath + path.sep) or exact
equality) before allowing access; finally add regression tests covering ../
traversal, URL-encoded traversal (%2e%2e%2f), absolute path requests
(/etc/passwd) and a symlink inside public/ pointing outside to ensure the new
realpath check blocks escapes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const userPart = decodeURI | ||
| ? (fastDecodeURI(params['*']) ?? params['*']) | ||
| : params['*'] | ||
|
|
||
| const joined = path.resolve(assetsDir, userPart) | ||
| if (joined !== assetsDir && !joined.startsWith(assetsDir + path.sep)) | ||
| throw new NotFoundError() | ||
|
|
||
| const pathName = normalizePath(joined) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Elysia router wildcard params['*'] value for empty match undefined or empty string
💡 Result:
In ElysiaJS, for a wildcard route like /path/, when the request exactly matches the prefix /path (empty match after the prefix), the wildcard parameter params[''] is an empty string '', not undefined. Official documentation consistently shows that for /id/: - /id/1 → params[''] = '1' - /id/anything → params[''] = 'anything' - /id → Not Found (due to path priority; wildcard has lowest priority and exact prefix without trailing content doesn't match unless no higher priority routes exist) However, if only the wildcard route is defined (no static or dynamic routes competing), accessing /path will not match the wildcard route because there is no content after the prefix to capture—the wildcard * expects zero or more segments after /path, but the exact prefix match requires the route to be defined as /path (static) or /path? pattern if needed. Sources explicitly state /id as "Not Found" for /id/ route. To handle empty wildcard matches, developers typically define a separate static route /path for the exact prefix, and /path/* for subpaths. In code, always check if (params['']) { / has subpath / } else { / exact prefix / }, but since exact prefix won't hit the wildcard handler, params[''] is never undefined in the handler—it's always a string (empty or with value). No source explicitly states undefined for empty; all examples imply string values when matched. Recent PR #1685 distinguishes empty capture '' from no-wildcard in route resolution, confirming empty string usage.
Citations:
- 1: https://elysiajs.com/essential/route
- 2: https://elysiajs.com/essential/path
- 3: fix: route specificity for wildcard vs mounted routes (#1682, #1515) elysia#1685
- 4: https://elysiajs.com/tutorial/getting-started/your-first-route/
🏁 Script executed:
cd / && find . -name "*.test.ts" -o -name "*.spec.ts" 2>/dev/null | grep -i index | head -5Repository: elysiajs/elysia-static
Length of output: 434
🏁 Script executed:
git ls-files src/ | head -20Repository: elysiajs/elysia-static
Length of output: 104
🏁 Script executed:
wc -l src/index.tsRepository: elysiajs/elysia-static
Length of output: 82
🏁 Script executed:
sed -n '295,320p' src/index.tsRepository: elysiajs/elysia-static
Length of output: 1016
🏁 Script executed:
sed -n '55,65p' src/index.tsRepository: elysiajs/elysia-static
Length of output: 435
🏁 Script executed:
sed -n '1,30p' src/index.tsRepository: elysiajs/elysia-static
Length of output: 811
🏁 Script executed:
sed -n '310,330p' src/index.tsRepository: elysiajs/elysia-static
Length of output: 869
🏁 Script executed:
cat -n home/jailuser/git/test/index.test.ts | head -50Repository: elysiajs/elysia-static
Length of output: 134
🏁 Script executed:
rg "\.\./" home/jailuser/git/test/ --type ts -A 2 -B 2Repository: elysiajs/elysia-static
Length of output: 129
🏁 Script executed:
find . -name "index.test.ts" -o -name "*.test.ts" 2>/dev/null | grep -v node_modules | head -10Repository: elysiajs/elysia-static
Length of output: 86
🏁 Script executed:
cat -n ./home/jailuser/git/test/index.test.ts 2>/dev/null | head -100 || echo "File not found, searching..."Repository: elysiajs/elysia-static
Length of output: 48
🏁 Script executed:
fd "\.test\." --type f | grep -v node_modules | head -10Repository: elysiajs/elysia-static
Length of output: 84
🏁 Script executed:
rg "traverse|\.\./" --type ts --max-count 20 -B 1 -A 1Repository: elysiajs/elysia-static
Length of output: 780
🏁 Script executed:
cat -n test/index.test.tsRepository: elysiajs/elysia-static
Length of output: 15323
🏁 Script executed:
rg "fs\.realpath|symlink" src/ --type ts -B 2 -A 2Repository: elysiajs/elysia-static
Length of output: 48
🏁 Script executed:
rg "fastDecodeURI" src/ --type ts -B 1 -A 1Repository: elysiajs/elysia-static
Length of output: 498
🏁 Script executed:
sed -n '55,75p' src/index.tsRepository: elysiajs/elysia-static
Length of output: 670
🏁 Script executed:
rg "%2e|encoded" test/ src/ --type tsRepository: elysiajs/elysia-static
Length of output: 48
Ditch the params['*'] undefined scare — Elysia guarantees it's always a string, dummy~ ♡
The path.resolve + startsWith(assetsDir + path.sep) combo is safe before the try/catch since params['*'] for the /* route is always a string (empty '' or populated), never undefined — Elysia docs are crystal clear on this. So that defensive wrapping suggestion you were about to make? Unnecessary, baka~ (๑´ლ`๑)
BUT you're still wide open to symlinks~ (｡•́︿•̀｡) path.resolve is purely lexical and doesn't follow symlinks, so if someone drops a convenience symlink inside public/ pointing outside (like /etc), your shiny boundary check waves it right through since the resolved path still starts with assetsDir. fs.stat and getFile will happily follow the link out of bounds~ That's a real escape route, not a hypothetical~ ♡
And here's the kicker — zero regression tests for the actual thing you're fixing♡ Scan through test/index.test.ts (all 460 lines) and there's not a single test for:
../traversal attempts- URL-encoded traversal like
%2e%2e%2f - Absolute paths like
/etc/passwd - Symlinks inside
public/pointing outside
The next refactor will waltz right through and undo your work, and nobody will notice because nobody's testing for it~ (๑•́ ω •̀๑) Add coverage or this gets reverted eventually, dummy~
🛡️ Optional hardening — resolve symlinks before boundary check ♡
- const joined = path.resolve(assetsDir, userPart)
- if (joined !== assetsDir && !joined.startsWith(assetsDir + path.sep))
- throw new NotFoundError()
-
- const pathName = normalizePath(joined)
+ const joined = path.resolve(assetsDir, userPart)
+ // Resolve symlinks so a link inside assetsDir can't escape the boundary.
+ // Falls back to the lexical path when the target doesn't exist yet.
+ const real = await fs.realpath(joined).catch(() => joined)
+ if (
+ real !== assetsDir &&
+ !real.startsWith(assetsDir + path.sep)
+ )
+ throw new NotFoundError()
+
+ const pathName = normalizePath(real)Note: if you adopt this, assetsDir itself should also be realpath-ed once at plugin init so the comparison is apples-to-apples when public is a symlink.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 302 - 310, Remove the unnecessary undefined-guard
around params['*'] (Elysia guarantees it's a string) and, to prevent symlink
escapes, resolve symlinks before the boundary check: compute and store a
realpath for assetsDir at plugin init (e.g., assetsRealPath) and replace the
lexical resolve of joined with fs.realpathSync(joined) (or async realpath) and
compare that real path against assetsRealPath (use startsWith(assetsRealPath +
path.sep) or exact equality) before allowing access; finally add regression
tests covering ../ traversal, URL-encoded traversal (%2e%2e%2f), absolute path
requests (/etc/passwd) and a symlink inside public/ pointing outside to ensure
the new realpath check blocks escapes.
There was a problem hiding this comment.
The path.resolve + startsWith(assetsDir + path.sep) combo is safe before the try/catch since params[''] for the / route is always a string (empty '' or populated), never undefined — Elysia docs are crystal clear on this. So that defensive wrapping suggestion you were about to make? Unnecessary, baka~ (๑´ლ`๑)
it's what was originally being done
BUT you're still wide open to symlinks~ (｡•́︿•̀｡) path.resolve is purely lexical and doesn't follow symlinks, so if someone drops a convenience symlink inside public/ pointing outside (like /etc), your shiny boundary check waves it right through since the resolved path still starts with assetsDir. fs.stat and getFile will happily follow the link out of bounds~ That's a real escape route, not a hypothetical~ ♡
if someone is setting a symlink to /pwd on their public folder thats on them😭
also doubt regression tests are needed here.
There was a problem hiding this comment.
@tiagozip Fair points on both counts~ ♡
On symlinks — you're right that a server operator consciously placing a symlink in their own assets directory is deliberate configuration, not an attack surface for remote users. The threat model here is untrusted remote input (../ in the URL), and this PR closes that gap correctly. Symlink policy is a local concern, same as how nginx/Apache treat FollowSymLinks as an opt-in~ (´▽｀)ノ
On tests — acknowledged, your call as the maintainer (๑•̀ ω •́๑)
The core fix — path.resolve + startsWith(assetsDir + path.sep) boundary check — is solid for the intended threat. No further objections from me~
1 participant
currently, the handler uses
path.join(assets, params['*'])and relies on the joined path staying within the assets directory. sinceparams['*']is a raw string from the router,path.joincan resolve..segments in a way that produces paths outsideassetsdepending on the input. fortunately this doesn't happen with normal browser usage, but it can be triggered by editing the packets. also, this is exclusively for development mode. the same wouldn't work on production mode.this switches to
path.resolve(assetsDir, userPart)and adds an explicit check that the resolved path is eitherassetsDiritself or a descendant of it. if it isn't, we fall through toNotFoundError(), the same behavior the handler already uses for missing files and ignored patterns.behavior should not change for valid requests!
Summary by CodeRabbit