From 930676a9bf217b44ca57f1505676d7bda8d2caf5 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Mon, 20 Jul 2026 19:37:20 -0700 Subject: [PATCH] fix(mcp): let an admin-pinned issuer drive OAuth discovery for url-less servers --- .../mcp_server/discoverable_endpoints.py | 27 +++++- .../mcp_server/mcp_server_manager.py | 48 +++++++--- .../mcp_server/test_discoverable_endpoints.py | 37 ++++++++ .../mcp_server/test_mcp_server_manager.py | 94 ++++++++++++++++++- 4 files changed, 187 insertions(+), 19 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py index 882c34dbd6a..9a1b5cf4864 100644 --- a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py @@ -597,7 +597,14 @@ async def authorize_with_server( ): _raise_if_not_oauth2(mcp_server) if mcp_server.authorization_url is None: - raise HTTPException(status_code=400, detail="MCP server authorization url is not set") + raise HTTPException( + status_code=400, + detail=( + "MCP server authorization url is not configured. Servers with no url (OpenAPI " + "spec or stdio) run no resource discovery, so set Authorization URL and Token URL " + "manually, or set Issuer to discover them from the identity provider (RFC 8414)." + ), + ) if mcp_server.is_dcr_bridge: # Enforce S256 PKCE on both bridge arms. The relay arm forwards the validated, @@ -702,7 +709,14 @@ async def exchange_token_with_server( raise HTTPException(status_code=400, detail="Unsupported grant_type") if mcp_server.token_url is None: - raise HTTPException(status_code=400, detail="MCP server token url is not set") + raise HTTPException( + status_code=400, + detail=( + "MCP server token url is not configured. Servers with no url (OpenAPI spec or " + "stdio) run no resource discovery, so set Token URL manually, or set Issuer to " + "discover it from the identity provider (RFC 8414)." + ), + ) # The id and secret must come from the same source. When the server-side client_id wins, # falling back to the caller's secret pairs the persisted client with a foreign secret; the @@ -1262,7 +1276,14 @@ async def register_client_with_server( return dummy_return if mcp_server.authorization_url is None: - raise HTTPException(status_code=400, detail="MCP server authorization url is not set") + raise HTTPException( + status_code=400, + detail=( + "MCP server authorization url is not configured. Servers with no url (OpenAPI " + "spec or stdio) run no resource discovery, so set Authorization URL and Token URL " + "manually, or set Issuer to discover them from the identity provider (RFC 8414)." + ), + ) if mcp_server.registration_url is None: return dummy_return diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index ff714b8de22..90b70dd01f2 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -224,6 +224,20 @@ def _uses_issuer_anchor(manual_issuer: str | None, is_discovery_auth_type: bool) return _blank_to_none(manual_issuer) is not None and is_discovery_auth_type +def _has_oauth_discovery_source(server_url: str | None, use_issuer_anchor: bool) -> bool: + """Whether the server has any source OAuth discovery can fetch metadata from. + + Resource-rooted discovery (RFC 9728) is fetched from the server ``url``, so spec-only + (OpenAPI) and stdio servers, which have none, could never discover: their OAuth endpoints + stayed unset unless entered manually and ``/authorize`` served its 400 with no hint of why. + An admin-pinned issuer is a trust anchor in its own right (RFC 8414 section 3.3) whose + metadata fetch does not touch the resource at all, so an anchored server can discover with + no ``url``. Called by both build paths (config and DB) so the two cannot disagree on when + discovery is reachable. + """ + return bool(server_url) or use_issuer_anchor + + def _endpoints_yield_to_issuer( issuer: str | None, is_discovery_auth_type: bool, @@ -1254,7 +1268,12 @@ 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 = _uses_issuer_anchor(manual_issuer, is_discovery_auth_type) + obo_needs_discovery = self._obo_needs_endpoint_discovery( + auth_type, + server_config.get("token_exchange_endpoint"), + manual_token_url, + ) + use_issuer_anchor = _uses_issuer_anchor(manual_issuer, is_discovery_auth_type or obo_needs_discovery) manual_authorization_url, manual_token_url, manual_registration_url = _endpoints_yield_to_issuer( manual_issuer, is_discovery_auth_type, @@ -1262,17 +1281,12 @@ class MCPServerManager: manual_token_url, manual_registration_url, ) - 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, - ) + should_discover = _has_oauth_discovery_source(server_url, use_issuer_anchor) and ( + is_discovery_auth_type or obo_needs_discovery ) if not should_discover: mcp_oauth_metadata = None - elif manual_issuer is not None and is_discovery_auth_type: + 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( @@ -1668,7 +1682,7 @@ class MCPServerManager: 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 ( + needs_discovery = _has_oauth_discovery_source(server_url, use_issuer_anchor) 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) ) @@ -1787,13 +1801,17 @@ 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 = _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 - ) token_exchange_endpoint = mcp_server.token_exchange_endpoint or ( credentials_dict.get("token_exchange_endpoint") if credentials_dict else None ) + use_issuer_anchor = _uses_issuer_anchor( + manual_issuer, + is_discovery_auth_type + or self._obo_needs_endpoint_discovery(auth_type, token_exchange_endpoint, 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 + ) gated_oauth_metadata = await self._resolve_table_oauth_metadata( mcp_server=mcp_server, auth_type=auth_type, @@ -1971,7 +1989,7 @@ class MCPServerManager: family: discovered ``authorization_url``/``token_url``/``scopes`` otherwise live only on the in-memory registry entry, which is rebuilt on every client connect (the DCR reuse path calls ``update_server``) and on every post-write DB reload, so one failed re-discovery - serves 400 "authorization url is not set" from /authorize until a later rebuild succeeds. + serves the 400 "authorization url is not configured" from /authorize until a later rebuild succeeds. Only fills row fields that are currently empty, never persists origin-fallback guesses (RFC 9728/8414-advertised metadata only), and deliberately skips ``registration_url`` because ``_dcr_bridge_relays_client_registration`` keys off that column. Best-effort: a diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py index 0489b197652..5b1768ee563 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py @@ -8031,3 +8031,40 @@ async def test_bare_origin_discovery_resolves_single_server_not_aggregate(): assert resource_response["authorization_servers"] == ["https://llm.example.com/test_oauth"] finally: global_mcp_server_manager.registry.clear() + + +@pytest.mark.asyncio +async def test_authorize_wall_names_the_fix_for_urlless_servers(): + """LIT-4629: the authorize wall previously said only "authorization url is not set" with no + hint that spec-only servers never discover; the detail must now name both remedies (manual + Authorization URL + Token URL, or an Issuer for RFC 8414 discovery).""" + from litellm.proxy._experimental.mcp_server.discoverable_endpoints import ( + authorize_with_server, + ) + from litellm.types.mcp import MCPAuth, MCPTransport + from litellm.types.mcp_server.mcp_server_manager import MCPServer + + server = MCPServer( + server_id="urlless-wall", + name="sheets_wall", + server_name="sheets_wall", + url=None, + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + spec_path="https://example.com/openapi.yaml", + ) + mock_request = MagicMock() + mock_request.base_url = "https://litellm.example.com/" + mock_request.headers = {} + + with pytest.raises(HTTPException) as exc_info: + await authorize_with_server( + request=mock_request, + mcp_server=server, + client_id="client", + redirect_uri="http://localhost/callback", + ) + assert exc_info.value.status_code == 400 + detail_text = str(exc_info.value.detail) + assert "set Authorization URL and Token URL" in detail_text + assert "Issuer" in detail_text 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 576b9f4f139..a5cb16822cf 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 @@ -5597,7 +5597,7 @@ class TestMCPServerTimestamps: async def test_build_mcp_server_from_table_persists_discovered_oauth_endpoints(self): """A DB-backed oauth2 server with no configured endpoints discovers them and must write authorization_url, token_url, and scopes back to the row; otherwise the resolved values - live only in memory and one failed re-discovery serves 400 "authorization url is not set" + live only in memory and one failed re-discovery serves the 400 "authorization url is not configured" from /authorize. registration_url must never be persisted because _dcr_bridge_relays_client_registration keys off that column.""" manager = MCPServerManager() @@ -8936,3 +8936,95 @@ class TestMaterializeAuthHeaders: assert await _materialize_auth_headers(None) is None assert await _materialize_auth_headers(NoOpAuth()) is None + + +class TestUrllessIssuerDiscovery: + """LIT-4629: servers with no url (OpenAPI spec_path, stdio) run no resource discovery, so + their OAuth endpoints could only ever come from manual entry; an admin-pinned issuer is a + url-independent trust anchor (RFC 8414 section 3.3) and must unlock discovery for them.""" + + def _urlless_row(self, **overrides): + fields = dict( + server_id="urlless-1", + alias="sheets_urlless", + description="spec-only server", + url=None, + spec_path="https://example.com/sheets-openapi.yaml", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + created_at=datetime.now(), + updated_at=datetime.now(), + ) + fields.update(overrides) + return LiteLLM_MCPServerTable(**fields) + + @pytest.mark.asyncio + async def test_urlless_server_with_issuer_discovers_endpoints(self): + """The gate previously required bool(server_url), so a url-less server with an issuer + configured never ran the issuer-anchored fetch and /authorize 400d. Kills the mutant that + restores the bare bool(server_url) term.""" + manager = MCPServerManager() + row = self._urlless_row(issuer="https://accounts.google.com") + + resolved = MCPOAuthMetadata( + authorization_url="https://accounts.google.com/o/oauth2/v2/auth", + token_url="https://oauth2.googleapis.com/token", + ) + resource_rooted = AsyncMock(return_value=None) + with ( + 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://accounts.google.com", None) + resource_rooted.assert_not_awaited() + assert built.issuer_is_anchored is True + assert built.authorization_url == "https://accounts.google.com/o/oauth2/v2/auth" + assert built.token_url == "https://oauth2.googleapis.com/token" + + @pytest.mark.asyncio + async def test_urlless_server_without_issuer_stays_undiscovered(self): + """With neither a url nor an issuer there is no discovery source; the build must not + attempt any fetch and the endpoints stay unset (manual entry remains the only path).""" + manager = MCPServerManager() + row = self._urlless_row() + + anchored = AsyncMock() + resource_rooted = AsyncMock() + with ( + patch.object(manager, "_fetch_issuer_anchored_oauth_metadata", new=anchored), + patch.object(manager, "_descovery_metadata", new=resource_rooted), + ): + built = await manager.build_mcp_server_from_table(row, credentials_are_encrypted=False) + + anchored.assert_not_awaited() + resource_rooted.assert_not_awaited() + assert built.authorization_url is None + assert built.token_url is None + assert built.issuer_is_anchored is False + + @pytest.mark.asyncio + async def test_urlless_obo_with_issuer_discovers_token_url(self): + """oauth2_token_exchange is not a discovery auth type, so the plain gate relax alone + would leave a url-less OBO server undiscovered; with an issuer pinned and no configured + exchange endpoint it must resolve token_url through the issuer-anchored fetch. Kills the + mutant that drops the OBO widening from the anchor computation.""" + manager = MCPServerManager() + row = self._urlless_row( + alias="obo_urlless", + auth_type=MCPAuth.oauth2_token_exchange, + issuer="https://idp.example.com", + ) + + resolved = MCPOAuthMetadata(token_url="https://idp.example.com/token") + resource_rooted = AsyncMock(return_value=None) + with ( + 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", None) + resource_rooted.assert_not_awaited() + assert built.token_url == "https://idp.example.com/token"