fix(mcp): drop phantom 401 span on delegated OAuth2 tool calls (#30494)

The OAuth2 passthrough ran user_api_key_auth on the client's upstream bearer
first and only recovered after the failed validation had already logged a 401
auth event to the tracer, so successful tool calls to a delegated server each
carried a phantom 401 span. Check delegate_auth_to_upstream before validating:
a delegated server skips the doomed call entirely so nothing is logged, and a
non-delegated server validates normally and surfaces a real 401 rather than
being exchanged for an anonymous upstream-passthrough session.
This commit is contained in:
ryan-crabbe-berri 2026-06-15 17:24:18 -07:00 committed by GitHub
parent 45d5153c12
commit 039a2d8bf5
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 107 additions and 183 deletions

View file

@ -67,9 +67,10 @@ def _is_mcp_passthrough_cold_start(
spec-compliant WWW-Authenticate challenge instead of surfacing a generic
admission error.
Uses "all" semantics (mirrors :meth:`MCPRequestHandler._target_servers_use_oauth2`):
one non-passthrough target in a co-targeted set must not flip the bypass
open for the others. Fails closed when any target cannot be resolved."""
Uses "all" semantics (mirrors
:meth:`MCPRequestHandler._target_servers_delegate_auth_to_upstream`): one
non-passthrough target in a co-targeted set must not flip the bypass open
for the others. Fails closed when any target cannot be resolved."""
if not mcp_servers:
return False
from litellm.proxy._experimental.mcp_server.mcp_server_manager import (
@ -214,101 +215,64 @@ class MCPRequestHandler:
# Only OAuth metadata routes registered under /.well-known/ are public.
if request_route.startswith("/.well-known/"):
validated_user_api_key_auth = UserAPIKeyAuth()
elif (
not litellm_api_key
and MCPRequestHandler._target_servers_delegate_auth_to_upstream( # noqa: E501
path=request_route,
mcp_servers=mcp_servers,
client_ip=IPAddressUtils.get_mcp_client_ip(request),
)
):
# Operator opted this oauth2 server into upstream-delegated auth
# (PKCE passthrough): skip LiteLLM API-key/SSO entirely so the
# client authenticates directly with the upstream MCP server.
# Fires ONLY when neither x-litellm-api-key nor Authorization is
# present. If any LiteLLM key is supplied (primary or secondary
# header), we fall through so user_id is resolved, spend/rate
# limiting apply, and any stored OAuth token can be retrieved
# and forwarded upstream. Gated by
# _target_servers_delegate_auth_to_upstream, which only returns
# True when EVERY target is auth_type=oauth2 AND has the
# delegate_auth_to_upstream flag set — fails closed otherwise.
validated_user_api_key_auth = UserAPIKeyAuth()
elif has_explicit_litellm_key:
# Explicit x-litellm-api-key provided - always validate normally
# An explicit x-litellm-api-key is always a LiteLLM credential, even
# for a delegated server, so validate it: identity / spend / rate
# limits resolve and any stored upstream token can be forwarded.
validated_user_api_key_auth = await user_api_key_auth(
api_key=litellm_api_key, request=request
)
elif MCPRequestHandler._target_servers_delegate_auth_to_upstream(
path=request_route,
mcp_servers=mcp_servers,
client_ip=IPAddressUtils.get_mcp_client_ip(request),
):
# Operator opted this oauth2 server into upstream-delegated auth: the
# client authenticates directly with the upstream MCP server, so any
# Authorization bearer is an upstream token, never a LiteLLM key. Skip
# LiteLLM validation entirely — covering both the no-credential
# discovery request and the authenticated call carrying the upstream
# bearer — so a tool call that succeeds never carries a phantom 401
# auth span; the bearer is forwarded upstream unchanged. Gated by
# _target_servers_delegate_auth_to_upstream, which returns True only
# when EVERY target is auth_type=oauth2 with delegate_auth_to_upstream
# set; fails closed otherwise.
validated_user_api_key_auth = UserAPIKeyAuth()
elif oauth2_headers:
# No x-litellm-api-key, but Authorization header present.
# Could be a LiteLLM key (backward compat) OR an opaque OAuth2 token
# the operator wants forwarded to an upstream OAuth2-mode MCP server.
# Try LiteLLM auth first; on auth failure, only fall back to anonymous
# passthrough when the request actually targets a server whose operator
# configured ``auth_type=oauth2``. For any other server (api_key,
# bearer_token, basic, etc.), a failed LiteLLM auth is a real failure
# and must propagate — otherwise an attacker can exchange any garbage
# bearer for an anonymous session.
# Authorization on a non-delegated server: the bearer must be a real
# LiteLLM credential, so a failed validation is a genuine 401/403 and
# propagates. The sole anonymous fallback is the auth_type=none
# pass-through cold-start (RFC 9728 discovery return), gated on a 401
# so a recognized-but-forbidden key still fails closed.
client_ip = IPAddressUtils.get_mcp_client_ip(request)
try:
validated_user_api_key_auth = await user_api_key_auth(
api_key=litellm_api_key, request=request
)
except (HTTPException, ProxyException) as e:
# HTTPException.status_code is int; ProxyException.code is
# normalized to str in its __init__ but can be ``"None"`` or any
# non-numeric string when the caller didn't supply a numeric
# code, so we compare against both int and str forms rather
# than coercing (``int("None")`` would raise ValueError and
# rewrite the auth error as a 500).
# ProxyException.code is normalized to str (possibly "None"), so
# compare both int and str forms rather than coercing.
status = e.status_code if isinstance(e, HTTPException) else e.code
is_auth_error = status in (401, 403, "401", "403")
is_unauthenticated = status in (401, "401")
client_ip = IPAddressUtils.get_mcp_client_ip(request)
if is_auth_error and MCPRequestHandler._target_servers_use_oauth2(
path=request_route,
mcp_servers=mcp_servers,
client_ip=client_ip,
mcp_servers_from_path = _parse_mcp_server_names_from_path(
request_route, mcp_servers
)
if (
is_unauthenticated
and mcp_servers_from_path is not None
and not _has_client_supplied_mcp_auth(
mcp_auth_header,
mcp_server_auth_headers,
)
and _is_mcp_passthrough_cold_start(
mcp_servers_from_path, client_ip=client_ip
)
):
verbose_logger.debug(
"MCP OAuth2: target server is OAuth2-mode, treating "
"Authorization as upstream OAuth2 token passthrough"
"MCP pass-through return: forwarding Authorization as "
"upstream OAuth token for delegated auth"
)
validated_user_api_key_auth = UserAPIKeyAuth()
elif is_unauthenticated:
# Pass-through cold-start return: per RFC 9728 / MCP
# Authorization spec the client completes upstream OAuth
# discovery and returns with ``Authorization: Bearer
# <upstream-token>``. For ``auth_type=none`` passthrough
# servers that bearer is not a LiteLLM key (auth above
# failed) but is meant to be forwarded upstream
# unchanged. Fall back to anonymous admission so the
# caller is not rejected for following the discovery
# flow without also setting ``x-litellm-api-key``.
# Only trigger on 401 (token unrecognized); a 403 means
# the key WAS recognized but is forbidden (e.g. over
# budget / rate limited) and must propagate so those
# controls are not bypassed via anonymous admission.
mcp_servers_from_path = _parse_mcp_server_names_from_path(
request_route, mcp_servers
)
if (
mcp_servers_from_path is not None
and not _has_client_supplied_mcp_auth(
mcp_auth_header,
mcp_server_auth_headers,
)
and _is_mcp_passthrough_cold_start(
mcp_servers_from_path, client_ip=client_ip
)
):
verbose_logger.debug(
"MCP pass-through return: target server is "
"passthrough, treating Authorization as "
"upstream OAuth token for delegated auth"
)
validated_user_api_key_auth = UserAPIKeyAuth()
else:
raise
else:
raise
else:
@ -412,45 +376,6 @@ class MCPRequestHandler:
return [single_server_match.group(1)]
return [servers_and_path]
@staticmethod
def _target_servers_use_oauth2(
path: str, mcp_servers: Optional[List[str]], client_ip: Optional[str]
) -> bool:
"""
True only when EVERY MCP server the request targets is configured for
``auth_type == oauth2``. If any target is non-OAuth2 or if the target
cannot be resolved at all return False so the caller fails closed.
Used to gate the "treat Authorization as opaque OAuth2 token" fallback
in :meth:`process_mcp_request` so a failed LiteLLM-auth cannot be
exchanged for an anonymous session against a non-OAuth2 server.
"""
# Inline imports avoid a circular dependency: mcp_server_manager imports
# from this module.
from litellm.proxy._experimental.mcp_server.mcp_server_manager import (
global_mcp_server_manager,
)
from litellm.types.mcp import MCPAuth
# Resolve the same target list downstream routing will use. For
# ``/mcp/...`` routes, ``extract_mcp_auth_context`` overrides the
# ``x-mcp-servers`` header with path-derived names, so we must mirror
# that here — otherwise a caller could set the header to a permissive
# server while the path targets a stricter one (header/path TOCTOU).
target_names = MCPRequestHandler._resolve_target_server_names(
path=path, mcp_servers_header=mcp_servers
)
if not target_names:
return False
for name in target_names:
server = global_mcp_server_manager.get_mcp_server_by_name(
name, client_ip=client_ip
)
if server is None or server.auth_type != MCPAuth.oauth2:
return False
return True
@staticmethod
def _target_servers_delegate_auth_to_upstream(
path: str, mcp_servers: Optional[List[str]], client_ip: Optional[str]
@ -472,8 +397,8 @@ class MCPRequestHandler:
)
from litellm.types.mcp import MCPAuth
# See _target_servers_use_oauth2: must mirror the downstream
# header-vs-path override or an attacker could set
# Must mirror the downstream header-vs-path override
# (``extract_mcp_auth_context``) or an attacker could set
# ``x-mcp-servers`` to a delegate-enabled server while the URL path
# targets a non-delegate server, skipping LiteLLM auth for it.
target_names = MCPRequestHandler._resolve_target_server_names(

View file

@ -658,12 +658,11 @@ class TestMCPOAuth2AuthFlow:
async def test_oauth2_token_in_authorization_header_fallback(self):
"""
When only Authorization header is present with a non-LiteLLM OAuth2 token
AND the target server is operator-configured for ``auth_type=oauth2``,
auth should fall back to permissive mode (OAuth2 passthrough).
When only the Authorization header is present with a non-LiteLLM OAuth2
token AND the target server delegates auth to upstream, LiteLLM skips its
own validation entirely (so the upstream token is never mistaken for a
virtual key) and forwards the bearer upstream.
"""
from fastapi import HTTPException
from litellm.types.mcp import MCPAuth
scope = {
@ -675,17 +674,16 @@ class TestMCPOAuth2AuthFlow:
],
}
async def mock_user_api_key_auth_fails(api_key, request):
raise HTTPException(status_code=401, detail="Invalid API key")
oauth2_server = MagicMock()
oauth2_server.auth_type = MCPAuth.oauth2
oauth2_server.delegate_auth_to_upstream = True
oauth2_server.has_client_credentials = False
with (
patch(
"litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp.user_api_key_auth",
side_effect=mock_user_api_key_auth_fails,
),
new_callable=AsyncMock,
) as mock_auth,
patch(
"litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager"
) as mock_mgr,
@ -700,10 +698,10 @@ class TestMCPOAuth2AuthFlow:
raw_headers,
) = await MCPRequestHandler.process_mcp_request(scope)
# Should succeed with default UserAPIKeyAuth (OAuth2 fallback)
assert auth_result is not None
assert isinstance(auth_result, UserAPIKeyAuth)
# OAuth2 headers should contain the token for upstream forwarding
# The upstream token is never validated as a LiteLLM key ...
mock_auth.assert_not_called()
# ... and is preserved for upstream forwarding.
assert (
oauth2_headers.get("Authorization")
== "Bearer atlassian-oauth2-access-token-xyz"
@ -813,11 +811,12 @@ class TestMCPOAuth2AuthFlow:
await MCPRequestHandler.process_mcp_request(scope)
assert exc_info.value.status_code == 500
async def test_proxy_exception_oauth2_fallback(self):
async def test_proxy_exception_non_delegate_oauth2_propagates(self):
"""
user_api_key_auth raises ProxyException (not HTTPException) in production.
The OAuth2 fallback must catch ProxyException with code 401/403 too,
but only when the target server is operator-configured for ``auth_type=oauth2``.
Production raises ProxyException (not HTTPException) on auth failure. For
a non-delegate oauth2 server the bearer is treated as a LiteLLM credential
and a 401 must propagate as a real auth error, not be exchanged for an
anonymous upstream-passthrough session.
"""
from litellm.proxy._types import ProxyException
from litellm.types.mcp import MCPAuth
@ -841,6 +840,8 @@ class TestMCPOAuth2AuthFlow:
oauth2_server = MagicMock()
oauth2_server.auth_type = MCPAuth.oauth2
oauth2_server.delegate_auth_to_upstream = False
oauth2_server.is_oauth_passthrough = False
with (
patch(
@ -852,22 +853,9 @@ class TestMCPOAuth2AuthFlow:
) as mock_mgr,
):
mock_mgr.get_mcp_server_by_name.return_value = oauth2_server
(
auth_result,
mcp_auth_header,
mcp_servers,
mcp_server_auth_headers,
oauth2_headers,
raw_headers,
) = await MCPRequestHandler.process_mcp_request(scope)
# Should succeed with default UserAPIKeyAuth (OAuth2 fallback)
assert auth_result is not None
assert isinstance(auth_result, UserAPIKeyAuth)
assert (
oauth2_headers.get("Authorization")
== "Bearer atlassian-oauth2-access-token-xyz"
)
with pytest.raises(ProxyException) as exc_info:
await MCPRequestHandler.process_mcp_request(scope)
assert str(exc_info.value.code) == "401"
async def test_proxy_exception_non_auth_still_raises(self):
"""
@ -1355,11 +1343,15 @@ class TestMCPOAuth2FallbackTargetGating:
await MCPRequestHandler.process_mcp_request(scope)
assert exc_info.value.status_code == 401
async def test_fallback_allowed_when_target_is_oauth2_mode(self):
async def test_non_delegate_oauth2_does_not_fall_back_to_anonymous(self):
"""
Operator-configured OAuth2 passthrough still works: target server has
``auth_type=oauth2`` failed LiteLLM auth falls back to anonymous so
the bearer can be forwarded to upstream.
An ``auth_type=oauth2`` server that has NOT opted into
``delegate_auth_to_upstream`` must not exchange a failed LiteLLM auth for
an anonymous session: forwarding an arbitrary bearer upstream is only
allowed once the operator explicitly delegates auth. A failed validation
here is a genuine 401 and propagates (which is also what keeps the
success-path trace free of a phantom 401, since no doomed validation runs
for a delegated server).
"""
from fastapi import HTTPException
@ -1389,8 +1381,9 @@ class TestMCPOAuth2FallbackTargetGating:
mock_mgr.get_mcp_server_by_name.return_value = (
TestMCPOAuth2FallbackTargetGating._make_server(MCPAuth.oauth2)
)
auth_result, *_rest = await MCPRequestHandler.process_mcp_request(scope)
assert isinstance(auth_result, UserAPIKeyAuth)
with pytest.raises(HTTPException) as exc_info:
await MCPRequestHandler.process_mcp_request(scope)
assert exc_info.value.status_code == 401
async def test_fallback_allowed_when_target_is_passthrough(self):
"""
@ -1668,19 +1661,16 @@ class TestMCPDelegateAuthToUpstream:
assert isinstance(auth_result, UserAPIKeyAuth)
mock_auth.assert_not_called()
async def test_delegate_with_upstream_token_in_authorization_falls_back_to_anonymous(
async def test_delegate_with_upstream_token_in_authorization_skips_litellm_auth(
self,
):
"""
oauth2 + delegate_auth_to_upstream=True with an upstream OAuth token in
``Authorization`` (not a LiteLLM key): LiteLLM auth is attempted first
(and fails), then the existing oauth2 fallback returns anonymous so the
bearer is forwarded upstream untouched. The delegate branch itself does
not fire when Authorization is present that is what protects spend
tracking for callers using Authorization-style LiteLLM keys.
``Authorization``: the delegate gate fires before any LiteLLM validation,
so ``user_api_key_auth`` is never called and the bearer is forwarded
upstream untouched. Skipping the doomed validation is what keeps a tool
call that actually succeeds from carrying a phantom 401 auth span.
"""
from fastapi import HTTPException
from litellm.types.mcp import MCPAuth
scope = {
@ -1690,14 +1680,11 @@ class TestMCPDelegateAuthToUpstream:
"headers": [(b"authorization", b"Bearer upstream-pkce-token")],
}
async def mock_user_api_key_auth_fails(api_key, request):
raise HTTPException(status_code=401, detail="Invalid API key")
with (
patch(
"litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp.user_api_key_auth",
side_effect=mock_user_api_key_auth_fails,
),
new_callable=AsyncMock,
) as mock_auth,
patch(
"litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager"
) as mock_mgr,
@ -1718,6 +1705,7 @@ class TestMCPDelegateAuthToUpstream:
) = await MCPRequestHandler.process_mcp_request(scope)
assert isinstance(auth_result, UserAPIKeyAuth)
assert oauth2_headers.get("Authorization") == "Bearer upstream-pkce-token"
mock_auth.assert_not_called()
async def test_delegate_off_still_requires_litellm_auth(self):
"""
@ -1912,12 +1900,15 @@ class TestMCPDelegateAuthToUpstream:
assert auth_result.user_id == "real-user"
mock_auth.assert_called_once()
async def test_litellm_key_via_authorization_header_not_bypassed(self):
async def test_authorization_bearer_on_delegate_server_treated_as_upstream(self):
"""
Regression: a LiteLLM key sent via the secondary ``Authorization`` header
(e.g. ``Authorization: Bearer sk-...``) must still trigger normal auth
and not be silently swallowed by the delegate bypass otherwise spend
tracking and rate limiting are skipped for those callers.
On a delegate server the ``Authorization`` header is, by contract, an
upstream token rather than a LiteLLM key even when it is sk-shaped. It
is forwarded upstream without LiteLLM validation, so ``user_api_key_auth``
is not called and no LiteLLM identity is resolved. Callers who need
LiteLLM identity / spend tracking on a delegate server must supply
``x-litellm-api-key`` (see
test_explicit_litellm_key_takes_precedence_over_delegate).
"""
from litellm.types.mcp import MCPAuth
@ -1944,10 +1935,18 @@ class TestMCPDelegateAuthToUpstream:
delegate_auth_to_upstream=True,
)
)
auth_result, *_rest = await MCPRequestHandler.process_mcp_request(scope)
(
auth_result,
_,
_,
_,
oauth2_headers,
_,
) = await MCPRequestHandler.process_mcp_request(scope)
assert isinstance(auth_result, UserAPIKeyAuth)
assert auth_result.user_id == "real-user"
mock_auth.assert_called_once()
assert auth_result.user_id is None
assert oauth2_headers.get("Authorization") == "Bearer sk-1234"
mock_auth.assert_not_called()
async def test_delegate_ignored_for_client_credentials_server(self):
"""