From 50f60e4dd8c85e94a205331e77b1a13a62e92757 Mon Sep 17 00:00:00 2001 From: Gaurav Pandey <112387553+gaurav-pandey-zocdoc@users.noreply.github.com> Date: Sat, 17 Oct 2026 00:32:31 +0530 Subject: [PATCH 1/2] fix(mcp): cap client-requested OAuth scopes at the server's configured scopes When an MCP server has scopes configured, a client could still ask the upstream for any scope by sending its own scope parameter, because the configured list was only a default. The gateway now intersects the client's scopes with the configured ones on the authorize redirect (both the callback and DCR relay arms), on the prior-grant union for managed per-user auth, and on refresh. When nothing survives, the configured set is used. Servers without configured scopes keep passing the client scope through, and the enforce_binding openid scope is still added Generated with AI Co-Authored-By: Claude Code --- .../mcp_server/discoverable_endpoints.py | 33 ++- .../mcp_server/test_discoverable_endpoints.py | 226 ++++++++++++++---- 2 files changed, 210 insertions(+), 49 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py index 6e5b5486696..72ad3d4b24e 100644 --- a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py @@ -893,6 +893,25 @@ def _require_s256_pkce( ) +def _cap_requested_scopes(requested: str | None, server: MCPServer) -> str | None: + configured: Final = server.configured_scopes + if not requested or not configured: + return requested + scopes: Final = requested.split() + dropped: Final = [scope for scope in scopes if scope not in configured] + if dropped: + verbose_logger.debug( + "MCP server %s: dropped requested OAuth scopes %s outside the configured scopes", + server.server_id, + dropped, + ) + return " ".join(scope for scope in scopes if scope in configured) or None + + +def _default_scope(server: MCPServer) -> str | None: + return " ".join(server.scopes) if server.scopes else None + + def _redirect_to_upstream_authorize( *, mcp_server: MCPServer, @@ -907,7 +926,7 @@ def _redirect_to_upstream_authorize( """The bridge relay arm's authorize redirect: every client-supplied parameter passes through to the upstream authorize endpoint verbatim, no relay state cookie is set, and the upstream enforces its own registered redirect binding for the client.""" - scope_value: Final = scope or (" ".join(mcp_server.scopes) if mcp_server.scopes else None) + scope_value: Final = _cap_requested_scopes(scope, mcp_server) or _default_scope(mcp_server) upstream_resource: Final = resolve_upstream_resource(mcp_server) passthrough_params: Final = { "client_id": client_id, @@ -1006,7 +1025,8 @@ async def _managed_authorization_scope( if scope_user_id and prisma_client is not None else None ) - merged_scope: Final = union_scopes(" ".join(prior_grant.get("scopes") or ()) if prior_grant else None, scope) + prior_scope: Final = " ".join(prior_grant.get("scopes") or ()) if prior_grant else None + merged_scope: Final = union_scopes(_cap_requested_scopes(prior_scope, server), scope) selected_scope: Final = ( get_client_metadata_scopes( merged_scope, @@ -1100,7 +1120,7 @@ async def authorize_with_server( managed_per_user: Final = resolved_server.is_gateway_managed_oauth2 and resolved_server.needs_user_oauth_token initial_scope: Final = union_scopes( "openid" if enforce_binding else None, - scope or (" ".join(resolved_server.scopes) if resolved_server.scopes else None), + _cap_requested_scopes(scope, resolved_server) or _default_scope(resolved_server), ) selected_scope, scope_user_id = ( await _managed_authorization_scope(request, resolved_server, initial_scope, litellm_user_id) @@ -1327,7 +1347,12 @@ async def exchange_token_with_server( "refresh_token": upstream_refresh_token, **token_request.body, } - refresh_request_scope = scope or bridge_upstream_scope + client_refresh_scope: Final = scope or bridge_upstream_scope + refresh_request_scope = ( + _cap_requested_scopes(client_refresh_scope, resolved_server) or _default_scope(resolved_server) + if client_refresh_scope + else None + ) if refresh_request_scope: token_data["scope"] = refresh_request_scope refresh_ownership = ( # rebind-ok: grant-specific branches assign one ownership value diff --git a/tests/unit/proxy/_experimental/mcp_server/test_discoverable_endpoints.py b/tests/unit/proxy/_experimental/mcp_server/test_discoverable_endpoints.py index f09409a023d..2137a434e42 100644 --- a/tests/unit/proxy/_experimental/mcp_server/test_discoverable_endpoints.py +++ b/tests/unit/proxy/_experimental/mcp_server/test_discoverable_endpoints.py @@ -5068,60 +5068,121 @@ async def test_oauth_authorize_includes_scopes_from_server_config(): ) -@pytest.mark.asyncio -async def test_oauth_authorize_prefers_request_scope_over_server_config(): - """Test that explicit scope parameter takes precedence over server configuration.""" - try: - from fastapi import Request +_READ_SCOPES: Final = ("ir.incidents:read", "ir.services:read") - from litellm.proxy._experimental.mcp_server.discoverable_endpoints import ( - authorize_with_server, - ) - from litellm.proxy._types import MCPTransport - from litellm.types.mcp import MCPAuth - from litellm.types.mcp_server.mcp_server_manager import MCPServer - except ImportError: - pytest.skip("MCP discoverable endpoints not available") - oauth_server = MCPServer( - server_id="test_server", - name="test", - server_name="test", - transport=MCPTransport.http, - auth_type=MCPAuth.oauth2, - authorization_url="https://provider.com/oauth/authorize", - token_url="https://provider.com/oauth/token", - client_id="test_client", - scopes=["default_scope1", "default_scope2"], - ) +async def _upstream_authorize_scope(server: "MCPServer", client_scope: str | None) -> list[str] | None: + from urllib.parse import parse_qs, urlparse + + from litellm.proxy._experimental.mcp_server.discoverable_endpoints import authorize_with_server mock_request = MagicMock(spec=Request) mock_request.base_url = "https://litellm.example.com/" mock_request.headers = {} - - with patch("litellm.proxy._experimental.mcp_server.discoverable_endpoints.encrypt_value_helper") as mock_encrypt: - mock_encrypt.return_value = "encrypted_state" - - # Call authorize WITH explicit scope parameter + with patch( # test-quality-ok: state sealing is unrelated to which scope reaches the upstream + "litellm.proxy._experimental.mcp_server.discoverable_endpoints.encrypt_value_helper", + return_value="encrypted_state", + ): response = await authorize_with_server( request=mock_request, - mcp_server=oauth_server, - client_id="test_client", + mcp_server=server, + client_id="caller-client", redirect_uri="http://localhost:3000/callback", - state="test_state", - code_challenge="test_challenge", + state="client-state", + code_challenge="challenge", code_challenge_method="S256", - response_type="code", - scope="custom_scope1 custom_scope2", # Explicit scope should take precedence + scope=client_scope, ) + scope_values = parse_qs(urlparse(response.headers["location"]).query).get("scope") + return None if scope_values is None else scope_values[0].split() - # Should use the explicit scope, not server config - assert response.status_code in (307, 302) - redirect_url = response.headers["location"] - assert ( - "scope=custom_scope1+custom_scope2" in redirect_url or "scope=custom_scope1%20custom_scope2" in redirect_url - ) - assert "default_scope" not in redirect_url + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "client_scope, expected", + [ + ("all", list(_READ_SCOPES)), + ("ir.incidents:read", ["ir.incidents:read"]), + ("ir.incidents:read ir.incidents:write", ["ir.incidents:read"]), + ("ir.incidents:write admin", list(_READ_SCOPES)), + (None, list(_READ_SCOPES)), + ], +) +async def test_authorize_never_requests_a_scope_outside_the_configured_scopes(client_scope, expected): + server = _resource_server(scopes=list(_READ_SCOPES), configured_scopes=_READ_SCOPES) + + assert await _upstream_authorize_scope(server, client_scope) == expected + + +@pytest.mark.asyncio +async def test_authorize_passes_client_scope_through_when_no_scopes_are_configured(): + discovered_only = _resource_server(scopes=["discovered:read"], configured_scopes=None) + + assert await _upstream_authorize_scope(discovered_only, "custom_scope1 custom_scope2") == [ + "custom_scope1", + "custom_scope2", + ] + + +@pytest.mark.asyncio +@pytest.mark.parametrize("client_scope", ["all", None]) +async def test_bridge_relay_authorize_never_requests_a_scope_outside_the_configured_scopes(client_scope): + from urllib.parse import parse_qs, urlparse + + from litellm.proxy._experimental.mcp_server.discoverable_endpoints import authorize_with_server + + response = await authorize_with_server( + request=_bridge_mock_request(), + mcp_server=_bridge_server( + auth_type=MCPAuth.oauth_delegate, scopes=list(_READ_SCOPES), configured_scopes=_READ_SCOPES + ), + client_id="dcr-client-123", + redirect_uri=_BRIDGE_CLIENT_REDIRECT, + state="client-state", + code_challenge="chal", + code_challenge_method="S256", + scope=client_scope, + ) + + assert parse_qs(urlparse(response.headers["location"]).query)["scope"] == [" ".join(_READ_SCOPES)] + + +@pytest.mark.asyncio +async def test_authorize_with_enforced_identity_binding_adds_only_openid_beyond_the_configured_scopes(): + from litellm.types.mcp_server.mcp_server_manager import MCPOAuthIdentityBinding + + server = _resource_server( + scopes=list(_READ_SCOPES), + configured_scopes=_READ_SCOPES, + oauth_identity_binding=MCPOAuthIdentityBinding( + mode="enforce", issuer="https://idp.example.com", audiences=["gateway-client"] + ), + ) + with ( + patch( # test-quality-ok: isolate caller resolution from the scope the upstream is asked for + "litellm.proxy._experimental.mcp_server.discoverable_endpoints._resolve_oauth_authorization_user", + new=AsyncMock(return_value="alice"), + ), + ): + requested = await _upstream_authorize_scope(server, "all ir.incidents:read") + + assert requested == ["openid", "ir.incidents:read"] + + +@pytest.mark.asyncio +async def test_authorize_logs_dropped_scope_names_and_server_but_not_client_credentials(caplog): + import logging + + server = _resource_server(scopes=list(_READ_SCOPES), configured_scopes=_READ_SCOPES) + with caplog.at_level(logging.DEBUG, logger="LiteLLM"): + await _upstream_authorize_scope(server, "all ir.incidents:read") + + dropped_logs = [record for record in caplog.records if "outside the configured" in record.getMessage()] + assert [record.levelno for record in dropped_logs] == [logging.DEBUG], caplog.text + message = dropped_logs[0].getMessage() + assert "'all'" in message and "res-srv" in message + assert "ir.incidents:read" not in message + assert "gateway-client" not in message and "gateway-secret" not in message @pytest.mark.asyncio @@ -6870,6 +6931,73 @@ async def test_bridge_refresh_re_requests_the_sealed_scope_when_client_omits_it( assert captured["client"].post.call_args.kwargs["data"]["scope"] == "read:tools write:tools" +@pytest.mark.asyncio +@pytest.mark.parametrize( + "sealed_scope, client_scope, expected_upstream_scope", + [ + ("ir.incidents:read ir.incidents:write", None, "ir.incidents:read"), + ("ir.incidents:read", "all ir.incidents:read", "ir.incidents:read"), + (None, "all", " ".join(_READ_SCOPES)), + ("ir.incidents:write", None, " ".join(_READ_SCOPES)), + (None, None, None), + ], +) +async def test_bridge_refresh_never_requests_a_scope_outside_the_configured_scopes( + sealed_scope, client_scope, expected_upstream_scope +): + from litellm.proxy._experimental.mcp_server.discoverable_endpoints import exchange_token_with_server + + server = _bridge_server(auth_type=MCPAuth.oauth_delegate, scopes=list(_READ_SCOPES), configured_scopes=_READ_SCOPES) + refresh_env = _mint_test_refresh_envelope(server_id=server.server_id, upstream_refresh="UP", scope=sealed_scope) + upstream: Final = MagicMock() + upstream.json.return_value = {"access_token": "NEW", "token_type": "Bearer", "expires_in": 3600} + http_client: Final = MagicMock(post=AsyncMock(return_value=upstream)) + with ( + patch( # test-quality-ok: fake the upstream token endpoint at the HTTP client boundary + "litellm.proxy._experimental.mcp_server.discoverable_endpoints.get_async_httpx_client", + return_value=http_client, + ), + patch( # test-quality-ok: the sealed key is active; revocation is covered by its own tests + "litellm.proxy._experimental.mcp_server.bridge_token_flow._revalidate_active_subject", + new=AsyncMock(return_value=None), + ), + patch("litellm.proxy.proxy_server.master_key", _BRIDGE_MASTER_KEY), + ): + response = await exchange_token_with_server( + request=_bridge_mock_request(), + mcp_server=server, + grant_type="refresh_token", + code=None, + redirect_uri=None, + client_id="dcr-client-123", + client_secret=None, + code_verifier=None, + refresh_token=refresh_env, + scope=client_scope, + ) + + assert response.status_code == 200, response.body + assert http_client.post.call_args.kwargs["data"].get("scope") == expected_upstream_scope + + +@pytest.mark.asyncio +async def test_refresh_passes_client_scope_through_when_no_scopes_are_configured(): + server = _bridge_server(auth_type=MCPAuth.oauth_delegate, scopes=["discovered:read"], configured_scopes=None) + refresh_env = _mint_test_refresh_envelope(server_id=server.server_id, upstream_refresh="UP", scope="a b") + captured: dict = {} + + response = await _refresh_for_bridge_server( + server, + refresh_env, + {"access_token": "NEW", "token_type": "Bearer", "expires_in": 3600}, + None, + fake_client_out=captured, + ) + + assert response.status_code == 200, response.body + assert captured["client"].post.call_args.kwargs["data"]["scope"] == "a b" + + @pytest.mark.asyncio async def test_bridge_refresh_re_seals_scope_when_upstream_omits_it_so_the_chain_keeps_it(): """RFC 6749 5.1 lets an upstream omit scope in a refresh response when it is unchanged. The re-minted @@ -14494,10 +14622,16 @@ async def test_saved_refresh_metadata_is_bound_to_the_callers_stored_grant( @pytest.mark.asyncio @pytest.mark.parametrize( - "prior_scopes, expected", [(["tools.read"], "tools.read tools.write"), (None, "tools.write"), ([], "tools.write")] + "prior_scopes, configured, expected", + [ + (["tools.read"], None, "tools.read tools.write"), + (None, None, "tools.write"), + ([], None, "tools.write"), + (["tools.read", "admin"], ("tools.read", "tools.write"), "tools.read tools.write"), + ], ) async def test_managed_authorize_retains_the_callers_previous_grant( - jwt_oauth_identity, monkeypatch, prior_scopes, expected + jwt_oauth_identity, monkeypatch, prior_scopes, configured, expected ): from types import SimpleNamespace from urllib.parse import parse_qs, urlsplit @@ -14508,7 +14642,9 @@ async def test_managed_authorize_retains_the_callers_previous_grant( _, signing_key = jwt_oauth_identity monkeypatch.setenv("LITELLM_SALT_KEY", "scope-grant-test-salt") - server = _cimd_oauth_server().model_copy(update={"client_id": "scope-client", "scopes": ["tools.read"]}) + server = _cimd_oauth_server().model_copy( + update={"client_id": "scope-client", "scopes": ["tools.read"], "configured_scopes": configured} + ) global_mcp_server_manager.registry[server.server_id] = server credential = db.encrypt_value_helper( json.dumps({"type": "oauth2", "access_token": "old-access", "scopes": prior_scopes}) From 08b6fe08b029cd17cd3aab5cd70a1d30446a5acc Mon Sep 17 00:00:00 2001 From: Gaurav Pandey <112387553+gaurav-pandey-zocdoc@users.noreply.github.com> Date: Sun, 18 Oct 2026 16:51:11 +0530 Subject: [PATCH 2/2] fix(mcp): log how many scopes were dropped instead of their names The dropped scope names come straight from the client request, which CodeQL flags as log injection. Logging the count still shows the cap kicked in for a server without putting request data in the log Generated with AI Co-Authored-By: Claude Code --- .../mcp_server/discoverable_endpoints.py | 4 ++-- .../mcp_server/test_discoverable_endpoints.py | 11 ++++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py index 72ad3d4b24e..436043ffbbb 100644 --- a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py @@ -901,9 +901,9 @@ def _cap_requested_scopes(requested: str | None, server: MCPServer) -> str | Non dropped: Final = [scope for scope in scopes if scope not in configured] if dropped: verbose_logger.debug( - "MCP server %s: dropped requested OAuth scopes %s outside the configured scopes", + "MCP server %s: dropped %d requested OAuth scopes outside the configured scopes", server.server_id, - dropped, + len(dropped), ) return " ".join(scope for scope in scopes if scope in configured) or None diff --git a/tests/unit/proxy/_experimental/mcp_server/test_discoverable_endpoints.py b/tests/unit/proxy/_experimental/mcp_server/test_discoverable_endpoints.py index 2137a434e42..f7a7b1d007d 100644 --- a/tests/unit/proxy/_experimental/mcp_server/test_discoverable_endpoints.py +++ b/tests/unit/proxy/_experimental/mcp_server/test_discoverable_endpoints.py @@ -5170,17 +5170,22 @@ async def test_authorize_with_enforced_identity_binding_adds_only_openid_beyond_ @pytest.mark.asyncio -async def test_authorize_logs_dropped_scope_names_and_server_but_not_client_credentials(caplog): +@pytest.mark.parametrize( + "client_scope, dropped_count", [("all ir.incidents:read", 1), ("all admin ir.incidents:read", 2)] +) +async def test_authorize_logs_dropped_scope_count_and_server_but_not_client_credentials( + caplog, client_scope, dropped_count +): import logging server = _resource_server(scopes=list(_READ_SCOPES), configured_scopes=_READ_SCOPES) with caplog.at_level(logging.DEBUG, logger="LiteLLM"): - await _upstream_authorize_scope(server, "all ir.incidents:read") + await _upstream_authorize_scope(server, client_scope) dropped_logs = [record for record in caplog.records if "outside the configured" in record.getMessage()] assert [record.levelno for record in dropped_logs] == [logging.DEBUG], caplog.text message = dropped_logs[0].getMessage() - assert "'all'" in message and "res-srv" in message + assert f"dropped {dropped_count} requested" in message and "res-srv" in message assert "ir.incidents:read" not in message assert "gateway-client" not in message and "gateway-secret" not in message