fix(mcp): enforce audit fail-closed before running a tool#174
Merged
Conversation
The MCP dispatch path gated tools on consentDecision (the env-driven PROMPTZERO_MCP_ALLOW_HIGH / ALLOW_CRITICAL opt-in) but never called audit.RequireOpen, the rail the REPL and --web dispatch both apply. Audit logging is wired best-effort in wireMCPSidecars: if audit.Open fails (a locked SQLite WAL from another promptzero process, an unwritable ~/.promptzero, an unset $HOME), the server continues with s.audit == nil and the RecordCtx calls silently no-op (they're nil-guarded). Result: with the operator opted in via env AND the audit log failed to open, a High/Critical MCP tool (an NFC / sub-GHz transmit, a run_payload deploy) ran with NO audit record — a fail-open relative to the documented invariant that every destructive action is audited, and a silent divergence from the REPL/web surfaces where the identical scenario is refused. Adds audit.RequireOpen(s.audit, effectiveLevel) before consentDecision, so MCP fails closed identically to the agent path: risk≥High is refused when no audit log is wired; Low/Medium tools are unaffected. The run_payload Critical escalation is already applied to effectiveLevel before this gate, so a path-resolved Critical can't slip through as High. Found via the internal audit sweep (bootstrap + web-route surfaces, otherwise clean). Tests: newTestHarness now wires an audit log by default (production does, and the new rail requires it for High/Critical) — tests exercising the nil-audit refusal call s.SetAuditLog(nil). New TestServer_CallTool_AuditFailClosedWhenNoAuditLog asserts a High tool with ALLOW_HIGH=1 but no audit log is refused with the "audit log not initialized" message and the handler never runs; -race clean, proven to fail pre-fix. The five consent matrix tests still pass. Verified: task ci green (lint 0 / vet / build / test:full -race 0 fail / govulncheck clean); task eval 17/17.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The MCP dispatch path gated on consentDecision (env opt-in) but never called audit.RequireOpen — the rail the REPL and --web both apply. Audit is wired best-effort; if audit.Open fails (locked WAL, unwritable ~/.promptzero, unset $HOME) the server runs with s.audit==nil and RecordCtx silently no-ops. So with ALLOW_HIGH/ALLOW_CRITICAL set AND audit failed to open, a High/Critical MCP tool ran UNAUDITED — a fail-open vs the documented 'every destructive action is audited' invariant, diverging from REPL/web. Adds audit.RequireOpen(s.audit, effectiveLevel) before consentDecision; risk≥High refused when no audit log, Low/Medium unaffected. Harness now wires audit by default; new fail-closed test proven to fail pre-fix, -race clean. task ci green; eval 17/17. Found via the internal audit sweep.