diff --git a/apps/cli/src/index.ts b/apps/cli/src/index.ts index 8845d14..049da6c 100644 --- a/apps/cli/src/index.ts +++ b/apps/cli/src/index.ts @@ -52,7 +52,7 @@ const SECTIONS = [ { body: `By default the agent runs unsandboxed — it can write anywhere you can and reach the network. Pass --safe to confine edits to the project directory, cut network access, and screen destructive shell commands. -Only codex confines writes at the OS level. On claude and opencode --safe screens edits by path and commands by pattern, which does not parse shell — a write redirected out of the project is not caught. Neither cuts raw network access; opencode additionally disables its network tools.`, +Only codex confines writes at the OS level. On claude and opencode --safe screens edits by path and commands by pattern, which does not parse shell — a write redirected out of the project is not caught. Neither cuts raw network access, though both disable their built-in web tools.`, title: "Sandboxing", }, { diff --git a/packages/core/src/providers/claude.ts b/packages/core/src/providers/claude.ts index 4b37f53..019b78d 100644 --- a/packages/core/src/providers/claude.ts +++ b/packages/core/src/providers/claude.ts @@ -9,7 +9,7 @@ */ import { existsSync } from "node:fs"; import { homedir } from "node:os"; -import { join } from "node:path"; +import { join, resolve } from "node:path"; import { EDIT_OUTPUT_JSON_SCHEMA, type EditStructuredOutput, @@ -30,28 +30,49 @@ import { type AgentRunOutcome, failureText, } from "../agent"; -import { isPathInside } from "../paths"; import { systemPrompt } from "../prompt"; -import { EDIT_TOOLS, makeSandboxHook } from "../sandbox"; +import { editPathOf, makeSandboxHook, screenTool } from "../sandbox"; import type { TimelineRecorder } from "../timeline"; import { describeTool } from "../tool-summary"; import { buildAirshipMcpServer } from "../tools"; -const ALLOWED_TOOLS = [ +/** + * The built-in toolset the agent runs with, passed as the SDK's `tools` + * option — which genuinely restricts availability (the CLI narrows via deny + * rules, in every permission mode) — not as `allowedTools`, which only + * auto-approves and restricts nothing. Grep and Glob stay listed: native + * builds otherwise fall back to Bash `find`/`grep`. BashOutput and KillShell + * ride along with Bash so a background command can still be read and + * stopped. The `mcp__airship__*` tools need no entry: MCP tools are exempt + * from narrowing, and their permission is `canUseTool`'s to grant. + * + * Deliberately absent: Task/agent spawning, Skill, SlashCommand, plan-mode + * tools, and AskUserQuestion — there is no interactive channel back to the + * user from inside a job, so a model that asks would hang the turn. + */ +const BUILTIN_TOOLS = [ "Read", "Glob", "Grep", "Edit", "Write", - "MultiEdit", "NotebookEdit", "TodoWrite", "Bash", + "BashOutput", + "KillShell", "WebFetch", "WebSearch", - "mcp__airship__get_element_context", ]; +/** + * Under `--safe` the built-in web tools go too, matching opencode's posture + * (its `--safe` denies webfetch/websearch) so the launch banner's "same + * guards" claim holds. Raw network access via the shell is still not cut on + * either backend; the banner says that too. + */ +const SAFE_EXCLUDED_TOOLS = new Set(["WebFetch", "WebSearch"]); + const EDIT_TOOL_MATCHER = "Write|Edit|MultiEdit|NotebookEdit"; /** Claude has no `minimal`; it is the closest thing to Codex's floor. */ @@ -169,9 +190,13 @@ async function run(ctx: AgentRunContext): Promise { hooks: [ (i: { hook_event_name?: string; tool_input?: unknown }) => { if (i.hook_event_name === "PreToolUse") { - const fp = (i.tool_input as Record | undefined) - ?.file_path; - if (typeof fp === "string") { + // `editPathOf`, not `.file_path`: NotebookEdit names its target + // `notebook_path`, and a notebook edit captured under the wrong key + // is a notebook edit with no diff and no undo. + const fp = editPathOf( + (i.tool_input as Record | undefined) ?? {} + ); + if (fp !== undefined) { diffCapture.recordBefore(fp); } } @@ -190,7 +215,11 @@ async function run(ctx: AgentRunContext): Promise { const options: Options = { abortController: input.abortController, - allowedTools: ALLOWED_TOOLS, + // The SDK requires this alongside `bypassPermissions`. It also matters + // when bypass is downgraded (background sessions, hardened settings): + // without it, non-safe mode would run permission asks with no canUseTool + // installed, and every ask would become a terminal auto-deny. + allowDangerouslySkipPermissions: !safe, cwd: input.cwd, effort: toClaudeEffort(input.effort), enableFileCheckpointing: true, @@ -207,9 +236,10 @@ async function run(ctx: AgentRunContext): Promise { schema: EDIT_OUTPUT_JSON_SCHEMA as unknown as Record, type: "json_schema", }, - // Under `--safe`, edits are auto-accepted but the sandbox hook and - // canUseTool still screen them. Otherwise nothing screens anything. - permissionMode: safe ? "acceptEdits" : "bypassPermissions", + // Under `--safe`, "default" routes every tool decision through + // `canUseTool` below — `acceptEdits` would auto-accept in-project edits + // before the callback ever ran. Otherwise nothing screens anything. + permissionMode: safe ? "default" : "bypassPermissions", persistSession: true, settingSources: ["project"], stderr: (data) => { @@ -222,23 +252,29 @@ async function run(ctx: AgentRunContext): Promise { preset: "claude_code", type: "preset", }, + tools: safe + ? BUILTIN_TOOLS.filter((t) => !SAFE_EXCLUDED_TOOLS.has(t)) + : BUILTIN_TOOLS, }; - // Programmatic permission catch-all. The sandbox hook is the primary guard - // (hooks run first); this is defense-in-depth in case ordering ever changes. - // Both belong to the same posture, so both are gated together. + // Two screens, one `screenTool` ruleset. The sandbox hook is the primary + // guard: hooks run ahead of permission evaluation, and unlike this callback + // they cannot be shadowed by a target project's own `permissions.allow` + // settings (which `settingSources: ["project"]` loads). This is the second, + // genuinely reachable layer — under `permissionMode: "default"` every tool + // decision funnels through it, Bash included. It must resolve on every + // path: an unresolved permission request parks the tool call forever. if (safe) { + const root = resolve(input.cwd); options.canUseTool = (toolName, toolInput) => { - if (EDIT_TOOLS.has(toolName)) { - const fp = (toolInput as Record).file_path; - if (typeof fp === "string" && !isPathInside(input.cwd, fp)) { - return Promise.resolve({ - behavior: "deny", - message: `Refusing to modify a path outside the project: ${fp}`, - }); - } + const verdict = screenTool(root, toolName, toolInput); + if (verdict.allowed) { + return Promise.resolve({ behavior: "allow", updatedInput: toolInput }); } - return Promise.resolve({ behavior: "allow", updatedInput: toolInput }); + return Promise.resolve({ + behavior: "deny", + message: verdict.reason ?? "denied", + }); }; } diff --git a/packages/core/src/sandbox.test.ts b/packages/core/src/sandbox.test.ts index 043ca10..ba671b2 100644 --- a/packages/core/src/sandbox.test.ts +++ b/packages/core/src/sandbox.test.ts @@ -11,8 +11,9 @@ * or PowerShell — the screen was a no-op while the banner still said it was on. * (Path containment lives in ./paths.test.ts.) */ +import { resolve } from "node:path"; import { describe, expect, it } from "vitest"; -import { screenBash } from "./sandbox"; +import { screenBash, screenTool } from "./sandbox"; const blocked = (command: string) => screenBash(command).allowed === false; @@ -67,3 +68,91 @@ describe("screenBash allows ordinary commands", () => { expect(blocked(command)).toBe(false); }); }); + +// `screenTool` is the single dispatch both the PreToolUse hook and Claude's +// `canUseTool` callback run — under `--safe` every permission decision +// funnels through it, so it must be total: a verdict on every input, never a +// throw. An unresolved `canUseTool` parks the tool call forever. +const ROOT = resolve("/project"); +const INSIDE = resolve("/project/src/app.ts"); +const OUTSIDE = resolve("/somewhere/else.ts"); + +describe("screenTool confines edit tools to the project", () => { + it.each(["Write", "Edit", "NotebookEdit"])( + "%s outside the root is denied, and the reason names the path", + (tool) => { + const verdict = screenTool(ROOT, tool, { file_path: OUTSIDE }); + expect(verdict.allowed).toBe(false); + expect(verdict.reason).toContain(OUTSIDE); + } + ); + + it.each(["Write", "Edit", "NotebookEdit"])( + "%s inside the root is allowed", + (tool) => { + expect(screenTool(ROOT, tool, { file_path: INSIDE }).allowed).toBe(true); + } + ); + + // NotebookEdit names its target `notebook_path`, not `file_path`. Keying on + // `file_path` alone is exactly the hole that let notebook writes anywhere on + // disk sail past the screen. + it("NotebookEdit is screened through notebook_path", () => { + const inside = screenTool(ROOT, "NotebookEdit", { + notebook_path: resolve("/project/analysis.ipynb"), + }); + expect(inside.allowed).toBe(true); + const outside = screenTool(ROOT, "NotebookEdit", { + notebook_path: resolve("/somewhere/else.ipynb"), + }); + expect(outside.allowed).toBe(false); + }); +}); + +describe("screenTool screens Bash commands", () => { + it("denies a destructive command", () => { + expect(screenTool(ROOT, "Bash", { command: "rm -rf /" }).allowed).toBe( + false + ); + }); + + it("allows an ordinary command", () => { + expect(screenTool(ROOT, "Bash", { command: "git status" }).allowed).toBe( + true + ); + }); +}); + +describe("screenTool fails open for everything it does not screen", () => { + // Unknown and MCP names must pass: under `--safe` there is no other route + // to an allow, so a fail-closed default would cut off the tools the flow + // depends on rather than guard anything. + it.each([ + "Read", + "Grep", + "Glob", + "TodoWrite", + "WebFetch", + "mcp__airship__get_element_context", + "mcp__airship__get_design_tokens", + "SomeFutureTool", + ])("allows %s", (tool) => { + expect(screenTool(ROOT, tool, {}).allowed).toBe(true); + }); +}); + +describe("screenTool is total on malformed input", () => { + it.each([ + ["missing path", "Edit", {}], + ["non-string file_path", "Write", { file_path: 42 }], + ["non-string notebook_path", "NotebookEdit", { notebook_path: null }], + ["missing command", "Bash", {}], + ["non-string command", "Bash", { command: ["rm", "-rf"] }], + ] as const)("returns a verdict for %s", (_label, tool, input) => { + const verdict = screenTool(ROOT, tool, input as Record); + expect(typeof verdict.allowed).toBe("boolean"); + // Malformed edits are the SDK's problem to reject, not a deny; a + // command that is not a string screens as the empty command. + expect(verdict.allowed).toBe(true); + }); +}); diff --git a/packages/core/src/sandbox.ts b/packages/core/src/sandbox.ts index db99cb8..b209be7 100644 --- a/packages/core/src/sandbox.ts +++ b/packages/core/src/sandbox.ts @@ -2,10 +2,14 @@ * The `--safe` guards, and the Claude hook that installs them. * * The two screens — `screenEdit` and `screenBash` — are the policy, expressed - * without reference to any backend. `makeSandboxHook` wraps them as a Claude - * `PreToolUse` hook, where a deny hard-blocks the tool because hooks run - * ahead of permission rules and `canUseTool`; that is the safety layer which - * replaces the reference tools' blanket `--dangerously-skip-permissions`. + * without reference to any backend, and `screenTool` is the single dispatch + * that routes a named tool call through them. `makeSandboxHook` wraps that + * dispatch as a Claude `PreToolUse` hook, where a deny hard-blocks the tool + * because hooks run ahead of permission rules and `canUseTool` — and unlike + * `canUseTool`, a hook cannot be shadowed by a target project's own + * `permissions.allow` settings, which makes it the primary guard. The Claude + * adapter's `canUseTool` calls the same `screenTool`, so the second layer + * enforces an identical rule set rather than a drifting copy. * * OpenCode has no hook of any kind, but it does emit a permission request and * accept a reply, so its adapter answers each request from these same two @@ -94,31 +98,66 @@ export function screenBash(command: string): ScreenResult { return ALLOWED; } +/** + * The path an edit tool is about to write. `NotebookEdit` names its target + * `notebook_path` where every other edit tool says `file_path` — keying on + * `file_path` alone let notebook writes anywhere on disk sail past both + * screens, silently. + */ +export function editPathOf( + toolInput: Record +): string | undefined { + const fp = toolInput.file_path ?? toolInput.notebook_path; + return typeof fp === "string" ? fp : undefined; +} + +/** + * Route one tool call through the policy. Everything that is neither an edit + * nor a shell command — read-only built-ins and the `mcp__airship__*` tools — + * passes. The screen must fail open for unknown names: under `--safe` every + * permission decision funnels through here, and a fail-closed default would + * cut off tools the flow depends on rather than guard anything. + * + * Total by construction: every branch returns a verdict, never throws. A + * caller that turns this into `canUseTool` relies on that — an unresolved + * permission request parks the tool call forever. + */ +export function screenTool( + root: string, + toolName: string, + toolInput: Record +): ScreenResult { + if (EDIT_TOOLS.has(toolName)) { + const fp = editPathOf(toolInput); + // A missing or malformed path is the SDK's problem to reject, not a + // security decision to make on its behalf. + return fp === undefined ? ALLOWED : screenEdit(root, fp); + } + if (toolName === "Bash") { + return screenBash( + typeof toolInput.command === "string" ? toolInput.command : "" + ); + } + return ALLOWED; +} + export function makeSandboxHook(cwd: string): HookCallback { const root = resolve(cwd); return (input) => { if (input.hook_event_name !== "PreToolUse") { return Promise.resolve({}); } - const name = input.tool_name; - const ti = (input.tool_input ?? {}) as Record; - - if (EDIT_TOOLS.has(name) && typeof ti.file_path === "string") { - const verdict = screenEdit(root, ti.file_path); - if (!verdict.allowed) { - return Promise.resolve(deny(verdict.reason ?? "denied")); - } + const verdict = screenTool( + root, + input.tool_name, + (input.tool_input ?? {}) as Record + ); + if (!verdict.allowed) { + return Promise.resolve(deny(verdict.reason ?? "denied")); } - - if (name === "Bash") { - const verdict = screenBash( - typeof ti.command === "string" ? ti.command : "" - ); - if (!verdict.allowed) { - return Promise.resolve(deny(verdict.reason ?? "denied")); - } - } - + // An allow stays `{}` — fall through to permission evaluation — never an + // explicit hook allow, which would skip `canUseTool` and silently delete + // the second screen. return Promise.resolve({}); }; }