From 45245c619149ee75beea29f32483703b2284d35d Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 27 Jul 2026 11:51:50 +0000 Subject: [PATCH] fix(mcp): reject the gateway's own /callback as a DCR client redirect_uri A DCR client that holds a registration whose redirect_uris echoed the gateway callback authorizes with LiteLLM's own /callback as its redirect. That URI is same-origin, so the trust policy accepted it, /callback then redirected the authorization response to itself, and the second hit failed to decrypt the client's opaque state, surfacing "Authentication incomplete" and leaving the tool call at 401. The redirect trust check now rejects that URI at /authorize, before the browser leaves for the upstream, and at the /callback sink for states minted earlier, with a hint that names re-registration and MCP_TRUSTED_REDIRECT_ORIGINS. --- .../mcp_server/discoverable_endpoints.py | 11 +-- .../_experimental/mcp_server/oauth_utils.py | 36 ++++++++ .../mcp_management_endpoints.py | 3 +- .../mcp_server/test_discoverable_endpoints.py | 90 +++++++++++++++++++ 4 files changed, 134 insertions(+), 6 deletions(-) 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 /