Skip to content

fix(local-runtime): keep turn-diff capture reads inside the workspace - #234

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

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

Conversation

@modacker

Copy link
Copy Markdown
Collaborator

What broke

packages/local-runtime/src/turns/file-changes.ts — the turn-diff capture layer — resolved the captured path lexically and then read it with fs.stat / fs.open. Both of those follow symlinks.

A path inside the workspace whose final component is a symlink to a file outside it passes the lexical check — the string is inside the workspace — and only then resolves outside it. The capture therefore put the content of a file outside the workspace into the turn-diff record, and from there into the database and into the WebUI review panel.

The write side of the same hole was closed by #232. The read side was still lexical.

Why it failed

normalizeCapturePath / safeCapturedPath answer the question "is this path inside the workspace as a string". snapshotCapturedFile took that answer as the containment decision and read the file.

With link.txt inside the workspace symlinked to outside/secret.txt, the persisted record held:

"content":"outside-secret-v1\n"
"diff":"-outside-secret-v1\n+outside-secret-v2-with-a-different-length\n"

What this changes

snapshotCapturedFile now asks a second question before it reads anything: does the path really live inside the workspace?

  • isCapturedPathInsideWorkspace compares the real path of the target with the real path of the workspace root, reusing the resolveRealPath / isInsideDirectory helpers fix(local-runtime): contain turn-diff mutation targets by real path #232 added to this file. The two helpers stay private to the module: reading and writing are the same boundary and the same file.
  • A target that escapes is still recorded — the turn did touch it, and the review panel should say so — but with metadata only: the size/mtime hash and the oversized marker this layer already reads as "no content was captured". The content is never read, never stored, never rendered.
  • Because the snapshot carries no content, the rewind path does not restore it either: assertSnapshotComplete rejects an oversized snapshot, and diff-rewind.ts turns that into a skipped entry.

Paths that resolve outside the workspace on purpose are unchanged: normalizeCapturePath hands back the absolute path for those, and that is the capture layer's deliberate record of "a write landed outside the workspace". Those keep their existing behaviour.

How it was checked

  • The fixtures drive the real capture service over the real SQLite store, so the assertions are about what actually lands in the persisted record — not about an internal helper's return value.
  • Before the fix: 3 failed | 3 passed. The failures carried "content":"outside-secret-v1\n" and the unified diff of the outside file.
  • After the fix: 6 passed — the three escapes (a file symlink, a symlinked directory, a relative symlink) plus three positive controls (a file inside the workspace, a symlink pointing at another file inside the workspace, and a workspace reached through a symlinked root).
  • diff-mutation-symlink-containment.test.ts (10) + diff-mutation-precheck.test.ts (11) + this file (6) → 27 passed.
  • packages/local-runtime/test → 38 files / 923 passed / 14 skipped.
  • npx tsc -p tsconfig.standalone.json --noEmit → exit 0.
  • node scripts/source-inventory.mjs → Public source check passed: 5420 files (the manifest is regenerated by the generator, not hand-edited).

Not addressed here

  • The race between the check and the read. The real path is resolved before the read, so a symlink swapped in between is still a race. Closing it needs a read that cannot follow a link (O_NOFOLLOW), which is a larger change than this refusal.
  • A hardlink is still not caught. realpath cannot tell a hardlink from a regular file: a hardlink inside the workspace that also points at a file outside it passes every path check, and writing to it changes the outside file. The path-based boundary does not cover it.
  • Deliberately-outside absolute paths still keep their content (see above). Tightening that is a product decision about the external marker, not a defect fix.
  • Windows has no coverage: the suite skips there, because creating a symlink needs privileges.

The turn-diff capture resolved a captured path lexically and then read it
with fs.stat/fs.open, both of which follow symlinks. A path inside the
workspace whose final component (or one of its parent directories) is a
symlink pointing outside it is inside the workspace as a string and outside
it as a file, so the capture put the content of a file outside the workspace
into the turn-diff record, and from there into the database and into the
WebUI review panel.

The write side of the same hole already resolves the real path of the target
and compares it with the real path of the workspace root (#232). The read
side now does the same before any content is read: the target's real path is
resolved, and a target that leaves the workspace is recorded by its metadata
only, never by its content. The path itself is still recorded, so the turn
is still visible in the review panel, and the snapshot is marked as having
no content, which also keeps the rewind path from restoring it.

A path that resolves outside the workspace in the first place is the
capture layer's deliberate record of an out-of-workspace write, and keeps
its existing behaviour; only the paths that merely look contained are
re-checked here.

Verified with a new test that drives the real capture service over the real
SQLite store: before the fix the persisted record held the outside file's
content and its unified diff, and after the fix it holds neither. The
positive controls (a file inside the workspace, a symlink that stays inside
the workspace, and a workspace reached through a symlinked root) still
capture their content. The existing diff-mutation suites and the whole
local-runtime suite pass (923 tests).

Known boundary: the real path is resolved before the read, so a symlink
swapped between the check and the read is still a race. Closing that needs a
read that cannot follow a link (O_NOFOLLOW), which is a larger change.
@modacker
modacker merged commit 90e0dce into webui Oct 23, 2026
8 checks passed
modacker added a commit that referenced this pull request Oct 23, 2026
…de packages/webui (#240)

The webui branch is a downstream client of MiniMax-AI/minimax-code. Changes
to the upstream packages do not flow back upstream and turn into unreviewed
divergence at the next upstream sync, so they do not belong here.

This removes the upstream-package half of the work merged today:

- packages/local-runtime/src/turns/file-changes.ts (#232, #234)
- packages/agent-tools/** (#238)
- packages/local-runtime-v2/src/service/workspace/operations/workspace-path.ts (#238)
- packages/local-runtime-v2/src/application/conversation/conversation-application.ts
  and .../stream/session-stream-service.ts (#237)

Everything under packages/webui stays: the queue-drain client half, the
regenerated visual baselines, the browser test budgets and the CJK font gate.

The inventories follow the files: eight removed paths leave
release/public-source.json and six removed suites leave
test/vitest-suites.json.

Co-authored-by: probe <probe@local>
@modacker
modacker deleted the fix/turn-diff-capture-symlink-containment branch October 23, 2026 17:40
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