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
📝 WalkthroughWalkthroughThis PR introduces ChangesPath Canonicalization with Symlink Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/utils/WorkspacePathResolver.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/utils/__tests__/WorkspacePathResolver.spec.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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! |
1 participant
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