feat(gateway): tool path hardening + sandbox escape prevention (P8-016) (#177)

Co-authored-by: Jason Woltje <jason@diversecanvas.com>
Co-committed-by: Jason Woltje <jason@diversecanvas.com>

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,

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}`

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 });
+    }
+  });
+});

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';
+  }
+}

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,

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          |

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