From 8e73ff057fb7f8aeac1e2054996ac6bec48de91a Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Wed, 15 Jul 2026 13:37:10 -0700 Subject: [PATCH 1/6] =?UTF-8?q?feat(mcp):=20issuer-anchored=20OAuth=20disc?= =?UTF-8?q?overy=20(RFC=208414=20=C2=A73.3)=20as=20the=20trust=20anchor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an admin-configured issuer to MCP servers. When set, OAuth metadata is fetched from the issuer's own origin and adopted only when the document self-attests that same issuer (RFC 8414 §3.3), making token_endpoint, registration_endpoint, and scopes authoritative for the pinned issuer instead of a document the MCP resource server chose. This closes the mix-up where a compromised resource echoes a pinned authorization_url to smuggle its own token endpoint and inflated scopes past the corroboration gate. Discovery is same-authority against the issuer origin, fails closed on a §3.3 mismatch, and does not fall back to resource-rooted discovery. Rows without an issuer keep the existing corroboration-gate behavior unchanged. Backend + schema only; UI field and live-proxy proof follow. --- .../migration.sql | 2 + .../litellm_proxy_extras/schema.prisma | 1 + litellm/models/mcp_server.py | 1 + litellm/proxy/_experimental/mcp_server/db.py | 2 + .../mcp_server/mcp_server_manager.py | 113 +++++++++--- .../mcp_server/rest_endpoints.py | 1 + litellm/proxy/_types.py | 2 + litellm/proxy/schema.prisma | 1 + .../types/mcp_server/mcp_server_manager.py | 1 + schema.prisma | 1 + .../mcp_server/test_mcp_server_manager.py | 162 ++++++++++++++++++ 11 files changed, 260 insertions(+), 27 deletions(-) create mode 100644 litellm-proxy-extras/litellm_proxy_extras/migrations/20260715000000_add_issuer_to_mcp_server_table/migration.sql diff --git a/litellm-proxy-extras/litellm_proxy_extras/migrations/20260715000000_add_issuer_to_mcp_server_table/migration.sql b/litellm-proxy-extras/litellm_proxy_extras/migrations/20260715000000_add_issuer_to_mcp_server_table/migration.sql new file mode 100644 index 00000000000..f7f23e6a55e --- /dev/null +++ b/litellm-proxy-extras/litellm_proxy_extras/migrations/20260715000000_add_issuer_to_mcp_server_table/migration.sql @@ -0,0 +1,2 @@ +-- AlterTable +ALTER TABLE "LiteLLM_MCPServerTable" ADD COLUMN IF NOT EXISTS "issuer" TEXT; diff --git a/litellm-proxy-extras/litellm_proxy_extras/schema.prisma b/litellm-proxy-extras/litellm_proxy_extras/schema.prisma index a23cecc3911..f842bf13da9 100644 --- a/litellm-proxy-extras/litellm_proxy_extras/schema.prisma +++ b/litellm-proxy-extras/litellm_proxy_extras/schema.prisma @@ -325,6 +325,7 @@ model LiteLLM_MCPServerTable { command String? args String[] @default([]) env Json? @default("{}") + issuer String? authorization_url String? token_url String? registration_url String? diff --git a/litellm/models/mcp_server.py b/litellm/models/mcp_server.py index af2efa822b0..23b26bd8e89 100644 --- a/litellm/models/mcp_server.py +++ b/litellm/models/mcp_server.py @@ -79,6 +79,7 @@ class LiteLLM_MCPServerTable(LiteLLMPydanticObjectBase): command: Optional[str] = None args: List[str] = Field(default_factory=list) env: Dict[str, str] = Field(default_factory=dict) + issuer: Optional[str] = None authorization_url: Optional[str] = None token_url: Optional[str] = None registration_url: Optional[str] = None diff --git a/litellm/proxy/_experimental/mcp_server/db.py b/litellm/proxy/_experimental/mcp_server/db.py index 97cefb3f2cb..ff018a1a021 100644 --- a/litellm/proxy/_experimental/mcp_server/db.py +++ b/litellm/proxy/_experimental/mcp_server/db.py @@ -48,6 +48,7 @@ if TYPE_CHECKING: _AUTH_FLOW_SCOPED_FIELDS: frozenset = frozenset( { + "issuer", "authorization_url", "token_url", "registration_url", @@ -1181,6 +1182,7 @@ def mcp_oauth_token_identity(server: object) -> tuple[object, ...]: getattr(server, "spec_path", None), getattr(server, "auth_type", None), getattr(server, "oauth2_flow", None), + getattr(server, "issuer", None), getattr(server, "authorization_url", None), getattr(server, "token_url", None), getattr(server, "registration_url", None), diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 8b1d00c2855..9c5d3bab5e9 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -217,6 +217,17 @@ def _normalized_authorize_endpoint(url: str) -> str: return f"{scheme}://{authority}{parsed.path.rstrip('/')}" +def _issuer_matches(claimed_issuer: object, configured_issuer: str) -> bool: + """RFC 8414 §3.3 issuer equality between the metadata document's self-attested ``issuer`` and the + admin-configured issuer, tolerant only of URL-insignificant differences (scheme/host case, the + default port, a trailing slash). A non-string or empty claimed issuer never matches, so a + document that omits ``issuer`` fails closed under issuer-anchored discovery. + """ + if not isinstance(claimed_issuer, str) or not claimed_issuer: + return False + return _normalized_authorize_endpoint(claimed_issuer) == _normalized_authorize_endpoint(configured_issuer) + + def _endpoints_corroborate_authorization_url( source_authorization_url: str | None, trusted_authorization_url: str | None, @@ -1137,34 +1148,41 @@ class MCPServerManager: ) auth_type = server_config.get("auth_type", None) + manual_issuer = _blank_to_none(server_config.get("issuer")) manual_authorization_url = _blank_to_none(server_config.get("authorization_url")) manual_token_url = _blank_to_none(server_config.get("token_url")) manual_registration_url = _blank_to_none(server_config.get("registration_url")) - if server_url and ( - auth_type in _UPSTREAM_OAUTH_DISCOVERY_AUTH_TYPES + 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 + should_discover = bool(server_url) and ( + is_discovery_auth_type or self._obo_needs_endpoint_discovery( auth_type, server_config.get("token_exchange_endpoint"), manual_token_url, ) - ): + ) + if not should_discover: + mcp_oauth_metadata = None + elif manual_issuer is not None and is_discovery_auth_type: + mcp_oauth_metadata = await self._fetch_issuer_anchored_oauth_metadata(manual_issuer) + else: mcp_oauth_metadata = await self._descovery_metadata( server_url=server_url, - allow_origin_fallback=auth_type in _UPSTREAM_OAUTH_DISCOVERY_AUTH_TYPES, + allow_origin_fallback=is_discovery_auth_type, ) - else: - mcp_oauth_metadata = None - gated_oauth_metadata = ( - _restrict_discovery_to_corroborated_authorization_server( + if use_issuer_anchor: + gated_oauth_metadata = mcp_oauth_metadata + elif is_discovery_auth_type: + gated_oauth_metadata = _restrict_discovery_to_corroborated_authorization_server( mcp_oauth_metadata, manual_authorization_url, server_name or server_id, bool(server_config.get("dcr_bridge")), ) - if auth_type in _UPSTREAM_OAUTH_DISCOVERY_AUTH_TYPES - else mcp_oauth_metadata - ) + else: + gated_oauth_metadata = mcp_oauth_metadata # Filter blank scopes (e.g. YAML ``scopes: [""]``) the same way the DB-build path does, so # an all-blank list normalizes to None rather than a ``("",)`` tuple that skips the @@ -1227,6 +1245,7 @@ class MCPServerManager: client_secret=server_config.get("client_secret", None), oauth2_flow=self._explicit_oauth2_flow(config_oauth2_flow), scopes=resolved_scopes, + issuer=manual_issuer, authorization_url=resolved_authorization_url, token_url=resolved_token_url, registration_url=resolved_registration_url, @@ -1570,12 +1589,15 @@ class MCPServerManager: auth_type = cast(MCPAuthType, mcp_server.auth_type) server_url = mcp_server.url + manual_issuer = _blank_to_none(mcp_server.issuer) manual_authorization_url = _blank_to_none(mcp_server.authorization_url) 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 has_all_upstream_oauth_fields = bool(manual_authorization_url and manual_token_url and scopes) needs_discovery = bool(server_url) and ( - (auth_type in _UPSTREAM_OAUTH_DISCOVERY_AUTH_TYPES and not has_all_upstream_oauth_fields) + (is_discovery_auth_type and not has_all_upstream_oauth_fields) or self._obo_needs_endpoint_discovery( auth_type, mcp_server.token_exchange_endpoint @@ -1583,31 +1605,33 @@ class MCPServerManager: manual_token_url, ) ) - mcp_oauth_metadata = ( - await self._descovery_metadata( + if not needs_discovery: + mcp_oauth_metadata = None + elif manual_issuer is not None and is_discovery_auth_type: + mcp_oauth_metadata = await self._fetch_issuer_anchored_oauth_metadata(manual_issuer) + else: + mcp_oauth_metadata = await self._descovery_metadata( server_url=server_url, # type: ignore[arg-type] - allow_origin_fallback=auth_type in _UPSTREAM_OAUTH_DISCOVERY_AUTH_TYPES, + allow_origin_fallback=is_discovery_auth_type, ) - if needs_discovery - else None - ) - if needs_discovery and mcp_oauth_metadata is None: + if needs_discovery and not use_issuer_anchor and mcp_oauth_metadata is None: verbose_logger.warning( "MCP OAuth discovery yielded no metadata for server %s (%s); " "OAuth endpoints/scopes stay unresolved until a rebuild succeeds", mcp_server.server_id, server_url, ) - gated_oauth_metadata = ( - _restrict_discovery_to_corroborated_authorization_server( + if use_issuer_anchor: + gated_oauth_metadata = mcp_oauth_metadata + elif is_discovery_auth_type: + gated_oauth_metadata = _restrict_discovery_to_corroborated_authorization_server( mcp_oauth_metadata, manual_authorization_url, mcp_server.server_id, bool(getattr(mcp_server, "dcr_bridge", None)), ) - if auth_type in _UPSTREAM_OAUTH_DISCOVERY_AUTH_TYPES - else mcp_oauth_metadata - ) + else: + gated_oauth_metadata = mcp_oauth_metadata resolved_scopes = scopes or (gated_oauth_metadata.scopes if gated_oauth_metadata else None) @@ -1629,6 +1653,7 @@ class MCPServerManager: client_secret=client_secret_value or getattr(mcp_server, "client_secret", None), oauth2_flow=self._explicit_oauth2_flow(getattr(mcp_server, "oauth2_flow", None)), scopes=resolved_scopes, + issuer=manual_issuer, 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), @@ -3337,8 +3362,28 @@ class MCPServerManager: return metadata return None + async def _fetch_issuer_anchored_oauth_metadata(self, issuer: str) -> Optional[MCPOAuthMetadata]: + """RFC 8414 issuer-anchored discovery. + + Fetch authorization-server metadata from the admin-configured issuer's own origin and adopt + it only when the document self-attests that same issuer (RFC 8414 §3.3). Because the trust + anchor is the pinned issuer rather than anything the MCP resource server advertises, the + resulting token_endpoint/registration_endpoint/scopes are authoritative for that issuer and + cannot be substituted by a compromised resource. Fails closed (returns None) on a §3.3 + mismatch or a fetch failure. The issuer is passed as its own ``server_url`` so the fetch is + treated as same-authority and is not subject to the resource-scoped SSRF shortcut. + """ + metadata = await self._fetch_single_authorization_server_metadata(issuer, issuer, require_issuer=issuer) + if metadata is None: + verbose_logger.warning( + "MCP OAuth issuer-anchored discovery for issuer %s yielded no metadata whose issuer " + "matched (RFC 8414 §3.3); OAuth endpoints/scopes stay unresolved until a rebuild succeeds", + issuer, + ) + return metadata + async def _fetch_single_authorization_server_metadata( - self, issuer_url: str, server_url: str + self, issuer_url: str, server_url: str, require_issuer: Optional[str] = None ) -> Optional[MCPOAuthMetadata]: try: parsed = urlparse(issuer_url) @@ -3382,15 +3427,27 @@ class MCPServerManager: ) continue - scopes = self._extract_scopes(data.get("scopes_supported")) + claimed_issuer = data.get("issuer") verbose_logger.debug( "Authorization server metadata from %s: issuer=%s grant_types_supported=%s " "token_endpoint_auth_methods_supported=%s", url, - data.get("issuer"), + claimed_issuer, data.get("grant_types_supported"), data.get("token_endpoint_auth_methods_supported"), ) + if require_issuer is not None and not _issuer_matches(claimed_issuer, require_issuer): + verbose_logger.warning( + "MCP OAuth issuer-anchored discovery: metadata at %s self-attests issuer %r, which " + "does not match the configured issuer %r (RFC 8414 §3.3); rejecting so a compromised " + "resource cannot substitute an attacker authorization server", + url, + claimed_issuer, + require_issuer, + ) + continue + + scopes = self._extract_scopes(data.get("scopes_supported")) metadata = MCPOAuthMetadata( scopes=scopes, authorization_url=data.get("authorization_endpoint"), @@ -3408,6 +3465,8 @@ class MCPServerManager: ): return metadata + if require_issuer is not None: + return None return self._build_azure_authorization_server_metadata(parsed) @staticmethod diff --git a/litellm/proxy/_experimental/mcp_server/rest_endpoints.py b/litellm/proxy/_experimental/mcp_server/rest_endpoints.py index 111fde86ea0..7ca4923337c 100644 --- a/litellm/proxy/_experimental/mcp_server/rest_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/rest_endpoints.py @@ -1138,6 +1138,7 @@ if MCP_AVAILABLE: static_headers=request.static_headers, client_id=client_id, client_secret=client_secret, + issuer=request.issuer, token_url=request.token_url, scopes=scopes, authorization_url=request.authorization_url, diff --git a/litellm/proxy/_types.py b/litellm/proxy/_types.py index 5e3ea4b7dcb..053f78a3698 100644 --- a/litellm/proxy/_types.py +++ b/litellm/proxy/_types.py @@ -1263,6 +1263,7 @@ class NewMCPServerRequest(LiteLLMPydanticObjectBase): command: Optional[str] = None args: List[str] = Field(default_factory=list) env: Dict[str, str] = Field(default_factory=dict) + issuer: Optional[str] = None authorization_url: Optional[str] = None token_url: Optional[str] = None registration_url: Optional[str] = None @@ -1368,6 +1369,7 @@ class UpdateMCPServerRequest(LiteLLMPydanticObjectBase): command: Optional[str] = None args: List[str] = Field(default_factory=list) env: Dict[str, str] = Field(default_factory=dict) + issuer: Optional[str] = None authorization_url: Optional[str] = None token_url: Optional[str] = None registration_url: Optional[str] = None diff --git a/litellm/proxy/schema.prisma b/litellm/proxy/schema.prisma index a23cecc3911..f842bf13da9 100644 --- a/litellm/proxy/schema.prisma +++ b/litellm/proxy/schema.prisma @@ -325,6 +325,7 @@ model LiteLLM_MCPServerTable { command String? args String[] @default([]) env Json? @default("{}") + issuer String? authorization_url String? token_url String? registration_url String? diff --git a/litellm/types/mcp_server/mcp_server_manager.py b/litellm/types/mcp_server/mcp_server_manager.py index 801436c774a..c892824fb54 100644 --- a/litellm/types/mcp_server/mcp_server_manager.py +++ b/litellm/types/mcp_server/mcp_server_manager.py @@ -60,6 +60,7 @@ class MCPServer(BaseModel): # OAuth-specific fields client_id: Optional[str] = None client_secret: Optional[str] = None + issuer: Optional[str] = None scopes: Optional[List[str]] = None authorization_url: Optional[str] = None token_url: Optional[str] = None diff --git a/schema.prisma b/schema.prisma index a23cecc3911..f842bf13da9 100644 --- a/schema.prisma +++ b/schema.prisma @@ -325,6 +325,7 @@ model LiteLLM_MCPServerTable { command String? args String[] @default([]) env Json? @default("{}") + issuer String? authorization_url String? token_url String? registration_url String? 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 bba00ed1819..58773642692 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 @@ -1230,6 +1230,80 @@ class TestMCPServerManager: assert built.registration_url == "https://idp.example.com/register" assert built.scopes == ["read"] + @pytest.mark.asyncio + async def test_build_from_table_issuer_anchor_ignores_resource_rooted_discovery(self): + """When an admin configures an issuer, discovery is anchored on that issuer's own metadata + (RFC 8414), not on the resource-rooted RFC 9728 chain a compromised MCP resource controls. + The resource-rooted _descovery_metadata must not run at all, and the authoritative token_url, + registration_url, and scopes come from the issuer document. This closes the mix-up where a + compromised resource echoes the pinned authorize endpoint to smuggle its own token_url.""" + manager = MCPServerManager() + row = LiteLLM_MCPServerTable( + server_id="issuer-anchored-1", + alias="issuer_anchored", + description="issuer configured, blank endpoints", + url="https://up.example.com/mcp", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + issuer="https://idp.example.com", + created_at=datetime.now(), + updated_at=datetime.now(), + ) + + authoritative = MCPOAuthMetadata( + authorization_url="https://idp.example.com/authorize", + token_url="https://idp.example.com/token", + registration_url="https://idp.example.com/register", + scopes=["read", "write"], + authorization_server_scopes=["read", "write"], + ) + resource_rooted = AsyncMock(return_value=MCPOAuthMetadata(token_url="https://attacker.example.com/steal")) + with ( + patch.object(manager, "_fetch_issuer_anchored_oauth_metadata", new=AsyncMock(return_value=authoritative)) as anchored, + patch.object(manager, "_descovery_metadata", new=resource_rooted), + ): + built = await manager.build_mcp_server_from_table(row, credentials_are_encrypted=False) + + anchored.assert_awaited_once_with("https://idp.example.com") + resource_rooted.assert_not_awaited() + assert built.issuer == "https://idp.example.com" + 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" + assert built.scopes == ["read", "write"] + + @pytest.mark.asyncio + async def test_build_from_table_issuer_anchor_fails_closed_without_falling_back_to_resource(self): + """A configured issuer whose metadata does not validate (RFC 8414 §3.3 mismatch or fetch + failure) yields None from the anchored fetch. The build must adopt nothing and must NOT fall + back to resource-rooted discovery, or the fail-closed guarantee would be defeated by the very + resource the issuer anchor exists to distrust.""" + manager = MCPServerManager() + row = LiteLLM_MCPServerTable( + server_id="issuer-anchored-2", + alias="issuer_anchored_failclosed", + description="issuer configured, upstream fails validation", + url="https://up.example.com/mcp", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + issuer="https://idp.example.com", + created_at=datetime.now(), + updated_at=datetime.now(), + ) + + resource_rooted = AsyncMock(return_value=MCPOAuthMetadata(token_url="https://attacker.example.com/steal")) + with ( + patch.object(manager, "_fetch_issuer_anchored_oauth_metadata", new=AsyncMock(return_value=None)), + patch.object(manager, "_descovery_metadata", new=resource_rooted), + ): + built = await manager.build_mcp_server_from_table(row, credentials_are_encrypted=False) + + resource_rooted.assert_not_awaited() + assert built.issuer == "https://idp.example.com" + assert built.token_url is None + assert built.registration_url is None + assert built.scopes is None + @pytest.mark.asyncio @pytest.mark.parametrize( "advertised_authorization_url", @@ -2462,6 +2536,78 @@ class TestMCPServerManager: assert result.token_url == "https://login.microsoftonline.com/test-tenant-id/oauth2/v2.0/token" assert result.scopes == ["api://some-scope/.default"] + @staticmethod + def _issuer_doc_response_builder(well_known_url: str, document: dict): + def build_response(url: str, **kwargs): + mock_response = MagicMock() + if url == well_known_url: + mock_response.json.return_value = document + mock_response.raise_for_status = MagicMock() + else: + request = httpx.Request("GET", url) + response_obj = httpx.Response(status_code=404, request=request) + mock_response.raise_for_status = MagicMock( + side_effect=httpx.HTTPStatusError("not found", request=request, response=response_obj) + ) + return mock_response + + return build_response + + @pytest.mark.asyncio + async def test_fetch_single_authorization_server_metadata_adopts_document_with_matching_issuer(self): + """RFC 8414 §3.3: under require_issuer, a document that self-attests the same issuer it was + fetched from is authoritative and its endpoints and scopes are adopted.""" + manager = MCPServerManager() + issuer = "https://idp.example.com" + build_response = self._issuer_doc_response_builder( + f"{issuer}/.well-known/oauth-authorization-server", + { + "issuer": issuer, + "authorization_endpoint": "https://idp.example.com/authorize", + "token_endpoint": "https://idp.example.com/token", + "scopes_supported": ["read", "write"], + }, + ) + mock_client = MagicMock() + mock_client.get = AsyncMock(side_effect=build_response) + with patch( + "litellm.proxy._experimental.mcp_server.mcp_server_manager.get_async_httpx_client", + return_value=mock_client, + ): + result = await manager._fetch_single_authorization_server_metadata(issuer, issuer, require_issuer=issuer) + + assert result is not None + assert result.authorization_url == "https://idp.example.com/authorize" + assert result.token_url == "https://idp.example.com/token" + assert result.scopes == ["read", "write"] + + @pytest.mark.asyncio + async def test_fetch_single_authorization_server_metadata_rejects_issuer_mismatch(self): + """RFC 8414 §3.3 fail-closed: a document self-attesting a DIFFERENT issuer than the one it was + fetched from is rejected even though it carries valid-looking endpoints, so a compromised + resource cannot point the issuer-anchored fetch at an attacker authorization server that + smuggles its own token_endpoint and inflated scopes.""" + manager = MCPServerManager() + issuer = "https://idp.example.com" + build_response = self._issuer_doc_response_builder( + f"{issuer}/.well-known/oauth-authorization-server", + { + "issuer": "https://attacker.example.com", + "authorization_endpoint": "https://idp.example.com/authorize", + "token_endpoint": "https://attacker.example.com/steal", + "scopes_supported": ["admin"], + }, + ) + mock_client = MagicMock() + mock_client.get = AsyncMock(side_effect=build_response) + with patch( + "litellm.proxy._experimental.mcp_server.mcp_server_manager.get_async_httpx_client", + return_value=mock_client, + ): + result = await manager._fetch_single_authorization_server_metadata(issuer, issuer, require_issuer=issuer) + + assert result is None + @pytest.mark.asyncio async def test_fetch_single_authorization_server_metadata_derives_azure_metadata( self, @@ -5487,6 +5633,22 @@ class TestMCPServerTimestamps: assert _normalized_authorize_endpoint("https://idp.example.com/authorize?prompt=consent") == canonical assert _normalized_authorize_endpoint("https://idp.example.com:8443/authorize") != canonical + def test_issuer_matches_rfc8414_section_3_3(self): + """Issuer equality tolerates only URL-insignificant differences (scheme/host case, default + port, a trailing slash). A different host, a non-string, an empty string, or a None issuer + never matches, so a document that omits issuer fails closed under issuer-anchored discovery.""" + from litellm.proxy._experimental.mcp_server.mcp_server_manager import _issuer_matches + + assert _issuer_matches("https://mcp.slack.com", "https://mcp.slack.com") + assert _issuer_matches("https://MCP.slack.com/", "https://mcp.slack.com") + assert _issuer_matches("https://mcp.slack.com:443", "https://mcp.slack.com") + assert _issuer_matches("https://login.example.com/tenant/v2.0", "https://login.example.com/tenant/v2.0") + assert not _issuer_matches("https://attacker.example.com", "https://mcp.slack.com") + assert not _issuer_matches("https://login.example.com/other/v2.0", "https://login.example.com/tenant/v2.0") + assert not _issuer_matches(None, "https://mcp.slack.com") + assert not _issuer_matches("", "https://mcp.slack.com") + assert not _issuer_matches(123, "https://mcp.slack.com") + def test_build_mcp_server_table_preserves_timestamps(self): """_build_mcp_server_table must use the MCPServer's stored timestamps, not datetime.now().""" manager = MCPServerManager() From 031922eec648fd5d5dc863d13833c89c1d769bc6 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Wed, 15 Jul 2026 14:06:51 -0700 Subject: [PATCH 2/6] feat(mcp): discover-by-default issuer (trust-on-first-use) + stale-clear on url/auth_type change + UI Discover the issuer from the upstream and persist it trust-on-first-use (fill-empty-only, frozen thereafter), so admins do not have to type it; an admin-configured issuer always wins and is never overwritten. Re-pointing the server url now clears the discovered issuer and endpoints (matching the existing auth_type-change clearing) so a new upstream re-discovers instead of anchoring on the previous upstream's issuer. Adds the issuer to the per-user OAuth token identity so re-pointing it purges stale tokens. Surfaces the issuer as an optional, auto-discovered, overridable field in the create and edit MCP server forms. C901 gate shows +1 vs staging; that is inherited from the #33317 stack base (delta 0 against --- litellm/proxy/_experimental/mcp_server/db.py | 10 ++-- .../mcp_server/mcp_server_manager.py | 13 +++++- .../types/mcp_server/mcp_server_manager.py | 5 ++ .../mcp_server/test_mcp_partial_update.py | 41 +++++++++++++++++ .../mcp_server/test_mcp_server_manager.py | 46 ++++++++++++++++++- .../_components/OAuthFormFields.tsx | 11 +++++ .../_components/create_mcp_server.tsx | 3 +- .../_components/mcp_server_edit.tsx | 24 +++++++++- .../src/components/mcp_tools/types.tsx | 2 + 9 files changed, 147 insertions(+), 8 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/db.py b/litellm/proxy/_experimental/mcp_server/db.py index ff018a1a021..22c531b5c79 100644 --- a/litellm/proxy/_experimental/mcp_server/db.py +++ b/litellm/proxy/_experimental/mcp_server/db.py @@ -698,13 +698,14 @@ async def update_mcp_server( # of being reset to a schema default (transport=sse, allow_all_keys=False...). data_dict = _prepare_mcp_server_data(data, exclude_unset=True, fields_set=fields_set) - # Pre-fetch existing record once if we need it for auth_type or credential logic + # Pre-fetch existing record once if we need it for auth_type, url, or credential logic existing = None has_credentials = "credentials" in data_dict and data_dict["credentials"] is not None # An explicit token-exchange column write (set or clear) also migrates the # legacy blob copies below, so the existing row is needed for those updates. explicit_te_write = bool(_TOKEN_EXCHANGE_COLUMN_FIELDS & data_dict.keys()) - if data.auth_type or has_credentials or explicit_te_write: + url_provided = "url" in data_dict and data_dict["url"] is not None + if data.auth_type or has_credentials or explicit_te_write or url_provided: existing = await MCPServerRepository(prisma_client).table.find_unique(where={"server_id": data.server_id}) auth_type_changed = bool( @@ -712,12 +713,15 @@ async def update_mcp_server( and existing and _credential_auth_class(existing.auth_type) != _credential_auth_class(data.auth_type) ) + # A url change re-points the server at a potentially different upstream, so any discovered or + # trust-on-first-use OAuth endpoints/issuer belong to the old upstream and must re-discover. + url_changed = bool(url_provided and existing and existing.url != data_dict["url"]) # Clear stale credentials when auth_type changes but no new credentials provided if auth_type_changed and "credentials" not in data_dict: data_dict["credentials"] = None - if auth_type_changed: + if auth_type_changed or url_changed: data_dict.update({field: None for field in _AUTH_FLOW_SCOPED_FIELDS if field not in data_dict}) # An explicit column write that does not touch credentials must still migrate diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 9c5d3bab5e9..42c9921ecf7 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -1713,6 +1713,7 @@ class MCPServerManager: await self._persist_discovered_oauth_endpoints( server_id=mcp_server.server_id, auth_type=auth_type, + existing_issuer=manual_issuer, existing_authorization_url=manual_authorization_url, existing_token_url=manual_token_url, existing_scopes=scopes, @@ -1760,6 +1761,7 @@ class MCPServerManager: *, server_id: str, auth_type: MCPAuthType | None, + existing_issuer: str | None, existing_authorization_url: str | None, existing_token_url: str | None, existing_scopes: list[str] | None, @@ -1782,6 +1784,9 @@ class MCPServerManager: return if metadata is None or metadata.from_origin_fallback: return + issuer_update = ( + {"issuer": metadata.discovered_issuer} if metadata.discovered_issuer and not existing_issuer else {} + ) authorization_url_update = ( {"authorization_url": metadata.authorization_url} if metadata.authorization_url and not existing_authorization_url @@ -1789,7 +1794,12 @@ class MCPServerManager: ) token_url_update = {"token_url": metadata.token_url} if metadata.token_url and not existing_token_url else {} scopes_update = {"credentials": {"scopes": metadata.scopes}} if metadata.scopes and not existing_scopes else {} - updates: dict[str, object] = {**authorization_url_update, **token_url_update, **scopes_update} + updates: dict[str, object] = { + **issuer_update, + **authorization_url_update, + **token_url_update, + **scopes_update, + } if not updates: return from litellm.proxy._experimental.mcp_server.db import ( # noqa: PLC0415 # db.py imports this module at load @@ -3453,6 +3463,7 @@ class MCPServerManager: authorization_url=data.get("authorization_endpoint"), token_url=data.get("token_endpoint"), registration_url=data.get("registration_endpoint"), + discovered_issuer=claimed_issuer if isinstance(claimed_issuer, str) and claimed_issuer else None, ) if any( diff --git a/litellm/types/mcp_server/mcp_server_manager.py b/litellm/types/mcp_server/mcp_server_manager.py index c892824fb54..cf3eed20f5b 100644 --- a/litellm/types/mcp_server/mcp_server_manager.py +++ b/litellm/types/mcp_server/mcp_server_manager.py @@ -26,6 +26,11 @@ class MCPOAuthMetadata(BaseModel): authorization_url: Optional[str] = None token_url: Optional[str] = None registration_url: Optional[str] = None + discovered_issuer: Optional[str] = None + """The ``issuer`` the authorization-server metadata document self-attests (RFC 8414). Persisted + trust-on-first-use as the server's ``issuer`` when none is configured, so that later rebuilds + anchor discovery on it (RFC 8414 §3.3) and a subsequently compromised resource cannot re-point + it. Never overwrites an admin-configured issuer.""" from_origin_fallback: bool = False """True when the metadata came from guessing the resource origin as its authorization server rather than from an RFC 9728/8414-advertised document. Guessed endpoints are diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py index 41d61c8f508..637ee409fbe 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py @@ -203,6 +203,7 @@ async def test_auth_type_switch_clears_stale_flow_scoped_fields(): data_dict = await _run_update_with_existing(data, existing_auth_type="oauth2") for stale_field in ( + "issuer", "authorization_url", "token_url", "registration_url", @@ -217,6 +218,46 @@ async def test_auth_type_switch_clears_stale_flow_scoped_fields(): assert _credentials_cleared(data_dict["credentials"]) +@pytest.mark.asyncio +async def test_url_change_clears_stale_discovered_oauth_fields(): + """Re-pointing the server url at a potentially different upstream must clear the discovered or + trust-on-first-use OAuth issuer and endpoints, so the new upstream re-discovers instead of + anchoring on the previous upstream's issuer (RFC 8414 §3.3 against a stale anchor).""" + mock_prisma = _mock_prisma() + existing = MagicMock() + existing.auth_type = "oauth2" + existing.url = "https://old.example.com/mcp" + existing.credentials = None + mock_prisma.db.litellm_mcpservertable.find_unique = AsyncMock(return_value=existing) + + data = UpdateMCPServerRequest(server_id="my-test-server", url="https://new.example.com/mcp") + await update_mcp_server(mock_prisma, data, "test-user") + data_dict = mock_prisma.db.litellm_mcpservertable.update.call_args[1]["data"] + + assert data_dict["url"] == "https://new.example.com/mcp" + for stale_field in ("issuer", "authorization_url", "token_url", "registration_url"): + assert data_dict[stale_field] is None, f"{stale_field} must be cleared on url change" + + +@pytest.mark.asyncio +async def test_unchanged_url_does_not_clear_discovered_oauth_fields(): + """A partial update that resends the same url (or omits it) must not clear the discovered OAuth + fields, so a routine save does not force needless re-discovery.""" + mock_prisma = _mock_prisma() + existing = MagicMock() + existing.auth_type = "oauth2" + existing.url = "https://same.example.com/mcp" + existing.credentials = None + mock_prisma.db.litellm_mcpservertable.find_unique = AsyncMock(return_value=existing) + + data = UpdateMCPServerRequest(server_id="my-test-server", url="https://same.example.com/mcp") + await update_mcp_server(mock_prisma, data, "test-user") + data_dict = mock_prisma.db.litellm_mcpservertable.update.call_args[1]["data"] + + for preserved_field in ("issuer", "authorization_url", "token_url", "registration_url"): + assert preserved_field not in data_dict, f"{preserved_field} must not be cleared when url is unchanged" + + @pytest.mark.asyncio async def test_auth_type_switch_keeps_explicitly_provided_flow_fields(): """Fields explicitly provided alongside the auth_type switch must survive it.""" 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 58773642692..3dcd05e5936 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 @@ -1255,7 +1255,6 @@ class TestMCPServerManager: token_url="https://idp.example.com/token", registration_url="https://idp.example.com/register", scopes=["read", "write"], - authorization_server_scopes=["read", "write"], ) resource_rooted = AsyncMock(return_value=MCPOAuthMetadata(token_url="https://attacker.example.com/steal")) with ( @@ -5340,6 +5339,7 @@ class TestMCPServerTimestamps: await manager._persist_discovered_oauth_endpoints( server_id="s", auth_type=MCPAuth.api_key, + existing_issuer=None, existing_authorization_url=None, existing_token_url=None, existing_scopes=None, @@ -5348,6 +5348,7 @@ class TestMCPServerTimestamps: await manager._persist_discovered_oauth_endpoints( server_id="s", auth_type=MCPAuth.oauth2, + existing_issuer=None, existing_authorization_url=None, existing_token_url=None, existing_scopes=None, @@ -5356,6 +5357,7 @@ class TestMCPServerTimestamps: await manager._persist_discovered_oauth_endpoints( server_id="s", auth_type=MCPAuth.oauth2, + existing_issuer=None, existing_authorization_url=None, existing_token_url=None, existing_scopes=None, @@ -5364,6 +5366,7 @@ class TestMCPServerTimestamps: await manager._persist_discovered_oauth_endpoints( server_id="s", auth_type=MCPAuth.oauth2, + existing_issuer=None, existing_authorization_url="https://configured.example.com/authorize", existing_token_url="https://configured.example.com/token", existing_scopes=["configured"], @@ -5389,6 +5392,7 @@ class TestMCPServerTimestamps: await manager._persist_discovered_oauth_endpoints( server_id="s", auth_type=MCPAuth.oauth2, + existing_issuer=None, existing_authorization_url=None, existing_token_url="https://configured.example.com/token", existing_scopes=None, @@ -5405,6 +5409,46 @@ class TestMCPServerTimestamps: assert persisted.credentials == {"scopes": ["s1"]} assert "token_url" not in persisted.fields_set() + @pytest.mark.asyncio + async def test_persist_discovered_oauth_endpoints_writes_discovered_issuer_trust_on_first_use(self): + """A server with no configured issuer records the discovered issuer trust-on-first-use, so the + next rebuild anchors discovery on it (RFC 8414 §3.3) instead of re-trusting the resource. When + an issuer is already set (admin-typed or a prior discovery), it is never overwritten.""" + manager = MCPServerManager() + metadata = MCPOAuthMetadata( + authorization_url="https://idp.example.com/authorize", + token_url="https://idp.example.com/token", + discovered_issuer="https://idp.example.com", + ) + + update_mcp_server_mock = AsyncMock() + with ( + patch("litellm.proxy._experimental.mcp_server.db.update_mcp_server", new=update_mcp_server_mock), + patch("litellm.proxy.proxy_server.prisma_client", MagicMock()), + ): + await manager._persist_discovered_oauth_endpoints( + server_id="s", + auth_type=MCPAuth.oauth2, + existing_issuer=None, + existing_authorization_url=None, + existing_token_url=None, + existing_scopes=None, + metadata=metadata, + ) + await manager._persist_discovered_oauth_endpoints( + server_id="s", + auth_type=MCPAuth.oauth2, + existing_issuer="https://admin-configured.example.com", + existing_authorization_url="https://admin-configured.example.com/authorize", + existing_token_url="https://admin-configured.example.com/token", + existing_scopes=["cfg"], + metadata=metadata, + ) + + assert update_mcp_server_mock.await_count == 1 + persisted = update_mcp_server_mock.call_args.kwargs["data"] + assert persisted.issuer == "https://idp.example.com" + @pytest.mark.asyncio async def test_build_mcp_server_from_table_skips_persistence_for_temporary_servers(self): """The session endpoint builds temporary servers whose server_id has no DB row; with diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/OAuthFormFields.tsx b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/OAuthFormFields.tsx index fc72981edd0..76e4342f52e 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/OAuthFormFields.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/OAuthFormFields.tsx @@ -167,6 +167,17 @@ const OAuthFormFields: React.FC = ({ > + @@ -1588,6 +1607,7 @@ const MCPServerEdit: React.FC = ({ oauthFlowTypeValue ?? oauth2FlowToFormValue(mcpServer.oauth2_flow) ?? OAUTH_FLOW.INTERACTIVE, static_headers: currentStaticHeaders ?? mcpServer.static_headers, credentials: currentCredentials, + issuer: currentIssuer ?? mcpServer.issuer, authorization_url: currentAuthorizationUrl ?? mcpServer.authorization_url, token_url: currentTokenUrl ?? mcpServer.token_url, registration_url: currentRegistrationUrl ?? mcpServer.registration_url, diff --git a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx index 04766aad7b4..dd7ed2bfbe4 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx @@ -81,6 +81,7 @@ export const getOAuthAuthorizationIdentity = (values: Record): client_id: credentials.client_id ?? null, client_secret: credentials.client_secret ?? null, scopes: credentials.scopes ?? null, + issuer: values.issuer ?? null, authorization_url: values.authorization_url ?? null, token_url: values.token_url ?? null, registration_url: values.registration_url ?? null, @@ -341,6 +342,7 @@ export interface MCPServer { transport?: string | null; auth_type?: string | null; oauth2_flow?: string | null; + issuer?: string | null; authorization_url?: string | null; token_url?: string | null; registration_url?: string | null; From b3af125078bcc2572986db0dbf77bd21380dc0d8 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Wed, 15 Jul 2026 16:36:52 -0700 Subject: [PATCH 3/6] fix(mcp): keep scopes resource-driven under a pinned issuer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issuer anchor is for the token/registration endpoints only (the RFC 9700 mix-up). Scope selection stays resource-driven per the MCP authorization spec Scope Selection Strategy: _fetch_issuer_anchored_oauth_metadata now validates the issuer document (RFC 8414 §3.3) for the endpoints and separately fetches the resource's advertised scopes (WWW-Authenticate challenge, else RFC 9728 scopes_supported) for the scope value, instead of using the issuer document's own scopes_supported. The resource can influence only the requested scope, which the authorization server and user consent bound (RFC 6749 §3.3), never the token endpoint. --- .../mcp_server/mcp_server_manager.py | 36 ++++++++----- .../mcp_server/test_mcp_server_manager.py | 53 +++++++++++++++---- 2 files changed, 68 insertions(+), 21 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 42c9921ecf7..70180d5d469 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -1165,7 +1165,7 @@ class MCPServerManager: if not should_discover: mcp_oauth_metadata = None elif manual_issuer is not None and is_discovery_auth_type: - mcp_oauth_metadata = await self._fetch_issuer_anchored_oauth_metadata(manual_issuer) + mcp_oauth_metadata = await self._fetch_issuer_anchored_oauth_metadata(manual_issuer, server_url) else: mcp_oauth_metadata = await self._descovery_metadata( server_url=server_url, @@ -1608,7 +1608,10 @@ class MCPServerManager: if not needs_discovery: mcp_oauth_metadata = None elif manual_issuer is not None and is_discovery_auth_type: - mcp_oauth_metadata = await self._fetch_issuer_anchored_oauth_metadata(manual_issuer) + mcp_oauth_metadata = await self._fetch_issuer_anchored_oauth_metadata( + manual_issuer, + server_url, # type: ignore[arg-type] + ) else: mcp_oauth_metadata = await self._descovery_metadata( server_url=server_url, # type: ignore[arg-type] @@ -3372,25 +3375,34 @@ class MCPServerManager: return metadata return None - async def _fetch_issuer_anchored_oauth_metadata(self, issuer: str) -> Optional[MCPOAuthMetadata]: - """RFC 8414 issuer-anchored discovery. + async def _fetch_issuer_anchored_oauth_metadata(self, issuer: str, server_url: str) -> Optional[MCPOAuthMetadata]: + """RFC 8414 issuer-anchored discovery for the OAuth endpoints, with resource-driven scopes. Fetch authorization-server metadata from the admin-configured issuer's own origin and adopt - it only when the document self-attests that same issuer (RFC 8414 §3.3). Because the trust - anchor is the pinned issuer rather than anything the MCP resource server advertises, the - resulting token_endpoint/registration_endpoint/scopes are authoritative for that issuer and - cannot be substituted by a compromised resource. Fails closed (returns None) on a §3.3 - mismatch or a fetch failure. The issuer is passed as its own ``server_url`` so the fetch is - treated as same-authority and is not subject to the resource-scoped SSRF shortcut. + its ``token_endpoint``/``registration_endpoint`` only when the document self-attests that same + issuer (RFC 8414 §3.3). Because the trust anchor is the pinned issuer rather than anything the + MCP resource advertises, the endpoints are authoritative for that issuer and cannot be + substituted by a compromised resource. Fails closed (returns None) on a §3.3 mismatch or a + fetch failure. The issuer is passed as its own ``server_url`` so the endpoint fetch is treated + as same-authority and is not subject to the resource-scoped SSRF shortcut. + + Scopes are NOT taken from the issuer document. Per the MCP authorization spec Scope Selection + Strategy and RFC 9728, the scopes a client requests are resource-driven (the WWW-Authenticate + challenge or the protected-resource ``scopes_supported``), so the resource's advertised scopes + are fetched separately and used; the resource can influence only the requested scope, which + the authorization server and user consent bound (RFC 6749 §3.3), never the token endpoint. """ metadata = await self._fetch_single_authorization_server_metadata(issuer, issuer, require_issuer=issuer) if metadata is None: verbose_logger.warning( "MCP OAuth issuer-anchored discovery for issuer %s yielded no metadata whose issuer " - "matched (RFC 8414 §3.3); OAuth endpoints/scopes stay unresolved until a rebuild succeeds", + "matched (RFC 8414 §3.3); OAuth endpoints stay unresolved until a rebuild succeeds", issuer, ) - return metadata + return None + resource_metadata = await self._descovery_metadata(server_url, allow_origin_fallback=False) + resource_scopes = resource_metadata.scopes if resource_metadata else None + return metadata.model_copy(update={"scopes": resource_scopes}) async def _fetch_single_authorization_server_metadata( self, issuer_url: str, server_url: str, require_issuer: 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 3dcd05e5936..75f77c1ec6d 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 @@ -1231,12 +1231,12 @@ class TestMCPServerManager: assert built.scopes == ["read"] @pytest.mark.asyncio - async def test_build_from_table_issuer_anchor_ignores_resource_rooted_discovery(self): - """When an admin configures an issuer, discovery is anchored on that issuer's own metadata - (RFC 8414), not on the resource-rooted RFC 9728 chain a compromised MCP resource controls. - The resource-rooted _descovery_metadata must not run at all, and the authoritative token_url, - registration_url, and scopes come from the issuer document. This closes the mix-up where a - compromised resource echoes the pinned authorize endpoint to smuggle its own token_url.""" + async def test_build_from_table_uses_issuer_anchored_endpoints_when_issuer_configured(self): + """When an admin configures an issuer, the build takes its endpoints from the issuer-anchored + fetch (RFC 8414 §3.3) rather than the resource-rooted corroboration path. The build path does + not call _descovery_metadata directly; the issuer-anchored helper is responsible for combining + issuer endpoints with resource-driven scopes internally, and is invoked with the server url so + it can fetch those scopes.""" manager = MCPServerManager() row = LiteLLM_MCPServerTable( server_id="issuer-anchored-1", @@ -1250,7 +1250,7 @@ class TestMCPServerManager: updated_at=datetime.now(), ) - authoritative = MCPOAuthMetadata( + resolved = MCPOAuthMetadata( authorization_url="https://idp.example.com/authorize", token_url="https://idp.example.com/token", registration_url="https://idp.example.com/register", @@ -1258,12 +1258,12 @@ class TestMCPServerManager: ) resource_rooted = AsyncMock(return_value=MCPOAuthMetadata(token_url="https://attacker.example.com/steal")) with ( - patch.object(manager, "_fetch_issuer_anchored_oauth_metadata", new=AsyncMock(return_value=authoritative)) as anchored, + patch.object(manager, "_fetch_issuer_anchored_oauth_metadata", new=AsyncMock(return_value=resolved)) as anchored, patch.object(manager, "_descovery_metadata", new=resource_rooted), ): built = await manager.build_mcp_server_from_table(row, credentials_are_encrypted=False) - anchored.assert_awaited_once_with("https://idp.example.com") + 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.authorization_url == "https://idp.example.com/authorize" @@ -1271,6 +1271,41 @@ class TestMCPServerManager: assert built.registration_url == "https://idp.example.com/register" assert built.scopes == ["read", "write"] + @pytest.mark.asyncio + async def test_fetch_issuer_anchored_metadata_takes_endpoints_from_issuer_scopes_from_resource(self): + """The issuer-anchored helper adopts token_endpoint/registration_endpoint from the pinned + issuer's own §3.3-validated document, but the scopes are resource-driven: it fetches the + resource's advertised scopes and uses those, not the issuer document's scopes_supported. This + keeps endpoint trust anchored on the issuer while scope selection stays resource-driven per the + MCP Scope Selection Strategy.""" + manager = MCPServerManager() + + issuer_document = MCPOAuthMetadata( + authorization_url="https://idp.example.com/authorize", + token_url="https://idp.example.com/token", + registration_url="https://idp.example.com/register", + scopes=["as.everything"], + ) + resource_document = MCPOAuthMetadata(scopes=["resource.read"]) + with ( + patch.object( + manager, "_fetch_single_authorization_server_metadata", new=AsyncMock(return_value=issuer_document) + ) as issuer_fetch, + patch.object(manager, "_descovery_metadata", new=AsyncMock(return_value=resource_document)) as resource_fetch, + ): + result = await manager._fetch_issuer_anchored_oauth_metadata( + "https://idp.example.com", "https://up.example.com/mcp" + ) + + issuer_fetch.assert_awaited_once_with( + "https://idp.example.com", "https://idp.example.com", require_issuer="https://idp.example.com" + ) + resource_fetch.assert_awaited_once() + assert result is not None + assert result.token_url == "https://idp.example.com/token" + assert result.registration_url == "https://idp.example.com/register" + assert result.scopes == ["resource.read"] + @pytest.mark.asyncio async def test_build_from_table_issuer_anchor_fails_closed_without_falling_back_to_resource(self): """A configured issuer whose metadata does not validate (RFC 8414 §3.3 mismatch or fetch From 032a2f2d76ef06cb71f982610503de73afd7bde0 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Wed, 15 Jul 2026 17:44:21 -0700 Subject: [PATCH 4/6] fix(mcp): enforce the issuer trust anchor at every endpoint adoption site When an admin pins an issuer, RFC 8414 section 3.3 makes that issuer the sole authoritative source of the authorization and token endpoints, so a compromised or misconfigured upstream cannot smuggle a token endpoint by echoing the pinned authorize URL. The first cut enforced that only on the database build path; the carry-forward, persistence, config-load, serialization and sanitization paths could still restore or emit upstream-derived endpoints for an issuer-anchored server, which is the class of gap the review flagged. Every site now routes through one predicate. _endpoints_yield_to_issuer returns all-None whenever the issuer is the anchor, so both build paths, has_all_upstream_oauth_fields, needs_discovery and the endpoint merge defer to the issuer. _carry_forward_resolved_oauth_endpoints carries only scopes for an issuer-anchored server and fails closed on endpoints. _persist_discovered_oauth_endpoints skips endpoint writes under the anchor. The two table serializers round-trip the issuer and both non-admin sanitizers redact it. Scope selection stays resource-driven per the MCP authorization spec: _fetch_issuer_anchored_oauth_metadata takes endpoints from the issuer document and scopes from the resource document. The OAuth metadata resolution and corroboration gating for the database build path move into _resolve_table_oauth_metadata so build_mcp_server_from_table stays within the cyclomatic-complexity budget without changing behavior. Regression tests pin the invariant at each site: the issuer overrides stored endpoints even when they are populated, carry-forward does not restore endpoints under the anchor, persistence does not write endpoints under the anchor, a url or auth_type change clears stale issuer-scoped fields even when resubmitted unchanged, the Azure heuristic stays reachable under a required issuer, and anchored metadata takes endpoints from the issuer while scopes come from the resource --- litellm/proxy/_experimental/mcp_server/db.py | 12 +- .../mcp_server/mcp_server_manager.py | 169 +++++++++++++----- .../mcp_management_endpoints.py | 3 + .../mcp_server/test_mcp_partial_update.py | 31 ++++ .../mcp_server/test_mcp_server_manager.py | 148 +++++++++++++++ 5 files changed, 317 insertions(+), 46 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/db.py b/litellm/proxy/_experimental/mcp_server/db.py index 22c531b5c79..a21710892eb 100644 --- a/litellm/proxy/_experimental/mcp_server/db.py +++ b/litellm/proxy/_experimental/mcp_server/db.py @@ -722,7 +722,17 @@ async def update_mcp_server( data_dict["credentials"] = None if auth_type_changed or url_changed: - data_dict.update({field: None for field in _AUTH_FLOW_SCOPED_FIELDS if field not in data_dict}) + # Clear each auth-flow-scoped field that the caller either omitted (partial update) or + # resubmitted unchanged. The edit form re-sends every field, so a stale issuer/endpoint + # belonging to the old upstream would otherwise survive a url/auth_type change and win in the + # resolution merge; only a genuinely new submitted value is kept. + data_dict.update( + { + field: None + for field in _AUTH_FLOW_SCOPED_FIELDS + if field not in data_dict or data_dict[field] == getattr(existing, field, None) + } + ) # An explicit column write that does not touch credentials must still migrate # the row's legacy blob copies: lift values for columns the caller left diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 70180d5d469..a547f5b6920 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -201,6 +201,26 @@ def _blank_to_none(value: str | None) -> str | None: return value.strip() or None +def _endpoints_yield_to_issuer( + issuer: str | None, + is_discovery_auth_type: bool, + authorization_url: str | None, + token_url: str | None, + registration_url: str | None, +) -> tuple[str | None, str | None, str | None]: + """The single rule that makes an admin-configured ``issuer`` the sole authoritative endpoint + source (RFC 8414 §3.3): when it is set for a discovery auth type, the stored/manual + ``authorization_url``/``token_url``/``registration_url`` do not apply. They neither anchor nor + short-circuit discovery, never override the issuer document in the merge, and never substitute for + it when the issuer fetch fails (fail-closed). Returns the endpoint values that remain in force, + i.e. all ``None`` when issuer-anchored, else the inputs unchanged. Called at every resolution site + so the invariant holds in one place instead of being re-derived per merge. + """ + if issuer is not None and is_discovery_auth_type: + return None, None, None + return authorization_url, token_url, registration_url + + def _normalized_authorize_endpoint(url: str) -> str: """Compare authorize endpoints on scheme, host, and path only. The default port is elided and the host is lowercased so ``https://IDP.example.com:443/authorize/`` and @@ -271,11 +291,23 @@ def _carry_forward_resolved_oauth_endpoints(new_server: MCPServer, previous_serv incoming build has no pinned authorize endpoint (``None`` -> we adopt the previous one too, a 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. """ 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): + # 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. + if not new_server.scopes and previous_server.scopes: + new_server.scopes = previous_server.scopes + return may_carry = _endpoints_corroborate_authorization_url( previous_server.authorization_url, new_server.authorization_url ) @@ -1154,6 +1186,13 @@ class MCPServerManager: 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 + 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, + ) should_discover = bool(server_url) and ( is_discovery_auth_type or self._obo_needs_endpoint_discovery( @@ -1506,6 +1545,52 @@ class MCPServerManager: decrypt_global_env_var_values(env_vars_list) return env_vars_list + async def _resolve_table_oauth_metadata( + self, + *, + mcp_server: LiteLLM_MCPServerTable, + auth_type: MCPAuthType, + server_url: Optional[str], + manual_issuer: Optional[str], + manual_authorization_url: Optional[str], + manual_token_url: Optional[str], + is_discovery_auth_type: bool, + use_issuer_anchor: bool, + scopes: Optional[list[str]], + token_exchange_endpoint: Optional[str], + ) -> Optional[MCPOAuthMetadata]: + has_all_upstream_oauth_fields = bool(manual_authorization_url and manual_token_url and scopes) + needs_discovery = bool(server_url) and ( + (is_discovery_auth_type and not has_all_upstream_oauth_fields) + or self._obo_needs_endpoint_discovery(auth_type, token_exchange_endpoint, manual_token_url) + ) + if not needs_discovery: + mcp_oauth_metadata: Optional[MCPOAuthMetadata] = None + elif use_issuer_anchor and manual_issuer is not None: + mcp_oauth_metadata = await self._fetch_issuer_anchored_oauth_metadata(manual_issuer, server_url) + else: + mcp_oauth_metadata = await self._descovery_metadata( + server_url=server_url, # type: ignore[arg-type] + allow_origin_fallback=is_discovery_auth_type, + ) + if needs_discovery and not use_issuer_anchor and mcp_oauth_metadata is None: + verbose_logger.warning( + "MCP OAuth discovery yielded no metadata for server %s (%s); " + "OAuth endpoints/scopes stay unresolved until a rebuild succeeds", + mcp_server.server_id, + server_url, + ) + if use_issuer_anchor: + return mcp_oauth_metadata + if is_discovery_auth_type: + return _restrict_discovery_to_corroborated_authorization_server( + mcp_oauth_metadata, + manual_authorization_url, + mcp_server.server_id, + bool(getattr(mcp_server, "dcr_bridge", None)), + ) + return mcp_oauth_metadata + async def build_mcp_server_from_table( self, mcp_server: LiteLLM_MCPServerTable, @@ -1595,46 +1680,24 @@ class MCPServerManager: 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 - has_all_upstream_oauth_fields = bool(manual_authorization_url and manual_token_url and scopes) - needs_discovery = bool(server_url) and ( - (is_discovery_auth_type and not has_all_upstream_oauth_fields) - or self._obo_needs_endpoint_discovery( - auth_type, - mcp_server.token_exchange_endpoint - or (credentials_dict.get("token_exchange_endpoint") if credentials_dict else None), - manual_token_url, - ) + 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 + ) + token_exchange_endpoint = mcp_server.token_exchange_endpoint or ( + credentials_dict.get("token_exchange_endpoint") if credentials_dict else None + ) + gated_oauth_metadata = await self._resolve_table_oauth_metadata( + mcp_server=mcp_server, + auth_type=auth_type, + server_url=server_url, + manual_issuer=manual_issuer, + manual_authorization_url=manual_authorization_url, + manual_token_url=manual_token_url, + is_discovery_auth_type=is_discovery_auth_type, + use_issuer_anchor=use_issuer_anchor, + scopes=scopes, + token_exchange_endpoint=token_exchange_endpoint, ) - if not needs_discovery: - mcp_oauth_metadata = None - elif manual_issuer is not None and is_discovery_auth_type: - mcp_oauth_metadata = await self._fetch_issuer_anchored_oauth_metadata( - manual_issuer, - server_url, # type: ignore[arg-type] - ) - else: - mcp_oauth_metadata = await self._descovery_metadata( - server_url=server_url, # type: ignore[arg-type] - allow_origin_fallback=is_discovery_auth_type, - ) - if needs_discovery and not use_issuer_anchor and mcp_oauth_metadata is None: - verbose_logger.warning( - "MCP OAuth discovery yielded no metadata for server %s (%s); " - "OAuth endpoints/scopes stay unresolved until a rebuild succeeds", - mcp_server.server_id, - server_url, - ) - if use_issuer_anchor: - gated_oauth_metadata = mcp_oauth_metadata - elif is_discovery_auth_type: - gated_oauth_metadata = _restrict_discovery_to_corroborated_authorization_server( - mcp_oauth_metadata, - manual_authorization_url, - mcp_server.server_id, - bool(getattr(mcp_server, "dcr_bridge", None)), - ) - else: - gated_oauth_metadata = mcp_oauth_metadata resolved_scopes = scopes or (gated_oauth_metadata.scopes if gated_oauth_metadata else None) @@ -1721,6 +1784,7 @@ class MCPServerManager: existing_token_url=manual_token_url, existing_scopes=scopes, metadata=gated_oauth_metadata, + is_issuer_anchored=use_issuer_anchor, ) return new_server @@ -1769,6 +1833,7 @@ class MCPServerManager: existing_token_url: str | None, existing_scopes: list[str] | None, metadata: MCPOAuthMetadata | None, + is_issuer_anchored: bool = False, ) -> None: """Write freshly discovered OAuth endpoints back onto the DB row. @@ -1782,6 +1847,12 @@ class MCPServerManager: because ``_dcr_bridge_relays_client_registration`` keys off that column. Best-effort: a failed write re-discovers on the next build. Scopes go through ``update_mcp_server`` so they merge into the credentials blob without touching the stored client credentials. + + For an issuer-anchored server (``is_issuer_anchored``) the endpoints are re-derived from the + §3.3-validated issuer document on every build, so they are NOT persisted into the endpoint + columns: persisting them would make the next build see populated endpoints and treat them as + authoritative stored values, defeating the "endpoints come solely from the issuer" invariant. + Only the resource-driven scopes are persisted for such servers. """ if auth_type not in _UPSTREAM_OAUTH_DISCOVERY_AUTH_TYPES: return @@ -1792,10 +1863,14 @@ class MCPServerManager: ) authorization_url_update = ( {"authorization_url": metadata.authorization_url} - if metadata.authorization_url and not existing_authorization_url + if metadata.authorization_url and not existing_authorization_url and not is_issuer_anchored + else {} + ) + token_url_update = ( + {"token_url": metadata.token_url} + if metadata.token_url and not existing_token_url and not is_issuer_anchored else {} ) - token_url_update = {"token_url": metadata.token_url} if metadata.token_url and not existing_token_url else {} scopes_update = {"credentials": {"scopes": metadata.scopes}} if metadata.scopes and not existing_scopes else {} updates: dict[str, object] = { **issuer_update, @@ -3375,7 +3450,9 @@ class MCPServerManager: return metadata return None - async def _fetch_issuer_anchored_oauth_metadata(self, issuer: str, server_url: str) -> Optional[MCPOAuthMetadata]: + async def _fetch_issuer_anchored_oauth_metadata( + self, issuer: str, server_url: Optional[str] + ) -> Optional[MCPOAuthMetadata]: """RFC 8414 issuer-anchored discovery for the OAuth endpoints, with resource-driven scopes. Fetch authorization-server metadata from the admin-configured issuer's own origin and adopt @@ -3400,7 +3477,9 @@ class MCPServerManager: issuer, ) return None - resource_metadata = await self._descovery_metadata(server_url, allow_origin_fallback=False) + resource_metadata = ( + await self._descovery_metadata(server_url, allow_origin_fallback=False) if server_url else None + ) resource_scopes = resource_metadata.scopes if resource_metadata else None return metadata.model_copy(update={"scopes": resource_scopes}) @@ -3488,8 +3567,6 @@ class MCPServerManager: ): return metadata - if require_issuer is not None: - return None return self._build_azure_authorization_server_metadata(parsed) @staticmethod @@ -5198,6 +5275,7 @@ class MCPServerManager: command=getattr(server, "command", None), args=getattr(server, "args", None) or [], env=getattr(server, "env", None) or {}, + issuer=server.issuer, authorization_url=server.authorization_url, token_url=server.token_url, registration_url=server.registration_url, @@ -5307,6 +5385,7 @@ class MCPServerManager: command=getattr(server, "command", None), args=getattr(server, "args", None) or [], env=getattr(server, "env", None) or {}, + issuer=server.issuer, authorization_url=server.authorization_url, token_url=server.token_url, registration_url=server.registration_url, diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index 288282dd08b..d920ee474cc 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -536,6 +536,7 @@ if MCP_AVAILABLE: sanitized.env = {} sanitized.command = None sanitized.args = [] + sanitized.issuer = None sanitized.authorization_url = None sanitized.token_url = None sanitized.registration_url = None @@ -581,6 +582,7 @@ if MCP_AVAILABLE: sanitized.teams = [] sanitized.env_vars = None + sanitized.issuer = None sanitized.authorization_url = None sanitized.token_url = None sanitized.registration_url = None @@ -686,6 +688,7 @@ if MCP_AVAILABLE: command=payload.command, args=payload.args, env=payload.env, + issuer=payload.issuer, authorization_url=payload.authorization_url, token_url=payload.token_url, registration_url=payload.registration_url, diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py index 637ee409fbe..0e53ebd76e1 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py @@ -239,6 +239,37 @@ async def test_url_change_clears_stale_discovered_oauth_fields(): assert data_dict[stale_field] is None, f"{stale_field} must be cleared on url change" +@pytest.mark.asyncio +async def test_url_change_clears_stale_oauth_fields_even_when_resubmitted_unchanged(): + """The edit form re-sends every field, so a URL change arrives WITH the previous upstream's issuer + and endpoints in the payload. Those resubmitted-unchanged values are stale and must still clear + (otherwise they survive the url change and win in the resolution merge). A genuinely new value the + caller changed in the same submit is kept.""" + mock_prisma = _mock_prisma() + existing = MagicMock() + existing.auth_type = "oauth2" + existing.url = "https://old.example.com/mcp" + existing.credentials = None + existing.issuer = "https://old-idp.example.com" + existing.token_url = "https://old-idp.example.com/token" + existing.authorization_url = "https://old-idp.example.com/authorize" + mock_prisma.db.litellm_mcpservertable.find_unique = AsyncMock(return_value=existing) + + data = UpdateMCPServerRequest( + server_id="my-test-server", + url="https://new.example.com/mcp", + issuer="https://old-idp.example.com", # resubmitted unchanged -> stale, must clear + token_url="https://old-idp.example.com/token", # resubmitted unchanged -> stale, must clear + authorization_url="https://new-idp.example.com/authorize", # genuinely changed -> kept + ) + await update_mcp_server(mock_prisma, data, "test-user") + data_dict = mock_prisma.db.litellm_mcpservertable.update.call_args[1]["data"] + + assert data_dict["issuer"] is None + assert data_dict["token_url"] is None + assert data_dict["authorization_url"] == "https://new-idp.example.com/authorize" + + @pytest.mark.asyncio async def test_unchanged_url_does_not_clear_discovered_oauth_fields(): """A partial update that resends the same url (or omits it) must not clear the discovered OAuth 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 75f77c1ec6d..1f966627aea 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 @@ -1338,6 +1338,50 @@ class TestMCPServerManager: assert built.registration_url is None assert built.scopes is None + @pytest.mark.asyncio + async def test_build_from_table_issuer_anchor_overrides_stored_endpoints_even_when_populated(self): + """When an issuer is pinned, the endpoints come SOLELY from the §3.3-validated issuer document + and win over any stored/manual endpoint values, even a fully-populated row. Otherwise an + attacker who controls a stored token endpoint keeps receiving codes/secrets after an admin + pins a trusted issuer: `needs_discovery` must not short-circuit on populated fields, and the + issuer's endpoints must override the stored ones.""" + manager = MCPServerManager() + row = LiteLLM_MCPServerTable( + server_id="issuer-anchored-populated", + alias="issuer_anchored_populated", + description="issuer set, but stale/hostile endpoints already stored", + url="https://up.example.com/mcp", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + issuer="https://idp.example.com", + authorization_url="https://attacker.example.com/authorize", + token_url="https://attacker.example.com/steal", + credentials={"scopes": ["stale"]}, + created_at=datetime.now(), + updated_at=datetime.now(), + ) + + issuer_resolved = MCPOAuthMetadata( + authorization_url="https://idp.example.com/authorize", + token_url="https://idp.example.com/token", + scopes=["read"], + ) + with ( + patch.object( + manager, "_fetch_issuer_anchored_oauth_metadata", new=AsyncMock(return_value=issuer_resolved) + ) as anchored, + patch.object(manager, "_persist_discovered_oauth_endpoints", new=AsyncMock()) as mock_persist, + ): + built = await manager.build_mcp_server_from_table(row, credentials_are_encrypted=False) + + anchored.assert_awaited_once_with("https://idp.example.com", "https://up.example.com/mcp") + assert built.authorization_url == "https://idp.example.com/authorize" + assert built.token_url == "https://idp.example.com/token" + assert built.token_url != "https://attacker.example.com/steal" + # The issuer-anchored endpoints are never persisted into the endpoint columns, so a later + # build cannot treat them as authoritative stored values. + assert mock_persist.await_args.kwargs["is_issuer_anchored"] is True + @pytest.mark.asyncio @pytest.mark.parametrize( "advertised_authorization_url", @@ -2669,6 +2713,37 @@ class TestMCPServerManager: assert result.authorization_url == "https://login.microsoftonline.com/test-tenant-id/oauth2/v2.0/authorize" assert result.token_url == "https://login.microsoftonline.com/test-tenant-id/oauth2/v2.0/token" + @pytest.mark.asyncio + async def test_azure_heuristic_reachable_under_require_issuer(self): + """Under issuer-anchored discovery (require_issuer set), an Entra issuer whose OIDC document + cannot be fetched still gets the deterministic Azure endpoint construction. The heuristic + derives the endpoints from the pinned issuer's own tenant URL, so it is authoritative-by- + construction and safe under require_issuer; only a non-Entra issuer stays fail-closed (None).""" + manager = MCPServerManager() + issuer = "https://login.microsoftonline.com/test-tenant-id/v2.0" + + request = httpx.Request("GET", issuer) + response_obj = httpx.Response(status_code=404, request=request) + mock_response = MagicMock() + mock_response.raise_for_status = MagicMock( + side_effect=httpx.HTTPStatusError("not found", request=request, response=response_obj) + ) + mock_client = MagicMock() + mock_client.get = AsyncMock(return_value=mock_response) + + with patch( + "litellm.proxy._experimental.mcp_server.mcp_server_manager.get_async_httpx_client", + return_value=mock_client, + ): + azure = await manager._fetch_single_authorization_server_metadata(issuer, issuer, require_issuer=issuer) + non_entra = await manager._fetch_single_authorization_server_metadata( + "https://idp.example.com", "https://idp.example.com", require_issuer="https://idp.example.com" + ) + + assert azure is not None + assert azure.token_url == "https://login.microsoftonline.com/test-tenant-id/oauth2/v2.0/token" + assert non_entra is None + @pytest.mark.asyncio async def test_descovery_metadata_falls_back_to_origin_when_no_auth_servers(self): manager = MCPServerManager() @@ -5484,6 +5559,41 @@ class TestMCPServerTimestamps: persisted = update_mcp_server_mock.call_args.kwargs["data"] assert persisted.issuer == "https://idp.example.com" + @pytest.mark.asyncio + async def test_persist_discovered_oauth_endpoints_does_not_persist_endpoints_for_issuer_anchored(self): + """For an issuer-anchored server the endpoints are re-derived from the §3.3-validated issuer + document every build, so they must NOT be written into the endpoint columns: persisting them + would make the next build see populated endpoints and treat them as authoritative stored + values, defeating the issuer-only invariant. Only the resource-driven scopes are persisted.""" + manager = MCPServerManager() + metadata = MCPOAuthMetadata( + authorization_url="https://idp.example.com/authorize", + token_url="https://idp.example.com/token", + scopes=["read"], + ) + + update_mcp_server_mock = AsyncMock() + with ( + patch("litellm.proxy._experimental.mcp_server.db.update_mcp_server", new=update_mcp_server_mock), + patch("litellm.proxy.proxy_server.prisma_client", MagicMock()), + ): + await manager._persist_discovered_oauth_endpoints( + server_id="s", + auth_type=MCPAuth.oauth2, + existing_issuer="https://idp.example.com", + existing_authorization_url=None, + existing_token_url=None, + existing_scopes=None, + metadata=metadata, + is_issuer_anchored=True, + ) + + update_mcp_server_mock.assert_awaited_once() + persisted = update_mcp_server_mock.call_args.kwargs["data"] + assert "authorization_url" not in persisted.fields_set() + assert "token_url" not in persisted.fields_set() + assert persisted.credentials == {"scopes": ["read"]} + @pytest.mark.asyncio async def test_build_mcp_server_from_table_skips_persistence_for_temporary_servers(self): """The session endpoint builds temporary servers whose server_id has no DB row; with @@ -5698,6 +5808,44 @@ class TestMCPServerTimestamps: assert same_authorize.token_url == "https://idp.example.com/token" 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 + 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.""" + 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", + authorization_url="https://idp.example.com/authorize", + token_url="https://idp.example.com/token", + registration_url="https://idp.example.com/register", + scopes=["read"], + ) + failed_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", + ) + + _carry_forward_resolved_oauth_endpoints(new_server=failed_rebuild, previous_server=previous) + + assert failed_rebuild.authorization_url is None + assert failed_rebuild.token_url is None + assert failed_rebuild.registration_url is None + assert failed_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 From ad73f3a7a282560cf6c654512b2f12366f24b666 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Wed, 15 Jul 2026 18:15:12 -0700 Subject: [PATCH 5/6] fix(mcp): keep issuer provenance consistent when it changes or is discovered Two lifecycle gaps let the issuer trust anchor drift out of sync with the endpoints it governs. Changing or clearing a previously pinned issuer left the authorization_url and token_url that were resolved under the old issuer in the row, so clearing the anchor could revive stale, possibly untrusted endpoints instead of re-discovering. And a build that discovered an issuer trust-on-first-use persisted it to the row while the returned in-memory server kept the issuer unset, so the registry and the row disagreed and the per-user OAuth token identity, which includes the issuer, differed between that build and the next rebuild and forced a spurious re-auth. update_mcp_server now treats a change to a previously pinned issuer the same as a url or auth_type change and clears the auth-flow-scoped endpoint fields that were resolved under it. The trigger fires only when an issuer was already pinned and is now changed or cleared, so establishing one for the first time, including the trust-on-first-use discovery write-back, does not wipe the fields it just resolved. Both build paths, build_mcp_server_from_table and load_servers_from_config, now construct the server with effective_issuer = manual_issuer or the discovered issuer, skipping an origin-fallback guess exactly as the persistence does, so the in-memory object always reflects what the row will hold. Regression tests pin each case: clearing and re-pointing a pinned issuer clear the stale endpoints, a first-time establish preserves the discovered fields, and a build reflects the discovered issuer while an origin-fallback guess is not reflected --- litellm/proxy/_experimental/mcp_server/db.py | 16 +++- .../mcp_server/mcp_server_manager.py | 16 +++- .../mcp_server/test_mcp_partial_update.py | 88 +++++++++++++++++++ .../mcp_server/test_mcp_server_manager.py | 60 +++++++++++++ 4 files changed, 176 insertions(+), 4 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/db.py b/litellm/proxy/_experimental/mcp_server/db.py index a21710892eb..d55eb3ac014 100644 --- a/litellm/proxy/_experimental/mcp_server/db.py +++ b/litellm/proxy/_experimental/mcp_server/db.py @@ -61,6 +61,13 @@ _AUTH_FLOW_SCOPED_FIELDS: frozenset = frozenset( } ) + +def _blank_to_none(value: Optional[str]) -> Optional[str]: + if not isinstance(value, str): + return None + return value.strip() or None + + # Token-exchange settings with dedicated columns that also exist on # ``MCPCredentials`` as a legacy shape (rows and REST callers that predate the # columns). Every write lifts blob values into the columns and strips them from @@ -705,7 +712,8 @@ async def update_mcp_server( # legacy blob copies below, so the existing row is needed for those updates. explicit_te_write = bool(_TOKEN_EXCHANGE_COLUMN_FIELDS & data_dict.keys()) url_provided = "url" in data_dict and data_dict["url"] is not None - if data.auth_type or has_credentials or explicit_te_write or url_provided: + issuer_provided = "issuer" in data_dict + if data.auth_type or has_credentials or explicit_te_write or url_provided or issuer_provided: existing = await MCPServerRepository(prisma_client).table.find_unique(where={"server_id": data.server_id}) auth_type_changed = bool( @@ -716,12 +724,16 @@ async def update_mcp_server( # A url change re-points the server at a potentially different upstream, so any discovered or # trust-on-first-use OAuth endpoints/issuer belong to the old upstream and must re-discover. url_changed = bool(url_provided and existing and existing.url != data_dict["url"]) + old_issuer = _blank_to_none(getattr(existing, "issuer", None)) if existing else None + issuer_changed = bool( + issuer_provided and old_issuer is not None and _blank_to_none(data_dict.get("issuer")) != old_issuer + ) # Clear stale credentials when auth_type changes but no new credentials provided if auth_type_changed and "credentials" not in data_dict: data_dict["credentials"] = None - if auth_type_changed or url_changed: + if auth_type_changed or url_changed or issuer_changed: # Clear each auth-flow-scoped field that the caller either omitted (partial update) or # resubmitted unchanged. The edit form re-sends every field, so a stale issuer/endpoint # belonging to the old upstream would otherwise survive a url/auth_type change and win in the diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index a547f5b6920..57cd755797b 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -1236,6 +1236,12 @@ class MCPServerManager: resolved_registration_url = manual_registration_url or ( gated_oauth_metadata.registration_url if gated_oauth_metadata else None ) + discovered_issuer = ( + gated_oauth_metadata.discovered_issuer + if gated_oauth_metadata and not gated_oauth_metadata.from_origin_fallback + else None + ) + effective_issuer = manual_issuer or discovered_issuer config_oauth2_flow = server_config.get("oauth2_flow", None) if auth_type == MCPAuth.oauth2 and config_oauth2_flow not in ( @@ -1284,7 +1290,7 @@ class MCPServerManager: client_secret=server_config.get("client_secret", None), oauth2_flow=self._explicit_oauth2_flow(config_oauth2_flow), scopes=resolved_scopes, - issuer=manual_issuer, + issuer=effective_issuer, authorization_url=resolved_authorization_url, token_url=resolved_token_url, registration_url=resolved_registration_url, @@ -1700,6 +1706,12 @@ class MCPServerManager: ) resolved_scopes = scopes or (gated_oauth_metadata.scopes if gated_oauth_metadata else None) + discovered_issuer = ( + gated_oauth_metadata.discovered_issuer + if gated_oauth_metadata and not gated_oauth_metadata.from_origin_fallback + else None + ) + effective_issuer = manual_issuer or discovered_issuer new_server = MCPServer( server_id=mcp_server.server_id, @@ -1719,7 +1731,7 @@ class MCPServerManager: client_secret=client_secret_value or getattr(mcp_server, "client_secret", None), oauth2_flow=self._explicit_oauth2_flow(getattr(mcp_server, "oauth2_flow", None)), scopes=resolved_scopes, - issuer=manual_issuer, + issuer=effective_issuer, 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/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py index 0e53ebd76e1..968fafc0e0e 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py @@ -270,6 +270,94 @@ async def test_url_change_clears_stale_oauth_fields_even_when_resubmitted_unchan assert data_dict["authorization_url"] == "https://new-idp.example.com/authorize" +@pytest.mark.asyncio +async def test_clearing_pinned_issuer_clears_stale_oauth_endpoints(): + """Clearing a previously pinned issuer must not revive the endpoints resolved under it. Under an + issuer anchor the endpoints come solely from the issuer document and are not persisted, but a row + that was resource-rooted before the pin can still hold stale authorization_url/token_url; clearing + the anchor without clearing those would let them win the resolution merge and be posted to without + fresh discovery (RFC 8414 §3.3 provenance).""" + mock_prisma = _mock_prisma() + existing = MagicMock() + existing.auth_type = "oauth2" + existing.url = "https://same.example.com/mcp" + existing.credentials = None + existing.issuer = "https://pinned-idp.example.com" + existing.token_url = "https://pinned-idp.example.com/token" + existing.authorization_url = "https://pinned-idp.example.com/authorize" + mock_prisma.db.litellm_mcpservertable.find_unique = AsyncMock(return_value=existing) + + data = UpdateMCPServerRequest( + server_id="my-test-server", + issuer="", # admin clears the anchor; url and auth_type unchanged + token_url="https://pinned-idp.example.com/token", + authorization_url="https://pinned-idp.example.com/authorize", + ) + await update_mcp_server(mock_prisma, data, "test-user") + data_dict = mock_prisma.db.litellm_mcpservertable.update.call_args[1]["data"] + + assert data_dict["token_url"] is None + assert data_dict["authorization_url"] is None + + +@pytest.mark.asyncio +async def test_repointing_pinned_issuer_clears_stale_endpoints_keeps_new_issuer(): + """Re-pointing the issuer to a different authorization server invalidates the old issuer's + endpoints while keeping the new issuer the admin submitted.""" + mock_prisma = _mock_prisma() + existing = MagicMock() + existing.auth_type = "oauth2" + existing.url = "https://same.example.com/mcp" + existing.credentials = None + existing.issuer = "https://old-idp.example.com" + existing.token_url = "https://old-idp.example.com/token" + existing.authorization_url = "https://old-idp.example.com/authorize" + mock_prisma.db.litellm_mcpservertable.find_unique = AsyncMock(return_value=existing) + + data = UpdateMCPServerRequest( + server_id="my-test-server", + issuer="https://new-idp.example.com", + token_url="https://old-idp.example.com/token", # resubmitted stale -> must clear + authorization_url="https://old-idp.example.com/authorize", # resubmitted stale -> must clear + ) + await update_mcp_server(mock_prisma, data, "test-user") + data_dict = mock_prisma.db.litellm_mcpservertable.update.call_args[1]["data"] + + assert data_dict["issuer"] == "https://new-idp.example.com" + assert data_dict["token_url"] is None + assert data_dict["authorization_url"] is None + + +@pytest.mark.asyncio +async def test_establishing_issuer_first_time_preserves_discovered_fields(): + """Establishing an issuer for the first time (None -> X), which is exactly what the trust-on-first-use + discovery write-back does, must NOT clear the endpoints or oauth2_flow it discovered in the same + write. Only an issuer that was already pinned and is now changed or cleared invalidates its + endpoints, so the discovery persist cannot wipe the fields it just resolved.""" + mock_prisma = _mock_prisma() + existing = MagicMock() + existing.auth_type = "oauth2" + existing.url = "https://same.example.com/mcp" + existing.credentials = None + existing.issuer = None + mock_prisma.db.litellm_mcpservertable.find_unique = AsyncMock(return_value=existing) + + data = UpdateMCPServerRequest( + server_id="my-test-server", + issuer="https://discovered-idp.example.com", + authorization_url="https://discovered-idp.example.com/authorize", + token_url="https://discovered-idp.example.com/token", + oauth2_flow="authorization_code", + ) + await update_mcp_server(mock_prisma, data, "mcp_oauth_discovery") + data_dict = mock_prisma.db.litellm_mcpservertable.update.call_args[1]["data"] + + assert data_dict["issuer"] == "https://discovered-idp.example.com" + assert data_dict["authorization_url"] == "https://discovered-idp.example.com/authorize" + assert data_dict["token_url"] == "https://discovered-idp.example.com/token" + assert data_dict.get("oauth2_flow") == "authorization_code" + + @pytest.mark.asyncio async def test_unchanged_url_does_not_clear_discovered_oauth_fields(): """A partial update that resends the same url (or omits it) must not clear the discovered OAuth 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 1f966627aea..b16840741af 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 @@ -1166,6 +1166,66 @@ class TestMCPServerManager: assert built.token_url == "https://idp.example.com/token" assert built.scopes == ["read", "admin"] + @pytest.mark.asyncio + async def test_build_from_table_reflects_discovered_issuer_trust_on_first_use(self): + """An unpinned server resolves endpoints resource-rooted on first discovery and records the + discovered issuer trust-on-first-use. The returned in-memory server must carry that discovered + issuer so the registry matches what gets persisted to the row; otherwise the OAuth token + identity (which includes issuer) differs between this build and the next rebuild, forcing a + spurious re-auth. Endpoints and issuer come from the same authorization-server document, so + they are consistent.""" + manager = MCPServerManager() + row = LiteLLM_MCPServerTable( + server_id="tofu-issuer-1", + alias="tofu_issuer", + description="unpinned, discovers its issuer", + url="https://up.example.com/mcp", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + created_at=datetime.now(), + updated_at=datetime.now(), + ) + + metadata = MCPOAuthMetadata( + authorization_url="https://idp.example.com/authorize", + token_url="https://idp.example.com/token", + scopes=["read"], + discovered_issuer="https://idp.example.com", + ) + with patch.object(manager, "_descovery_metadata", new=AsyncMock(return_value=metadata)): + built = await manager.build_mcp_server_from_table(row, credentials_are_encrypted=False) + + assert built.issuer == "https://idp.example.com" + assert built.authorization_url == "https://idp.example.com/authorize" + + @pytest.mark.asyncio + async def test_build_from_table_origin_fallback_issuer_is_not_reflected(self): + """An origin-fallback discovery is a guess that is deliberately never persisted, so the built + server must not claim an issuer the row will not hold; otherwise in-memory and DB would + disagree in the opposite direction.""" + manager = MCPServerManager() + row = LiteLLM_MCPServerTable( + server_id="origin-fallback-1", + alias="origin_fallback", + description="unpinned, origin-fallback discovery", + url="https://up.example.com/mcp", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + created_at=datetime.now(), + updated_at=datetime.now(), + ) + + metadata = MCPOAuthMetadata( + authorization_url="https://up.example.com/authorize", + token_url="https://up.example.com/token", + discovered_issuer="https://up.example.com", + from_origin_fallback=True, + ) + with patch.object(manager, "_descovery_metadata", new=AsyncMock(return_value=metadata)): + built = await manager.build_mcp_server_from_table(row, credentials_are_encrypted=False) + + assert built.issuer is None + @pytest.mark.asyncio async def test_build_from_table_whitespace_authorization_url_is_not_a_pin(self): """A whitespace-only authorization_url on the row must not be kept for redirects while the From afd7917b8b39ca66e48321f4fe182914720617c6 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Wed, 15 Jul 2026 23:11:37 -0700 Subject: [PATCH 6/6] 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