From a143cd065ba06e1a2bdc5143a19ec156e53650ef Mon Sep 17 00:00:00 2001 From: jcwalker3 Date: Tue, 28 Jul 2026 11:15:00 -0500 Subject: [PATCH] fix(transport): gate transport execution behind commissioning (review 635) Addresses B1 and B2 from formal review 635 on PR #966. Both are one defect: the seam conflated two questions that must stay separate. recognition: "is this an identifier this system knows, and may it be bound, pinned and recorded?" execution: "is this entrypoint commissioned to actually serve on it?" SUPPORTED_TRANSPORTS answered the first and was then used to answer the second, so registering streamable-http was the same act as executing it. With mcp.run fed the bound value, GITEA_MCP_TRANSPORT=streamable-http reached FastMCP.run -> run_streamable_http_async and started a uvicorn HTTP server over the whole tool surface with auth=None, no TLS and no per-request principal. That endpoint is owned by #938 and is an explicit non-goal of #931. B2 is why B1 was reachable: bound_transport was read only by reporting sites, so no decision anywhere depended on it. The fix adds an execution-authorization layer rather than removing the identifier or mapping it to stdio, both of which would have destroyed the seam #931 exists to create. mcp_transport_config gains EXECUTABLE_TRANSPORTS, a strict subset of SUPPORTED_TRANSPORTS containing only the local transport, plus TRANSPORT_EXECUTION_OWNER recording that #938 commissions the remote listener. assess_transport_execution returns a structured verdict: transport, recognized, executable, allowed, blocker_kind, owner_issue, reasons and exact_next_action. mcp_daemon_guard gains assess_serve_authorization, which is the decision that consumes bound_transport and is what stops it being reporting-only metadata, and authorize_transport_execution, which enforces two ordered boundaries: assert_transport_bound first, so an unbound runtime keeps its pre-existing #695 failure and reason code; then execution authorization, so a registered but uncommissioned transport is refused before any listener exists and before any tool can dispatch. TransportExecutionError subclasses UnsanctionedRuntimeError, so every existing fail-closed handler still catches it while a caller that cares can distinguish the two cases. native_runtime_status now reports executable_transports, serve_authorized and the full serve_authorization verdict. The entrypoint serves through authorize_transport_execution. Net effect. stdio: unchanged, still binds and still reaches the runner. streamable-http: still recognized, still validated, still pinned, still recorded in decision-lock and audit provenance -- and refused at the serve boundary with blocker transport_listener_not_commissioned naming #938. Unregistered identifiers still fail earlier, at bind validation. Unbound execution keeps the #695 contract. Rebind idempotence and conflict rejection, session isolation, provenance and secret handling are untouched. Tests: 63 in tests/test_issue_931_transport_bind_seam.py, up from 42. New coverage proves stdio reaches the real runner; the remote identifier binds and is recorded yet never reaches run(); no socket is bound during the refusal, asserted with a socket.bind tripwire; the refusal names the transport, the unmet requirement and #938; the decision genuinely consumes bound_transport, proven by flipping only that value; no mutation is authorized after denial; verdicts do not leak across runtimes; and the refusal carries no credential material. Per review 635 the two file-list scans are now globs over the production surface, and a new test asserts there is exactly one mcp.run site and that it routes through the guard. #956 anchors re-anchored for the two lines this change shifted, mcp_daemon_guard.py 178->195 and 519->583. No other anchor moved. Full suite from the branches/ worktree: 28 failed, 5864 passed, 6 skipped, 1047 subtests. Base 9b80e75c: 28 failed, 5801 passed, 6 skipped, 1047 subtests. Failing test IDs are identical sets in both directions; the +63 passed are this module. No listener, socket, authentication, TLS or principal code is added. #938, #957, #958 and #959 remain unimplemented. Refs #931, #938. Addresses review 635. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/remote-mcp/threat-model-anchors.json | 4 +- docs/remote-mcp/threat-model.md | 8 +- gitea_mcp_server.py | 12 +- mcp_daemon_guard.py | 71 ++++ mcp_transport_config.py | 138 ++++++++ tests/test_issue_931_transport_bind_seam.py | 359 +++++++++++++++++++- 6 files changed, 567 insertions(+), 25 deletions(-) diff --git a/docs/remote-mcp/threat-model-anchors.json b/docs/remote-mcp/threat-model-anchors.json index 3fe4e3a..c20c28d 100644 --- a/docs/remote-mcp/threat-model-anchors.json +++ b/docs/remote-mcp/threat-model-anchors.json @@ -12,7 +12,7 @@ "anchors": [ {"anchor": "gitea_mcp_server.py:24728", "expect": "mcp_daemon_guard.bind_native_mcp_transport()"}, {"anchor": "mcp_daemon_guard.py:49", "expect": "_PRODUCTION_TRANSPORTS = mcp_transport_config.SUPPORTED_TRANSPORTS"}, - {"anchor": "mcp_daemon_guard.py:178", "expect": "def bind_native_mcp_transport"}, + {"anchor": "mcp_daemon_guard.py:195", "expect": "def bind_native_mcp_transport"}, {"anchor": "irrecoverable_provenance.py:497", "expect": "def assess_transport_for_auth_mint"}, {"anchor": "gitea_mcp_server.py:9132", "expect": "assess_transport_for_auth_mint()"}, {"anchor": "gitea_mcp_server.py:9381", "expect": "assess_transport_for_auth_mint()"}, @@ -36,7 +36,7 @@ {"anchor": "gitea_config.py:974", "expect": "def resolve_token"}, {"anchor": "gitea_config.py:1015", "expect": "def keychain_auth"}, {"anchor": "gitea_config.py:294", "expect": "def _validate_identity_auth"}, - {"anchor": "mcp_daemon_guard.py:519", "expect": "def assert_keychain_access_allowed"}, + {"anchor": "mcp_daemon_guard.py:583", "expect": "def assert_keychain_access_allowed"}, {"anchor": "gitea_mcp_server.py:19261", "expect": "def gitea_list_profiles"}, {"anchor": "gitea_mcp_server.py:19312", "expect": "gitea_config.resolve_token(p)"}, {"anchor": "gitea_mcp_server.py:19555", "expect": "def gitea_audit_config"}, diff --git a/docs/remote-mcp/threat-model.md b/docs/remote-mcp/threat-model.md index 4662e4d..e4ce636 100644 --- a/docs/remote-mcp/threat-model.md +++ b/docs/remote-mcp/threat-model.md @@ -77,9 +77,9 @@ authenticate the *caller*, not the *intent*. | ID | Boundary | Protects | Crossing requires today | Crossing must require remotely | | -- | -------- | -------- | ----------------------- | ------------------------------ | -| B1 | LLM client ↔ MCP server session | A1, A3, A10 — that a mutating session was established through the sanctioned client path | A single configured bind (`gitea_mcp_server.py:24728`) validated against one closed allowlist (`mcp_daemon_guard.py:49`, `mcp_daemon_guard.py:178`) — since #931 the identifier comes from deployment configuration and defaults to the local transport, so the boundary no longer rests on a literal, but it still rests on the *bind* rather than on an authenticated caller; client-managed provenance (`gitea_mcp_server.py:15415`) or a refusal (`gitea_mcp_server.py:15453`); production transport before recovery-authorization mint (`irrecoverable_provenance.py:497`, consumed at `gitea_mcp_server.py:9132` and `gitea_mcp_server.py:9381`) | An authenticated handshake issuing a server-side session identity bound to a principal, with the transport recorded in provenance. The physical proof (a pipe) must become a cryptographic one. | +| B1 | LLM client ↔ MCP server session | A1, A3, A10 — that a mutating session was established through the sanctioned client path | A single configured bind (`gitea_mcp_server.py:24728`) validated against one closed allowlist (`mcp_daemon_guard.py:49`, `mcp_daemon_guard.py:195`) — since #931 the identifier comes from deployment configuration and defaults to the local transport, so the boundary no longer rests on a literal, but it still rests on the *bind* rather than on an authenticated caller; client-managed provenance (`gitea_mcp_server.py:15415`) or a refusal (`gitea_mcp_server.py:15453`); production transport before recovery-authorization mint (`irrecoverable_provenance.py:497`, consumed at `gitea_mcp_server.py:9132` and `gitea_mcp_server.py:9381`) | An authenticated handshake issuing a server-side session identity bound to a principal, with the transport recorded in provenance. The physical proof (a pipe) must become a cryptographic one. | | B2 | Role ↔ role | A9 — that author, reviewer, merger, and reconciler are distinct authorities | **The process boundary only.** The role is a property of the process, read once from `GITEA_MCP_PROFILE` (`gitea_config.py:54`). A caller gets author permissions by connecting to the author process. Review and merge are the operations singled out for extra care (`gitea_config.py:97`) | A per-request principal, so the role follows from the credential presented and cannot be selected by reaching a different endpoint. | -| B3 | MCP server ↔ credential store | A3, A8 — that only sanctioned code turns a profile into a token | `_keychain_token` shelling out to the login keychain (`gitea_config.py:956`), dispatched by `resolve_token` (`gitea_config.py:974`) with the reference type built at `gitea_config.py:1015`, gated by `assert_keychain_access_allowed` (`mcp_daemon_guard.py:519`). Inline secrets are rejected at config load (`gitea_config.py:294`) | A credential provider keyed by the *request* principal, returning only that principal's credential, with the source recorded and the value never returned. | +| B3 | MCP server ↔ credential store | A3, A8 — that only sanctioned code turns a profile into a token | `_keychain_token` shelling out to the login keychain (`gitea_config.py:956`), dispatched by `resolve_token` (`gitea_config.py:974`) with the reference type built at `gitea_config.py:1015`, gated by `assert_keychain_access_allowed` (`mcp_daemon_guard.py:583`). Inline secrets are rejected at config load (`gitea_config.py:294`) | A credential provider keyed by the *request* principal, returning only that principal's credential, with the source recorded and the value never returned. | | B4 | MCP server ↔ Gitea | A1, A2 — that only authorized calls reach the forge | A bearer token over TLS. Server-side, nothing distinguishes one role's token from another beyond the account it belongs to | Unchanged at the forge; the endpoint in front of it must refuse unauthenticated and plaintext connections before tool dispatch. | | B5 | MCP server ↔ caller's filesystem | A7 — that a tool acts on the *caller's* disk or refuses | Nothing. The server's disk *is* the caller's disk. Worktree bootstrap writes directly (`gitea_mcp_server.py:10897`); the active workspace is process-global (`gitea_mcp_server.py:193`, `gitea_mcp_server.py:194`) | An explicit per-tool classification, enforced at dispatch, refusing filesystem tools over a transport that cannot reach the caller's disk. A green verdict about the wrong disk is the failure to prevent. | | B6 | MCP server ↔ coordination state | A6, A9 — mutual exclusion | Local files and a local SQLite database, with liveness judged from the local process table (`issue_lock_store.py:98`), keyed on paths under one user's home (`issue_lock_store.py:26`, `mcp_session_state.py:27`, `control_plane_db.py:47`) and on `os.getpid()` (`control_plane_db.py:1145`, `gitea_mcp_server.py:12804`). A legacy global slot still exists at `gitea_mcp_server.py:2351`, and the session-pointer file is named per PID (`issue_lock_store.py:83`) | One authority per ownership question, with liveness from session identity and expiry, and atomic acquire, renew, and release across hosts. | @@ -158,7 +158,7 @@ an attacker holding a token calls the API, not our tools. | CR10 | MDCPS GlitchTip read credential | macOS keychain, read from the Gitea server process (`gitea_config.py:851`) | B7 | Read error events and their payloads (A5). Enabled today. | | CR11 | `SENTRY_AUTH_TOKEN` | Process environment, read in-process (`sentry_incident_bridge.py:36`, `sentry_incident_bridge.py:190`), sent as a bearer header (`sentry_incident_bridge.py:289`) | B7 | Read and reconcile Sentry issues (A5). Not a keychain credential — an env var, so it is inherited by anything the process spawns. | | CR12 | `SENTRY_DSN` | Process environment (`sentry_observability.py:55`) | B7 | Write events into the observability project. Low read value, real forgery value: an attacker can inject fabricated events into the record (A10). | -| CR13 | macOS login keychain access | The operator's login session; gated by `assert_keychain_access_allowed` (`mcp_daemon_guard.py:519`) | B3, ADV5 | **Every other credential in this table except CR11 and CR12.** This is the aggregation point. | +| CR13 | macOS login keychain access | The operator's login session; gated by `assert_keychain_access_allowed` (`mcp_daemon_guard.py:583`) | B3, ADV5 | **Every other credential in this table except CR11 and CR12.** This is the aggregation point. | | CR14 | Coordination-store access (no secret) | Filesystem permissions — `control_plane_db.py:47`, created `0o700` (`control_plane_db.py:380`), opened with a local file lock (`control_plane_db.py:386`) | B6, ADV5 | Full read/write of locks, leases, and decision records (A6). **There is no credential here at all** — anything running as the operator can rewrite ownership. | ### Findings @@ -336,7 +336,7 @@ The client is attached to the local fleet over stdio. | -------- | ----------------- | ---------- | | B1 | Everything the fleet serves. The client *is* the sanctioned launcher: it satisfies the client-managed check (`gitea_mcp_server.py:15415`) by construction, and provenance is never re-verified after launch (Finding 8). | Nothing. The guard authenticates the launch, not the caller. | | B2 | All five roles — it is attached to all five namespaces. It can author a PR, approve it from the reviewer namespace, and merge it from the merger namespace. | Only the in-process self-review check, which compares `jcwalker3` (author) against `sysadmin` (reviewer) and **passes**, because Finding 1 made them different accounts while leaving reviewer and merger identical. A9 falls in one sequence of legitimate calls. | -| B3 | Every credential in CR1–CR10 via CR13, with no additional prompt — the daemon is already sanctioned, so `assert_keychain_access_allowed` (`mcp_daemon_guard.py:519`) returns immediately. | Nothing. | +| B3 | Every credential in CR1–CR10 via CR13, with no additional prompt — the daemon is already sanctioned, so `assert_keychain_access_allowed` (`mcp_daemon_guard.py:583`) returns immediately. | Nothing. | | B4 | A1 and A2 in full. | Branch protection at the forge, to the extent configured. | | B5 | The operator's checkout and every worktree, through the author tools (`gitea_mcp_server.py:10897`), plus the shared stderr path at `mcp_server.py:13`. | Nothing; the server's disk is the target disk. | | B6 | All coordination state — no credential required (CR14). It can forge lease ownership and clear decision locks. | Filesystem permissions, which it already satisfies. | diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index c50c3a8..b285870 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -24743,7 +24743,11 @@ if __name__ == "__main__": sys.stderr.write( f"--- Sentry observability: {_sentry_status.get('reason')} ---\n" ) - # #931: serve over exactly the transport that was bound and pinned, read - # back through the one shared accessor. An absent bind fails closed here - # rather than serving tools over a transport no guard can name. - mcp.run(transport=mcp_daemon_guard.assert_transport_bound("tool service")) + # #931: serve over exactly the transport that was bound and pinned, and only + # if this entrypoint is commissioned to execute it. Recognition is not + # execution authorization (review 635): a registered remote identifier is + # bound, pinned and recorded, but serving it would start a listener with no + # authentication or per-request principal, which #938 owns. An absent bind + # keeps its pre-existing #695 failure; an uncommissioned transport fails + # closed here, before any listener exists and before any tool can dispatch. + mcp.run(transport=mcp_daemon_guard.authorize_transport_execution("tool service")) diff --git a/mcp_daemon_guard.py b/mcp_daemon_guard.py index 7524db7..99e29c8 100644 --- a/mcp_daemon_guard.py +++ b/mcp_daemon_guard.py @@ -62,6 +62,23 @@ class UnsanctionedRuntimeError(RuntimeError): """Raised when mutation/credential code runs outside a native MCP daemon.""" +class TransportExecutionError(UnsanctionedRuntimeError): + """Raised when a bound transport may not be served by this entrypoint (#931). + + Subclasses :class:`UnsanctionedRuntimeError` so every existing fail-closed + handler still catches it, while letting a caller that cares distinguish + "nothing is bound" from "something valid is bound but its listener has not + been commissioned". Carries the structured verdict on ``.assessment``. + """ + + def __init__(self, message: str, assessment: dict[str, Any] | None = None): + super().__init__(message) + self.assessment = assessment or {} + self.blocker_kind = self.assessment.get("blocker_kind") + self.owner_issue = self.assessment.get("owner_issue") + self.transport = self.assessment.get("transport") + + def is_pytest_runtime() -> bool: if (os.environ.get(FORCE_PROVENANCE_FAIL_ENV) or "").strip() in { "1", @@ -432,6 +449,53 @@ def assert_transport_bound(context: str = "tool service") -> str: ) +def assess_serve_authorization() -> dict[str, Any]: + """Structured verdict on whether this process may serve tools (#931). + + This is the decision that consumes :func:`bound_transport`. It is what stops + the bound identifier from being reporting-only metadata: the serve path + cannot proceed unless the value pinned at bind is one this entrypoint is + commissioned to execute. + + Never raises; returns the verdict so callers and diagnostics can inspect it. + """ + return mcp_transport_config.assess_transport_execution(bound_transport()) + + +def authorize_transport_execution(context: str = "tool service") -> str: + """Return the transport this process may serve, or fail closed (#931). + + Two distinct boundaries, in order: + + 1. **Bind presence** — :func:`assert_transport_bound` enforces the + pre-existing #695 contract, so an unbound runtime keeps its established + failure and reason code. + 2. **Execution authorization** — the bound identifier must be one this + entrypoint is commissioned to serve. A registered transport whose + listener has not been commissioned is refused here, before any listener + is created and before any tool can dispatch. + + That ordering matters: recognition, validation and durable recording all + still happen for a remote identifier, so #931's seam is intact; only the act + of *serving* it is withheld until its owning issue commissions it. + """ + # Boundary 1: unbound stays exactly as fail-closed as it was under #695. + assert_transport_bound(context) + + # Boundary 2: bound, but is this entrypoint allowed to serve it? + assessment = assess_serve_authorization() + if assessment.get("allowed"): + return str(assessment["transport"]) + + reasons = "; ".join(assessment.get("reasons") or []) or "not authorized" + next_action = assessment.get("exact_next_action") or "" + raise TransportExecutionError( + f"Refusing {context} (#931) [{assessment.get('blocker_kind')}]: " + f"{reasons} {next_action}".strip(), + assessment, + ) + + def is_sanctioned_mcp_daemon() -> bool: """Backward-compatible name; #695 requires native transport, not env alone.""" if is_production_native_mcp_transport(): @@ -563,6 +627,13 @@ def native_runtime_status() -> dict[str, Any]: "default_transport": mcp_transport_config.DEFAULT_TRANSPORT, "supported_transports": list(mcp_transport_config.supported_transports()), "transport_env": mcp_transport_config.TRANSPORT_ENV, + # #931 review 635: recognition and execution authorization are distinct. + # ``supported`` is what may be bound; ``executable`` is what this + # entrypoint may actually serve. A recognized-but-uncommissioned + # transport reports serve_authorized False with a named blocker. + "executable_transports": list(mcp_transport_config.executable_transports()), + "serve_authorized": bool(assess_serve_authorization().get("allowed")), + "serve_authorization": assess_serve_authorization(), "mode": rt.get("mode"), "session_state_dir": pinned_session_state_dir() or rt.get("session_state_dir"), "session_state_dir_pinned": pinned_session_state_dir() is not None, diff --git a/mcp_transport_config.py b/mcp_transport_config.py index 44b6157..fd230d2 100644 --- a/mcp_transport_config.py +++ b/mcp_transport_config.py @@ -53,6 +53,30 @@ REMOTE_TRANSPORT = "streamable-http" # bind time, and ``sse`` is held to that rule like any other. SUPPORTED_TRANSPORTS = frozenset({DEFAULT_TRANSPORT, REMOTE_TRANSPORT}) +# Recognition is not execution authorization (#931, review 635 B1/B2). +# +# SUPPORTED_TRANSPORTS answers "is this an identifier this system knows, and may +# it be bound, pinned and recorded?". It deliberately includes the remote +# identifier, because #931 requires the bind to become pluggable. +# +# EXECUTABLE_TRANSPORTS answers a strictly narrower question: "is this entrypoint +# commissioned to actually *serve* on that transport?". Only the local transport +# is. Handing ``streamable-http`` to ``mcp.run`` would start FastMCP's HTTP +# listener with no authentication, no TLS and no per-request principal — the +# endpoint #938 owns and gates. Recognition must therefore never imply execution. +# +# #938 commissions the remote listener by adding REMOTE_TRANSPORT here, together +# with the authentication and principal boundary its acceptance criteria require. +EXECUTABLE_TRANSPORTS = frozenset({DEFAULT_TRANSPORT}) + +# Which issue owns commissioning each recognized-but-not-executable transport. +# Used to make the refusal actionable rather than a generic denial. +TRANSPORT_EXECUTION_OWNER = {REMOTE_TRANSPORT: "#938"} + +BLOCKER_TRANSPORT_NOT_BOUND = "transport_not_bound" +BLOCKER_TRANSPORT_NOT_RECOGNIZED = "transport_not_recognized" +BLOCKER_LISTENER_NOT_COMMISSIONED = "transport_listener_not_commissioned" + SOURCE_CONFIGURED = "deployment_configuration" SOURCE_DEFAULT = "default" @@ -89,6 +113,120 @@ def is_remote_transport(value: Any) -> bool: return name in SUPPORTED_TRANSPORTS and name != DEFAULT_TRANSPORT +def executable_transports() -> tuple[str, ...]: + """Transports this entrypoint is commissioned to serve, sorted.""" + return tuple(sorted(EXECUTABLE_TRANSPORTS)) + + +def is_executable_transport(value: Any) -> bool: + """True when *value* may actually be served by this entrypoint (#931). + + Strictly narrower than :func:`is_supported_transport`. A recognized + identifier that is not executable is a correct, fully-bound configuration + whose listener has simply not been commissioned yet. + """ + return normalize_transport(value) in EXECUTABLE_TRANSPORTS + + +def assess_transport_execution(value: Any) -> dict[str, Any]: + """Structured serve-authorization verdict for a bound transport (#931). + + This is the decision that separates a *recognized* transport from one this + entrypoint may execute. It is deliberately a pure function of the bound + identifier so the serve path cannot reach a listener the deployment has not + commissioned. + + Args: + value: The bound transport identifier, or ``None`` when unbound. + + Returns: + dict with ``transport``, ``recognized``, ``executable``, ``allowed``, + ``blocker_kind``, ``owner_issue``, ``reasons`` and + ``exact_next_action``. ``allowed`` is true only for a bound, recognized, + commissioned transport. + """ + name = normalize_transport(value) + + if not name: + return { + "transport": None, + "recognized": False, + "executable": False, + "allowed": False, + "blocker_kind": BLOCKER_TRANSPORT_NOT_BOUND, + "owner_issue": None, + "supported_transports": list(supported_transports()), + "executable_transports": list(executable_transports()), + "reasons": [ + "no transport is bound; the serve path is fail-closed until " + "bind_native_mcp_transport succeeds (#695/#931)" + ], + "exact_next_action": ( + "Launch through the canonical entrypoint so " + "bind_native_mcp_transport runs before tool service." + ), + } + + recognized = name in SUPPORTED_TRANSPORTS + if not recognized: + return { + "transport": name, + "recognized": False, + "executable": False, + "allowed": False, + "blocker_kind": BLOCKER_TRANSPORT_NOT_RECOGNIZED, + "owner_issue": None, + "supported_transports": list(supported_transports()), + "executable_transports": list(executable_transports()), + "reasons": [ + f"transport {name!r} is not a registered MCP transport (#931); " + "it should have been refused at bind time" + ], + "exact_next_action": ( + f"Set {TRANSPORT_ENV} to one of {list(supported_transports())}." + ), + } + + if name in EXECUTABLE_TRANSPORTS: + return { + "transport": name, + "recognized": True, + "executable": True, + "allowed": True, + "blocker_kind": None, + "owner_issue": None, + "supported_transports": list(supported_transports()), + "executable_transports": list(executable_transports()), + "reasons": [], + "exact_next_action": None, + } + + owner = TRANSPORT_EXECUTION_OWNER.get(name) + owner_text = owner or "the issue that commissions this transport's listener" + return { + "transport": name, + "recognized": True, + "executable": False, + "allowed": False, + "blocker_kind": BLOCKER_LISTENER_NOT_COMMISSIONED, + "owner_issue": owner, + "supported_transports": list(supported_transports()), + "executable_transports": list(executable_transports()), + "reasons": [ + f"transport {name!r} is registered and was bound and recorded, but " + f"this entrypoint is not commissioned to serve it (#931). Serving it " + f"would start a listener with no authentication, no transport " + f"security and no per-request principal; that endpoint is owned by " + f"{owner_text}." + ], + "exact_next_action": ( + f"Serve on {DEFAULT_TRANSPORT} until {owner_text} commissions the " + f"{name!r} listener with its authentication and principal boundary, " + f"which adds {name!r} to EXECUTABLE_TRANSPORTS." + ), + } + + def resolve_configured_transport( env: Mapping[str, str] | None = None, ) -> dict[str, Any]: diff --git a/tests/test_issue_931_transport_bind_seam.py b/tests/test_issue_931_transport_bind_seam.py index 2186284..f28e4e5 100644 --- a/tests/test_issue_931_transport_bind_seam.py +++ b/tests/test_issue_931_transport_bind_seam.py @@ -110,21 +110,32 @@ class TestPermittedSetIsSingleSourceOfTruth(unittest.TestCase): ) def test_no_default_transport_literal_outside_the_seam(self): - """AC3: no guard reads the literal outside the seam definition.""" + """AC3: no production module reads the literal outside the seam. + + Review 635 flagged that a fixed five-module list cannot catch a *new* + module reintroducing the literal. This globs every production module in + the repository root instead, so the guarantee holds for code that does + not exist yet. + """ default = mcp_transport_config.DEFAULT_TRANSPORT needles = (f'"{default}"', f"'{default}'") + seam = Path(mcp_transport_config.__file__).name + scanned: list[str] = [] offenders: list[str] = [] - for name in ( - "mcp_daemon_guard.py", - "gitea_mcp_server.py", - "irrecoverable_provenance.py", - "mcp_session_state.py", - "review_quarantine.py", - ): - text = (REPO_ROOT / name).read_text(encoding="utf-8") - for lineno, line in enumerate(text.splitlines(), start=1): - if any(needle in line for needle in needles): - offenders.append(f"{name}:{lineno}: {line.strip()}") + for path in sorted(REPO_ROOT.glob("*.py")): + if path.name == seam: + continue # the seam is the one place the literal may live + scanned.append(path.name) + for lineno, line in enumerate( + path.read_text(encoding="utf-8").splitlines(), start=1 + ): + code = line.split("#", 1)[0] + if any(needle in code for needle in needles): + offenders.append(f"{path.name}:{lineno}: {line.strip()}") + # Guard the guard: a glob that silently matched nothing would pass. + self.assertGreater(len(scanned), 20, "production glob matched too little") + self.assertIn("mcp_daemon_guard.py", scanned) + self.assertIn("gitea_mcp_server.py", scanned) self.assertEqual(offenders, [], "\n".join(offenders)) @@ -577,18 +588,336 @@ class TestStdioBehaviourUnchanged(unittest.TestCase): self.assertIsNone(verdict["bound_transport"]) +class TestExecutionBoundary(unittest.TestCase): + """Recognition is not execution authorization (#931, review 635 B1/B2). + + A registered remote identifier must still bind, pin and record — the #931 + seam — while being refused at the serve boundary, because serving it would + start a listener with no authentication or per-request principal. That + listener belongs to #938. + """ + + def tearDown(self) -> None: + mcp_daemon_guard.clear_native_runtime_for_tests() + + # -- the two sets are distinct, and narrower in the right direction ---- + + def test_executable_set_is_a_strict_subset_of_recognized(self): + self.assertTrue( + mcp_transport_config.EXECUTABLE_TRANSPORTS + < mcp_transport_config.SUPPORTED_TRANSPORTS + ) + + def test_default_transport_is_executable(self): + self.assertTrue( + mcp_transport_config.is_executable_transport( + mcp_transport_config.DEFAULT_TRANSPORT + ) + ) + + def test_remote_transport_is_recognized_but_not_executable(self): + remote = mcp_transport_config.REMOTE_TRANSPORT + self.assertTrue(mcp_transport_config.is_supported_transport(remote)) + self.assertFalse(mcp_transport_config.is_executable_transport(remote)) + + def test_remote_listener_ownership_is_declared(self): + self.assertEqual( + mcp_transport_config.TRANSPORT_EXECUTION_OWNER[ + mcp_transport_config.REMOTE_TRANSPORT + ], + "#938", + ) + + # -- stdio still reaches the runner, unchanged ------------------------- + + def test_default_transport_is_authorized_for_service(self): + with _ProductionBind() as guard: + guard.bind_native_mcp_transport() + self.assertEqual( + guard.authorize_transport_execution("tool service"), + mcp_transport_config.DEFAULT_TRANSPORT, + ) + self.assertTrue(guard.assess_serve_authorization()["allowed"]) + + def test_default_transport_reaches_the_real_runner(self): + """The production runner is actually invoked, with stdio, unchanged.""" + with _ProductionBind() as guard: + guard.bind_native_mcp_transport() + seen = {} + + class _Runner: + def run(self, transport=None, **kw): + seen["transport"] = transport + + _Runner().run(transport=guard.authorize_transport_execution("tool service")) + self.assertEqual( + seen["transport"], mcp_transport_config.DEFAULT_TRANSPORT + ) + + # -- streamable-http binds, records, and is refused before serving ----- + + def test_remote_transport_binds_and_is_recorded(self): + """The #931 seam is intact: it binds, pins and records.""" + with _ProductionBind( + { + mcp_transport_config.TRANSPORT_ENV: ( + mcp_transport_config.REMOTE_TRANSPORT + ) + } + ) as guard: + guard.bind_native_mcp_transport() + self.assertEqual( + guard.bound_transport(), mcp_transport_config.REMOTE_TRANSPORT + ) + self.assertEqual( + guard.mutation_provenance_fields()["bound_transport"], + mcp_transport_config.REMOTE_TRANSPORT, + ) + + def test_remote_transport_is_refused_at_the_serve_boundary(self): + with _ProductionBind( + { + mcp_transport_config.TRANSPORT_ENV: ( + mcp_transport_config.REMOTE_TRANSPORT + ) + } + ) as guard: + guard.bind_native_mcp_transport() + with self.assertRaises(guard.TransportExecutionError) as ctx: + guard.authorize_transport_execution("tool service") + err = ctx.exception + self.assertEqual( + err.blocker_kind, + mcp_transport_config.BLOCKER_LISTENER_NOT_COMMISSIONED, + ) + self.assertEqual(err.owner_issue, "#938") + self.assertEqual(err.transport, mcp_transport_config.REMOTE_TRANSPORT) + + def test_refusal_names_transport_and_unmet_requirement_and_owner(self): + with _ProductionBind( + { + mcp_transport_config.TRANSPORT_ENV: ( + mcp_transport_config.REMOTE_TRANSPORT + ) + } + ) as guard: + guard.bind_native_mcp_transport() + with self.assertRaises(guard.TransportExecutionError) as ctx: + guard.authorize_transport_execution("tool service") + text = str(ctx.exception) + self.assertIn(mcp_transport_config.REMOTE_TRANSPORT, text) + self.assertIn("#938", text) + self.assertIn("not commissioned", text) + self.assertIn("#931", text) + + def test_remote_transport_never_reaches_the_runner(self): + """No transport value is handed to a run() call for the remote case.""" + with _ProductionBind( + { + mcp_transport_config.TRANSPORT_ENV: ( + mcp_transport_config.REMOTE_TRANSPORT + ) + } + ) as guard: + guard.bind_native_mcp_transport() + calls = [] + + class _Runner: + def run(self, transport=None, **kw): + calls.append(transport) + + with self.assertRaises(guard.TransportExecutionError): + _Runner().run( + transport=guard.authorize_transport_execution("tool service") + ) + self.assertEqual(calls, [], "runner must never be invoked") + + def test_no_http_listener_is_created_for_remote_transport(self): + """Nothing in the refusal path touches uvicorn or a socket bind.""" + with _ProductionBind( + { + mcp_transport_config.TRANSPORT_ENV: ( + mcp_transport_config.REMOTE_TRANSPORT + ) + } + ) as guard: + guard.bind_native_mcp_transport() + import socket + + bound_sockets = [] + real_bind = socket.socket.bind + + def _tripwire(self, addr): # pragma: no cover - must not run + bound_sockets.append(addr) + return real_bind(self, addr) + + with patch.object(socket.socket, "bind", _tripwire): + with self.assertRaises(guard.TransportExecutionError): + guard.authorize_transport_execution("tool service") + self.assertEqual(bound_sockets, [], "no socket may be bound") + + def test_no_mutation_is_authorized_after_the_denial(self): + """The denial leaves no partial state that would let a tool dispatch.""" + with _ProductionBind( + { + mcp_transport_config.TRANSPORT_ENV: ( + mcp_transport_config.REMOTE_TRANSPORT + ) + } + ) as guard: + guard.bind_native_mcp_transport() + with self.assertRaises(guard.TransportExecutionError): + guard.authorize_transport_execution("tool service") + # Serve stays unauthorized on every subsequent query. + self.assertFalse(guard.assess_serve_authorization()["allowed"]) + self.assertFalse(guard.native_runtime_status()["serve_authorized"]) + with self.assertRaises(guard.TransportExecutionError): + guard.authorize_transport_execution("tool service") + + # -- B2: the decision genuinely consumes bound_transport --------------- + + def test_serve_decision_consumes_bound_transport(self): + """Changing only bound_transport flips the verdict.""" + with _ProductionBind() as guard: + guard.bind_native_mcp_transport() + self.assertTrue(guard.assess_serve_authorization()["allowed"]) + with patch.object( + guard, + "bound_transport", + return_value=mcp_transport_config.REMOTE_TRANSPORT, + ): + verdict = guard.assess_serve_authorization() + self.assertFalse(verdict["allowed"]) + self.assertEqual( + verdict["transport"], mcp_transport_config.REMOTE_TRANSPORT + ) + with self.assertRaises(guard.TransportExecutionError): + guard.authorize_transport_execution("tool service") + + def test_serve_verdict_reports_the_bound_transport(self): + with _ProductionBind( + { + mcp_transport_config.TRANSPORT_ENV: ( + mcp_transport_config.REMOTE_TRANSPORT + ) + } + ) as guard: + guard.bind_native_mcp_transport() + verdict = guard.assess_serve_authorization() + self.assertEqual( + verdict["transport"], mcp_transport_config.REMOTE_TRANSPORT + ) + self.assertTrue(verdict["recognized"]) + self.assertFalse(verdict["executable"]) + + # -- earlier and later boundaries are unchanged ------------------------ + + def test_unregistered_identifier_still_fails_at_bind_not_at_serve(self): + with _ProductionBind( + {mcp_transport_config.TRANSPORT_ENV: "carrier-pigeon"} + ) as guard: + with self.assertRaises(guard.UnsanctionedRuntimeError) as ctx: + guard.bind_native_mcp_transport() + self.assertIn("not a registered MCP transport", str(ctx.exception)) + self.assertIsNone(guard.bound_transport()) + + def test_unbound_execution_keeps_the_pre_existing_failure(self): + with _ProductionBind() as guard: + with self.assertRaises(guard.UnsanctionedRuntimeError) as ctx: + guard.authorize_transport_execution("tool service") + self.assertIn("No MCP transport is bound", str(ctx.exception)) + self.assertNotIsInstance(ctx.exception, guard.TransportExecutionError) + + def test_transport_execution_error_is_caught_by_existing_handlers(self): + """Subclassing keeps every pre-existing fail-closed handler correct.""" + self.assertTrue( + issubclass( + mcp_daemon_guard.TransportExecutionError, + mcp_daemon_guard.UnsanctionedRuntimeError, + ) + ) + + def test_test_mode_runtime_is_not_servable(self): + """The pytest-only record is outside the recognized and executable sets.""" + mcp_daemon_guard.clear_native_runtime_for_tests() + mcp_daemon_guard.install_test_native_runtime() + self.assertNotIn( + mcp_daemon_guard.bound_transport(), + mcp_transport_config.SUPPORTED_TRANSPORTS, + ) + self.assertFalse(mcp_daemon_guard.assess_serve_authorization()["allowed"]) + + def test_serve_authorization_does_not_leak_across_runtimes(self): + """A later runtime's verdict never reflects an earlier one.""" + with _ProductionBind( + { + mcp_transport_config.TRANSPORT_ENV: ( + mcp_transport_config.REMOTE_TRANSPORT + ) + } + ) as guard: + guard.bind_native_mcp_transport() + self.assertFalse(guard.assess_serve_authorization()["allowed"]) + with _ProductionBind() as guard: + guard.bind_native_mcp_transport() + self.assertTrue(guard.assess_serve_authorization()["allowed"]) + mcp_daemon_guard.clear_native_runtime_for_tests() + self.assertEqual( + mcp_daemon_guard.assess_serve_authorization()["blocker_kind"], + mcp_transport_config.BLOCKER_TRANSPORT_NOT_BOUND, + ) + + def test_refusal_carries_no_credential_material(self): + with _ProductionBind( + { + mcp_transport_config.TRANSPORT_ENV: ( + mcp_transport_config.REMOTE_TRANSPORT + ), + "GITEA_TOKEN": "super-secret-value", + } + ) as guard: + guard.bind_native_mcp_transport() + with self.assertRaises(guard.TransportExecutionError) as ctx: + guard.authorize_transport_execution("tool service") + blob = str(ctx.exception) + repr(ctx.exception.assessment) + self.assertNotIn("super-secret-value", blob) + self.assertNotIn("GITEA_TOKEN", blob) + + class TestEntrypointWiring(unittest.TestCase): - """The live entrypoint must use the seam, not a literal.""" + """The live entrypoint must use the seam and the execution guard.""" def test_entrypoint_binds_without_a_literal_transport(self): text = (REPO_ROOT / "gitea_mcp_server.py").read_text(encoding="utf-8") self.assertIn("mcp_daemon_guard.bind_native_mcp_transport()", text) self.assertNotIn('bind_native_mcp_transport(transport="stdio")', text) - def test_entrypoint_serves_the_bound_transport(self): + def test_entrypoint_serves_only_through_the_execution_guard(self): text = (REPO_ROOT / "gitea_mcp_server.py").read_text(encoding="utf-8") - self.assertIn("mcp.run(transport=mcp_daemon_guard.assert_transport_bound", text) + self.assertIn( + "mcp.run(transport=mcp_daemon_guard.authorize_transport_execution", text + ) self.assertNotIn('mcp.run(transport="stdio")', text) + self.assertNotIn( + "mcp.run(transport=mcp_daemon_guard.assert_transport_bound", text + ) + + def test_every_run_call_in_production_goes_through_the_guard(self): + """Glob the production surface: no serve site may bypass the guard.""" + offenders: list[str] = [] + run_sites = 0 + for path in sorted(REPO_ROOT.glob("*.py")): + for lineno, line in enumerate( + path.read_text(encoding="utf-8").splitlines(), start=1 + ): + code = line.split("#", 1)[0] + if "mcp.run(" not in code: + continue + run_sites += 1 + if "authorize_transport_execution" not in code: + offenders.append(f"{path.name}:{lineno}: {line.strip()}") + self.assertEqual(run_sites, 1, "expected exactly one serve site") + self.assertEqual(offenders, [], "\n".join(offenders)) if __name__ == "__main__":