diff --git a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py index caa5c65894c..99e462fab2b 100644 --- a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py @@ -54,6 +54,7 @@ from litellm.proxy._experimental.mcp_server.gateway_dcr_flow import ( from litellm.proxy._experimental.mcp_server.oauth_utils import ( TOKEN_NO_CACHE_HEADERS, build_upstream_oauth2_token_request, + gateway_callback_url, get_request_base_url, resolve_upstream_resource, validate_trusted_redirect_uri, @@ -831,7 +832,7 @@ async def authorize_with_server( params = { "client_id": mcp_server.client_id if mcp_server.client_id else client_id, - "redirect_uri": f"{request_base_url}/callback", + "redirect_uri": gateway_callback_url(request_base_url), "state": relay_state, "response_type": response_type or "code", } @@ -983,7 +984,7 @@ async def exchange_token_with_server( ), ) proxy_base_url = get_request_base_url(request) - resolved_redirect_uri = redirect_uri if bridge_token_relay else f"{proxy_base_url}/callback" + resolved_redirect_uri = redirect_uri if bridge_token_relay else gateway_callback_url(proxy_base_url) token_data = { "grant_type": "authorization_code", "code": code, @@ -1501,7 +1502,7 @@ async def mint_ephemeral_dcr_client(request: Request, mcp_server: MCPServer) -> return cached_after_wait register_data: dict[str, object] = { "client_name": mcp_server.server_name or mcp_server.server_id, - "redirect_uris": [f"{request_base_url}/callback"], + "redirect_uris": [gateway_callback_url(request_base_url)], "grant_types": ["authorization_code", "refresh_token"], "response_types": ["code"], "token_endpoint_auth_method": "none", @@ -1576,7 +1577,7 @@ async def register_client_with_server( ): _raise_if_not_oauth2(mcp_server) request_base_url = get_request_base_url(request) - current_redirect_uri = f"{request_base_url}/callback" + current_redirect_uri = gateway_callback_url(request_base_url) client_facing_redirect_uris = client_redirect_uris or [current_redirect_uri] dummy_return = { "client_id": fallback_client_id or mcp_server.server_name, @@ -2525,7 +2526,7 @@ async def register_client(request: Request, mcp_server_name: Optional[str] = Non dummy_return = { "client_id": mcp_server_name or "dummy_client", "client_secret": "dummy", - "redirect_uris": client_redirect_uris or [f"{request_base_url}/callback"], + "redirect_uris": client_redirect_uris or [gateway_callback_url(request_base_url)], } client_ip = IPAddressUtils.get_mcp_client_ip(request) if not mcp_server_name: diff --git a/litellm/proxy/_experimental/mcp_server/oauth_utils.py b/litellm/proxy/_experimental/mcp_server/oauth_utils.py index 5daec9f97be..c2435a3131a 100644 --- a/litellm/proxy/_experimental/mcp_server/oauth_utils.py +++ b/litellm/proxy/_experimental/mcp_server/oauth_utils.py @@ -555,6 +555,41 @@ def _raise_trusted_redirect_uri_rejected( ) +def gateway_callback_url(base_url: str) -> str: + """The proxy's own MCP OAuth callback, spelled once so the value the relay registers upstream + and the value :func:`_raise_if_gateway_callback_redirect` rejects for a client cannot diverge.""" + return f"{base_url}/callback" + + +def _raise_if_gateway_callback_redirect(redirect_uri: str, proxy_base: str | None) -> None: + """Reject a client ``redirect_uri`` that is the gateway's own ``/callback``. + + Such a URI is same-origin, so the trust policy would accept it, and the flow then dies in a + way that looks like anything but a redirect loop: ``/callback`` decodes the relay state, + redirects the authorization response to the client's redirect_uri, which is itself, and the + second hit tries to decrypt the client's opaque ``state`` as a relay handle and surfaces an + "Incorrect padding" decrypt error. A DCR client adopts this URI when it holds a registration + that echoed the gateway callback back as its own ``redirect_uris``, so the actionable failure + belongs here, before the browser leaves for the upstream. + """ + if not proxy_base: + return + if canonicalize_url_identity(redirect_uri) != canonicalize_url_identity(gateway_callback_url(proxy_base)): + return + _oauth_invalid_request( + "redirect_uri is the proxy's own MCP OAuth callback, so the authorization response " + "would be redirected back into the proxy instead of to your client.", + hint=( + "The client is using the proxy's callback as its own redirect_uri; that happens when it " + "still holds a dynamic client registration whose redirect_uris echoed the gateway " + "callback. Delete the client's stored registration for this server and register again " + "so it sends its own redirect_uri, then add that client's origin to " + f"{_TRUSTED_REDIRECT_ORIGINS_ENV} if it is hosted on another host." + ), + redirect_uri=redirect_uri, + ) + + def validate_trusted_redirect_uri(request: Request, redirect_uri: str) -> None: """Accept ``redirect_uri`` when it is (a) same-origin with the proxy's own request origin, (b) loopback, (c) listed in the @@ -583,6 +618,7 @@ def validate_trusted_redirect_uri(request: Request, redirect_uri: str) -> None: return redirect_netloc = _strip_default_port(parsed.scheme, parsed.netloc) proxy_base = _resolve_proxy_base_for_redirect(request) + _raise_if_gateway_callback_redirect(redirect_uri, proxy_base) if _trusted_redirect_uri_is_allowed(parsed, redirect_netloc, proxy_base): return _raise_trusted_redirect_uri_rejected(request, redirect_uri, parsed, redirect_netloc, proxy_base) diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index f591e855a81..d429ec99f61 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -138,6 +138,7 @@ if MCP_AVAILABLE: authorize_with_server, client_supplied_redirect_uris, exchange_token_with_server, + gateway_callback_url, get_request_base_url, redeem_passthrough_authorization_code, register_client_with_server, @@ -1777,7 +1778,7 @@ if MCP_AVAILABLE: resolved_code = sealed_code.upstream_code if sealed_code else code # A sealed flow ran the gateway /callback as its upstream redirect (bridge short-circuit # or plain flow alike), so the exchange must present that binding, not the browser page. - resolved_redirect_uri = f"{get_request_base_url(request)}/callback" if sealed_code else redirect_uri + resolved_redirect_uri = gateway_callback_url(get_request_base_url(request)) if sealed_code else redirect_uri caller_client_id = sealed_code.client_id if sealed_code else client_id caller_client_secret = sealed_code.client_secret if sealed_code else client_secret resolved_client_id = mcp_server.client_id or caller_client_id or "" diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py index 694583dde88..6299c84eba2 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py @@ -975,6 +975,96 @@ async def test_register_client_valid_multi_redirect_uris_all_echoed(): assert result["redirect_uris"] == client_redirects +@pytest.mark.parametrize( + "gateway_callback_variant", + [ + "https://proxy.litellm.example/callback", + "https://proxy.litellm.example/callback/", + "https://proxy.litellm.example:443/callback", + "https://PROXY.litellm.example/callback", + "https://proxy.litellm.example/callback?client=open-webui", + ], +) +def test_authorize_rejects_the_gateway_callback_as_a_client_redirect_uri(gateway_callback_variant, monkeypatch): + """Regression for the reopened DCR self-redirect loop (#34771, #33699). A DCR client holding a + registration that echoed the gateway callback back as its own redirect_uris authorizes with + LiteLLM's /callback as its redirect. That URI is same-origin, so the trust policy used to accept + it, /callback then redirected the authorization response to itself, and the second hit failed to + decrypt the client's opaque state ("Incorrect padding"). The flow must fail at /authorize with an + actionable error instead, and must not send the browser upstream.""" + from fastapi import FastAPI + from fastapi.testclient import TestClient + + from litellm.proxy._experimental.mcp_server.discoverable_endpoints import router + from litellm.proxy._experimental.mcp_server.mcp_server_manager import global_mcp_server_manager + + monkeypatch.setenv("PROXY_BASE_URL", "https://proxy.litellm.example") + monkeypatch.delenv("MCP_TRUSTED_REDIRECT_ORIGINS", raising=False) + global_mcp_server_manager.registry.clear() + oauth2_server = _create_oauth2_server(server_id="atlassian", name="atlassian", server_name="atlassian") + global_mcp_server_manager.registry[oauth2_server.server_id] = oauth2_server + + app = FastAPI() + app.include_router(router) + try: + response = TestClient(app).get( + "/atlassian/authorize", + params={ + "response_type": "code", + "client_id": "upstream-issued-client-id", + "redirect_uri": gateway_callback_variant, + "state": "client-opaque-state", + "code_challenge": "c" * 43, + "code_challenge_method": "S256", + }, + follow_redirects=False, + ) + finally: + global_mcp_server_manager.registry.clear() + + assert response.status_code == 400 + detail = response.json()["detail"] + assert detail["error"] == "invalid_request" + assert "own MCP OAuth callback" in detail["error_description"] + assert "dynamic client registration" in detail["hint"] + assert "MCP_TRUSTED_REDIRECT_ORIGINS" in detail["hint"] + + +def test_callback_refuses_to_redirect_an_authorization_response_to_itself(monkeypatch): + """The /callback sink is the second half of the same loop: a state minted before the /authorize + guard (they never expire) still carries the gateway callback as the client redirect. Forwarding + the code there re-enters this handler with the client's own state and dies in the decrypt path, + so the sink must reject it too (#34771).""" + from fastapi import FastAPI + from fastapi.testclient import TestClient + + from litellm.proxy._experimental.mcp_server.discoverable_endpoints import ( + encode_state_with_base_url, + router, + ) + + monkeypatch.setenv("PROXY_BASE_URL", "https://proxy.litellm.example") + monkeypatch.setenv("LITELLM_SALT_KEY", "sk-test-salt-for-lit34771") + monkeypatch.setattr("litellm.proxy.proxy_server.master_key", "sk-test-salt-for-lit34771", raising=False) + + stale_state = encode_state_with_base_url( + base_url="https://proxy.litellm.example/callback", + original_state="client-opaque-state", + client_redirect_uri="https://proxy.litellm.example/callback", + ) + + app = FastAPI() + app.include_router(router) + response = TestClient(app).get( + "/callback", + params={"code": "upstream-code", "state": stale_state}, + follow_redirects=False, + ) + + assert response.status_code == 400 + assert "own MCP OAuth callback" in response.json()["detail"]["error_description"] + + @pytest.mark.asyncio async def test_register_client_persists_dcr_client_identity(): """A dynamic client registration (RFC 7591) must persist the issued client_id /