From af0d5cf177e1f123a787308b5aff551ff6aa720f Mon Sep 17 00:00:00 2001 From: probe Date: Fri, 23 Oct 2026 09:37:47 +0800 Subject: [PATCH] fix(agent-tools): keep desktop read/write/edit inside the workspace What was broken: the desktop fs tools resolved their target lexically (path.resolve against the workspace root) and handed that spelling to pi, which opens it as-is. A symlink inside the workspace therefore turned any read, write or edit into an operation outside the workspace, and the permission layer could not see it, because it compares paths lexically too: a symlinked path produced no prompt at all. A dangling symlink was worse - a write through it created the file it pointed at, outside. Why this fix: the desktop fs tools now run their target through a real-path fence. The target's real location - following symlinks, and following dangling ones by hand the way the turn-diff capture fence does (#232) - has to sit inside the workspace. The fence only validates: pi still receives the lexical absolute path, byte for byte, because the ledger key has to stay aligned with pi's own resolveToCwd output. Scope: the fence governs the paths the workspace claims. A target that is already outside the workspace lexically keeps its existing approval flow in the permission layer; turning that approval into a hard failure would break cross-workspace reads that users approve today, and that change belongs to the permission layer, not here. The same dangling-symlink blind spot is closed in the desktop and cloud path guards (a dangling link was treated as a missing file and compared lexically) and in the local-runtime-v2 write path (a realpath failure fell back to the lexical spelling, i.e. a path inside the workspace). How it was verified: with a real probe against this same base commit, the symlink read / write / edit, the directory-symlink read, the dangling symlink write and the local-runtime-v2 dangling write path all escaped before and are refused now, while in-workspace symlinks, a symlinked workspace root and ordinary reads / writes / edits keep working. The new suites (23 tests) cover the same ground. --- .../src/cloud/matrix-tools/path-guard.ts | 46 ++-- .../agent-tools/src/desktop/local-pi-tools.ts | 97 ++++++++- .../src/desktop/path-guard.test.ts | 84 ++++++++ .../agent-tools/src/desktop/path-guard.ts | 45 ++-- .../src/desktop/workspace-fence.test.ts | 202 ++++++++++++++++++ .../src/desktop/workspace-fence.ts | 78 +++++++ .../src/shared/workspace-real-path.ts | 92 ++++++++ .../operations/workspace-path.test.ts | 78 +++++++ .../workspace/operations/workspace-path.ts | 74 ++++++- release/public-source.json | 5 + test/vitest-suites.json | 3 + 11 files changed, 742 insertions(+), 62 deletions(-) create mode 100644 packages/agent-tools/src/desktop/path-guard.test.ts create mode 100644 packages/agent-tools/src/desktop/workspace-fence.test.ts create mode 100644 packages/agent-tools/src/desktop/workspace-fence.ts create mode 100644 packages/agent-tools/src/shared/workspace-real-path.ts create mode 100644 packages/local-runtime-v2/src/service/workspace/operations/workspace-path.test.ts 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",