mirror of
https://github.com/BerriAI/litellm.git
synced 2026-08-28 05:25:59 +00:00
fix(mcp): serve token-forwarding servers when oauth discovery fails (#37399)
true_passthrough and oauth_delegate forward the caller's own bearer and mint nothing, so their sessions consume no discovered OAuth endpoint. The discovery completeness gate still failed them closed with a 503 raised before the upstream was ever contacted, which the tools/list fan-out swallowed into HTTP 200 with an empty tool list. Any upstream that publishes no RFC 9728 metadata, including every OpenAPI-backed internal API, was permanently unusable. A failed discovery is now fatal only to servers whose OAuth flow the gateway runs itself. Discovery still runs for the forwarding modes, so /authorize, /token and /register keep resolving their endpoints, and each keeps its own 400 when they are missing. MCPServer.is_client_forwarded_token now owns the mode pair that five call sites spelled inline, one of which had already named it is_client_forwarded_mode.
This commit is contained in:
parent
704cc41f28
commit
f6eaca9069
5 changed files with 161 additions and 10 deletions
|
|
@ -1341,7 +1341,7 @@ async def _persist_dcr_client_registration(
|
|||
``update_mcp_server`` merges credential blobs: a re-registered public client must not
|
||||
inherit the previous client's secret or auth method.
|
||||
"""
|
||||
if mcp_server.is_true_passthrough or mcp_server.is_oauth_delegate:
|
||||
if mcp_server.is_client_forwarded_token:
|
||||
return "skipped"
|
||||
|
||||
try:
|
||||
|
|
@ -2187,7 +2187,7 @@ async def _build_oauth_protected_resource_response(
|
|||
)
|
||||
|
||||
if upstream_metadata is not None:
|
||||
if mcp_server.is_true_passthrough or mcp_server.is_oauth_delegate:
|
||||
if mcp_server.is_client_forwarded_token:
|
||||
return upstream_metadata
|
||||
return {**upstream_metadata, "resource": resource_url}
|
||||
|
||||
|
|
|
|||
|
|
@ -1740,12 +1740,14 @@ class MCPServerManager:
|
|||
server: The MCP server whose OAuth metadata must be resolved.
|
||||
|
||||
Returns:
|
||||
The resolved server, or the registered server when no discovery is
|
||||
pending.
|
||||
The resolved server; the registered server when no discovery is
|
||||
pending, or when discovery failed for a client-forwarded-token
|
||||
server, whose session consumes no discovered endpoint.
|
||||
|
||||
Raises:
|
||||
HTTPException: Status 503 when discovery times out or returns
|
||||
incomplete metadata.
|
||||
incomplete metadata for a server whose OAuth flow the gateway
|
||||
runs itself.
|
||||
"""
|
||||
acquisition: Final = self._get_or_start_oauth_discovery_task(server)
|
||||
if acquisition is None:
|
||||
|
|
@ -1764,6 +1766,8 @@ class MCPServerManager:
|
|||
return await self.ensure_oauth_metadata_discovered(server)
|
||||
case _OAuthDiscoveryFailed(timed_out=timed_out):
|
||||
current: Final = self._registered_server(server)
|
||||
if current.is_client_forwarded_token:
|
||||
return current
|
||||
server_ref: Final = current.alias or current.server_name or current.name or current.server_id
|
||||
reason: Final = "timed out" if timed_out else "returned incomplete metadata"
|
||||
raise HTTPException(
|
||||
|
|
@ -5256,7 +5260,7 @@ class MCPServerManager:
|
|||
user_api_key_auth=user_api_key_auth,
|
||||
):
|
||||
extra_headers = _without_authorization(extra_headers)
|
||||
elif mcp_server.is_true_passthrough or mcp_server.is_oauth_delegate:
|
||||
elif mcp_server.is_client_forwarded_token:
|
||||
extra_headers = _client_forwarded_authorization_headers(
|
||||
mcp_server=mcp_server,
|
||||
oauth2_headers=oauth2_headers,
|
||||
|
|
@ -5363,7 +5367,7 @@ class MCPServerManager:
|
|||
# Scoped to the two client-forwarded token modes this stack introduced; legacy
|
||||
# oauth2 + delegate_auth_to_upstream (is_oauth_passthrough) is being removed, so it is not
|
||||
# added here even though the list path still relays for it.
|
||||
relays_upstream_auth: Final = mcp_server.is_true_passthrough or mcp_server.is_oauth_delegate
|
||||
relays_upstream_auth: Final = mcp_server.is_client_forwarded_token
|
||||
server_label: Final = mcp_server.name or mcp_server.server_name or mcp_server.alias or ""
|
||||
|
||||
async def _call_tool_via_client(client, params):
|
||||
|
|
|
|||
|
|
@ -1704,7 +1704,7 @@ if MCP_AVAILABLE:
|
|||
)
|
||||
|
||||
extra_headers: dict[str, str] | None = None
|
||||
is_client_forwarded_mode: Final = server.is_true_passthrough or server.is_oauth_delegate
|
||||
is_client_forwarded_mode: Final = server.is_client_forwarded_token
|
||||
# In a multi-server listing scope the request-wide Authorization can only carry one token,
|
||||
# so it is withheld from a client-forwarded server when another server in scope also consumes
|
||||
# it (RFC 9700 cross-resource replay); such scopes must bind per-server via
|
||||
|
|
|
|||
|
|
@ -224,6 +224,14 @@ class MCPServer(BaseModel):
|
|||
JWT) but forwards the caller's separate upstream ``Authorization`` unchanged, minting nothing."""
|
||||
return self.auth_type == MCPAuth.oauth_delegate
|
||||
|
||||
@property
|
||||
def is_client_forwarded_token(self) -> bool:
|
||||
"""True for the two modes whose upstream credential is the caller's own bearer, forwarded
|
||||
unchanged: the gateway mints nothing for them and holds no OAuth client identity, so a
|
||||
discovered ``authorization_url`` / ``token_url`` enriches only the gateway's own OAuth front
|
||||
door and is never a precondition for opening a session."""
|
||||
return self.is_true_passthrough or self.is_oauth_delegate
|
||||
|
||||
@property
|
||||
def is_dcr_bridge(self) -> bool:
|
||||
"""True when this client-forwarded-token server serves the gateway-hosted DCR front door
|
||||
|
|
@ -231,7 +239,7 @@ class MCPServer(BaseModel):
|
|||
authorize, and token relays) instead of relaying the upstream's own OAuth discovery
|
||||
verbatim. ``dcr_bridge`` is rejected on every other auth type at create, update, and
|
||||
config load, so the mode gate here only defends rows edited outside those paths."""
|
||||
return bool(self.dcr_bridge) and (self.is_true_passthrough or self.is_oauth_delegate)
|
||||
return bool(self.dcr_bridge) and self.is_client_forwarded_token
|
||||
|
||||
@property
|
||||
def requires_per_user_auth(self) -> bool:
|
||||
|
|
@ -248,7 +256,7 @@ class MCPServer(BaseModel):
|
|||
if self.needs_user_oauth_token:
|
||||
return True
|
||||
|
||||
if self.is_true_passthrough or self.is_oauth_delegate:
|
||||
if self.is_client_forwarded_token:
|
||||
return True
|
||||
|
||||
# PAT passthrough: auth_type is none but extra_headers includes auth headers
|
||||
|
|
|
|||
|
|
@ -10525,3 +10525,142 @@ class TestSessionResourceScopeIntersect:
|
|||
):
|
||||
fallback = await manager.get_allowed_mcp_servers(auth)
|
||||
assert fallback == ["granted-id"]
|
||||
|
||||
|
||||
class TestClientForwardedDiscoveryFailureIsNotFatal:
|
||||
"""A failed OAuth metadata discovery may only brick the flows the gateway runs itself.
|
||||
|
||||
``true_passthrough`` / ``oauth_delegate`` forward the caller's own bearer and mint nothing, so an
|
||||
upstream that publishes no RFC 9728 metadata (an internal API, or any IdP unreachable from the
|
||||
pod) must still serve sessions instead of 503-ing before the upstream is ever contacted.
|
||||
"""
|
||||
|
||||
@staticmethod
|
||||
def _config(auth_type: MCPAuthType, dcr_bridge: bool | None) -> dict[str, dict[str, object]]:
|
||||
entry: Final[dict[str, object]] = {
|
||||
"url": "https://up.example.com/mcp",
|
||||
"transport": MCPTransport.http,
|
||||
"auth_type": auth_type,
|
||||
**({"oauth2_flow": "authorization_code"} if auth_type == MCPAuth.oauth2 else {}),
|
||||
**({"dcr_bridge": dcr_bridge} if dcr_bridge is not None else {}),
|
||||
}
|
||||
return {"upstream": entry}
|
||||
|
||||
async def _registered(self, manager: MCPServerManager, auth_type: MCPAuthType, dcr_bridge: bool | None):
|
||||
with (
|
||||
patch.object(manager, "_discover_oauth_metadata_for_server", new=AsyncMock(return_value=None)),
|
||||
patch.object(manager, "initialize_tool_name_to_mcp_server_name_mapping"),
|
||||
):
|
||||
await manager.load_servers_from_config(self._config(auth_type, dcr_bridge))
|
||||
return next(iter(manager.config_mcp_servers.values()))
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"auth_type, dcr_bridge, serves_without_endpoints",
|
||||
[
|
||||
(MCPAuth.true_passthrough, None, True),
|
||||
(MCPAuth.true_passthrough, True, True),
|
||||
(MCPAuth.oauth_delegate, None, True),
|
||||
(MCPAuth.oauth_delegate, True, True),
|
||||
(MCPAuth.oauth2, None, False),
|
||||
(MCPAuth.oauth2_token_exchange, None, False),
|
||||
],
|
||||
)
|
||||
@pytest.mark.parametrize("failure", ["incomplete", "timed_out"])
|
||||
@pytest.mark.asyncio
|
||||
async def test_discovery_failure_blocks_only_gateway_run_flows(
|
||||
self,
|
||||
auth_type: MCPAuthType,
|
||||
dcr_bridge: bool | None,
|
||||
serves_without_endpoints: bool,
|
||||
failure: str,
|
||||
):
|
||||
manager = MCPServerManager()
|
||||
server = await self._registered(manager, auth_type, dcr_bridge)
|
||||
manager._set_oauth_discovery_deferred(server.server_id, True)
|
||||
|
||||
async def never_returns(_server):
|
||||
await asyncio.Future()
|
||||
|
||||
discovery_patch: Final = (
|
||||
{"new": AsyncMock(return_value=None)} if failure == "incomplete" else {"side_effect": never_returns}
|
||||
)
|
||||
with (
|
||||
patch("litellm.proxy._experimental.mcp_server.mcp_server_manager.MCP_METADATA_TIMEOUT", 0.01),
|
||||
patch.object(manager, "_discover_oauth_metadata_for_server", **discovery_patch),
|
||||
):
|
||||
if not serves_without_endpoints:
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
await manager.ensure_oauth_metadata_discovered(server)
|
||||
assert exc.value.status_code == 503
|
||||
return
|
||||
|
||||
resolved = await manager.ensure_oauth_metadata_discovered(server)
|
||||
|
||||
assert resolved is manager.config_mcp_servers[server.server_id]
|
||||
assert resolved.authorization_url is None
|
||||
assert resolved.token_url is None
|
||||
assert manager._oauth_discovery_slot(server.server_id) is not None
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"auth_type, serves_the_listing",
|
||||
[(MCPAuth.true_passthrough, True), (MCPAuth.oauth_delegate, True), (MCPAuth.oauth2, False)],
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
async def test_listing_leg_serves_a_forwarding_server_whose_discovery_failed(
|
||||
self, auth_type: MCPAuthType, serves_the_listing: bool
|
||||
):
|
||||
"""The listing leg is where the 503 became an empty tool list, so pin the fix there too.
|
||||
|
||||
``_get_tools_from_server`` is the per-server leg the aggregate absorbs: a failure here is what
|
||||
the fan-out turns into HTTP 200 with ``tools: []``, which is why the outage carried no
|
||||
diagnostic. A forwarding server must now reach its upstream, and a gateway-run flow must still
|
||||
surface the fault rather than be silently listed as empty.
|
||||
"""
|
||||
manager = MCPServerManager()
|
||||
server = await self._registered(manager, auth_type, None)
|
||||
manager._set_oauth_discovery_deferred(server.server_id, True)
|
||||
manager._fetch_tools_with_timeout = AsyncMock(
|
||||
return_value=[MCPTool(name="list_reports", description="d", inputSchema={"type": "object"})]
|
||||
)
|
||||
|
||||
with patch.object(manager, "_discover_oauth_metadata_for_server", new=AsyncMock(return_value=None)):
|
||||
if not serves_the_listing:
|
||||
with pytest.raises(MCPServerListError):
|
||||
await manager._get_tools_from_server(server=server)
|
||||
return
|
||||
tools = await manager._get_tools_from_server(server=server)
|
||||
|
||||
assert [tool.name for tool in tools] == ["upstream-list_reports"]
|
||||
manager._fetch_tools_with_timeout.assert_awaited_once()
|
||||
|
||||
@pytest.mark.parametrize("auth_type", [MCPAuth.true_passthrough, MCPAuth.oauth_delegate])
|
||||
@pytest.mark.asyncio
|
||||
async def test_client_forwarded_servers_keep_discovering_their_front_door_endpoints(
|
||||
self, auth_type: MCPAuthType
|
||||
):
|
||||
"""Exempting these modes from the FAILURE must not exempt them from discovery itself.
|
||||
|
||||
``/authorize``, ``/token`` and ``/register`` read the discovered endpoints for these servers
|
||||
(``_resolve_ephemeral_dcr_client`` mints for ``true_passthrough`` whatever ``dcr_bridge``
|
||||
says), so an exemption written into the unresolved-endpoints predicate would disarm the slot
|
||||
and silently drop a working front door.
|
||||
"""
|
||||
manager = MCPServerManager()
|
||||
metadata: Final = MCPOAuthMetadata(
|
||||
authorization_url="https://idp.example.com/authorize",
|
||||
token_url="https://idp.example.com/token",
|
||||
registration_url="https://idp.example.com/register",
|
||||
)
|
||||
with (
|
||||
patch.object(manager, "_discover_oauth_metadata_for_server", new=AsyncMock(return_value=metadata)),
|
||||
patch.object(manager, "initialize_tool_name_to_mcp_server_name_mapping"),
|
||||
):
|
||||
await manager.load_servers_from_config(self._config(auth_type, None))
|
||||
server = next(iter(manager.config_mcp_servers.values()))
|
||||
resolved = await manager.ensure_oauth_metadata_discovered(server)
|
||||
|
||||
assert resolved.authorization_url == "https://idp.example.com/authorize"
|
||||
assert resolved.token_url == "https://idp.example.com/token"
|
||||
assert resolved.registration_url == "https://idp.example.com/register"
|
||||
assert manager.config_mcp_servers[server.server_id].authorization_url == "https://idp.example.com/authorize"
|
||||
assert manager._oauth_discovery_slot(server.server_id) is None
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue