diff --git a/docs/adr/0001-recover-full-bash-command-from-session.md b/docs/adr/0001-recover-full-bash-command-from-session.md index ac9b942..1b42026 100644 --- a/docs/adr/0001-recover-full-bash-command-from-session.md +++ b/docs/adr/0001-recover-full-bash-command-from-session.md @@ -15,17 +15,40 @@ Proceed only when all evidence agrees: - the request and recovered tool call both identify the native `bash` tool; - the tool-call ID has exactly one match within the latest assistant message that contains it (a cross-message reuse resolves to the latest, which is the call being authorized); - `arguments.command` is a string; -- the complete input matches the strict wrapper grammar; +- the ask-triggering unit (`details.command`) matches the wrapper grammar; - the inner command is not another recognized wrapper. -V0.1 recognizes only: +V0.1 recognizes: ```regex -^timeout[ \t]+([1-9][0-9]*[smhd])[ \t]+(.+)$ +^timeout[ \t]+([1-9][0-9]*(?:\.[0-9]+)?[smhd]?)[ \t]+(.+)$ ``` +The duration follows GNU timeout's grammar: a positive number (integer or +decimal) with an optional unit `s`/`m`/`h`/`d` (default seconds), so a bare +integer such as `timeout 240 cmd` is accepted. The duration format is irrelevant +to unwrap soundness — the duration is discarded and only the inner command is +re-evaluated — so the full numeric grammar is allowed. `0`/leading-zero, `ms`, +and flags remain excluded. (Amended from the original unit-required grammar +after `timeout 240 …` was seen in real usage.) + The complete inner command is re-evaluated through the Deterministic Permission Policy. Map `allow` to `allow`, `deny` to `deny`, and `ask` to `defer`. Missing or ambiguous session evidence, forwarded requests, shell aliases, unsupported `timeout` options, nested wrappers, parse failures, and exceptions all produce `defer`. +**Scaffolded commands (amended).** The recovered full command may be a compound +that does not start with the wrapper — e.g. a subagent scaffold +`cd … && timeout … | tail; echo EXIT`. Detection therefore runs on +`details.command` (the unit the permission system isolated, always +wrapper-leading). The wrapper is stripped from the *full* command (the unit +text is replaced by its unwrapped inner, required to occur exactly once) and the +entire de-wrapped compound is re-evaluated through the Deterministic Permission +Policy, which decomposes it into units and keeps the most restrictive — so +sibling commands are still judged and cannot hide behind the wrapper's allow. If +the unit is not a recognized wrapper, cannot be located exactly once, or the +re-evaluation is not `allow`, the authorizer defers fail-closed. (The original +design detected on the full command only, which missed every scaffolded command; +`details.command` is now used for *detection and locating* only — judgment still +runs on the full command, preserving the "never trust a truncated unit" rule.) + ## Consequences This avoids changing permission-system and avoids parsing authorization evidence from the human-facing `details.message`. It deliberately supports fewer contexts: forwarded and non-native shell calls continue through the existing authority chain. diff --git a/packages/pi-permission-inner-cmd/src/handlers/timeout.ts b/packages/pi-permission-inner-cmd/src/handlers/timeout.ts index 7b970a8..3efd149 100644 --- a/packages/pi-permission-inner-cmd/src/handlers/timeout.ts +++ b/packages/pi-permission-inner-cmd/src/handlers/timeout.ts @@ -1,56 +1,128 @@ -import { classifyWrapper, isRecognizedWrapper } from "../recognizer"; +import { + isRecognizedWrapper, + parseTimeoutWrapper, + TIMEOUT_PREFIX, +} from "../recognizer"; import type { CommandHandler } from "./types"; /** Bash permission surface queried when re-evaluating the inner command. */ const BASH_SURFACE = "bash"; /** - * The strict simple-timeout wrapper handler (ADR 0001). + * Replace the wrapper unit with its unwrapped inner inside the full command, + * exactly once. Returns `undefined` when the unit is not a unique substring + * (absent, or appears more than once), so the caller defers fail-closed rather + * than guess where to strip. + */ +function stripWrapperUnit( + fullCommand: string, + unit: string, + inner: string, +): string | undefined { + const first = fullCommand.indexOf(unit); + if (first === -1) { + return undefined; + } + if (fullCommand.indexOf(unit, first + unit.length) !== -1) { + return undefined; + } + return ( + fullCommand.slice(0, first) + + inner + + fullCommand.slice(first + unit.length) + ); +} + +/** + * The simple-timeout wrapper handler (ADR 0001). * - * Unwraps `timeout `, rejects nested wrappers, and - * re-evaluates the complete inner program through the deterministic policy. - * Unsupported timeout syntax and nested wrappers defer with a debug log. - * Commands that are not timeout at all return `undefined` so the engine can try - * the next handler. + * Detection runs on `details.command` — the command unit the permission system + * isolated as the ask trigger — which is always wrapper-leading even when the + * full recovered command is a scaffold that starts with `cd`/`echo`/…. The + * wrapper is then stripped from the FULL command and the whole de-wrapped + * compound is re-evaluated, so sibling commands (including dangerous ones) are + * still judged and cannot hide behind the wrapper's allow. + * + * Unsupported timeout syntax, a nested wrapper, a unit that cannot be located + * exactly once in the full command, and any non-allowing re-evaluation all + * defer fail-closed. */ export const timeoutHandler: CommandHandler = { id: "timeout", decide(ctx) { - const { command, details, query, log, evidence } = ctx; - const classification = classifyWrapper(command); - switch (classification.kind) { - case "nonTimeout": - return undefined; - case "unsupportedTimeout": - log.debug("inner_cmd.unsupported_timeout_syntax", { command }); + const { command: fullCommand, details, query, log, evidence } = ctx; + const unit = details.command; + if (unit === undefined) { + return undefined; + } + + const unitMatch = parseTimeoutWrapper(unit); + if (unitMatch === undefined) { + // Not the recognized form. If it still names `timeout`, surface it + // as unsupported; otherwise this unit is not ours. + if (TIMEOUT_PREFIX.test(unit)) { + log.debug("inner_cmd.unsupported_timeout_syntax", { + command: fullCommand, + }); return { kind: "defer" }; - case "recognized": { - const innerCommand = classification.match.innerCommand; - // Record the derived inner command so the engine's exception - // log retains it if the re-evaluation below throws. - evidence.innerCommand = innerCommand; - if (isRecognizedWrapper(innerCommand)) { - log.debug("inner_cmd.nested_timeout", { command, innerCommand }); - return { kind: "defer" }; - } - const result = query.checkPermission( - BASH_SURFACE, - innerCommand, - details.agentName ?? undefined, - ); - switch (result.state) { - case "allow": - log.review("inner_cmd.allow", { command, innerCommand }); - return { kind: "allow" }; - case "deny": - log.review("inner_cmd.deny", { command, innerCommand }); - return { kind: "deny" }; - case "ask": - default: - log.debug("inner_cmd.inner_ask", { command, innerCommand }); - return { kind: "defer" }; - } } + return undefined; + } + + const innerCommand = unitMatch.innerCommand; + evidence.innerCommand = innerCommand; + + // Never unwrap into another wrapper. + if (isRecognizedWrapper(innerCommand)) { + log.debug("inner_cmd.nested_timeout", { + command: fullCommand, + innerCommand, + }); + return { kind: "defer" }; + } + + // Strip the wrapper from the full command (handles scaffolds). Defer + // fail-closed if the unit is not a unique substring. + const unwrappedFull = stripWrapperUnit( + fullCommand, + unit, + innerCommand, + ); + if (unwrappedFull === undefined) { + log.debug("inner_cmd.wrapper_not_located", { + command: fullCommand, + }); + return { kind: "defer" }; + } + + // Authoritative: re-evaluate the full de-wrapped compound. The + // permission system decomposes it into units and keeps the most + // restrictive, so any non-allowing sibling defers here. + const result = query.checkPermission( + BASH_SURFACE, + unwrappedFull, + details.agentName ?? undefined, + ); + switch (result.state) { + case "allow": + log.review("inner_cmd.allow", { + command: fullCommand, + innerCommand, + }); + return { kind: "allow" }; + case "deny": + log.review("inner_cmd.deny", { + command: fullCommand, + innerCommand, + }); + return { kind: "deny" }; + case "ask": + default: + log.debug("inner_cmd.inner_ask", { + command: fullCommand, + innerCommand, + }); + return { kind: "defer" }; } }, }; diff --git a/packages/pi-permission-inner-cmd/src/recognizer.ts b/packages/pi-permission-inner-cmd/src/recognizer.ts index e83bd3c..2eb51e5 100644 --- a/packages/pi-permission-inner-cmd/src/recognizer.ts +++ b/packages/pi-permission-inner-cmd/src/recognizer.ts @@ -1,23 +1,28 @@ /** * V0.1 wrapper recognizer. * - * The strict simple-timeout grammar from ADR 0001. V0.1 unwraps exactly + * The simple-timeout grammar from ADR 0001. V0.1 unwraps exactly * `timeout `; every other `timeout` invocation is left to * the next authority. */ /** - * Matches `timeout ` where `` is a positive - * integer (no leading zero) followed by a single unit `s`/`m`/`h`/`d`. + * Matches `timeout ` where `` follows GNU + * timeout's grammar: a positive number (integer or decimal, no leading zero) + * with an optional unit `s`/`m`/`h`/`d` (default seconds). A bare integer such + * as `timeout 240 cmd` is therefore accepted (240 seconds). * - * Flags (`-k`, `--preserve-status`, GNU `--`), compound durations, and the - * bare form are intentionally excluded so v0.1 never unwraps a wrapper it - * cannot re-evaluate safely. + * The duration format is irrelevant to unwrap soundness — the duration is + * discarded and only the inner command is re-evaluated — so GNU's full numeric + * grammar is accepted. Still excluded: `0`/leading-zero durations, `ms` (not a + * timeout unit), multi-letter units, and flags (`-k`, `--preserve-status`, GNU + * `--`), so a wrapper that cannot be re-evaluated safely is never unwrapped. */ -const TIMEOUT_WRAPPER_PATTERN = /^timeout[ \t]+([1-9][0-9]*[smhd])[ \t]+(.+)$/; +const TIMEOUT_WRAPPER_PATTERN = + /^timeout[ \t]+([1-9][0-9]*(?:\.[0-9]+)?[smhd]?)[ \t]+(.+)$/; /** A command that begins with the bare `timeout` wrapper program. */ -const TIMEOUT_PREFIX = /^timeout(?:[ \t]|$)/; +export const TIMEOUT_PREFIX = /^timeout(?:[ \t]|$)/; export interface TimeoutWrapperMatch { readonly duration: string; diff --git a/packages/pi-permission-inner-cmd/test/authorizer.test.ts b/packages/pi-permission-inner-cmd/test/authorizer.test.ts index 212ac1b..492052a 100644 --- a/packages/pi-permission-inner-cmd/test/authorizer.test.ts +++ b/packages/pi-permission-inner-cmd/test/authorizer.test.ts @@ -79,6 +79,7 @@ function entriesRecovering(command: string, toolCallId = "call_1"): SessionEntry function bashDetails( toolCallId = "call_1", agentName: string | null = null, + command?: string, ): PromptPermissionDetails { return { requestId: "req-1", @@ -87,8 +88,8 @@ function bashDetails( message: "May I run bash?", toolCallId, toolName: "bash", - // details.command is intentionally the winning unit, not the full input. - command: "ignored-winning-unit", + // details.command is the winning unit the permission system isolated. + command, }; } @@ -117,6 +118,8 @@ function makeSessionProbe(args: { async function run(args: { recoveredCommand: string; + /** details.command — the ask-triggering unit; defaults to recoveredCommand. */ + unitCommand?: string; states?: Record; details?: Partial; getEntriesThrows?: boolean; @@ -141,8 +144,12 @@ async function run(args: { getSessionIdThrows: args.getSessionIdThrows, sessionId: args.sessionMismatch ? "session-changed" : ROOT_SESSION_ID, }); + const unitCommand = args.unitCommand ?? args.recoveredCommand; const verdict = await authorizeInnerCommand({ - details: { ...bashDetails(toolCallId), ...args.details } as PromptPermissionDetails, + details: { + ...bashDetails(toolCallId, null, unitCommand), + ...args.details, + } as PromptPermissionDetails, query, log, session, @@ -462,6 +469,66 @@ describe("authorizeInnerCommand — xargs wrapper", () => { }); }); +describe("authorizeInnerCommand — scaffolded commands", () => { + it("unwraps a timeout buried in a cd/echo/|/tail scaffold", async () => { + const full = + "cd /repo && echo go && timeout 240 pnpm install --frozen-lockfile 2>&1 | tail -30; echo EXIT"; + const deWrapped = + "cd /repo && echo go && pnpm install --frozen-lockfile 2>&1 | tail -30; echo EXIT"; + const { verdict, log, check } = await run({ + recoveredCommand: full, + unitCommand: "timeout 240 pnpm install --frozen-lockfile", + states: { [deWrapped]: "allow" }, + }); + expect(verdict.kind).toBe("allow"); + // re-evaluated the full de-wrapped compound, not just the unit's inner + expect(check).toEqual([ + { surface: "bash", value: deWrapped, agentName: undefined }, + ]); + expect(log).toEqual([ + { + level: "review", + event: "inner_cmd.allow", + details: { + command: full, + innerCommand: "pnpm install --frozen-lockfile", + }, + }, + ]); + }); + + it("defers when the de-wrapped compound is not fully allowing (sibling)", async () => { + const full = "cd /repo && timeout 30s pnpm test && git push origin"; + const deWrapped = "cd /repo && pnpm test && git push origin"; + const { verdict, check } = await run({ + recoveredCommand: full, + unitCommand: "timeout 30s pnpm test", + states: {}, + }); + expect(verdict.kind).toBe("defer"); + // the de-wrapped compound still contains the git push sibling + expect(check).toEqual([ + { surface: "bash", value: deWrapped, agentName: undefined }, + ]); + }); + + it("defers fail-closed when the unit is not a unique substring", async () => { + const { verdict, log } = await run({ + recoveredCommand: "timeout 30s pnpm test", + unitCommand: "timeout 30s pnpm test", + states: { "pnpm test": "allow" }, + }); + expect(verdict.kind).toBe("defer"); + expect(log).toEqual([ + { + level: "debug", + event: "inner_cmd.wrapper_not_located", + details: { command: "timeout 30s pnpm test" }, + }, + ]); + }); +}); + describe("authorizeInnerCommand — exceptions defer with a debug log", () => { it("defers when reading the session id throws (logs only safe data)", async () => { const { verdict, log } = await run({ diff --git a/packages/pi-permission-inner-cmd/test/recognizer.test.ts b/packages/pi-permission-inner-cmd/test/recognizer.test.ts index f943f44..eae42e1 100644 --- a/packages/pi-permission-inner-cmd/test/recognizer.test.ts +++ b/packages/pi-permission-inner-cmd/test/recognizer.test.ts @@ -6,7 +6,7 @@ import { } from "../src/recognizer"; describe("parseTimeoutWrapper", () => { - it("matches the strict simple-timeout form", () => { + it("matches the simple-timeout form", () => { expect(parseTimeoutWrapper("timeout 30s pnpm test")).toEqual({ duration: "30s", innerCommand: "pnpm test", @@ -25,6 +25,21 @@ describe("parseTimeoutWrapper", () => { }); }); + it("accepts GNU durations: bare integer (seconds) and decimals", () => { + expect(parseTimeoutWrapper("timeout 240 pnpm test")).toEqual({ + duration: "240", + innerCommand: "pnpm test", + }); + expect(parseTimeoutWrapper("timeout 1.5h deploy")).toEqual({ + duration: "1.5h", + innerCommand: "deploy", + }); + expect(parseTimeoutWrapper("timeout 2.5s build")).toEqual({ + duration: "2.5s", + innerCommand: "build", + }); + }); + it("preserves compound inner programs as the inner command", () => { expect(parseTimeoutWrapper("timeout 60s pnpm test && git push")).toEqual({ duration: "60s", @@ -47,8 +62,10 @@ describe("parseTimeoutWrapper", () => { }); }); - it("rejects leading-zero and multi-letter durations", () => { + it("rejects zero, leading-zero, ms, and multi-letter durations", () => { + expect(parseTimeoutWrapper("timeout 0 pnpm test")).toBeUndefined(); expect(parseTimeoutWrapper("timeout 0s pnpm test")).toBeUndefined(); + expect(parseTimeoutWrapper("timeout 0.5s pnpm test")).toBeUndefined(); expect(parseTimeoutWrapper("timeout 030s pnpm test")).toBeUndefined(); expect(parseTimeoutWrapper("timeout 30ms pnpm test")).toBeUndefined(); expect(parseTimeoutWrapper("timeout 30sec pnpm test")).toBeUndefined();