feat: add ALLOWED_PATHS environment variable to restrict file system access#350
feat: add ALLOWED_PATHS environment variable to restrict file system access#350feraudet wants to merge 1 commit intositeboon:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughIntroduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
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)
server/index.js (1)
492-573: Symlink escape possible in/api/browse-filesystemallowed-path check.
path.resolveis lexical; a symlink inside an allowed directory can point outside and still pass the check. Resolve both the target and allowed paths withrealpathbefore comparison.🔒 Suggested fix
- if (allowedPaths && allowedPaths.length > 0) { - const isAllowed = allowedPaths.some(allowedPath => { - const resolved = path.resolve(allowedPath); - return targetPath === resolved || targetPath.startsWith(resolved + path.sep); - }); + if (allowedPaths && allowedPaths.length > 0) { + const resolvedTarget = await fs.promises.realpath(targetPath) + .catch(() => path.resolve(targetPath)); + let isAllowed = false; + for (const allowedPath of allowedPaths) { + const resolvedAllowed = await fs.promises.realpath(allowedPath) + .catch(() => path.resolve(allowedPath)); + if (resolvedTarget === resolvedAllowed || resolvedTarget.startsWith(resolvedAllowed + path.sep)) { + isAllowed = true; + break; + } + } if (!isAllowed) { return res.status(403).json({ error: `Access restricted to: ${allowedPaths.join(', ')}`, allowedPaths: allowedPaths }); } + targetPath = resolvedTarget; }
🤖 Fix all issues with AI agents
In `@server/projects.js`:
- Around line 428-438: Extract the path-check logic into a shared isAllowedPath
helper (used in the Claude-discovered block that currently checks
actualProjectDir and in the manual-projects block later) that: 1) reads
ALLOWED_PATHS via getAllowedPaths(), 2) resolves both the candidate project path
and each allowedPath to their real paths (e.g., using fs.realpathSync or async
equivalent) to prevent symlink escape, and 3) returns true only if the real
project path equals or is a child of a real allowedPath; replace the inline
checks in the current actualProjectDir block and the manual-projects addition to
call isAllowedPath and skip projects when it returns false.
In `@server/routes/projects.js`:
- Around line 43-44: Remove the dead exported constant ALLOWED_PATHS: delete the
export declaration for ALLOWED_PATHS and any associated comment so the module
only exposes the real accessors (getAllowedPaths) and uses
process.env.ALLOWED_PATHS; ensure no other code relies on the removed symbol and
run tests to confirm nothing imports ALLOWED_PATHS.
…access Implements path-based access restrictions for enhanced security: - Add getAllowedPaths() function to read and cache ALLOWED_PATHS env var - Add isPathAllowed() function with symlink protection using realpath - Update /api/browse-filesystem to enforce ALLOWED_PATHS restrictions - Filter projects in getProjects() based on allowed paths - Add startup logging for ALLOWED_PATHS configuration - Default to first allowed path when browsing without explicit path The ALLOWED_PATHS variable accepts comma-separated directory paths. When set, only projects within these paths are accessible. Closes siteboon#348 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
3776f19 to
7df4dfd
Compare
Summary
Implements #348 - Add
ALLOWED_PATHSenvironment variable to restrict which directories can be accessed through the UI.Changes
getAllowedPaths()function inserver/routes/projects.jsgetProjects()based on allowed paths/api/browse-filesystemendpoint to allowed pathsConfiguration
Technical Notes
Uses a getter function
getAllowedPaths()instead of a constant to work around ES Modules hoisting (imports run before .env is loaded).Test plan
ALLOWED_PATHSto a single directory - only that project appearsCloses #348
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.