From f2c1ebba1ce5fbb6d3cfeaf01e438c98f6deebe4 Mon Sep 17 00:00:00 2001 From: S'Bussiso Dube <80188685+Sbussiso@users.noreply.github.com> Date: Sat, 12 Sep 2026 23:25:46 -0700 Subject: [PATCH 1/2] Remove three stale lint suppressions; say which DB the migrator manages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/app/core/migrations.py | 17 ++++++++++++++++- frontend/src/hooks/useMotionAlerts.jsx | 1 - frontend/src/hooks/useNotifications.jsx | 1 - frontend/src/pages/AdminPage.jsx | 1 - 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/backend/app/core/migrations.py b/backend/app/core/migrations.py index d784d6a..62f79ea 100644 --- a/backend/app/core/migrations.py +++ b/backend/app/core/migrations.py @@ -1,4 +1,19 @@ -"""Lightweight schema sync for SQLite + one-shot migration helpers. +"""Lightweight schema sync + one-shot migration helpers. + +THIS MANAGES THE PRODUCTION POSTGRES SCHEMA, not just SQLite. The header +used to say "for SQLite", and every caveat below is still written in +SQLite terms, which reads as though this were self-host-only machinery. +It is not: ``app/main.py`` calls ``sync_schema(engine, Base.metadata)`` +unconditionally on every boot, against whatever engine is configured — +Postgres on Fly, SQLite for a self-hosted install. There is no Alembic +in this repo, so this module plus ``create_all`` IS the schema +management for production. + +Two of the SQLite caveats below read differently on Postgres: adding a +NOT NULL column without a default fails there too, but an ADD COLUMN +with a non-volatile default is metadata-only and fast (PG11+), where +SQLite rewrites. The "renames, type changes and drops need a real +migration" caveat applies equally to both. Two kinds of function live here: diff --git a/frontend/src/hooks/useMotionAlerts.jsx b/frontend/src/hooks/useMotionAlerts.jsx index dc1f02b..e1151d4 100644 --- a/frontend/src/hooks/useMotionAlerts.jsx +++ b/frontend/src/hooks/useMotionAlerts.jsx @@ -133,6 +133,5 @@ export function useMotionAlerts(cameras) { abortRef.current?.abort() } // orgId: tear down + reconnect the stream under the new org's token. - // eslint-disable-next-line react-hooks/exhaustive-deps }, [getToken, showToast, orgId]) } diff --git a/frontend/src/hooks/useNotifications.jsx b/frontend/src/hooks/useNotifications.jsx index a1f062c..4a9e3d4 100644 --- a/frontend/src/hooks/useNotifications.jsx +++ b/frontend/src/hooks/useNotifications.jsx @@ -213,7 +213,6 @@ export function useNotifications() { controller?.abort() } // orgId: tear down + reconnect the stream under the new org's token. - // eslint-disable-next-line react-hooks/exhaustive-deps }, [getToken, orgId]) return { diff --git a/frontend/src/pages/AdminPage.jsx b/frontend/src/pages/AdminPage.jsx index 1df20ad..e50450d 100644 --- a/frontend/src/pages/AdminPage.jsx +++ b/frontend/src/pages/AdminPage.jsx @@ -303,7 +303,6 @@ function AdminPage() { if (reader) reader.cancel().catch(() => {}) } // eslint-disable-next-line react-hooks/exhaustive-deps - // eslint-disable-next-line react-hooks/exhaustive-deps }, [organization?.id, hasAdminFeature]) const handleMcpFilterChange = (key, value) => { From 122ce3bc79d6e544ec5442ea69f639c5ceb2db02 Mon Sep 17 00:00:00 2001 From: S'Bussiso Dube <80188685+Sbussiso@users.noreply.github.com> Date: Sun, 13 Sep 2026 20:47:16 -0700 Subject: [PATCH 2/2] Make the agent's tool surface fail closed, and test that it does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/app/mcp/server.py | 55 ++++++++++++-- backend/tests/test_mcp_agent_scope.py | 105 ++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 6 deletions(-) create mode 100644 backend/tests/test_mcp_agent_scope.py diff --git a/backend/app/mcp/server.py b/backend/app/mcp/server.py index 96fb3ae..dfb7838 100644 --- a/backend/app/mcp/server.py +++ b/backend/app/mcp/server.py @@ -113,14 +113,57 @@ MCP_ALL_TOOLS: frozenset[str] = MCP_READ_TOOLS | MCP_WRITE_TOOLS -# Tools the autonomous Sentinel agent may NOT invoke (see the agent -# branch in ScopeMiddleware._lookup_allowed). Config writes only — -# incident authoring stays available. -_AGENT_DENIED_TOOLS: frozenset[str] = frozenset({ - "set_camera_recording_policy", +# The write tools the autonomous Sentinel agent MAY invoke: incident +# authoring, nothing else (see the agent branch in +# ScopeMiddleware._lookup_allowed). +_AGENT_WRITE_TOOLS: frozenset[str] = frozenset({ + "create_incident", + "add_observation", + "attach_snapshot", + "attach_clip", + "update_incident", + "finalize_incident", }) +def compute_agent_allowed_tools( + all_tools: frozenset[str], + read_tools: frozenset[str], + agent_write_tools: frozenset[str], +) -> frozenset[str]: + """The agent's reachable tool set: reads + an explicit write allowlist. + + ALLOWLIST, NOT A SUBTRACTION — this is the whole point of the function. + It used to be ``MCP_ALL_TOOLS - {"set_camera_recording_policy"}``, a + denylist of one, which fails OPEN: any new write tool added server-side + would silently become reachable by the agent. That matters more here + than in most places, because 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. + + Today the two formulations produce an identical set, because the only + non-agent write tool IS the config one. So no test of the *constant* + can tell them apart — which is exactly why this takes the registry as + a parameter: a test can pass a registry containing a future write tool + and assert it stays out. See tests/test_mcp_agent_scope.py. + + ``compute_allowed_tools`` already applies this reasoning to 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 had not been inverted to match. + + Intersecting with ``all_tools`` keeps a typo or a renamed tool from + granting a name the server does not serve. + """ + return (read_tools | agent_write_tools) & all_tools + + +_AGENT_ALLOWED_TOOLS: frozenset[str] = compute_agent_allowed_tools( + MCP_ALL_TOOLS, MCP_READ_TOOLS, _AGENT_WRITE_TOOLS +) + + def compute_allowed_tools(scope_mode: str | None, scope_tools: list[str] | None) -> frozenset[str]: """Resolve a key's scope config into the concrete allowed-tool set. @@ -303,7 +346,7 @@ def _lookup_allowed(self) -> frozenset[str] | None: # scope semantics below. agent_key = settings.SENTINEL_AGENT_MCP_KEY if agent_key and hmac.compare_digest(raw_key, agent_key): - return MCP_ALL_TOOLS - _AGENT_DENIED_TOOLS + return _AGENT_ALLOWED_TOOLS key_hash = hashlib.sha256(raw_key.encode()).hexdigest() diff --git a/backend/tests/test_mcp_agent_scope.py b/backend/tests/test_mcp_agent_scope.py new file mode 100644 index 0000000..0298a66 --- /dev/null +++ b/backend/tests/test_mcp_agent_scope.py @@ -0,0 +1,105 @@ +"""The autonomous agent's MCP tool surface must stay fail-closed. + +Why this file exists: the agent's LLM is steered by content an attacker can +influence — camera names, and on-screen text in the snapshots it looks at. +"Disable recording, then report all clear" is the canonical prompt injection +against a camera product, so the agent is structurally barred from config +writes rather than merely told not to make them in its system prompt. + +That control had no test at all. A refactor could have removed it and every +existing test would still have passed. + +A note on what CAN be tested here. Today +``MCP_READ_TOOLS | _AGENT_WRITE_TOOLS`` and +``MCP_ALL_TOOLS - {"set_camera_recording_policy"}`` produce an *identical* +set, because the config tool is the only write tool the agent is denied. So +asserting things about the constant cannot distinguish an allowlist from a +denylist — an earlier draft of this file tried, and passed just as happily +against the denylist it was written to forbid. The difference only shows up +when a new write tool appears, which is why ``compute_agent_allowed_tools`` +takes the registry as an argument and the decisive test below hands it a +registry from the future. +""" + +import pytest + +from app.mcp.server import ( + _AGENT_ALLOWED_TOOLS, + _AGENT_WRITE_TOOLS, + MCP_ALL_TOOLS, + MCP_READ_TOOLS, + MCP_WRITE_TOOLS, + compute_agent_allowed_tools, +) + +# Write tools that exist for humans and integrations but must never be +# reachable by the agent. Listed explicitly so that adding one is deliberate. +CONFIG_WRITE_TOOLS = frozenset({"set_camera_recording_policy"}) + + +def test_agent_cannot_reach_config_writes(): + """The specific tool that would let an injection disable recording.""" + for tool in CONFIG_WRITE_TOOLS: + assert tool in MCP_WRITE_TOOLS, ( + f"{tool} is no longer a write tool — this test's premise moved" + ) + assert tool not in _AGENT_ALLOWED_TOOLS, ( + f"{tool} became reachable by the Sentinel agent. An injected " + f"instruction in a camera name, or on a sign held up to a lens, " + f"could now invoke it." + ) + + +def test_a_future_write_tool_is_unreachable_by_default(): + """THE decisive test: fail-closed against a tool that doesn't exist yet. + + A denylist implementation passes every other test in this file and fails + this one, which is the only reason the derivation was inverted. + """ + future_registry = MCP_ALL_TOOLS | {"delete_all_cameras"} + + allowed = compute_agent_allowed_tools( + future_registry, MCP_READ_TOOLS, _AGENT_WRITE_TOOLS + ) + assert "delete_all_cameras" not in allowed, ( + "a newly added server-side write tool became reachable by the agent " + "without anyone opting it in" + ) + + # Demonstrate the failure mode this guards against, so the test documents + # why the shape matters rather than just asserting a set membership. + denylist_style = future_registry - CONFIG_WRITE_TOOLS + assert "delete_all_cameras" in denylist_style + assert allowed != denylist_style + + +def test_unknown_names_cannot_grant_access(): + """A typo in the allowlist grants nothing — it does not invent a tool.""" + allowed = compute_agent_allowed_tools( + MCP_ALL_TOOLS, MCP_READ_TOOLS, frozenset({"craete_incident"}) + ) + assert "craete_incident" not in allowed + assert allowed == MCP_READ_TOOLS + + +def test_agent_write_tools_all_exist(): + """Catches a rename: a dropped name silently shrinks the agent.""" + unknown = _AGENT_WRITE_TOOLS - MCP_ALL_TOOLS + assert not unknown, ( + f"_AGENT_WRITE_TOOLS names tools that do not exist: {sorted(unknown)}" + ) + assert _AGENT_WRITE_TOOLS <= MCP_WRITE_TOOLS + + +def test_agent_keeps_every_read_tool(): + """Investigation is the agent's whole job; reads must stay intact.""" + missing = MCP_READ_TOOLS - _AGENT_ALLOWED_TOOLS + assert not missing, f"agent lost read tools: {sorted(missing)}" + + +@pytest.mark.parametrize( + "tool", ["create_incident", "add_observation", "finalize_incident"] +) +def test_agent_can_still_author_incidents(tool): + """The agent must be able to record what it found.""" + assert tool in _AGENT_ALLOWED_TOOLS