fix(mcp): remove auth gate from OAuth broker authorize and token endpoints - #27106
Closed
Sameerlite wants to merge 12 commits into
Closed
fix(mcp): remove auth gate from OAuth broker authorize and token endpoints#27106Sameerlite wants to merge 12 commits into
Sameerlite wants to merge 12 commits into
Conversation
…oints
Browser-initiated OAuth flows cannot send an API key, so requiring
user_api_key_auth on /server/oauth/{id}/authorize and /server/oauth/{id}/token
caused a 401 for all end users. Remove the dependency from both endpoints and
make user_api_key_dict optional in _get_cached_temporary_mcp_server_or_404 so
unauthenticated OAuth browser flows skip the admin-view gate.
Add regression tests:
- unit tests for loopback validation, state round-trip, and token validation
- respx HTTP integration tests covering the full authorize → callback → token flow
- Playwright E2E: Layer 1 directly asserts /authorize returns !401 without an API key; Layer 2 asserts the full UI OAuth form flow succeeds
- extend test-mcp.yml CI job to run both new test files
Co-authored-by: Cursor <cursoragent@cursor.com>
…r existing CI job Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Greptile Summary
Confidence Score: 4/5Safe to merge; the core auth regression fix is correct and the new 403 gate for unauthenticated global-registry access is properly implemented and tested. Only P2 findings (redundant header iteration and a tracing gap). The security logic is sound and covered by new integration tests. Score kept at 4 rather than 5 to reflect the security-sensitive nature of the change and a minor unit-test coverage gap for the authenticated admin path through the broker. litellm/proxy/management_endpoints/mcp_management_endpoints.py and litellm/proxy/auth/user_api_key_auth.py — both touch the authentication layer and warrant careful review.
|
| Filename | Overview |
|---|---|
| litellm/proxy/auth/user_api_key_auth.py | Refactored user_api_key_auth into a thin FastAPI shim plus a shared run_user_api_key_auth_pipeline; adds user_api_key_auth_from_request_headers helper for routes that cannot use FastAPI dependency injection. @tracer.wrap() is preserved on the shim but not on the pipeline, creating a tracing gap for the new OAuth broker path. |
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | Removes user_api_key_auth dependency from /authorize and /token; adds _try_resolve_mcp_oauth_broker_user for optional auth resolution and tightens access-control in _get_cached_temporary_mcp_server_or_404 to block unauthenticated access to global-registry servers (403). Minor code smell: redundant case-insensitive header iteration. |
| tests/mcp_tests/test_mcp_oauth_flow_http_respx.py | New integration tests covering both the management broker routes (the actual regression fix) and the discoverable OAuth router; includes regression guard (404 not 401), 403 for unauthenticated global-registry access, and positive temp-session path. All calls are mocked via ASGITransport/respx — no real network calls. |
| tests/mcp_tests/test_mcp_oauth_security_unit.py | New unit tests for loopback URI validation, state encrypt/decrypt round-trip, and redirect URI guard logic. All tests are pure mock/unit with no network calls. |
| tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py | Updated existing tests to adapt to the removed user_api_key_dict parameter: mocks _try_resolve_mcp_oauth_broker_user returning None and updates assertions accordingly. Tests now only verify the unauthenticated (None) path through the broker; authenticated admin path is not unit-tested here. |
| ui/litellm-dashboard/e2e_tests/tests/mcp/mcp_oauth_flow.spec.ts | New Playwright E2E spec with two layers: direct API assertion (no Authorization header must not return 401) and full UI flow using page.route intercepts to simulate the authorize redirect and token exchange. |
| ui/litellm-dashboard/e2e_tests/playwright.oauth.config.ts | New minimal Playwright config for the MCP OAuth E2E test; single-worker, no global setup, targets a live proxy on port 4000. |
Reviews (5): Last reviewed commit: "Fix failing tests" | Re-trigger Greptile
…ready covered Co-authored-by: Cursor <cursoragent@cursor.com>
…orize and token endpoints Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@greptile-apps re review |
…n 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 <cursoragent@cursor.com>
Contributor
Author
|
@greptile-apps re review |
Bojun-Vvibe
added a commit
to Bojun-Vvibe/oss-contributions
that referenced
this pull request
May 4, 2026
BerriAI/litellm#27106 (mcp oauth auth-gate fix), charmbracelet/crush#2606 (split-pane+pty infra), google-gemini/gemini-cli#26249 (hide read-only settings scopes).
Contributor
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Silently ignored property passed as constructor argument
- Replaced the ignored computed-property constructor argument with the real oauth2_flow field so the fixture no longer enters the per-user token branch unintentionally.
Preview (a969c974d9)
diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py
--- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py
+++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py
@@ -1450,7 +1450,7 @@
async def _get_cached_temporary_mcp_server_or_404(
server_id: str,
- user_api_key_dict: UserAPIKeyAuth,
+ user_api_key_dict: Optional[UserAPIKeyAuth] = None,
request: Optional[Request] = None,
) -> MCPServer:
server = await get_cached_temporary_mcp_server(server_id)
@@ -1472,12 +1472,37 @@
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.
- if 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,
@@ -1498,12 +1523,10 @@
@router.get(
"/server/oauth/{server_id}/authorize",
include_in_schema=False,
- dependencies=[Depends(user_api_key_auth)],
)
async def mcp_authorize(
request: Request,
server_id: str,
- user_api_key_dict: UserAPIKeyAuth = Depends(user_api_key_auth),
client_id: Optional[str] = None,
redirect_uri: str = Query(...),
state: str = "",
@@ -1513,7 +1536,7 @@
scope: Optional[str] = None,
):
mcp_server = await _get_cached_temporary_mcp_server_or_404(
- server_id, user_api_key_dict, request=request
+ server_id, request=request
)
# Use the server's stored client_id when the caller doesn't supply one
resolved_client_id = mcp_server.client_id or client_id or ""
@@ -1543,12 +1566,10 @@
@router.post(
"/server/oauth/{server_id}/token",
include_in_schema=False,
- dependencies=[Depends(user_api_key_auth)],
)
async def mcp_token(
request: Request,
server_id: str,
- user_api_key_dict: UserAPIKeyAuth = Depends(user_api_key_auth),
grant_type: str = Form(...),
code: Optional[str] = Form(None),
redirect_uri: Optional[str] = Form(None),
@@ -1559,7 +1580,7 @@
scope: Optional[str] = Form(None),
):
mcp_server = await _get_cached_temporary_mcp_server_or_404(
- server_id, user_api_key_dict, request=request
+ server_id, request=request
)
resolved_client_id = mcp_server.client_id or client_id or ""
if not resolved_client_id:
diff --git a/tests/mcp_tests/test_mcp_oauth_flow_http_respx.py b/tests/mcp_tests/test_mcp_oauth_flow_http_respx.py
new file mode 100644
--- /dev/null
+++ b/tests/mcp_tests/test_mcp_oauth_flow_http_respx.py
@@ -1,0 +1,417 @@
+"""
+HTTP-level integration tests for MCP OAuth.
+
+Covers two layers:
+1. Management broker endpoints (mcp_management_endpoints.py) — the routes
+ actually modified by the auth-gate regression fix. These tests use
+ ASGITransport to hit the management router directly and assert that
+ /server/oauth/{id}/authorize and /server/oauth/{id}/token do NOT
+ require an API key (regression: they previously returned 401 for browsers).
+
+2. Discoverable OAuth router (discoverable_endpoints.py) — end-to-end
+ authorize → callback → token flow mocked with respx.
+"""
+
+from __future__ import annotations
+
+import urllib.parse
+from typing import Iterator
+from unittest.mock import patch
+
+import httpx
+import litellm
+import pytest
+from fastapi import FastAPI
+from httpx import ASGITransport
+
+from litellm.types.mcp import MCPTransport
+from litellm.types.mcp import MCPAuth
+from litellm.types.mcp_server.mcp_server_manager import MCPServer
+
+
+@pytest.fixture(autouse=True)
+def mock_mcp_client_ip() -> Iterator[None]:
+ with patch(
+ "litellm.proxy._experimental.mcp_server.discoverable_endpoints.IPAddressUtils.get_mcp_client_ip",
+ return_value=None,
+ ):
+ yield
+
+
+@pytest.fixture
+def oauth_asgi_app(monkeypatch) -> Iterator[FastAPI]:
+ monkeypatch.setenv("LITELLM_SALT_KEY", "integration-test-salt-key-32chars")
+ # Outbound token exchange must use httpx so respx can intercept (not aiohttp).
+ monkeypatch.setattr(litellm, "disable_aiohttp_transport", True)
+
+ from litellm.proxy._experimental.mcp_server.discoverable_endpoints import (
+ router as discoverable_oauth_router,
+ )
+ from litellm.proxy._experimental.mcp_server.mcp_server_manager import (
+ global_mcp_server_manager,
+ )
+
+ global_mcp_server_manager.registry.clear()
+ server = MCPServer(
+ server_id="mock-oauth-srv",
+ name="mock_oauth",
+ server_name="mock_oauth",
+ alias="mock_oauth",
+ transport=MCPTransport.http,
+ auth_type=MCPAuth.oauth2,
+ client_id="upstream-client",
+ client_secret="upstream-secret",
+ authorization_url="https://mock-idp.example/oauth/authorize",
+ token_url="https://mock-idp.example/oauth/token",
+ scopes=["openid"],
+ oauth2_flow="client_credentials",
+ )
+ global_mcp_server_manager.registry[server.server_id] = server
+
+ app = FastAPI()
+ app.include_router(discoverable_oauth_router)
+ try:
+ yield app
+ finally:
+ global_mcp_server_manager.registry.clear()
+
+
+@pytest.mark.asyncio
+@pytest.mark.respx
+async def test_authorize_redirect_uri_to_upstream_is_proxy_callback_not_client_loopback(
+ oauth_asgi_app: FastAPI,
+) -> None:
+ transport = ASGITransport(app=oauth_asgi_app)
+ async with httpx.AsyncClient(
+ transport=transport, base_url="http://proxy.test", follow_redirects=False
+ ) as client:
+ r = await client.get(
+ "/mock_oauth/authorize",
+ params={
+ "client_id": "upstream-client",
+ "redirect_uri": "http://127.0.0.1:60108/ui/mcp/oauth/callback",
+ "state": "plain-client-state",
+ "code_challenge": "challenge",
+ "code_challenge_method": "S256",
+ },
+ )
+ assert r.status_code in (301, 302, 303, 307, 308)
+ loc = r.headers["location"]
+ assert loc.startswith("https://mock-idp.example/oauth/authorize")
+ q = urllib.parse.urlparse(loc).query
+ parsed = urllib.parse.parse_qs(q)
+ upstream_redirect = parsed["redirect_uri"][0]
+ assert upstream_redirect == "http://proxy.test/callback"
+ assert "challenge" == parsed["code_challenge"][0]
+ encrypted_state = parsed["state"][0]
+ from litellm.proxy._experimental.mcp_server.discoverable_endpoints import (
+ decode_state_hash,
+ )
+
+ state_data = decode_state_hash(encrypted_state)
+ assert state_data["original_state"] == "plain-client-state"
+ assert (
+ state_data["client_redirect_uri"]
+ == "http://127.0.0.1:60108/ui/mcp/oauth/callback"
+ )
+
+
+@pytest.mark.asyncio
+async def test_authorize_rejects_non_loopback_client_redirect_uri(
+ oauth_asgi_app: FastAPI,
+) -> None:
+ transport = ASGITransport(app=oauth_asgi_app)
+ async with httpx.AsyncClient(
+ transport=transport, base_url="http://proxy.test", follow_redirects=False
+ ) as client:
+ r = await client.get(
+ "/mock_oauth/authorize",
+ params={
+ "client_id": "upstream-client",
+ "redirect_uri": "https://attacker.example/capture",
+ "state": "x",
+ },
+ )
+ assert r.status_code == 400
+
+
+@pytest.mark.asyncio
+async def test_callback_redirects_to_client_loopback_with_upstream_code(
+ oauth_asgi_app: FastAPI,
+) -> None:
+ from litellm.proxy._experimental.mcp_server.discoverable_endpoints import (
+ encode_state_with_base_url,
+ )
+
+ client_cb = "http://127.0.0.1:7777/oauth/callback"
+ base_no_query = "http://127.0.0.1:7777/oauth/callback"
+ state_token = encode_state_with_base_url(
+ base_url=base_no_query,
+ original_state="csrf-token-9",
+ code_challenge="cc",
+ code_challenge_method="S256",
+ client_redirect_uri=client_cb,
+ )
+ transport = ASGITransport(app=oauth_asgi_app)
+ async with httpx.AsyncClient(
+ transport=transport, base_url="http://proxy.test", follow_redirects=False
+ ) as client:
+ r = await client.get(
+ "/callback",
+ params={"code": "upstream-auth-code", "state": state_token},
+ )
+ assert r.status_code in (301, 302, 303, 307, 308)
+ loc = r.headers["location"]
+ assert loc.startswith("http://127.0.0.1:7777/oauth/callback")
+ q = urllib.parse.urlparse(loc).query
+ parsed = urllib.parse.parse_qs(q)
+ assert parsed["code"][0] == "upstream-auth-code"
+ assert parsed["state"][0] == "csrf-token-9"
+
+
+@pytest.mark.asyncio
+async def test_callback_rejects_non_loopback_in_decrypted_state(
+ oauth_asgi_app: FastAPI,
+) -> None:
+ with patch(
+ "litellm.proxy._experimental.mcp_server.discoverable_endpoints.decode_state_hash",
+ return_value={
+ "original_state": "ok",
+ "client_redirect_uri": "https://evil.com/y",
+ "base_url": "https://evil.com/y",
+ },
+ ):
+ transport = ASGITransport(app=oauth_asgi_app)
+ async with httpx.AsyncClient(
+ transport=transport, base_url="http://proxy.test", follow_redirects=False
+ ) as client:
+ r = await client.get(
+ "/callback",
+ params={"code": "c", "state": "opaque"},
+ )
+ assert r.status_code == 400
+
+
+@pytest.mark.asyncio
+@pytest.mark.respx
+async def test_token_exchange_posts_proxy_callback_redirect_uri_to_upstream(
+ oauth_asgi_app: FastAPI,
+ respx_mock,
+) -> None:
+ captured: dict = {}
+
+ def on_request(request: httpx.Request) -> httpx.Response:
+ captured["body"] = request.content.decode()
+ return httpx.Response(
+ 200,
+ json={
+ "access_token": "at-upstream",
+ "token_type": "Bearer",
+ "expires_in": 3600,
+ },
+ )
+
+ respx_mock.post("https://mock-idp.example/oauth/token").mock(side_effect=on_request)
+
+ transport = ASGITransport(app=oauth_asgi_app)
+ async with httpx.AsyncClient(
+ transport=transport, base_url="http://proxy.test", follow_redirects=False
+ ) as client:
+ r = await client.post(
+ "/mock_oauth/token",
+ data={
+ "grant_type": "authorization_code",
+ "code": "code-from-upstream",
+ "client_id": "upstream-client",
+ "client_secret": "upstream-secret",
+ "code_verifier": "verifier",
+ "redirect_uri": "ignored-by-litellm-for-upstream-exchange",
+ },
+ )
+ assert r.status_code == 200
+ assert r.headers.get("cache-control") == "no-store"
+ assert r.headers.get("pragma") == "no-cache"
+ body = r.json()
+ assert body["access_token"] == "at-upstream"
+
+ parsed = urllib.parse.parse_qs(captured.get("body", ""))
+ assert parsed["grant_type"][0] == "authorization_code"
+ assert parsed["redirect_uri"][0] == "http://proxy.test/callback"
+ assert parsed["code"][0] == "code-from-upstream"
+ assert parsed["code_verifier"][0] == "verifier"
+
+
+@pytest.mark.asyncio
+@pytest.mark.respx
+async def test_token_exchange_applies_token_validation_rules(
+ oauth_asgi_app: FastAPI,
+ respx_mock,
+) -> None:
+ from litellm.proxy._experimental.mcp_server.mcp_server_manager import (
+ global_mcp_server_manager,
+ )
+
+ srv = global_mcp_server_manager.registry["mock-oauth-srv"]
+ prev_validation = getattr(srv, "token_validation", None)
+ srv.token_validation = {"org_id": "expected-org"}
+ try:
+ respx_mock.post("https://mock-idp.example/oauth/token").respond(
+ 200,
+ json={
+ "access_token": "tok",
+ "token_type": "Bearer",
+ "expires_in": 60,
+ "org_id": "wrong-org",
+ },
+ )
+ transport = ASGITransport(app=oauth_asgi_app)
+ async with httpx.AsyncClient(
+ transport=transport, base_url="http://proxy.test", follow_redirects=False
+ ) as client:
+ r = await client.post(
+ "/mock_oauth/token",
+ data={
+ "grant_type": "authorization_code",
+ "code": "c",
+ "client_id": "upstream-client",
+ "client_secret": "upstream-secret",
+ "code_verifier": "v",
+ },
+ )
+ assert r.status_code == 403
+ err = r.json()
+ assert err["detail"]["error"] == "token_validation_failed"
+ finally:
+ srv.token_validation = prev_validation
+
+
+# ---------------------------------------------------------------------------
+# Regression + security tests: management broker endpoints
+#
+# 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)
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture
+def management_asgi_app(monkeypatch) -> FastAPI:
+ monkeypatch.setenv("LITELLM_SALT_KEY", "integration-test-salt-key-32chars")
+ from litellm.proxy.management_endpoints.mcp_management_endpoints import router
+
+ app = FastAPI()
+ app.include_router(router)
+ return app
+
+
+@pytest.mark.asyncio
+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,
+ base_url="http://test",
+ follow_redirects=False,
+ ) as client:
+ r = await client.get(
+ "/v1/mcp/server/oauth/nonexistent-server-id/authorize",
+ params={
+ "redirect_uri": "http://127.0.0.1:8080/callback",
+ "state": "regression-test-state",
+ "response_type": "code",
+ "code_challenge": "abc123",
+ "code_challenge_method": "S256",
+ "client_id": "test-client",
+ },
+ )
+ assert r.status_code != 401, (
+ "Got 401 — user_api_key_auth was re-added to /authorize. " f"Response: {r.text}"
+ )
+ assert r.status_code == 404
+
+
+@pytest.mark.asyncio
+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,
+ base_url="http://test",
+ follow_redirects=False,
+ ) as client:
+ r = await client.post(
+ "/v1/mcp/server/oauth/nonexistent-server-id/token",
+ data={
+ "grant_type": "authorization_code",
+ "code": "test-code",
+ "redirect_uri": "http://127.0.0.1:8080/callback",
+ "client_id": "test-client",
+ },
+ )
+ assert r.status_code != 401, (
+ "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)
diff --git a/tests/mcp_tests/test_mcp_oauth_security_unit.py b/tests/mcp_tests/test_mcp_oauth_security_unit.py
new file mode 100644
--- /dev/null
+++ b/tests/mcp_tests/test_mcp_oauth_security_unit.py
@@ -1,0 +1,119 @@
+"""Unit tests for MCP OAuth broker security helpers (discoverable / UI flow).
+
+``_validate_token_response`` rules are covered in ``tests/mcp_tests/test_per_user_oauth_cache.py``.
+"""
+
+from __future__ import annotations
+
+import pytest
+from fastapi import HTTPException
+
+from litellm.proxy._experimental.mcp_server.discoverable_endpoints import (
+ _get_validated_client_redirect_uri,
+ decode_state_hash,
+ encode_state_with_base_url,
+)
+from litellm.proxy._experimental.mcp_server.oauth_utils import (
+ validate_loopback_redirect_uri,
+)
+
+
+@pytest.mark.parametrize(
+ "uri",
+ [
+ "https://evil.com/callback",
+ "http://192.168.1.1/callback",
+ "http://10.0.0.1/callback",
+ "http://example.com/callback",
+ ],
+)
+def test_validate_loopback_redirect_uri_rejects_non_loopback(uri: str) -> None:
+ with pytest.raises(HTTPException) as exc:
+ validate_loopback_redirect_uri(uri)
+ assert exc.value.status_code == 400
+
+
+@pytest.mark.parametrize(
+ "uri",
+ [
+ "http://127.0.0.1:9/callback",
+ "http://127.0.0.2:4000/ui/mcp/oauth/callback",
+ "http://localhost:3000/cb",
+ "http://[::1]:8080/oauth/callback",
+ ],
+)
+def test_validate_loopback_redirect_uri_accepts_loopback(uri: str) -> None:
+ validate_loopback_redirect_uri(uri)
+
+
+def test_encode_state_with_base_url_decode_state_hash_roundtrip(monkeypatch) -> None:
+ """State must survive encrypt → decrypt with a stable salt (CI-safe)."""
+ monkeypatch.setenv("LITELLM_SALT_KEY", "unit-test-salt-key-32chars!!!")
+
+ enc = encode_state_with_base_url(
+ base_url="http://127.0.0.1:60108/callback",
+ original_state="client-state-xyz",
+ code_challenge="cc",
+ code_challenge_method="S256",
+ client_redirect_uri="http://127.0.0.1:60108/callback",
+ )
+ assert enc != ""
+ data = decode_state_hash(enc)
+ assert data["base_url"] == "http://127.0.0.1:60108/callback"
+ assert data["original_state"] == "client-state-xyz"
+ assert data["code_challenge"] == "cc"
+ assert data["code_challenge_method"] == "S256"
+ assert data["client_redirect_uri"] == "http://127.0.0.1:60108/callback"
+
+
+def test_get_validated_client_redirect_uri_accepts_loopback_from_state() -> None:
+ uri = _get_validated_client_redirect_uri(
+ {
+ "client_redirect_uri": "http://127.0.0.1:55/x",
+ "base_url": "ignored-when-client-set",
+ }
+ )
+ assert uri == "http://127.0.0.1:55/x"
+
+
+def test_get_validated_client_redirect_uri_falls_back_to_base_url_loopback() -> None:
+ uri = _get_validated_client_redirect_uri(
+ {
+ "original_state": "s",
+ "base_url": "http://localhost:9/oauth",
+ }
+ )
+ assert uri == "http://localhost:9/oauth"
+
+
+def test_get_validated_client_redirect_uri_rejects_public_client_redirect() -> None:
+ with pytest.raises(HTTPException) as exc:
+ _get_validated_client_redirect_uri(
+ {
+ "client_redirect_uri": "https://evil.com/steal",
+ "base_url": "http://127.0.0.1:1/x",
+ }
+ )
+ assert exc.value.status_code == 400
+
+
+def test_get_validated_client_redirect_uri_rejects_public_base_url_fallback() -> None:
+ with pytest.raises(HTTPException) as exc:
+ _get_validated_client_redirect_uri({"base_url": "https://evil.com/noloop"})
+ assert exc.value.status_code == 400
+
+
+def test_get_validated_client_redirect_uri_empty_client_uses_loopback_base_url() -> (
+ None
+):
+ """When client_redirect_uri is absent/empty, base_url must still be loopback-validated."""
+ uri = _get_validated_client_redirect_uri(
+ {"client_redirect_uri": "", "base_url": "http://127.0.0.1:1/x"}
+ )
+ assert uri == "http://127.0.0.1:1/x"
+
+
+def test_get_validated_client_redirect_uri_rejects_missing_uri() -> None:
+ with pytest.raises(HTTPException) as exc:
+ _get_validated_client_redirect_uri({"original_state": "x"})
+ assert exc.value.status_code == 400
diff --git a/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py
--- a/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py
+++ b/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py
@@ -1560,10 +1560,6 @@
request = MagicMock()
server = generate_mock_mcp_server_config_record(server_id="server-1")
authorize_response = MagicMock()
- admin_auth = generate_mock_user_api_key_auth(
- user_role=LitellmUserRoles.PROXY_ADMIN,
- )
-
with (
patch(
"litellm.proxy.management_endpoints.mcp_management_endpoints._get_cached_temporary_mcp_server_or_404",
@@ -1577,7 +1573,6 @@
result = await mcp_authorize(
request=request,
server_id="server-1",
- user_api_key_dict=admin_auth,
client_id="client-id",
redirect_uri="https://example.com/callback",
state="state123",
@@ -1588,7 +1583,7 @@
)
assert result is authorize_response
- get_server.assert_awaited_once_with("server-1", admin_auth, request=request)
+ get_server.assert_awaited_once_with("server-1", request=request)
authorize_mock.assert_awaited_once_with(
request=request,
mcp_server=server,
@@ -1610,9 +1605,6 @@
request = MagicMock()
server = generate_mock_mcp_server_config_record(server_id="server-1")
exchange_response = {"access_token": "token"}
- admin_auth = generate_mock_user_api_key_auth(
- user_role=LitellmUserRoles.PROXY_ADMIN,
- )
with (
patch(
@@ -1627,7 +1619,6 @@
result = await mcp_token(
request=request,
server_id="server-1",
- user_api_key_dict=admin_auth,
grant_type="authorization_code",
code="code-123",
redirect_uri="https://example.com/callback",
@@ -1639,7 +1630,7 @@
)
assert result is exchange_response
- get_server.assert_awaited_once_with("server-1", admin_auth, request=request)
+ get_server.assert_awaited_once_with("server-1", request=request)
exchange_mock.assert_awaited_once_with(
request=request,
mcp_server=server,
@@ -1662,9 +1653,6 @@
request = MagicMock()
server = generate_mock_mcp_server_config_record(server_id="server-1")
exchange_response = {"access_token": "new-token", "refresh_token": "new-rt"}
- admin_auth = generate_mock_user_api_key_auth(
- user_role=LitellmUserRoles.PROXY_ADMIN,
- )
with (
patch(
@@ -1679,7 +1667,6 @@
result = await mcp_token(
request=request,
server_id="server-1",
- user_api_key_dict=admin_auth,
grant_type="refresh_token",
code=None,
redirect_uri=None,
@@ -1691,7 +1678,7 @@
)
assert result is exchange_response
- get_server.assert_awaited_once_with("server-1", admin_auth, request=request)
+ get_server.assert_awaited_once_with("server-1", request=request)
exchange_mock.assert_awaited_once_with(
request=request,
mcp_server=server,
diff --git a/ui/litellm-dashboard/e2e_tests/playwright.oauth.config.ts b/ui/litellm-dashboard/e2e_tests/playwright.oauth.config.ts
new file mode 100644
--- /dev/null
+++ b/ui/litellm-dashboard/e2e_tests/playwright.oauth.config.ts
@@ -1,0 +1,31 @@
+/**
+ * Minimal Playwright config for the MCP OAuth E2E test.
+ * Runs against a pre-existing proxy on port 4000 with no globalSetup.
+ */
+import { defineConfig, devices } from "@playwright/test";
+
+export default defineConfig({
+ testDir: ".",
+ testMatch: ["tests/mcp/mcp_oauth_flow.spec.ts"],
+ fullyParallel: false,
+ retries: 0,
+ workers: 1,
+ reporter: [["list"], ["html", { outputFolder: "playwright-report-oauth" }]],
+ use: {
+ baseURL: "http://localhost:4000",
+ trace: "on-first-retry",
+ actionTimeout: 20 * 1000,
+ navigationTimeout: 45 * 1000,
+ },
+ projects: [
+ {
+ name: "chromium",
+ use: { ...devices["Desktop Chrome"] },
+ },
+ ],
+ timeout: 5 * 60 * 1000,
+ expect: {
+ timeout: 15 * 1000,
+ },
+ // No globalSetup — the test handles its own login
+});
diff --git a/ui/litellm-dashboard/e2e_tests/tests/mcp/mcp_oauth_flow.spec.ts b/ui/litellm-dashboard/e2e_tests/tests/mcp/mcp_oauth_flow.spec.ts
new file mode 100644
--- /dev/null
+++ b/ui/litellm-dashboard/e2e_tests/tests/mcp/mcp_oauth_flow.spec.ts
@@ -1,0 +1,160 @@
+/**
+ * E2E test: MCP OAuth flow (Interactive / PKCE)
+ *
+ * Two-layer regression coverage:
+ *
+ * Layer 1 — API assertion (catches the original auth bug directly)
+ * ---------------------------------------------------------------
+ * Makes a real GET to /v1/mcp/server/oauth/{id}/authorize with NO
+ * Authorization header. If someone re-adds `user_api_key_auth` to
+ * that route, the proxy returns 401 and this assertion fails immediately.
+ * With auth absent the proxy returns 404 (unknown server) or 307
+ * (valid server), never 401.
+ *
+ * Layer 2 — Full UI flow (catches UI / OAuth wiring regressions)
+ * ---------------------------------------------------------------
+ * Logs in, fills the "Add MCP Server" form with OAuth settings, clicks
+ * "Authorize & Fetch Token", and asserts "Token fetched." appears.
+ *
+ * Intercept strategy for Layer 2:
+ * A. /v1/mcp/server/oauth/{*}/authorize* — return an HTML page that
+ * writes the fake OAuth result to sessionStorage (same encoding as
+ * setSecureItem) then navigates back, so the real resumeOAuthFlow()
+ * hook picks it up.
... diff truncated: showing 800 of 937 linesYou can send follow-ups to the cloud agent here.
|
|
Contributor
Author
|
bugbot run |
Contributor
Author
|
@greptile-apps re review |
Contributor
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a969c97. Configure here.
mateo-berri
reviewed
May 5, 2026
mateo-berri
left a comment
mateo-berri
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Thanks!
Just some small non-blocking issues
Resolve proxy credentials on OAuth broker /authorize and /token when Authorization is present so allowlist and non-admin temp-cache rules apply. Add HTTP test that seeds a temp MCP server and asserts unauthenticated GET /authorize redirects to the upstream IdP. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
bugbot run |
Contributor
Author
|
@greptile-apps re review |
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Optional auth only checks Authorization header, ignoring others
- Updated optional MCP OAuth broker auth detection to recognize every supported LiteLLM API-key header before invoking the auth pipeline.
You can send follow-ups to the cloud agent here.
Previously _try_resolve_mcp_oauth_broker_user only inspected the 'Authorization' header to decide whether to run the auth pipeline. user_api_key_auth_from_request_headers supports six auth headers: Authorization, API-Key, x-api-key, x-goog-api-key, Ocp-Apim-Subscription-Key, and x-litellm-api-key. Callers authenticating via any of the alternative headers (e.g. Azure's API-Key) had their credentials silently ignored, causing them to be treated as unauthenticated. This would bypass admin/allowlist access control for temp-cache servers, and result in a 403 for global-registry servers they otherwise have permission to use. Fix: iterate over all six header names before deciding whether to skip the full auth pipeline call. Co-authored-by: Sameer Kankute <Sameerlite@users.noreply.github.com>
Contributor
Author
|
bugbot run |
Contributor
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 23fc23e. Configure here.
Contributor
Author
|
@mateo-berri I have incorporated your suggestions. THanks! Please re review https://app.circleci.com/pipelines/github/BerriAI/litellm/76398. I can see that CI is itself failing from litellm_internal_staging. THis is not because of the changes in this PR |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
Author
|
@mateo-berri Passing all tests now |
mateo-berri
self-requested a review
May 5, 2026 20:22
7 tasks
yuneng-berri
requested changes
May 5, 2026
| @router.get( | ||
| "/server/oauth/{server_id}/authorize", | ||
| include_in_schema=False, | ||
| dependencies=[Depends(user_api_key_auth)], |
Collaborator
There was a problem hiding this comment.
This looks like an insecure workaround, will message on Slack
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
/v1/mcp/server/oauth/{server_id}/authorizeand/v1/mcp/server/oauth/{server_id}/tokenwere protected byuser_api_key_auth. Browser-initiated OAuth flows have no API key to send — the user's browser navigates directly to these URLs — so every end user got a 401.Root cause: a recent commit added
dependencies=[Depends(user_api_key_auth)]anduser_api_key_dict: UserAPIKeyAuth = Depends(user_api_key_auth)to both routes.Fix
user_api_key_authdependency from/authorizeand/tokenuser_api_key_dictoptional in_get_cached_temporary_mcp_server_or_404; unauthenticated OAuth browser requests skip the admin-view gate (same behaviour as before the regression was introduced)Tests
Unit (
test_mcp_oauth_security_unit.py): loopback redirect validation, state encrypt/decrypt round-trip, token response validationIntegration (
test_mcp_oauth_flow_http_respx.py): full authorize → callback → token flow usingrespxto mock the upstream IdP, asserting correct redirect_uri, encrypted state, no-cache headers, and loopback enforcementE2E (
e2e_tests/tests/mcp/mcp_oauth_flow.spec.ts):/authorizewithout anyAuthorizationheader must not return 401 (catches auth being re-added immediately)https://www.loom.com/share/b0eb04c554ce46028675a5ca9fc639cf
Note
Medium Risk
Touches authentication/authorization behavior on MCP OAuth broker endpoints; while it fixes a browser OAuth regression, mistakes could inadvertently allow unauthorized access to globally configured MCP servers.
Overview
Fixes a regression where MCP OAuth broker routes (
/v1/mcp/server/oauth/{server_id}/authorizeand/token) were hard-gated byuser_api_key_auth, causing browser-initiated OAuth flows to fail with 401.Adds an internal auth helper (
run_user_api_key_auth_pipeline+user_api_key_auth_from_request_headers) and updates the broker endpoints to optionally resolve credentials from request headers; access control is adjusted so unauthenticated requests are only allowed for temp-cached session servers, while unauthenticated access to global-registry servers is explicitly rejected (403) and authenticated non-admins must still pass allowlist checks.Adds new unit/integration coverage for the OAuth broker security rules and redirect/state handling, plus a Playwright E2E test that asserts
/authorizenever returns 401 without an API key and that the UI flow can complete token fetch.Reviewed by Cursor Bugbot for commit 23fc23e. Bugbot is set up for automated code reviews on this repo. Configure here.