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 bac05e87e..01eb1c1a1 100644 --- a/packages/agent-tools/src/cloud/matrix-tools/path-guard.ts +++ b/packages/agent-tools/src/cloud/matrix-tools/path-guard.ts @@ -1,5 +1,18 @@ +/** + * 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 { basename, dirname, isAbsolute, relative, resolve, sep } from 'node:path'; +import { isAbsolute, resolve } from 'node:path'; + +import { isInsideDirectory, resolveRealPath } from '../../shared/workspace-real-path.js'; /** * Input-path fence scope. A plain string keeps the legacy workspace-only @@ -56,31 +69,12 @@ export async function assertWithinWorkspace( ): Promise { const rootReal = await realpath(workspaceRoot); - 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; + // `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}`); } } diff --git a/packages/agent-tools/src/desktop/local-pi-tools.ts b/packages/agent-tools/src/desktop/local-pi-tools.ts index d044552e3..9e8a0a486 100644 --- a/packages/agent-tools/src/desktop/local-pi-tools.ts +++ b/packages/agent-tools/src/desktop/local-pi-tools.ts @@ -17,11 +17,12 @@ 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, resolve as resolvePath } from 'node:path'; +import { isAbsolute, join, 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, @@ -124,6 +125,20 @@ 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. @@ -251,6 +266,17 @@ 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 @@ -286,6 +312,20 @@ 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 @@ -787,6 +827,61 @@ 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 new file mode 100644 index 000000000..7d4227768 --- /dev/null +++ b/packages/agent-tools/src/desktop/path-guard.test.ts @@ -0,0 +1,84 @@ +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 4a1c20d3a..aca396e3b 100644 --- a/packages/agent-tools/src/desktop/path-guard.ts +++ b/packages/agent-tools/src/desktop/path-guard.ts @@ -2,44 +2,27 @@ * 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/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. + * 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. */ import { realpath } from 'node:fs/promises'; -import { basename, dirname, isAbsolute, relative, resolve, sep } from 'node:path'; +import { isAbsolute, resolve } from 'node:path'; + +import { isInsideDirectory, resolveRealPath } from '../shared/workspace-real-path.js'; async function assertWithinWorkspace(absolutePath: string, workspaceRoot: string): Promise { const rootReal = await realpath(workspaceRoot); - 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; + // `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}`); } } diff --git a/packages/agent-tools/src/desktop/workspace-fence.test.ts b/packages/agent-tools/src/desktop/workspace-fence.test.ts new file mode 100644 index 000000000..1cd59543f --- /dev/null +++ b/packages/agent-tools/src/desktop/workspace-fence.test.ts @@ -0,0 +1,202 @@ +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 new file mode 100644 index 000000000..e40cc9d53 --- /dev/null +++ b/packages/agent-tools/src/desktop/workspace-fence.ts @@ -0,0 +1,78 @@ +/** + * 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 new file mode 100644 index 000000000..13ac3666a --- /dev/null +++ b/packages/agent-tools/src/shared/workspace-real-path.ts @@ -0,0 +1,92 @@ +/** + * 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/service/workspace/operations/workspace-path.test.ts b/packages/local-runtime-v2/src/service/workspace/operations/workspace-path.test.ts new file mode 100644 index 000000000..97953f4ba --- /dev/null +++ b/packages/local-runtime-v2/src/service/workspace/operations/workspace-path.test.ts @@ -0,0 +1,78 @@ +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 42d25c133..c7fd4483f 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,9 +1,12 @@ -import { mkdir, readFile, realpath } from 'node:fs/promises'; +import { mkdir, readFile, readlink, 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', @@ -157,7 +160,8 @@ export async function resolveWorkspaceWritePath( const realParent = await createValidatedParent(realRoot, dirname(lexicalTarget)); if (!realParent) return undefined; const target = join(realParent, basename(lexicalTarget)); - const existing = await realpathOrFallback(target); + const existing = await resolveRealPath(target); + if (!existing) return undefined; return isPathInside(realRoot, existing) ? target : undefined; } catch { return undefined; @@ -212,11 +216,73 @@ function isAlreadyExistsError(error: unknown): boolean { return (error as NodeJS.ErrnoException | undefined)?.code === 'EEXIST'; } -async function realpathOrFallback(target: string): Promise { +/** + * 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 { try { return await realpath(target); } catch { - return target; + return undefined; + } +} + +async function readlinkOrUndefined(target: string): Promise { + try { + return await readlink(target); + } catch { + return undefined; } } diff --git a/release/public-source.json b/release/public-source.json index 84d66b6c7..e4d65d5f7 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -490,11 +490,14 @@ "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", @@ -527,6 +530,7 @@ "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", @@ -1631,6 +1635,7 @@ "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", diff --git a/test/vitest-suites.json b/test/vitest-suites.json index 3b36f3dc3..e16eb895e 100644 --- a/test/vitest-suites.json +++ b/test/vitest-suites.json @@ -13,6 +13,9 @@ "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",