Skip to content

fix(mcp): a denied tool call is audited as a refusal, not a success (#2807) - #2855

Merged
vybe merged 4 commits into
devfrom
feature/2807-audit-denied-calls
Sep 17, 2026
Merged

vybe merged 4 commits into
devfrom
feature/2807-audit-denied-calls

Conversation

@webmixgamer

Copy link
Copy Markdown
Contributor

Summary

  • A denial any MCP gate RETURNS was audited as success: true, because withAudit labels a call by throw/no-throw and every gate on the tool surface returns its denial as JSON (the envelope agents parse). Reproduced live: the refused chat_with_agent row read {"tool":"chat_with_agent","duration_ms":32,"success":true}, so an operator could not tell a permitted call from a refused one.
  • Fix: the deny sites stamp the per-call tool context through one helper (access.ts::accessDenied → context.outcome = {kind: "denied", reason}, the seam feat(mcp): expose git status/sync/log/pull as direct MCP tools (bypass LLM) #905 already uses for requestId) and withAudit reads it after execute: success: false, denied: true, error: <reason>. A thrown backend 403 is marked denied too. The caller's JSON is byte-identical.
  • Mechanism, not discipline: all 29 deny branches in 10 tool modules use the helper, and audit-denial.test.ts fails a !allowed branch or an error: "Access denied" envelope that bypasses it. createServer injects the audit URL + secret (configureAudit) so the row is observable in-process and over the real transport.

Changes

  • src/mcp-server/src/access.ts — accessDenied(context, envelope, auditReason?) + DenyCallContext; the ent#628 withAgentAccess denial uses it.
  • src/mcp-server/src/audit.ts — ToolCallContext.outcome; configureAudit; withAudit hands execute a defined context and reads the stamp on both exits; details.denied.
  • src/mcp-server/src/types.ts — ToolOutcome. server.ts — internalApiSecret option → configureAudit.
  • src/mcp-server/src/tools/{chat,agents,a2a,a2a_call,executions,git,loops,operator_queue,reports,schedules}.ts — every deny site through the helper (envelopes unchanged, key order preserved); loop-id and get_report refusals pass the internal reason for the admin-only row.
  • Tests: src/mcp-server/src/audit-denial.test.ts (new: captured audit POST, byte-identical envelopes, root-context-only stamp, stamp-then-throw, thrown 403, the guard), access-wiring.test.ts (+2 real-transport cases: the refused run_agent_loop row, deny→allow on one session), tests/journeys/test_j10_agent_calls_agent_journey.py (the strict=True xfail removed).
  • Docs: audit-trail.md, agent-to-agent-collaboration.md, run-agent-loop.md, mcp-git-tools.md, architecture/mcp-server.md, requirements/security.md, learnings.md (+1 entry), tests/registry.json, docs/security-reports/cso-diff-2026-09-16-2807-denied-call-audit.*.

Test Plan

  • cd src/mcp-server && npm run build (typecheck) + npm test: 423 tests, 0 failures locally (node --import tsx --test), re-run green after merging dev.
  • Live on a local stack with the MCP server rebuilt from this branch: the same refused call now leaves {"tool": "chat_with_agent", "duration_ms": 33, "success": false, "error": "Permission denied: …", "denied": true}; pytest tests/journeys/test_j10_agent_calls_agent_journey.py -k "refused or without_permission" → 5 passed.
  • CI: mcp-server-test (build + boot smoke + unit) and journey-smoke (J10 runs credential-free on every PR; the flipped test_the_operator_can_see_that_a_call_was_refused is the operator-side proof).

Journey Impact: extends: J10

Mutation: reverting the wrapper's read of the stamp (const outcome = ctx.outcome → undefined, plus the catch-path denied) turns 10 of 17 tests red across audit-denial.test.ts + access-wiring.test.ts (every labelled-row case, both transports); un-stamping one deny site (chat.ts runAgentChat back to a bare JSON.stringify) turns 5 red including both guard rules; files restored byte-identical from scratch copies, control 17/17.

Out of scope, registered in the trinity-dev debt inbox (2026-09-16): a tool that catches a backend error and RETURNS {error} still audits success: true (the outcome shape adds a kind for it, not a second field); id-addressed tools (loops, get_report, operator-queue item/respond) leave target_id empty; refusals are visible per row but /api/audit-log cannot filter them.

/review: 0 critical. /cso --diff: 0 findings introduced (tenth consecutive clean diff audit; report in docs/security-reports/).

Fixes #2807

🤖 Generated with Claude Code

webmixgamer and others added 2 commits September 16, 2026 17:50
…2807)

Every access gate on the MCP tool surface RETURNS its denial as JSON (the
envelope agents parse), and the audit wrapper labelled a call by
throw/no-throw. So a refused chat_with_agent / chat_with_<slug> / fan_out /
run_agent_loop call left an mcp_operation row reading `success: true`, and an
operator reading the audit log could not tell a permitted call from a refused
one. Reproduced live before the fix: the refused call's row was
{"tool": "chat_with_agent", "duration_ms": 32, "success": true}.

The deny sites now serialise through one helper, access.ts::accessDenied,
which stamps `context.outcome = {kind: "denied", reason}` on the per-call tool
context (the seam #905 already uses for requestId); withAudit reads it after
execute and writes `success: false`, `denied: true`, `error: <reason>`. A
thrown backend 403 is marked denied too. The caller's JSON is byte-identical.
All 29 deny branches across 10 tool modules go through the helper, and a
decision-based guard fails `npm test` for a `!allowed` branch or an
`error: "Access denied"` envelope that bypasses it. createServer injects the
audit URL and secret (configureAudit), so a test can observe the row.

Tests: audit-denial.test.ts (captured audit POST, byte-identical envelopes,
root-context-only stamp, stamp-then-throw, thrown 403, the guard);
access-wiring.test.ts records /api/internal/audit over the real transport and
runs deny-then-allow on one session; the J10 strict xfail
test_the_operator_can_see_that_a_call_was_refused is flipped.

Fixes #2807

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
# Conflicts:
#	docs/memory/learnings.md
@webmixgamer

Copy link
Copy Markdown
Contributor Author

First run: every check green except regression diff, which flagged one test in one seed — test_2638_subscription_switch_on_turn::TestApiKeyFallback::test_it_never_raises (head, seed 12345). Not introduced here: this PR touches no backend code, and the collection order under that seed is byte-identical to dev; the test is worker-history dependent (its is None rides on the no-API-key guard because the setting read fails open). Filed as #2856 with the analysis. The run was re-triggered in full so the head leg re-executes.

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge-train: batch validated on train/20260917-1237 (#2868)

webmixgamer and others added 2 commits September 17, 2026 14:10
…ied-calls

# Conflicts:
#	docs/memory/learnings.md
#	src/mcp-server/src/server.ts
#	tests/registry.json
@vybe
vybe merged commit 6c7e3d2 into dev Sep 17, 2026
28 checks passed
@webmixgamer
webmixgamer deleted the feature/2807-audit-denied-calls branch September 17, 2026 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants