From e6db6206d19de2c8d33aeaccdd00049fdb296603 Mon Sep 17 00:00:00 2001 From: shrey Date: Tue, 10 Nov 2026 10:23:50 -0800 Subject: [PATCH] fix(mcp): accept comma-separated vendor scope grants on the connect check (#50717) --- .../mcp_server/discoverable_endpoints.py | 12 +- litellm/proxy/mcp_server/oauth_utils.py | 31 ++++- .../mcp_server/test_discoverable_endpoints.py | 110 +++++++++++++++++- 3 files changed, 150 insertions(+), 3 deletions(-) diff --git a/litellm/proxy/mcp_server/discoverable_endpoints.py b/litellm/proxy/mcp_server/discoverable_endpoints.py index 07a267d6caa..ecc40b6e98f 100644 --- a/litellm/proxy/mcp_server/discoverable_endpoints.py +++ b/litellm/proxy/mcp_server/discoverable_endpoints.py @@ -105,9 +105,11 @@ from litellm.proxy.mcp_server.oauth_utils import ( get_cimd_client_id, get_cimd_document_url, get_request_base_url, + grant_covers_scopes, needs_cimd_discovery, oauth_client_registration_matches, resolve_upstream_resource, + server_scope_names, validate_trusted_redirect_uri, well_known_root_suffix, ) @@ -2537,6 +2539,9 @@ async def _vendor_credential_state( get_user_oauth_credential, oauth_grant_state, ) + from litellm.proxy.mcp_server.mcp_server_manager import ( # noqa: PLC0415 # circular import at module load + global_mcp_server_manager, + ) from litellm.proxy.proxy_server import prisma_client # noqa: PLC0415 # circular import at module load if prisma_client is None: @@ -2548,7 +2553,12 @@ async def _vendor_credential_state( if oauth_grant_state(credential) == "absent": return "absent" granted_scopes: Final = credential.get("scopes") if credential else None - return "present" if set(requested_scopes).issubset(granted_scopes or ()) else "absent" + covered: Final = grant_covers_scopes( + tuple(granted_scopes or ()), + requested_scopes, + lambda: server_scope_names(global_mcp_server_manager.get_mcp_server_by_id(server_id)), + ) + return "present" if covered else "absent" @router.get("/authorize/flow") diff --git a/litellm/proxy/mcp_server/oauth_utils.py b/litellm/proxy/mcp_server/oauth_utils.py index 76343f77e32..3171c476ede 100644 --- a/litellm/proxy/mcp_server/oauth_utils.py +++ b/litellm/proxy/mcp_server/oauth_utils.py @@ -3,7 +3,7 @@ import os import re -from collections.abc import Mapping +from collections.abc import Callable, Mapping from dataclasses import dataclass from ipaddress import ip_address from types import MappingProxyType @@ -11,6 +11,7 @@ from typing import TYPE_CHECKING, Final, NoReturn from urllib.parse import ParseResult, parse_qsl, urlencode, urlparse, urlsplit, urlunparse, urlunsplit from fastapi import HTTPException, Request +from pydantic import TypeAdapter from starlette.types import Scope from litellm._logging import verbose_logger @@ -131,6 +132,34 @@ def build_upstream_authorize_url(authorization_url: str, params: Mapping[str, st return urlunparse(parsed._replace(query=urlencode(merged))) +def grant_covers_scopes( + granted: tuple[str, ...], requested: tuple[str, ...], server_scopes: Callable[[], frozenset[str]] +) -> bool: + """Whether a stored grant holds every requested scope. A grant stored as one comma-joined token, the form + Slack and GitHub reply with, counts as its parts only when every part is a requested or server scope.""" + if all(scope in granted for scope in requested): + return True + if len(granted) != 1 or "," not in granted[0]: + return False + parts: Final = frozenset(granted[0].split(",")) + known: Final = frozenset(requested) | server_scopes() + return parts <= known and all(scope in granted or scope in parts for scope in requested) + + +_PUBLISHED_SCOPES: Final = TypeAdapter[tuple[str, ...] | None](tuple[str, ...] | None) + + +def server_scope_names(server: "MCPServer | None") -> frozenset[str]: + if server is None: + return frozenset() + published: Final = ( + _PUBLISHED_SCOPES.validate_python(server.authorization_server_metadata.get("scopes_supported")) + if server.authorization_server_metadata is not None + else None + ) + return frozenset((*(server.scopes or ()), *(published or ()))) + + def proxy_base_url_from_env() -> str | None: """The gateway's fixed public URL from ``PROXY_BASE_URL``, ``None`` when unset or malformed.""" return _resolve_proxy_base_url_env() diff --git a/tests/unit/proxy/mcp_server/test_discoverable_endpoints.py b/tests/unit/proxy/mcp_server/test_discoverable_endpoints.py index c17f4b392cd..47d24a9626f 100644 --- a/tests/unit/proxy/mcp_server/test_discoverable_endpoints.py +++ b/tests/unit/proxy/mcp_server/test_discoverable_endpoints.py @@ -16,7 +16,7 @@ from litellm.proxy._types import LiteLLM_MCPServerTable from litellm.types.mcp import MCPAuth if TYPE_CHECKING: - from collections.abc import Iterator + from collections.abc import Iterator, Mapping import httpx from cryptography.hazmat.primitives.asymmetric.rsa import RSAPrivateKey @@ -15278,6 +15278,9 @@ async def test_managed_resource_metadata_excludes_refresh_permission(monkeypatch (["tools.read", "tools.write"], ("tools.write",), "present"), (None, ("tools.write",), "absent"), (["tools.read"], (), "present"), + (["channels:read,chat:write"], ("channels:read", "chat:write"), "present"), + (["channels:read,chat:write"], ("channels:read", "files:write"), "absent"), + (["channels:read"], ("channels:read", "chat:write"), "absent"), ], ) async def test_connect_requires_the_requested_vendor_grant(monkeypatch, granted, required, expected): @@ -15293,6 +15296,111 @@ async def test_connect_requires_the_requested_vendor_grant(monkeypatch, granted, read.assert_awaited_once_with(prisma, "owner", "server") +_SLACK_SCOPES: Final = frozenset(("channels:read", "chat:write", "files:write")) +_COMMA_NAMED_SCOPES: Final = frozenset(("files:read", "files:read,write")) + + +@pytest.mark.parametrize( + ("granted", "requested", "known", "expected"), + [ + (("channels:read,chat:write",), ("channels:read", "chat:write"), _SLACK_SCOPES, True), + (("channels:read,chat:write",), ("channels:read", "files:write"), _SLACK_SCOPES, False), + (("channels:read,chat:write,files:read",), ("channels:read",), _SLACK_SCOPES, False), + (("x:unknown,chat:write",), ("chat:write",), _SLACK_SCOPES, False), + (("files:read,write",), ("files:read",), _COMMA_NAMED_SCOPES, False), + (("files:read,write",), ("files:read,write",), _COMMA_NAMED_SCOPES, True), + (("channels:read,chat:write",), ("read",), _SLACK_SCOPES | {"read"}, False), + (("read", "write,admin"), ("admin",), frozenset(("read", "write", "admin")), False), + (("write,admin", "read"), ("admin",), frozenset(("read", "write", "admin")), False), + (("read", "write"), ("read", "write"), frozenset[str](), True), + ((), ("read",), frozenset(("read",)), False), + ((), (), frozenset[str](), True), + ], +) +def test_grant_covers_scopes_splits_a_lone_token_only_into_known_scopes( + granted: tuple[str, ...], requested: tuple[str, ...], known: frozenset[str], expected: bool +) -> None: + from litellm.proxy.mcp_server.oauth_utils import grant_covers_scopes + + assert grant_covers_scopes(granted, requested, lambda: known) is expected + + +@pytest.mark.parametrize( + ("granted", "requested", "expected"), + [ + (("channels:read",), ("channels:read",), True), + (("read", "write"), ("read", "write"), True), + (("read", "write"), ("admin",), False), + (("read",), ("admin",), False), + ((), (), True), + ], +) +def test_grant_covers_scopes_answers_like_a_subset_check_without_reading_server_scopes( + granted: tuple[str, ...], requested: tuple[str, ...], expected: bool +) -> None: + from litellm.proxy.mcp_server.oauth_utils import grant_covers_scopes + + def unreachable_server_scopes() -> frozenset[str]: + raise AssertionError("server scopes read for a grant with no comma token") + + assert grant_covers_scopes(granted, requested, unreachable_server_scopes) is expected + + +def test_server_scope_names_joins_configured_and_published_scopes() -> None: + from litellm.proxy.mcp_server.oauth_utils import server_scope_names + + server: Final = _cimd_oauth_server().model_copy( + update={ + "scopes": ["channels:read"], + "authorization_server_metadata": { + "issuer": "https://idp.example.com", + "authorization_endpoint": "https://idp.example.com/authorize", + "token_endpoint": "https://idp.example.com/token", + "response_types_supported": ["code"], + "scopes_supported": ["chat:write", "files:write"], + }, + } + ) + assert server_scope_names(server) == frozenset(("channels:read", "chat:write", "files:write")) + assert server_scope_names(_cimd_oauth_server()) == frozenset[str]() + assert server_scope_names(None) == frozenset[str]() + + +_PUBLISHED_SLACK_METADATA: Final = { + "issuer": "https://idp.example.com", + "authorization_endpoint": "https://idp.example.com/authorize", + "token_endpoint": "https://idp.example.com/token", + "response_types_supported": ["code"], + "scopes_supported": ["channels:read", "chat:write", "files:read"], +} + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "server_update", + [ + {"scopes": ["channels:read", "chat:write", "files:read"]}, + {"authorization_server_metadata": _PUBLISHED_SLACK_METADATA}, + ], +) +async def test_connect_reads_a_comma_grant_against_the_server_scopes( + monkeypatch: pytest.MonkeyPatch, server_update: "Mapping[str, object]" +) -> None: + from litellm.proxy.mcp_server import db + from litellm.proxy.mcp_server import discoverable_endpoints as endpoints + from litellm.proxy.mcp_server.mcp_server_manager import global_mcp_server_manager + + server: Final = _cimd_oauth_server().model_copy(update=server_update) + credential: Final = {**_stored_grant(), "scopes": ["channels:read,chat:write,files:read"]} + monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", MagicMock()) + monkeypatch.setattr(db, "get_user_oauth_credential", AsyncMock(return_value=credential)) + assert await endpoints._vendor_credential_state("owner", server.server_id, ("channels:read",)) == "absent" + monkeypatch.setitem(global_mcp_server_manager.registry, server.server_id, server) + assert await endpoints._vendor_credential_state("owner", server.server_id, ("channels:read",)) == "present" + assert await endpoints._vendor_credential_state("owner", server.server_id, ("files:write",)) == "absent" + assert await endpoints._vendor_credential_state("owner", server.server_id, ()) == "present" + + @pytest.mark.parametrize( ("header", "expected"), [