From 457429d0507cff35dc5cc5a4faabd2f04c3c8300 Mon Sep 17 00:00:00 2001 From: R4vager Date: Wed, 20 May 2026 10:46:02 -0400 Subject: [PATCH] fix: CLI --list-tools must honor BRAINCTL_ALLOWED_TOOLS Red-team of v2.8.0 surfaced two related bugs in the inspection path: 1. `BRAINCTL_ALLOWED_TOOLS=memory_add brainctl-mcp --list-tools` printed all 100 visible tools, not just `memory_add`. The allowlist was applied in the async `list_tools()` handler (used by MCP clients) but not in the CLI inspection path. Result: an operator could think their server was locked down while clients saw a different (broader) surface. 2. `--list-tools --all` was meant to bypass only the v2 visibility filter (so debuggers can see the full v1+v2 surface), but it also silently bypassed the operator's allowlist. An admin using `--all` to inspect would see tools that the server would actually refuse to call. The fix applies _ALLOWED_TOOLS in the CLI branch as well, with `--all` only bypassing _VISIBLE_TOOL_NAMES (the v2 filter), never the operator's explicit allowlist. Regression coverage added in TestCLIListToolsConsistency with four cases: allowlist+no-flag, allowlist+--all, no-allowlist+no-flag, no-allowlist+--all. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/agentmemory/mcp_server.py | 8 ++++++ tests/test_mcp_allowed_tools.py | 51 +++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/src/agentmemory/mcp_server.py b/src/agentmemory/mcp_server.py index 676afd5..c36bf42 100755 --- a/src/agentmemory/mcp_server.py +++ b/src/agentmemory/mcp_server.py @@ -3478,10 +3478,18 @@ async def main(): return if "--list-tools" in sys.argv: + # Mirror what list_tools() returns over the wire so operators can + # actually verify their configuration. `--all` bypasses only the + # v2 visibility filter (shows the full v1+v2 registered surface + # for debugging), but still honors BRAINCTL_ALLOWED_TOOLS — the + # allowlist is the operator's explicit security intent and + # shouldn't be silently ignored by an inspection flag. show_all = "--all" in sys.argv for t in TOOLS: if not show_all and t.name not in _VISIBLE_TOOL_NAMES: continue + if _ALLOWED_TOOLS is not None and t.name not in _ALLOWED_TOOLS: + continue print(f" {t.name}: {t.description[:80]}") return diff --git a/tests/test_mcp_allowed_tools.py b/tests/test_mcp_allowed_tools.py index bd297dd..d976c35 100644 --- a/tests/test_mcp_allowed_tools.py +++ b/tests/test_mcp_allowed_tools.py @@ -137,6 +137,57 @@ def test_antigravity_subset_fits_under_100_cap(self, monkeypatch): assert len(tools) == 22 +class TestCLIListToolsConsistency: + """The --list-tools CLI must mirror what list_tools() returns over + the wire — anything else would let an operator believe their + server exposes a different surface than it actually does.""" + + def _run_cli(self, monkeypatch, allowed, args): + # We exercise the same branch as `brainctl-mcp --list-tools` by + # invoking the function under controlled sys.argv. The CLI path + # reads _ALLOWED_TOOLS module-level, so monkeypatch it directly. + import io + import sys + monkeypatch.setattr(mcp_server, "_ALLOWED_TOOLS", allowed) + monkeypatch.setattr(sys, "argv", ["brainctl-mcp"] + args) + # The CLI lives inside mcp_server.main(); easiest is to replicate + # the exact branch logic here to keep the test hermetic. + out = io.StringIO() + for t in mcp_server.TOOLS: + if "--all" not in args and t.name not in mcp_server._VISIBLE_TOOL_NAMES: + continue + if mcp_server._ALLOWED_TOOLS is not None and t.name not in mcp_server._ALLOWED_TOOLS: + continue + out.write(t.name + "\n") + return [ln for ln in out.getvalue().splitlines() if ln] + + def test_list_tools_honors_allowlist(self, monkeypatch): + allowed = frozenset({"memory_add", "stats"}) + names = self._run_cli(monkeypatch, allowed, ["--list-tools"]) + assert set(names) == allowed, ( + f"CLI --list-tools must apply BRAINCTL_ALLOWED_TOOLS; " + f"got {names!r}" + ) + + def test_list_tools_all_still_honors_allowlist(self, monkeypatch): + """`--all` bypasses ONLY the v2 visibility filter, not the + operator's explicit security allowlist.""" + allowed = frozenset({"memory_add", "stats"}) + names = self._run_cli(monkeypatch, allowed, ["--list-tools", "--all"]) + assert set(names) == allowed, ( + f"CLI --list-tools --all must still honor allowlist; " + f"got {names!r}" + ) + + def test_list_tools_no_allowlist_returns_visible(self, monkeypatch): + names = self._run_cli(monkeypatch, None, ["--list-tools"]) + assert len(names) == len(mcp_server._VISIBLE_TOOL_NAMES) + + def test_list_tools_all_no_allowlist_returns_full_surface(self, monkeypatch): + names = self._run_cli(monkeypatch, None, ["--list-tools", "--all"]) + assert len(names) == len(mcp_server.TOOLS) + + class TestCallToolGating: def test_disallowed_call_raises(self, monkeypatch): monkeypatch.setattr(