diff --git a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py index f5c54451006..8d2ad4c02ff 100644 --- a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py @@ -91,6 +91,7 @@ from litellm.proxy._experimental.mcp_server.oauth_utils import ( TOKEN_NO_CACHE_HEADERS, build_upstream_authorize_url, build_upstream_oauth2_token_request, + canonical_origin, configured_proxy_base_url, get_cimd_client_id, get_cimd_document_url, @@ -232,6 +233,7 @@ def encode_state_with_base_url( oauth_nonce: str | None = None, managed_scope: str | None = None, scope_user_id: str | None = None, + configured_authorization_origin: str | None = None, ) -> str: """ Encode the base_url, original state, and PKCE parameters using encryption. @@ -276,6 +278,7 @@ def encode_state_with_base_url( "dcr_client_secret": dcr_client_secret, "dcr_token_endpoint_auth_method": dcr_token_endpoint_auth_method, "expected_issuer": expected_issuer, + "configured_authorization_origin": configured_authorization_origin, "authorization_response_iss_parameter_supported": authorization_response_iss_parameter_supported, } state_json: Final = json.dumps(state_data, sort_keys=True) @@ -1173,6 +1176,7 @@ async def authorize_with_server( if ephemeral_dcr_client else None, expected_issuer=resolved_server.issuer, + configured_authorization_origin=_configured_authorization_origin(resolved_server), authorization_response_iss_parameter_supported=resolved_server.authorization_response_iss_parameter_supported, ) relay_state: Final = secrets.token_urlsafe(_OAUTH_STATE_HANDLE_BYTES) @@ -2671,11 +2675,46 @@ def _render_oauth_error_html(error: str, description: str | None) -> HTMLRespons return HTMLResponse(body, status_code=400) +def _configured_authorization_origin(server: MCPServer) -> str | None: + """Discovery without an issuer cannot vouch for the authorization server's identity.""" + configured_url: Final = server.configured_authorization_url + if configured_url is None or configured_url != server.effective_authorization_url: + return None + configured_token_url: Final = server.configured_token_url + if not configured_token_url or configured_token_url != server.effective_token_url: + return None + return canonical_origin(configured_url) + + +def _is_origin_comparable_issuer(issuer: str) -> bool: + """Reject issuer syntax that URL parsers normalize or interpret inconsistently.""" + if not all(char.isprintable() and not char.isspace() for char in issuer) or "?" in issuer or "#" in issuer: + return False + try: + parsed: Final = urlparse(issuer) + return ( + parsed.scheme in ("http", "https") + and bool(parsed.hostname) + and "@" not in parsed.netloc + and (parsed.port is None or 0 <= parsed.port <= 65535) + ) + except ValueError: + return False + + def _authorization_response_issuer_is_trusted(response_issuer: str | None, state_data: Mapping[str, object]) -> bool: + """Known issuers match exactly. Without one, the configured origin covers every tenant path on that host.""" expected_issuer: Final = state_data.get("expected_issuer") if response_issuer is None: return state_data.get("authorization_response_iss_parameter_supported") is not True - return isinstance(expected_issuer, str) and bool(expected_issuer) and response_issuer == expected_issuer + if isinstance(expected_issuer, str) and expected_issuer: + return response_issuer == expected_issuer + configured_origin: Final = state_data.get("configured_authorization_origin") + return ( + isinstance(configured_origin, str) + and _is_origin_comparable_issuer(response_issuer) + and canonical_origin(response_issuer) == configured_origin + ) @router.get("/callback") @@ -2696,7 +2735,8 @@ async def callback( forwarded back to the validated client ``redirect_uri`` with the original (un-wrapped) ``state``, once the RFC 9207 ``iss`` (when the authorization server sent one) matches the issuer /authorize sealed - into the state. + into the state. Without a known issuer, it must match the sealed origin of + the configured authorization URL. - An error response (``error``[+``error_description``/``error_uri``]), per RFC 6749 §4.1.2.1. When ``state`` is present and decodes to a trusted ``redirect_uri``, the error params are propagated back to the client so @@ -2774,9 +2814,10 @@ async def callback( if not _authorization_response_issuer_is_trusted(iss, issuer_state): verbose_logger.warning( "MCP /callback rejected an authorization response: RFC 9207 iss=%r does not match the " - "issuer this flow was sent to (%r)", + "issuer this flow was sent to (%r) or, with none known, the configured authorization origin (%r)", iss, issuer_state.get("expected_issuer"), + issuer_state.get("configured_authorization_origin"), ) issuer_error_response: Final = _render_oauth_error_html( "invalid_issuer", diff --git a/litellm/proxy/_experimental/mcp_server/oauth_issuer_stamp_backfill.py b/litellm/proxy/_experimental/mcp_server/oauth_issuer_stamp_backfill.py index eb02ed64e91..ecb45d03da2 100644 --- a/litellm/proxy/_experimental/mcp_server/oauth_issuer_stamp_backfill.py +++ b/litellm/proxy/_experimental/mcp_server/oauth_issuer_stamp_backfill.py @@ -37,10 +37,9 @@ runs at most once per row. """ from typing import Final, Protocol -from urllib.parse import urlparse from litellm._logging import verbose_proxy_logger -from litellm.proxy._experimental.mcp_server.oauth_utils import canonicalize_url_identity +from litellm.proxy._experimental.mcp_server.oauth_utils import canonical_origin from litellm.proxy.db.db_span import db_span from litellm.proxy.utils import PrismaClient @@ -54,18 +53,6 @@ _BACKFILL_ACTOR: Final = "mcp_oauth_issuer_stamp_backfill" _AUTH_TYPES_WITH_ISSUER_ANCHORING: Final = ("oauth2", "true_passthrough", "oauth_delegate") -def _origin(url: str) -> str | None: - """The scheme-and-authority identity of ``url``, or ``None`` when it has none. - - Built on the shared URL canonicalizer so the lowercase-host and default-port rules match the - RFC 8414 issuer comparison the resolution path uses, instead of being re-derived here. - """ - parsed: Final = urlparse(canonicalize_url_identity(url)) - if not parsed.scheme or not parsed.netloc: - return None - return f"{parsed.scheme}://{parsed.netloc}" - - class _MCPServerRow(Protocol): """The MCP server row fields this heal reads, so the untyped DB record is narrowed once here.""" @@ -100,8 +87,8 @@ def _is_stamped_issuer_row(row: _MCPServerRow) -> bool: ) if not configured: return False - issuer_origin: Final = _origin(row.issuer or "") - return issuer_origin is not None and all(_origin(endpoint) == issuer_origin for endpoint in configured) + issuer_origin: Final = canonical_origin(row.issuer or "") + return issuer_origin is not None and all(canonical_origin(endpoint) == issuer_origin for endpoint in configured) async def backfill_discovery_stamped_issuers(prisma_client: PrismaClient) -> int: diff --git a/litellm/proxy/_experimental/mcp_server/oauth_utils.py b/litellm/proxy/_experimental/mcp_server/oauth_utils.py index 133bad780bf..90966cbc5a4 100644 --- a/litellm/proxy/_experimental/mcp_server/oauth_utils.py +++ b/litellm/proxy/_experimental/mcp_server/oauth_utils.py @@ -744,6 +744,18 @@ def canonicalize_url_identity(url: str) -> str: return urlunparse((scheme, netloc, parsed.path.rstrip("/"), "", "", "")) +def canonical_origin(url: str) -> str | None: + """The scheme-and-authority identity of ``url``, or ``None`` when it has none. + + Built on the shared URL canonicalizer so the lowercase-host and default-port rules match the + RFC 8414 issuer comparison the resolution path uses, instead of being re-derived here. + """ + parsed: Final = urlparse(canonicalize_url_identity(url)) + if not parsed.scheme or not parsed.netloc: + return None + return f"{parsed.scheme}://{parsed.netloc}" + + def oauth_client_registration_matches( registered_issuer: str | None, registered_url: str | None, diff --git a/litellm/proxy/_lazy_openapi_snapshot.json b/litellm/proxy/_lazy_openapi_snapshot.json index 4e183bf9d5c..2f08265c825 100644 --- a/litellm/proxy/_lazy_openapi_snapshot.json +++ b/litellm/proxy/_lazy_openapi_snapshot.json @@ -40080,7 +40080,7 @@ }, "/callback": { "get": { - "description": "OAuth 2.0 authorization response handler for MCP loopback clients.\n\nAccepts either:\n\n- A successful authorization response (``code`` + ``state``), which is\n forwarded back to the validated client ``redirect_uri`` with the\n original (un-wrapped) ``state``, once the RFC 9207 ``iss`` (when the\n authorization server sent one) matches the issuer /authorize sealed\n into the state.\n- An error response (``error``[+``error_description``/``error_uri``]), per\n RFC 6749 \u00a74.1.2.1. When ``state`` is present and decodes to a trusted\n ``redirect_uri``, the error params are propagated back to the client so\n its OAuth library can surface them. Otherwise we render an HTML error\n page so the user is not left on an opaque 422 / blank screen.", + "description": "OAuth 2.0 authorization response handler for MCP loopback clients.\n\nAccepts either:\n\n- A successful authorization response (``code`` + ``state``), which is\n forwarded back to the validated client ``redirect_uri`` with the\n original (un-wrapped) ``state``, once the RFC 9207 ``iss`` (when the\n authorization server sent one) matches the issuer /authorize sealed\n into the state. Without a known issuer, it must match the sealed origin of\n the configured authorization URL.\n- An error response (``error``[+``error_description``/``error_uri``]), per\n RFC 6749 \u00a74.1.2.1. When ``state`` is present and decodes to a trusted\n ``redirect_uri``, the error params are propagated back to the client so\n its OAuth library can surface them. Otherwise we render an HTML error\n page so the user is not left on an opaque 422 / blank screen.", "operationId": "callback_callback_get", "parameters": [ { 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 a43dacaf100..24a283f8ef2 100644 --- a/tests/unit/proxy/_experimental/mcp_server/test_discoverable_endpoints.py +++ b/tests/unit/proxy/_experimental/mcp_server/test_discoverable_endpoints.py @@ -4851,7 +4851,7 @@ async def test_callback_error_path_reads_cookie_and_clears_it(monkeypatch): assert cleared[cookie_name]["max-age"] == "0" -def _issuer_anchored_oauth_server(issuer: str | None = "https://idp.example.com"): +def _issuer_anchored_oauth_server(issuer: str | None = "https://idp.example.com") -> "MCPServer": from litellm.proxy._types import MCPTransport from litellm.types.mcp import MCPAuth from litellm.types.mcp_server.mcp_server_manager import MCPServer @@ -4870,23 +4870,30 @@ def _issuer_anchored_oauth_server(issuer: str | None = "https://idp.example.com" ) -async def _authorize_then_callback(server, iss, monkeypatch, expected_issuer_override=..., error=None): +async def _authorize_then_callback( + server: "MCPServer", + iss: str | None, + monkeypatch: pytest.MonkeyPatch, + error: str | None = None, + drop_state_keys: tuple[str, ...] = (), +) -> tuple[Response, dict[str, object]]: """Run /authorize for ``server``, then feed the resulting flow back through /callback with the RFC 9207 ``iss`` the authorization server supposedly returned. Returns (callback_response, sealed_state_data).""" from http.cookies import SimpleCookie from urllib.parse import parse_qs, urlparse - from fastapi import Request + from pydantic import TypeAdapter from litellm.proxy._experimental.mcp_server.discoverable_endpoints import ( _oauth_state_cookie_name, authorize_with_server, callback, decode_state_hash, - encode_state_with_base_url, ) + from litellm.proxy.common_utils.encrypt_decrypt_utils import encrypt_value_helper + monkeypatch.setenv("LITELLM_SALT_KEY", "sk-test-salt-for-LIT-5940") client_redirect_uri = "http://127.0.0.1:6274/oauth/callback/debug" @@ -4907,15 +4914,15 @@ async def _authorize_then_callback(server, iss, monkeypatch, expected_issuer_ove jar = SimpleCookie() jar.load(authorize_response.headers["set-cookie"]) cookie_name = _oauth_state_cookie_name(handle) - sealed_state = jar[cookie_name].value - if expected_issuer_override is not ...: - # A state minted before the issuer was sealed into it: same shape, key absent. - sealed_state = encode_state_with_base_url( - base_url=client_redirect_uri, - original_state="client-original-state-9207", - client_redirect_uri=client_redirect_uri, - expected_issuer=expected_issuer_override, + original_state: Final = jar[cookie_name].value + state_data: Final = TypeAdapter(dict[str, object]).validate_python(decode_state_hash(original_state)) + sealed_state: Final = ( + encrypt_value_helper( + json.dumps({key: value for key, value in state_data.items() if key not in drop_state_keys}) ) + if drop_state_keys + else original_state + ) callback_request = MagicMock(spec=Request) callback_request.base_url = "https://proxy.example.com/" @@ -4929,7 +4936,7 @@ async def _authorize_then_callback(server, iss, monkeypatch, expected_issuer_ove state=handle, iss=iss, ) - return response, decode_state_hash(sealed_state) + return response, TypeAdapter(dict[str, object]).validate_python(decode_state_hash(sealed_state)) @pytest.mark.asyncio @@ -5015,11 +5022,229 @@ async def test_callback_legacy_state_requires_absent_issuer(monkeypatch, iss, ex _issuer_anchored_oauth_server(), iss=iss, monkeypatch=monkeypatch, - expected_issuer_override=None, + drop_state_keys=("expected_issuer", "configured_authorization_origin"), ) assert response.status_code == expected_status +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("authorization_url", "iss"), + ( + ("https://idp.example.com/oauth/authorize", "https://idp.example.com"), + ("https://idp.example.com/tenant-a/authorize", "https://idp.example.com/tenant-a"), + ("https://IDP.example.com:443/authorize?prompt=consent", "https://idp.example.com"), + ("https://idp.example.com/authorize", "https://IDP.example.com:443"), + ("http://127.0.0.1:54582/authorize", "http://127.0.0.1:54582"), + ("http://localhost/authorize", "http://localhost:80"), + ("http://[::1]:54582/authorize", "http://[::1]:54582"), + ), +) +async def test_callback_accepts_an_issuer_on_the_configured_authorization_origin( + monkeypatch: pytest.MonkeyPatch, authorization_url: str, iss: str +) -> None: + from urllib.parse import parse_qs, urlparse + + server: Final = _issuer_anchored_oauth_server(issuer=None).model_copy( + update={ + "authorization_url": authorization_url, + "configured_authorization_url": authorization_url, + "token_url": "https://tokens.example.com/token", + "configured_token_url": "https://tokens.example.com/token", + } + ) + response, _ = await _authorize_then_callback(server, iss, monkeypatch) + + assert response.status_code == 302 + destination: Final = urlparse(response.headers["location"]) + assert destination._replace(query="").geturl() == "http://127.0.0.1:6274/oauth/callback/debug" + assert parse_qs(destination.query) == {"code": ["upstream-auth-code"], "state": ["client-original-state-9207"]} + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "iss", + ( + "https://attacker.example.com", + "https://idp.example.com.evil.com", + "http://idp.example.com", + "ftp://idp.example.com", + "https://idp.example.com:8443", + "https://idp.example.com.", + "https://idp.example.com%40evil.com", + "https://idp.example.com?tenant=a", + "https://idp.example.com?", + "https://idp.example.com#fragment", + "https://idp.example.com#", + "https://idp.example.com@evil.com", + "https://user@idp.example.com", + "https://evil.com\\@idp.example.com", + " https://idp.example.com", + "https://idp.example.com\n", + "https://idp.exa\tmple.com", + "https://idp.example.com/\x00", + "idp.example.com", + "", + "https://[idp.example.com", + "https://[not-ipv6]/", + "https://idp.example.com:+443", + "https://idp.example.com:٤٤٣", + "https://idp.example.com:-443", + "https://idp.example.com:65536", + ), +) +async def test_callback_rejects_an_issuer_off_the_configured_authorization_origin( + monkeypatch: pytest.MonkeyPatch, iss: str +) -> None: + server: Final = _issuer_anchored_oauth_server(issuer=None).model_copy( + update={ + "configured_authorization_url": "https://idp.example.com/oauth/authorize", + "configured_token_url": "https://idp.example.com/oauth/token", + } + ) + response, _ = await _authorize_then_callback(server, iss, monkeypatch) + + assert response.status_code == 400 + assert "location" not in response.headers + assert b"invalid_issuer" in response.body + assert b"upstream-auth-code" not in response.body + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("authorization_url", "iss"), + ( + ("http://[::1]:54582/authorize", "http://[0:0::1]:54582"), + ("https://xn--bcher-kva.example/authorize", "https://bücher.example"), + ), +) +async def test_callback_rejects_an_issuer_whose_host_is_spelled_differently( + monkeypatch: pytest.MonkeyPatch, authorization_url: str, iss: str +) -> None: + server: Final = _issuer_anchored_oauth_server(issuer=None).model_copy( + update={ + "authorization_url": authorization_url, + "configured_authorization_url": authorization_url, + "token_url": "https://tokens.example.com/token", + "configured_token_url": "https://tokens.example.com/token", + } + ) + response, _ = await _authorize_then_callback(server, iss, monkeypatch) + + assert response.status_code == 400 + assert "location" not in response.headers + assert b"invalid_issuer" in response.body + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "configured_url", (None, "https://other.example.com/authorize", "https://idp.example.com/manual-authorize") +) +async def test_callback_rejects_an_issuer_when_the_authorization_url_was_discovered( + monkeypatch: pytest.MonkeyPatch, configured_url: str | None +) -> None: + server: Final = _issuer_anchored_oauth_server(issuer=None).model_copy( + update={ + "configured_authorization_url": configured_url, + "configured_token_url": "https://idp.example.com/oauth/token", + } + ) + response, _ = await _authorize_then_callback(server, "https://idp.example.com", monkeypatch) + + assert response.status_code == 400 + assert "location" not in response.headers + assert b"invalid_issuer" in response.body + + +@pytest.mark.asyncio +@pytest.mark.parametrize("iss, expected_status", ((None, 302), ("https://idp.example.com", 400))) +async def test_callback_legacy_state_without_a_configured_origin( + monkeypatch: pytest.MonkeyPatch, iss: str | None, expected_status: int +) -> None: + server: Final = _issuer_anchored_oauth_server(issuer=None).model_copy( + update={ + "configured_authorization_url": "https://idp.example.com/oauth/authorize", + "configured_token_url": "https://idp.example.com/oauth/token", + } + ) + response, _ = await _authorize_then_callback( + server, iss, monkeypatch, drop_state_keys=("configured_authorization_origin",) + ) + + assert response.status_code == expected_status + + +@pytest.mark.asyncio +@pytest.mark.parametrize("iss, expected_status", (("https://idp.example.com", 302), ("https://other.example.com", 400))) +async def test_callback_relays_an_error_only_from_the_configured_authorization_origin( + monkeypatch: pytest.MonkeyPatch, iss: str, expected_status: int +) -> None: + from urllib.parse import parse_qs, urlparse + + server: Final = _issuer_anchored_oauth_server(issuer=None).model_copy( + update={ + "configured_authorization_url": "https://idp.example.com/oauth/authorize", + "configured_token_url": "https://idp.example.com/oauth/token", + } + ) + response, _ = await _authorize_then_callback(server, iss, monkeypatch, error="access_denied") + + assert response.status_code == expected_status + if expected_status == 302: + assert parse_qs(urlparse(response.headers["location"]).query) == { + "error": ["access_denied"], + "state": ["client-original-state-9207"], + } + else: + assert "location" not in response.headers + assert b"invalid_issuer" in response.body + + +@pytest.mark.asyncio +async def test_callback_holds_a_known_issuer_to_exact_match_on_the_configured_origin( + monkeypatch: pytest.MonkeyPatch, +) -> None: + server: Final = _issuer_anchored_oauth_server().model_copy( + update={ + "configured_authorization_url": "https://idp.example.com/oauth/authorize", + "configured_token_url": "https://idp.example.com/oauth/token", + } + ) + response, _ = await _authorize_then_callback(server, "https://idp.example.com/", monkeypatch) + + assert response.status_code == 400 + assert "location" not in response.headers + assert b"invalid_issuer" in response.body + + +@pytest.mark.asyncio +@pytest.mark.parametrize("error", (None, "access_denied")) +@pytest.mark.parametrize( + ("configured_token_url", "token_url"), + ( + (None, None), + (None, "https://collector.example.com/token"), + ("https://idp.example.com/oauth/token", "https://collector.example.com/token"), + ), +) +async def test_callback_rejects_an_unknown_issuer_when_the_token_endpoint_is_not_configured( + monkeypatch: pytest.MonkeyPatch, configured_token_url: str | None, token_url: str | None, error: str | None +) -> None: + server: Final = _issuer_anchored_oauth_server(issuer=None).model_copy( + update={ + "configured_authorization_url": "https://idp.example.com/oauth/authorize", + "configured_token_url": configured_token_url, + "token_url": token_url, + } + ) + response, _ = await _authorize_then_callback(server, "https://idp.example.com", monkeypatch, error=error) + + assert response.status_code == 400 + assert "location" not in response.headers + assert b"invalid_issuer" in response.body + assert b"upstream-auth-code" not in response.body + + @pytest.mark.asyncio async def test_oauth_authorize_includes_scopes_from_server_config(): """Test that authorize endpoint includes scopes from server configuration.""" diff --git a/ui/litellm-dashboard/src/lib/http/schema.d.ts b/ui/litellm-dashboard/src/lib/http/schema.d.ts index 262e8b6deae..ed83b87e66a 100644 --- a/ui/litellm-dashboard/src/lib/http/schema.d.ts +++ b/ui/litellm-dashboard/src/lib/http/schema.d.ts @@ -2275,7 +2275,8 @@ export interface paths { * forwarded back to the validated client ``redirect_uri`` with the * original (un-wrapped) ``state``, once the RFC 9207 ``iss`` (when the * authorization server sent one) matches the issuer /authorize sealed - * into the state. + * into the state. Without a known issuer, it must match the sealed origin of + * the configured authorization URL. * - An error response (``error``[+``error_description``/``error_uri``]), per * RFC 6749 §4.1.2.1. When ``state`` is present and decodes to a trusted * ``redirect_uri``, the error params are propagated back to the client so