From afd7917b8b39ca66e48321f4fe182914720617c6 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Wed, 15 Jul 2026 23:11:37 -0700 Subject: [PATCH] fix(mcp): separate issuer identity from anchoring so carry-forward keeps endpoints Making the in-memory issuer reflect a trust-on-first-use discovered value fixed the registry/row token-identity drift, but it overloaded a single field: the carry-forward gate keyed on issuer truthiness as a proxy for "endpoints are anchored to a pinned issuer, fail-closed". A discovered issuer is truthy yet not anchored, so a resource-rooted server that had learned its issuer would drop its last-known-good endpoints on a transient discovery blip instead of carrying them forward. Anchoring is now a first-class property rather than a proxy. MCPServer carries issuer_is_anchored, set at both build paths from the single _uses_issuer_anchor definition (a pinned issuer on a discovery auth type). issuer stays the identity value used by the token-identity tuple and the serializers; issuer_is_anchored is the provenance value the carry-forward gate reads to decide fail-closed. The two properties can no longer be conflated, so a discovered issuer keeps its resource-rooted endpoints carrying forward while a pinned issuer still fails closed. Regression tests pin both directions: a discovered-but-not-anchored server restores its endpoints on a discovery blip, an anchored server does not, and the build sets issuer_is_anchored true only when the issuer is pinned --- .../mcp_server/mcp_server_manager.py | 32 +++++++++--- .../types/mcp_server/mcp_server_manager.py | 1 + .../mcp_server/test_mcp_server_manager.py | 50 ++++++++++++++++++- 3 files changed, 74 insertions(+), 9 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 57cd755797b..115ff2e492c 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -201,6 +201,18 @@ def _blank_to_none(value: str | None) -> str | None: return value.strip() or None +def _uses_issuer_anchor(manual_issuer: str | None, is_discovery_auth_type: bool) -> bool: + """Whether the endpoints are authoritatively anchored to an admin-pinned issuer (RFC 8414 §3.3). + + This is the trust/provenance property, distinct from whether the ``issuer`` field is merely + populated: a trust-on-first-use discovered issuer sets ``issuer`` for token identity but is NOT + anchored, so its endpoints stay resource-rooted. Anchoring holds only when the issuer was pinned + (present on the row/config) on a discovery auth type. Every consumer of "is this anchored" reads + this one definition, so the answer cannot diverge across build paths. + """ + return _blank_to_none(manual_issuer) is not None and is_discovery_auth_type + + def _endpoints_yield_to_issuer( issuer: str | None, is_discovery_auth_type: bool, @@ -292,16 +304,20 @@ def _carry_forward_resolved_oauth_endpoints(new_server: MCPServer, previous_serv consistent group) or pins the same one. An admin re-pointing ``authorization_url`` to a different server must not keep serving the old server's token endpoint or granted scopes. - When an ``issuer`` is configured the endpoints must come solely from the §3.3-validated issuer - document, so carry-forward is skipped entirely for its endpoints: a failed issuer fetch leaves - them ``None`` and must stay ``None`` (fail-closed), never resurrected from the previous registry - entry. Scopes stay resource-driven and can still carry. + When the server is issuer-anchored (``issuer_is_anchored`` -- a pinned issuer on a discovery auth + type), the endpoints come solely from the §3.3-validated issuer document, so carry-forward is + skipped entirely for its endpoints: a failed issuer fetch leaves them ``None`` and must stay + ``None`` (fail-closed), never resurrected from the previous registry entry. A merely discovered + (trust-on-first-use) issuer is NOT anchored -- ``issuer`` is set for token identity but the + endpoints are resource-rooted, so they still carry forward as last-known-good, gated by the + corroboration check below like any other resource-rooted server. Scopes stay resource-driven and + can carry either way. """ if previous_server is None: return if previous_server.url != new_server.url or previous_server.auth_type != new_server.auth_type: return - if _blank_to_none(new_server.issuer): + if new_server.issuer_is_anchored: # Endpoints come solely from the §3.3-validated issuer document; a failed fetch stays # fail-closed and must not be resurrected from the previous entry. Only the resource-driven # scopes carry as last-known-good. @@ -1185,7 +1201,7 @@ class MCPServerManager: manual_token_url = _blank_to_none(server_config.get("token_url")) manual_registration_url = _blank_to_none(server_config.get("registration_url")) is_discovery_auth_type = auth_type in _UPSTREAM_OAUTH_DISCOVERY_AUTH_TYPES - use_issuer_anchor = manual_issuer is not None and is_discovery_auth_type + use_issuer_anchor = _uses_issuer_anchor(manual_issuer, is_discovery_auth_type) manual_authorization_url, manual_token_url, manual_registration_url = _endpoints_yield_to_issuer( manual_issuer, is_discovery_auth_type, @@ -1291,6 +1307,7 @@ class MCPServerManager: oauth2_flow=self._explicit_oauth2_flow(config_oauth2_flow), scopes=resolved_scopes, issuer=effective_issuer, + issuer_is_anchored=use_issuer_anchor, authorization_url=resolved_authorization_url, token_url=resolved_token_url, registration_url=resolved_registration_url, @@ -1685,7 +1702,7 @@ class MCPServerManager: manual_token_url = _blank_to_none(mcp_server.token_url) manual_registration_url = _blank_to_none(mcp_server.registration_url) is_discovery_auth_type = auth_type in _UPSTREAM_OAUTH_DISCOVERY_AUTH_TYPES - use_issuer_anchor = manual_issuer is not None and is_discovery_auth_type + use_issuer_anchor = _uses_issuer_anchor(manual_issuer, is_discovery_auth_type) manual_authorization_url, manual_token_url, manual_registration_url = _endpoints_yield_to_issuer( manual_issuer, is_discovery_auth_type, manual_authorization_url, manual_token_url, manual_registration_url ) @@ -1732,6 +1749,7 @@ class MCPServerManager: oauth2_flow=self._explicit_oauth2_flow(getattr(mcp_server, "oauth2_flow", None)), scopes=resolved_scopes, issuer=effective_issuer, + issuer_is_anchored=use_issuer_anchor, authorization_url=manual_authorization_url or getattr(gated_oauth_metadata, "authorization_url", None), token_url=manual_token_url or getattr(gated_oauth_metadata, "token_url", None), registration_url=manual_registration_url or getattr(gated_oauth_metadata, "registration_url", None), diff --git a/litellm/types/mcp_server/mcp_server_manager.py b/litellm/types/mcp_server/mcp_server_manager.py index cf3eed20f5b..d0d8cc4cb28 100644 --- a/litellm/types/mcp_server/mcp_server_manager.py +++ b/litellm/types/mcp_server/mcp_server_manager.py @@ -66,6 +66,7 @@ class MCPServer(BaseModel): client_id: Optional[str] = None client_secret: Optional[str] = None issuer: Optional[str] = None + issuer_is_anchored: bool = False scopes: Optional[List[str]] = None authorization_url: Optional[str] = None token_url: Optional[str] = None diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py index b16840741af..80bf08a5eba 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py @@ -1196,6 +1196,7 @@ class TestMCPServerManager: built = await manager.build_mcp_server_from_table(row, credentials_are_encrypted=False) assert built.issuer == "https://idp.example.com" + assert built.issuer_is_anchored is False assert built.authorization_url == "https://idp.example.com/authorize" @pytest.mark.asyncio @@ -1326,6 +1327,7 @@ class TestMCPServerManager: anchored.assert_awaited_once_with("https://idp.example.com", "https://up.example.com/mcp") resource_rooted.assert_not_awaited() assert built.issuer == "https://idp.example.com" + assert built.issuer_is_anchored is True assert built.authorization_url == "https://idp.example.com/authorize" assert built.token_url == "https://idp.example.com/token" assert built.registration_url == "https://idp.example.com/register" @@ -5869,11 +5871,12 @@ class TestMCPServerTimestamps: assert same_authorize.registration_url == "https://idp.example.com/register" def test_carry_forward_does_not_restore_endpoints_for_issuer_anchored_server(self): - """When an issuer is configured the endpoints come solely from the §3.3-validated issuer + """When the server is issuer-anchored the endpoints come solely from the §3.3-validated issuer document, so a failed issuer fetch (token_url None) must stay fail-closed. Carry-forward must NOT resurrect the previous registry entry's token endpoint, or the very attacker-controlled endpoint the issuer anchor distrusts would keep being served across rebuilds. Resource-driven - scopes still carry as last-known-good.""" + scopes still carry as last-known-good. Anchoring is keyed on the explicit issuer_is_anchored + flag, not on issuer truthiness, so a discovered issuer does not trip this fail-closed branch.""" from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( _carry_forward_resolved_oauth_endpoints, ) @@ -5885,6 +5888,7 @@ class TestMCPServerTimestamps: transport=MCPTransport.http, auth_type=MCPAuth.oauth2, issuer="https://idp.example.com", + issuer_is_anchored=True, authorization_url="https://idp.example.com/authorize", token_url="https://idp.example.com/token", registration_url="https://idp.example.com/register", @@ -5897,6 +5901,7 @@ class TestMCPServerTimestamps: transport=MCPTransport.http, auth_type=MCPAuth.oauth2, issuer="https://idp.example.com", + issuer_is_anchored=True, ) _carry_forward_resolved_oauth_endpoints(new_server=failed_rebuild, previous_server=previous) @@ -5906,6 +5911,47 @@ class TestMCPServerTimestamps: assert failed_rebuild.registration_url is None assert failed_rebuild.scopes == ["read"] + def test_carry_forward_restores_endpoints_for_discovered_issuer_not_anchored(self): + """A server that merely DISCOVERED its issuer trust-on-first-use is not anchored: issuer is set + for token identity but the endpoints are resource-rooted, so on a transient discovery blip they + must still carry forward as last-known-good, the same as any resource-rooted server. This is the + regression the explicit issuer_is_anchored flag prevents: keying fail-closed on issuer truthiness + alone would drop the working endpoints the moment the server learned its issuer.""" + from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( + _carry_forward_resolved_oauth_endpoints, + ) + + previous = MCPServer( + server_id="s1", + name="s1", + url="https://up.example.com/mcp", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + issuer="https://idp.example.com", + issuer_is_anchored=False, + authorization_url="https://idp.example.com/authorize", + token_url="https://idp.example.com/token", + registration_url="https://idp.example.com/register", + scopes=["read"], + ) + blipped_rebuild = MCPServer( + server_id="s1", + name="s1", + url="https://up.example.com/mcp", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + issuer="https://idp.example.com", + issuer_is_anchored=False, + authorization_url=None, + ) + + _carry_forward_resolved_oauth_endpoints(new_server=blipped_rebuild, previous_server=previous) + + assert blipped_rebuild.authorization_url == "https://idp.example.com/authorize" + assert blipped_rebuild.token_url == "https://idp.example.com/token" + assert blipped_rebuild.registration_url == "https://idp.example.com/register" + assert blipped_rebuild.scopes == ["read"] + def test_normalized_authorize_endpoint_treats_default_port_and_slash_as_identity(self): """The corroboration check must not fail on formatting-only differences an IdP legitimately emits: default port, trailing slash, host case, and query string are not identity, but a