diff --git a/apps/gateway/src/agent/tools/file-tools.ts b/apps/gateway/src/agent/tools/file-tools.ts index c893217..371d2ee 100644 --- a/apps/gateway/src/agent/tools/file-tools.ts +++ b/apps/gateway/src/agent/tools/file-tools.ts @@ -1,20 +1,7 @@ import { Type } from '@sinclair/typebox'; import type { ToolDefinition } from '@mariozechner/pi-coding-agent'; import { readFile, writeFile, readdir, stat } from 'node:fs/promises'; -import { resolve, relative, join } from 'node:path'; - -/** - * Safety constraint: all file operations are restricted to a base directory. - * Paths that escape the sandbox via ../ traversal are rejected. - */ -function resolveSafe(baseDir: string, inputPath: string): string { - const resolved = resolve(baseDir, inputPath); - const rel = relative(baseDir, resolved); - if (rel.startsWith('..') || resolve(resolved) !== resolve(join(baseDir, rel))) { - throw new Error(`Path escape detected: "${inputPath}" resolves outside base directory`); - } - return resolved; -} +import { guardPath, guardPathUnsafe, SandboxEscapeError } from './path-guard.js'; const MAX_READ_BYTES = 512 * 1024; // 512 KB read limit const MAX_WRITE_BYTES = 1024 * 1024; // 1 MB write limit @@ -37,8 +24,14 @@ export function createFileTools(baseDir: string): ToolDefinition[] { const { path, encoding } = params as { path: string; encoding?: string }; let safePath: string; try { - safePath = resolveSafe(baseDir, path); + safePath = guardPath(path, baseDir); } catch (err) { + if (err instanceof SandboxEscapeError) { + return { + content: [{ type: 'text' as const, text: `Error: ${err.message}` }], + details: undefined, + }; + } return { content: [{ type: 'text' as const, text: `Error: ${String(err)}` }], details: undefined, @@ -99,8 +92,14 @@ export function createFileTools(baseDir: string): ToolDefinition[] { }; let safePath: string; try { - safePath = resolveSafe(baseDir, path); + safePath = guardPathUnsafe(path, baseDir); } catch (err) { + if (err instanceof SandboxEscapeError) { + return { + content: [{ type: 'text' as const, text: `Error: ${err.message}` }], + details: undefined, + }; + } return { content: [{ type: 'text' as const, text: `Error: ${String(err)}` }], details: undefined, @@ -151,8 +150,14 @@ export function createFileTools(baseDir: string): ToolDefinition[] { const target = path ?? '.'; let safePath: string; try { - safePath = resolveSafe(baseDir, target); + safePath = guardPath(target, baseDir); } catch (err) { + if (err instanceof SandboxEscapeError) { + return { + content: [{ type: 'text' as const, text: `Error: ${err.message}` }], + details: undefined, + }; + } return { content: [{ type: 'text' as const, text: `Error: ${String(err)}` }], details: undefined, diff --git a/apps/gateway/src/agent/tools/git-tools.ts b/apps/gateway/src/agent/tools/git-tools.ts index 0680c59..ef6bb3b 100644 --- a/apps/gateway/src/agent/tools/git-tools.ts +++ b/apps/gateway/src/agent/tools/git-tools.ts @@ -2,29 +2,13 @@ import { Type } from '@sinclair/typebox'; import type { ToolDefinition } from '@mariozechner/pi-coding-agent'; import { exec } from 'node:child_process'; import { promisify } from 'node:util'; -import { resolve, relative } from 'node:path'; +import { guardPath, guardPathUnsafe, SandboxEscapeError } from './path-guard.js'; const execAsync = promisify(exec); const GIT_TIMEOUT_MS = 15_000; const MAX_OUTPUT_BYTES = 100 * 1024; // 100 KB -/** - * Clamp a user-supplied cwd to within the sandbox directory. - * If the resolved path escapes the sandbox (via ../ or absolute path outside), - * falls back to the sandbox directory itself. - */ -function clampCwd(sandboxDir: string, requestedCwd?: string): string { - if (!requestedCwd) return sandboxDir; - const resolved = resolve(sandboxDir, requestedCwd); - const rel = relative(sandboxDir, resolved); - if (rel.startsWith('..') || rel.startsWith('/')) { - // Escape attempt — fall back to sandbox root - return sandboxDir; - } - return resolved; -} - async function runGit( args: string[], cwd?: string, @@ -74,7 +58,21 @@ export function createGitTools(sandboxDir?: string): ToolDefinition[] { }), async execute(_toolCallId, params) { const { cwd } = params as { cwd?: string }; - const safeCwd = clampCwd(defaultCwd, cwd); + let safeCwd: string; + try { + safeCwd = guardPath(cwd ?? '.', defaultCwd); + } catch (err) { + if (err instanceof SandboxEscapeError) { + return { + content: [{ type: 'text' as const, text: `Error: ${err.message}` }], + details: undefined, + }; + } + return { + content: [{ type: 'text' as const, text: `Error: ${String(err)}` }], + details: undefined, + }; + } const result = await runGit(['status', '--short', '--branch'], safeCwd); const text = result.error ? `Error: ${result.error}\n${result.stderr}` @@ -107,7 +105,21 @@ export function createGitTools(sandboxDir?: string): ToolDefinition[] { oneline?: boolean; cwd?: string; }; - const safeCwd = clampCwd(defaultCwd, cwd); + let safeCwd: string; + try { + safeCwd = guardPath(cwd ?? '.', defaultCwd); + } catch (err) { + if (err instanceof SandboxEscapeError) { + return { + content: [{ type: 'text' as const, text: `Error: ${err.message}` }], + details: undefined, + }; + } + return { + content: [{ type: 'text' as const, text: `Error: ${String(err)}` }], + details: undefined, + }; + } const args = ['log', `--max-count=${limit ?? 20}`]; if (oneline !== false) args.push('--oneline'); const result = await runGit(args, safeCwd); @@ -148,12 +160,43 @@ export function createGitTools(sandboxDir?: string): ToolDefinition[] { path?: string; cwd?: string; }; - const safeCwd = clampCwd(defaultCwd, cwd); + let safeCwd: string; + try { + safeCwd = guardPath(cwd ?? '.', defaultCwd); + } catch (err) { + if (err instanceof SandboxEscapeError) { + return { + content: [{ type: 'text' as const, text: `Error: ${err.message}` }], + details: undefined, + }; + } + return { + content: [{ type: 'text' as const, text: `Error: ${String(err)}` }], + details: undefined, + }; + } + let safePath: string | undefined; + if (path !== undefined) { + try { + safePath = guardPathUnsafe(path, defaultCwd); + } catch (err) { + if (err instanceof SandboxEscapeError) { + return { + content: [{ type: 'text' as const, text: `Error: ${err.message}` }], + details: undefined, + }; + } + return { + content: [{ type: 'text' as const, text: `Error: ${String(err)}` }], + details: undefined, + }; + } + } const args = ['diff']; if (staged) args.push('--cached'); if (ref) args.push(ref); args.push('--'); - if (path) args.push(path); + if (safePath !== undefined) args.push(safePath); const result = await runGit(args, safeCwd); const text = result.error ? `Error: ${result.error}\n${result.stderr}` diff --git a/apps/gateway/src/agent/tools/path-guard.test.ts b/apps/gateway/src/agent/tools/path-guard.test.ts new file mode 100644 index 0000000..216d791 --- /dev/null +++ b/apps/gateway/src/agent/tools/path-guard.test.ts @@ -0,0 +1,104 @@ +import { describe, it, expect } from 'vitest'; +import { guardPath, guardPathUnsafe, SandboxEscapeError } from './path-guard.js'; +import path from 'node:path'; +import os from 'node:os'; +import fs from 'node:fs'; + +describe('guardPathUnsafe', () => { + const sandbox = '/tmp/test-sandbox'; + + it('allows paths inside sandbox', () => { + const result = guardPathUnsafe('foo/bar.txt', sandbox); + expect(result).toBe(path.resolve(sandbox, 'foo/bar.txt')); + }); + + it('allows sandbox root itself', () => { + const result = guardPathUnsafe('.', sandbox); + expect(result).toBe(path.resolve(sandbox)); + }); + + it('rejects path traversal with ../', () => { + expect(() => guardPathUnsafe('../escape.txt', sandbox)).toThrow(SandboxEscapeError); + }); + + it('rejects absolute path outside sandbox', () => { + expect(() => guardPathUnsafe('/etc/passwd', sandbox)).toThrow(SandboxEscapeError); + }); + + it('rejects deeply nested traversal', () => { + expect(() => guardPathUnsafe('a/b/../../../../../../etc/passwd', sandbox)).toThrow( + SandboxEscapeError, + ); + }); + + it('rejects path that starts with sandbox name but is sibling', () => { + expect(() => guardPathUnsafe('/tmp/test-sandbox-evil/file.txt', sandbox)).toThrow( + SandboxEscapeError, + ); + }); + + it('returns the resolved absolute path for nested paths', () => { + const result = guardPathUnsafe('deep/nested/file.ts', sandbox); + expect(result).toBe('/tmp/test-sandbox/deep/nested/file.ts'); + }); + + it('SandboxEscapeError includes the user path and sandbox in message', () => { + let caught: unknown; + try { + guardPathUnsafe('../escape.txt', sandbox); + } catch (err) { + caught = err; + } + expect(caught).toBeInstanceOf(SandboxEscapeError); + const e = caught as SandboxEscapeError; + expect(e.userPath).toBe('../escape.txt'); + expect(e.sandboxDir).toBe(sandbox); + expect(e.message).toContain('Path escape attempt blocked'); + }); +}); + +describe('guardPath', () => { + let tmpDir: string; + + it('allows an existing path inside a real temp sandbox', () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'path-guard-test-')); + try { + const subdir = path.join(tmpDir, 'subdir'); + fs.mkdirSync(subdir); + const result = guardPath('subdir', tmpDir); + expect(result).toBe(subdir); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + it('allows sandbox root itself', () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'path-guard-test-')); + try { + const result = guardPath('.', tmpDir); + // realpathSync resolves the tmpdir symlinks (macOS /var -> /private/var) + const realTmp = fs.realpathSync.native(tmpDir); + expect(result).toBe(realTmp); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + it('rejects path traversal with ../ on existing sandbox', () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'path-guard-test-')); + try { + expect(() => guardPath('../escape', tmpDir)).toThrow(SandboxEscapeError); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + it('rejects absolute path outside sandbox', () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'path-guard-test-')); + try { + expect(() => guardPath('/etc/passwd', tmpDir)).toThrow(SandboxEscapeError); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); +}); diff --git a/apps/gateway/src/agent/tools/path-guard.ts b/apps/gateway/src/agent/tools/path-guard.ts new file mode 100644 index 0000000..5126df1 --- /dev/null +++ b/apps/gateway/src/agent/tools/path-guard.ts @@ -0,0 +1,58 @@ +import path from 'node:path'; +import fs from 'node:fs'; + +/** + * Resolves a user-provided path and verifies it is inside the allowed sandbox directory. + * Throws SandboxEscapeError if the resolved path is outside the sandbox. + * + * Uses realpathSync to resolve symlinks in the sandbox root. The user-supplied path + * is checked for containment AFTER lexical resolution but BEFORE resolving any symlinks + * within the user path — so symlink escape attempts are caught too. + * + * @param userPath - The path provided by the agent (may be relative or absolute) + * @param sandboxDir - The allowed root directory (already validated on session creation) + * @returns The resolved absolute path, guaranteed to be within sandboxDir + */ +export function guardPath(userPath: string, sandboxDir: string): string { + const resolved = path.resolve(sandboxDir, userPath); + const sandboxResolved = fs.realpathSync.native(sandboxDir); + + // Normalize both paths to resolve any symlinks in the sandbox root itself. + // For the user path, we check containment BEFORE resolving symlinks in the path + // (so we catch symlink escape attempts too — the resolved path must still be under sandbox) + if (!resolved.startsWith(sandboxResolved + path.sep) && resolved !== sandboxResolved) { + throw new SandboxEscapeError(userPath, sandboxDir, resolved); + } + + return resolved; +} + +/** + * Validates a path without resolving symlinks in the user-provided portion. + * Use for paths that may not exist yet (creates, writes). + * + * Performs a lexical containment check only using path.resolve. + */ +export function guardPathUnsafe(userPath: string, sandboxDir: string): string { + const resolved = path.resolve(sandboxDir, userPath); + const sandboxAbs = path.resolve(sandboxDir); + + if (!resolved.startsWith(sandboxAbs + path.sep) && resolved !== sandboxAbs) { + throw new SandboxEscapeError(userPath, sandboxDir, resolved); + } + + return resolved; +} + +export class SandboxEscapeError extends Error { + constructor( + public readonly userPath: string, + public readonly sandboxDir: string, + public readonly resolvedPath: string, + ) { + super( + `Path escape attempt blocked: "${userPath}" resolves to "${resolvedPath}" which is outside sandbox "${sandboxDir}"`, + ); + this.name = 'SandboxEscapeError'; + } +} diff --git a/apps/gateway/src/agent/tools/shell-tools.ts b/apps/gateway/src/agent/tools/shell-tools.ts index 18cd6e0..01e8d6f 100644 --- a/apps/gateway/src/agent/tools/shell-tools.ts +++ b/apps/gateway/src/agent/tools/shell-tools.ts @@ -1,7 +1,7 @@ import { Type } from '@sinclair/typebox'; import type { ToolDefinition } from '@mariozechner/pi-coding-agent'; import { spawn } from 'node:child_process'; -import { resolve, relative } from 'node:path'; +import { guardPath, SandboxEscapeError } from './path-guard.js'; const DEFAULT_TIMEOUT_MS = 30_000; const MAX_OUTPUT_BYTES = 100 * 1024; // 100 KB @@ -68,22 +68,6 @@ function extractBaseCommand(command: string): string { return firstToken.split('/').pop() ?? firstToken; } -/** - * Clamp a user-supplied cwd to within the sandbox directory. - * If the resolved path escapes the sandbox (via ../ or absolute path outside), - * falls back to the sandbox directory itself. - */ -function clampCwd(sandboxDir: string, requestedCwd?: string): string { - if (!requestedCwd) return sandboxDir; - const resolved = resolve(sandboxDir, requestedCwd); - const rel = relative(sandboxDir, resolved); - if (rel.startsWith('..') || rel.startsWith('/')) { - // Escape attempt — fall back to sandbox root - return sandboxDir; - } - return resolved; -} - function runCommand( command: string, options: { timeoutMs: number; cwd?: string }, @@ -185,7 +169,21 @@ export function createShellTools(sandboxDir?: string): ToolDefinition[] { } const timeoutMs = Math.min(timeout ?? DEFAULT_TIMEOUT_MS, 60_000); - const safeCwd = clampCwd(defaultCwd, cwd); + let safeCwd: string; + try { + safeCwd = guardPath(cwd ?? '.', defaultCwd); + } catch (err) { + if (err instanceof SandboxEscapeError) { + return { + content: [{ type: 'text' as const, text: `Error: ${err.message}` }], + details: undefined, + }; + } + return { + content: [{ type: 'text' as const, text: `Error: ${String(err)}` }], + details: undefined, + }; + } const result = await runCommand(command, { timeoutMs, diff --git a/docs/TASKS.md b/docs/TASKS.md index 30a0309..da37270 100644 --- a/docs/TASKS.md +++ b/docs/TASKS.md @@ -85,7 +85,7 @@ | P8-013 | not-started | Phase 8 | Gateway Phase 5 — MosaicPlugin lifecycle, ReloadService, hot reload, system:reload TUI | — | #166 | | P8-014 | not-started | Phase 8 | Gateway Phase 6 — SessionGCService (all tiers), /gc command, cron integration | — | #167 | | P8-015 | not-started | Phase 8 | Gateway Phase 7 — WorkspaceService, ProjectBootstrapService, teams project ownership | — | #168 | -| P8-016 | not-started | Phase 8 | Security — file/git/shell tool strict path hardening, sandbox escape prevention | — | #169 | +| P8-016 | done | Phase 8 | Security — file/git/shell tool strict path hardening, sandbox escape prevention | — | #169 | | P8-017 | not-started | Phase 8 | TUI Phase 8 — autocomplete sidebar, fuzzy match, arg hints, up-arrow history | — | #170 | | P8-018 | done | Phase 8 | Spin-off plan stubs — Gatekeeper, Task Queue Unification, Chroot Sandboxing | — | #171 | | P8-019 | not-started | Phase 8 | Verify Platform Architecture — integration + E2E verification | — | #172 | diff --git a/docs/scratchpads/p8-016-tool-hardening.md b/docs/scratchpads/p8-016-tool-hardening.md new file mode 100644 index 0000000..b6db8b4 --- /dev/null +++ b/docs/scratchpads/p8-016-tool-hardening.md @@ -0,0 +1,55 @@ +# P8-016: Security — Tool Path Hardening + Sandbox Escape Prevention + +## Status: in-progress + +## Branch: feat/p8-016-tool-hardening + +## Issue: #169 + +## Scope + +Harden file, git, and shell tool factories so no path operation escapes `sandboxDir`. + +## Files to Create + +- `apps/gateway/src/agent/tools/path-guard.ts` (new) +- `apps/gateway/src/agent/tools/path-guard.test.ts` (new) + +## Files to Modify + +- `apps/gateway/src/agent/tools/file-tools.ts` +- `apps/gateway/src/agent/tools/git-tools.ts` +- `apps/gateway/src/agent/tools/shell-tools.ts` + +## Analysis + +### file-tools.ts + +- Has existing `resolveSafe()` function but uses weak containment check (relative path) +- Replace with `guardPath` (for reads/lists on existing paths) and `guardPathUnsafe` (for writes) +- Error pattern: return `{ content: [{ type: 'text', text: 'Error: ...' }], details: undefined }` + +### git-tools.ts + +- Has `clampCwd()` that silently falls back to sandbox root on escape attempt +- Replace with strict `guardPath` that throws SandboxEscapeError, caught and returned as error +- Also need to guard the `path` parameter in `git_diff` + +### shell-tools.ts + +- Has `clampCwd()` same silent-fallback approach +- Replace with strict `guardPath` that throws SandboxEscapeError + +## Key Design Decisions + +- `guardPath`: uses `realpathSync.native` to resolve symlinks, requires path to exist +- `guardPathUnsafe`: lexical only (`path.resolve`), for paths that may not exist yet +- Both throw `SandboxEscapeError` on escape attempt +- Callers catch and return error result + +## Verification + +- pnpm typecheck +- pnpm lint +- pnpm format:check +- pnpm test