fix(security): stop tool arguments shadowing governance fields; close LLM classifier channel gap - #36
Merged
Merged
Conversation
… LLM channel gap
Three related fixes to the runtime integrity path.
1. Untrusted tool arguments could shadow the fields governance matches
on. The pre_tool_use buffer entry spread the agent's own arguments
over the trusted fields, and that entry becomes context["action"], so
an argument named "tool" overwrote the real tool name: a
prompt-injected agent calling payment_execute with {"tool": "noop"}
skipped GOV-001's sensitive-tool gate entirely. Under enforce=True
the dangerous tool ran; in observe-only mode the signed attestation
recorded governance:pass for a check that never happened. The same
trick overrode "type". Arguments now nest under action["arguments"],
the shape the decision-record path already used, so no
agent-controlled key can collide with a trusted field. GOV-003 reads
the nested amount.
2. The LLM classifier skipped the two multi-agent channels. The regex
scanner covers seven channels; aevaluate fed only five to the
classifier, so shared_memory and broadcast_messages reached neither
strong detector (the regex taxonomy scores ~0 on the action-oriented
injections that travel through them). That left the cross-agent
cascade path covered only by the detector that cannot see it. Both
channels are now classified, using the regex scanner's content keys
and channel labels.
3. Classification cost is bounded. Calls ran sequentially with no
ceiling while the multi-agent buffers cap at 1000 entries per
channel. Calls now run with bounded concurrency and a per-evaluation
target ceiling; truncation is logged and reported so a capped scan
never reads as full coverage. Verdicts are cached per (channel,
content hash) with oldest-first eviction, and fail-open verdicts are
never cached since they record an outage rather than a judgment.
Adds regression tests for the bypass, both channel gaps, and every cost
bound. Full suite green (640 passed), ruff and mypy --strict clean.
…w-arbrx0 # Conflicts: # CHANGELOG.md
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
… example
The custom-rule example read a call argument off the top level of the
action dict (action.get("data_classification")). Now that untrusted
arguments nest under action["arguments"], that pattern silently stops
matching for adapter-captured tool calls: the condition does not raise,
it just never fires again. Anyone following the spec would write a
policy rule that looks correct and enforces nothing.
Adds an "Action Dict" section stating the shape, which fields are
trusted, and why the nesting is a security boundary rather than a style
choice. Updates the example to read from arguments, and calls out the
silent-failure mode explicitly so rules written against the old shape
get re-checked during migration.
…t once per batch Closes the remaining half of the issue-31 requirement that the layer log when it truncates OR skips. Truncation was already logged and surfaced; verdict reuse (the skip path) was invisible. Every reuse is now counted in details["llm_classifier"] as reused_verdicts alongside llm_calls, with a debug log per evaluation. Also deduplicates identical (channel, text) targets within a single evaluation: the same text mirrored into a channel N times was N concurrent LLM calls, since all N missed the cache before any verdict landed. Unique keys are classified once and fanned back out to every target, so intra-batch repeats no longer re-bill.
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
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.
Reopens #35 against the renamed branch (
claude/pr28-p29-review-arbrx0→bug/pr28-p29-review). Closes #30 and #31.What this fixes
[Critical] Tool arguments could shadow the fields governance matches on.
The
pre_tool_usebuffer entry spread the agent's own arguments over the trusted fields ({"tool": tool_name, "type": "tool_call", **tool_input}). That entry becomescontext["action"], so an argument namedtooloverwrote the real tool name and GOV-001's sensitive-tool gate evaluated the forgery. A prompt-injected agent callingpayment_executewith{"tool": "noop"}skipped approval entirely: underenforce=Truethe dangerous tool ran, and in observe-only mode the signed attestation recordedgovernance: passfor a check that never happened. The same trick overrodetype.Verified reproducible before fixing — GOV-001 returns
passon the old shape andescalateon the new one for the same poisoned call.[High] The LLM classifier skipped the two multi-agent channels.
The regex scanner covers seven channels;
AdversarialLLMLayer.aevaluatefed only five to the classifier, soshared_memoryandbroadcast_messagesreached neither strong detector — the regex taxonomy scores ~0 on the action-oriented injections that travel through those channels. A compromised peer's injection into shared memory passed clean, which is the cross-agent cascade (T-CASCADE) the v0.8 channels exist to defend.Unbounded LLM classification cost (#31).
Calls ran sequentially with no ceiling while the multi-agent buffers cap at 1000 entries per channel — a latency tail and a cost-amplification lever on attacker-writable content.
Approach
Arguments now nest under
action["arguments"], the shape the decision-record and enforcement paths already used, rather than the narrower spread-order fix. Spread-first would only guaranteetoolandtypewin; untrusted keys would still sit at the top level where any future rule reading a flat key inherits the same bug. Nesting makes the collision structurally impossible. GOV-003 readsaction["arguments"]["amount"]accordingly (#30).Cost is bounded on two axes —
max_concurrency(default 8) andmax_targets_per_evaluation(default 200) — with truncation logged and surfaced asdetails["llm_classifier"]["targets_dropped"]so a capped scan never reads as full coverage. Verdicts are cached per (channel, content-hash) with oldest-first eviction so growing buffers don't re-bill classified text. Fail-open verdicts are never cached: they record an outage, not a judgment, and pinning one would blind every later pass over the same text.Breaking change
context["action"]for a tool call is now{"tool": …, "type": "tool_call", "arguments": {…}}. CustomPolicyRuleconditions reading call-derived keys off the top level must read them fromaction["arguments"]; rules matching onlytoolortypeare unaffected. Callers who build their own action dicts and pass them toevaluate()directly are unaffected, since they control the shape end to end.The migration hazard here is that the failure is silent: a rule reading an argument off the top level doesn't raise, it just stops matching. So this PR also updates
spec/layers/governance-layer.md— its custom-rule example previously taught exactly that pattern. The spec now documents the action-dict contract (which fields are trusted and why the nesting is a security boundary), reads fromargumentsin the example, and states the silent-failure mode so rules written against the old shape get re-checked during migration. The CHANGELOG carries the same migration note.Tests
19 new regression tests covering the bypass end to end, both channel gaps (including content-key fallbacks and the deliberate
broadcast_messages→broadcast_channelslabel mapping the dedup map depends on), and every cost bound. Full suite 682 passing,ruffandmypy --strictclean.