fix(security): follow symlinks in workspace boundary check (#169)#241
fix(security): follow symlinks in workspace boundary check (#169)#241proyectoauraorg wants to merge 4 commits into
Conversation
proyectoauraorg
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughResolves symlink bypass by canonicalizing paths and updating workspace containment checks (fail-closed). Adds tests, a new ChangesSymlink-aware path containment validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@proyectoauraorg Thank you! I think this needs to be optional via the
setting suggested in #169. There will be inevitably be users whose workflow will be broken without such a setting. |
| try { | ||
| const resolved = fs.realpathSync.native(current) | ||
| return trailing.length > 0 ? path.join(resolved, ...trailing.reverse()) : resolved | ||
| } catch { |
There was a problem hiding this comment.
What happens if realpathSync.native throws something other than ENOENT — e.g. EACCES on a symlink whose target has restricted permissions? The walk-up would skip the symlink entirely and return the lexical path, so isPathOutsideWorkspace would report "inside workspace" even though the real target might be outside.
For a security boundary, would it be safer to catch only ENOENT and let other errors propagate (or treat them as "outside")? Something like:
| } catch { | |
| } catch (err: unknown) { | |
| if (!(err instanceof Error && 'code' in err && (err as NodeJS.ErrnoException).code === 'ENOENT')) { | |
| // Non-ENOENT error (e.g. EACCES) — fail closed for safety. | |
| return path.resolve(target) | |
| } |
There was a problem hiding this comment.
Great catch — thanks @edelauna 🙏
Fixed in 49a70d9: the walk-up now only happens on ENOENT. Any other error (e.g. EACCES on a symlink whose target has restricted permissions) propagates, and isPathOutsideWorkspace catches it and fails closed — treating the path as outside rather than falling back to the lexical path.
I went with propagate-and-fail-closed instead of returning path.resolve(target) directly, because returning the lexical path would still report a workspace-internal symlink as "inside" in exactly the case you flagged. Added a regression test that stubs realpath to throw EACCES.
Happy to adjust if you'd prefer a different shape.
|
Thanks @nh2 — that's a fair point. Symlinks pointing outside the workspace are a legitimate workflow for some users, and a hard block shouldn't break them silently. The opt-in setting from #169 ("Include symlinks resolving outside of workspace") is the right way to cover that. My suggestion would be to land this PR as the default-safe security fix and add the setting as a focused follow-up, so the config + UI + i18n changes get their own review — but I'm equally happy to add it here if a single PR is preferred. @edelauna any preference on scope? |
|
@proyectoauraorg lets track the ui/setting as a sub-issue. I'll wait to approve this PR until we also have the settings so that we can push both in the same release. |
…-Code-Org#169) Adds an opt-in `allowSymlinksOutsideWorkspace` setting (default off) so users who deliberately rely on symlinks pointing outside the workspace can bypass the fail-closed boundary check from Zoo-Code-Org#169/Zoo-Code-Org#241. When enabled, isPathOutsideWorkspace compares lexical paths instead of resolving symlinks. Threaded to the read/list/edit tools via a BaseTool helper. UI + i18n follow.
…Org#246) Surfaces the opt-in setting in the Context settings panel (toggle, default off) and adds its label + description across all 18 locales. Pairs with the workspace-boundary symlink fix so both ship together (Zoo-Code-Org#169 / Zoo-Code-Org#241).
|
Added the opt-in setting (#246) to this PR so the fix and the escape hatch ship together, as discussed.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webview-ui/src/components/settings/SettingsView.tsx (1)
406-407: ⚡ Quick winAdd local webview tests for the new symlink setting wiring.
Please add/extend Vitest coverage for this setting’s dirty-state and save payload path (including init vs user-edit behavior).
As per coding guidelines,
webview-ui/src/**/*.{ts,tsx}: “Prefer localwebview-uitests… Add or update Vitest coverage underwebview-ui/src/**/__tests__”, andwebview-ui/src/**/SettingsView.{ts,tsx}: “tests should distinguish automatic initialization from real user edits.”Also applies to: 840-840
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/settings/SettingsView.tsx` around lines 406 - 407, Add Vitest unit tests under webview-ui/src/**/__tests__ that cover the new allowSymlinksOutsideWorkspace wiring in the SettingsView component: verify initial initialization (when SettingsView reads default or persisted settings) does not mark the form as dirty and does not include unintended values in the save payload, and verify a real user edit to the allowSymlinksOutsideWorkspace toggle marks the view as dirty and produces the expected save payload (including allowSymlinksOutsideWorkspace and maxImageFileSize fields). Target the SettingsView component (and any local helpers used to build the save payload) and write assertions for initial vs user-edited behavior, dirty-state changes, and that the saved payload contains the correct boolean/number values for allowSymlinksOutsideWorkspace and maxImageFileSize.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@webview-ui/src/components/settings/SettingsView.tsx`:
- Around line 406-407: Add Vitest unit tests under webview-ui/src/**/__tests__
that cover the new allowSymlinksOutsideWorkspace wiring in the SettingsView
component: verify initial initialization (when SettingsView reads default or
persisted settings) does not mark the form as dirty and does not include
unintended values in the save payload, and verify a real user edit to the
allowSymlinksOutsideWorkspace toggle marks the view as dirty and produces the
expected save payload (including allowSymlinksOutsideWorkspace and
maxImageFileSize fields). Target the SettingsView component (and any local
helpers used to build the save payload) and write assertions for initial vs
user-edited behavior, dirty-state changes, and that the saved payload contains
the correct boolean/number values for allowSymlinksOutsideWorkspace and
maxImageFileSize.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff706651-a819-4254-9e23-9915d643fbf3
📒 Files selected for processing (29)
.changeset/symlink-workspace-boundary.mdpackages/types/src/global-settings.tspackages/types/src/vscode-extension-host.tssrc/core/tools/BaseTool.tssrc/core/tools/EditFileTool.tssrc/core/tools/ListFilesTool.tssrc/core/tools/SearchReplaceTool.tssrc/utils/__tests__/pathUtils.spec.tssrc/utils/pathUtils.tswebview-ui/src/components/settings/ContextManagementSettings.tsxwebview-ui/src/components/settings/SettingsView.tsxwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
…-Org#169) isPathOutsideWorkspace() only normalized ./.. so a symlink living inside the workspace but pointing outside passed the check, trivially bypassing the out-of-workspace read protection. Resolve the real path (following symlinks) for both the target and the workspace folders before comparing. Paths that don't exist yet resolve via their nearest existing ancestor. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…g#169) Per @edelauna's review: only ENOENT triggers the nearest-ancestor walk-up. Any other error (e.g. EACCES on a symlink whose target has restricted permissions) now propagates, and isPathOutsideWorkspace fails closed — treating the path as outside instead of masking the symlink with the lexical path. Adds a regression test that stubs realpath to throw EACCES. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…-Code-Org#169) Adds an opt-in `allowSymlinksOutsideWorkspace` setting (default off) so users who deliberately rely on symlinks pointing outside the workspace can bypass the fail-closed boundary check from Zoo-Code-Org#169/Zoo-Code-Org#241. When enabled, isPathOutsideWorkspace compares lexical paths instead of resolving symlinks. Threaded to the read/list/edit tools via a BaseTool helper. UI + i18n follow.
…Org#246) Surfaces the opt-in setting in the Context settings panel (toggle, default off) and adds its label + description across all 18 locales. Pairs with the workspace-boundary symlink fix so both ship together (Zoo-Code-Org#169 / Zoo-Code-Org#241).
proyectoauraorg
force-pushed
the
fix/169-symlink-workspace-boundary
branch
from
7652419 to
19cc696
Compare
3 participants
Summary
Fixes #169 (security). The out-of-workspace read protection could be bypassed with a symlink:
isPathOutsideWorkspace()only resolved./..(path.resolve), not symlink targets. A symlink that lives inside the workspace but points outside it was therefore treated as inside, so it slipped past the boundary check.Fix
Resolve the real path (following symlinks via
fs.realpathSync) for both the target path and the workspace folders before comparing. A symlink inside the workspace that points outside now correctly resolves outside and is flagged.For paths that don't exist yet (e.g. a file about to be created), the realpath of the nearest existing ancestor is resolved and the remaining segments re-appended, so creation flows still work and any symlinked ancestor is still followed.
Testing
New tests use real symlinks in a temp dir: a symlinked file and a symlinked directory inside the workspace that point outside are both flagged as outside; real in-workspace files (existing and not-yet-created) stay inside; real outside files stay outside.
Summary by CodeRabbit
Bug Fixes
New Features
Tests