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