diff --git a/.changeset/realpath-symlink-guard.md b/.changeset/realpath-symlink-guard.md new file mode 100644 index 0000000000..51ccb454fd --- /dev/null +++ b/.changeset/realpath-symlink-guard.md @@ -0,0 +1,5 @@ +--- +'@electric-ax/agents-runtime': patch +--- + +The built-in `read`, `write`, and `edit` tools now reject paths whose realpath resolves outside the working directory. Previously the cwd guard was a string prefix check on the un-resolved path, which followed symlinks transparently — a symlink at `/link.txt` pointing to `/etc/hostname` was readable, and a symlinked directory could redirect writes outside the workspace (CVE-2025-53109/53110 class). Intra-workspace symlinks (pnpm `.pnpm` trees, etc.) keep working. For non-existent write/edit targets the guard realpaths the deepest existing ancestor instead. Known gap: hardlinks across the cwd boundary still bypass; a proper fix requires a sandbox. diff --git a/packages/agents-runtime/src/tools/edit.ts b/packages/agents-runtime/src/tools/edit.ts index c66def3426..a4c8a5504d 100644 --- a/packages/agents-runtime/src/tools/edit.ts +++ b/packages/agents-runtime/src/tools/edit.ts @@ -1,8 +1,9 @@ import { readFile, writeFile } from 'node:fs/promises' -import { relative, resolve } from 'node:path' +import { relative } from 'node:path' import { createTwoFilesPatch } from 'diff' import { Type } from '@sinclair/typebox' import { runtimeLog } from '../log' +import { resolveInsideWorkdir } from './path-guard' import type { AgentTool } from '@mariozechner/pi-agent-core' const READ_GUARD_MESSAGE = (rel: string): string => @@ -45,19 +46,17 @@ export function createEditTool( replace_all?: boolean } try { - const resolved = resolve(workingDirectory, filePath) - const rel = relative(workingDirectory, resolved) - if (rel.startsWith(`..`)) { + const guard = await resolveInsideWorkdir(filePath, workingDirectory) + if (!guard.ok) { return { content: [ - { - type: `text` as const, - text: `Error: Path "${filePath}" is outside the working directory`, - }, + { type: `text` as const, text: `Error: ${guard.reason}` }, ], details: { replacements: 0 }, } } + const resolved = guard.resolved + const rel = relative(workingDirectory, resolved) if (!readSet.has(resolved)) { return { diff --git a/packages/agents-runtime/src/tools/path-guard.ts b/packages/agents-runtime/src/tools/path-guard.ts new file mode 100644 index 0000000000..f7d1511cfd --- /dev/null +++ b/packages/agents-runtime/src/tools/path-guard.ts @@ -0,0 +1,49 @@ +import { realpath } from 'node:fs/promises' +import { dirname, relative, resolve } from 'node:path' + +export type PathGuardResult = + | { ok: true; resolved: string } + | { ok: false; reason: string } + +// Resolves `filePath` relative to `workingDirectory` and verifies it stays +// inside, both before and after symlink expansion. For non-existent +// targets (write/edit may create files) `realpath` is applied to the +// deepest existing ancestor. Hardlinks across the cwd boundary remain a +// known gap. +export async function resolveInsideWorkdir( + filePath: string, + workingDirectory: string +): Promise { + const resolved = resolve(workingDirectory, filePath) + if (relative(workingDirectory, resolved).startsWith(`..`)) { + return { + ok: false, + reason: `Path "${filePath}" is outside the working directory`, + } + } + const realCwd = await realpath(workingDirectory) + let probe = resolved + while (true) { + try { + const realTarget = await realpath(probe) + if (relative(realCwd, realTarget).startsWith(`..`)) { + return { + ok: false, + reason: `Path "${filePath}" resolves outside the working directory via a symlink`, + } + } + return { ok: true, resolved } + } catch (err) { + const code = (err as NodeJS.ErrnoException).code + if (code !== `ENOENT` && code !== `ENOTDIR`) throw err + const parent = dirname(probe) + if (parent === probe) { + return { + ok: false, + reason: `Path "${filePath}" resolves outside the working directory via a symlink`, + } + } + probe = parent + } + } +} diff --git a/packages/agents-runtime/src/tools/read-file.ts b/packages/agents-runtime/src/tools/read-file.ts index 42a1041049..e8497d514f 100644 --- a/packages/agents-runtime/src/tools/read-file.ts +++ b/packages/agents-runtime/src/tools/read-file.ts @@ -1,7 +1,7 @@ import { readFile, stat } from 'node:fs/promises' -import { relative, resolve } from 'node:path' import { Type } from '@sinclair/typebox' import { runtimeLog } from '../log' +import { resolveInsideWorkdir } from './path-guard' import type { AgentTool } from '@mariozechner/pi-agent-core' const MAX_FILE_SIZE = 512 * 1024 // 512 KB @@ -22,19 +22,16 @@ export function createReadFileTool( execute: async (_toolCallId, params) => { const { path: filePath } = params as { path: string } try { - const resolved = resolve(workingDirectory, filePath) - const rel = relative(workingDirectory, resolved) - if (rel.startsWith(`..`)) { + const guard = await resolveInsideWorkdir(filePath, workingDirectory) + if (!guard.ok) { return { content: [ - { - type: `text` as const, - text: `Error: Path "${filePath}" is outside the working directory`, - }, + { type: `text` as const, text: `Error: ${guard.reason}` }, ], details: { charCount: 0 }, } } + const resolved = guard.resolved const fileStat = await stat(resolved) if (fileStat.size > MAX_FILE_SIZE) { diff --git a/packages/agents-runtime/src/tools/write.ts b/packages/agents-runtime/src/tools/write.ts index 9ba9079f91..94661751aa 100644 --- a/packages/agents-runtime/src/tools/write.ts +++ b/packages/agents-runtime/src/tools/write.ts @@ -1,8 +1,9 @@ import { mkdir, readFile, writeFile } from 'node:fs/promises' -import { dirname, relative, resolve } from 'node:path' +import { dirname, relative } from 'node:path' import { createTwoFilesPatch } from 'diff' import { Type } from '@sinclair/typebox' import { runtimeLog } from '../log' +import { resolveInsideWorkdir } from './path-guard' import type { AgentTool } from '@mariozechner/pi-agent-core' export function createWriteTool( @@ -27,19 +28,17 @@ export function createWriteTool( content: string } try { - const resolved = resolve(workingDirectory, filePath) - const rel = relative(workingDirectory, resolved) - if (rel.startsWith(`..`)) { + const guard = await resolveInsideWorkdir(filePath, workingDirectory) + if (!guard.ok) { return { content: [ - { - type: `text` as const, - text: `Error: Path "${filePath}" is outside the working directory`, - }, + { type: `text` as const, text: `Error: ${guard.reason}` }, ], details: { bytesWritten: 0 }, } } + const resolved = guard.resolved + const rel = relative(workingDirectory, resolved) let original = `` let existed = true diff --git a/packages/agents-runtime/test/path-guard.test.ts b/packages/agents-runtime/test/path-guard.test.ts new file mode 100644 index 0000000000..be2ca09492 --- /dev/null +++ b/packages/agents-runtime/test/path-guard.test.ts @@ -0,0 +1,117 @@ +import { mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises' +import { realpathSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { resolveInsideWorkdir } from '../src/tools/path-guard' + +describe(`resolveInsideWorkdir`, () => { + let cwd: string + let outside: string + + beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), `path-guard-`)) + outside = await mkdtemp(join(tmpdir(), `path-guard-outside-`)) + }) + + afterEach(async () => { + await rm(cwd, { recursive: true, force: true }) + await rm(outside, { recursive: true, force: true }) + }) + + it(`returns the resolved absolute path for an in-bounds relative path`, async () => { + await writeFile(join(cwd, `file.txt`), `hello`) + const result = await resolveInsideWorkdir(`file.txt`, cwd) + expect(result).toEqual({ ok: true, resolved: join(cwd, `file.txt`) }) + }) + + it(`accepts an in-bounds path that does not yet exist`, async () => { + const result = await resolveInsideWorkdir(`fresh/dir/file.txt`, cwd) + expect(result).toEqual({ + ok: true, + resolved: join(cwd, `fresh/dir/file.txt`), + }) + }) + + it(`rejects ".." escape via the prefix check`, async () => { + const result = await resolveInsideWorkdir(`../escape.txt`, cwd) + expect(result).toEqual({ + ok: false, + reason: `Path "../escape.txt" is outside the working directory`, + }) + }) + + it(`rejects an absolute path outside the working directory`, async () => { + const result = await resolveInsideWorkdir(`/etc/hostname`, cwd) + expect(result).toEqual({ + ok: false, + reason: `Path "/etc/hostname" is outside the working directory`, + }) + }) + + it(`rejects a symlink that resolves outside the working directory`, async () => { + const secret = join(outside, `secret.txt`) + await writeFile(secret, `secret`) + await symlink(secret, join(cwd, `link.txt`)) + const result = await resolveInsideWorkdir(`link.txt`, cwd) + expect(result).toEqual({ + ok: false, + reason: `Path "link.txt" resolves outside the working directory via a symlink`, + }) + }) + + it(`rejects a write target whose parent is a symlinked-out directory`, async () => { + await symlink(outside, join(cwd, `escape-dir`)) + const result = await resolveInsideWorkdir(`escape-dir/new.txt`, cwd) + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.reason).toMatch(/resolves outside the working directory/) + } + }) + + it(`treats a regular-file ancestor like a missing ancestor (ENOTDIR walks up)`, async () => { + await writeFile(join(cwd, `not-a-dir`), `content`) + const result = await resolveInsideWorkdir(`not-a-dir/sub/new.txt`, cwd) + expect(result).toEqual({ + ok: true, + resolved: join(cwd, `not-a-dir/sub/new.txt`), + }) + }) + + it(`accepts a symlink that stays inside the working directory`, async () => { + const inner = join(cwd, `inner.txt`) + await writeFile(inner, `inner`) + await symlink(inner, join(cwd, `link.txt`)) + const result = await resolveInsideWorkdir(`link.txt`, cwd) + expect(result).toEqual({ ok: true, resolved: join(cwd, `link.txt`) }) + }) + + it(`accepts writing through a directory symlink that points inside cwd`, async () => { + await mkdir(join(cwd, `data`)) + await symlink(join(cwd, `data`), join(cwd, `data-link`)) + const result = await resolveInsideWorkdir(`data-link/new.txt`, cwd) + expect(result).toEqual({ + ok: true, + resolved: join(cwd, `data-link/new.txt`), + }) + }) + + it(`handles a working directory that is itself a symlink`, async () => { + const realDir = await mkdtemp(join(tmpdir(), `path-guard-real-`)) + const linkDir = join(tmpdir(), `path-guard-link-${Date.now()}`) + await symlink(realDir, linkDir) + try { + await writeFile(join(realDir, `file.txt`), `data`) + const result = await resolveInsideWorkdir(`file.txt`, linkDir) + expect(result.ok).toBe(true) + if (result.ok) { + expect(realpathSync(result.resolved)).toBe( + realpathSync(join(realDir, `file.txt`)) + ) + } + } finally { + await rm(linkDir, { force: true }) + await rm(realDir, { recursive: true, force: true }) + } + }) +}) diff --git a/packages/agents-runtime/test/tool-path-symlink.test.ts b/packages/agents-runtime/test/tool-path-symlink.test.ts index cbce1ca01a..7fdf737db0 100644 --- a/packages/agents-runtime/test/tool-path-symlink.test.ts +++ b/packages/agents-runtime/test/tool-path-symlink.test.ts @@ -6,12 +6,10 @@ import { createEditTool } from '../src/tools/edit' import { createReadFileTool } from '../src/tools/read-file' import { createWriteTool } from '../src/tools/write' -// Characterization: read/write/edit guard the working directory using a -// path-prefix check (resolve + relative + startsWith('..')) but do NOT call -// `realpath`, so a symlink inside the working directory that points outside -// is followed transparently — CVE-2025-53109/53110 class bypass. A follow-up -// PR will add realpath resolution; update these expectations when it lands. -describe(`tool path traversal — current symlink behavior`, () => { +// End-to-end coverage that read/write/edit reject escape paths and that +// the target on the other side of an escape symlink is left untouched. +// Path-resolution semantics themselves are exercised in path-guard.test.ts. +describe(`tool path traversal`, () => { let cwd: string let outside: string @@ -25,74 +23,40 @@ describe(`tool path traversal — current symlink behavior`, () => { await rm(outside, { recursive: true, force: true }) }) - it(`read: ".." escape is rejected by the prefix check`, async () => { - const tool = createReadFileTool(cwd) - const result = await tool.execute(`r-dotdot`, { path: `../escape.txt` }) - expect((result.content[0] as { text: string }).text).toMatch( - /outside the working directory/ - ) - }) - - it(`read: symlink inside cwd pointing outside currently succeeds`, async () => { + it(`read rejects an escape symlink with the guard's error message`, async () => { const secret = join(outside, `secret.txt`) await writeFile(secret, `secret data`, `utf-8`) await symlink(secret, join(cwd, `link.txt`)) const tool = createReadFileTool(cwd) - const result = await tool.execute(`r-link`, { path: `link.txt` }) - expect((result.content[0] as { text: string }).text).toBe(`secret data`) - }) - - it(`write: ".." escape is rejected by the prefix check`, async () => { - const tool = createWriteTool(cwd) - const result = await tool.execute(`w-dotdot`, { - path: `../escape.txt`, - content: `nope`, - }) + const result = await tool.execute(`r`, { path: `link.txt` }) expect((result.content[0] as { text: string }).text).toMatch( - /outside the working directory/ + /resolves outside the working directory via a symlink/ ) }) - it(`write: symlink inside cwd pointing outside currently clobbers the target`, async () => { + it(`write rejects an escape symlink and leaves the outside target untouched`, async () => { const target = join(outside, `target.txt`) await writeFile(target, `original`, `utf-8`) await symlink(target, join(cwd, `link.txt`)) const tool = createWriteTool(cwd) - const result = await tool.execute(`w-link`, { - path: `link.txt`, - content: `clobbered`, - }) - expect(result.details).toMatchObject({ bytesWritten: 9 }) - expect(await readFile(target, `utf-8`)).toBe(`clobbered`) + await tool.execute(`w`, { path: `link.txt`, content: `clobbered` }) + expect(await readFile(target, `utf-8`)).toBe(`original`) }) - it(`edit: ".." escape is rejected by the prefix check`, async () => { - const tool = createEditTool(cwd, new Set()) - const result = await tool.execute(`e-dotdot`, { - path: `../escape.txt`, - old_string: `a`, - new_string: `b`, - }) - expect((result.content[0] as { text: string }).text).toMatch( - /outside the working directory/ - ) - }) - - it(`edit: symlink inside cwd pointing outside currently edits through the link`, async () => { + it(`edit rejects an escape symlink even when the link is in readSet`, async () => { const target = join(outside, `t.txt`) await writeFile(target, `hello world`, `utf-8`) const linkPath = join(cwd, `link.txt`) await symlink(target, linkPath) - // The edit tool requires the file to be in readSet; populate it with the - // resolved path the tool would compute. This mirrors what read would have - // done in the same session. const tool = createEditTool(cwd, new Set([linkPath])) - const result = await tool.execute(`e-link`, { + const result = await tool.execute(`e`, { path: `link.txt`, old_string: `world`, new_string: `there`, }) - expect(result.details).toMatchObject({ replacements: 1 }) - expect(await readFile(target, `utf-8`)).toBe(`hello there`) + expect((result.content[0] as { text: string }).text).toMatch( + /resolves outside the working directory via a symlink/ + ) + expect(await readFile(target, `utf-8`)).toBe(`hello world`) }) })