fix(mcp): an adopting profile keeps its own trust policy for a shared MCP connection
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 ceaf622c6d, 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.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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]] = {}
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user