security(mcp): govern manual_mcp as the twelfth bridge, fix double mount (#14586)

[mrveiss]

Closes #14586

Thinking Path

api/manual_mcp.py exposed three MCP tools (lookup_man_page,
search_man_pages, get_doc_index) gated only by Depends(get_current_user)
— any authenticated user, no permission check. It was excluded from
mcp_bridge_scan.bridge_files() by a bare if p.stem != "manual_mcp" with no
comment, absent from mcp_registry.py's _BRIDGE_MODULE_REGISTRY, and
therefore unreachable by required_permission() — meaning the LLM/agent
dispatch path (MCPDispatcher.dispatch()) already denied every call to these
tools as "undeclared", for every role including a plain user. Also mounted
twice: once as a core router (core_routers.py, prefix /manual) and again
as an "optional" router (mcp_routers.py, prefix "").

Checked what the three tools actually reach: _query_doc_index runs
subprocess.run(["man", "-k", query], capture_output=True, text=True, timeout=10) — argument list, no shell=True, explicit timeout — and
_lookup_man_page parses local man pages via services/man_page_parser.py,
caching results in the knowledge Redis database. No filesystem write, no
network egress, no code execution beyond that fixed man invocation. This is
read-only system documentation, confirming the issue's own severity read: an
authorization/governance gap, not an injection one.

Decision: govern it as the twelfth bridge, not keep the exclusion. Two
peer bridges (knowledge_mcp, redis_mcp) already use the identical
"authenticated-user-only HTTP + agent-path-governed" model for read-mostly
tools, so this brings manual_mcp to parity with that precedent rather than
inventing a fourth authorization shape. A read-level permission
(Permission.MCP_READ — held by every role except readonly) is the correct
grant, per the issue's own hint.

One discovery mid-fix: manual_mcp's tool schemas were plain dicts
({"name": ..., ...}), a fourth source shape mcp_bridge_scan's three
regexes never matched. Un-excluding it without fixing that would have
registered a bridge with zero discovered tools — the "guard narrower than
its own subject" trap this repo has hit before (sequential_thinking_mcp,
redis_mcp). Reshaped the three schema functions to return MCPTool(name=...)
instances, the shape every other bridge already uses, before adding the
TOOL_PERMISSIONS entries.

What Changed

• autobot-backend/api/manual_mcp.py — tool schemas are now MCPTool
  instances instead of raw dicts; added a MANIFEST (matching every sibling
  bridge) so mcp_registry.discover_bridges()'s module-scan fallback works.
• autobot_shared/auth/mcp_bridge_scan.py — replaced the bare
  p.stem != "manual_mcp" check with an auditable EXCLUDED_BRIDGE_STEMS: dict
  (empty today). A future exclusion needs a reasoned entry there, not a second
  inline stem check.
