fix(mcp): a read key could read notes but not snippets or design systems
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 7s
CI & Build / integration (push) Successful in 16s
CI & Build / TypeScript typecheck (push) Successful in 23s
CI & Build / Python tests (push) Successful in 47s
CI & Build / Build & push image (push) Successful in 28s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 7s
CI & Build / integration (push) Successful in 16s
CI & Build / TypeScript typecheck (push) Successful in 23s
CI & Build / Python tests (push) Successful in 47s
CI & Build / Build & push image (push) Successful in 28s
_READ_ONLY_TOOLS fails closed, which is the right design — but the list had gone stale, so a read-only key could get_note and not get_snippet, both pure reads of the same table, and could not read a design system at all. That inverts the sensitivity ordering: the free-text records were reachable and the structured, low-sensitivity ones were not. `find_duplicate_snippets` sitting in the list was the tell — someone classified the report and missed the getters beside it. Adds the twelve reads that were missing: snippets, processes, the six design system tools, and list_repo_bindings. Each verified to mutate nothing rather than assumed — this is a security boundary, and a wrong entry does not cost what a missing one costs. record_pulled on four getters is telemetry about the read, not a change to what was read, and get_note already carried it inside the boundary. The list stays explicit. Deriving it from the name would be worse than staleness: it makes the boundary follow a naming convention, so any future get_* grants itself access. list_starter_role_groups is the live illustration — it reads a constant, but names create_design_system in its docstring, so a pattern-matcher flags it. So derive the CANDIDATES and keep the DECISION explicit: a new test asserts every read-shaped tool appears in _READ_ONLY_TOOLS or in a declared _DELIBERATELY_WRITE_SCOPED, and that neither set names a tool that no longer exists. Adding a getter now forces a classification at review time instead of denying it silently. The second set is empty and stays declared — otherwise a future get_or_create_* would be pushed into the allow-list to make the test pass, which is the wrong way to satisfy it. Third instance of the same shape, after #2476 and #2444: a hand-written enumeration that missed the members added after it was written. Refs #2496
This commit is contained in:
@@ -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).
|
||||
|
||||
Reference in New Issue
Block a user