From e609efb06aa3a9d117a605158e70ec0dc89e3159 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sun, 13 Sep 2026 12:35:50 -0700 Subject: [PATCH] fix(mcp): an adopting profile keeps its own trust policy for a shared MCP connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under gateway.multiplex_profiles a profile whose mcp_servers entry has the same route and credentials as another profile's live connection adopts that connection instead of opening its own. _same_server_route() compares only the connection identity, so a `trust: untrusted` profile adopted a `trust: full` profile's connection; _trust_gate_check() then resolved the OWNER's connection key, read `full`, and let the untrusted profile run write-capable tools without the approval prompt its config demands (review of #108352, finding A; regression from ceaf622c6d34, where the name-keyed ledger let the last registrant's trust win instead). `trust` is the consuming profile's policy, not a property of the connection: _server_trust_levels is now keyed by the calling profile's own key (recorded at its own registration and at adoption, dropped when its overlay is removed), while readOnlyHint stays under the connection key because it describes the server's tools. Sharing the connection is still allowed — only the gate is per profile. --- .../test_mcp_multiplex_connection_keys.py | 30 ++++++++++++++++++- tools/mcp_tool.py | 4 ++- tools/mcp_tool_handlers.py | 10 ++++--- tools/mcp_tool_registration.py | 21 ++++++++++--- 4 files changed, 55 insertions(+), 10 deletions(-) diff --git a/tests/tools/test_mcp_multiplex_connection_keys.py b/tests/tools/test_mcp_multiplex_connection_keys.py index b77699635e..b7f80a13b9 100644 --- a/tests/tools/test_mcp_multiplex_connection_keys.py +++ b/tests/tools/test_mcp_multiplex_connection_keys.py @@ -38,7 +38,8 @@ def two_profiles(tmp_path, monkeypatch): ledgers = ("_servers", "_server_scope_keys", "_server_tool_scopes", "_server_connecting", "_server_connect_errors", "_server_connect_retry_after", "_server_connect_failures", "_server_error_counts", "_server_breaker_opened_at", "_lazy_server_configs", - "_mcp_tool_server_names", "_orphaned_adopters") + "_mcp_tool_server_names", "_orphaned_adopters", "_parallel_safe_servers", + "_server_trust_levels", "_tool_read_only_hints") saved = {n: type(getattr(core, n))(getattr(core, n)) for n in ledgers} for n in ledgers: getattr(core, n).clear() @@ -192,3 +193,30 @@ def test_owner_reload_reregisters_profiles_that_adopted_its_connection(two_profi two_profiles("b") assert registry.get_tool_names_for_toolset("mcp-x") == ["mcp__x__t"] assert disc.get_mcp_status({"x": cfg})[0]["status"] == "connected" + + +def test_untrusted_adopter_of_a_full_profiles_connection_keeps_its_own_trust_gate(two_profiles, monkeypatch): + """Trust is the consuming profile's policy: adopting A's ``trust: full`` connection must not let + B's ``trust: untrusted`` write-capable call skip approval.""" + from tools import mcp_tool_discovery as disc, mcp_tool_handlers as handlers + from tools import mcp_tool_registration as reg + import tools.approval_prompt as approval_prompt + + route = {"url": "https://mcp.example/x", "headers": {"Authorization": "Bearer shared"}} + cfg_a, cfg_b = dict(route, trust="full"), dict(route, trust="untrusted") + asked = [] + monkeypatch.setattr(approval_prompt, "request_elicitation_consent", + lambda *a, **k: asked.append(a) or "deny") + + two_profiles("a") + srv_a = _server("x", cfg_a) + disc._adopt_server("x", srv_a) + srv_a._registered_tool_names = reg._register_server_tools("x", srv_a, cfg_a) + + two_profiles("b") + assert reg.register_connected_into_current_scope({"x": cfg_b}) == 1 + assert handlers._trust_gate_check("x", "t") is not None and asked + + two_profiles("a") + assert handlers._trust_gate_check("x", "t") is None and len(asked) == 1 + diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 9868754f5e..c1ac8d83c2 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -468,7 +468,9 @@ _CIRCUIT_BREAKER_THRESHOLD, _CIRCUIT_BREAKER_COOLDOWN_SEC = 3, 60.0 # before the RPC fires. A lying readOnlyHint can only skip approval for calls the operator was # already warned about, never widen access. Missing trust = full; unrecognized = untrusted (a # typo must never disable the gate). Classified at CALL time from DISCOVERY data: no schema -# mutation, prompt cache intact. +# mutation, prompt cache intact. ``_server_trust_levels`` is keyed by the CONSUMING profile's own +# key (its policy for the name, even when it adopted another profile's connection); +# ``_tool_read_only_hints`` by the connection key (the server's own tool annotations). _server_trust_levels: Dict[Any, str] = {} _tool_read_only_hints: Dict[Any, Dict[str, bool]] = {} diff --git a/tools/mcp_tool_handlers.py b/tools/mcp_tool_handlers.py index 36ff99f4d6..e8df28d9a9 100644 --- a/tools/mcp_tool_handlers.py +++ b/tools/mcp_tool_handlers.py @@ -41,10 +41,12 @@ _STDIO_OUTCOME_UNCERTAIN_MSG = ( def _trust_gate_check(server_name: str, tool_name: str) -> Optional[str]: """Approval gate for write-capable tools on ``trust: untrusted`` servers. None to proceed, else a ``tool_error``. Fail-closed: approval-system errors block.""" - from tools.mcp_tool_scope import _resolve_server_key - key = _resolve_server_key(server_name) - if (_core._server_trust_levels.get(key, _core._TRUST_FULL) != _core._TRUST_UNTRUSTED - or _core._tool_read_only_hints.get(key, {}).get(tool_name) is True): + from tools.mcp_tool_scope import _resolve_server_key, _server_key + # Trust is the calling profile's own policy (an adopter of a shared connection keeps its own tier); + # readOnlyHint is a property of the connection's tools, so it lives under the connection key. + trust = _core._server_trust_levels.get(_server_key(server_name), _core._TRUST_FULL) + if (trust != _core._TRUST_UNTRUSTED + or _core._tool_read_only_hints.get(_resolve_server_key(server_name), {}).get(tool_name) is True): return None try: # lazy: tools.approval routes the prompt to whichever surface owns the session from tools.approval_prompt import request_elicitation_consent diff --git a/tools/mcp_tool_registration.py b/tools/mcp_tool_registration.py index a9a4d02280..a01d24c2c5 100644 --- a/tools/mcp_tool_registration.py +++ b/tools/mcp_tool_registration.py @@ -51,16 +51,27 @@ def _annotation_read_only_hint(mcp_tool: Any) -> bool: return hint is True -def _record_tool_trust_metadata(server_name: str, config: dict, tools: List[Any]) -> None: +def _record_tool_trust_metadata(server_name: str, config: dict, tools: List[Any], key=None) -> None: """Capture per-server trust and per-tool readOnlyHint at discovery — the security boundary: the call-time gate - classifies from data we control, never re-read server-supplied state.""" + classifies from data we control, never re-read server-supplied state. *key* is the connection (default: the + registering profile's own); the ``trust`` policy is recorded under it for the profile that owns it — an + adopting profile records its own policy in ``_record_scope_trust``.""" with _core._lock: - key = _resolve_server_key(server_name) + if key is None: + key = _server_key(server_name) _core._server_trust_levels[key] = _normalize_server_trust((config or {}).get("trust")) hints = _core._tool_read_only_hints.setdefault(key, {}) hints.update({t.name: _annotation_read_only_hint(t) for t in tools if getattr(t, "name", None)}) +def _record_scope_trust(server_name: str, config: dict, scope: str) -> None: + """``trust`` is the CONSUMING profile's policy, never the connection's: an ``untrusted`` profile that + adopts a ``full`` profile's live connection must still be asked before every write-capable call.""" + with _core._lock: + _core._server_trust_levels[_server_key(server_name, scope, current=False)] = _normalize_server_trust( + (config or {}).get("trust")) + + def _track_mcp_tool_server(tool_name: str, server_name: str) -> None: """Remember the exact raw MCP server that registered *tool_name*.""" with _core._lock: @@ -131,6 +142,7 @@ def _remove_server_scope(key, scope: str) -> None: _core._server_tool_scopes[key] = scopes else: _core._server_tool_scopes.pop(key, None) + _core._server_trust_levels.pop(_server_key(server_name, scope, current=False), None) _restore_server_toolset_alias(key) @@ -384,7 +396,7 @@ def _register_server_tools(name: str, server: "MCPServerTask", config: dict) -> ``toolsets.TOOLSETS``; lossy normalization collisions (``read-file``/``read_file``) fail closed.""" should_register = _make_tool_filter(name, config) key = _server_key_for_task(server) - _record_tool_trust_metadata(name, config, server._tools) + _record_tool_trust_metadata(name, config, server._tools, key) candidates = _tool_candidates(name, server._tools, should_register, server.tool_timeout) candidates += _utility_candidates(name, _select_utility_schemas(name, server, config), server.tool_timeout) registered = _register_candidates( @@ -485,6 +497,7 @@ def _register_connected_into_current_scope(servers: dict) -> int: # Visibility for this profile: the owner keeps teardown, this scope sees the connection. with _core._lock: _core._server_tool_scopes.setdefault(key, set()).add(scope) + _record_scope_trust(name, config, scope) if registry.get_tool_names_for_toolset(f"mcp-{name}"): continue candidates = _tool_candidates(name, server._tools, _make_tool_filter(name, config), server.tool_timeout)