diff --git a/CONTEXT.md b/CONTEXT.md index 1340935..6b9f2da 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -1,53 +1,28 @@ -# Permission Authorization +# pi-extensions -This context describes how ambiguous coding-agent operations are reviewed before they may execute. +Personal `pi` coding-agent extensions. This context covers the permission +extensions that inspect and re-evaluate Bash commands before they are allowed. ## Language -**Deterministic Permission Policy**: -The rule-based authority that classifies an operation as allowed, denied, or requiring a decision. -_Avoid_: Static judge +### Wrappers -**Authorization Judge**: -An independent reviewer that proposes a verdict for an operation the Deterministic Permission Policy could not decide. -_Avoid_: Bash parser, safety classifier +**Wrapper**: +A Bash command of the form ` [modifier-args] `, where the +authorization question is "what does the inner command do?". Whether a wrapper +may be unwrapped depends on whether its modifier args are transparent. +_Avoid_: command type, prefix command -**Shadow Mode**: -An observation mode in which an Authorization Judge records a verdict without changing whether the operation executes. -_Avoid_: Dry run +**Transparent wrapper**: +A wrapper whose modifier args do not change which program the inner command +resolves to or its trust boundary (e.g. `timeout`). Stripping the modifiers and +re-evaluating the inner command is sound: the verdict applies to the same +program that actually runs. +_Avoid_: safe wrapper -**Enforce Mode**: -An authority mode in which an Authorization Judge's allow verdict may approve an operation. In the initial rollout, deny and defer still pass to the next authority. -_Avoid_: Production mode - -**Judge Participation**: -The presence of an Authorization Judge in the configured authorizer chain. Participation determines whether the Judge is consulted, independently of whether it is in Shadow Mode or Enforce Mode. -_Avoid_: Enabled, installed - -**Defer**: -A verdict stating that the available information or the judge itself is insufficient to decide, leaving the decision to the next authority. -_Avoid_: Deny, error - -**False Allow**: -A Shadow Mode outcome in which the Authorization Judge proposes approval and the human reviewer rejects the same permission request. -_Avoid_: False positive - -**Requesting Session**: -The session in which the operation requiring authorization originated. For a forwarded request, this is the child session. -_Avoid_: Current session - -**Serving Session**: -The authority-bearing session that resolves a forwarded request and runs its configured Authorization Judge before the terminal human authority. -_Avoid_: Parent context, current session - -**Conversation Owner**: -The session whose conversation entries are supplied to an Authorization Judge. It may differ from the Requesting Session for forwarded requests. -_Avoid_: Requester - -**Judgment Evidence**: -The minimal operation, execution, user-intent, and forwarded-provenance facts supplied to an Authorization Judge for one verdict. Adapter validation state and review-log metadata are not Judgment Evidence. -_Avoid_: Judge request, full request context - -**Execution Working Directory**: -The working directory of the Requesting Session in which an operation will execute and relative paths are resolved. For a forwarded request, it must not be replaced with the Serving Session's working directory. -_Avoid_: Current cwd, parent cwd +**Non-transparent wrapper**: +A wrapper whose modifier args change the inner command's resolution or effect +(e.g. `env`, whose `PATH=` or `-i` can make the inner name resolve to a +different binary). Stripping the modifiers and re-evaluating the inner command +is unsound: the verdict may apply to a different program than the one that runs. +_Avoid_: unsafe wrapper diff --git a/docs/adr/0002-env-defers-to-ai-judge.md b/docs/adr/0002-env-defers-to-ai-judge.md new file mode 100644 index 0000000..9a7c4a4 --- /dev/null +++ b/docs/adr/0002-env-defers-to-ai-judge.md @@ -0,0 +1,61 @@ +--- +status: accepted +--- + +# Defer `env` to the AI judge; inner-cmd never unwraps it + +`env` is the second command type considered for `pi-permission-inner-cmd`'s +deterministic unwrapping. We decided inner-cmd **always defers** `env` — it +never strips modifiers or re-evaluates an inner command — and lets the AI judge +authority handle `env` commands together with their full environment context. + +## Why + +`env` is a *non-transparent wrapper*: its modifier args (`NAME=VALUE`, `-i`, +`-u`, `-C`, `-S`) form an unbounded, implicit input channel that the +command-string policy cannot see. Stripping them and re-evaluating the inner +command is unsound, because the modifiers can change what actually runs without +changing the inner command string: + +- `PATH=/evil` — resolve the inner name to a different binary; +- `LD_PRELOAD=/x.so`, `LD_LIBRARY_PATH` — inject arbitrary native code into the + inner binary before its `main()`; +- `BASH_ENV`, `PYTHONPATH`, `NODE_OPTIONS`, `PERL5OPT`, … — source scripts or + inject code into the inner command's runtime; +- `-i`, `-u NAME`, `-C DIR` — alter the execution context. + +A "detect dangerous variables and defer" denylist is infeasible: the dangerous +set is open-ended (new runtimes keep introducing new `*_OPT` / `*_PATH` +injection variables), so any denylist is permanently incomplete, and one miss is +an RCE. Detecting "does *any* modifier exist" is trivial and complete, but the +only sound unwrap case is bare `env ` (zero modifiers), which is too rare in +practice to justify the shell parsing it would require (assignment/flag +detection, `-S`, `--`, `/usr/bin/env` basename, quoting). + +## Considered options + +- **Detect a `PATH=` override, otherwise unwrap** — rejected as false security: + `LD_PRELOAD` and the interpreter-injection variables defeat it without + touching `PATH`. +- **Unwrap only bare `env ` (zero modifiers)** — sound, but bare `env` is + rarely written and the boundary parsing adds real complexity for near-zero + coverage. Not worth it. +- **Always defer (chosen)** — zero parsing risk, zero soundness surface; + non-transparent commands get the semantic judgement they require from the AI + judge. + +## Consequences + +This establishes a clean division between the two permission authorities: + +- **inner-cmd (deterministic)** unwraps only *transparent* wrappers (`timeout`) + — sound, fast, narrow. Every uncertain or non-transparent command defers + fail-closed. +- **The AI judge authority** receives deferred commands (including `env`) and + reasons about the full command *with* its environment, where non-transparency + can be understood semantically rather than stripped. + +This is why the AI judge consumes the full recovered command from the shared +recovery module (ADR 0001) — it needs the complete input, modifiers included, to +judge non-transparent wrappers. See `CONTEXT.md` for the transparent / +non-transparent wrapper distinction. diff --git a/packages/pi-permission-inner-cmd/src/authorizer.ts b/packages/pi-permission-inner-cmd/src/authorizer.ts index ff470a2..9ec8215 100644 --- a/packages/pi-permission-inner-cmd/src/authorizer.ts +++ b/packages/pi-permission-inner-cmd/src/authorizer.ts @@ -5,14 +5,11 @@ import type { PermissionQuery, PromptPermissionDetails, } from "@gotgenes/pi-permission-system"; -import { classifyWrapper, isRecognizedWrapper } from "./recognizer"; import { NATIVE_BASH_TOOL_NAME, recoverNativeBashCommand, } from "@sikongjueluo/pi-permission-shared"; - -/** Bash permission surface queried when re-evaluating the inner command. */ -const BASH_SURFACE = "bash"; +import { handlers } from "./handlers"; /** Convert a thrown value into a short, log-safe string. */ function toErrorString(error: unknown): string { @@ -49,24 +46,22 @@ export interface InnerCommandAuthorizerDeps { } /** - * V0.1 inner-command Authorizer decision (ADR 0001). + * Inner-command Authorizer decision (ADR 0001). * - * Recovers the complete native Bash command for `details.toolCallId` from the - * captured session, unwraps one strict `timeout` level, and re-evaluates the - * inner command through the deterministic permission policy. Every uncertain - * path — forwarded requests, a session-identity mismatch, non-Bash tools, - * missing/duplicate/malformed session evidence, unsupported or nested wrapper - * syntax, parse failures, and exceptions — defers to the next authority. + * Revalidates root ownership, recovers the complete native Bash command for + * `details.toolCallId` from the captured session, then hands it to the first + * registered handler that claims it. Each handler owns its own recognition and + * verdict logic: the timeout handler unwraps one level and re-evaluates the + * inner command; the env handler defers as non-transparent. * - * Logging contract: - * - `review` only for a recognized wrapper whose inner command resolves to a - * decisive `allow`/`deny`. - * - `debug` for a recognized inner `ask`, unsupported timeout syntax, nested - * wrappers, a session-identity mismatch, and exceptions. - * - ordinary non-timeout commands defer silently. - * - recognized logs carry both `command` and `innerCommand`; an exception after - * recognition retains both alongside `error`, while an earlier exception logs - * only the safe data available at that point. + * Every uncertain path — forwarded requests, a session-identity mismatch, + * non-Bash tools, missing session evidence, an unrecognized command, or any + * exception — defers to the next authority (fail-closed). + * + * Logging: handlers emit their own review/debug events for decisive and + * notable-defer outcomes; silent deferrals log nothing. Exceptions are logged + * by this engine as `inner_cmd.exception`, retaining the recovered command and + * any partial evidence the active handler recorded before throwing. */ export async function authorizeInnerCommand( deps: InnerCommandAuthorizerDeps, @@ -75,18 +70,17 @@ export async function authorizeInnerCommand( // Track recovered evidence so an exception after recognition can retain it. let command: string | undefined; - let innerCommand: string | undefined; + let evidence: Record = {}; try { - // Forwarded subagent asks are out of scope for v0.1: the captured - // session is the serving root's conversation, not the requester's. + // Forwarded subagent asks are out of scope: the captured session is the + // serving root's conversation, not the requester's. if (details.forwarding) { return { kind: "defer" }; } // Revalidate root ownership: the live session must still be the one we - // registered for. A mismatch (or a session id that cannot be read) - // means the captured conversation can no longer be attributed safely. + // registered for. const currentSessionId = session.getSessionId(); if (currentSessionId !== expectedSessionId) { log.debug("inner_cmd.session_mismatch", { @@ -113,59 +107,15 @@ export async function authorizeInnerCommand( return { kind: "defer" }; } - const classification = classifyWrapper(command); - - switch (classification.kind) { - case "nonTimeout": - // An ordinary Bash command this authorizer does not unwrap. - return { kind: "defer" }; - - case "unsupportedTimeout": - log.debug("inner_cmd.unsupported_timeout_syntax", { command }); - return { kind: "defer" }; - - case "recognized": { - innerCommand = classification.match.innerCommand; - - // Never unwrap more than one level in v0.1. - 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: - // Treat any unexpected state as a safe defer. - log.debug("inner_cmd.inner_ask", { - command, - innerCommand, - }); - return { kind: "defer" }; - } + // Dispatch to the first registered handler that claims the command. + for (const handler of handlers) { + evidence = {}; + const verdict = handler.decide({ command, details, query, log, evidence }); + if (verdict !== undefined) { + return verdict; } } + return { kind: "defer" }; } catch (error) { const exceptionDetails: Record = { error: toErrorString(error), @@ -173,9 +123,7 @@ export async function authorizeInnerCommand( if (command !== undefined) { exceptionDetails.command = command; } - if (innerCommand !== undefined) { - exceptionDetails.innerCommand = innerCommand; - } + Object.assign(exceptionDetails, evidence); log.debug("inner_cmd.exception", exceptionDetails); return { kind: "defer" }; } diff --git a/packages/pi-permission-inner-cmd/src/handlers/env.ts b/packages/pi-permission-inner-cmd/src/handlers/env.ts new file mode 100644 index 0000000..647947e --- /dev/null +++ b/packages/pi-permission-inner-cmd/src/handlers/env.ts @@ -0,0 +1,25 @@ +import type { CommandHandler } from "./types"; + +/** Matches a command whose leading program is `env`. */ +const ENV_PREFIX = /^env(?:[ \t]|$)/; + +/** + * The `env` wrapper handler. + * + * `env` is non-transparent: its modifier args (`NAME=VALUE`, `-i`, `-u`) can + * change which binary the inner command resolves to (e.g. a `PATH=` override), + * so stripping them and re-evaluating the inner command is unsound. `env` is + * therefore claimed but always deferred — it never unwraps. Commands not + * starting with `env` return `undefined` so the engine can try the next + * handler. + */ +export const envHandler: CommandHandler = { + id: "env", + decide({ command, log }) { + if (!ENV_PREFIX.test(command)) { + return undefined; + } + log.debug("inner_cmd.env_non_transparent", { command }); + return { kind: "defer" }; + }, +}; diff --git a/packages/pi-permission-inner-cmd/src/handlers/index.ts b/packages/pi-permission-inner-cmd/src/handlers/index.ts new file mode 100644 index 0000000..b2be538 --- /dev/null +++ b/packages/pi-permission-inner-cmd/src/handlers/index.ts @@ -0,0 +1,10 @@ +import { envHandler } from "./env"; +import { timeoutHandler } from "./timeout"; +import type { CommandHandler } from "./types"; + +/** + * Registered command handlers, tried in order. The first to return a verdict + * claims the command; the rest are not consulted. Add a handler here (and a + * new file under `handlers/`) to support a new command type. + */ +export const handlers: readonly CommandHandler[] = [timeoutHandler, envHandler]; diff --git a/packages/pi-permission-inner-cmd/src/handlers/timeout.ts b/packages/pi-permission-inner-cmd/src/handlers/timeout.ts new file mode 100644 index 0000000..7b970a8 --- /dev/null +++ b/packages/pi-permission-inner-cmd/src/handlers/timeout.ts @@ -0,0 +1,56 @@ +import { classifyWrapper, isRecognizedWrapper } 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). + * + * 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. + */ +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 }); + 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" }; + } + } + } + }, +}; diff --git a/packages/pi-permission-inner-cmd/src/handlers/types.ts b/packages/pi-permission-inner-cmd/src/handlers/types.ts new file mode 100644 index 0000000..acd5a47 --- /dev/null +++ b/packages/pi-permission-inner-cmd/src/handlers/types.ts @@ -0,0 +1,39 @@ +import type { + AuthorizerLog, + AuthorizerVerdict, + PermissionQuery, + PromptPermissionDetails, +} from "@gotgenes/pi-permission-system"; + +/** Context handed to a handler for one recovered command. */ +export interface HandlerContext { + /** The full recovered Bash command. */ + readonly command: string; + readonly details: PromptPermissionDetails; + readonly query: PermissionQuery; + readonly log: AuthorizerLog; + /** + * Partial-result bag. A handler that recognizes the command records derived + * values here (e.g. `ctx.evidence.innerCommand = innerCommand`) so the + * engine's exception log retains them if `decide` later throws. The engine + * resets this bag per handler. + */ + readonly evidence: Record; +} + +/** + * A self-contained verdict strategy for one kind of Bash command. + * + * The engine walks registered handlers in order; the first that returns a + * verdict claims the command. Returning `undefined` means "not mine" and lets + * the engine try the next handler. + */ +export interface CommandHandler { + /** Stable id for logs, e.g. "timeout". */ + readonly id: string; + /** + * Inspect the command: return a verdict to claim it (the engine stops), or + * `undefined` to pass to the next handler. + */ + decide(ctx: HandlerContext): AuthorizerVerdict | undefined; +} diff --git a/packages/pi-permission-inner-cmd/test/authorizer.test.ts b/packages/pi-permission-inner-cmd/test/authorizer.test.ts index acc009e..44cdf9f 100644 --- a/packages/pi-permission-inner-cmd/test/authorizer.test.ts +++ b/packages/pi-permission-inner-cmd/test/authorizer.test.ts @@ -402,6 +402,36 @@ describe("authorizeInnerCommand — fail-closed deferrals", () => { }); }); +describe("authorizeInnerCommand — env wrapper", () => { + it("defers on an env wrapper with a debug log (non-transparent)", async () => { + const { verdict, log, check } = await run({ + recoveredCommand: "env FOO=bar pnpm test", + states: { "pnpm test": "allow" }, + }); + expect(verdict.kind).toBe("defer"); + // env is non-transparent: never unwrapped, so the inner command is not + // re-evaluated through the deterministic policy. + expect(check).toEqual([]); + expect(log).toEqual([ + { + level: "debug", + event: "inner_cmd.env_non_transparent", + details: { command: "env FOO=bar pnpm test" }, + }, + ]); + }); + + it("does not claim a command that only contains env later", async () => { + // Starts with printf, not env -> no handler claims it -> silent defer. + const { verdict, log } = await run({ + recoveredCommand: "printf hi; env | sort", + states: {}, + }); + expect(verdict.kind).toBe("defer"); + expect(log).toEqual([]); + }); +}); + 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({