From 8650f6c7d352ca079fe82fba3a84f1f5adcf3c91 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Wed, 15 Jul 2026 10:57:40 -0700 Subject: [PATCH] fix(mcp): bound pinned-config discovery to the corroborated authorization server, scopes included Provenance is a property of the whole discovered metadata document, not per field. Waving scopes through while gating endpoints left a second inflation vector: a compromised upstream advertises broad scopes via the resource metadata (RFC 9728 / WWW-Authenticate), the gateway requests them from the trusted authorization server, and the resulting token flows back to the upstream. Both that and the token-endpoint mix-up are now one rule: when authorization_url is admin-pinned, discovered token_url/registration_url are kept only if the document corroborates the pin, and scopes come from the authorization server's own scopes_supported (a new authorization_server_scopes field, trusted tier) rather than the resource-advertised scopes. A document that does not corroborate backfills nothing. Blank (empty-string) authorization_url is treated as unpinned so the merge and the gate agree. Carry-forward, the other non-manual source, drops the same three across an authorization_url change. --- .../mcp_server/mcp_server_manager.py | 51 ++++--- .../types/mcp_server/mcp_server_manager.py | 9 ++ .../mcp_server/test_mcp_server_manager.py | 137 ++++++++++++++---- 3 files changed, 147 insertions(+), 50 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index b84c5601560..2ace0d9b16a 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -219,7 +219,7 @@ def _endpoints_corroborate_authorization_url( value (``trusted_authorization_url is None``) there is nothing to protect: the authorize endpoint comes from the same source as the token endpoint, so they corroborate each other by construction. """ - if trusted_authorization_url is None: + if not (trusted_authorization_url and trusted_authorization_url.strip()): return True return bool(source_authorization_url) and _normalized_authorize_endpoint( source_authorization_url @@ -240,43 +240,53 @@ def _carry_forward_resolved_oauth_endpoints(new_server: MCPServer, previous_serv servers that never had one configured. Carry-forward is a non-manual endpoint source, so the same trust rule as discovery applies: the - previous ``token_url``/``registration_url`` are carried only when the previous + previous ``token_url``/``registration_url``/``scopes`` are carried only when the previous ``authorization_url`` corroborates the authorize endpoint this build will use, i.e. when the 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 token endpoint. + server must not keep serving the old server's token endpoint or granted scopes. """ if previous_server is None: return if previous_server.url != new_server.url or previous_server.auth_type != new_server.auth_type: return - may_carry_endpoints = _endpoints_corroborate_authorization_url( + may_carry = _endpoints_corroborate_authorization_url( previous_server.authorization_url, new_server.authorization_url ) if new_server.authorization_url is None and previous_server.authorization_url: new_server.authorization_url = previous_server.authorization_url - if may_carry_endpoints and new_server.token_url is None and previous_server.token_url: + if may_carry and new_server.token_url is None and previous_server.token_url: new_server.token_url = previous_server.token_url - if may_carry_endpoints and new_server.registration_url is None and previous_server.registration_url: + if may_carry and new_server.registration_url is None and previous_server.registration_url: new_server.registration_url = previous_server.registration_url - if not new_server.scopes and previous_server.scopes: + if may_carry and not new_server.scopes and previous_server.scopes: new_server.scopes = previous_server.scopes -def _gate_discovered_endpoints_against_manual_authorization_url( +def _restrict_discovery_to_corroborated_authorization_server( metadata: MCPOAuthMetadata | None, manual_authorization_url: str | None, server_identifier: str, is_dcr_bridge: bool, ) -> MCPOAuthMetadata | None: - """Apply :func:`_endpoints_corroborate_authorization_url` to freshly discovered metadata, the - other non-manual endpoint source. Drops the discovered ``token_url``/``registration_url`` (never - the scopes, which carry no credentials) when they cannot be vouched for by the pinned authorize - endpoint, logging why so an intentional mismatch can be resolved by setting Token URL by hand. + """Bound what freshly discovered metadata may backfill into a manually pinned config. + + Discovery is rooted at the MCP resource, so provenance is a property of the whole metadata + document, not per field: a compromised upstream can advertise both an attacker ``token_endpoint`` + (the RFC 9700 mix-up) and inflated ``scopes`` (tricking the user into granting a broader token + that then flows to the upstream). Both are closed by one rule. When ``authorization_url`` is + admin-pinned, the discovered ``token_url`` and ``registration_url`` are kept only if the document + corroborates the pin (its ``authorization_endpoint`` matches), and scopes are taken from the + authorization server's own ``scopes_supported`` (``authorization_server_scopes``, trusted tier) + rather than the resource-advertised ``scopes`` a compromised upstream controls. A document that + does not corroborate backfills nothing. With no pin there is no trust anchor to protect and the + authorize endpoint comes from the same chain as everything else, so discovery is returned as-is. """ - if metadata is None or (not metadata.token_url and not metadata.registration_url): + if metadata is None or not (manual_authorization_url and manual_authorization_url.strip()): return metadata if _endpoints_corroborate_authorization_url(metadata.authorization_url, manual_authorization_url): + return metadata.model_copy(update={"scopes": metadata.authorization_server_scopes}) + if not metadata.token_url and not metadata.registration_url and not metadata.scopes: return metadata bridge_note = ( " The discovered registration_url is rejected with it, so this dcr_bridge server stays on the" @@ -286,15 +296,15 @@ def _gate_discovered_endpoints_against_manual_authorization_url( ) verbose_logger.warning( "MCP OAuth discovery for server %s advertised authorization_endpoint %s, which does not match the " - "manually configured authorization_url %s; rejecting the discovered token_url/registration_url so " - "authorization codes and client credentials only go to endpoints vouched for by the configured " - "authorization server. Configure Token URL manually if the mismatch is intentional.%s", + "manually configured authorization_url %s; rejecting the discovered token_url/registration_url/scopes " + "so authorization codes, client credentials, and granted scopes only follow the configured " + "authorization server. Configure Token URL and Scopes manually if the mismatch is intentional.%s", server_identifier, _normalized_authorize_endpoint(metadata.authorization_url) if metadata.authorization_url else "", - _normalized_authorize_endpoint(manual_authorization_url) if manual_authorization_url else "", + _normalized_authorize_endpoint(manual_authorization_url), bridge_note, ) - return metadata.model_copy(update={"token_url": None, "registration_url": None}) + return metadata.model_copy(update={"token_url": None, "registration_url": None, "scopes": None}) def invalidate_user_env_vars_cache(user_id: str, server_id: str) -> None: @@ -1126,7 +1136,7 @@ class MCPServerManager: mcp_oauth_metadata = None gated_oauth_metadata = ( - _gate_discovered_endpoints_against_manual_authorization_url( + _restrict_discovery_to_corroborated_authorization_server( mcp_oauth_metadata, server_config.get("authorization_url"), server_name or server_id, @@ -1568,7 +1578,7 @@ class MCPServerManager: server_url, ) gated_oauth_metadata = ( - _gate_discovered_endpoints_against_manual_authorization_url( + _restrict_discovery_to_corroborated_authorization_server( mcp_oauth_metadata, mcp_server.authorization_url, mcp_server.server_id, @@ -3365,6 +3375,7 @@ class MCPServerManager: authorization_url=data.get("authorization_endpoint"), token_url=data.get("token_endpoint"), registration_url=data.get("registration_endpoint"), + authorization_server_scopes=scopes, ) if any( diff --git a/litellm/types/mcp_server/mcp_server_manager.py b/litellm/types/mcp_server/mcp_server_manager.py index 82ff15303f3..79a523f7c8b 100644 --- a/litellm/types/mcp_server/mcp_server_manager.py +++ b/litellm/types/mcp_server/mcp_server_manager.py @@ -17,9 +17,18 @@ MCPInfo = Dict[str, Any] class MCPOAuthMetadata(BaseModel): scopes: Optional[List[str]] = None + """Effective scopes, resource-preferred: the RFC 9728 protected-resource advertisement or the + WWW-Authenticate challenge when the resource supplied one, else the authorization server's + ``scopes_supported``. A compromised resource server can influence this, so it must not expand a + manually pinned ``authorization_url`` (see ``authorization_server_scopes``).""" authorization_url: Optional[str] = None token_url: Optional[str] = None registration_url: Optional[str] = None + authorization_server_scopes: Optional[List[str]] = None + """The ``scopes_supported`` enumerated by the authorization-server metadata document itself + (RFC 8414), independent of anything the resource server advertised. This is the only scope + source trusted to backfill a manually pinned ``authorization_url``, because it shares provenance + with the ``authorization_endpoint`` used to corroborate that pin.""" 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_server_manager.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py index 229b1bfb014..d14ae561f71 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 @@ -357,17 +357,18 @@ class TestMCPServerManager: assert server.needs_user_oauth_token is True @pytest.mark.asyncio - async def test_load_servers_from_config_rejects_discovered_token_url_on_authorization_endpoint_mismatch(self): + async def test_load_servers_from_config_rejects_uncorroborated_discovery_including_scopes(self): """The config loader always runs discovery and or-merges per field, so a yaml server with a - manual authorization_url has the same config-time mix-up exposure as a DB row: a discovered - token_url from a document advertising a different authorize endpoint must not be combined - with the pinned one. Scopes still backfill.""" + manual authorization_url has the same config-time mix-up exposure as a DB row: a document + advertising a different authorize endpoint backfills nothing, neither its token_url nor its + scopes.""" manager = MCPServerManager() metadata = MCPOAuthMetadata( authorization_url="https://attacker.example.com/authorize", token_url="https://attacker.example.com/token", - scopes=["read"], + scopes=["read", "admin"], + authorization_server_scopes=["read", "admin"], ) config = self._oauth2_config( oauth2_flow="authorization_code", @@ -380,19 +381,20 @@ class TestMCPServerManager: server = next(iter(manager.config_mcp_servers.values())) assert server.authorization_url == "https://idp.example.com/authorize" assert server.token_url is None - assert server.scopes == ["read"] + assert server.scopes is None @pytest.mark.asyncio async def test_load_servers_from_config_fills_token_url_when_metadata_corroborates_manual_authorization_url(self): """Corroborated metadata keeps the self-heal on the config path: when the discovered document advertises the same authorize endpoint the admin pinned, its token_url fills the - blank field.""" + blank field and scopes come from the authorization server's own scopes_supported.""" manager = MCPServerManager() metadata = MCPOAuthMetadata( authorization_url="https://idp.example.com/authorize", token_url="https://idp.example.com/token", - scopes=["read"], + scopes=["read", "admin"], + authorization_server_scopes=["read"], ) config = self._oauth2_config( oauth2_flow="authorization_code", @@ -404,6 +406,34 @@ class TestMCPServerManager: server = next(iter(manager.config_mcp_servers.values())) assert server.token_url == "https://idp.example.com/token" + 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.""" + manager = MCPServerManager() + + metadata = MCPOAuthMetadata( + authorization_url="https://idp.example.com/authorize", + token_url="https://idp.example.com/token", + scopes=["read"], + authorization_server_scopes=["read"], + ) + config = self._oauth2_config( + oauth2_flow="authorization_code", + authorization_url="", + token_url=None, + ) + with patch.object(manager, "_descovery_metadata", new=AsyncMock(return_value=metadata)): + await manager.load_servers_from_config(config) + + server = next(iter(manager.config_mcp_servers.values())) + assert server.authorization_url == "https://idp.example.com/authorize" + assert server.token_url == "https://idp.example.com/token" + assert server.scopes == ["read"] @pytest.mark.asyncio async def test_load_servers_from_config_non_oauth2_needs_no_flow(self): @@ -1102,12 +1132,12 @@ class TestMCPServerManager: assert built.token_url == "https://idp.example.com/token" @pytest.mark.asyncio - async def test_build_from_table_discovers_scopes_when_authorization_url_is_manual(self): - """An admin-typed authorization_url must not switch off discovery for the fields left - blank: without the scopes_supported backfill the authorize redirect goes out scope-less - and IdPs like Google hard-fail it with 400 "Missing required parameter: scope". Scope - backfill works even when the advertised authorization_endpoint differs from the manual - value, because scopes only steer the redirect to the trusted authorize endpoint.""" + async def test_build_from_table_backfills_scopes_from_authorization_server_not_resource(self): + """When authorization_url is admin-pinned, scopes backfill from the authorization server's + own scopes_supported (trusted tier), never from the resource-advertised scopes a compromised + upstream controls. Here the corroborating document carries an inflated resource `scopes` + (`admin`) alongside the real authorization_server_scopes; only the latter may be requested, + otherwise a hostile resource could trick the user into granting a broader token.""" manager = MCPServerManager() row = LiteLLM_MCPServerTable( server_id="manual-auth-url-1", @@ -1116,24 +1146,24 @@ class TestMCPServerManager: url="https://up.example.com/mcp", transport=MCPTransport.http, auth_type=MCPAuth.oauth2, - authorization_url="https://idp.example.com/manual-authorize", + authorization_url="https://idp.example.com/authorize", created_at=datetime.now(), updated_at=datetime.now(), ) metadata = MCPOAuthMetadata( - authorization_url="https://idp.example.com/discovered-authorize", + authorization_url="https://idp.example.com/authorize", token_url="https://idp.example.com/token", - registration_url=None, - scopes=["calendar.read", "calendar.write"], + scopes=["read", "admin"], + authorization_server_scopes=["read", "write"], ) with patch.object(manager, "_descovery_metadata", new=AsyncMock(return_value=metadata)) as mock_discovery: built = await manager.build_mcp_server_from_table(row, credentials_are_encrypted=False) mock_discovery.assert_awaited_once() - assert built.authorization_url == "https://idp.example.com/manual-authorize" - assert built.token_url is None - assert built.scopes == ["calendar.read", "calendar.write"] + assert built.authorization_url == "https://idp.example.com/authorize" + assert built.token_url == "https://idp.example.com/token" + assert built.scopes == ["read", "write"] @pytest.mark.asyncio async def test_build_from_table_fills_endpoints_when_metadata_corroborates_manual_authorization_url(self): @@ -1159,6 +1189,7 @@ class TestMCPServerManager: token_url="https://idp.example.com/token", registration_url="https://idp.example.com/register", 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) @@ -1173,15 +1204,15 @@ class TestMCPServerManager: "advertised_authorization_url", ["https://attacker.example.com/authorize", None], ) - async def test_build_from_table_rejects_discovered_token_url_on_authorization_endpoint_mismatch( + async def test_build_from_table_rejects_uncorroborated_discovery_including_scopes( self, advertised_authorization_url ): """Resource-rooted discovery lets a compromised upstream advertise its own authorization - server. With a manual authorization_url pinned, accepting that document's token_url would - send the authorization code, stored client secret, and PKCE verifier to the attacker's - token endpoint (config-time RFC 9700 mix-up), and the persist hook would make the hostile - endpoint durable. Both the in-memory merge and the persisted metadata must drop the - uncorroborated token_url and registration_url.""" + server. With a manual authorization_url pinned, a document that does not corroborate it + backfills nothing: accepting its token_url would send the code, client secret, and PKCE + verifier to the attacker (config-time RFC 9700 mix-up), and accepting its scopes would let + the upstream inflate the granted token. Both the in-memory merge and the persisted metadata + must drop the uncorroborated token_url, registration_url, and scopes.""" manager = MCPServerManager() row = LiteLLM_MCPServerTable( server_id="manual-auth-url-3", @@ -1199,7 +1230,8 @@ class TestMCPServerManager: authorization_url=advertised_authorization_url, token_url="https://attacker.example.com/token", registration_url="https://attacker.example.com/register", - scopes=["read"], + scopes=["read", "admin"], + authorization_server_scopes=["read", "admin"], ) with ( patch.object(manager, "_descovery_metadata", new=AsyncMock(return_value=metadata)), @@ -1210,11 +1242,11 @@ class TestMCPServerManager: assert built.authorization_url == "https://idp.example.com/authorize" assert built.token_url is None assert built.registration_url is None - assert built.scopes == ["read"] + assert built.scopes is None persisted_metadata = mock_persist.await_args.kwargs["metadata"] assert persisted_metadata.token_url is None assert persisted_metadata.registration_url is None - assert persisted_metadata.scopes == ["read"] + assert persisted_metadata.scopes is None @pytest.mark.asyncio async def test_build_from_table_skips_discovery_when_all_upstream_oauth_fields_present(self): @@ -2317,6 +2349,48 @@ class TestMCPServerManager: assert result.scopes == ["api://some-scope/.default"] assert result.from_origin_fallback is False + @pytest.mark.asyncio + async def test_descovery_metadata_preserves_authorization_server_scopes_under_resource_override(self): + """The effective `scopes` field is resource-preferred (RFC 9728 / WWW-Authenticate), but the + authorization server's own scopes_supported must survive on `authorization_server_scopes` so + the pinned-config backfill can request the trusted-tier scopes instead of resource-advertised + ones. This is the provenance split the scope-inflation defense depends on.""" + manager = MCPServerManager() + + mock_response = MagicMock() + mock_response.raise_for_status = MagicMock() + mock_client = MagicMock() + mock_client.get = AsyncMock(return_value=mock_response) + + authorization_server_metadata = MCPOAuthMetadata( + scopes=["as.read", "as.write"], + authorization_url="https://idp.example.com/authorize", + token_url="https://idp.example.com/token", + authorization_server_scopes=["as.read", "as.write"], + ) + + with ( + patch( + "litellm.proxy._experimental.mcp_server.mcp_server_manager.get_async_httpx_client", + return_value=mock_client, + ), + patch.object( + manager, + "_attempt_well_known_discovery", + AsyncMock(return_value=(["https://idp.example.com"], ["resource.only"])), + ), + patch.object( + manager, + "_fetch_authorization_server_metadata", + AsyncMock(return_value=authorization_server_metadata), + ), + ): + result = await manager._descovery_metadata("https://up.example.com/mcp") + + assert result is not None + assert result.scopes == ["resource.only"] + assert result.authorization_server_scopes == ["as.read", "as.write"] + @pytest.mark.asyncio async def test_fetch_single_authorization_server_metadata_supports_azure_issuer_path( self, @@ -2357,6 +2431,9 @@ 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" assert result.scopes == ["api://some-scope/.default"] + # The authorization server's own scopes_supported is retained under a dedicated field so a + # later resource-scope override cannot erase the trusted-tier value used to backfill a pin. + assert result.authorization_server_scopes == ["api://some-scope/.default"] @pytest.mark.asyncio async def test_fetch_single_authorization_server_metadata_derives_azure_metadata(