diff --git a/packages/agent-tools/src/cloud/matrix-tools/path-guard.ts b/packages/agent-tools/src/cloud/matrix-tools/path-guard.ts index 01eb1c1a1..bac05e87e 100644 --- a/packages/agent-tools/src/cloud/matrix-tools/path-guard.ts +++ b/packages/agent-tools/src/cloud/matrix-tools/path-guard.ts @@ -1,18 +1,5 @@ -/** - * Workspace path boundary (cloud version). Cloud tools must pass this check before fs access to - * prevent the LLM escaping the working directory with `..`, absolute paths, or symlinks. - * - * NOTE(shared-logic): Equivalent to `../../desktop/path-guard.ts`. Each side currently keeps a - * copy to preserve local / cloud tool directory isolation (AGENTS.md §1), so the two must be - * changed together. The real-path resolution both copies now use lives in - * `../../shared/workspace-real-path.ts` — one implementation, so the copies cannot drift apart on - * the part that decides whether a symlink escapes. - */ - import { realpath } from 'node:fs/promises'; -import { isAbsolute, resolve } from 'node:path'; - -import { isInsideDirectory, resolveRealPath } from '../../shared/workspace-real-path.js'; +import { basename, dirname, isAbsolute, relative, resolve, sep } from 'node:path'; /** * Input-path fence scope. A plain string keeps the legacy workspace-only @@ -69,12 +56,31 @@ export async function assertWithinWorkspace( ): Promise { const rootReal = await realpath(workspaceRoot); - // `resolveRealPath` follows symlinks — dangling ones included. A plain - // `realpath` walk treated a dangling link as a missing file and compared the - // lexical spelling, so a link pointing outside the workspace passed. - const targetReal = await resolveRealPath(absolutePath); - if (targetReal === undefined || !isInsideDirectory(rootReal, targetReal)) { - throw new Error(`Workspace path escapes Matrix tool root: ${absolutePath}`); + let candidate = resolve(absolutePath); + const suffixParts: string[] = []; + while (true) { + let ancestorReal: string; + try { + ancestorReal = await realpath(candidate); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code !== 'ENOENT' && code !== 'ENOTDIR') throw err; + const parent = dirname(candidate); + if (parent === candidate) { + throw new Error(`Workspace path escapes Matrix tool root: ${absolutePath}`); + } + suffixParts.unshift(basename(candidate)); + candidate = parent; + continue; + } + + const finalPath = + suffixParts.length === 0 ? ancestorReal : resolve(ancestorReal, ...suffixParts); + const rel = relative(rootReal, finalPath); + if (rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel)) { + throw new Error(`Workspace path escapes Matrix tool root: ${absolutePath}`); + } + return; } } diff --git a/packages/agent-tools/src/desktop/local-pi-tools.ts b/packages/agent-tools/src/desktop/local-pi-tools.ts index 9e8a0a486..d044552e3 100644 --- a/packages/agent-tools/src/desktop/local-pi-tools.ts +++ b/packages/agent-tools/src/desktop/local-pi-tools.ts @@ -17,12 +17,11 @@ import { import { getShellConfig } from '@earendil-works/pi-coding-agent/shell'; import type { AgentTool, AgentToolResult } from '@earendil-works/pi-agent-core'; import { access } from 'node:fs/promises'; -import { isAbsolute, join, resolve as resolvePath } from 'node:path'; +import { isAbsolute, resolve as resolvePath } from 'node:path'; import { homedir, release } from 'node:os'; import { fileURLToPath } from 'node:url'; import { randomUUID } from 'node:crypto'; import { resolveBashDescription } from './local-bash-input.js'; -import { findWorkspaceFenceViolation } from './workspace-fence.js'; import { DEFAULT_FOREGROUND_BASH_SOFT_YIELD_MS, resolveLocalBashTiming, @@ -125,20 +124,6 @@ export class LocalReadTool implements ToolImpl< if (signal?.aborted) throw new Error('Operation aborted'); const readAbs = await resolveLocalReadPath(input.path, this.workspaceRoot); - // Workspace fence: the file the tool touches has to really live inside - // the workspace. A symlink inside the workspace would otherwise let the - // read reach out of it, and the permission layer cannot see that because - // it compares paths lexically. Validation only — pi still receives the - // lexical path below, so the ledger key stays aligned with pi's own - // `resolveToCwd` output. - const fenceViolation = await findWorkspaceFenceViolation( - [...piReadPathCandidates(input.path, this.workspaceRoot), readAbs], - this.workspaceRoot, - ); - if (fenceViolation) { - return workspaceFenceErrorResult(LocalReadToolDef.name, fenceViolation); - } - // Guard order (design doc §3): the device check comes first because // every later step that opens the file (binary sampling, PDF/notebook // parsing, pi itself) would hang forever on /dev/zero-style paths. @@ -266,17 +251,6 @@ export class LocalWriteTool implements ToolImpl< signal?: AbortSignal, ): Promise { if (signal?.aborted) throw new Error('Operation aborted'); - // Workspace fence: pi writes the path it resolves from `input.path`, so - // that is the path the fence has to check. A symlinked directory or a - // dangling symlink would otherwise create the file outside the - // workspace. - const fenceViolation = await findWorkspaceFenceViolation( - [piResolvedPath(input.path, this.workspaceRoot)], - this.workspaceRoot, - ); - if (fenceViolation) { - return workspaceFenceErrorResult(LocalWriteToolDef.name, fenceViolation); - } // Per-execute capture context. This tool instance is a process-level // singleton (buildLocalToolRegistry), so an instance-level pending map // would be shared across concurrent writes; binding capture to a single @@ -312,20 +286,6 @@ export class LocalEditTool implements ToolImpl< ): Promise { if (signal?.aborted) throw new Error('Operation aborted'); - // Workspace fence: pi edits the path it resolves from `input.file_path`, - // and the `replace_all` branch below reads that path itself — so both - // spellings have to be inside the workspace for real. - const fenceViolation = await findWorkspaceFenceViolation( - [ - piResolvedPath(input.file_path, this.workspaceRoot), - resolveLocalCandidatePath(input.file_path, this.workspaceRoot), - ], - this.workspaceRoot, - ); - if (fenceViolation) { - return workspaceFenceErrorResult(LocalEditToolDef.name, fenceViolation); - } - // Translate the compatible style public schema (file_path + old_string + // new_string + optional replace_all) into pi's internal // { path, edits: [{ oldText, newText }] } shape so we still get @@ -827,61 +787,6 @@ function resolveLocalCandidatePath(input: string, workspaceRoot: string): string return normalizeLocalExistingPath(isAbsolute(path) ? path : resolvePath(workspaceRoot, path)); } -/** - * The absolute path pi itself opens for `input`. - * - * Mirrors pi's `resolveToCwd` (`normalizePath` + `path.resolve` against the - * tool cwd): unicode-space normalization, the `@` prefix strip, `~` expansion - * and `file://` URLs all happen before the path is resolved. The fence has to - * check the same target pi touches — otherwise a spelling pi rewrites could - * still reach outside the workspace. - */ -function piResolvedPath(input: string, workspaceRoot: string): string { - let normalized = input.replace(/[\u00a0\u2000-\u200a\u202f\u205f\u3000]/g, ' '); - if (normalized.startsWith('@')) normalized = normalized.slice(1); - if (normalized === '~') return resolvePath(homedir()); - if (normalized.startsWith('~/') || (process.platform === 'win32' && normalized.startsWith('~\\'))) { - normalized = join(homedir(), normalized.slice(2)); - } - if (/^file:\/\//.test(normalized)) normalized = fileURLToPath(normalized); - return isAbsolute(normalized) ? resolvePath(normalized) : resolvePath(workspaceRoot, normalized); -} - -/** - * Every path the read tool can end up opening for `input`. - * - * pi's read tool resolves the requested path and, when that spelling does not - * exist, retries the macOS-flavoured ones (narrow no-break space before - * AM/PM, NFD, curly quote). The desktop wrapper resolves the same variants - * for its own dispatch (PDF / notebook / binary sniffing). The fence checks - * all of them: a variant spelling must not be a way around it. - */ -function piReadPathCandidates(input: string, workspaceRoot: string): string[] { - const resolved = piResolvedPath(input, workspaceRoot); - const amPm = resolved.replace(/ (AM|PM)\./gi, '\u202f$1.'); - const nfd = resolved.normalize('NFD'); - const curly = resolved.replace(/'/g, '\u2019'); - const nfdCurly = nfd.replace(/'/g, '\u2019'); - return [resolved, amPm, nfd, curly, nfdCurly]; -} - -/** - * Uniform, recoverable tool error for a workspace-fence violation. - * - * The model sees it as a tool error it can react to — the same channel the - * other wrapper-level guards use — instead of a thrown exception that - * arrives without structured details. - */ -function workspaceFenceErrorResult(toolName: string, reason: string): ToolResult { - return { - tool_name: toolName, - text: reason, - content: [{ type: 'text', text: reason }], - details: { status: 'workspace_fence_violation', error_code: 'WORKSPACE_PATH_ESCAPE' }, - isError: true, - }; -} - async function resolveLocalReadPath(input: string, workspaceRoot: string): Promise { const literalResolved = resolveLocalCandidatePath(input, workspaceRoot); if (await pathExists(literalResolved)) return literalResolved; diff --git a/packages/agent-tools/src/desktop/path-guard.test.ts b/packages/agent-tools/src/desktop/path-guard.test.ts deleted file mode 100644 index 7d4227768..000000000 --- a/packages/agent-tools/src/desktop/path-guard.test.ts +++ /dev/null @@ -1,84 +0,0 @@ -import { mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises'; -import { tmpdir } from 'node:os'; -import { join } from 'node:path'; - -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; - -import { resolveWithinWorkspace } from './path-guard.js'; - -/** - * The desktop path guard has to hold for symlinks, not just for `..` and - * absolute paths: a link inside the workspace that points outside it is the - * shape an escape actually takes. - */ -describe('desktop workspace path guard', () => { - let workspace: string; - let outside: string; - - beforeEach(async () => { - workspace = await mkdtemp(join(tmpdir(), 'path-guard-ws-')); - outside = await mkdtemp(join(tmpdir(), 'path-guard-out-')); - }); - - afterEach(async () => { - await rm(workspace, { recursive: true, force: true }); - await rm(outside, { recursive: true, force: true }); - }); - - it('rejects a symlink that points outside the workspace', async () => { - await writeFile(join(outside, 'secret.txt'), 'secret\n'); - await symlink(join(outside, 'secret.txt'), join(workspace, 'escape.txt')); - - await expect(resolveWithinWorkspace('escape.txt', workspace)).rejects.toThrow( - /escapes local runtime root/, - ); - }); - - it('rejects a dangling symlink that points outside the workspace', async () => { - await symlink(join(outside, 'not-created-yet.txt'), join(workspace, 'dangling.txt')); - - await expect(resolveWithinWorkspace('dangling.txt', workspace)).rejects.toThrow( - /escapes local runtime root/, - ); - }); - - it('rejects a path whose parent directory is a symlink pointing outside', async () => { - await mkdir(join(outside, 'sub'), { recursive: true }); - await symlink(join(outside, 'sub'), join(workspace, 'outdir')); - - await expect(resolveWithinWorkspace('outdir/file.txt', workspace)).rejects.toThrow( - /escapes local runtime root/, - ); - }); - - it('rejects a lexical escape', async () => { - await expect(resolveWithinWorkspace('../outside/secret.txt', workspace)).rejects.toThrow( - /escapes local runtime root/, - ); - }); - - it('allows a symlink that points inside the workspace', async () => { - await writeFile(join(workspace, 'real.txt'), 'inside\n'); - await symlink(join(workspace, 'real.txt'), join(workspace, 'inside-link.txt')); - - await expect(resolveWithinWorkspace('inside-link.txt', workspace)).resolves.toBe( - join(workspace, 'inside-link.txt'), - ); - }); - - it('allows a dangling symlink that points inside the workspace', async () => { - await symlink(join(workspace, 'not-created-yet.txt'), join(workspace, 'inside-dangling.txt')); - - await expect(resolveWithinWorkspace('inside-dangling.txt', workspace)).resolves.toBe( - join(workspace, 'inside-dangling.txt'), - ); - }); - - it('allows a workspace root that is itself a symlink', async () => { - await writeFile(join(workspace, 'real.txt'), 'inside\n'); - const alias = join(outside, 'ws-alias'); - await symlink(workspace, alias); - - await expect(resolveWithinWorkspace('real.txt', alias)).resolves.toBe(join(alias, 'real.txt')); - }); -}); diff --git a/packages/agent-tools/src/desktop/path-guard.ts b/packages/agent-tools/src/desktop/path-guard.ts index aca396e3b..4a1c20d3a 100644 --- a/packages/agent-tools/src/desktop/path-guard.ts +++ b/packages/agent-tools/src/desktop/path-guard.ts @@ -2,27 +2,44 @@ * Workspace path boundary (desktop local version). Local tools must pass this check before fs * access to prevent the LLM escaping the working directory with `..`, absolute paths, or symlinks. * - * NOTE(shared-logic): Equivalent to `../cloud/matrix-tools/path-guard.ts`. Each side currently - * keeps a copy to preserve local / cloud tool directory isolation (AGENTS.md §1), so the two must - * be changed together. The real-path resolution both copies now use lives in - * `../shared/workspace-real-path.ts` — one implementation, so the copies cannot drift apart on - * the part that decides whether a symlink escapes. + * NOTE(shared-logic): Equivalent to `../cloud/path-guard.ts`. Each side currently keeps a copy to + * preserve local / cloud tool directory isolation (AGENTS.md §1). This is a prime candidate for + * future shared-logic extraction. */ import { realpath } from 'node:fs/promises'; -import { isAbsolute, resolve } from 'node:path'; - -import { isInsideDirectory, resolveRealPath } from '../shared/workspace-real-path.js'; +import { basename, dirname, isAbsolute, relative, resolve, sep } from 'node:path'; async function assertWithinWorkspace(absolutePath: string, workspaceRoot: string): Promise { const rootReal = await realpath(workspaceRoot); - // `resolveRealPath` follows symlinks — dangling ones included. A plain - // `realpath` walk treated a dangling link as a missing file and compared the - // lexical spelling, so a link pointing outside the workspace passed. - const targetReal = await resolveRealPath(absolutePath); - if (targetReal === undefined || !isInsideDirectory(rootReal, targetReal)) { - throw new Error(`Workspace path escapes local runtime root: ${absolutePath}`); + let candidate = resolve(absolutePath); + const suffixParts: string[] = []; + while (true) { + let ancestorReal: string; + try { + ancestorReal = await realpath(candidate); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code !== 'ENOENT' && code !== 'ENOTDIR') { + throw err; + } + const parent = dirname(candidate); + if (parent === candidate) { + throw new Error(`Workspace path escapes local runtime root: ${absolutePath}`); + } + suffixParts.unshift(basename(candidate)); + candidate = parent; + continue; + } + + const finalPath = + suffixParts.length === 0 ? ancestorReal : resolve(ancestorReal, ...suffixParts); + const rel = relative(rootReal, finalPath); + if (rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel)) { + throw new Error(`Workspace path escapes local runtime root: ${absolutePath}`); + } + return; } } diff --git a/packages/agent-tools/src/desktop/workspace-fence.test.ts b/packages/agent-tools/src/desktop/workspace-fence.test.ts deleted file mode 100644 index 1cd59543f..000000000 --- a/packages/agent-tools/src/desktop/workspace-fence.test.ts +++ /dev/null @@ -1,202 +0,0 @@ -import { mkdir, mkdtemp, readFile, rm, symlink, writeFile } from 'node:fs/promises'; -import { tmpdir } from 'node:os'; -import { join } from 'node:path'; - -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; - -import { readPluginHookCompatibleToolResponse } from '../plugin-hooks/vendor-tool-response.js'; -import { LocalEditTool, LocalReadTool, LocalWriteTool } from './local-pi-tools.js'; - -const context = { sessionId: 'workspace-fence-session', turnId: 'workspace-fence-turn' }; - -const OUTSIDE_SECRET = 'TOP-SECRET-OUTSIDE\n'; - -/** - * The desktop fs tools resolve their target lexically and hand that spelling - * to pi, so a symlink inside the workspace used to reach out of it — and the - * permission layer could not see it, because it compares paths lexically too. - * - * The fence closes that. These tests pin both halves of it: the escape is - * refused, and everything that legitimately lives inside the workspace keeps - * working — including the paths that are symlinks. - */ -describe('desktop workspace fence', () => { - let workspace: string; - let outside: string; - - beforeEach(async () => { - workspace = await mkdtemp(join(tmpdir(), 'workspace-fence-ws-')); - outside = await mkdtemp(join(tmpdir(), 'workspace-fence-out-')); - }); - - afterEach(async () => { - await rm(workspace, { recursive: true, force: true }); - await rm(outside, { recursive: true, force: true }); - }); - - async function fileExists(path: string): Promise { - try { - await readFile(path, 'utf8'); - return true; - } catch { - return false; - } - } - - it('refuses to read a file the workspace symlinks to', async () => { - await writeFile(join(outside, 'secret.txt'), OUTSIDE_SECRET); - await symlink(join(outside, 'secret.txt'), join(workspace, 'escape.txt')); - - const result = await new LocalReadTool(workspace).execute(context, { path: 'escape.txt' }); - - expect(result.isError).toBe(true); - expect(result.text).toContain('outside the workspace'); - expect(result.text).not.toContain('TOP-SECRET-OUTSIDE'); - }); - - it('refuses to write through a symlinked directory', async () => { - await symlink(outside, join(workspace, 'outlink')); - - const result = await new LocalWriteTool(workspace).execute(context, { - path: 'outlink/pwned.txt', - content: 'PWNED-BY-AGENT\n', - }); - - expect(result.isError).toBe(true); - expect(result.text).toContain('outside the workspace'); - expect(await fileExists(join(outside, 'pwned.txt'))).toBe(false); - }); - - it('refuses to edit a file the workspace symlinks to', async () => { - await writeFile(join(outside, 'target.ts'), 'const value = 1;\n'); - await symlink(join(outside, 'target.ts'), join(workspace, 'target.ts')); - - const result = await new LocalEditTool(workspace).execute(context, { - file_path: 'target.ts', - old_string: 'const value = 1;', - new_string: 'const value = 99;', - }); - - expect(result.isError).toBe(true); - expect(result.text).toContain('outside the workspace'); - expect(await readFile(join(outside, 'target.ts'), 'utf8')).toBe('const value = 1;\n'); - }); - - it('refuses to write through a dangling symlink that points outside', async () => { - await symlink(join(outside, 'created-by-dangling.txt'), join(workspace, 'dangling.txt')); - - const result = await new LocalWriteTool(workspace).execute(context, { - path: 'dangling.txt', - content: 'PWNED-BY-AGENT\n', - }); - - expect(result.isError).toBe(true); - expect(result.text).toContain('outside the workspace'); - expect(await fileExists(join(outside, 'created-by-dangling.txt'))).toBe(false); - }); - - it('refuses to read through a directory symlink that points outside', async () => { - await mkdir(join(outside, 'sub'), { recursive: true }); - await writeFile(join(outside, 'sub', 'secret.txt'), OUTSIDE_SECRET); - await symlink(join(outside, 'sub'), join(workspace, 'outdir')); - - const result = await new LocalReadTool(workspace).execute(context, { - path: 'outdir/secret.txt', - }); - - expect(result.isError).toBe(true); - expect(result.text).toContain('outside the workspace'); - expect(result.text).not.toContain('TOP-SECRET-OUTSIDE'); - }); - - it('still reads a symlink that points inside the workspace', async () => { - await writeFile(join(workspace, 'real.txt'), 'inside the workspace\n'); - await symlink(join(workspace, 'real.txt'), join(workspace, 'inside-link.txt')); - - const result = await new LocalReadTool(workspace).execute(context, { - path: 'inside-link.txt', - }); - - expect(result.isError).toBeUndefined(); - expect(result.text).toContain('inside the workspace'); - }); - - it('still writes and edits through a symlink that points inside the workspace', async () => { - await writeFile(join(workspace, 'real.ts'), 'const value = 1;\n'); - await symlink(join(workspace, 'real.ts'), join(workspace, 'inside-link.ts')); - - const write = await new LocalWriteTool(workspace).execute(context, { - path: 'inside-link.ts', - content: 'const value = 7;\n', - }); - expect(write.isError).toBeUndefined(); - expect(await readFile(join(workspace, 'real.ts'), 'utf8')).toBe('const value = 7;\n'); - - const edit = await new LocalEditTool(workspace).execute(context, { - file_path: 'inside-link.ts', - old_string: 'const value = 7;', - new_string: 'const value = 8;', - }); - expect(edit.isError).toBeUndefined(); - expect(await readFile(join(workspace, 'real.ts'), 'utf8')).toBe('const value = 8;\n'); - }); - - it('still works when the workspace root is itself a symlink', async () => { - await writeFile(join(workspace, 'real.txt'), 'inside the workspace\n'); - const alias = join(outside, 'ws-alias'); - await symlink(workspace, alias); - - const read = await new LocalReadTool(alias).execute(context, { path: 'real.txt' }); - expect(read.isError).toBeUndefined(); - expect(read.text).toContain('inside the workspace'); - - const write = await new LocalWriteTool(alias).execute(context, { - path: 'written.txt', - content: 'written through the alias\n', - }); - expect(write.isError).toBeUndefined(); - expect(await readFile(join(workspace, 'written.txt'), 'utf8')).toBe( - 'written through the alias\n', - ); - }); - - it('keeps ordinary in-workspace read, write and edit working', async () => { - await writeFile(join(workspace, 'subject.ts'), 'const value = 1;\n'); - - const read = await new LocalReadTool(workspace).execute(context, { path: 'subject.ts' }); - expect(read.isError).toBeUndefined(); - expect(read.text).toContain('const value = 1;'); - - const write = await new LocalWriteTool(workspace).execute(context, { - path: 'created.ts', - content: 'const created = true;\n', - }); - expect(write.isError).toBeUndefined(); - expect(await readFile(join(workspace, 'created.ts'), 'utf8')).toBe('const created = true;\n'); - - const edit = await new LocalEditTool(workspace).execute(context, { - file_path: 'subject.ts', - old_string: 'const value = 1;', - new_string: 'const value = 2;', - }); - expect(edit.isError).toBeUndefined(); - expect(await readFile(join(workspace, 'subject.ts'), 'utf8')).toBe('const value = 2;\n'); - }); - - it('keeps the path pi receives lexical, not realpath-ed', async () => { - await writeFile(join(workspace, 'real.ts'), 'const value = 1;\n'); - await symlink(join(workspace, 'real.ts'), join(workspace, 'inside-link.ts')); - - const result = await new LocalEditTool(workspace).execute(context, { - file_path: 'inside-link.ts', - old_string: 'const value = 1;', - new_string: 'const value = 2;', - }); - - // The ledger key has to stay byte-for-byte aligned with pi's own - // `resolveToCwd` output, so the capture reports the lexical spelling. - expect(readPluginHookCompatibleToolResponse(result)?.filePath).toBe( - join(workspace, 'inside-link.ts'), - ); - }); -}); diff --git a/packages/agent-tools/src/desktop/workspace-fence.ts b/packages/agent-tools/src/desktop/workspace-fence.ts deleted file mode 100644 index e40cc9d53..000000000 --- a/packages/agent-tools/src/desktop/workspace-fence.ts +++ /dev/null @@ -1,78 +0,0 @@ -/** - * Workspace fence for the desktop fs tools (`read` / `write` / `edit`). - * - * Why this exists: the desktop tools resolve their target lexically - * (`path.resolve` against the workspace root) and hand that spelling to pi, - * which opens it as-is. A symlink inside the workspace therefore turns any - * tool call into a read or a write outside the workspace — and the - * permission layer cannot see it, because it compares paths lexically too. - * - * What it does: resolve the *real* location of the target (following - * symlinks, dangling ones included — see `../shared/workspace-real-path.ts`) - * and require it to sit inside the workspace. - * - * What it deliberately does NOT do: - * - * - It does not change the path spelling that reaches pi. The fence only - * validates; pi still receives the lexical absolute path, byte for byte, - * because the ledger key must stay aligned with pi's own `resolveToCwd` - * output (see `normalizeLocalExistingPath` in `local-pi-tools.ts`). - * - It does not take over the permission layer's job for targets that are - * *already* outside the workspace lexically (`/etc/passwd`, `~/notes`, - * `../../elsewhere`). Those keep their existing approval flow — the - * permission layer asks the user before the tool runs. Turning that - * approval into a hard failure would break cross-workspace reads users - * approve today; the hole this fence closes is the one where the - * permission layer does not even get to ask. - */ - -import { resolve } from 'node:path'; - -import { isInsideDirectory, realpathOrUndefined, resolveRealPath } from '../shared/workspace-real-path.js'; - -/** - * Check the given absolute paths against the workspace fence. - * - * Returns `undefined` when every path is acceptable, or a message describing - * the first violation. The message is written for the model: it names the - * path the tool asked for and the place it really points at. - */ -export async function findWorkspaceFenceViolation( - absolutePaths: readonly string[], - workspaceRoot: string, -): Promise { - const lexicalRoot = resolve(workspaceRoot); - - // Only the targets the workspace *claims* are this fence's business. A - // target that is already outside the workspace lexically belongs to the - // permission layer, which asks the user for approval before the tool runs. - const claimed = absolutePaths - .map((candidate) => resolve(candidate)) - .filter((candidate) => isInsideDirectory(lexicalRoot, candidate)); - if (claimed.length === 0) return undefined; - - const rootReal = await realpathOrUndefined(lexicalRoot); - if (rootReal === undefined) { - return ( - `Workspace path blocked: the workspace root ${workspaceRoot} cannot be resolved, ` + - `so ${claimed[0]} cannot be checked against it.` - ); - } - - for (const candidate of claimed) { - const targetReal = await resolveRealPath(candidate); - if (targetReal === undefined) { - return ( - `Workspace path blocked: the real location of ${candidate} cannot be resolved ` + - `(symlink loop or unreadable path), so it cannot be checked against the workspace.` - ); - } - if (!isInsideDirectory(rootReal, targetReal)) { - return ( - `Workspace path blocked: ${candidate} resolves to ${targetReal}, which is outside ` + - `the workspace (${rootReal}). Only paths inside the workspace can be read or written.` - ); - } - } - return undefined; -} diff --git a/packages/agent-tools/src/shared/workspace-real-path.ts b/packages/agent-tools/src/shared/workspace-real-path.ts deleted file mode 100644 index 13ac3666a..000000000 --- a/packages/agent-tools/src/shared/workspace-real-path.ts +++ /dev/null @@ -1,92 +0,0 @@ -/** - * Real-path resolution shared by the workspace boundary checks. - * - * The shape follows the turn-diff capture fence - * (`packages/local-runtime/src/turns/file-changes.ts`, #232): `realpath` the - * target and, when the target does not exist yet — a write creates a file - * that is not there — resolve its nearest existing ancestor and re-attach the - * missing segments. - * - * The piece 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. Without that branch a dangling link that points outside the - * workspace resolves to a path *inside* the workspace and passes the fence. - * - * 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. - */ - -import { readlink, realpath } from 'node:fs/promises'; -import { basename, dirname, isAbsolute, relative, resolve, sep } from 'node:path'; - -/** Symlink hops before the walk gives up (guards against cycles). */ -const MAX_REAL_PATH_SYMLINK_HOPS = 32; - -/** - * Resolve a path to the real location the filesystem uses for it, following - * symlinks — including dangling ones. - */ -export async function resolveRealPath( - absolutePath: string, - hops = 0, -): Promise { - let candidate = resolve(absolutePath); - const suffixParts: string[] = []; - for (;;) { - try { - const ancestorReal = await realpath(candidate); - return suffixParts.length === 0 - ? ancestorReal - : 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(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 = isAbsolute(link) ? resolve(link) : resolve(realParent, link); - const resolved = await resolveRealPath(followed, hops + 1); - if (!resolved) return undefined; - return suffixParts.length ? resolve(resolved, ...suffixParts) : resolved; - } - const parent = dirname(candidate); - if (parent === candidate) return undefined; - suffixParts.unshift(basename(candidate)); - candidate = parent; - } - } -} - -/** - * Whether `candidate` is `root` itself or a path inside it. - * - * The comparison is `path.relative`-based, and it is case-insensitive on - * Windows, where the filesystem is. - */ -export function isInsideDirectory(root: string, candidate: string): boolean { - const rel = relative(root, candidate); - return !(rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel)); -} - -export async function realpathOrUndefined(target: string): Promise { - try { - return await realpath(target); - } catch { - return undefined; - } -} - -async function readlinkOrUndefined(target: string): Promise { - try { - return await readlink(target); - } catch { - return undefined; - } -} diff --git a/packages/local-runtime-v2/src/application/conversation/conversation-application.ts b/packages/local-runtime-v2/src/application/conversation/conversation-application.ts index 2c763d49d..cce59a0c1 100644 --- a/packages/local-runtime-v2/src/application/conversation/conversation-application.ts +++ b/packages/local-runtime-v2/src/application/conversation/conversation-application.ts @@ -290,15 +290,6 @@ export class ConversationApplication { `Session not found: ${req.id}`, ); } - // The active turn is the authoritative answer to "is a turn running", and - // it is the answer `getActiveTurn` already gave the client that is - // attaching. The session's persisted status is an asynchronous projection - // of the runtime's status events, so it still reads "idle" while a turn - // runs — for a queue-drained turn that is exactly what happens, and the - // resumed stream would then replay the history and close, dropping the - // answer the drained turn commits a moment later. Read both, and let the - // turn decide. - const activeTurnId = this.options.turnInspection?.activeTurnId(req.id); const frames = this.options.stream.resume({ sessionId: req.id, ...(req.afterMsgId ? { afterMsgId: req.afterMsgId } : {}), @@ -306,9 +297,7 @@ export class ConversationApplication { waitForTerminal: shouldWaitForTerminal( req.drainQueued === true || req.continuePausedQueue === true, session.status, - activeTurnId !== undefined, ), - ...(activeTurnId ? { followTurnId: activeTurnId } : {}), }); try { if (req.continuePausedQueue) { @@ -524,9 +513,8 @@ function queueContinueApplicationError( function shouldWaitForTerminal( drainQueued: boolean | undefined, status: SessionStatus, - turnRunning: boolean, ): boolean { - return drainQueued === true || status === "started" || turnRunning; + return drainQueued === true || status === "started"; } function turnContinuationStateView( diff --git a/packages/local-runtime-v2/src/service/session-system/stream/session-stream-service.test.ts b/packages/local-runtime-v2/src/service/session-system/stream/session-stream-service.test.ts deleted file mode 100644 index 5965716ec..000000000 --- a/packages/local-runtime-v2/src/service/session-system/stream/session-stream-service.test.ts +++ /dev/null @@ -1,140 +0,0 @@ -// The resumed session stream's liveness. -// -// A resume that follows a turn is live: it must stay open until that turn -// publishes its terminal, because the turn's answer is committed after the -// resume and a stream that closed at the replay would drop it. The one case -// that has to end is the turn that is already over — it will publish no other -// terminal, so a stream that waited for one would wait forever. - -import { describe, expect, it } from "vitest"; - -import { SessionStreamService } from "./session-stream-service.js"; - -const SESSION_ID = "session-1"; - -function createService() { - return new SessionStreamService({ - messages: { listAfter: async () => ({ status: "ok", messages: [] }) }, - }); -} - -function writeTurnFrame( - service: SessionStreamService, - input: { readonly identity: string; readonly turnId: string; readonly kind?: "runtime-event" | "turn-terminal" }, -): void { - service.write({ - identity: input.identity, - sessionId: SESSION_ID, - kind: input.kind ?? "runtime-event", - data: {}, - turnId: input.turnId, - }); -} - -/** Drain the resume until it ends, and fail if it does not. */ -async function drainToEnd( - service: SessionStreamService, - input: { readonly afterMsgId: string; readonly followTurnId: string }, -): Promise { - const iterator = service.resume({ - sessionId: SESSION_ID, - afterMsgId: input.afterMsgId, - waitForTerminal: true, - followTurnId: input.followTurnId, - }); - const identities: string[] = []; - const ended = await Promise.race([ - (async () => { - for (;;) { - const step = await iterator.next(); - if (step.done) return true; - identities.push(step.value.identity); - } - })(), - new Promise((resolve) => { - const timer = setTimeout(() => resolve(false), 250); - timer.unref?.(); - }), - ]); - if (!ended) { - throw new Error("the resumed stream did not end"); - } - return identities; -} - -describe("SessionStreamService · the resumed stream's liveness", () => { - it("stays live for a turn that has not published its terminal", async () => { - const service = createService(); - writeTurnFrame(service, { identity: "frame-1", turnId: "turn-1" }); - - const iterator = service.resume({ - sessionId: SESSION_ID, - afterMsgId: "msg-0", - waitForTerminal: true, - followTurnId: "turn-1", - }); - - // The replay carries what the ring already holds. - const first = await iterator.next(); - expect(first.done).toBe(false); - expect(first.value?.identity).toBe("frame-1"); - - // The turn ends while the stream is open: the terminal closes it. - writeTurnFrame(service, { - identity: "terminal-1", - turnId: "turn-1", - kind: "turn-terminal", - }); - const second = await iterator.next(); - expect(second.done).toBe(false); - expect(second.value?.kind).toBe("turn-terminal"); - - const third = await iterator.next(); - expect(third.done).toBe(true); - }); - - it("ends at the replay for a turn that already published its terminal", async () => { - const service = createService(); - writeTurnFrame(service, { identity: "frame-1", turnId: "turn-1" }); - writeTurnFrame(service, { - identity: "terminal-1", - turnId: "turn-1", - kind: "turn-terminal", - }); - - // The follower names the turn it follows. That turn is over, so the - // stream must end rather than wait for a terminal that cannot arrive. - const identities = await drainToEnd(service, { - afterMsgId: "msg-0", - followTurnId: "turn-1", - }); - expect(identities).toEqual(["frame-1"]); - }); - - it("keeps waiting for the turn it follows while that turn runs", async () => { - const service = createService(); - writeTurnFrame(service, { identity: "frame-1", turnId: "turn-1" }); - // Turn 0 ended; turn 1 is the one being followed and it is still running. - writeTurnFrame(service, { - identity: "terminal-0", - turnId: "turn-0", - kind: "turn-terminal", - }); - - const iterator = service.resume({ - sessionId: SESSION_ID, - afterMsgId: "msg-0", - waitForTerminal: true, - followTurnId: "turn-1", - }); - const ended = await Promise.race([ - iterator.next().then((step) => step.done), - new Promise((resolve) => { - const timer = setTimeout(() => resolve(false), 250); - timer.unref?.(); - }), - ]); - expect(ended).toBe(false); - await iterator.return?.(); - }); -}); diff --git a/packages/local-runtime-v2/src/service/session-system/stream/session-stream-service.ts b/packages/local-runtime-v2/src/service/session-system/stream/session-stream-service.ts index 951623fc4..1d88ee962 100644 --- a/packages/local-runtime-v2/src/service/session-system/stream/session-stream-service.ts +++ b/packages/local-runtime-v2/src/service/session-system/stream/session-stream-service.ts @@ -42,15 +42,6 @@ export interface ResumeSessionInput { * always live-only. Defaults to true for internal callers. */ readonly waitForTerminal?: boolean; - /** - * The turn this stream follows. - * - * A turn that already published its terminal will publish no other, so a - * stream that waits for one has to end at the replay instead. The caller - * names the turn it is following so the stream can tell "this turn is still - * running" from "this turn is already over". - */ - readonly followTurnId?: string; } export interface SessionStreamReservation { @@ -69,14 +60,6 @@ interface SessionStreamState { hasFrames: boolean; reservationCount: number; deleted: boolean; - /** - * The turn of the last terminal this session published. - * - * A resume that follows that turn must end at its replay: the turn has - * published its terminal and will publish no other, so waiting for one - * would wait forever. - */ - lastTerminalTurnId?: string; } type SessionRingEntry = RingEntry; @@ -100,11 +83,6 @@ export class SessionStreamService implements SessionStreamWriter { write(input: SessionFrameInput): SessionFrameWriteResult { const state = this.state(input.sessionId); - // Recorded before the frame is published, so a resume that subscribes in - // the same tick sees the terminal as already published. - if (input.kind === 'turn-terminal' && input.turnId) { - state.lastTerminalTurnId = input.turnId; - } const value: UnsequencedSessionFrame = { identity: input.identity, sessionId: input.sessionId, @@ -268,12 +246,6 @@ class SessionResumeIterator implements AsyncIterableIterator { private async initialize(): Promise { const waitForTerminal = this.input.waitForTerminal ?? true; if (this.initializeUnanchored(waitForTerminal)) return; - // The anchored paths deliver a replay, so the stream can end at it. The - // turn this stream follows already published its terminal: it will publish - // no other, so waiting for one would wait forever. - if (waitForTerminal && this.followedTurnAlreadyTerminated()) { - this.closeAfterReplay = true; - } const initial = this.state.ring.replay(this.input.afterCursor); if (initial.status !== 'ok') { this.finishWithResync(initial.status); @@ -286,19 +258,6 @@ class SessionResumeIterator implements AsyncIterableIterator { await this.initializeDurable(initial.entries, waitForTerminal); } - /** - * Whether the turn this stream follows has already published its terminal. - * - * Read against the same state the subscription was taken from, so the two - * cannot disagree: a terminal published before the subscription is already - * recorded, and one published after it is delivered to the subscriber. - */ - private followedTurnAlreadyTerminated(): boolean { - const followTurnId = this.input.followTurnId; - if (!followTurnId) return false; - return this.state.lastTerminalTurnId === followTurnId; - } - private async initializeDurable( initial: readonly SessionRingEntry[], waitForTerminal: boolean, diff --git a/packages/local-runtime-v2/src/service/workspace/operations/workspace-path.test.ts b/packages/local-runtime-v2/src/service/workspace/operations/workspace-path.test.ts deleted file mode 100644 index 97953f4ba..000000000 --- a/packages/local-runtime-v2/src/service/workspace/operations/workspace-path.test.ts +++ /dev/null @@ -1,78 +0,0 @@ -import { mkdtemp, realpath, rm, symlink, writeFile } from 'node:fs/promises'; -import { tmpdir } from 'node:os'; -import { join } from 'node:path'; - -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; - -import { resolveWorkspacePath, resolveWorkspaceWritePath } from './workspace-path.js'; - -/** - * The write path resolves the target's *real* location. A symlink inside the - * workspace that points outside it has to be refused — including a dangling - * one, which `realpath` cannot resolve and which used to fall back to the - * lexical spelling (a path inside the workspace) and pass. - */ -describe('workspace write path boundary', () => { - let workspace: string; - let outside: string; - - beforeEach(async () => { - workspace = await mkdtemp(join(tmpdir(), 'workspace-path-ws-')); - outside = await mkdtemp(join(tmpdir(), 'workspace-path-out-')); - }); - - afterEach(async () => { - await rm(workspace, { recursive: true, force: true }); - await rm(outside, { recursive: true, force: true }); - }); - - it('refuses a write through a symlink that points outside the workspace', async () => { - await writeFile(join(outside, 'target.txt'), 'outside\n'); - await symlink(join(outside, 'target.txt'), join(workspace, 'escape.txt')); - - await expect(resolveWorkspaceWritePath(workspace, 'escape.txt')).resolves.toBeUndefined(); - }); - - it('refuses a write through a dangling symlink that points outside the workspace', async () => { - await symlink(join(outside, 'not-created-yet.txt'), join(workspace, 'dangling.txt')); - - await expect(resolveWorkspaceWritePath(workspace, 'dangling.txt')).resolves.toBeUndefined(); - }); - - it('refuses a write whose parent directory is a symlink pointing outside', async () => { - await symlink(outside, join(workspace, 'outdir')); - - await expect(resolveWorkspaceWritePath(workspace, 'outdir/pwned.txt')).resolves.toBeUndefined(); - }); - - it('allows a write through a symlink that points inside the workspace', async () => { - await writeFile(join(workspace, 'real.txt'), 'inside\n'); - await symlink(join(workspace, 'real.txt'), join(workspace, 'inside-link.txt')); - - // The write path resolves the parent directory for real, so the returned - // path is the real parent plus the requested name. - await expect(resolveWorkspaceWritePath(workspace, 'inside-link.txt')).resolves.toBe( - join(await realpath(workspace), 'inside-link.txt'), - ); - }); - - it('allows a write to a new file inside the workspace', async () => { - await expect(resolveWorkspaceWritePath(workspace, 'created.txt')).resolves.toBe( - join(await realpath(workspace), 'created.txt'), - ); - }); - - it('leaves the read path contract alone', async () => { - await writeFile(join(workspace, 'real.txt'), 'inside\n'); - await symlink(join(workspace, 'real.txt'), join(workspace, 'inside-link.txt')); - - const realWorkspace = await realpath(workspace); - await expect(resolveWorkspacePath(workspace, 'real.txt')).resolves.toBe( - join(realWorkspace, 'real.txt'), - ); - await expect(resolveWorkspacePath(workspace, 'inside-link.txt')).resolves.toBe( - join(realWorkspace, 'real.txt'), - ); - await expect(resolveWorkspacePath(workspace, '../outside')).resolves.toBeUndefined(); - }); -}); diff --git a/packages/local-runtime-v2/src/service/workspace/operations/workspace-path.ts b/packages/local-runtime-v2/src/service/workspace/operations/workspace-path.ts index c7fd4483f..42d25c133 100644 --- a/packages/local-runtime-v2/src/service/workspace/operations/workspace-path.ts +++ b/packages/local-runtime-v2/src/service/workspace/operations/workspace-path.ts @@ -1,12 +1,9 @@ -import { mkdir, readFile, readlink, realpath } from 'node:fs/promises'; +import { mkdir, readFile, realpath } from 'node:fs/promises'; import { basename, dirname, extname, isAbsolute, join, relative, resolve } from 'node:path'; import { TextDecoder } from 'node:util'; import type { WorkspaceFileContent } from '../contracts.js'; -/** Symlink hops before the real-path walk gives up (guards against cycles). */ -const MAX_REAL_PATH_SYMLINK_HOPS = 32; - const FATAL_UTF8_DECODER = new TextDecoder('utf-8', { fatal: true }); const TEXT_EXTENSIONS = new Set([ '.css', @@ -160,8 +157,7 @@ export async function resolveWorkspaceWritePath( const realParent = await createValidatedParent(realRoot, dirname(lexicalTarget)); if (!realParent) return undefined; const target = join(realParent, basename(lexicalTarget)); - const existing = await resolveRealPath(target); - if (!existing) return undefined; + const existing = await realpathOrFallback(target); return isPathInside(realRoot, existing) ? target : undefined; } catch { return undefined; @@ -216,73 +212,11 @@ function isAlreadyExistsError(error: unknown): boolean { return (error as NodeJS.ErrnoException | undefined)?.code === 'EEXIST'; } -/** - * Resolve the real location of a path, following symlinks. - * - * Same shape as the shared fence helper - * (`packages/agent-tools/src/shared/workspace-real-path.ts`, copied from the - * turn-diff capture fence in `packages/local-runtime/src/turns/file-changes.ts`): - * `realpath` the target, and when the target does not exist yet — a write - * creates a file that is not there — resolve its nearest existing ancestor and - * re-attach the missing segments. - * - * The part that matters here: 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. Falling back to - * the lexical spelling (the old `realpathOrFallback`) compared a path inside - * the workspace and let the write land outside it. - * - * 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 = resolve(absolutePath); - const suffixParts: string[] = []; - for (;;) { - try { - const ancestorReal = await realpath(candidate); - return suffixParts.length === 0 ? ancestorReal : 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(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 = isAbsolute(link) ? resolve(link) : resolve(realParent, link); - const resolved = await resolveRealPath(followed, hops + 1); - if (!resolved) return undefined; - return suffixParts.length ? resolve(resolved, ...suffixParts) : resolved; - } - const parent = dirname(candidate); - if (parent === candidate) return undefined; - suffixParts.unshift(basename(candidate)); - candidate = parent; - } - } -} - -async function realpathOrUndefined(target: string): Promise { +async function realpathOrFallback(target: string): Promise { try { return await realpath(target); } catch { - return undefined; - } -} - -async function readlinkOrUndefined(target: string): Promise { - try { - return await readlink(target); - } catch { - return undefined; + return target; } } diff --git a/packages/local-runtime/src/turns/file-changes.ts b/packages/local-runtime/src/turns/file-changes.ts index dbda2884e..69442b401 100644 --- a/packages/local-runtime/src/turns/file-changes.ts +++ b/packages/local-runtime/src/turns/file-changes.ts @@ -44,14 +44,6 @@ 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', @@ -414,19 +406,9 @@ 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 = await resolveMutationTargetPath( - workspaceRoot, - realWorkspaceRoot, - undo.file, - ); + const absolute = resolveMutationTargetPath(record.workspaceDir, undo.file); if (!absolute) return { success: false, reason: 'unsafe_path' }; const target = undo[targetKey]; if (target.exists && target.content === undefined) { @@ -464,135 +446,29 @@ 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/`, - * `.harness-def/`, the root itself), which is also a refusal here. The + * `node_modules/`, 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. * - * 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. + * 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. */ -async function resolveMutationTargetPath( - workspaceRoot: string, - realWorkspaceRoot: string, +function resolveMutationTargetPath( + workspaceDir: string, file: string, -): Promise { - const normalized = normalizeCapturePath(workspaceRoot, file); +): string | undefined { + const normalized = normalizeCapturePath(workspaceDir, file); if (!normalized) return undefined; if (path.isAbsolute(normalized) || path.win32.isAbsolute(normalized)) { return 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; - } + 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; } function makeCaptureId(): string { @@ -838,15 +714,6 @@ 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; @@ -854,39 +721,6 @@ 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, @@ -894,13 +728,6 @@ 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 { @@ -912,15 +739,6 @@ 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 deleted file mode 100644 index 35ff7c979..000000000 --- a/packages/local-runtime/test/unit/diff-capture-symlink-containment.test.ts +++ /dev/null @@ -1,234 +0,0 @@ -// 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/packages/local-runtime/test/unit/diff-mutation-symlink-containment.test.ts b/packages/local-runtime/test/unit/diff-mutation-symlink-containment.test.ts deleted file mode 100644 index 717b7bf99..000000000 --- a/packages/local-runtime/test/unit/diff-mutation-symlink-containment.test.ts +++ /dev/null @@ -1,332 +0,0 @@ -// 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 db35d87d1..27d0c1ce4 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -490,14 +490,11 @@ "packages/agent-tools/src/desktop/matrix-media-client.ts", "packages/agent-tools/src/desktop/matrix-tools.ts", "packages/agent-tools/src/desktop/output-limit.ts", - "packages/agent-tools/src/desktop/path-guard.test.ts", "packages/agent-tools/src/desktop/path-guard.ts", "packages/agent-tools/src/desktop/prepare-ask-user-arguments.ts", "packages/agent-tools/src/desktop/subagent-roles.ts", "packages/agent-tools/src/desktop/task-verification.ts", "packages/agent-tools/src/desktop/types.ts", - "packages/agent-tools/src/desktop/workspace-fence.test.ts", - "packages/agent-tools/src/desktop/workspace-fence.ts", "packages/agent-tools/src/index.ts", "packages/agent-tools/src/mcp-disclosure/hint.ts", "packages/agent-tools/src/mcp-disclosure/mcp-disclosure.ts", @@ -530,7 +527,6 @@ "packages/agent-tools/src/shared/rg-scan-policy.ts", "packages/agent-tools/src/shared/rg-sensitive.ts", "packages/agent-tools/src/shared/web-search.ts", - "packages/agent-tools/src/shared/workspace-real-path.ts", "packages/agent-tools/src/shared/write-capture.ts", "packages/agent-tools/test/desktop/local-bash-output.test.ts", "packages/agent-tools/test/desktop/local-bash-timeout.test.ts", @@ -1429,7 +1425,6 @@ "packages/local-runtime-v2/src/service/session-system/stream/index.ts", "packages/local-runtime-v2/src/service/session-system/stream/session-frame-retention.ts", "packages/local-runtime-v2/src/service/session-system/stream/session-frame.ts", - "packages/local-runtime-v2/src/service/session-system/stream/session-stream-service.test.ts", "packages/local-runtime-v2/src/service/session-system/stream/session-stream-service.ts", "packages/local-runtime-v2/src/service/session-system/usage/commit-signal.ts", "packages/local-runtime-v2/src/service/session-system/usage/index.ts", @@ -1636,7 +1631,6 @@ "packages/local-runtime-v2/src/service/workspace/operations/review-search.ts", "packages/local-runtime-v2/src/service/workspace/operations/review.ts", "packages/local-runtime-v2/src/service/workspace/operations/workspace-identity.ts", - "packages/local-runtime-v2/src/service/workspace/operations/workspace-path.test.ts", "packages/local-runtime-v2/src/service/workspace/operations/workspace-path.ts", "packages/local-runtime-v2/src/service/workspace/operations/worktrees.ts", "packages/local-runtime-v2/src/service/workspace/snapshot/snapshot-entry-cache.ts", @@ -2853,9 +2847,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", "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 8ed5ebc95..2d2d8009e 100644 --- a/test/vitest-suites.json +++ b/test/vitest-suites.json @@ -13,9 +13,6 @@ "packages/agent-tools/src/shared/replace-all-edit.test.ts", "packages/agent-tools/src/shared/cron-schedule-input.test.ts", "packages/agent-tools/src/desktop/edit-diff-bounds.test.ts", - "packages/agent-tools/src/desktop/path-guard.test.ts", - "packages/agent-tools/src/desktop/workspace-fence.test.ts", - "packages/local-runtime-v2/src/service/workspace/operations/workspace-path.test.ts", "packages/tui/test/unit/tui-tool-preview-diff-omitted.test.ts", "packages/local-runtime-v2/test/unit/agent/agent-import.test.ts", "packages/tui/test/unit/auth-application.test.ts", @@ -190,8 +187,6 @@ "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", - "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", @@ -203,7 +198,6 @@ "packages/tui/test/unit/tui/controller/side-session-flow.test.ts", "packages/tui/test/unit/observability.test.ts", "packages/local-runtime-v2/src/service/session-system/fork/side-history-boundary.test.ts", - "packages/local-runtime-v2/src/service/session-system/stream/session-stream-service.test.ts", "packages/tui/test/unit/host-tui-settings.test.ts", "packages/tui/test/unit/tui/features/settings/theme-picker.test.ts", "packages/tui/test/unit/tui/theme/custom-themes.test.ts",