Skip to content

fix(governance): agent argument named "tool" bypasses the sensitive-tool approval gate#28

Closed
sxyrxyy wants to merge 1 commit into
Cogensec:mainfrom
sxyrxyy:fix/governance-tool-name-gate
Closed

fix(governance): agent argument named "tool" bypasses the sensitive-tool approval gate#28
sxyrxyy wants to merge 1 commit into
Cogensec:mainfrom
sxyrxyy:fix/governance-tool-name-gate

Conversation

@sxyrxyy

@sxyrxyy sxyrxyy commented Jul 23, 2026

Copy link
Copy Markdown

The problem

Some tools are on a watchlist (payment_execute, admin_api, file_delete, database_write). When an agent uses one, governance (GOV-001) is meant to stop and require human approval.

The action dict it checks was built as {"tool": tool_name, "type": "tool_call", **tool_input}. The agent's own arguments get spread in last, so an argument named tool overwrites the real tool name. GOV-001 then checks the fake name, sees something harmless, and skips approval.

Impact: a prompt-injected agent calls payment_execute with an argument {"tool": "noop"} and the approval gate never fires. With enforce=True the dangerous tool just runs. Even in observe-only mode, the signed attestation records it as governance: pass, so the audit trail certifies a check that never happened. The same trick overrides type, which the type-keyed rules rely on.

The fix

Spread the agent's arguments first so the trusted tool/type fields always win:

{**tool_input, "tool": tool_name, "type": "tool_call"}

Keeps the flat action shape, so GOV-003 still reads action["amount"]. The sibling decision-record paths already nest arguments safely and are unchanged.

Verified

Through the real adapter path with enforce=True: the poisoned call now resolves to payment_execute and is denied (escalate, fail-closed with no approval handler). The honest call is unchanged. Existing test suite unaffected (the async adapter tests fail identically on main without this change; they need pytest-asyncio, unrelated to this fix).

… policy gate

The action dict inspected by GovernanceLayer was built as
`{"tool": tool_name, "type": "tool_call", **tool_input}`. Spreading the
agent-supplied arguments last let an argument named `tool` overwrite the
real tool name. GOV-001 checks `action["tool"]` against the sensitive-tool
set, so a HIGH/CRITICAL agent calling e.g. `payment_execute` with an
argument `{"tool": "noop"}` read as a non-sensitive tool and skipped the
REQUIRE_APPROVAL escalation.

Spread the arguments first so the trusted `tool`/`type` fields always win.
Keeps the flat action shape (GOV-003 still reads `action["amount"]`).
@requie
requie self-requested a review July 23, 2026 04:05

@requie requie left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Confirmed the vulnerability and the fix. tool_input spread last into the buffer entry lets an argument named tool or type overwrite the trusted fields, that entry becomes context["action"] via to_evaluation_context(), and GOV-001 matches on action["tool"]. So payment_execute(tool="noop") skips the gate and the attestation records a pass for a check that evaluated a forged name. Spread-first is the right minimal fix, and I verified it’s the only spread-style construction in the codebase (the decision-record paths already nest under arguments).

One change requested before merge: commit the regression test you describe in the PR body. This is a silent-bypass class of bug, exactly the kind that regresses without a test pinning it. Something like: high-risk profile, payment_execute with tool_input={"tool": "noop"}, assert GOV-001 fires and enforcement denies with no approval handler. The honest-call case asserting no behavior change would be a nice second assertion.

Two notes, not blockers:
• After this fix a legitimate tool argument literally named tool or type is shadowed in the flattened action. Acceptable trade, trusted-field integrity wins.
• The flat action shape still lets untrusted args drive rules (GOV-003 reads amount straight from agent-supplied args). The long-term fix is nesting args under arguments like the decision-record path. That’s a breaking change and out of scope here; tracking it separately.

Good find, and thanks for the accurate writeup. Everything in the PR body checked out against the source.

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