Skip to content

fix(local-runtime): contain turn-diff mutation targets by real path - #232

Merged
modacker merged 1 commit into
webuifrom
fix/turn-diff-symlink-containment
Oct 23, 2026
Merged

modacker merged 1 commit into
webuifrom
fix/turn-diff-symlink-containment

Conversation

@modacker

Copy link
Copy Markdown
Collaborator

Change

The turn-diff revert/reapply could write into a file outside the workspace.

resolveMutationTargetPath (packages/local-runtime/src/turns/file-changes.ts) is the containment check for the turn-diff mutation — the undo/revert and reapply write. It is not the check on the tool's own write, and not the capture.

Call chain:

  • WebUI review panel → routeLocalTurnDiffMutationApi (packages/local-runtime/src/turns/diff-api.ts:157) → mutateLocalTurnDiff → applyTurnDiffMutation (diff-api.ts:174) → applyLocalTurnDiffSnapshotMutation (file-changes.ts:389) → resolveMutationTargetPath → fs.writeFile / fs.rm (file-changes.ts:431).
  • Fork/rewind: rewindRecordFiles (packages/local-runtime/src/turns/diff-capability.ts:127) calls the same mutation with the fork's target workspace.

PR #220 made that mutation all-or-nothing and added this check, and left the remaining boundary written down in the code:

Known boundary (lexical containment only): a contained path that is a symlink pointing outside the workspace still resolves outside.

Lexical containment is not containment. A path inside the workspace whose final component is a symlink pointing outside it is inside the workspace as a string and outside it as a file — and fs.writeFile follows the link. Reproduced on the base commit with the test in this PR: the file outside the workspace held outside-v1 before the revert and outside-v0 after it, so the revert wrote through the in-workspace symlink. The same held for reapply, for a symlinked directory inside the workspace, for a relative link, and for a dangling link — where the write creates the file it points at, outside the workspace.

The fix

packages/local-runtime/src/turns/file-changes.ts

  • The lexical check stays exactly as it was. It costs no syscall and still refuses an absolute path, a .. climb, a sibling directory whose name starts with the workspace name, and everything normalizeCapturePath filters (.git/, .harness-def/, the root itself).
  • On top of it, the target's real path is resolved and compared with the workspace root's real path. The real root is resolved once per run, not once per entry.
  • The resolver follows the tool path guard (packages/agent-tools/src/desktop/path-guard.ts): realpath the target; when the target does not exist yet — a revert recreates a deleted file — resolve its nearest existing ancestor and re-attach the missing segments. A dangling symlink is followed by hand, because realpath fails on it the way it fails on a missing file while a write through it creates the file it points at.
  • Containment is decided with path.relative, like the tool path guard — case-insensitive on Windows, where the filesystem is.
  • A path that cannot be resolved at all (a symlink cycle, a permission error) is refused, not guessed.

Both sides are compared as real paths, so a workspace that is itself reached through a symlink still contains its own files, and a link that leaves it does not.

No existing refusal was weakened: the 11 pre-existing precheck tests are unchanged and still pass. What is added on top is the symlink escape. The "Known boundary (lexical containment only)" note is removed — the boundary it described is closed. What remains is stated instead: the real path is resolved before the write, so a symlink swapped between the check and the write is still a race.

Validation

  • Checks run and results (base webui @ 63b9739e, head bd9bb67c, macOS arm64, Node v25.9.0):
    • New suite packages/local-runtime/test/unit/diff-mutation-symlink-containment.test.ts — 8 failed / 2 passed before the fix, 10 passed after. The two that pass in both runs are the legitimate cases that must keep working (a symlink pointing at another file inside the workspace, a workspace reached through a symlinked root).
    • Existing packages/local-runtime/test/unit/diff-mutation-precheck.test.ts — 11 passed (no existing refusal weakened).
    • packages/webui/test/unit/diff-mutation-error.test.tsx — 22 passed.
    • node scripts/run-vitest-suite.mjs capability — 234 files, 5804 passed, 17 skipped, 0 failed.
    • node scripts/run-vitest-suite.mjs policy — 142 passed.
    • node scripts/run-vitest-suite.mjs webui — 3253 passed; 4–5 files are flaky on this machine and fail the same way on the base commit without this change (webui-design-tokens, webui-role-check, webui-host-event-generation, webui-workspace-archive, agent-transcript). The turn-diff webui test passes.
    • tsc -p tsconfig.standalone.json — clean.
    • node scripts/source-inventory.mjs — passed, 5419 files.
  • Performance: basic. The containment check adds one realpath per mutation target plus one for the workspace root, resolved once per run; the lexical fast reject still runs first, so an out-of-workspace path costs no syscall.
  • NOT RUN, platform limitations and live-service boundaries: the new suite skips on Windows, where creating a symlink needs privileges. The lexical refusals stay covered on every platform by the existing precheck suite. The WebUI review panel was not driven by hand; the mutation path is covered at the runtime boundary the WebUI calls.

Publication and contribution checks

  • I have permission to contribute these changes under the existing licenses applicable to the changed files/packages; imported material and its provenance are identified and existing notices are preserved.
  • No credentials, account data, real user content, internal source history or private review material is included.
  • Added/removed source files were reviewed before regenerating release/public-source.json; new tests are declared in test/vitest-suites.json where applicable.
  • Shared English/Chinese documentation and capability/verification records are updated where applicable. Mock/offline results are not described as live-service acceptance.

Maintainer handoff

Publication scope or license changes (if any): none.

Shared-source port: not needed.

Still open — for a decision, not fixed here

The capture layer shares the same lexical assumption, on the read side:

  • normalizeCapturePath / safeCapturedPath / snapshotCapturedFile (file-changes.ts) snapshot a target through a symlink, so a link inside the workspace to a file outside it puts the outside file's content into the turn-diff snapshot, the stored record and the review panel. Closing that is a different decision — the capture layer deliberately records out-of-workspace writes as absolute paths, and this change does not touch that contract.
  • Other lexical isInside / isPathInside workspace checks elsewhere in the repo have the same shape (e.g. packages/local-runtime/src/permissions/checkers.ts, packages/local-runtime/src/observability/diagnostic-bundle.ts, packages/shared/src/local-runtime-paths.ts, packages/local-runtime-v2/src/service/workspace/operations/workspace-path.ts). Each is a separate boundary with its own blast radius, and none of them is the turn-diff mutation.

The turn-diff revert/reapply resolved each target path lexically: the
resolved path was compared to the workspace root as a string. A path inside
the workspace whose final component is a symlink pointing outside it is
inside the workspace as a string and outside it as a file, and the write
follows the link — so a revert could write into a file outside the
workspace.

Resolve the target's real path and compare it with the workspace root's
real path. The lexical check stays as the cheap fast reject, so every
existing refusal (an absolute path, `.git/`, `.harness-def/`, the root
itself) is unchanged; the real-path check adds the symlink escape on top
of it.

The resolver follows the tool path guard
(`agent-tools/src/desktop/path-guard.ts`): realpath the target, and when it
does not exist yet, resolve its nearest existing ancestor and re-attach
the missing segments. A dangling symlink is followed by hand — realpath
fails on it the way it fails on a missing file, but a write through it
creates the file it points at.

The "Known boundary (lexical containment only)" note in the code is
removed: the boundary it described is closed.
@modacker
modacker merged commit 2bd9a2a into webui Oct 23, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant