From bd9bb67cb963fff81562ccd96aeaa7bde827406c Mon Sep 17 00:00:00 2001 From: probe Date: Fri, 23 Oct 2026 08:18:31 +0800 Subject: [PATCH] fix(local-runtime): contain turn-diff mutation targets by real path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../local-runtime/src/turns/file-changes.ts | 152 +++++++- .../diff-mutation-symlink-containment.test.ts | 332 ++++++++++++++++++ release/public-source.json | 1 + test/vitest-suites.json | 1 + 4 files changed, 472 insertions(+), 14 deletions(-) create mode 100644 packages/local-runtime/test/unit/diff-mutation-symlink-containment.test.ts diff --git a/packages/local-runtime/src/turns/file-changes.ts b/packages/local-runtime/src/turns/file-changes.ts index 69442b401..a5397051f 100644 --- a/packages/local-runtime/src/turns/file-changes.ts +++ b/packages/local-runtime/src/turns/file-changes.ts @@ -44,6 +44,14 @@ const PATCH_CONTEXT_LINES = 3; * past this budget we fall back to the whole-file deletion+addition hunk. */ const MAX_DIFF_EDIT_LENGTH = 2000; +/** + * Upper bound on the symlink hops followed while resolving a mutation + * target's real path. A path that needs more hops than this — a symlink + * cycle, or a chain long enough to look like one — is refused rather than + * followed: this is a containment boundary, so a path that cannot be + * resolved is not a safe path. + */ +const MAX_REAL_PATH_SYMLINK_HOPS = 32; const STRUCTURED_WRITE_TOOLS = new Set([ 'edit', 'write', @@ -406,9 +414,19 @@ export async function applyLocalTurnDiffSnapshotMutation( * * Safety first, ahead of the conflict check below: an operation that would * write outside the workspace is refused without even reading the files. */ + const workspaceRoot = path.resolve(record.workspaceDir); + /* The real workspace root, resolved once for the whole run: the + * containment check compares real paths, and the root does not change while + * the operation runs. A workspace that cannot be resolved refuses the run. */ + const realWorkspaceRoot = await resolveRealPath(workspaceRoot); + if (!realWorkspaceRoot) return { success: false, reason: 'unsafe_path' }; const targets: string[] = []; for (const undo of record.undo) { - const absolute = resolveMutationTargetPath(record.workspaceDir, undo.file); + const absolute = await resolveMutationTargetPath( + workspaceRoot, + realWorkspaceRoot, + undo.file, + ); if (!absolute) return { success: false, reason: 'unsafe_path' }; const target = undo[targetKey]; if (target.exists && target.content === undefined) { @@ -446,29 +464,135 @@ export async function applyLocalTurnDiffSnapshotMutation( * records out-of-workspace writes — so a truthy result is NOT proof of * containment and the mutation has to check it. It returns `undefined` for * the in-workspace paths the capture layer filters (`.git/`, - * `node_modules/`, the root itself), which is also a refusal here. The + * `.harness-def/`, the root itself), which is also a refusal here. The * containment re-check after `path.resolve` is deliberate: this is a * security boundary, so it does not rely on `normalizeCapturePath`'s * contract holding forever. * - * Known boundary (lexical containment only): a contained path that is a - * symlink pointing outside the workspace still resolves outside. Closing - * that needs `realpath` on every target with careful handling of - * not-yet-existing files, which is a larger change than this refusal. + * Containment is checked twice, and the second check is the one that binds: + * + * 1. Lexically, against the workspace root. It costs no syscall and + * refuses the plain escapes — an absolute path, a `..` climb, a sibling + * directory whose name starts with the workspace's name — before any + * path on disk is looked at. + * 2. Against the *real* paths, with symlinks resolved. 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 the write follows the link. The real + * path of the target is therefore compared with the real path of the + * workspace root, which also means a workspace that is itself reached + * through a symlink still contains its own files. + * + * `realWorkspaceRoot` is the already-resolved real root, passed in so a + * record with many entries does not resolve it once per entry. + * + * Known boundary (unchanged by this check): the real path is resolved before + * the write, so a symlink swapped between the check and the write is still + * a race. Closing that needs a write that cannot follow a link, which is a + * larger change than this refusal. */ -function resolveMutationTargetPath( - workspaceDir: string, +async function resolveMutationTargetPath( + workspaceRoot: string, + realWorkspaceRoot: string, file: string, -): string | undefined { - const normalized = normalizeCapturePath(workspaceDir, file); +): Promise { + const normalized = normalizeCapturePath(workspaceRoot, file); if (!normalized) return undefined; if (path.isAbsolute(normalized) || path.win32.isAbsolute(normalized)) { return undefined; } - const root = path.resolve(workspaceDir); - const absolute = path.resolve(root, normalized); - const prefix = root.endsWith(path.sep) ? root : `${root}${path.sep}`; - return absolute === root || absolute.startsWith(prefix) ? absolute : undefined; + const absolute = path.resolve(workspaceRoot, normalized); + const prefix = workspaceRoot.endsWith(path.sep) + ? workspaceRoot + : `${workspaceRoot}${path.sep}`; + if (absolute !== workspaceRoot && !absolute.startsWith(prefix)) return undefined; + const realTarget = await resolveRealPath(absolute); + if (!realTarget) return undefined; + return isInsideDirectory(realWorkspaceRoot, realTarget) ? absolute : undefined; +} + +/** + * Resolve a path to the real location the filesystem uses for it, following + * symlinks. + * + * The shape follows the tool path guard + * (`agent-tools/src/desktop/path-guard.ts`): `realpath` the target, and when + * the target does not exist yet — a revert recreates a deleted file — resolve + * its nearest existing ancestor and re-attach the missing segments. + * + * One thing that guard does not have to handle and this one does: the target + * may be a *dangling* symlink. `realpath` fails on it the same way it fails + * on a missing file, but a write through a dangling link creates the file it + * points at, so the link is followed by hand instead of being treated as a + * missing path. + * + * Returns `undefined` when the path cannot be resolved at all — a symlink + * cycle, a permission error — so the caller refuses the target instead of + * guessing where it points. + */ +async function resolveRealPath( + absolutePath: string, + hops = 0, +): Promise { + let candidate = path.resolve(absolutePath); + const suffixParts: string[] = []; + for (;;) { + try { + const ancestorReal = await fs.realpath(candidate); + return suffixParts.length === 0 + ? ancestorReal + : path.resolve(ancestorReal, ...suffixParts); + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code !== 'ENOENT' && code !== 'ENOTDIR') return undefined; + const link = await readlinkOrUndefined(candidate); + if (link !== undefined) { + if (hops >= MAX_REAL_PATH_SYMLINK_HOPS) return undefined; + const realParent = await realpathOrUndefined(path.dirname(candidate)); + if (!realParent) return undefined; + // A relative link is relative to the directory that holds it, which + // is the *real* directory when the link sits in a symlinked one. + const followed = path.isAbsolute(link) + ? path.resolve(link) + : path.resolve(realParent, link); + const resolved = await resolveRealPath(followed, hops + 1); + if (!resolved) return undefined; + return suffixParts.length ? path.resolve(resolved, ...suffixParts) : resolved; + } + const parent = path.dirname(candidate); + if (parent === candidate) return undefined; + suffixParts.unshift(path.basename(candidate)); + candidate = parent; + } + } +} + +/** + * Whether `candidate` is `root` itself or a path inside it. + * + * The comparison follows the tool path guard + * (`agent-tools/src/desktop/path-guard.ts`): `path.relative` decides, and it + * is case-insensitive on Windows, where the filesystem is. + */ +function isInsideDirectory(root: string, candidate: string): boolean { + const rel = path.relative(root, candidate); + return !(rel === '..' || rel.startsWith(`..${path.sep}`) || path.isAbsolute(rel)); +} + +async function realpathOrUndefined(target: string): Promise { + try { + return await fs.realpath(target); + } catch { + return undefined; + } +} + +async function readlinkOrUndefined(target: string): Promise { + try { + return await fs.readlink(target); + } catch { + return undefined; + } } function makeCaptureId(): string { diff --git a/packages/local-runtime/test/unit/diff-mutation-symlink-containment.test.ts b/packages/local-runtime/test/unit/diff-mutation-symlink-containment.test.ts new file mode 100644 index 000000000..717b7bf99 --- /dev/null +++ b/packages/local-runtime/test/unit/diff-mutation-symlink-containment.test.ts @@ -0,0 +1,332 @@ +// D-1 follow-up — the turn-diff revert/reapply must not write *through* a +// symlink that leaves the workspace. +// +// `resolveMutationTargetPath` checked containment lexically: it compared the +// resolved target path to the workspace root as a string. A path inside the +// workspace whose final component is a symlink pointing outside the workspace +// passes that check — the string is inside the workspace — and only then +// resolves outside it. `fs.writeFile` follows the link, so the revert wrote +// into a file outside the workspace. +// +// The fix resolves the *real* path of the target (symlinks included, missing +// tails attached to the nearest existing ancestor) and compares it with the +// *real* path of the workspace root, so the containment check follows the same +// path the write follows. +// +// Note on the conflict precheck: it reads the target through the same path, +// so on the OLD code this fixture reached the write loop with a passing +// conflict check — which is what makes "the file outside the workspace was +// rewritten" a real assertion rather than a vacuous one. +// +// Symlink creation needs privileges on Windows, so the whole suite skips +// there; the lexical refusals stay covered by `diff-mutation-precheck.test.ts`, +// which runs on every platform. + +import { createHash } from 'node:crypto'; +import { mkdir, mkdtemp, readFile, realpath, rm, symlink, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; + +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import type { + LocalTurnDiffRecord, + LocalTurnDiffSnapshotEntry, + LocalTurnDiffUndoEntry, +} from '../../src/persistence/ports.js'; +import { applyLocalTurnDiffSnapshotMutation } from '../../src/turns/file-changes.js'; + +let root: string; +let workspaceDir: string; +let outsideDir: string; +let outsideFile: string; + +/** The hash `snapshotCapturedFile` computes for a small text file. */ +function contentHash(content: string): string { + return createHash('sha256').update(content).digest('hex'); +} + +function snapshot(file: string, content: string): LocalTurnDiffSnapshotEntry { + return { file, exists: true, hash: contentHash(content), content }; +} + +function undoEntry( + file: string, + before: LocalTurnDiffSnapshotEntry, + after: LocalTurnDiffSnapshotEntry, +): LocalTurnDiffUndoEntry { + return { file, before, after }; +} + +function record( + undo: readonly LocalTurnDiffUndoEntry[], + workspace = workspaceDir, +): LocalTurnDiffRecord { + return { + changeSetId: 'changes-1', + sessionId: 'test-session', + turnId: 'turn-1', + workspaceDir: workspace, + capturedAtMs: 1_700_000_000_000, + status: 'active', + fileChanges: [], + undo: [...undo], + undoable: true, + }; +} + +async function read(file: string): Promise { + return readFile(path.join(workspaceDir, file), 'utf8'); +} + +beforeEach(async () => { + // `realpath` so the fixture root is the canonical path: on macOS the + // temporary directory itself sits behind a symlink, and a fixture that + // mixes the two spellings would not say anything about containment. + root = await realpath(await mkdtemp(path.join(tmpdir(), 'mcode-diff-symlink-'))); + workspaceDir = path.join(root, 'workspace'); + outsideDir = path.join(root, 'outside'); + outsideFile = path.join(outsideDir, 'secret.txt'); + await mkdir(workspaceDir, { recursive: true }); + await mkdir(outsideDir, { recursive: true }); + await writeFile(path.join(workspaceDir, 'one.ts'), 'one-v1'); + await writeFile(outsideFile, 'outside-v1'); +}); + +afterEach(async () => { + await rm(root, { recursive: true, force: true, maxRetries: 4 }); +}); + +describe.skipIf(process.platform === 'win32')( + 'applyLocalTurnDiffSnapshotMutation symlink containment', + () => { + it('refuses a revert that would write through a symlink pointing outside the workspace', async () => { + // The reported defect: `link.txt` lives inside the workspace, but it is + // a symlink to a file outside it. The lexical containment check accepts + // the path, and the OLD code then wrote the pre-turn content straight + // through the link into `outside/secret.txt`. + await symlink(outsideFile, path.join(workspaceDir, 'link.txt')); + + const result = await applyLocalTurnDiffSnapshotMutation( + record([ + undoEntry( + 'link.txt', + snapshot('link.txt', 'outside-v0'), + snapshot('link.txt', 'outside-v1'), + ), + ]), + 'revert', + ); + + expect(result).toEqual({ success: false, reason: 'unsafe_path' }); + // The file outside the workspace still holds its post-turn content. + expect(await readFile(outsideFile, 'utf8')).toBe('outside-v1'); + // The link itself is left alone: a refusal touches nothing on disk. + expect(await read('link.txt')).toBe('outside-v1'); + }); + + it('refuses a reapply that would write through the same symlink', async () => { + // `revert` and `reapply` are the same operation with the two snapshots + // swapped, and the containment check reads the entry's path — not the + // action — so the refusal has to be the same on both. + await symlink(outsideFile, path.join(workspaceDir, 'link.txt')); + + const result = await applyLocalTurnDiffSnapshotMutation( + record([ + undoEntry( + 'link.txt', + snapshot('link.txt', 'outside-v0'), + snapshot('link.txt', 'outside-v1'), + ), + ]), + 'reapply', + ); + + expect(result).toEqual({ success: false, reason: 'unsafe_path' }); + expect(await readFile(outsideFile, 'utf8')).toBe('outside-v1'); + }); + + it('refuses a revert that would write through a symlinked directory pointing outside', async () => { + // The escape does not have to be the final path component: a symlinked + // directory inside the workspace carries the write out of it just the + // same. + await symlink(outsideDir, path.join(workspaceDir, 'dir-link'), 'dir'); + + const result = await applyLocalTurnDiffSnapshotMutation( + record([ + undoEntry( + 'dir-link/secret.txt', + snapshot('dir-link/secret.txt', 'outside-v0'), + snapshot('dir-link/secret.txt', 'outside-v1'), + ), + ]), + 'revert', + ); + + expect(result).toEqual({ success: false, reason: 'unsafe_path' }); + expect(await readFile(outsideFile, 'utf8')).toBe('outside-v1'); + }); + + it('refuses a dangling symlink pointing outside the workspace', async () => { + // The link target does not exist, so there is nothing to read — but + // `fs.writeFile` would create it, outside the workspace. A containment + // check that only looks at existing files would miss this. + const dangling = path.join(outsideDir, 'not-created-yet.txt'); + await symlink(dangling, path.join(workspaceDir, 'dangling.txt')); + + const result = await applyLocalTurnDiffSnapshotMutation( + record([ + undoEntry( + 'dangling.txt', + snapshot('dangling.txt', 'created-v0'), + { file: 'dangling.txt', exists: false }, + ), + ]), + 'revert', + ); + + expect(result).toEqual({ success: false, reason: 'unsafe_path' }); + await expect(readFile(dangling, 'utf8')).rejects.toThrow(); + }); + + it('refuses a relative symlink that points outside the workspace', async () => { + // A relative link is relative to the directory that holds it, so this + // one leaves the workspace as surely as an absolute link does. + await symlink( + path.relative(workspaceDir, outsideFile), + path.join(workspaceDir, 'relative-link.txt'), + ); + + const result = await applyLocalTurnDiffSnapshotMutation( + record([ + undoEntry( + 'relative-link.txt', + snapshot('relative-link.txt', 'outside-v0'), + snapshot('relative-link.txt', 'outside-v1'), + ), + ]), + 'revert', + ); + + expect(result).toEqual({ success: false, reason: 'unsafe_path' }); + expect(await readFile(outsideFile, 'utf8')).toBe('outside-v1'); + }); + + it('refuses a symlink cycle instead of following it', async () => { + // The link points at itself. There is no real path to compare, so the + // target is refused — the containment check never resolves a path it + // cannot follow to the end. + await symlink(path.join(workspaceDir, 'cycle.txt'), path.join(workspaceDir, 'cycle.txt')); + + const result = await applyLocalTurnDiffSnapshotMutation( + record([ + undoEntry( + 'cycle.txt', + snapshot('cycle.txt', 'cycle-v0'), + snapshot('cycle.txt', 'cycle-v1'), + ), + ]), + 'revert', + ); + + expect(result).toEqual({ success: false, reason: 'unsafe_path' }); + }); + + it('still reverts a symlink that points at another file inside the workspace', async () => { + // The fix must not refuse every symlink: a link that stays inside the + // workspace is contained, so it reverts like any other file. + await writeFile(path.join(workspaceDir, 'target.ts'), 'target-v1'); + await symlink(path.join(workspaceDir, 'target.ts'), path.join(workspaceDir, 'link.ts')); + + const result = await applyLocalTurnDiffSnapshotMutation( + record([ + undoEntry( + 'link.ts', + snapshot('link.ts', 'target-v0'), + snapshot('link.ts', 'target-v1'), + ), + ]), + 'revert', + ); + + expect(result).toEqual({ success: true }); + expect(await readFile(path.join(workspaceDir, 'target.ts'), 'utf8')).toBe('target-v0'); + }); + + it('still reverts when the workspace is reached through a symlinked root', async () => { + // The workspace root itself can be a symlink (a linked checkout, a + // temporary directory behind `/var` on macOS). The containment check + // compares real paths on both sides, so a legitimate workspace is not + // refused for it. + const realWorkspace = path.join(root, 'real-workspace'); + const alias = path.join(root, 'workspace-alias'); + await mkdir(realWorkspace, { recursive: true }); + await writeFile(path.join(realWorkspace, 'one.ts'), 'one-v1'); + await symlink(realWorkspace, alias, 'dir'); + + const result = await applyLocalTurnDiffSnapshotMutation( + record( + [ + undoEntry('one.ts', snapshot('one.ts', 'one-v0'), snapshot('one.ts', 'one-v1')), + ], + alias, + ), + 'revert', + ); + + expect(result).toEqual({ success: true }); + expect(await readFile(path.join(realWorkspace, 'one.ts'), 'utf8')).toBe('one-v0'); + }); + + it('refuses a revert that escapes through a symlinked workspace root', async () => { + // The same root alias, but the target leaves the workspace through a + // symlink inside it: the alias must not launder the escape. + const realWorkspace = path.join(root, 'real-workspace'); + const alias = path.join(root, 'workspace-alias'); + await mkdir(realWorkspace, { recursive: true }); + await writeFile(path.join(realWorkspace, 'one.ts'), 'one-v1'); + await symlink(realWorkspace, alias, 'dir'); + await symlink(outsideFile, path.join(realWorkspace, 'link.txt')); + + const result = await applyLocalTurnDiffSnapshotMutation( + record( + [ + undoEntry( + 'link.txt', + snapshot('link.txt', 'outside-v0'), + snapshot('link.txt', 'outside-v1'), + ), + ], + alias, + ), + 'revert', + ); + + expect(result).toEqual({ success: false, reason: 'unsafe_path' }); + expect(await readFile(outsideFile, 'utf8')).toBe('outside-v1'); + }); + + it('refuses the whole run, so a safe file in the same record is not written either', async () => { + // The mutation is all-or-nothing: one unsafe target refuses the run + // before the first write, so the contained file in the same record + // keeps its post-turn content. + await symlink(outsideFile, path.join(workspaceDir, 'link.txt')); + + const result = await applyLocalTurnDiffSnapshotMutation( + record([ + undoEntry('one.ts', snapshot('one.ts', 'one-v0'), snapshot('one.ts', 'one-v1')), + undoEntry( + 'link.txt', + snapshot('link.txt', 'outside-v0'), + snapshot('link.txt', 'outside-v1'), + ), + ]), + 'revert', + ); + + expect(result).toEqual({ success: false, reason: 'unsafe_path' }); + expect(await read('one.ts')).toBe('one-v1'); + expect(await readFile(outsideFile, 'utf8')).toBe('outside-v1'); + }); + }, +); diff --git a/release/public-source.json b/release/public-source.json index fa2e2769a..99ade015f 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -2844,6 +2844,7 @@ "packages/local-runtime/test/unit/content-safety-turn-v2.test.ts", "packages/local-runtime/test/unit/cron/once-time.test.ts", "packages/local-runtime/test/unit/diff-mutation-precheck.test.ts", + "packages/local-runtime/test/unit/diff-mutation-symlink-containment.test.ts", "packages/local-runtime/test/unit/error-reporting-privacy.test.ts", "packages/local-runtime/test/unit/local-background-task-task-output-polling-reminder.test.ts", "packages/local-runtime/test/unit/local-background-task-user-stop-cascade.test.ts", diff --git a/test/vitest-suites.json b/test/vitest-suites.json index 8d9e14bc0..9454e2c8d 100644 --- a/test/vitest-suites.json +++ b/test/vitest-suites.json @@ -186,6 +186,7 @@ "packages/agent-modules/context-manager/test/compaction-usage.test.ts", "packages/local-runtime/test/unit/session-diff-fallback.test.ts", "packages/local-runtime/test/unit/diff-mutation-precheck.test.ts", + "packages/local-runtime/test/unit/diff-mutation-symlink-containment.test.ts", "test/history-processing.test.ts", "test/sqlite-message-contention.test.ts", "packages/tui/test/unit/tui-feature-viewport-contract.test.ts",