docs(security): ADR-004 PostToolUse hook ordering + invariant test #4

Merged

vikingowl merged 1 commits from docs/adr-posttooluse-hook-ordering into main

2026-05-19 23:29:31 +02:00

vikingowl commented

2026-05-19 23:28:57 +02:00

Owner

Summary

Closes the last remaining 2026-05-19 audit finding. The audit raised PostToolUse hook ordering as High: hooks see raw tool output before the firewall scan runs, so a `type = "prompt"` hook could pipe secrets to a remote LLM. The audit proposed reordering or splitting the event into raw-local vs. redacted-for-LLM variants.

After investigating, the audit's threat model is already closed transitively by Wave 1. Wave 1's SafeProvider boundary scans outgoing messages at every router arm and at every non-engine provider consumer. For each hook command type:

Type	Path	Outbound scanning
`command` (shell)	Local subprocess	N/A — never reaches an LLM
`prompt`	`PromptExecutor.streamer.Stream` → `routerStreamer` → `router.Stream` → `arm.Provider.Stream`	`arm.Provider` is `*SafeProvider` after Wave 1
`agent`	Spawn elf → elf engine.Config has `Firewall` set	`buildRequest` scans inline

So the audit's literal observation about source ordering remains true; the practical leak it implied does not.

Decision (ADR-004)

Accept the current ordering. Document the transitive guarantee. Lock it in with a regression test.

Alternatives considered:

Reorder (scan before hook). Breaks legitimate shell-hook use cases (audit log, forensic hash, local alert) that need raw access.
Split contract (`PostToolUseRaw` + `PostToolUseRedacted`). Migration cost for every existing config; wrong-variant footgun.
Dispatcher routes by command type. Dispatcher would need firewall access (new dep) plus per-handler payload variants.

Position D (this PR) wins because: no churn, shell-hook use cases preserved, transitive guarantee made explicit and tested.

What changes

`docs/essentials/decisions/004-posttooluse-hook-ordering.md` — the decision record. Lists conditions for re-opening: any new hook type with non-router LLM transport, SafeProvider removed/relaxed, telemetry showing prompt-PostToolUse adoption, or a real-world incident.
`internal/hook/event.go` — doc comment on `PostToolUse` constant making the transitive guarantee explicit.
`internal/engine/loop.go` — inline comment at the hook fire site pointing at the ADR.
`internal/hook/posttooluse_redaction_test.go` — regression test. Builds a recording fake provider behind `SafeProvider`, registers it as a router arm, configures a `type = "prompt"` PostToolUse hook whose template embeds `{{.Result}}`, fires the hook on a payload containing `sk-ant-api03-...`. Asserts the inner provider receives a redacted prompt. If this test fails after a refactor, ADR-004 is no longer correct and the audit finding re-opens.

Test plan

`go test ./...` green
`go test -race ./...` green
`golangci-lint run ./...` — 0 issues
New invariant test passes: prompt PostToolUse hook on a secret-bearing tool result produces a redacted prompt at the inner provider

