fix(coder): separator-bounded worktree path guard in acp-client-fs
The ACP fs bridge's worktree guard used an unbounded `startsWith(resolve( worktreePath))`, so a sibling path sharing the worktree as a string prefix (`<worktree>-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) <noreply@anthropic.com>
This commit is contained in:
50
apps/coder/src/services/__tests__/acp-client-fs.test.ts
Normal file
50
apps/coder/src/services/__tests__/acp-client-fs.test.ts
Normal file
@@ -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: `<wt>-evil/...`. A bare `startsWith(<wt>)` 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/,
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user