From 8a7dda5a6f2bf3ca94514664eb5335fee9c99c12 Mon Sep 17 00:00:00 2001 From: Sameer Kankute Date: Mon, 4 May 2026 13:10:28 +0530 Subject: [PATCH] fix(mcp): restrict unauthenticated OAuth broker bypass to temp-session servers only Unauthenticated callers (browser OAuth flows) were bypassing access controls for all server types, including global-registry servers. This created a privilege inversion: an unauthenticated caller could invoke the proxy OAuth broker (which uses the server's stored client_secret) while an authenticated non-admin without allowlist access received 403. Restrict the no-auth bypass to temp-session servers only (resolved_from_temp_cache=True). The LiteLLM UI always creates a temp session via /server/oauth/session before calling /authorize, so legitimate browser flows are unaffected. Unauthenticated access to global-registry servers now returns 403. Add test_management_broker_rejects_unauthenticated_access_to_global_registry_server to verify the new protection. Also fix URL prefix (/v1/mcp) in existing management broker regression tests so they actually reach the endpoint. Co-authored-by: Cursor --- .../mcp_management_endpoints.py | 40 +++++++--- .../test_mcp_oauth_flow_http_respx.py | 74 +++++++++++++++++-- 2 files changed, 98 insertions(+), 16 deletions(-) diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index 22eddd448e0..abd7563eaba 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -1472,15 +1472,37 @@ if MCP_AVAILABLE: detail={"error": f"MCP server {server_id} not found"}, ) - # Per-server access policy mirrors `fetch_mcp_server`: admin-view - # callers are unrestricted; non-admins must have the server in their - # allowed-servers set. Temporary cached servers come from the - # admin-only `/server/oauth/session` setup flow and are not exposed - # to non-admins. Unauthenticated OAuth browser flows omit the key and - # skip this gate (same as pre-broker-auth behavior on authorize/token). - if user_api_key_dict is not None and not _user_has_admin_view( - user_api_key_dict - ): + # Access-control for the OAuth broker endpoints. + # + # Unauthenticated callers (browser-initiated OAuth, no API key): + # - Temp-cache servers: allowed. These are created by admins via the + # admin-only /server/oauth/session endpoint specifically to drive + # this browser flow. The LiteLLM UI always creates a temp session + # before calling /authorize, so all legitimate browser flows use a + # temp server_id. + # - Global-registry servers: rejected (403). Allowing unauthenticated + # access to global-registry servers would let any caller invoke the + # proxy's OAuth broker with the server's stored client_secret, while + # authenticated non-admins without allowlist access receive 403 — + # an unintended privilege inversion. + # + # Authenticated callers: + # - Admins: unrestricted. + # - Non-admins: temp servers are always denied (temp sessions are + # admin-internal); global servers require allowlist membership. + if user_api_key_dict is None: + if not resolved_from_temp_cache: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail={ + "error": ( + "Unauthenticated access to global-registry MCP server " + f"{server_id} is not permitted. " + "Pass a valid API key or use a session-scoped server ID." + ) + }, + ) + elif not _user_has_admin_view(user_api_key_dict): if resolved_from_temp_cache: raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, diff --git a/tests/mcp_tests/test_mcp_oauth_flow_http_respx.py b/tests/mcp_tests/test_mcp_oauth_flow_http_respx.py index 529b4b9c27e..f6d6f710a8e 100644 --- a/tests/mcp_tests/test_mcp_oauth_flow_http_respx.py +++ b/tests/mcp_tests/test_mcp_oauth_flow_http_respx.py @@ -286,12 +286,16 @@ async def test_token_exchange_applies_token_validation_rules( # --------------------------------------------------------------------------- -# Regression tests: management broker endpoints must not require an API key +# Regression + security tests: management broker endpoints # -# These tests hit the exact routes modified in mcp_management_endpoints.py -# (/server/oauth/{server_id}/authorize and /server/oauth/{server_id}/token). -# A 401 means user_api_key_auth was re-added to the route; any other status -# (404 = server not found is expected here) means the auth gate is absent. +# Three cases are verified against the exact routes in mcp_management_endpoints.py: +# +# 1. Nonexistent server_id → 404 (not 401, which would mean auth was re-added) +# 2. Global-registry server + no API key → 403 (unauthenticated callers must +# not be able to invoke the OAuth broker for globally configured servers, +# as doing so would use the proxy's stored client_secret) +# 3. Temp-session server + no API key → passes access check (browser OAuth +# flow; temp sessions are admin-created and scoped to this flow) # --------------------------------------------------------------------------- @@ -309,6 +313,7 @@ def management_asgi_app(monkeypatch) -> FastAPI: async def test_management_broker_authorize_requires_no_api_key( management_asgi_app: FastAPI, ) -> None: + """Nonexistent server → 404, not 401 (auth gate must not be present).""" transport = ASGITransport(app=management_asgi_app) async with httpx.AsyncClient( transport=transport, @@ -316,7 +321,7 @@ async def test_management_broker_authorize_requires_no_api_key( follow_redirects=False, ) as client: r = await client.get( - "/server/oauth/nonexistent-server-id/authorize", + "/v1/mcp/server/oauth/nonexistent-server-id/authorize", params={ "redirect_uri": "http://127.0.0.1:8080/callback", "state": "regression-test-state", @@ -336,6 +341,7 @@ async def test_management_broker_authorize_requires_no_api_key( async def test_management_broker_token_requires_no_api_key( management_asgi_app: FastAPI, ) -> None: + """Nonexistent server → 404, not 401 (auth gate must not be present).""" transport = ASGITransport(app=management_asgi_app) async with httpx.AsyncClient( transport=transport, @@ -343,7 +349,7 @@ async def test_management_broker_token_requires_no_api_key( follow_redirects=False, ) as client: r = await client.post( - "/server/oauth/nonexistent-server-id/token", + "/v1/mcp/server/oauth/nonexistent-server-id/token", data={ "grant_type": "authorization_code", "code": "test-code", @@ -355,3 +361,57 @@ async def test_management_broker_token_requires_no_api_key( "Got 401 — user_api_key_auth was re-added to /token. " f"Response: {r.text}" ) assert r.status_code == 404 + + +@pytest.mark.asyncio +async def test_management_broker_rejects_unauthenticated_access_to_global_registry_server( + management_asgi_app: FastAPI, +) -> None: + """ + Unauthenticated callers must not reach the OAuth broker for a global-registry + server. Allowing it would let anyone invoke the proxy's OAuth broker using the + server's stored client_secret, while authenticated non-admins without allowlist + access get 403 — an unintended privilege inversion. + """ + from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( + global_mcp_server_manager, + ) + from litellm.types.mcp import MCPAuth, MCPTransport + + server = MCPServer( + server_id="global-oauth-srv", + name="global_oauth", + server_name="global_oauth", + alias="global_oauth", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + client_id="client", + client_secret="secret", + authorization_url="https://idp.example/oauth/authorize", + token_url="https://idp.example/oauth/token", + ) + global_mcp_server_manager.registry["global-oauth-srv"] = server + try: + transport = ASGITransport(app=management_asgi_app) + async with httpx.AsyncClient( + transport=transport, + base_url="http://test", + follow_redirects=False, + ) as client: + r = await client.get( + "/v1/mcp/server/oauth/global-oauth-srv/authorize", + params={ + "redirect_uri": "http://127.0.0.1:8080/callback", + "state": "test", + "response_type": "code", + "code_challenge": "abc", + "code_challenge_method": "S256", + "client_id": "client", + }, + ) + assert r.status_code == 403, ( + f"Expected 403 for unauthenticated access to a global-registry server, " + f"got {r.status_code}. Response: {r.text}" + ) + finally: + global_mcp_server_manager.registry.pop("global-oauth-srv", None)