Make the agent's tool surface fail closed, and test that it does - #301
Merged
Merged
Conversation
Two small accuracy fixes found while reviewing, neither changing behaviour. 1. Three `eslint-disable-next-line react-hooks/exhaustive-deps` comments suppress nothing. eslint reports them as unused directives, which means the rule no longer fires on those lines — the dependency arrays were completed and the suppressions left behind. AdminPage.jsx was a plain duplicate: two identical directives on consecutive lines, so the first applied to the second comment rather than to any code. These matter more than the count suggests. A stale exhaustive-deps suppression silently swallows the next genuine missing dependency on that line, which is a real bug class in effect hooks — exactly the kind of thing the rule exists to catch. Lint drops 47 -> 44 problems, still 0 errors. 2. app/core/migrations.py opened with "Lightweight schema sync for SQLite", and all of its caveats are written in SQLite terms, which reads as though it were self-host-only machinery. It is not: main.py calls sync_schema() unconditionally on every boot against whatever engine is configured, so on Fly this module plus create_all() IS the production Postgres schema management — there is no Alembic in this repo. Says so now, and notes where the SQLite caveats read differently on Postgres. ruff clean, 864 backend tests pass, frontend lint exits 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The autonomous agent's reachable tool set was computed as `MCP_ALL_TOOLS - _AGENT_DENIED_TOOLS`, a denylist holding one entry: set_camera_recording_policy. That fails OPEN. Any new write tool added server-side would silently become reachable by the agent, and nothing about adding a tool prompts you to remember a denylist elsewhere in the file. It matters more here than in most places. The agent's LLM is steered by content an attacker can influence — camera names, and text on a sign held up to a lens — and "disable recording, then report all clear" is the canonical injection against a camera product. The existing defenses are good: the system prompt names that exact attack and tells the model to treat the attempt as suspicious activity, and ScopeMiddleware enforces the tool set at on_call_tool, not just on_list_tools, so an unlisted tool can't be invoked anyway. This closes the remaining direction of drift. The surface is now reads + an explicit incident-authoring allowlist, computed by compute_agent_allowed_tools(). Intersecting with the registry means a typo grants nothing rather than naming a tool the server doesn't serve. This mirrors compute_allowed_tools(), which already reasons this way for user-key custom scopes — "unknown names are silently dropped so a disallowed tool can't be enabled by typo or by adding a new WRITE tool server-side". The agent path just hadn't been inverted to match. ON TESTING THIS, because it is subtle: today the allowlist and the denylist produce an IDENTICAL set, since the config tool is the only write tool the agent is denied. So no assertion about the constant can tell them apart. My first attempt at these tests passed just as happily against the denylist they were written to forbid, which is the same always-green gate this codebase has been cleaning up all week. That is why compute_agent_allowed_tools() takes the registry as a parameter: the decisive test hands it a registry containing a tool that does not exist yet and asserts it stays out. Verified by putting the denylist back — 2 of the 8 tests fail, and they are the two that encode the property. There was no test of this control at all before; a refactor could have removed it with every suite still green. 872 backend tests pass, ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The shape of the problem
The autonomous agent's reachable tool set was computed as:
That fails open. Any new write tool added server-side silently becomes
reachable by the agent, and nothing about adding a tool prompts you to
remember a denylist elsewhere in the file.
It matters more here than in most places: the agent's LLM is steered by
content an attacker can influence — camera names, and text on a sign held up
to a lens. "Disable recording, then report all clear" is the canonical
injection against a camera product.
What was already good
This is hardening an existing, well-built defense, not fixing a hole:
attempt itself as suspicious activity worth reporting
ScopeMiddlewareenforces the tool set aton_call_tool, not juston_list_tools— so an unlisted tool can't be invoked anywayso a leaked secret can't be pointed at an arbitrary tenant
The change
Reads plus an explicit incident-authoring allowlist, via
compute_agent_allowed_tools(). Intersecting with the registry means a typogrants nothing rather than naming a tool the server doesn't serve.
This mirrors
compute_allowed_tools(), which already reasons exactly this wayfor user-key custom scopes — "unknown names are silently dropped so a
disallowed tool can't be enabled by typo or by adding a new WRITE tool
server-side." The agent path simply hadn't been inverted to match.
On testing this, because it's subtle
Today the allowlist and the denylist produce an identical set — the config
tool is the only write tool the agent is denied. So no assertion about the
constant can distinguish them.
My first draft of these tests passed just as happily against the denylist they
were written to forbid. That's the same always-green gate this repo has been
cleaning up all week, so it's worth being explicit about.
That's why
compute_agent_allowed_tools()takes the registry as a parameter:the decisive test hands it a registry containing a tool that doesn't exist yet.
Those are the two tests that encode the property. There was no test of this
control at all before — a refactor could have removed it with every suite
still green.
Verification
ruffclean🤖 Generated with Claude Code