[saphid]

Current assessment (2026-10-17): upstream #20449, commit 8e0d5e5e4d, now includes the containment behavior this PR proposed. At inspected main 44ecfd65b6, writes resolve the deepest existing ancestor, reject outside and dangling symlinks, and support links within the workspace and a symlinked workspace root. Main also requires known workspaces and protects Git metadata, safeguards this older branch must preserve.

The upstream symlink rejection/allowed-write cases passed in a fresh focused workspace test run using an overlay on the existing dependency checkout (the unrelated short-read candidate was present; it changes only reading). This is source inspection and focused test evidence, not a full-tree build or native Windows verification. This PR remains open and its branch is unchanged as instructed; its merge conflict is retained rather than reintroducing redundant older code or removing newer safeguards. The original proposal and historical evidence follow.

---

Problem

Saving a file from the file editor (projectsWriteFile) only checked the path lexically. If the project contains a symlink that points outside it (to a file, to a directory, or dangling), saving linked/notes.txt writes through the link. That can overwrite a file outside the workspace, or create one, and recursive parent creation can make directories outside it. Reads already reject the same path with WorkspaceFilePathEscapeError.

Change

Before writing, the server canonicalizes the workspace root and the deepest part of the target path that already exists (the file itself when it exists), appends the missing remainder, and rejects the write with the same escape error reads use if the result is outside the root. Directories are created and the file is written at that resolved path. Symlinks that stay inside the workspace, missing nested directories, and a symlinked workspace root keep working. A dangling symlink is now refused rather than written through.

Scope and approval

A focused bug fix (the write path did not honor the containment the read path already enforces), so it uses the no-prior-issue exception. One service.

Verification

• New WorkspaceFileSystem.test.ts cases on real temp directories and symlinks. On main the three escape cases fail because the write succeeds: an outside file is overwritten through a file symlink, created through a dangling symlink, and created along with new directories under a directory symlink (log). With the fix, those writes are rejected and leave the outside tree unchanged; the allowed cases pass. 29 passed (log), rerun after rebasing on current main.
• vp lint and apps/server typecheck: clean.
• Independent review by GPT-6.1 Sol (high) through a T3 delegated task: no defects. It also checked ENOTDIR, .. normalization, dangling directory links and Windows drive/UNC handling.
• Known limits: a symlink swapped in between the check and the write is not caught (Node has no per-component O_NOFOLLOW); native Windows was not run (symlink tests skip where symlinks are unsupported).

Found by a GPT-6.1 Sol (high) audit and implemented by Claude Opus 5.5 (high) in T3 Code. Reviewed by GPT-6.1 Sol. Coordinated by Claude Opus 5.5 in Claude Code via T3 Code. Agent-authored, sponsored by Alex Southwell.

🤖 Generated with Claude Code

[macroscopeapp]

Approvability

Verdict: Approved at 1ce0635

Macroscope's review found this PR approvable — The reviewed head is identical to the base commit, with no changed files or runtime impact. The filesystem fix described by the title exists only in a superseded parent and is not included in this PR's final tree.

Notes:

• No code objects were reviewed. Approvability was decided on eligibility alone.

^{You can add or adjust custom eligibility rules. Learn more.}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Path: .coderabbit.config.ts
• Review profile: CHILL
• Plan: Advanced
• Run ID: 179bdb98-ff42-4a0f-b848-66660523a907

📥 Commits

Reviewing files that changed from the base of the PR and between 511d20c and ea300fb.

📒 Files selected for processing (2)

• apps/server/src/workspace/WorkspaceFileSystem.test.ts
• apps/server/src/workspace/WorkspaceFileSystem.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

📝 Walkthrough

Workspace writes now resolve symlinks, verify that the canonical target remains inside the workspace, and write to that target. New tests cover outside-pointing and inside-pointing symlinks, dangling symlinks, and a workspace root reached through a symlink.

Changes: Workspace write boundary

Layer / File(s)	Summary
Canonicalize and validate write targets apps/server/src/workspace/WorkspaceFileSystem.ts, apps/server/src/workspace/WorkspaceFileSystem.test.ts	Write targets are resolved through symlinks and checked against the canonical workspace root. Directory-creation and write errors use the resolved target. Tests cover outside and inside symlinks, dangling symlinks, and a symlinked workspace root.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to ea300

This change stops file saves from following symlinks out of the workspace, while still allowing in-workspace symlinks and symlinked workspace roots. No merge-blocking risk was identified. The one known limitation is a symlink swapped between the check and the write.

Security Architecture Review

Security architecture risk: 🔵 Low · up to ea300

The change strengthens protection against writes escaping the selected workspace. No introduced or worsened security concern was established, but concurrent filesystem changes remain outside the confinement guarantee, and workspace ownership assumptions are not fully established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The sink executes with server-process filesystem permissions. Because the request supplies cwd and the inspected authorization checks an environment scope rather than a workspace identifier, canonical containment should not be interpreted as tenant isolation or authorization for a particular root. Potential exposure is bounded by the host paths writable by that process, not necessarily one registered project; this authority model is unchanged by the PR.

Security Findings and Attack Paths

• inferred — A concurrent actor able to replace a validated target or parent component could still redirect a subsequent pathname-based mkdir or write outside the checked root. Canonical resolution is not atomic with mutation. Base already exposed pathname mutation without canonical confinement, so this is a pre-existing limitation, not an established introduced or worsened PR concern; concurrent-actor reachability remains unproven.

Trust Boundaries and Controls

• observed — The WebSocket route authenticates a session and installs scope authorization. projectsWriteFile requires filesystem:write before delegating client-provided inputs to WorkspaceFileSystem. Lexical validation rejects absolute and parent-traversal write paths, and the new canonical check adds stable-tree symlink confinement before the sensitive filesystem sink.

Hardening Proposals

• proposed — If confinement must hold against concurrent hostile filesystem mutation, evaluate descriptor-relative, race-resistant enforcement rooted in a trusted directory identity, together with adversarial replacement tests. Treat this as additional hardening beyond the PR's stable-tree protection.

Pre-merge checks: 4

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly and concisely describes the main change: preventing server file saves from following symlinks outside the workspace.
Description check		The description includes the required Problem, Change, Scope and approval, and Verification sections. It explains the issue, implementation, testing results, known limitations, and approval rationale …

Full details: Description check

Explanation

The description includes the required Problem, Change, Scope and approval, and Verification sections. It explains the issue, implementation, testing results, known limitations, and approval rationale with sufficient detail.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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