From d0334ca54466e87918b641fa5f4ae185c1fb10bb Mon Sep 17 00:00:00 2001 From: indifferentketchup Date: Fri, 29 May 2026 22:22:51 +0000 Subject: [PATCH] fix(coder): separator-bounded worktree path guard in acp-client-fs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ACP fs bridge's worktree guard used an unbounded `startsWith(resolve( worktreePath))`, so a sibling path sharing the worktree as a string prefix (`-evil/...`) escaped the scope. Since writeWorktreeTextFile hits disk directly (no pending_changes gate), a confused/buggy ACP agent could write outside its worktree. Now uses a separator-bounded check matching write_guard.ts (resolve() + `startsWith(root + sep)` / `=== root`) via a shared resolveInWorktree, with a regression test (../ traversal + the sibling-prefix bug). Symlink-swap hardening intentionally skipped — consistent with write_guard's no-realpath stance; the agent runs with host FS access so this is a containment guard, not a trust boundary. Flagged by the automated push security review. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../services/__tests__/acp-client-fs.test.ts | 50 +++++++++++++++++++ apps/coder/src/services/acp-client-fs.ts | 32 ++++++++---- 2 files changed, 73 insertions(+), 9 deletions(-) create mode 100644 apps/coder/src/services/__tests__/acp-client-fs.test.ts diff --git a/apps/coder/src/services/__tests__/acp-client-fs.test.ts b/apps/coder/src/services/__tests__/acp-client-fs.test.ts new file mode 100644 index 0000000..fe782e4 --- /dev/null +++ b/apps/coder/src/services/__tests__/acp-client-fs.test.ts @@ -0,0 +1,50 @@ +import { describe, it, expect, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { readWorktreeTextFile, writeWorktreeTextFile } from '../acp-client-fs.js'; + +const created: string[] = []; +function freshWorktree(): string { + const wt = mkdtempSync(join(tmpdir(), 'acp-wt-')); + created.push(wt); + return wt; +} +afterEach(() => { + for (const d of created.splice(0)) { + try { + rmSync(d, { recursive: true, force: true }); + rmSync(`${d}-evil`, { recursive: true, force: true }); + } catch { + /* ignore */ + } + } +}); + +describe('acp-client-fs worktree scoping', () => { + it('writes then reads a file inside the worktree', async () => { + const wt = freshWorktree(); + await writeWorktreeTextFile(wt, 'sub/dir/note.txt', 'hello'); + expect(await readWorktreeTextFile(wt, 'sub/dir/note.txt')).toBe('hello'); + }); + + it('rejects ../ traversal on read', async () => { + const wt = freshWorktree(); + await expect(readWorktreeTextFile(wt, '../../etc/passwd')).rejects.toThrow(/escapes worktree/); + }); + + it('rejects ../ traversal on write', async () => { + const wt = freshWorktree(); + await expect(writeWorktreeTextFile(wt, '../escape.txt', 'x')).rejects.toThrow(/escapes worktree/); + }); + + it('rejects a sibling-prefix path (the unbounded-startsWith bug)', async () => { + const wt = freshWorktree(); + // Absolute path that shares the worktree as a STRING prefix but is a sibling + // dir: `-evil/...`. A bare `startsWith()` wrongly admits it. + await expect(readWorktreeTextFile(wt, `${wt}-evil/secret.txt`)).rejects.toThrow(/escapes worktree/); + await expect(writeWorktreeTextFile(wt, `${wt}-evil/secret.txt`, 'x')).rejects.toThrow( + /escapes worktree/, + ); + }); +}); diff --git a/apps/coder/src/services/acp-client-fs.ts b/apps/coder/src/services/acp-client-fs.ts index 87d6061..e2a0d04 100644 --- a/apps/coder/src/services/acp-client-fs.ts +++ b/apps/coder/src/services/acp-client-fs.ts @@ -1,5 +1,25 @@ import { promises as fs } from 'node:fs'; -import { dirname, isAbsolute, join, resolve } from 'node:path'; +import { dirname, isAbsolute, resolve, sep } from 'node:path'; + +/** + * Resolve an ACP-supplied path against the agent worktree and reject anything + * that escapes it. Mirrors `write_guard.ts`'s check: `resolve()` to normalize + * `../` segments, then a **separator-bounded** prefix test — a bare + * `startsWith(root)` wrongly admits a sibling dir like `-evil/...`. + * + * No realpath (consistent with `write_guard.ts`: the target may not exist yet on + * write). This is a containment guard for the ACP fs bridge, not a hard trust + * boundary — the agent process already runs with host FS access; symlink-swap + * hardening (`O_NOFOLLOW`/realpath) is out of scope here. + */ +function resolveInWorktree(worktreePath: string, filePath: string): string { + const root = resolve(worktreePath); + const absolute = isAbsolute(filePath) ? resolve(filePath) : resolve(root, filePath); + if (absolute !== root && !absolute.startsWith(root + sep)) { + throw new Error(`path escapes worktree: ${filePath}`); + } + return absolute; +} /** Resolve an ACP path against the agent worktree and read a slice of lines. */ export async function readWorktreeTextFile( @@ -8,10 +28,7 @@ export async function readWorktreeTextFile( line?: number | null, limit?: number | null, ): Promise { - const absolute = isAbsolute(filePath) ? filePath : resolve(worktreePath, filePath); - if (!absolute.startsWith(resolve(worktreePath))) { - throw new Error(`path escapes worktree: ${filePath}`); - } + const absolute = resolveInWorktree(worktreePath, filePath); const raw = await fs.readFile(absolute, 'utf8'); if (!line && !limit) return raw; const lines = raw.split(/\r?\n/); @@ -26,10 +43,7 @@ export async function writeWorktreeTextFile( filePath: string, content: string, ): Promise { - const absolute = isAbsolute(filePath) ? filePath : resolve(worktreePath, filePath); - if (!absolute.startsWith(resolve(worktreePath))) { - throw new Error(`path escapes worktree: ${filePath}`); - } + const absolute = resolveInWorktree(worktreePath, filePath); await fs.mkdir(dirname(absolute), { recursive: true }); await fs.writeFile(absolute, content, 'utf8'); }