diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index 9b7cd43..9bec58e 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -282,6 +282,20 @@ operator. "Works for one user" is not done. # Tools a read-only API key may call. Anything not listed is treated as a # write for read keys (default-deny), so a newly-added tool is locked down # until explicitly classified here. +# +# The list stays EXPLICIT rather than being derived from the name. A read key is +# what you hand to something you don't fully trust — a dashboard, a CI job, a +# shared integration — and a boundary inferred from a naming convention grants +# access to whatever a future author happens to call `get_*`. Enumerating it is +# the point; staleness is the cost, and test_mcp_auth covers that (a read-shaped +# tool must appear here or in _DELIBERATELY_WRITE_SCOPED below, so adding one +# forces a decision instead of silently denying it). +# +# Membership means "reads the operator's data and mutates none of it". Several +# getters record a retrieval event via record_pulled; that is telemetry about +# the read itself, not a change to what was read, and it must keep working for a +# read key or the corpus's surfaced:pulled ratio silently under-counts whichever +# consumers hold one. _READ_ONLY_TOOLS = frozenset({ "get_note", "get_project", "get_rule", "get_rulebook", "get_task", "get_milestone", "get_recent", "enter_project", @@ -292,8 +306,30 @@ _READ_ONLY_TOOLS = frozenset({ # Reports on the snippet corpus. Reads only — the merge it suggests is a # separate, explicitly-called write. "find_duplicate_snippets", + # Snippets and processes are notes with a kind. A key that may read a note + # but not a snippet inverts the sensitivity ordering: it exposes the + # free-text records and withholds the structured ones (#2496). + "get_snippet", "list_snippets", + "get_process", "list_processes", + # Design systems: read, resolve (inheritance + mode), render, and compare + # against recorded snippets. All four compute from stored records and write + # nothing — the drift report is a report, and applying it is a separate + # explicit call. + "get_design_system", "list_design_systems", "resolve_design_system", + "get_design_system_stylesheet", "list_design_tokens", + "check_snippets_against_design_system", "list_starter_role_groups", + # Which repos map to which project. Read-only by nature; bind_repo / + # unbind_repo are the writes. + "list_repo_bindings", }) +# Read-SHAPED tools that must NOT be reachable with a read key — a getter that +# creates on miss, a list that has a side effect. Empty today, and deliberately +# kept as a declared escape hatch rather than left implicit: without it, the +# completeness test would push a future `get_or_create_*` into the allow-list +# above, which is exactly the wrong way to make a test pass. +_DELIBERATELY_WRITE_SCOPED: frozenset[str] = frozenset() + async def _buffer_request_body(receive): """Drain the ASGI request body and return (body_bytes, replay_receive). diff --git a/tests/test_mcp_auth.py b/tests/test_mcp_auth.py index 3a8203d..6c3a32c 100644 --- a/tests/test_mcp_auth.py +++ b/tests/test_mcp_auth.py @@ -92,3 +92,68 @@ def test_body_calls_write_tool_classifies_correctly(): json.dumps({"method": "tools/list"}).encode() ) is False assert _body_calls_write_tool(b"not json") is False + + +def test_every_read_shaped_tool_is_explicitly_classified(): + """A read-shaped tool must be classified, not left to default-deny. + + `_READ_ONLY_TOOLS` is hand-maintained, and default-deny means a getter + omitted from it fails CLOSED — safe, but silent. That is how a read key + ended up able to `get_note` and not `get_snippet`, both pure reads of the + same table, while design systems were unreachable entirely (#2496). The + `find_duplicate_snippets` entry was the tell: someone classified the report + and missed the getters beside it. + + This is the same shape as #2476 (record_pulled on three of four getters) — + a hand-written enumeration that missed the members added after it. The fix + there and here is the same: derive the CANDIDATES, keep the DECISION + explicit. Deriving the decision itself would be worse than a stale list — + it would make a security boundary follow a naming convention, so any future + `get_*` grants itself access. + + So: every tool whose name reads like a read must appear in one of the two + sets. Adding a getter then forces a choice at review time. + """ + import ast + import pathlib + + from scribe.mcp.server import _DELIBERATELY_WRITE_SCOPED, _READ_ONLY_TOOLS + + tools_dir = (pathlib.Path(__file__).resolve().parents[1] + / "src" / "scribe" / "mcp" / "tools") + read_shaped = { + node.name + for path in tools_dir.glob("*.py") if path.name != "__init__.py" + for node in ast.parse(path.read_text()).body + if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)) + and node.name.startswith(("get_", "list_", "search", "resolve_", + "check_", "find_")) + } + assert read_shaped, "found no read-shaped tools — the tools package moved" + + unclassified = sorted(read_shaped - _READ_ONLY_TOOLS + - _DELIBERATELY_WRITE_SCOPED) + assert not unclassified, ( + f"these read-shaped tools are classified by neither set: {unclassified}. " + f"They currently fail closed for read-only keys, silently. Add each to " + f"_READ_ONLY_TOOLS if it mutates nothing, or to " + f"_DELIBERATELY_WRITE_SCOPED with a comment saying what it writes." + ) + + # The reverse: a name in either set that no longer exists is a rename or a + # deletion, and a stale grant is worth surfacing even though it grants + # access to nothing. `enter_project` is the one read tool without a read + # prefix, so it is checked against the full tool set, not `read_shaped`. + all_tools = { + node.name + for path in tools_dir.glob("*.py") if path.name != "__init__.py" + for node in ast.parse(path.read_text()).body + if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)) + and not node.name.startswith("_") and node.name != "register" + } + phantom = sorted((_READ_ONLY_TOOLS | _DELIBERATELY_WRITE_SCOPED) - all_tools) + assert not phantom, ( + f"these names are classified but are not tools: {phantom}. They were " + f"renamed or removed — drop them, and check whatever replaced them got " + f"classified." + )