Skip to content

fix(server): contain workspace writes across symlinks#4226

Open
StiensWout wants to merge 2 commits into
pingdotgg:mainfrom
StiensWout:fix/workspace-write-symlink-containment
Open

fix(server): contain workspace writes across symlinks#4226
StiensWout wants to merge 2 commits into
pingdotgg:mainfrom
StiensWout:fix/workspace-write-symlink-containment

Conversation

@StiensWout

@StiensWout StiensWout commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • validate the nearest existing canonical ancestor before creating write directories
  • reject existing linked files, dangling final links, and linked parents that escape the workspace
  • create absent targets exclusively and revalidate a target that appears concurrently
  • preserve writes through file and directory links whose canonical targets remain inside the workspace

Root cause

WorkspaceFileSystem.writeFile only 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_NOFOLLOW on 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 passed
  • vp lint apps/server/src/workspace/WorkspaceFileSystem.ts apps/server/src/workspace/WorkspaceFileSystem.test.ts --deny-warnings
  • vp fmt --check apps/server/src/workspace/WorkspaceFileSystem.ts apps/server/src/workspace/WorkspaceFileSystem.test.ts
  • vp run --filter t3 typecheck
  • git diff --check

Fixes #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.writeFile so 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. readFile reuses a shared assertRealPathWithinWorkspace helper 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 WorkspaceFileSystem

  • writeFile now resolves the canonical workspace root and each ancestor of the target path, rejecting writes that would escape the workspace through symlinks.
  • Files are opened with O_NOFOLLOW to prevent following symlinks at the final path component; dangling symlinks are rejected with a structured OperationError.
  • Writes through symlinks that resolve within the workspace root are still permitted, and the symlink itself is preserved.
  • A new nextAncestorPath helper terminates ancestor traversal at the filesystem root with a WorkspaceFileSystemOperationError instead of looping indefinitely.
  • The cause field on WorkspaceFileSystemOperationError is now optional, and symlink-related tests are skipped on platforms that do not support creating symlinks.
  • Risk: the write path now performs several additional realpath/lstat syscalls per write, and existing callers that write through out-of-workspace symlinks will receive errors where previously writes may have succeeded.

Macroscope summarized 2e83a2b.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f550c08a-248b-41f4-b399-c38231008a8d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 21, 2026

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effect service review: one error-modeling convention issue in the new WorkspaceFileSystem code. See the inline comment.

Posted via Macroscope — Effect Service Conventions

Comment thread apps/server/src/workspace/WorkspaceFileSystem.ts Outdated
@macroscopeapp

macroscopeapp Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Workspace writes can escape the project root through symlinks

1 participant