feat(security): introduce WorkspacePathResolver — async symlink-aware path canonicalization (#389)#428
Conversation
… path canonicalization (Zoo-Code-Org#389) First of four sub-issues under Zoo-Code-Org#169. Adds `src/utils/WorkspacePathResolver.ts` with a single async `resolveRealPath(target)` that canonicalizes a path by following symlinks via `fs.promises.realpath`: - Walks up to the nearest existing ancestor on ENOENT and re-appends the trailing segments, so not-yet-created files under a symlinked ancestor still resolve correctly. - Re-throws non-ENOENT errors (EACCES, ELOOP) so callers can fail closed rather than fall back to a lexical path. - Case-normalizes the result on macOS/Windows for reliable comparison against uri.fsPath. Pure utility — no workspace policy, settings, or tool changes (those land in WorkspaceFileAccess (Zoo-Code-Org#390) and the migrations in Zoo-Code-Org#391/Zoo-Code-Org#392). Covered by integration tests using real symlinks in a temp directory.
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)
📝 WalkthroughWalkthroughThis PR introduces resolveRealPath(), an async utility for canonicalizing workspace paths by resolving symlinks, handling non-existent path ancestors via a walk-up strategy, and normalizing case on case-insensitive platforms. The implementation rethrows non-ENOENT errors to preserve security-relevant failures. ChangesPath Canonicalization with Symlink Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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. 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/WorkspacePathResolver.ts`:
- Around line 13-16: The normalizeCase function currently lowercases paths based
solely on process.platform which is unsafe on macOS with case-sensitive APFS;
update normalizeCase to avoid global lowercasing and instead canonicalize
per-path using filesystem-aware APIs (e.g., fs.realpathSync / fs.realpath or
other OS canonicalization) to obtain the actual on-disk case for the given path,
and only fall back to a platform heuristic if canonicalization fails; locate and
modify the normalizeCase function and any callers that rely on its behavior so
comparisons use the fs-canonicalized path or an explicit per-mount
case-sensitivity probe rather than always lowercasing when process.platform ===
"darwin" || "win32".
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bfd718fc-24c9-4c57-a666-d0272f1a3f45
📒 Files selected for processing (2)
src/utils/WorkspacePathResolver.tssrc/utils/__tests__/WorkspacePathResolver.spec.ts
| function normalizeCase(p: string): string { | ||
| const caseInsensitive = process.platform === "darwin" || process.platform === "win32" | ||
| return caseInsensitive ? p.toLowerCase() : p | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Can APFS volumes on macOS be configured as case-sensitive, and is it valid to assume all darwin paths are case-insensitive for security path comparisons?
💡 Result:
Yes. APFS volumes on macOS can be formatted as either case-insensitive or case-sensitive (with case-insensitive being the default on macOS) [1]. No, you should not assume that “all darwin paths” are case-insensitive for security path comparisons. macOS runs on top of filesystems/volumes that can be configured case-sensitively; Apple explicitly documents that APFS has both case-sensitive and case-insensitive variants on macOS [1]. Therefore, security-sensitive comparisons must be based on the actual filesystem/volume behavior for the path being validated, not just the OS (“Darwin”) itself. A common example is software behavior (e.g., nix) that breaks when it assumes Darwin implies a case-insensitive filesystem, because users can mount case-sensitive volumes on macOS [2]. Practical implication for path-boundary checks: treat case-folding/case-normalization as filesystem-dependent, or perform comparisons on canonicalized paths resolved by the OS (e.g., after resolving symlinks and walking to the real target) rather than trusting raw string equality/case-insensitive heuristics. Relying on blanket “case-insensitive” assumptions can create security bugs when the target volume is case-sensitive [2].
Citations:
- 1: https://apple.stackexchange.com/questions/307165/change-apfs-volume-to-case-sensitive
- 2: https://swild.dev/dev/apfs-case-insensitive/
Avoid platform-wide lowercasing in normalizeCase()
Lowercasing whenever process.platform === "darwin" || "win32" assumes those platforms are case-insensitive, but APFS on macOS can be configured as case-sensitive. This can conflate distinct paths on case-sensitive volumes and break security/path-boundary comparisons. Normalize/compare using filesystem-actual case behavior (e.g., detect case-sensitivity per root/mount or rely on OS-level canonicalization) rather than a Darwin/Windows heuristic.
🤖 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 `@src/utils/WorkspacePathResolver.ts` around lines 13 - 16, The normalizeCase
function currently lowercases paths based solely on process.platform which is
unsafe on macOS with case-sensitive APFS; update normalizeCase to avoid global
lowercasing and instead canonicalize per-path using filesystem-aware APIs (e.g.,
fs.realpathSync / fs.realpath or other OS canonicalization) to obtain the actual
on-disk case for the given path, and only fall back to a platform heuristic if
canonicalization fails; locate and modify the normalizeCase function and any
callers that rely on its behavior so comparisons use the fs-canonicalized path
or an explicit per-mount case-sensitivity probe rather than always lowercasing
when process.platform === "darwin" || "win32".
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2 participants
Closes #389. First of four sub-issues under #169 toward a central
WorkspaceFileAccesslayer that makes the workspace boundary check structurally impossible to forget.Adds
src/utils/WorkspacePathResolver.ts— a single asyncresolveRealPath(target)that canonicalizes a path by following symlinks. It owns only path resolution: no workspace policy, no settings, no tool changes (those land in #390 / #391 / #392).Behavior
Carried over from the proven logic in #241, made async per the issue:
fs.promises.realpath— never blocks the extension host event loop.ENOENTerrors (EACCES,ELOOP) so a caller performing a security check can fail closed instead of falling back to a lexical path ([BUG] vscode out-of-workspace read protection trivially circumvented by symlinks #169).uri.fsPath.Tests
src/utils/__tests__/WorkspacePathResolver.spec.ts— real symlinks in a temp directory, nofsmocking:EACCESre-thrown,ELOOP(circular symlink) re-thrownSymlink/permission cases skip cleanly on hosts that can't reproduce them (Windows without privileges, root).
Does not change any tool behavior.
tsc,eslint, and the spec (7 tests) pass locally.Summary by CodeRabbit
Bug Fixes
Tests