From 5871aabf1088b386c1f49ac1f94d5bcf41e3e63b Mon Sep 17 00:00:00 2001 From: Chris Lee Date: Sat, 20 Jun 2026 23:09:32 +1000 Subject: [PATCH 1/2] feat(agent-sdk): block destructive Bash at runtime via PreToolUse hook The agent runs bypassPermissions with Bash allowed, so the bans on force push, git reset --hard, gh pr merge, and the GraphQL merge mutations were prompt-only. Add a Claude Agent SDK PreToolUse hook that denies matching Bash commands at runtime (permissionDecision deny), emitting agent.hook.denied (rule label only, never the raw command). Lift the canonical FORBIDDEN set into src/utils/forbidden-bash.ts as the single source of truth shared by the runtime hook, the static check:no-destructive guard, and the regression test. Defense-in-depth backing branch protection, not a sandbox (see #240 for adjacent vectors). Closes #222 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_015v2bspPF1ZTTG9ZZrbBgA1 --- CLAUDE.md | 6 +- docs/build/architecture.md | 1 + docs/operate/observability.md | 8 + scripts/check-no-destructive-actions.ts | 19 +- src/core/executor.ts | 7 + src/core/hooks/forbidden-bash.test.ts | 204 ++++++++++++++++++ src/core/hooks/forbidden-bash.ts | 57 +++++ src/utils/forbidden-bash.ts | 44 ++++ .../ship/destructive-actions.test.ts | 24 +-- 9 files changed, 329 insertions(+), 41 deletions(-) create mode 100644 src/core/hooks/forbidden-bash.test.ts create mode 100644 src/core/hooks/forbidden-bash.ts create mode 100644 src/utils/forbidden-bash.ts diff --git a/CLAUDE.md b/CLAUDE.md index 5b9b8045..c39401f2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -124,11 +124,11 @@ Five workflow files form the pipeline; each owns one responsibility. - **Runners are explicitly pinned repo-wide.** Every `runs-on:` across `.github/workflows/` targets an explicit image (`ubuntu-24.04`), never a `*-latest` rolling alias, so a GitHub-side image bump can't silently change the OS / preinstalled tools / Docker daemon under a job. `ci.yml` runs `bun run check:runner-pins` (`scripts/check-runner-pins.ts`), which fails the build if any `runs-on:` is on a `*-latest` alias; matrix expressions (`runs-on: ${{ matrix.runner }}`) are exempt since the literal lives at the matrix entry. See issue #173. - Defense-in-depth on workflow injection: every dynamic input flowing into a `run:` block is passed via `env:` first. - **GitHub Actions are SHA-pinned.** Every third-party `uses:` reference is pinned to a full 40-char commit SHA (with a `# vX.Y.Z` comment), not a mutable tag, so a force-moved upstream tag cannot change the bytes a runner executes. Renovate keeps the SHAs current via the `helpers:pinGitHubActionDigests` preset behind the existing 7-day `minimumReleaseAge` soak. `ci.yml` runs `bun run check:action-pins` (`scripts/check-action-pins.ts`), which fails the build if any third-party `uses:` is on a tag. Local reusable-workflow calls (`uses: ./...`) are exempt. -- **Test-glob + destructive-action guards run in CI.** `ci.yml` also runs `bun run check:test-globs` (`scripts/check-test-globs.ts`, fails if any `*.test.ts` is unreachable by the runner glob set, issue #201) and `bun run check:no-destructive` (`scripts/check-no-destructive-actions.ts`, FR-009: fails if `src/workflows/ship/` or a `src/daemon/scoped-*-executor.ts` contains a force-push / `reset --hard` / `gh pr merge` / merge-mutation call outside comments; the scoped-executor set is derived from the filesystem so it cannot go stale, issue #203). +- **Test-glob + destructive-action guards run in CI.** `ci.yml` also runs `bun run check:test-globs` (`scripts/check-test-globs.ts`, fails if any `*.test.ts` is unreachable by the runner glob set, issue #201) and `bun run check:no-destructive` (`scripts/check-no-destructive-actions.ts`, FR-009: fails if `src/workflows/ship/` or a `src/daemon/scoped-*-executor.ts` contains a force-push / `reset --hard` / `gh pr merge` / merge-mutation call outside comments; the scoped-executor set is derived from the filesystem so it cannot go stale, issue #203). Its `FORBIDDEN` pattern set is shared with the paired runtime layer, the `PreToolUse` destructive-Bash hook (`src/core/hooks/forbidden-bash.ts`, security invariant #5), via `src/utils/forbidden-bash.ts` so the static and runtime gates stay in lockstep (issue #222). ## Security invariants (prompt-injection hardening) -Four contracts contributors MUST preserve when touching the agent execution path or any GitHub-bound write: +Five contracts contributors MUST preserve when touching the agent execution path or any GitHub-bound write: 1. **Subprocess env allowlist** (`src/core/executor.ts buildProviderEnv()`). The agent CLI receives an explicit allowlist + prefix patterns, NOT `...process.env`. If you add a new env var the CLI needs, extend the allowlist. Banned: `GITHUB_APP_PRIVATE_KEY`, `GITHUB_WEBHOOK_SECRET`, `DAEMON_AUTH_TOKEN`, `DATABASE_URL`, `VALKEY_URL`, `REDIS_URL`, `CONTEXT7_API_KEY`, `GITHUB_PERSONAL_ACCESS_TOKEN`. See `docs/operate/configuration.md` ยง "Subprocess env allowlist". @@ -144,6 +144,8 @@ Four contracts contributors MUST preserve when touching the agent execution path 4. **Structured-output chokepoint** (`src/ai/structured-output.ts parseStructuredResponse()` + `src/utils/tolerant-json.ts`). Every LLM call site that expects a JSON-shaped response MUST route the model output through `parseStructuredResponse(raw, schema)` and append the encoding rules to its system prompt via `withStructuredRules(systemPrompt)`. The pipeline does code-fence stripping, strict JSON.parse, and a tolerant fallback that escapes raw LF/CR/TAB/control bytes inside JSON string values (the most common LLM failure mode, observed shipping on chat-thread). Returns a discriminated `StructuredResult`; callers MUST switch on `result.ok` and own their failure policy (clarify fallback, fail-open, throw, etc.): the pipeline does not encode policy. **Wired today**: `workflows/intent-classifier.ts`, `workflows/ship/nl-classifier.ts`, `workflows/ship/scoped/chat-thread.ts`, `workflows/ship/scoped/meta-issue-classifier.ts`, `orchestrator/triage.ts`, `utils/llm-output-scanner.ts`, `workflows/handlers/triage.ts`. When adding a new LLM call site that expects JSON, do NOT call `JSON.parse` directly, route through this chokepoint so future LLM-quirk fixes apply uniformly. Strategy is observable via `result.strategy` (`"strict"` | `"tolerant"`) for monitoring model JSON-quality regressions. +5. **Runtime destructive-Bash gate** (`src/core/hooks/forbidden-bash.ts` + `src/utils/forbidden-bash.ts`). The agent subprocess runs under `bypassPermissions` with the Bash tool allowed, so a prompt-injected force-push / `git reset --hard` / `gh pr merge` / GraphQL merge mutation (`mergePullRequest` / `mergeBranch`) would otherwise execute unchecked. A `PreToolUse` hook (wired in `src/core/executor.ts` `queryOptions.hooks`, matcher `Bash`) denies any Bash command matching the shared `FORBIDDEN` pattern set at runtime, backing the prompt-only bans with an enforced gate. The patterns live in `src/utils/forbidden-bash.ts` as the single source of truth, shared with the static `check:no-destructive` CI guard (`scripts/check-no-destructive-actions.ts`) so the build-time and runtime layers cannot drift. A deny emits `event: "agent.hook.denied"` (fields `tool`, `rule`) and NEVER logs the raw command (token-leak risk). This stops literal destructive commands and is backed by remote branch protection plus a least-privilege token; a regex denylist on free-form shell is not a complete sandbox (it cannot defeat obfuscation like variable indirection or `base64|sh`). Issue #222. + The `triggerUsername` is rejected (not silently stripped) if it contains whitespace/newline, git commit trailer forging vector. Don't relax that check. ## Documentation diff --git a/docs/build/architecture.md b/docs/build/architecture.md index 7612ac28..4c582947 100644 --- a/docs/build/architecture.md +++ b/docs/build/architecture.md @@ -50,6 +50,7 @@ flowchart TD - **The webhook server never runs the pipeline.** Only daemons execute `runPipeline`. The webhook server is the orchestrator: it enqueues jobs and optionally spawns ephemeral daemons. - **Every orchestrator runs a queue worker.** `src/orchestrator/queue-worker.ts` polls `queue:jobs` via `LMOVE` into a per-instance processing list (`queue:processing:{instanceId}`), offers the job to a locally-connected daemon, and atomically re-queues it to the head when no local daemon can take it. Multi-orchestrator HA: `LMOVE` grants exactly-once claim across instances; the offer/accept round-trip stays in-process. Crash recovery is handled by each orchestrator draining its own processing list at startup, plus a cross-instance reaper (`src/orchestrator/valkey-cleanup.ts`) draining processing lists owned by instances whose `orchestrator:{id}:alive` liveness key has expired. - **MCP servers.** Tracking-comment updates, inline PR reviews, scoped review-thread resolves, daemon-capability reports, repo-memory, and (optionally) Context7 library docs are exposed as MCP servers the agent can call. Git changes are made via the Bash tool against the cloned repo, not through a dedicated MCP server. +- **Destructive Bash is runtime-gated.** The agent runs under `bypassPermissions` with the Bash tool allowed, so prompt-only bans alone do not stop a prompt-injected force-push or merge. A `PreToolUse` hook (`src/core/hooks/forbidden-bash.ts`, wired in `src/core/executor.ts`) denies any Bash command matching the shared `FORBIDDEN` set (force-push, `git reset --hard`, branch delete, history rewrite, `gh pr merge`, GraphQL merge mutations) before it executes. The pattern set is shared with the static `check:no-destructive` CI guard via `src/utils/forbidden-bash.ts`, so build-time and runtime gates cannot drift. A deny emits an `agent.hook.denied` log line. ## Dispatch flow diff --git a/docs/operate/observability.md b/docs/operate/observability.md index 90539a34..c680fce2 100644 --- a/docs/operate/observability.md +++ b/docs/operate/observability.md @@ -76,6 +76,14 @@ cache_read_input_tokens / (input_tokens + cache_read_input_tokens + cache_creati | ----------------- | ----- | ------------------------------------------------------------------------------------------------------------ | | `workspace.sweep` | info | `swept` (entries removed), `retained` (entries kept as fresh), `durationMs` (wall-clock time for the sweep). | +### Agent hook denials + +The `PreToolUse` destructive-Bash hook (`src/core/hooks/forbidden-bash.ts`) emits one line each time it blocks a Bash command matching the shared `FORBIDDEN` set (force-push, `git reset --hard`, branch delete, history rewrite, `gh pr merge`, GraphQL merge mutations). A sustained `agent.hook.denied` rate means the agent is repeatedly attempting destructive operations, worth investigating as a possible prompt-injection signal. The raw command is never logged (token-leak risk); only the matched rule label is. + +| `event` | Level | Fields | +| ------------------- | ----- | -------------------------------------------------------------------------------------------- | +| `agent.hook.denied` | warn | `tool` (always `Bash`), `rule` (the matched FORBIDDEN description, e.g. `git push --force`). | + ## GitHub API rate-limit fields The `App` is constructed with `ObservableOctokit` (`src/utils/octokit-observability.ts`), an `Octokit.plugin` subclass shared by `app.octokit` and every installation octokit. It logs GitHub's per-installation rate-limit headers via `octokit.hook.after` / `hook.error`. The `pipeline.stage`-style strict Zod schema (`GithubApiLogFieldsSchema`) pins the field shape. diff --git a/scripts/check-no-destructive-actions.ts b/scripts/check-no-destructive-actions.ts index 5d013578..d8ea0e5b 100644 --- a/scripts/check-no-destructive-actions.ts +++ b/scripts/check-no-destructive-actions.ts @@ -12,6 +12,8 @@ import { readdirSync, readFileSync, statSync } from "node:fs"; import { join } from "node:path"; +import { FORBIDDEN } from "../src/utils/forbidden-bash"; + // T046b scope: the NEW ship_intents lifecycle code under // `src/workflows/ship/`. Legacy handlers under `src/workflows/handlers/` // embed agent prompt strings that DOCUMENT what the agent must not do @@ -52,23 +54,6 @@ function scopedExecutorFiles(): string[] { return files; } -const FORBIDDEN: { readonly pattern: RegExp; readonly description: string }[] = [ - { pattern: /git\s+push\s+--force(?!-with-lease-if)/i, description: "git push --force" }, - { pattern: /git\s+push\s+--force-with-lease/i, description: "git push --force-with-lease" }, - { pattern: /git\s+push\s+-f\b/i, description: "git push -f" }, - { pattern: /git\s+push\s+\+/, description: "git push with + force-refspec" }, - { pattern: /git\s+push\s+--mirror/i, description: "git push --mirror" }, - { pattern: /git\s+reset\s+--hard\b/i, description: "git reset --hard" }, - { pattern: /git\s+branch\s+-D\b/i, description: "git branch -D" }, - { pattern: /git\s+push\b[^"\n]*\s--delete\b/i, description: "git push --delete" }, - { pattern: /git\s+filter-branch/i, description: "git filter-branch" }, - { pattern: /git\s+filter-repo/i, description: "git filter-repo" }, - { pattern: /git\s+replace\b/i, description: "git replace" }, - { pattern: /\bgh\s+pr\s+merge/i, description: "gh pr merge" }, - { pattern: /mergePullRequest\s*\(/, description: "mergePullRequest GraphQL mutation" }, - { pattern: /mergeBranch\s*\(/, description: "mergeBranch GraphQL mutation" }, -]; - function* walk(dir: string): Generator { // Fail-closed: this is a CI safety guard. Swallowing readdir/stat errors // would let a missing or unreadable scan root report `clean` and silently diff --git a/src/core/executor.ts b/src/core/executor.ts index a7b28eb0..c51203bb 100644 --- a/src/core/executor.ts +++ b/src/core/executor.ts @@ -3,6 +3,7 @@ import { type ModelUsage, query, type SDKResultMessage } from "@anthropic-ai/cla import { config } from "../config"; import type { BotContext, ExecutionResult, McpServerConfig, ModelUsageEntry } from "../types"; import { redactSecrets } from "../utils/sanitize"; +import { createForbiddenBashHook } from "./hooks/forbidden-bash"; function isResultMessage(msg: unknown): msg is SDKResultMessage { return ( @@ -266,6 +267,12 @@ export async function executeAgent({ // single fat tracking-comment dump. Block it so the model uses the eager // tool list delivered in the SDK init message instead. disallowedTools: ["ToolSearch"], + // Runtime backstop for the prompt-only destructive-action bans (issue #222): + // the agent runs bypassPermissions with Bash allowed, so this PreToolUse + // hook denies force-push / reset --hard / gh pr merge / GraphQL merge calls + // before they execute. Patterns are shared with the static check:no-destructive + // CI guard via src/utils/forbidden-bash.ts. + hooks: { PreToolUse: [{ matcher: "Bash", hooks: [createForbiddenBashHook(log)] }] }, // cwd is the cloned PR head (checkout.ts), which can carry an attacker's // .claude/settings.json. Omitting settingSources makes the SDK load // user/project/local filesystem settings by default (v0.3.x), so a diff --git a/src/core/hooks/forbidden-bash.test.ts b/src/core/hooks/forbidden-bash.test.ts new file mode 100644 index 00000000..9c941c2f --- /dev/null +++ b/src/core/hooks/forbidden-bash.test.ts @@ -0,0 +1,204 @@ +import { describe, expect, it } from "bun:test"; + +import type { Logger } from "../../logger"; +import { createForbiddenBashHook } from "./forbidden-bash"; + +// Minimal logger stub: records `.warn` calls so a deny can be asserted. The +// factory's `log` param is a pino Logger in production; only `.warn` is used +// here, so the cast at the call site keeps the test honest without pulling in +// the full pino surface. +function makeLog() { + const warns: { obj: Record; msg: string }[] = []; + const log = { + warn: (obj: Record, msg: string) => warns.push({ obj, msg }), + info: () => {}, + error: () => {}, + debug: () => {}, + }; + return { log, warns }; +} + +// Invoke the hook with a Bash command. Input is cast so the test does not need +// to import SDK hook-input types. `overrides` lets a case swap the event name or +// tool_input to exercise the non-PreToolUse and malformed-input branches. +async function run( + hook: ReturnType, + command: string, + toolName = "Bash", + overrides: { hookEventName?: string; toolInput?: unknown } = {}, +): Promise<{ + hookSpecificOutput?: { permissionDecision?: string; permissionDecisionReason?: string }; +}> { + return hook( + { + hook_event_name: overrides.hookEventName ?? "PreToolUse", + tool_name: toolName, + tool_input: "toolInput" in overrides ? overrides.toolInput : { command }, + tool_use_id: "t1", + } as unknown as Parameters[0], + "t1", + { signal: new AbortController().signal }, + ) as Promise<{ + hookSpecificOutput?: { permissionDecision?: string; permissionDecisionReason?: string }; + }>; +} + +describe("createForbiddenBashHook", () => { + it("denies git push --force", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, "git push --force origin feature"); + expect(result.hookSpecificOutput?.permissionDecision).toBe("deny"); + expect(typeof result.hookSpecificOutput?.permissionDecisionReason).toBe("string"); + expect(result.hookSpecificOutput?.permissionDecisionReason?.length).toBeGreaterThan(0); + }); + + it("denies git push -f", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, "git push -f origin feature"); + expect(result.hookSpecificOutput?.permissionDecision).toBe("deny"); + expect(typeof result.hookSpecificOutput?.permissionDecisionReason).toBe("string"); + expect(result.hookSpecificOutput?.permissionDecisionReason?.length).toBeGreaterThan(0); + }); + + it("denies git push --force-with-lease", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, "git push --force-with-lease origin feature"); + expect(result.hookSpecificOutput?.permissionDecision).toBe("deny"); + expect(typeof result.hookSpecificOutput?.permissionDecisionReason).toBe("string"); + expect(result.hookSpecificOutput?.permissionDecisionReason?.length).toBeGreaterThan(0); + }); + + it("denies git reset --hard", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, "git reset --hard HEAD~1"); + expect(result.hookSpecificOutput?.permissionDecision).toBe("deny"); + expect(typeof result.hookSpecificOutput?.permissionDecisionReason).toBe("string"); + expect(result.hookSpecificOutput?.permissionDecisionReason?.length).toBeGreaterThan(0); + }); + + it("denies gh pr merge", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, "gh pr merge 5 --squash"); + expect(result.hookSpecificOutput?.permissionDecision).toBe("deny"); + expect(typeof result.hookSpecificOutput?.permissionDecisionReason).toBe("string"); + expect(result.hookSpecificOutput?.permissionDecisionReason?.length).toBeGreaterThan(0); + }); + + it("denies a graphql mergePullRequest mutation", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run( + hook, + `gh api graphql -f query='mutation { mergePullRequest(input: {pullRequestId: "x"}) { clientMutationId } }'`, + ); + expect(result.hookSpecificOutput?.permissionDecision).toBe("deny"); + expect(typeof result.hookSpecificOutput?.permissionDecisionReason).toBe("string"); + expect(result.hookSpecificOutput?.permissionDecisionReason?.length).toBeGreaterThan(0); + }); + + it("allows a normal push", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, "git push origin HEAD"); + expect(result.hookSpecificOutput?.permissionDecision).toBeUndefined(); + }); + + it("allows git status", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, "git status"); + expect(result.hookSpecificOutput?.permissionDecision).toBeUndefined(); + }); + + it("allows gh pr checks", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, "gh pr checks 5"); + expect(result.hookSpecificOutput?.permissionDecision).toBeUndefined(); + }); + + it("allows git reset --soft", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, "git reset --soft HEAD~1"); + expect(result.hookSpecificOutput?.permissionDecision).toBeUndefined(); + }); + + it("allows git commit", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, `git commit -m "fix: x"`); + expect(result.hookSpecificOutput?.permissionDecision).toBeUndefined(); + }); + + it("logs agent.hook.denied with the rule on deny", async () => { + const { log, warns } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + await run(hook, "git push --force origin SECRET_BRANCH_token"); + expect(warns.length).toBe(1); + expect(warns[0]?.obj).toMatchObject({ event: "agent.hook.denied", tool: "Bash" }); + // The raw command must never be logged. The static rule label + // ("git push --force") may appear since it carries no attacker input, but the + // command's attacker-controlled tail (the ref/args) must not leak. + expect(JSON.stringify(warns[0]?.obj)).not.toContain("SECRET_BRANCH_token"); + // Exact key set: a future change adding a raw-command field would fail here. + expect(Object.keys(warns[0]!.obj).sort()).toEqual(["event", "rule", "tool"]); + }); + + it("allows a non-Bash tool", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, "git push --force origin feature", "Read"); + expect(result.hookSpecificOutput?.permissionDecision).toBeUndefined(); + }); + + // One representative command per FORBIDDEN rule: a deny here proves the rule + // fires, and the +refspec entry proves the broadened pattern from issue #222. + const denyCases: readonly [label: string, command: string][] = [ + ["git push --force", "git push --force origin main"], + ["git push --force-with-lease", "git push --force-with-lease origin main"], + ["git push -f", "git push -f origin main"], + ["git push +refspec", "git push origin +HEAD:main"], + ["git push --mirror", "git push --mirror origin"], + ["git reset --hard", "git reset --hard HEAD~1"], + ["git branch -D", "git branch -D feature"], + ["git push --delete", "git push origin --delete feature"], + ["git filter-branch", "git filter-branch --tree-filter x HEAD"], + ["git filter-repo", "git filter-repo --path x"], + ["git replace", "git replace a b"], + ["gh pr merge", "gh pr merge 5 --squash"], + [ + "mergePullRequest mutation", + "gh api graphql -f query='mutation { mergePullRequest(input:{}) { x } }'", + ], + ["mergeBranch mutation", "gh api graphql -f query='mutation { mergeBranch(input:{}) { x } }'"], + ]; + + it.each(denyCases)("denies %s", async (_label, command) => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, command); + expect(result.hookSpecificOutput?.permissionDecision).toBe("deny"); + }); + + it("allows malformed tool_input (no command key)", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, "", "Bash", { toolInput: {} }); + expect(result.hookSpecificOutput?.permissionDecision).not.toBe("deny"); + }); + + it("allows a non-PreToolUse event", async () => { + const { log } = makeLog(); + const hook = createForbiddenBashHook(log as unknown as Logger); + const result = await run(hook, "git push --force origin main", "Bash", { + hookEventName: "Stop", + }); + expect(result.hookSpecificOutput?.permissionDecision).not.toBe("deny"); + }); +}); diff --git a/src/core/hooks/forbidden-bash.ts b/src/core/hooks/forbidden-bash.ts new file mode 100644 index 00000000..04bbb619 --- /dev/null +++ b/src/core/hooks/forbidden-bash.ts @@ -0,0 +1,57 @@ +import type { HookCallback, PreToolUseHookInput } from "@anthropic-ai/claude-agent-sdk"; + +import type { Logger } from "../../logger"; +import { findForbiddenBash } from "../../utils/forbidden-bash"; + +/** + * Runtime backstop for the prompt-only destructive-action bans (issue #222). + * + * The agent subprocess runs under `bypassPermissions` with the Bash tool + * allowed, so a prompt-injected force-push / `git reset --hard` / `gh pr merge` + * / GraphQL merge mutation would otherwise execute unchecked. This PreToolUse + * hook denies any Bash command matching the shared FORBIDDEN set before the + * tool runs, backing the prompt instructions with an enforced gate. + * + * Patterns are shared with the static `check:no-destructive` CI guard via + * `src/utils/forbidden-bash.ts`, so the two layers stay in lockstep. + * + * This is a defense-in-depth backstop for the naive/literal destructive command, + * not a sandbox: a regex denylist on free-form shell cannot defeat obfuscation + * (variable indirection, command substitution, base64|sh). The durable control is + * remote branch protection plus a least-privilege token. + */ +export function createForbiddenBashHook(log: Logger): HookCallback { + // Non-async: the HookCallback contract needs a Promise return, but the body + // does no awaiting, so wrap the result in Promise.resolve to satisfy the type + // without a lint-flagged empty async (require-await). + return (input, _toolUseId, _opts) => { + // input is the HookInput discriminated union; the event-name guard narrows + // it to PreToolUseHookInput, so no cast is needed. + if (input.hook_event_name !== "PreToolUse") return Promise.resolve({}); + const pre: PreToolUseHookInput = input; + if (pre.tool_name !== "Bash") return Promise.resolve({}); + + const ti = pre.tool_input; + const command = + typeof ti === "object" && ti !== null && "command" in ti && typeof ti.command === "string" + ? ti.command + : ""; + + const hit = findForbiddenBash(command); + if (!hit) return Promise.resolve({}); + + // Never log the raw command: it can carry tokens/secrets (token-leak risk). + log.warn( + { event: "agent.hook.denied", tool: "Bash", rule: hit.description }, + "Blocked destructive Bash command", + ); + + return Promise.resolve({ + hookSpecificOutput: { + hookEventName: "PreToolUse" as const, + permissionDecision: "deny" as const, + permissionDecisionReason: `Blocked by forbidden-bash hook: ${hit.description}`, + }, + }); + }; +} diff --git a/src/utils/forbidden-bash.ts b/src/utils/forbidden-bash.ts new file mode 100644 index 00000000..1220b0e3 --- /dev/null +++ b/src/utils/forbidden-bash.ts @@ -0,0 +1,44 @@ +/** + * Single source of truth for the destructive-action pattern set. + * + * Shared by two layers (issue #222): + * - the static CI guard `scripts/check-no-destructive-actions.ts`, which greps + * ship-workflow source for these patterns at build time, and + * - the runtime PreToolUse hook `src/core/hooks/forbidden-bash.ts`, which + * denies a matching Bash command before the agent subprocess executes it. + * + * Keeping the list in one module guarantees the static and runtime layers can + * never drift apart. + */ + +export interface ForbiddenRule { + readonly pattern: RegExp; + readonly description: string; +} + +export const FORBIDDEN: readonly ForbiddenRule[] = [ + { pattern: /git\s+push\s+--force/i, description: "git push --force" }, + { pattern: /git\s+push\s+--force-with-lease/i, description: "git push --force-with-lease" }, + { pattern: /git\s+push\s+-f\b/i, description: "git push -f" }, + // Whitespace then `+` then a word char catches `+HEAD:main` after a remote; + // a JS concat like `" + branch"` has a space after `+` so it does not match. + { pattern: /git\s+push\b[^|;&\n]*\s\+\w/i, description: "git push with + force-refspec" }, + { pattern: /git\s+push\s+--mirror/i, description: "git push --mirror" }, + { pattern: /git\s+reset\s+--hard\b/i, description: "git reset --hard" }, + { pattern: /git\s+branch\s+-D\b/i, description: "git branch -D" }, + { pattern: /git\s+push\b[^"\n]*\s--delete\b/i, description: "git push --delete" }, + { pattern: /git\s+filter-branch/i, description: "git filter-branch" }, + { pattern: /git\s+filter-repo/i, description: "git filter-repo" }, + { pattern: /git\s+replace\b/i, description: "git replace" }, + { pattern: /\bgh\s+pr\s+merge/i, description: "gh pr merge" }, + { pattern: /mergePullRequest\s*\(/, description: "mergePullRequest GraphQL mutation" }, + { pattern: /mergeBranch\s*\(/, description: "mergeBranch GraphQL mutation" }, +]; + +/** + * First FORBIDDEN entry whose pattern matches `command`, else undefined. + * Patterns carry no /g flag, so reusing them across calls via .test() is safe. + */ +export function findForbiddenBash(command: string): ForbiddenRule | undefined { + return FORBIDDEN.find((entry) => entry.pattern.test(command)); +} diff --git a/test/workflows/ship/destructive-actions.test.ts b/test/workflows/ship/destructive-actions.test.ts index 8a83dd4d..9825372e 100644 --- a/test/workflows/ship/destructive-actions.test.ts +++ b/test/workflows/ship/destructive-actions.test.ts @@ -23,6 +23,8 @@ import { join } from "node:path"; import { describe, expect, it } from "bun:test"; +import { FORBIDDEN } from "../../../src/utils/forbidden-bash"; + // Mirrors the SCAN_ROOTS list in scripts/check-no-destructive-actions.ts. // The static scan is intentionally restricted to `src/workflows/ship/`, // the legacy `src/workflows/handlers/{ship,resolve,review,branch-refresh}.ts` @@ -34,28 +36,6 @@ import { describe, expect, it } from "bun:test"; // JS, so a static text scan would not catch a runtime leak anyway. const SCAN_ROOTS = ["src/workflows/ship"]; -interface ForbiddenRule { - readonly pattern: RegExp; - readonly description: string; -} - -const FORBIDDEN: readonly ForbiddenRule[] = [ - { pattern: /git\s+push\s+--force(?!-with-lease-if)/i, description: "git push --force" }, - { pattern: /git\s+push\s+--force-with-lease/i, description: "git push --force-with-lease" }, - { pattern: /git\s+push\s+-f\b/i, description: "git push -f" }, - { pattern: /git\s+push\s+\+/, description: "git push with + force-refspec" }, - { pattern: /git\s+push\s+--mirror/i, description: "git push --mirror" }, - { pattern: /git\s+reset\s+--hard\b/i, description: "git reset --hard" }, - { pattern: /git\s+branch\s+-D\b/i, description: "git branch -D" }, - { pattern: /git\s+push\b[^"\n]*\s--delete\b/i, description: "git push --delete" }, - { pattern: /git\s+filter-branch/i, description: "git filter-branch" }, - { pattern: /git\s+filter-repo/i, description: "git filter-repo" }, - { pattern: /git\s+replace\b/i, description: "git replace" }, - { pattern: /\bgh\s+pr\s+merge/i, description: "gh pr merge" }, - { pattern: /mergePullRequest\s*\(/, description: "mergePullRequest GraphQL mutation" }, - { pattern: /mergeBranch\s*\(/, description: "mergeBranch GraphQL mutation" }, -]; - function* walkTs(rootPath: string): Generator { const stat = statSync(rootPath); if (stat.isFile()) { From d5271f10de820e89b983a97ff72b7086c1ede5b8 Mon Sep 17 00:00:00 2001 From: Chris Lee Date: Sat, 20 Jun 2026 23:15:02 +1000 Subject: [PATCH 2/2] fix(agent-sdk): make --force and --force-with-lease patterns mutually exclusive Address review: a --force-with-lease command now reports its specific rule and is not double-counted by the static scanner. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_015v2bspPF1ZTTG9ZZrbBgA1 --- src/utils/forbidden-bash.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/utils/forbidden-bash.ts b/src/utils/forbidden-bash.ts index 1220b0e3..7689e5b9 100644 --- a/src/utils/forbidden-bash.ts +++ b/src/utils/forbidden-bash.ts @@ -17,7 +17,11 @@ export interface ForbiddenRule { } export const FORBIDDEN: readonly ForbiddenRule[] = [ - { pattern: /git\s+push\s+--force/i, description: "git push --force" }, + // `(?!-with-lease)` keeps this rule mutually exclusive with the next one, so + // `--force-with-lease` reports the specific rule (not "git push --force") and + // the static scanner does not double-count one line. `--force-if-includes` + // still matches here, which is intended. + { pattern: /git\s+push\s+--force(?!-with-lease)/i, description: "git push --force" }, { pattern: /git\s+push\s+--force-with-lease/i, description: "git push --force-with-lease" }, { pattern: /git\s+push\s+-f\b/i, description: "git push -f" }, // Whitespace then `+` then a word char catches `+HEAD:main` after a remote;