From e4a6516b4916eaae239ba9b147ae71b28b7a0022 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Wed, 15 Jul 2026 16:27:15 -0700 Subject: [PATCH] fix(mcp): keep scope selection resource-driven, not authorization-server-driven MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts the over-correction that restricted a pinned-authorization_url server's discovered scopes to the authorization server's own scopes_supported. Per the MCP authorization spec Scope Selection Strategy and RFC 9700 §2.3, the scopes a client requests are resource-driven: the WWW-Authenticate 401 challenge scope, else the RFC 9728 protected-resource scopes_supported. The authorization server's RFC 8414 scopes_supported is a non-exhaustive capability list (the server MAY omit supported scopes) and is never the selection source; scope inflation by a compromised resource is bounded by the authorization server and user consent (RFC 6749 §3.3), not by the client restricting the request. The corroboration gate now rejects only the uncorroborated token_url/registration_url (the RFC 9700 endpoint mix-up) and leaves scopes untouched. Removes the now-unused authorization_server_scopes field. --- .../mcp_server/mcp_server_manager.py | 37 ++++----- .../types/mcp_server/mcp_server_manager.py | 15 ++-- .../mcp_server/test_mcp_server_manager.py | 77 +++++++++---------- 3 files changed, 60 insertions(+), 69 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index e011c8f8e88..8b1d00c2855 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -284,24 +284,26 @@ def _restrict_discovery_to_corroborated_authorization_server( server_identifier: str, is_dcr_bridge: bool, ) -> MCPOAuthMetadata | None: - """Bound what freshly discovered metadata may backfill into a manually pinned config. + """Reject discovered token/registration endpoints a manually pinned authorize endpoint cannot + vouch for (the RFC 9700 authorization-server mix-up). - 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. + Discovery is rooted at the MCP resource, so a compromised upstream can advertise an attacker + ``token_endpoint``: with ``authorization_url`` admin-pinned but ``token_url`` blank, the merge + would pair the trusted authorize endpoint with that attacker token endpoint, and the gateway would + post the authorization code and client secret there. So the discovered ``token_url`` and + ``registration_url`` are kept only if the document corroborates the pin (its + ``authorization_endpoint`` matches). ``scopes`` are deliberately NOT gated here: per the MCP + authorization spec Scope Selection Strategy and RFC 9700 §2.3, the scopes a client requests are + resource-driven (the WWW-Authenticate challenge or the RFC 9728 protected-resource + ``scopes_supported``), and scope inflation by a compromised resource is bounded by the + authorization server and user consent (RFC 6749 §3.3), not by the client second-guessing the + request. With no pin there is no trust anchor to protect, so discovery is returned as-is. """ 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 + if not metadata.token_url and not metadata.registration_url: return metadata bridge_note = ( " The discovered registration_url is rejected with it, so this dcr_bridge server stays on the" @@ -311,15 +313,15 @@ def _restrict_discovery_to_corroborated_authorization_server( ) 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/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", + "manually configured authorization_url %s; rejecting the discovered token_url/registration_url so " + "authorization codes and client credentials only follow the configured authorization server. " + "Configure Token URL 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), bridge_note, ) - return metadata.model_copy(update={"token_url": None, "registration_url": None, "scopes": None}) + return metadata.model_copy(update={"token_url": None, "registration_url": None}) def invalidate_user_env_vars_cache(user_id: str, server_id: str) -> None: @@ -3394,7 +3396,6 @@ 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 79a523f7c8b..e5c726296b2 100644 --- a/litellm/types/mcp_server/mcp_server_manager.py +++ b/litellm/types/mcp_server/mcp_server_manager.py @@ -17,18 +17,15 @@ 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``).""" + """Resource-driven scopes for the authorization request: the RFC 9728 protected-resource + ``scopes_supported``, or the ``scope`` from the WWW-Authenticate 401 challenge when the resource + supplied one, else the authorization server's ``scopes_supported``. This is the scope value a + client requests per the MCP authorization spec Scope Selection Strategy; scope minimization and + inflation control are the authorization server's and user's job at consent (RFC 6749 §3.3), not + the client's.""" 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 989281c27e9..bba00ed1819 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,18 +357,18 @@ class TestMCPServerManager: assert server.needs_user_oauth_token is True @pytest.mark.asyncio - 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 document - advertising a different authorize endpoint backfills nothing, neither its token_url nor its - scopes.""" + async def test_load_servers_from_config_rejects_uncorroborated_endpoints_but_keeps_resource_scopes(self): + """A yaml server with a manual authorization_url has the same config-time mix-up exposure as a + DB row: a document advertising a different authorize endpoint has its token_url rejected. The + resource-driven scopes are kept, because scope selection is resource-driven (MCP Scope + Selection Strategy) and scope inflation is bounded by the authorization server at consent, not + by dropping scopes when an endpoint mismatches.""" manager = MCPServerManager() metadata = MCPOAuthMetadata( authorization_url="https://attacker.example.com/authorize", token_url="https://attacker.example.com/token", scopes=["read", "admin"], - authorization_server_scopes=["read", "admin"], ) config = self._oauth2_config( oauth2_flow="authorization_code", @@ -381,20 +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 is None + assert server.scopes == ["read", "admin"] @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 and scopes come from the authorization server's own scopes_supported.""" + """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 + and scopes come through resource-driven (the discovered document's resource-preferred scopes), + not the authorization server's own capability list.""" manager = MCPServerManager() metadata = MCPOAuthMetadata( authorization_url="https://idp.example.com/authorize", token_url="https://idp.example.com/token", scopes=["read", "admin"], - authorization_server_scopes=["read"], ) config = self._oauth2_config( oauth2_flow="authorization_code", @@ -406,7 +406,7 @@ class TestMCPServerManager: server = next(iter(manager.config_mcp_servers.values())) assert server.token_url == "https://idp.example.com/token" - assert server.scopes == ["read"] + assert server.scopes == ["read", "admin"] @pytest.mark.asyncio @pytest.mark.parametrize("blank_authorization_url", ["", " "]) @@ -422,7 +422,6 @@ class TestMCPServerManager: 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", @@ -1134,12 +1133,13 @@ class TestMCPServerManager: assert built.token_url == "https://idp.example.com/token" @pytest.mark.asyncio - 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.""" + async def test_build_from_table_backfills_resource_driven_scopes_for_pinned_authorization_url(self): + """When authorization_url is admin-pinned and corroborated, scopes backfill as the + resource-driven value (the WWW-Authenticate challenge scope, else the RFC 9728 + protected-resource scopes_supported), per the MCP authorization spec Scope Selection Strategy. + The client does not restrict scopes to the authorization server's own scopes_supported; scope + minimization and inflation control are the authorization server's and user's job at consent + (RFC 6749 §3.3).""" manager = MCPServerManager() row = LiteLLM_MCPServerTable( server_id="manual-auth-url-1", @@ -1157,7 +1157,6 @@ class TestMCPServerManager: authorization_url="https://idp.example.com/authorize", token_url="https://idp.example.com/token", 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) @@ -1165,7 +1164,7 @@ class TestMCPServerManager: mock_discovery.assert_awaited_once() assert built.authorization_url == "https://idp.example.com/authorize" assert built.token_url == "https://idp.example.com/token" - assert built.scopes == ["read", "write"] + assert built.scopes == ["read", "admin"] @pytest.mark.asyncio async def test_build_from_table_whitespace_authorization_url_is_not_a_pin(self): @@ -1190,7 +1189,6 @@ class TestMCPServerManager: 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) @@ -1223,7 +1221,6 @@ 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) @@ -1238,15 +1235,17 @@ class TestMCPServerManager: "advertised_authorization_url", ["https://attacker.example.com/authorize", None], ) - async def test_build_from_table_rejects_uncorroborated_discovery_including_scopes( + async def test_build_from_table_rejects_uncorroborated_endpoints_but_keeps_resource_scopes( self, advertised_authorization_url ): """Resource-rooted discovery lets a compromised upstream advertise its own authorization - 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.""" + server. With a manual authorization_url pinned, a document that does not corroborate it has + its token_url and registration_url dropped: accepting them would send the code, client secret, + and PKCE verifier to the attacker (config-time RFC 9700 mix-up). The resource-driven scopes + are kept, because scope selection is resource-driven (MCP Scope Selection Strategy) and scope + inflation is bounded by the authorization server at consent (RFC 6749 §3.3), not by dropping + scopes on an endpoint mismatch. Both the in-memory merge and the persisted metadata drop only + the uncorroborated endpoints.""" manager = MCPServerManager() row = LiteLLM_MCPServerTable( server_id="manual-auth-url-3", @@ -1265,7 +1264,6 @@ class TestMCPServerManager: token_url="https://attacker.example.com/token", registration_url="https://attacker.example.com/register", scopes=["read", "admin"], - authorization_server_scopes=["read", "admin"], ) with ( patch.object(manager, "_descovery_metadata", new=AsyncMock(return_value=metadata)), @@ -1276,11 +1274,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 is None + assert built.scopes == ["read", "admin"] 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 is None + assert persisted_metadata.scopes == ["read", "admin"] @pytest.mark.asyncio async def test_build_from_table_skips_discovery_when_all_upstream_oauth_fields_present(self): @@ -2384,11 +2382,11 @@ class TestMCPServerManager: 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.""" + async def test_descovery_metadata_scopes_are_resource_driven(self): + """The effective `scopes` are resource-driven: the RFC 9728 protected-resource advertisement + (or WWW-Authenticate challenge) overrides the authorization server's own scopes_supported. This + is the MCP Scope Selection Strategy: the client requests what the resource needs, not the AS's + full capability list.""" manager = MCPServerManager() mock_response = MagicMock() @@ -2400,7 +2398,6 @@ class TestMCPServerManager: 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 ( @@ -2423,7 +2420,6 @@ class TestMCPServerManager: 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( @@ -2465,9 +2461,6 @@ 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(