From 58f5998f0fdcd1afa810debc1f7bd6f958b93f5c Mon Sep 17 00:00:00 2001 From: mubashir1osmani Date: Mon, 3 Aug 2026 16:40:58 -0700 Subject: [PATCH] fix(mcp/oauth): rediscover interactive endpoints when authorize is empty Interactive PKCE should resolve authorize/token from upstream OAuth discovery (RFC 9728/8414) without admins pasting URLs. A transient discovery miss on oauth/session was cached and /authorize hard-400'd forever. Re-run discovery before failing authorize/register, retry on temp session cache, surface discovered fields on the session response, stamp oauth2_flow on temp records, and cover the Figma-style GET 405 without WWW-Authenticate path --- .../mcp_server/discoverable_endpoints.py | 18 +++ .../mcp_server/mcp_server_manager.py | 69 +++++++++++ .../mcp_management_endpoints.py | 18 +++ .../mcp_server/test_discoverable_endpoints.py | 73 +++++++++++ .../mcp_server/test_mcp_server_manager.py | 113 ++++++++++++++++++ .../_components/CreateMCPServer.tsx | 11 ++ 6 files changed, 302 insertions(+) diff --git a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py index 40844ec1937..a426ae03966 100644 --- a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py @@ -755,6 +755,12 @@ async def authorize_with_server( ephemeral_dcr_client: "EphemeralDcrClient | None" = None, ): _raise_if_not_oauth2(mcp_server) + if mcp_server.authorization_url is None: + from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( # noqa: PLC0415 + global_mcp_server_manager, + ) + + await global_mcp_server_manager.ensure_oauth_endpoints_resolved(mcp_server) if mcp_server.authorization_url is None: raise HTTPException( status_code=400, @@ -1552,6 +1558,12 @@ async def resolve_ephemeral_dcr_client( usable to generate orphan IdP clients).""" if not (mcp_server.is_true_passthrough or (mcp_server.is_oauth_delegate and not mcp_server.is_dcr_bridge)): return None + if mcp_server.authorization_url is None: + from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( # noqa: PLC0415 + global_mcp_server_manager, + ) + + await global_mcp_server_manager.ensure_oauth_endpoints_resolved(mcp_server) if mcp_server.authorization_url is None: raise HTTPException( status_code=400, @@ -1596,6 +1608,12 @@ async def register_client_with_server( ): return dummy_return + if mcp_server.authorization_url is None or mcp_server.registration_url is None: + from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( # noqa: PLC0415 + global_mcp_server_manager, + ) + + await global_mcp_server_manager.ensure_oauth_endpoints_resolved(mcp_server) if mcp_server.authorization_url is None: raise HTTPException( status_code=400, diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 66a55f30b74..fab364d7af0 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -3797,6 +3797,15 @@ class MCPServerManager: ), ) if metadata is None and resource_scopes: + verbose_logger.warning( + "MCP OAuth discovery for %s found resource scopes %s but no authorization/token " + "endpoints. Attempts: %s. Interactive OAuth will fail until discovery resolves " + "the authorization server (RFC 9728 / RFC 8414), or Authorization URL and Token " + "URL are set manually", + origin, + resource_scopes, + "; ".join(attempts) if attempts else "none recorded", + ) return MCPOAuthMetadata(scopes=resource_scopes), attempts if metadata is not None and resource_scopes: metadata.scopes = resource_scopes @@ -3882,12 +3891,72 @@ class MCPServerManager: preferred_scopes = scopes or resource_scopes if metadata is None and preferred_scopes: + verbose_logger.warning( + "MCP OAuth discovery for %s found resource scopes %s but no authorization/token " + "endpoints. Attempts: %s. Interactive OAuth will fail until discovery resolves " + "the authorization server (RFC 9728 / RFC 8414), or Authorization URL and Token " + "URL are set manually", + origin, + preferred_scopes, + "; ".join(attempts) if attempts else "none recorded", + ) return MCPOAuthMetadata(scopes=preferred_scopes), attempts if metadata is not None and preferred_scopes: metadata.scopes = preferred_scopes return metadata, attempts + def _apply_discovered_oauth_metadata(self, server: MCPServer, metadata: MCPOAuthMetadata) -> None: + """Fill blank OAuth fields on ``server`` from discovery without overwriting admin pins.""" + if not server.authorization_url and metadata.authorization_url: + server.authorization_url = metadata.authorization_url + if not server.token_url and metadata.token_url: + server.token_url = metadata.token_url + if not server.registration_url and metadata.registration_url: + server.registration_url = metadata.registration_url + if not server.scopes and metadata.scopes: + server.scopes = list(metadata.scopes) + if ( + not server.issuer + and metadata.discovered_issuer + and not metadata.from_origin_fallback + ): + server.issuer = metadata.discovered_issuer + + async def ensure_oauth_endpoints_resolved(self, server: MCPServer) -> bool: + """Re-run OAuth discovery when interactive endpoints are missing on a live server object. + + Discovery normally runs only at build/cache time (``build_mcp_server_from_table`` / + ``oauth/session``). A transient upstream failure leaves ``authorization_url`` / + ``token_url`` unset, and ``/authorize`` would otherwise hard-400 for the life of that + cache entry even though the upstream advertises valid well-known metadata (e.g. Figma + MCP returns HTTP 405 without ``WWW-Authenticate`` on GET, then resolves via + ``/.well-known/oauth-protected-resource``). Call this before failing authorize/register + so a live rediscovery can recover without the admin pasting URLs. + """ + if not _oauth_endpoints_unresolved(server): + return True + if not server.url: + return False + if server.auth_type not in _UPSTREAM_OAUTH_DISCOVERY_AUTH_TYPES: + return False + + if server.issuer_is_anchored and server.issuer: + metadata = await self._fetch_issuer_anchored_oauth_metadata(server.issuer, server.url) + else: + metadata = await self._descovery_metadata( + server.url, + allow_origin_fallback=True, + warn_when_no_metadata=True, + ) + if metadata is None: + self._record_oauth_discovery_outcome(server) + return False + + self._apply_discovered_oauth_metadata(server, metadata) + self._record_oauth_discovery_outcome(server) + return not _oauth_endpoints_unresolved(server) + def _parse_www_authenticate_header(self, header_value: str | None) -> tuple[str | None, list[str] | None]: if not header_value: return None, None diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index 6a212dd3bf9..5d599511bd7 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -741,6 +741,7 @@ if MCP_AVAILABLE: authorization_url=payload.authorization_url, token_url=payload.token_url, registration_url=payload.registration_url, + oauth2_flow=payload.oauth2_flow, allow_all_keys=payload.allow_all_keys, available_on_public_internet=payload.available_on_public_internet, timeout=payload.timeout, @@ -1550,6 +1551,23 @@ if MCP_AVAILABLE: temp_record, credentials_are_encrypted=False, ) + # Interactive OAuth needs authorize/token endpoints. Discovery can fail once + # (transient upstream / 405 without WWW-Authenticate then well-known race); retry + # before caching so /authorize does not inherit a permanently empty session. + await global_mcp_server_manager.ensure_oauth_endpoints_resolved(temporary_server) + # Surface discovered endpoints on the response so the UI can show what was resolved + # without requiring the admin to paste Authorization URL / Token URL by hand. + temp_record.authorization_url = temporary_server.authorization_url + temp_record.token_url = temporary_server.token_url + temp_record.registration_url = temporary_server.registration_url + temp_record.issuer = temporary_server.issuer + if temporary_server.scopes and isinstance(temp_record.credentials, dict): + temp_record.credentials = { + **temp_record.credentials, + "scopes": list(temporary_server.scopes), + } + elif temporary_server.scopes and temp_record.credentials is None: + temp_record.credentials = {"scopes": list(temporary_server.scopes)} _cache_temporary_mcp_server( temporary_server, ttl_seconds=TEMPORARY_MCP_SERVER_TTL_SECONDS, 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 9bc84b43fc5..24d36f1c4d3 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 @@ -8265,6 +8265,79 @@ async def test_authorize_wall_names_the_fix_for_urlless_servers(): assert "Issuer" in detail_text +@pytest.mark.asyncio +async def test_authorize_rediscovers_endpoints_when_authorization_url_missing(): + """Interactive PKCE must not require the admin to paste Authorization URL when the upstream + advertises it via OAuth discovery. If the session/cache build left authorization_url empty, + /authorize re-runs discovery once and proceeds with the discovered redirect.""" + from urllib.parse import parse_qs, urlparse + + 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="rediscover-authorize", + name="figma_like", + server_name="figma_like", + url="https://mcp.figma.example/mcp", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + oauth2_flow="authorization_code", + authorization_url=None, + token_url=None, + client_id="figma-client", + ) + mock_request = MagicMock() + mock_request.base_url = "https://litellm.example.com/" + mock_request.headers = {} + mock_request.url = MagicMock() + mock_request.url.scheme = "https" + mock_request.url.netloc = "litellm.example.com" + mock_request.cookies = {} + + async def _fake_ensure(s): + s.authorization_url = "https://www.figma.example/oauth/mcp" + s.token_url = "https://api.figma.example/v1/oauth/token" + s.scopes = ["mcp:connect"] + return True + + with ( + patch( + "litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager.ensure_oauth_endpoints_resolved", + new=AsyncMock(side_effect=_fake_ensure), + ), + patch( + "litellm.proxy._experimental.mcp_server.discoverable_endpoints.encrypt_value_helper", + return_value="mocked_encrypted_state", + ), + patch( + "litellm.proxy._experimental.mcp_server.discoverable_endpoints.validate_trusted_redirect_uri", + return_value=None, + ), + ): + response = await authorize_with_server( + request=mock_request, + mcp_server=server, + client_id="figma-client", + redirect_uri="http://127.0.0.1:60108/callback", + state="s", + code_challenge="chal", + code_challenge_method="S256", + scope="mcp:connect", + ) + + assert response.status_code in (302, 307) + location = response.headers["location"] + assert location.startswith("https://www.figma.example/oauth/mcp") + query = parse_qs(urlparse(location).query) + assert query["client_id"] == ["figma-client"] + assert query["scope"] == ["mcp:connect"] + assert server.authorization_url == "https://www.figma.example/oauth/mcp" + + @pytest.mark.asyncio async def test_token_wall_names_the_fix_for_urlless_servers(): """The /token wall is the second stop on the same misconfiguration (LIT-4629): after an admin 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 54fd5242d5f..166f4c0c29a 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 @@ -1431,6 +1431,119 @@ class TestMCPServerManager: assert built.token_url == "https://idp.example.com/token" assert built.scopes == ["read"] + @pytest.mark.asyncio + async def test_ensure_oauth_endpoints_resolved_rediscovers_when_authorization_url_missing(self): + """A temp oauth/session or registry entry that was built while discovery failed must recover + on the next ensure call without the admin pasting Authorization URL / Token URL. This is the + recovery path used by /authorize for Figma-style upstreams (GET 405, well-known still works).""" + manager = MCPServerManager() + server = MCPServer( + server_id="ensure-rediscover-1", + name="figma_like", + url="https://mcp.example.com/mcp", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + oauth2_flow="authorization_code", + authorization_url=None, + token_url=None, + ) + metadata = MCPOAuthMetadata( + authorization_url="https://www.example.com/oauth/mcp", + token_url="https://api.example.com/v1/oauth/token", + registration_url="https://api.example.com/v1/oauth/mcp/register", + scopes=["mcp:connect"], + discovered_issuer="https://api.example.com", + ) + with patch.object(manager, "_descovery_metadata", new=AsyncMock(return_value=metadata)) as mock_discovery: + ok = await manager.ensure_oauth_endpoints_resolved(server) + + mock_discovery.assert_awaited_once() + assert ok is True + assert server.authorization_url == "https://www.example.com/oauth/mcp" + assert server.token_url == "https://api.example.com/v1/oauth/token" + assert server.registration_url == "https://api.example.com/v1/oauth/mcp/register" + assert server.scopes == ["mcp:connect"] + assert server.issuer == "https://api.example.com" + + @pytest.mark.asyncio + async def test_ensure_oauth_endpoints_resolved_noops_when_endpoints_present(self): + manager = MCPServerManager() + server = MCPServer( + server_id="ensure-noop-1", + name="already_resolved", + url="https://mcp.example.com/mcp", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + authorization_url="https://idp.example.com/authorize", + token_url="https://idp.example.com/token", + ) + with patch.object(manager, "_descovery_metadata", new=AsyncMock()) as mock_discovery: + ok = await manager.ensure_oauth_endpoints_resolved(server) + + mock_discovery.assert_not_awaited() + assert ok is True + + @pytest.mark.asyncio + async def test_discover_metadata_figma_style_405_without_www_authenticate_uses_well_known(self): + """Figma remote MCP answers GET /mcp with HTTP 405 and no WWW-Authenticate. Discovery must + still resolve authorize/token/registration via protected-resource + authorization-server + well-known documents so interactive PKCE does not require manual endpoint paste.""" + manager = MCPServerManager() + resource_payload = { + "resource": "https://mcp.figma.example/mcp", + "authorization_servers": ["https://api.figma.example"], + "scopes_supported": ["mcp:connect"], + } + as_payload = { + "issuer": "https://api.figma.example", + "authorization_endpoint": "https://www.figma.example/oauth/mcp", + "token_endpoint": "https://api.figma.example/v1/oauth/token", + "registration_endpoint": "https://api.figma.example/v1/oauth/mcp/register", + "scopes_supported": ["mcp:connect"], + } + + def _response(url: str, status: int, payload: dict | None = None) -> httpx.Response: + request = httpx.Request("GET", url) + return httpx.Response( + status, + request=request, + json=payload, + headers={"content-type": "application/json"} if payload is not None else {}, + ) + + async def fake_client_get(url: str, **kwargs): + if url.rstrip("/").endswith("/mcp") and "well-known" not in url: + return _response(url, 405) + raise AssertionError(f"unexpected client.get url: {url}") + + async def fake_fetch_discovery(url: str, server_url: str): + if "oauth-protected-resource" in url: + return _response(url, 200, resource_payload) + if "oauth-authorization-server" in url or url.rstrip("/") == "https://api.figma.example": + return _response(url, 200, as_payload) + return _response(url, 404) + + mock_client = MagicMock() + mock_client.get = AsyncMock(side_effect=fake_client_get) + with ( + patch( + "litellm.proxy._experimental.mcp_server.mcp_server_manager.get_async_httpx_client", + return_value=mock_client, + ), + patch.object(manager, "_fetch_oauth_discovery_url", new=AsyncMock(side_effect=fake_fetch_discovery)), + ): + metadata = await manager._descovery_metadata( + "https://mcp.figma.example/mcp", + warn_when_no_metadata=True, + ) + + assert metadata is not None + assert metadata.authorization_url == "https://www.figma.example/oauth/mcp" + assert metadata.token_url == "https://api.figma.example/v1/oauth/token" + assert metadata.registration_url == "https://api.figma.example/v1/oauth/mcp/register" + assert metadata.scopes == ["mcp:connect"] + assert metadata.discovered_issuer == "https://api.figma.example" + @pytest.mark.asyncio async def test_build_from_table_fills_endpoints_when_metadata_corroborates_manual_authorization_url(self): """A discovered token_url is only trusted next to a manual authorization_url when the same diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/CreateMCPServer.tsx b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/CreateMCPServer.tsx index 0785dd142ff..75795c2fe9c 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/CreateMCPServer.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/CreateMCPServer.tsx @@ -192,6 +192,17 @@ const CreateMCPServer: React.FC = ({ url, transport: transport === TRANSPORT.OPENAPI ? "http" : transport, auth_type: isClientForwardedTokenMode(values.auth_type) ? values.auth_type : AUTH_TYPE.OAUTH2, + // Interactive (PKCE) is the create-form default for oauth2; stamp it so temp-session + // discovery/authorize treat the server as authorization_code rather than inferring M2M from + // a prefilled client_id/secret + token_url shape. Client-forwarded modes leave it unset. + ...(isClientForwardedTokenMode(values.auth_type) + ? {} + : { + oauth2_flow: + values.oauth_flow_type === OAUTH_FLOW.M2M + ? MCP_OAUTH2_FLOW_M2M + : MCP_OAUTH2_FLOW_INTERACTIVE, + }), // Mirror getCredentials: merge the ref-held DCR client for oauth2 so a re-authorize reuses the // registered client (useMcpOAuthFlow keys reuse off credentials.client_id) instead of re-DCRing; // the client-forwarded modes carry only the declared app.