From 6d2da73b86fd5a7c023b06c037f25bc6b85d155d Mon Sep 17 00:00:00 2001 From: probe Date: Fri, 23 Oct 2026 08:48:47 +0800 Subject: [PATCH] fix(local-runtime): keep turn-diff capture reads inside the workspace 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. --- .../local-runtime/src/turns/file-changes.ts | 58 +++++ .../diff-capture-symlink-containment.test.ts | 234 ++++++++++++++++++ release/public-source.json | 1 + test/vitest-suites.json | 1 + 4 files changed, 294 insertions(+) create mode 100644 packages/local-runtime/test/unit/diff-capture-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 a5397051f..dbda2884e 100644 --- a/packages/local-runtime/src/turns/file-changes.ts +++ b/packages/local-runtime/src/turns/file-changes.ts @@ -838,6 +838,15 @@ function normalizeCapturePath(workspaceDir: string, filePath: string): string | return relative; } +/** + * The absolute path the capture layer may read, or `undefined` when the path + * is not one the capture layer handles at all. + * + * The result is a *lexical* answer: the path is inside the workspace as a + * string. Whether it is inside the workspace as a file is decided by + * `isCapturedPathInsideWorkspace`, which the caller has to ask before it + * reads anything. + */ function safeCapturedPath(workspaceDir: string, filePath: string): string | undefined { const normalized = normalizeCapturePath(workspaceDir, filePath); if (!normalized) return undefined; @@ -845,6 +854,39 @@ function safeCapturedPath(workspaceDir: string, filePath: string): string | unde return path.resolve(workspaceDir, normalized); } +/** + * Whether `absolute` really lives inside the workspace. + * + * Containment is decided on the *real* paths, not on the strings: 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, and `fs.stat`/`fs.open` follow the + * link. Reading such a path would 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 check is the read-side twin of the one the mutation does before it + * writes (`resolveMutationTargetPath`): the real path of the target is + * compared with the real path of the workspace root, so a workspace that is + * itself reached through a symlink still contains its own files, and a + * target that cannot be resolved at all is refused rather than guessed at. + * + * Known boundary (unchanged by this check): 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 than this refusal. + */ +async function isCapturedPathInsideWorkspace( + workspaceDir: string, + absolute: string, +): Promise { + const realRoot = await resolveRealPath(path.resolve(workspaceDir)); + if (!realRoot) return false; + const realTarget = await resolveRealPath(absolute); + if (!realTarget) return false; + return isInsideDirectory(realRoot, realTarget); +} + async function snapshotCapturedFile( workspaceDir: string, filePath: string, @@ -852,6 +894,13 @@ async function snapshotCapturedFile( ): Promise { const normalized = normalizeCapturePath(workspaceDir, filePath); if (!normalized) return { file: filePath, exists: false }; + /* An *absolute* normalized path is the capture layer's deliberate record + * of a write that landed outside the workspace: `normalizeCapturePath` + * hands the absolute path back for those, and they keep their existing + * behaviour. A workspace-relative path is the one that has to be + * re-checked by real path below — it only *looks* contained, and a + * symlink inside it can point anywhere. */ + const outsideWorkspace = path.isAbsolute(normalized) || path.win32.isAbsolute(normalized); const absolute = safeCapturedPath(workspaceDir, normalized); if (!absolute) return { file: normalized, exists: false }; try { @@ -863,6 +912,15 @@ async function snapshotCapturedFile( hash: metadataSnapshotHash(stats), sizeBytes: stats.size, }; + if (!outsideWorkspace && !(await isCapturedPathInsideWorkspace(workspaceDir, absolute))) { + /* The path is inside the workspace as a string and outside it as a + * file: a symlink. Record that something is there — the turn did + * touch it — but never its content. `oversized` is the marker the + * rest of this layer already reads as "no content was captured", so + * the snapshot is not restorable either, which is what we want: the + * mutation refuses such a target anyway. */ + return { ...metadataEntry, oversized: true }; + } if (stats.size > MAX_TEXT_SNAPSHOT_BYTES) return { ...metadataEntry, oversized: true }; if (budget && stats.size > budget.remainingTextBytes) { return { ...metadataEntry, oversized: true }; diff --git a/packages/local-runtime/test/unit/diff-capture-symlink-containment.test.ts b/packages/local-runtime/test/unit/diff-capture-symlink-containment.test.ts new file mode 100644 index 000000000..35ff7c979 --- /dev/null +++ b/packages/local-runtime/test/unit/diff-capture-symlink-containment.test.ts @@ -0,0 +1,234 @@ +// D-1 follow-up — the turn-diff *capture* must not read the content of a +// file outside the workspace through a symlink. +// +// The write side of the same hole is closed by PR #232: the mutation refuses +// a target whose real path leaves the workspace. The read side was still +// lexical. `snapshotCapturedFile` resolved the captured path with +// `path.resolve` and then read it with `fs.stat`/`fs.open`, and 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 fixtures below 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. +// +// Symlink creation needs privileges on Windows, so the whole suite skips +// there. + +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 { closeLocalRuntimeDb } from '../../src/persistence/db.js'; +import type { LocalTurnDiffRecord } from '../../src/persistence/ports.js'; +import { SqliteLocalTurnDiffStore } from '../../src/persistence/sqlite-persistence.js'; +import type { LocalSessionRecord } from '../../src/sessions/controller.js'; +import { LocalTurnFileChangeCaptureService } from '../../src/turns/file-changes.js'; + +const SESSION_ID = 'test-session'; +const TURN_ID = 'turn-1'; +const ASSISTANT_MESSAGE_ID = 'msg-1'; +/** A sentinel that appears nowhere else, so "the content leaked" is exact. */ +const OUTSIDE_V1 = 'outside-secret-v1\n'; +const OUTSIDE_V2 = 'outside-secret-v2-with-a-different-length\n'; + +let root: string; +let workspaceDir: string; +let outsideDir: string; +let outsideFile: string; +let dataDir: string; +let store: SqliteLocalTurnDiffStore; +let service: LocalTurnFileChangeCaptureService; +let idCounter: number; + +/** + * Run one turn's worth of capture over the real store and return the + * persisted record. + * + * The mutation between the two snapshots stands in for the agent's write: + * the capture reads the file *before* the tool runs and again *after* it. + */ +async function captureTurn( + filePath: string, + mutate: () => Promise, + workspace: string = workspaceDir, +): Promise { + const session = { + sessionId: SESSION_ID, + agentName: 'test-agent', + workspaceDir: workspace, + } as LocalSessionRecord; + await service.begin(session, TURN_ID); + await service.captureToolUseStart({ + sessionId: SESSION_ID, + toolName: 'write', + toolCallId: 'call-1', + toolArgs: { file_path: filePath }, + }); + await mutate(); + await service.captureToolUseFinish({ + sessionId: SESSION_ID, + toolName: 'write', + toolCallId: 'call-1', + toolArgs: { file_path: filePath }, + }); + await service.finalizeLatestPending(SESSION_ID, { assistantMessageId: ASSISTANT_MESSAGE_ID }); + return store.getByTurn(SESSION_ID, TURN_ID); +} + +/** The undo entry the capture recorded for one path. */ +function undoEntry(record: LocalTurnDiffRecord | undefined, file: string) { + return record?.undo.find((entry) => entry.file === file); +} + +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-capture-'))); + workspaceDir = path.join(root, 'workspace'); + outsideDir = path.join(root, 'outside'); + outsideFile = path.join(outsideDir, 'secret.txt'); + dataDir = path.join(root, 'data'); + await mkdir(workspaceDir, { recursive: true }); + await mkdir(outsideDir, { recursive: true }); + await mkdir(dataDir, { recursive: true }); + await writeFile(path.join(workspaceDir, 'inside.ts'), 'inside-v1\n'); + await writeFile(outsideFile, OUTSIDE_V1); + idCounter = 0; + store = new SqliteLocalTurnDiffStore(dataDir); + service = new LocalTurnFileChangeCaptureService(store, { + nowMs: () => 1_700_000_000_000, + makeId: (prefix) => `${prefix}-${(idCounter += 1)}`, + }); +}); + +afterEach(async () => { + closeLocalRuntimeDb(dataDir); + await rm(root, { recursive: true, force: true, maxRetries: 4 }); +}); + +describe.skipIf(process.platform === 'win32')( + 'turn-diff capture symlink containment', + () => { + it('does not capture the content of a file outside the workspace reached through a symlink', async () => { + // The reported defect: `link.txt` lives inside the workspace, but it + // is a symlink to a file outside it. The OLD code read the link's + // target and put its content into the turn-diff record. + await symlink(outsideFile, path.join(workspaceDir, 'link.txt')); + + const record = await captureTurn('link.txt', async () => { + // The agent's write lands outside the workspace, through the link. + await writeFile(outsideFile, OUTSIDE_V2); + }); + + expect(record).toBeDefined(); + // Nothing of the file outside the workspace reaches the record. + expect(JSON.stringify(record)).not.toContain('outside-secret'); + const entry = undoEntry(record, 'link.txt'); + expect(entry?.before.content).toBeUndefined(); + expect(entry?.after.content).toBeUndefined(); + // The path itself is still recorded: the turn did touch it, and the + // review panel should say so even though it cannot show the content. + expect(entry?.before.exists).toBe(true); + expect(entry?.after.exists).toBe(true); + // The outside file is untouched by the capture itself. + expect(await readFile(outsideFile, 'utf8')).toBe(OUTSIDE_V2); + }); + + it('does not capture the content of a file outside the workspace reached through a symlinked directory', async () => { + // The escape does not have to be the final path component: a symlinked + // directory inside the workspace carries the read out of it just the + // same. + await symlink(outsideDir, path.join(workspaceDir, 'dir-link'), 'dir'); + + const record = await captureTurn('dir-link/secret.txt', async () => { + await writeFile(outsideFile, OUTSIDE_V2); + }); + + expect(record).toBeDefined(); + expect(JSON.stringify(record)).not.toContain('outside-secret'); + const entry = undoEntry(record, 'dir-link/secret.txt'); + expect(entry?.before.content).toBeUndefined(); + expect(entry?.after.content).toBeUndefined(); + }); + + it('does not capture the content of a file outside the workspace reached through a relative symlink', 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 record = await captureTurn('relative-link.txt', async () => { + await writeFile(outsideFile, OUTSIDE_V2); + }); + + expect(record).toBeDefined(); + expect(JSON.stringify(record)).not.toContain('outside-secret'); + expect(undoEntry(record, 'relative-link.txt')?.after.content).toBeUndefined(); + }); + + it('still captures a file inside the workspace', async () => { + // The positive control: the containment check must not stop the + // capture from doing its job. + const record = await captureTurn('inside.ts', async () => { + await writeFile(path.join(workspaceDir, 'inside.ts'), 'inside-v2\n'); + }); + + expect(record).toBeDefined(); + const entry = undoEntry(record, 'inside.ts'); + expect(entry?.before.content).toBe('inside-v1\n'); + expect(entry?.after.content).toBe('inside-v2\n'); + expect(record?.fileChanges[0]?.diff).toContain('inside-v2'); + }); + + it('still captures 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 is captured like any other file. + await writeFile(path.join(workspaceDir, 'target.ts'), 'target-v1\n'); + await symlink(path.join(workspaceDir, 'target.ts'), path.join(workspaceDir, 'link.ts')); + + const record = await captureTurn('link.ts', async () => { + await writeFile(path.join(workspaceDir, 'target.ts'), 'target-v2\n'); + }); + + expect(record).toBeDefined(); + const entry = undoEntry(record, 'link.ts'); + expect(entry?.before.content).toBe('target-v1\n'); + expect(entry?.after.content).toBe('target-v2\n'); + }); + + it('still captures through a workspace 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, 'inside.ts'), 'inside-v1\n'); + await symlink(realWorkspace, alias, 'dir'); + + const record = await captureTurn( + 'inside.ts', + async () => { + await writeFile(path.join(realWorkspace, 'inside.ts'), 'inside-v2\n'); + }, + alias, + ); + + expect(record).toBeDefined(); + const entry = undoEntry(record, 'inside.ts'); + expect(entry?.before.content).toBe('inside-v1\n'); + expect(entry?.after.content).toBe('inside-v2\n'); + }); + }, +); diff --git a/release/public-source.json b/release/public-source.json index 99ade015f..e2330026a 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -2843,6 +2843,7 @@ "packages/local-runtime/test/unit/content-safety-api-v2.test.ts", "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-capture-symlink-containment.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", diff --git a/test/vitest-suites.json b/test/vitest-suites.json index 9454e2c8d..6e07b3e32 100644 --- a/test/vitest-suites.json +++ b/test/vitest-suites.json @@ -187,6 +187,7 @@ "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", + "packages/local-runtime/test/unit/diff-capture-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",