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",