• autobot_shared/auth/mcp_tool_permissions.py — manual_mcp: MCP_READ in
  BRIDGE_DEFAULT_PERMISSIONS; lookup_man_page, search_man_pages,
  get_doc_index each declared MCP_READ in TOOL_PERMISSIONS. Updated the
  "eleven governed bridges" prose to twelve where it describes current
  behaviour (left dated #14523-time measurement snapshots alone).
• autobot-backend/api/mcp_registry.py — added manual_mcp to
  _BRIDGE_MODULE_REGISTRY so discover_bridges() includes it and its tools
  reach MCPDispatcher._tool_cache.
• autobot-backend/initialization/router_registry/mcp_routers.py — removed
  the duplicate manual_mcp entry from optional_mcp_configs; it stays
  mounted once, as a core router.
• tools/lint/check_mcp_tool_permission_coverage.py — added
  "manual_mcp": 3 to PER_BRIDGE_DISCOVERY_FLOOR, raised DISCOVERY_FLOOR
  90 → 93 for the new 107-tool total.
• autobot_shared/auth/mcp_bridge_scan_test.py (new) — the durable guard:
  statically cross-checks every *_mcp router actually mounted in
  core_routers.py/mcp_routers.py against bridge_files() and
  EXCLUDED_BRIDGE_STEMS, failing if a future router lands in neither. Also
  pins EXCLUDED_BRIDGE_STEMS == {} and requires any future entry to carry a
  reason and not name an already-governed bridge.
• autobot_shared/auth/mcp_tool_permissions_test.py — added a test pinning
  manual_mcp's three tools to MCP_READ.
• autobot-backend/middleware/builtin/permission_enforcement.py,
  autobot-backend/services/mcp_dispatch.py — updated "eleven governed
  bridges" → "twelve" in the two present-tense docstring claims about the
  current precondition (left historical #14523-time snapshots as written).

Not changed, deliberately: the direct HTTP endpoints in manual_mcp.py still
gate on Depends(get_current_user) alone, matching knowledge_mcp and
redis_mcp exactly. #13228 already documents this as one of three parallel
authorization models (HTTP-direct vs. agent-dispatch vs. external MCP/JWT);
unifying HTTP-direct access across all bridges is out of scope for a
twelfth-bridge governance fix and would need its own issue.

Verification

Static, no app boot (regex cross-checks against the real source files, not
imports):

• mcp_bridge_scan's three tool-declaration regexes now find manual_mcp's
  3 tools (lookup_man_page, search_man_pages, get_doc_index) — 0 before
  this PR's schema reshape.
• Full bridge scan across all 12 bridges: 107 live tools total (was 104),
  manual_mcp: 3, every PER_BRIDGE_DISCOVERY_FLOOR entry matches exactly,
  0 name collisions, 0 tools missing a TOOL_PERMISSIONS entry, 0 unexplained
  TOOL_PERMISSIONS entries outside _DECLARED_AHEAD_OF_TIME.
• Router-mount cross-check: all 12 bridges mounted in core_routers.py
  resolve as governed; autobot_mcp_router (a distinct, out-of-scope module —
  api/autobot_mcp_router.py, not a *_mcp.py/*_mcp/ bridge) correctly
  does not false-positive as an ungoverned bridge.

Exploit-driven verification of the actual decision path (_would_deny /
required_permission), per the task's "run the exploit" requirement — a
standalone scratch reproduction (not imported from the repo), driving two
non-admin identities (user, readonly) at all three tools:

• Before (tools absent from the permission table, the pre-PR state):
  both user and readonly denied on all 3 tools via the agent/LLM dispatch
  path — 6/6 denied. Confirms these tools were unreachable by an LLM agent
  call before this PR, not merely under-permissioned.
• After (this PR's TOOL_PERMISSIONS entries, MCP_READ): user (holds
  MCP_READ) allowed on all 3 tools, readonly (does not hold it) still
  denied on all 3, admin allowed on all 3 — 9/9 assertions as intended.
• Mutation (table reverted to empty, simulating a silently regressed
  declaration): the after/user assertion fails immediately — the guard does
  not fail open.

Confirmed no new TOOL_PERMISSIONS entries were needed to avoid taking the
feature offline under #14523's deny-by-default for the agent path — the
tools were already denied there pre-PR (undeclared), so #14523 changed
nothing about their agent-path reachability. What this PR adds is the actual
grant: without it they would remain permanently unreachable by an agent, even
though the direct HTTP endpoint (unaffected by this table either way) still
answers any authenticated caller.

Model Used

Claude Opus 5 (1M context)

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!

[mrveiss]

Review — APPROVE

Every load-bearing claim re-derived independently rather than taken from the body.

The zero-tools discovery is real, and it is the most valuable part of this PR. Traced the three scan regexes by hand against the pre-PR source: _TUPLE_TOOL/_MODULE_TUPLE_TOOL need a bare ( or ALL_CAPS = ( opener and the code returned return {; _KWARG_TOOL needs a literal name="..." and the source had "name": "lookup_man_page" — a colon, not an equals. 0 of 3 tools were parseable. Un-excluding the bridge without reshaping would have registered a bridge that looks governed and gates nothing, which is strictly worse than the named exclusion it replaced. Catching that mid-fix is the difference between closing this issue and appearing to.

It also confirms, by instance, the premise #14523 was filed on: "a tool declared in a source shape mcp_bridge_scan does not recognise." A fourth shape existed. That does not invalidate the 104/104 precondition I verified before merging #14572 — that count was correct for the shapes the scanner parses — but it is exactly why deny-by-default was the right call. The CI-time count can be incomplete; the runtime now fails closed regardless.

Counts, independently derived: 104 across the eleven pre-existing bridges, each matching its PER_BRIDGE_DISCOVERY_FLOOR entry exactly; manual_mcp contributes 3 after the reshape (the MANIFEST(name="manual_mcp", ...) line self-cancels via the bridge-name exclusion); 107 tools, 12 bridges, 0 collisions. The 6-entry gap between 110 TOOL_PERMISSIONS keys and 104 live tools is exactly _DECLARED_AHEAD_OF_TIME, and the 3 new entries cover the 3 new tools with nothing spare.

MCP_READ is the right grant, and the cache-write question resolves cleanly. MCP_READ is held by ADMIN/OPERATOR/ANALYST/EDITOR/USER and absent from READONLY. The Redis setex is a genuine write, but command/section are validated by _VALID_COMMAND_RE/_VALID_SECTION_RE before reaching either the subprocess or the key builder, so the key stays inside the fixed manual_mcp: namespace and the value is the subprocess's own parsed output — never caller-supplied. A caller cannot write anything it does not already have read access to. That is a cache-aside pattern, not a smuggled write capability.

The mount guard is load-bearing, not vacuous. Both regexes resolve to a non-empty 12-stem set. And api/autobot_mcp_router.py is excluded by the intersection with real files on disk, not by luck: _VAR_REF does yield a false autobot_mcp candidate, and no such module or package exists, so it is filtered. Worth stating because a guard that excludes the right thing for the wrong reason breaks on the next rename.

Double mount: the mcp_routers.py entry is removed and core_routers.py's registration is untouched — exactly one remains.

Nit, for a follow-up rather than here

_query_doc_index's ["man", "-k", query] carries no # nosec and no explicit validation of query, while the two comparable calls in services/man_page_parser.py both carry # nosec B603 plus a regex-validation comment. Argv-list form rules out injection so the risk is low — but this PR is precisely about bringing manual_mcp to parity with its peers, and annotation style is part of that. The code is untouched by the diff and your "not changed, deliberately" section scopes it out correctly; worth its own issue.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!