diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 2ace0d9b16a..e011c8f8e88 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -186,6 +186,21 @@ _UPSTREAM_OAUTH_DISCOVERY_AUTH_TYPES: tuple[MCPAuth, ...] = ( ) +def _blank_to_none(value: str | None) -> str | None: + """Collapse an absent, empty, or whitespace-only string to ``None``. + + OAuth endpoint fields are consumed by truthiness-based merges (``row or discovered``) and by the + corroboration gate. A whitespace-only value is truthy to ``or`` but is not a usable endpoint, so + without this the merge would keep the blank value for redirects while the gate treats it as + unpinned and backfills the other fields, yielding a broken half-discovered config. Normalizing + the pinned fields once, at each build entry point, gives every downstream consumer a single + notion of "blank" so those code paths cannot disagree. + """ + if not isinstance(value, str): + return None + return value.strip() or None + + 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 @@ -1120,12 +1135,15 @@ class MCPServerManager: ) auth_type = server_config.get("auth_type", None) + 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 or self._obo_needs_endpoint_discovery( auth_type, server_config.get("token_exchange_endpoint"), - server_config.get("token_url"), + manual_token_url, ) ): mcp_oauth_metadata = await self._descovery_metadata( @@ -1138,7 +1156,7 @@ class MCPServerManager: gated_oauth_metadata = ( _restrict_discovery_to_corroborated_authorization_server( mcp_oauth_metadata, - server_config.get("authorization_url"), + manual_authorization_url, server_name or server_id, bool(server_config.get("dcr_bridge")), ) @@ -1152,13 +1170,11 @@ class MCPServerManager: resolved_scopes = self._extract_scopes(server_config.get("scopes")) or ( gated_oauth_metadata.scopes if gated_oauth_metadata else None ) - resolved_authorization_url = server_config.get("authorization_url") or ( + resolved_authorization_url = manual_authorization_url or ( gated_oauth_metadata.authorization_url if gated_oauth_metadata else None ) - resolved_token_url = server_config.get("token_url") or ( - gated_oauth_metadata.token_url if gated_oauth_metadata else None - ) - resolved_registration_url = server_config.get("registration_url") or ( + resolved_token_url = manual_token_url or (gated_oauth_metadata.token_url if gated_oauth_metadata else None) + resolved_registration_url = manual_registration_url or ( gated_oauth_metadata.registration_url if gated_oauth_metadata else None ) @@ -1552,14 +1568,17 @@ class MCPServerManager: auth_type = cast(MCPAuthType, mcp_server.auth_type) server_url = mcp_server.url - has_all_upstream_oauth_fields = bool(mcp_server.authorization_url and mcp_server.token_url and scopes) + 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) + 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) 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), - mcp_server.token_url, + manual_token_url, ) ) mcp_oauth_metadata = ( @@ -1580,7 +1599,7 @@ class MCPServerManager: gated_oauth_metadata = ( _restrict_discovery_to_corroborated_authorization_server( mcp_oauth_metadata, - mcp_server.authorization_url, + manual_authorization_url, mcp_server.server_id, bool(getattr(mcp_server, "dcr_bridge", None)), ) @@ -1608,9 +1627,9 @@ 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, - authorization_url=mcp_server.authorization_url or getattr(gated_oauth_metadata, "authorization_url", None), - token_url=mcp_server.token_url or getattr(gated_oauth_metadata, "token_url", None), - registration_url=mcp_server.registration_url or getattr(gated_oauth_metadata, "registration_url", None), + 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), token_endpoint_auth_method=( credentials_dict.get("token_endpoint_auth_method") if credentials_dict else None ), @@ -1661,14 +1680,14 @@ class MCPServerManager: await self._persist_discovered_obo_token_url( server_id=mcp_server.server_id, auth_type=auth_type, - existing_token_url=mcp_server.token_url, + existing_token_url=manual_token_url, discovered_token_url=new_server.token_url, ) await self._persist_discovered_oauth_endpoints( server_id=mcp_server.server_id, auth_type=auth_type, - existing_authorization_url=mcp_server.authorization_url, - existing_token_url=mcp_server.token_url, + existing_authorization_url=manual_authorization_url, + existing_token_url=manual_token_url, existing_scopes=scopes, metadata=gated_oauth_metadata, ) 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 d14ae561f71..989281c27e9 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 @@ -409,11 +409,13 @@ class TestMCPServerManager: assert server.scopes == ["read"] @pytest.mark.asyncio - async def test_load_servers_from_config_blank_authorization_url_is_not_a_pin(self): - """A blank (empty-string) authorization_url is not a trust anchor, so discovery backfills - the whole set — authorize endpoint, token_url, and its resource-preferred scopes — from the - same chain, exactly as if the field had been omitted. The corroboration gate must treat - empty-string as unpinned so it does not strand the token_url the merge still fills.""" + @pytest.mark.parametrize("blank_authorization_url", ["", " "]) + async def test_load_servers_from_config_blank_authorization_url_is_not_a_pin(self, blank_authorization_url): + """A blank authorization_url — empty or whitespace-only — is not a trust anchor, so discovery + backfills the whole set (authorize endpoint, token_url, and its resource-preferred scopes) + from the same chain, exactly as if the field had been omitted. The merge and the corroboration + gate must agree that blank means unpinned; a whitespace value that the merge kept for redirects + while the gate treated as unpinned would strand a broken half-discovered config.""" manager = MCPServerManager() metadata = MCPOAuthMetadata( @@ -424,7 +426,7 @@ class TestMCPServerManager: ) config = self._oauth2_config( oauth2_flow="authorization_code", - authorization_url="", + authorization_url=blank_authorization_url, token_url=None, ) with patch.object(manager, "_descovery_metadata", new=AsyncMock(return_value=metadata)): @@ -1165,6 +1167,38 @@ class TestMCPServerManager: assert built.token_url == "https://idp.example.com/token" assert built.scopes == ["read", "write"] + @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 + gate treats it as unpinned. It is normalized to unpinned everywhere, so the built server + takes the discovered authorize endpoint, token_url, and scopes as one consistent group + rather than serving the whitespace value with half-discovered fields.""" + manager = MCPServerManager() + row = LiteLLM_MCPServerTable( + server_id="whitespace-auth-url", + alias="whitespace_auth_url", + description="whitespace authorization_url is not a pin", + url="https://up.example.com/mcp", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + authorization_url=" ", + 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"], + authorization_server_scopes=["read"], + ) + 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.authorization_url == "https://idp.example.com/authorize" + assert built.token_url == "https://idp.example.com/token" + assert built.scopes == ["read"] + @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