fix(server): contain workspace writes across symlinks#4226
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Effect service review: one error-modeling convention issue in the new WorkspaceFileSystem code. See the inline comment.
Posted via Macroscope — Effect Service Conventions
ApprovabilityVerdict: Needs human review This PR introduces significant changes to security-critical file write operations, adding symlink traversal protection logic. Security-sensitive changes to file system validation warrant human review to verify the protection mechanisms are correctly implemented. You can customize Macroscope's approvability policy. Learn more. |
Summary
Root cause
WorkspaceFileSystem.writeFileonly enforced lexical containment before recursively creating directories and writing the requested path. Filesystem links could therefore redirect a lexically in-root path outside the selected workspace.Impact
Deterministic pre-existing linked-file, dangling-final-link, and linked-parent escapes are rejected before an outside file or missing directory can be written. Ordinary new and existing writes, workspace-entry cache refresh, and safe in-workspace link behavior remain supported.
Missing final targets are opened with exclusive creation. If a final component appears after the absence check, it is canonicalized and containment-checked before opening. Existing targets use no-follow protection where Node exposes it.
Node does not expose descriptor-relative traversal for the complete parent chain, so a concurrent parent-directory replacement remains a residual TOCTOU limitation. Node also does not expose
O_NOFOLLOWon Windows; dangling-entry detection and exclusive-create/revalidation close the deterministic cases covered here.Validation
vp test run apps/server/src/workspace/WorkspaceFileSystem.test.ts— 12 passed, 4 skipped on this Windows host after actual file-symlink capability failures (EPERM); directory-junction and fixed-point cases ran and passedvp lint apps/server/src/workspace/WorkspaceFileSystem.ts apps/server/src/workspace/WorkspaceFileSystem.test.ts --deny-warningsvp fmt --check apps/server/src/workspace/WorkspaceFileSystem.ts apps/server/src/workspace/WorkspaceFileSystem.test.tsvp run --filter t3 typecheckgit diff --checkFixes #4223
Note
High Risk
Changes security-critical workspace filesystem boundary enforcement; incorrect checks could still allow path escape or break legitimate writes.
Overview
Hardens
WorkspaceFileSystem.writeFileso lexically in-root paths cannot be redirected outside the workspace via symlinks. Writes now canonicalize the workspace root, walk parents to find the nearest existing ancestor (with a filesystem-root stop), and reject paths whose real targets fall outside the root—including dangling final symlinks.Opening uses
O_NOFOLLOW(where Node exposes it), exclusive create for missing files, truncate for existing ones, and a re-check if a target appears between probe and open.readFilereuses a sharedassertRealPathWithinWorkspacehelper for the same containment rule.Tests add symlink-aware helpers (skip when the OS blocks file symlinks) and cover escapes, safe in-workspace links, overwrites, and ancestor discovery at the volume root.
Reviewed by Cursor Bugbot for commit df766e1. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Prevent workspace writes from escaping via symlinks in
WorkspaceFileSystemwriteFilenow resolves the canonical workspace root and each ancestor of the target path, rejecting writes that would escape the workspace through symlinks.O_NOFOLLOWto prevent following symlinks at the final path component; dangling symlinks are rejected with a structuredOperationError.nextAncestorPathhelper terminates ancestor traversal at the filesystem root with aWorkspaceFileSystemOperationErrorinstead of looping indefinitely.causefield onWorkspaceFileSystemOperationErroris now optional, and symlink-related tests are skipped on platforms that do not support creating symlinks.realpath/lstatsyscalls per write, and existing callers that write through out-of-workspace symlinks will receive errors where previously writes may have succeeded.Macroscope summarized 2e83a2b.