fix(security): resolve symlinks before path validation to prevent directory traversal#398
fix(security): resolve symlinks before path validation to prevent directory traversal#398sorlen008 wants to merge 2 commits intowonderwhy-er:mainfrom
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
codeant-ai
Bot
added
the
size:M
Sequence DiagramThis PR updates path validation to resolve parent or ancestor directories when the target file does not yet exist, ensuring symlinks are followed before checking directory allowlists and preventing traversal through symlinked paths. sequenceDiagram
participant Caller
participant FilesystemTool
participant OS
participant SecurityPolicy
Caller->>FilesystemTool: Request file access with path
FilesystemTool->>OS: Resolve full real path
alt Path exists
OS-->>FilesystemTool: Real path
else Path missing (ENOENT)
OS-->>FilesystemTool: ENOENT error
FilesystemTool->>OS: Resolve parent or ancestor directory
OS-->>FilesystemTool: Resolved directory path
FilesystemTool->>FilesystemTool: Rebuild full path from resolved directory
end
FilesystemTool->>SecurityPolicy: Check resolved path against allowed directories
alt Path allowed
SecurityPolicy-->>FilesystemTool: Allowed
FilesystemTool-->>Caller: Proceed with file operation
else Path not allowed
SecurityPolicy-->>FilesystemTool: Denied
FilesystemTool-->>Caller: Reject path
end
Generated by CodeAnt AI |
Nitpicks 🔍
|
|
CodeAnt AI finished reviewing your PR. |
![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/tools/filesystem.ts`:
- Around line 206-234: The validation currently computes resolvedRealPath (via
fs.realpath fallback) but continues to return or use absoluteOriginal for
mutations, leaving a TOCTOU gap; update validatePath() to return the
resolvedRealPath (or both resolved and original) and ensure callers (writeFile,
createDirectory, moveFile, writePdf) use the resolved path for the actual
filesystem mutations so the same symlink-resolved path used for allowlist checks
is used for writes/moves/creates; locate references to validatePath(),
writeFile(), createDirectory(), moveFile(), and writePdf and switch their
mutation target to the resolved path returned by validatePath().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:M
|
CodeAnt AI Incremental review completed. |
sorlen008
force-pushed
the
fix/symlink-directory-traversal
branch
from
0f9e2a4 to
8021546
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:M
|
CodeAnt AI Incremental review completed. |
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/tools/filesystem.ts`:
- Around line 211-233: The fallback path in the canonicalization logic (around
resolvedRealPath / absoluteOriginal where fs.realpath(parentDir) is caught) must
first detect if the unresolvable leaf is a symlink: call fs.lstat on the
basename/first unresolvable component and if lstat indicates a symlink, read its
target with fs.readlink, resolve that target to an absolute path and feed that
resolved target back through the same canonicalization (fs.realpath) and
allowlist checks instead of simply joining the unresolved basename; keep the
existing walk-up-to-ancestor loop (current, remaining) as the fallback only when
the leaf is not a symlink or the link target cannot be resolved. Ensure you use
the same error handling and do not bypass the allowlist check when handling
readlink results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…outside-allowlist gap When fs.realpath() fails with ENOENT, the previous fallback resolved the parent directory and rejoined the basename (the symlink name itself). Since fs.writeFile() follows symlinks, a broken symlink inside an allowed directory could redirect a write to an arbitrary path outside the allowlist. Fix: before falling back to parent-directory resolution, use fs.lstat() to detect whether the unreachable path is a symlink. If it is, read the symlink target with fs.readlink() and resolve it to an absolute canonical path so that the allowlist check and all subsequent file operations use the actual destination, not the symlink entry. Also tighten the ancestor-walk loop to stop on non-ENOENT errors (e.g. EPERM) rather than silently swallowing them, preventing a degraded-resolution bypass on restricted directory trees. Closes coderabbitai review comment on PR wonderwhy-er#398.
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:M
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/tools/filesystem.ts (1)
224-226: Consider logging swallowed errors for debugging.The empty catch block silently swallows all errors (including EPERM, EIO, etc.). While the fallback to parent resolution is safe, logging these errors would aid debugging in production environments.
Optional improvement
} catch (symlinkErr) { - // Not a symlink or unreadable; fall through to parent-directory resolution + // Not a symlink or unreadable; fall through to parent-directory resolution + // Log for debugging but don't fail - parent resolution will handle this + const se = symlinkErr as NodeJS.ErrnoException; + if (se.code && se.code !== 'ENOENT' && se.code !== 'ENOTDIR') { + console.debug(`Symlink check failed for ${absoluteOriginal}: ${se.code}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/filesystem.ts` around lines 224 - 226, The empty catch that currently contains the comment "Not a symlink or unreadable; fall through to parent-directory resolution" should log the caught error instead of swallowing it: capture the exception in the catch, emit a debug/warn log via your project's logger (or console.debug) including the error object and the path being inspected for symlink resolution so you retain the existing fallback behavior but surface EPERM/EIO/etc. for troubleshooting.
🤖 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/tools/filesystem.ts`:
- Around line 212-226: The symlink handling in the try block sets
resolvedRealPath from path.resolve(readlink(...)) which does not resolve
symlinks in intermediate components and skips the parent-directory realpath
fallback; change the logic in the lstat/readlink branch (around lstat, readlink,
resolvedRealPath) to call fs.realpath() on the resolved link target and assign
that result to resolvedRealPath (fall back to path.resolve only for forming the
initial candidate), so that any nested symlinks are fully resolved before
running isPathAllowed and before subsequent file operations; ensure errors from
fs.realpath are caught and fall through to the existing parent-directory
resolution logic if realpath fails.
---
Nitpick comments:
In `@src/tools/filesystem.ts`:
- Around line 224-226: The empty catch that currently contains the comment "Not
a symlink or unreadable; fall through to parent-directory resolution" should log
the caught error instead of swallowing it: capture the exception in the catch,
emit a debug/warn log via your project's logger (or console.debug) including the
error object and the path being inspected for symlink resolution so you retain
the existing fallback behavior but surface EPERM/EIO/etc. for troubleshooting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…outside-allowlist gap When fs.realpath() fails with ENOENT, the previous fallback resolved the parent directory and rejoined the basename (the symlink name itself). Since fs.writeFile() follows symlinks, a broken symlink inside an allowed directory could redirect a write to an arbitrary path outside the allowlist. Fix: before falling back to parent-directory resolution, use fs.lstat() to detect whether the unreachable path is a symlink. If it is, read the symlink target with fs.readlink() and resolve it to an absolute canonical path so that the allowlist check and all subsequent file operations use the actual destination, not the symlink entry. Also tighten the ancestor-walk loop to stop on non-ENOENT errors (e.g. EPERM) rather than silently swallowing them, preventing a degraded-resolution bypass on restricted directory trees. Closes coderabbitai review comment on PR wonderwhy-er#398.
sorlen008
force-pushed
the
fix/symlink-directory-traversal
branch
from
ade51d7 to
bd32bbb
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:M
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/tools/filesystem.ts (1)
255-265:⚠️ Potential issue | 🔴 CriticalResolve the first broken symlink component, not just the leaf.
lstat(absoluteOriginal)only handles the case where the requested path itself is the broken symlink. A path like/allowed/link/subdirwithlink -> /restricted/missingstill reconstructs/allowed/link/subdir, andmkdir(..., { recursive: true })will follow that symlink and create/restricted/missing/subdir; likewise,readlink()targets are only passed throughpath.resolve(), so chained symlinks inside the target path can still survive the allowlist check. Please canonicalize the first unresolved symlink component /readlink()target with the samerealpathwalk before callingisPathAllowed(), and add a regression for both shapes.In Node.js, does `path.resolve()` resolve symlinks or only normalize lexical path segments? If `fs.promises.mkdir('/allowed/link/subdir', { recursive: true })` is called and `link` is a symlink to `/restricted/missing`, will Node follow the symlink and create `/restricted/missing/subdir`?Also applies to: 275-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/filesystem.ts` around lines 255 - 265, The current logic only lstats the full absoluteOriginal and, on a symlink leaf, resolves its readlink target but does not canonicalize the first unresolved symlink component or the readlink target through the same realpath-walking logic before calling isPathAllowed; update the resolution in the function that sets resolvedRealPath (the block using fs.lstat, fs.readlink and resolvedRealPath) to iterate path components from the root, lstat each component, and when encountering a symlink resolve its readlink, then re-run the canonicalization/walk on the resolved target (i.e., perform the same realpath walk on the readlink result) so the first broken/intermediate symlink is resolved to its true absolute path before any mkdir or isPathAllowed check; ensure the same fix is applied to the later analogous block (lines ~275-305) and add regression tests that cover both a broken-symlink leaf (/allowed/link -> /restricted/missing + /allowed/link/subdir) and an intermediate symlink component (/allowed/link/subdir where link -> /restricted/missing) to verify mkdir and allowlist checks use the canonical resolved path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/tools/filesystem.ts`:
- Around line 255-265: The current logic only lstats the full absoluteOriginal
and, on a symlink leaf, resolves its readlink target but does not canonicalize
the first unresolved symlink component or the readlink target through the same
realpath-walking logic before calling isPathAllowed; update the resolution in
the function that sets resolvedRealPath (the block using fs.lstat, fs.readlink
and resolvedRealPath) to iterate path components from the root, lstat each
component, and when encountering a symlink resolve its readlink, then re-run the
canonicalization/walk on the resolved target (i.e., perform the same realpath
walk on the readlink result) so the first broken/intermediate symlink is
resolved to its true absolute path before any mkdir or isPathAllowed check;
ensure the same fix is applied to the later analogous block (lines ~275-305) and
add regression tests that cover both a broken-symlink leaf (/allowed/link ->
/restricted/missing + /allowed/link/subdir) and an intermediate symlink
component (/allowed/link/subdir where link -> /restricted/missing) to verify
mkdir and allowlist checks use the canonical resolved path.
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:M
|
CodeAnt AI Incremental review completed. |
|
Hello @sorlen008!| |
…ectory traversal When a file doesn't exist yet (e.g., write operations), fs.realpath() fails with ENOENT and the code fell back to the unresolved path for the allowedDirectories check. An attacker could create a symlink inside an allowed directory pointing to a restricted location, then write a new file through that symlink — the path appeared to be inside the allowed directory but actually targeted a restricted one. The fix resolves the parent directory (and walks up to the deepest existing ancestor if needed) so symlinks anywhere in the path chain are followed to their real targets before the directory allowlist check runs. Closes wonderwhy-er#219 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…bypass Previously, validatePath() resolved symlinks for the allowlist check but returned the original (unresolved) path. This meant callers like writeFile() and createDirectory() would still operate on the symlink path, following it to the restricted target. Now validatePath() consistently returns the resolved (canonical) path so all subsequent file operations use the real target path, closing the symlink write bypass vector. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sorlen008
force-pushed
the
fix/symlink-directory-traversal
branch
from
38ccae1 to
93c9825
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
@edgarsskore Sorry about that — those deletions came from a stale automated rebase that added some garbage commits. I just force-pushed a clean version rebased on current main. The actual change is now what it should have been: +42 / -12 in Should be ready for review now. Apologies for the noise. |
codeant-ai
Bot
added
size:M
|
CodeAnt AI Incremental review completed. |
2 participants
User description
Summary
This fixes a directory traversal vulnerability via symlink bypass in
isPathAllowed()/validatePath()— reported in #219 (open for ~7 months now).The attack is straightforward: create a symlink inside an allowed directory that points to a restricted location, then read/write a file through that symlink. Because the target file doesn't exist yet,
fs.realpath()fails with ENOENT and the code falls back to the raw (unresolved) path for the allowlist check. The path looks like it's inside the allowed directory, but the symlink redirects the actual I/O to somewhere else entirely.Example:
/home/user/projects/home/user/projects/evil->/etc/(symlink)/home/user/projects/evil/crontab/home/user/projects/), file gets written to/etc/crontab/home/user/projects/evilis resolved to/etc/, reconstructed path/etc/crontabfails the allowlist checkWhat changed
In
validatePath()insrc/tools/filesystem.ts: whenfs.realpath()fails with ENOENT (path doesn't exist), the fix now resolves the parent directory to follow any symlinks in the path chain. If the parent also doesn't exist (nested new directories), it walks up to the deepest existing ancestor and resolves from there. The resolved path is then used for the allowlist check instead of the raw path.This is a minimal, targeted fix — about 30 lines added, no API changes, no breaking changes. Existing tests (allowed-directories, directory-creation) all pass. The existing
test-symlink-security.jstest suite already covers this exact attack scenario and expects the fix.Test plan
npx tsc --noEmitpassestest-allowed-directories.js— all 6 tests passtest-directory-creation.js— all tests pass (write-to-nonexistent-path still works)test-symlink-security.js— covers the exact attack vector (requires symlink privileges to run; passes on Linux/macOS)Closes #219
Summary by CodeRabbit
CodeAnt-AI Description
Block symlink-based writes outside allowed folders
What Changed
Impact
✅ Prevented writes outside allowed directories✅ Safer file creation in symlinked folders✅ Fewer directory traversal security bypasses💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.