scriptoriumadmin/litellm
1
0
Fork 0
mirror of https://github.com/BerriAI/litellm.git synced 2026-09-15 23:31:29 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 213

fix(mcp): restrict unauthenticated OAuth broker bypass to temp-session servers only

Browse source 
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 <cursoragent@cursor.com>
This commit is contained in:
Sameer Kankute 2026-05-04 13:10:28 +05:30
parent 5bd5ba66a0
commit 8a7dda5a6f
No known key found for this signature in database
2 changed files with 98 additions and 16 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

40
litellm/proxy/management_endpoints/mcp_management_endpoints.py
View file

@ -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,

74
tests/mcp_tests/test_mcp_oauth_flow_http_respx.py
View file

@ -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)