diff --git a/.claude/tasks/0023-bench-tool-isolation-guard.md b/.claude/tasks/0023-bench-tool-isolation-guard.md index fa1b49d..e4aeb51 100644 --- a/.claude/tasks/0023-bench-tool-isolation-guard.md +++ b/.claude/tasks/0023-bench-tool-isolation-guard.md @@ -10,8 +10,22 @@ The guard is a callback that inspects every proposed shell command and permits o ## Acceptance criteria -- [ ] Each arm's allow-listed binary passes the guard; a foreign binary is denied. -- [ ] An absolute-path invocation of a foreign binary is denied. -- [ ] An interpreter-based fetch attempt (e.g. driving an HTTP request through a language runtime) is denied. -- [ ] A curated per-arm PATH is produced exposing only that arm's allowed binary. -- [ ] Unit tests cover the allowed-binary, foreign-binary, absolute-path, and interpreter-fetch cases per arm. +- [x] Each arm's allow-listed binary passes the guard; a foreign binary is denied. +- [x] An absolute-path invocation of a foreign binary is denied. +- [x] An interpreter-based fetch attempt (e.g. driving an HTTP request through a language runtime) is denied. +- [x] A curated per-arm PATH is produced exposing only that arm's allowed binary. +- [x] Unit tests cover the allowed-binary, foreign-binary, absolute-path, and interpreter-fetch cases per arm. + +## Implementation Notes + +The guard lives in `bench/guard.ts` and exposes a small interface over a deliberately deep implementation: + +- `guardCommand(arm, command)` — the authoritative guard, returning `{ allowed: true }` or `{ allowed: false, reason }`. +- `provisionArmBin(arm, binDir, locate?)` — populates a per-arm bin directory with a single symlink to the arm's binary (empty for `gitea-mcp`); `locate` is an injectable resolver so tests stay host-independent. +- `ARM_BINARY` and `HARMLESS_BINARIES` — the per-arm allow-listed binary (`null` for the shell-disabled `gitea-mcp` arm) and the curated set of harmless read/text/flow utilities. + +Depth added beyond the literal criteria, invited by ADR 0016 ("isolation strength rests on the completeness of the guard's deny rules"): the guard checks *every* binary a command would reach, not just the leading token, via a hand-rolled shell command parser (`extractCommands`) that handles pipelines, `;`/`&&`/`||` sequences, subshells, `$(...)` and backtick substitutions, process substitutions, redirections (including `2>&1` and `&>` forms), and leading `NAME=value` assignments. This closes pipe-hiding and substitution-hiding evasions in addition to the named absolute-path and interpreter-fetch cases. Path-qualified invocations are refused even for the arm's own binary, since the curated PATH is meant to resolve it by name and a path-qualified form is a symlink/copy evasion vector. + +The `gitea-mcp` arm runs with the shell disabled entirely (no allow-listed binary), so `guardCommand` denies every shell command for it with a shell-disabled reason, and `provisionArmBin` exposes nothing. + +Tests are colocated in `bench/guard.test.ts` (31 tests) and run via `npm run test:bench`, kept out of the `src/` coverage tier. diff --git a/bench/README.md b/bench/README.md index 61b87b0..4d26117 100644 --- a/bench/README.md +++ b/bench/README.md @@ -33,8 +33,9 @@ The raw component breakdown is retained on every sample so the data can be re-we - `result.ts` — the immutable result-record shape and its tags (arm, task, tier, trial, timestamp). - `store.ts` — the append-only, per-cell sample store that accumulates result records. +- `guard.ts` — the authoritative tool-isolation guard plus the curated per-arm bin directory that backs it. -Later slices add the tool-isolation guard, the seed provisioning, the arm scaffolding, the single-cell runner, the task suite, the run-loop CLI, and the aggregator. +Later slices add the seed provisioning, the arm scaffolding, the single-cell runner, the task suite, the run-loop CLI, and the aggregator. ## Tests diff --git a/bench/guard.test.ts b/bench/guard.test.ts new file mode 100644 index 0000000..80d60d9 --- /dev/null +++ b/bench/guard.test.ts @@ -0,0 +1,222 @@ +import { mkdtempSync, readdirSync, readlinkSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import type { Arm } from "./result.js"; +import { guardCommand, provisionArmBin } from "./guard.js"; + +/** + * Behavior: each shell-driving arm's own allow-listed binary passes the guard. + * + * The (arm, command) pairs below are independent literals — the benchmark spec + * and ADR 0016 fix which binary each arm drives — rather than values derived + * from the module under test, so the assertion stays a genuine check. + */ +const allowedForOwnBinary: ReadonlyArray<{ arm: Arm; command: string }> = [ + { arm: "gitea-axi", command: "gitea-axi issue list --state open" }, + { arm: "tea", command: "tea issues list --output json" }, + { arm: "raw-api", command: "curl -s https://host/api/v1/repos/o/r/issues" }, +]; + +/** + * Behavior: a foreign binary — one belonging to a different arm — is denied. + * + * Only one binary is allow-listed per arm (ADR 0016), so driving another arm's + * tool must be refused. Each pair is an independent literal: a shell-driving + * arm paired with a command whose executable is a foreign binary. + */ +const foreignBinaryForArm: ReadonlyArray<{ arm: Arm; command: string; foreign: string }> = [ + { arm: "gitea-axi", command: "curl -s https://host/api/v1/repos", foreign: "curl" }, + { arm: "tea", command: "gitea-axi issue list", foreign: "gitea-axi" }, + { arm: "raw-api", command: "tea issues list --output json", foreign: "tea" }, +]; + +/** + * Behavior: an absolute-path invocation of a foreign binary is denied. + * + * Naming a foreign binary by absolute path sidesteps the curated PATH, so the + * guard must still refuse it (ADR 0016). Each pair is an independent literal: + * a shell-driving arm paired with an absolute-path invocation of another arm's + * binary. + */ +const foreignAbsolutePathForArm: ReadonlyArray<{ arm: Arm; command: string }> = [ + { arm: "gitea-axi", command: "/usr/bin/curl -s https://host/api/v1/repos" }, + { arm: "tea", command: "/opt/bin/gitea-axi issue list" }, + { arm: "raw-api", command: "/usr/local/bin/tea issues list" }, +]; + +/** + * Behavior: an interpreter-based fetch attempt is denied. + * + * Reaching the API over HTTP through a language runtime's HTTP client is a + * foreign path for every arm — including raw-api, whose only allowed binary is + * curl, not an interpreter (ADR 0016). Each pair is an independent literal: a + * shell-driving arm paired with an interpreter invocation that fetches over HTTP. + */ +const interpreterFetchForArm: ReadonlyArray<{ arm: Arm; command: string }> = [ + { + arm: "gitea-axi", + command: `python3 -c "import urllib.request as u; u.urlopen('https://host/api/v1')"`, + }, + { + arm: "tea", + command: `node -e "fetch('https://host/api/v1').then(r => r.text())"`, + }, + { + arm: "raw-api", + command: `ruby -e "require 'net/http'; Net::HTTP.get(URI('https://host/api/v1'))"`, + }, +]; + +describe("guardCommand", () => { + it.each(allowedForOwnBinary)( + "permits the $arm arm to run its own allow-listed binary", + ({ arm, command }) => { + expect(guardCommand(arm, command)).toEqual({ allowed: true }); + }, + ); + + it.each(foreignBinaryForArm)( + "denies the $arm arm a command driving the foreign binary $foreign", + ({ arm, command, foreign }) => { + const decision = guardCommand(arm, command); + expect(decision.allowed).toBe(false); + // A genuine foreign-binary denial names the offending binary in its reason, + // distinguishing it from denial for some unrelated cause. + if (decision.allowed === false) { + expect(decision.reason).toContain(foreign); + } + }, + ); + + it("denies a foreign binary reached downstream of a pipe, not only the leading command", () => { + // The tea arm's own binary leads the line and is allow-listed, but a foreign + // interpreter (python3) is reached after the pipe. The guard must inspect + // every binary the command reaches, not just the first token. + const decision = guardCommand("tea", 'tea issues list | python3 -c "import urllib.request"'); + + expect(decision.allowed).toBe(false); + if (decision.allowed === false) { + expect(decision.reason).toContain("python3"); + } + }); + + it.each(foreignAbsolutePathForArm)( + "denies the $arm arm a foreign binary named by absolute path", + ({ arm, command }) => { + const decision = guardCommand(arm, command); + expect(decision.allowed).toBe(false); + }, + ); + + it.each(interpreterFetchForArm)( + "denies the $arm arm an interpreter-based fetch over HTTP", + ({ arm, command }) => { + const decision = guardCommand(arm, command); + expect(decision.allowed).toBe(false); + }, + ); + + // The gitea-mcp arm reaches Gitea only through its attached MCP tools; its shell + // is disabled entirely, so no command — not even another arm's allow-listed + // binary or a bare harmless utility — may run (ADR 0016). Independent literals. + it.each([ + "gitea-axi issue list", + "tea issues list", + "curl https://host", + "ls", + ])("denies the gitea-mcp arm every shell command, including %j", (command) => { + const decision = guardCommand("gitea-mcp", command); + expect(decision.allowed).toBe(false); + if (decision.allowed === false) { + // The denial is about the shell being off for this arm, not an ordinary + // foreign-binary rejection. + expect(decision.reason).toMatch(/shell/i); + } + }); + + it("denies a foreign binary hidden inside a command substitution", () => { + // The leading curl is allow-listed for raw-api, but python3 hides inside the + // $(...) substitution. The guard must look inside substitutions, not just at + // the top-level command. + const decision = guardCommand( + "raw-api", + `curl -s $(python3 -c "print('https://host')")/api/v1/repos`, + ); + + expect(decision.allowed).toBe(false); + }); + + // The arm's own binary alongside curated harmless utilities (jq, head — which + // cannot reach the network or execute code) is permitted, and shell plumbing + // like a 2>&1 redirection must not be mistaken for a foreign command (ADR 0016). + it.each([ + { arm: "raw-api" as Arm, command: "curl -s https://host/api/v1/repos 2>&1 | head -n 5" }, + { arm: "tea" as Arm, command: "tea issues list --output json | jq '.[].number'" }, + ])( + "permits the $arm arm's binary piped through a curated read-only utility", + ({ arm, command }) => { + expect(guardCommand(arm, command)).toEqual({ allowed: true }); + }, + ); + + it("permits a leading NAME=value assignment before the arm's binary", () => { + // The runner passes the API token via a leading environment assignment; the + // command is the binary that follows, not the assignment itself. + expect(guardCommand("gitea-axi", "TOKEN=secret gitea-axi issue list")).toEqual({ + allowed: true, + }); + }); + + it("denies a path-qualified invocation of the arm's own binary", () => { + // The curated PATH resolves the arm's binary by name; a path-qualified form + // sidesteps that and could resolve to something else via a symlink or copy, + // so it is refused even for the arm's own allow-listed binary. + const decision = guardCommand("tea", "/usr/bin/tea issues list"); + + expect(decision.allowed).toBe(false); + }); +}); + +describe("provisionArmBin", () => { + let binDir: string; + + beforeEach(() => { + binDir = mkdtempSync(join(tmpdir(), "gitea-axi-armbin-")); + }); + + afterEach(() => { + rmSync(binDir, { recursive: true, force: true }); + }); + + // Fake resolver so the test never depends on binaries present on the host. + const locate = (binary: string) => `/fake/prefix/${binary}`; + + // Independent literals: each shell arm's one allow-listed binary (ADR 0016), + // NOT read back from ARM_BINARY. + const shellArmBinary: ReadonlyArray<{ arm: Arm; binary: string }> = [ + { arm: "gitea-axi", binary: "gitea-axi" }, + { arm: "tea", binary: "tea" }, + { arm: "raw-api", binary: "curl" }, + ]; + + it.each(shellArmBinary)( + "exposes only the $arm arm's allow-listed binary $binary as a symlink", + ({ arm, binary }) => { + provisionArmBin(arm, binDir, locate); + + expect(readdirSync(binDir)).toEqual([binary]); + expect(readlinkSync(join(binDir, binary))).toBe(`/fake/prefix/${binary}`); + }, + ); + + it("exposes nothing for the gitea-mcp arm, whose shell is disabled", () => { + provisionArmBin("gitea-mcp", binDir, locate); + + expect(readdirSync(binDir)).toEqual([]); + }); + + it("throws when the arm's binary cannot be located", () => { + expect(() => provisionArmBin("tea", binDir, () => null)).toThrow(/tea/); + }); +}); diff --git a/bench/guard.ts b/bench/guard.ts new file mode 100644 index 0000000..3e8c806 --- /dev/null +++ b/bench/guard.ts @@ -0,0 +1,322 @@ +import { accessSync, constants, mkdirSync, symlinkSync } from "node:fs"; +import { join } from "node:path"; +import type { Arm } from "./result.js"; + +/** + * The single binary each arm's agent is allowed to invoke through the shell. + * `null` marks an arm that runs with the shell disabled entirely (gitea-mcp, + * which reaches Gitea only through its attached MCP tools and so has no shell + * leakage surface at all). + */ +export const ARM_BINARY: Record = { + "gitea-axi": "gitea-axi", + tea: "tea", + "gitea-mcp": null, + "raw-api": "curl", +}; + +/** + * The curated set of harmless utilities any arm may reach in addition to its own + * allow-listed binary. Every entry is a read/text/flow utility that cannot reach + * the network or execute arbitrary code. The set deliberately excludes anything + * that can launch another program or open a socket — language interpreters + * (python, node, ruby, perl, php, lua), shells (sh, bash, zsh), program-launching + * wrappers (env, xargs, find, timeout, nohup, nice), code-capable text tools + * (sed, awk), and every network tool (curl, wget, nc, ssh, git). Those are the + * evasion surface the guard exists to close, so none of them is "harmless". + */ +export const HARMLESS_BINARIES: ReadonlySet = new Set([ + "cat", "head", "tail", "wc", "cut", "tr", "sort", "uniq", "comm", + "grep", "egrep", "fgrep", "diff", "echo", "printf", "ls", "pwd", "cd", + "mkdir", "rmdir", "tee", "test", "[", "true", "false", "basename", + "dirname", "seq", "sleep", "date", "nl", "rev", "tac", "fold", "column", + "expr", "jq", +]); + +/** The guard's verdict on one proposed shell command. */ +export type GuardDecision = { allowed: true } | { allowed: false; reason: string }; + +/** + * The authoritative tool-isolation guard. Inspects a proposed shell command and + * permits it only if every binary it would reach is either the active arm's one + * allow-listed binary or a curated harmless utility. Foreign binaries, + * absolute-path evasions, and interpreter-based fetch tricks are denied. + */ +export function guardCommand(arm: Arm, command: string): GuardDecision { + const allowed = ARM_BINARY[arm]; + if (allowed === null) { + return { + allowed: false, + reason: `the ${arm} arm runs with the shell disabled; only its MCP tools are available`, + }; + } + const commands = extractCommands(command); + if (commands.length === 0) { + return { allowed: false, reason: "no command was found to run" }; + } + for (const name of commands) { + if (name.includes("/")) { + const base = name.slice(name.lastIndexOf("/") + 1); + return { + allowed: false, + reason: `path-qualified command "${name}" is not permitted; invoke "${base}" by name so the ${arm} arm's curated PATH governs which binary resolves`, + }; + } + if (name === allowed) { + continue; + } + if (HARMLESS_BINARIES.has(name)) { + continue; + } + return { + allowed: false, + reason: `"${name}" is not permitted for the ${arm} arm; only "${allowed}" and curated read-only utilities are allowed`, + }; + } + return { allowed: true }; +} + +/** Whether `word` is a leading `NAME=value` environment assignment, not a command. */ +function isAssignment(word: string): boolean { + return /^[A-Za-z_][A-Za-z0-9_]*=/.test(word); +} + +/** + * Extract every command name a shell command line would reach — across pipelines, + * sequences (`;`, `&&`, `||`), subshells, command substitutions (`$(...)` and + * backticks), and process substitutions. Leading `NAME=value` assignments and + * redirections (including forms like `2>&1`) are skipped so only genuine command + * names are returned. The guard checks every returned name, which is what stops a + * foreign binary from hiding downstream of a pipe or inside a substitution. + */ +function extractCommands(command: string): string[] { + const s = command; + const len = s.length; + const cur = { i: 0 }; + const names: string[] = []; + + const isSpace = (c: string | undefined): boolean => c === " " || c === "\t" || c === "\r"; + const isWordBreak = (c: string | undefined): boolean => + c === undefined || + isSpace(c) || + c === "\n" || + c === "|" || + c === "&" || + c === ";" || + c === "(" || + c === ")" || + c === "{" || + c === "}" || + c === "<" || + c === ">" || + c === "`"; + + function skipSpaces(): void { + while (cur.i < len && isSpace(s[cur.i])) cur.i++; + } + + // Read one word starting at cur.i, honoring single/double quotes and backslash + // escapes, and recursing into any `$(...)` / backtick substitution embedded in + // a double-quoted span so its command names are captured too. + function readWord(): string { + let w = ""; + while (cur.i < len) { + const c = s[cur.i]; + if (c === "'") { + cur.i++; + while (cur.i < len && s[cur.i] !== "'") { + w += s[cur.i]; + cur.i++; + } + cur.i++; + continue; + } + if (c === '"') { + cur.i++; + while (cur.i < len && s[cur.i] !== '"') { + if (s[cur.i] === "\\") { + w += s[cur.i + 1] ?? ""; + cur.i += 2; + continue; + } + if (s[cur.i] === "$" && s[cur.i + 1] === "(") { + cur.i += 2; + parseSequence(")"); + continue; + } + if (s[cur.i] === "`") { + cur.i++; + parseSequence("`"); + continue; + } + w += s[cur.i]; + cur.i++; + } + cur.i++; + continue; + } + if (c === "\\") { + w += s[cur.i + 1] ?? ""; + cur.i += 2; + continue; + } + if (c === "$" && s[cur.i + 1] === "(") break; + if (isWordBreak(c)) break; + w += c; + cur.i++; + } + return w; + } + + // Consume a redirection operator (cur.i is at `<` or `>`) and its target, so + // neither the operator nor the target file/descriptor is mistaken for a command. + function consumeRedirection(): void { + cur.i++; // the leading < or > + if (s[cur.i] === ">" || s[cur.i] === "<") cur.i++; // >>, <<, <> + if (s[cur.i] === "&") cur.i++; // >&, <& (duplicate a descriptor) + skipSpaces(); + if (s[cur.i] === "-") { + cur.i++; // close a descriptor: >&- + return; + } + if (cur.i < len && s[cur.i] === "$" && s[cur.i + 1] === "(") { + cur.i += 2; + parseSequence(")"); + return; + } + if (cur.i < len && !isWordBreak(s[cur.i])) { + readWord(); // discard the redirection target + } + } + + // Parse a run of commands until end of input, or until `closer` (`)` for a + // subshell/substitution, `` ` `` for a backtick substitution) is reached. + function parseSequence(closer: string | null): void { + let expectCommand = true; + while (cur.i < len) { + skipSpaces(); + const c = s[cur.i]; + if (c === undefined) break; + if (closer !== null && c === closer) { + cur.i++; + return; + } + if (c === "\n" || c === ";") { + cur.i++; + expectCommand = true; + continue; + } + if (c === "&") { + if (s[cur.i + 1] === ">") { + cur.i++; // `&>` / `&>>` redirect both streams + consumeRedirection(); + continue; + } + cur.i++; + if (s[cur.i] === "&") cur.i++; // && vs background & + expectCommand = true; + continue; + } + if (c === "|") { + cur.i++; + if (s[cur.i] === "|" || s[cur.i] === "&") cur.i++; // ||, |& + expectCommand = true; + continue; + } + if (c === "(") { + cur.i++; + parseSequence(")"); + expectCommand = false; + continue; + } + if (c === "{") { + cur.i++; + expectCommand = true; + continue; + } + if (c === "}") { + cur.i++; + continue; + } + if (c === "`") { + if (closer === "`") { + cur.i++; + return; + } + cur.i++; + parseSequence("`"); + expectCommand = false; + continue; + } + if (c === "$" && s[cur.i + 1] === "(") { + cur.i += 2; + parseSequence(")"); + expectCommand = false; + continue; + } + if (c === "<" || c === ">") { + consumeRedirection(); + continue; + } + const word = readWord(); + if (/^\d+$/.test(word) && (s[cur.i] === "<" || s[cur.i] === ">")) { + consumeRedirection(); // a file-descriptor prefix, e.g. 2>&1 + continue; + } + if (word.length === 0) continue; + if (expectCommand) { + if (isAssignment(word)) continue; // NAME=value prefix; the command follows + names.push(word); + expectCommand = false; + } + } + } + + parseSequence(null); + return names; +} + +/** + * Provision a curated per-arm bin directory that exposes only the arm's one + * allow-listed binary, as a convenience layer behind the authoritative guard. + * + * The directory is populated with a single symlink named after the arm's binary + * pointing at its resolved absolute path, so prepending the directory to PATH + * lets the arm's tool resolve by name while no foreign binary is reachable that + * way. The gitea-mcp arm has no shell binary, so its directory is left empty. + * `locate` resolves a binary name to an absolute path (defaulting to a search of + * the real PATH); a test injects a deterministic fake. + */ +export function provisionArmBin( + arm: Arm, + binDir: string, + locate: (binary: string) => string | null = locateOnPath, +): void { + mkdirSync(binDir, { recursive: true }); + const binary = ARM_BINARY[arm]; + if (binary === null) { + return; // gitea-mcp: the shell is disabled, so nothing is exposed. + } + const target = locate(binary); + if (target === null) { + throw new Error( + `cannot provision the ${arm} arm: its binary "${binary}" was not found on PATH`, + ); + } + symlinkSync(target, join(binDir, binary)); +} + +/** Resolve `binary` to the absolute path of the first executable of that name on PATH. */ +function locateOnPath(binary: string): string | null { + for (const dir of (process.env.PATH ?? "").split(":")) { + if (dir === "") continue; + const candidate = join(dir, binary); + try { + accessSync(candidate, constants.X_OK); + return candidate; + } catch { + // Not here (or not executable); keep looking. + } + } + return null; +}