## Summary Closes the last remaining 2026-05-19 audit finding. The audit raised PostToolUse hook ordering as High: hooks see raw tool output before the firewall scan runs, so a \`type = \"prompt\"\` hook could pipe secrets to a remote LLM. The audit proposed reordering or splitting the event into raw-local vs. redacted-for-LLM variants. After investigating, **the audit's threat model is already closed transitively by Wave 1**. Wave 1's SafeProvider boundary scans outgoing messages at every router arm and at every non-engine provider consumer. For each hook command type: | Type | Path | Outbound scanning | |---|---|---| | \`command\` (shell) | Local subprocess | N/A — never reaches an LLM | | \`prompt\` | \`PromptExecutor.streamer.Stream\` → \`routerStreamer\` → \`router.Stream\` → \`arm.Provider.Stream\` | \`arm.Provider\` is \`*SafeProvider\` after Wave 1 | | \`agent\` | Spawn elf → elf engine.Config has \`Firewall\` set | \`buildRequest\` scans inline | So the audit's literal observation about source ordering remains true; the practical leak it implied does not. ## Decision (ADR-004) **Accept the current ordering. Document the transitive guarantee. Lock it in with a regression test.** Alternatives considered: - **Reorder (scan before hook).** Breaks legitimate shell-hook use cases (audit log, forensic hash, local alert) that need raw access. - **Split contract (\`PostToolUseRaw\` + \`PostToolUseRedacted\`).** Migration cost for every existing config; wrong-variant footgun. - **Dispatcher routes by command type.** Dispatcher would need firewall access (new dep) plus per-handler payload variants. Position D (this PR) wins because: no churn, shell-hook use cases preserved, transitive guarantee made explicit and tested. ## What changes 1. \`docs/essentials/decisions/004-posttooluse-hook-ordering.md\` — the decision record. Lists conditions for re-opening: any new hook type with non-router LLM transport, SafeProvider removed/relaxed, telemetry showing prompt-PostToolUse adoption, or a real-world incident. 2. \`internal/hook/event.go\` — doc comment on \`PostToolUse\` constant making the transitive guarantee explicit. 3. \`internal/engine/loop.go\` — inline comment at the hook fire site pointing at the ADR. 4. \`internal/hook/posttooluse_redaction_test.go\` — regression test. Builds a recording fake provider behind \`SafeProvider\`, registers it as a router arm, configures a \`type = \"prompt\"\` PostToolUse hook whose template embeds \`{{.Result}}\`, fires the hook on a payload containing \`sk-ant-api03-...\`. Asserts the inner provider receives a redacted prompt. If this test fails after a refactor, ADR-004 is no longer correct and the audit finding re-opens. ## Test plan - [x] \`go test ./...\` green - [x] \`go test -race ./...\` green - [x] \`golangci-lint run ./...\` — 0 issues - [x] New invariant test passes: prompt PostToolUse hook on a secret-bearing tool result produces a redacted prompt at the inner provider

vikingowl added 1 commit 2026-05-19 23:28:57 +02:00

docs(security): ADR-004 PostToolUse hook ordering + invariant test 20327d354d

Closes the last remaining 2026-05-19 audit finding by documenting the
existing transitive guarantee rather than restructuring the hook
contract.

The audit observed that PostToolUse hooks receive raw tool output
before the firewall scan runs, and proposed reordering or splitting
the event into raw-local-only and redacted-for-LLM variants. After
Wave 1 (SafeProvider boundary at every router arm + non-engine
provider consumer), the audit's threat model is closed transitively:

- Shell hooks see raw output but never reach an LLM.
- Prompt hooks route Stream calls through routerStreamer → router →
  arm.Provider, every arm.Provider is now *SafeProvider, outgoing
  messages are scanned at the boundary.
- Agent hooks spawn an elf whose engine has Firewall set;
  buildRequest scans inline.

Reordering would regress legitimate shell-hook use cases (audit,
forensic, local alert) that need raw access. Splitting the contract
forces every existing hook config to migrate and introduces a
wrong-variant footgun. Neither is justified by the residual risk.

Three changes ship with the ADR:
- ADR-004 records the decision and the conditions for re-opening it.
- Doc comments on hook.PostToolUse and the dispatcher call site in
  the engine point at the ADR.
- internal/hook/posttooluse_redaction_test.go locks in the invariant:
  a prompt PostToolUse hook firing on a secret-bearing tool result
  produces a redacted prompt at the inner provider. If this test
  fails, ADR-004's Position A is no longer correct and the audit
  finding re-opens.

vikingowl merged commit 4227769ff4 into main

2026-05-19 23:29:31 +02:00

vikingowl referenced this issue from a commit

2026-05-20 03:27:47 +02:00

Merge pull request 'docs(security): ADR-004 PostToolUse hook ordering + invariant test' (#4) from docs/adr-posttooluse-hook-ordering into main

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Owlibou/gnoma#4