diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 95d00554034b..810400347162 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -2122,7 +2122,7 @@ async def _get_tools_from_server( ] return tools else: - tools = await self._fetch_tools_with_timeout(client, server.name, server=server) + tools = await self._fetch_tools_with_timeout(client, server.name) self._remember_upstream_initialize_instructions(server, client) prefixed_or_original_tools = self._create_prefixed_tools(tools, server, add_prefix=add_prefix) @@ -2134,6 +2134,17 @@ async def _get_tools_from_server( # client triggers the upstream OAuth flow. The multi-server # aggregator catches this explicitly to keep absorbing. raise + except HTTPException as e: + headers = e.headers or {} + www_authenticate = headers.get("WWW-Authenticate") or headers.get("www-authenticate") + if e.status_code == 401 and www_authenticate is not None: + raise MCPUpstreamAuthError( + status_code=401, + www_authenticate=www_authenticate, + server_name=server.name, + ) from e + verbose_logger.warning(f"Failed to get tools from server {server.name}: {str(e)}") + return [] except Exception as e: verbose_logger.warning(f"Failed to get tools from server {server.name}: {str(e)}") return [] @@ -2684,7 +2695,6 @@ async def _fetch_tools_with_timeout( self, client: MCPClient, server_name: str, - server: Optional[MCPServer] = None, ) -> List[MCPTool]: """ Fetch tools from MCP client with timeout and error handling. @@ -2692,38 +2702,27 @@ async def _fetch_tools_with_timeout( Uses anyio.fail_after() instead of asyncio.wait_for() to avoid conflicts with the MCP SDK's anyio TaskGroup. See GitHub issue #20715 for details. - For OAuth pass-through and upstream-delegated OAuth2 MCP servers, an - upstream HTTP 401 is converted into :class:`MCPUpstreamAuthError` - instead of being swallowed to an empty tool list. That lets the - single-server HTTP routes surface a proper 401 + ``WWW-Authenticate`` - challenge so standards-compliant MCP clients trigger the upstream - OAuth flow. Other servers keep today's swallow-and-log behaviour so - the multi-server ``/mcp`` aggregator doesn't get tainted by a single - bad server. + An upstream HTTP 401 is converted into :class:`MCPUpstreamAuthError` + instead of being swallowed to an empty tool list, regardless of the + server's auth_type. Callers route it by surface: the single-server HTTP + routes turn it into a 401 + ``WWW-Authenticate`` challenge so standards- + compliant MCP clients trigger the upstream OAuth flow, while the + multi-server ``/mcp`` aggregator absorbs it to an empty list so one + unauthenticated server doesn't fail the whole listing. Only a 401 + (missing/invalid credential) drives the re-auth challenge; a 403 + (authenticated but forbidden, e.g. insufficient scope) is not a re-auth + signal and, like other non-auth errors, returns an empty list. Args: client: MCP client instance server_name: Name of the server for logging - server: Optional MCPServer; when upstream auth is delegated, auth - errors are re-raised as :class:`MCPUpstreamAuthError`. Returns: List of tools from the server """ - should_surface_upstream_auth = bool( - server is not None - and ( - server.is_oauth_passthrough - or ( - server.auth_type == MCPAuth.oauth2 - and getattr(server, "delegate_auth_to_upstream", False) is True - and not server.has_client_credentials - ) - ) - ) try: with anyio.fail_after(MCP_TOOL_LISTING_TIMEOUT): - tools = await client.list_tools(raise_on_error=should_surface_upstream_auth) + tools = await client.list_tools(raise_on_error=True) verbose_logger.debug(f"Tools from {server_name}: {tools}") return tools except TimeoutError: @@ -2736,16 +2735,15 @@ async def _fetch_tools_with_timeout( verbose_logger.warning(f"Connection error while listing tools from {server_name}: {str(e)}") return [] except Exception as e: - if should_surface_upstream_auth: - auth_info = _extract_upstream_auth_failure(e) - if auth_info is not None: - status_code, www_authenticate = auth_info - verbose_logger.info(f"Upstream auth failure from MCP server {server_name}: HTTP {status_code}") - raise MCPUpstreamAuthError( - status_code=status_code, - www_authenticate=www_authenticate, - server_name=server_name, - ) from e + auth_info = _extract_upstream_auth_failure(e) + if auth_info is not None and auth_info[0] == 401: + _, www_authenticate = auth_info + verbose_logger.info(f"Upstream auth failure from MCP server {server_name}: HTTP 401") + raise MCPUpstreamAuthError( + status_code=401, + www_authenticate=www_authenticate, + server_name=server_name, + ) from e verbose_logger.warning(f"Error listing tools from {server_name}: {str(e)}") return [] diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_oauth_passthrough_tools.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_oauth_passthrough_tools.py index b836c3aef331..16c36af5156d 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_oauth_passthrough_tools.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_oauth_passthrough_tools.py @@ -76,9 +76,7 @@ async def test_fetch_tools_from_passthrough_raises_on_upstream_401(): mock_client.list_tools = AsyncMock(side_effect=upstream_error) with pytest.raises(MCPUpstreamAuthError) as exc_info: - await manager._fetch_tools_with_timeout( - mock_client, passthrough_server.name, server=passthrough_server - ) + await manager._fetch_tools_with_timeout(mock_client, passthrough_server.name) assert exc_info.value.status_code == 401 assert exc_info.value.www_authenticate == ( @@ -113,9 +111,7 @@ async def test_fetch_tools_from_delegated_oauth2_raises_on_upstream_401(): mock_client.list_tools = AsyncMock(side_effect=upstream_error) with pytest.raises(MCPUpstreamAuthError) as exc_info: - await manager._fetch_tools_with_timeout( - mock_client, delegated_server.name, server=delegated_server - ) + await manager._fetch_tools_with_timeout(mock_client, delegated_server.name) assert exc_info.value.status_code == 401 assert exc_info.value.www_authenticate == ( @@ -126,7 +122,10 @@ async def test_fetch_tools_from_delegated_oauth2_raises_on_upstream_401(): @pytest.mark.asyncio -async def test_fetch_tools_from_client_credentials_oauth2_keeps_swallow_behavior(): +async def test_fetch_tools_from_client_credentials_oauth2_surfaces_upstream_401(): + """The auth_type carve-out was removed: a client_credentials (M2M) server now + surfaces an upstream 401 as MCPUpstreamAuthError too, instead of swallowing it + to an empty list, so single-server routes can return a 401 challenge.""" manager = MCPServerManager() m2m_server = MCPServer( server_id="oauth-m2m", @@ -150,12 +149,12 @@ async def test_fetch_tools_from_client_credentials_oauth2_keeps_swallow_behavior mock_client = MagicMock() mock_client.list_tools = AsyncMock(side_effect=upstream_error) - tools = await manager._fetch_tools_with_timeout( - mock_client, m2m_server.name, server=m2m_server - ) + with pytest.raises(MCPUpstreamAuthError) as exc_info: + await manager._fetch_tools_with_timeout(mock_client, m2m_server.name) - assert tools == [] - mock_client.list_tools.assert_awaited_with(raise_on_error=False) + assert exc_info.value.status_code == 401 + assert exc_info.value.server_name == "m2m_docs" + mock_client.list_tools.assert_awaited_with(raise_on_error=True) @pytest.mark.asyncio @@ -176,9 +175,7 @@ async def test_fetch_tools_from_passthrough_returns_tools_on_success(): mock_client = MagicMock() mock_client.list_tools = AsyncMock(return_value=[tool]) - tools = await manager._fetch_tools_with_timeout( - mock_client, passthrough_server.name, server=passthrough_server - ) + tools = await manager._fetch_tools_with_timeout(mock_client, passthrough_server.name) assert tools == [tool] @@ -238,8 +235,11 @@ def test_to_http_exception_skips_challenge_for_non_401_status(): @pytest.mark.asyncio -async def test_fetch_tools_from_gateway_managed_swallows_errors(): - """Regression guard: non-pass-through servers keep returning [] on errors.""" +async def test_fetch_tools_from_gateway_managed_surfaces_upstream_401(): + """An oauth2 server that is neither pass-through nor delegate now surfaces an + upstream 401 as MCPUpstreamAuthError as well; the auth_type carve-out that + swallowed it to [] was removed. A missing upstream WWW-Authenticate is carried + through as None (the single-server route fabricates one from the gateway URL).""" manager = MCPServerManager() oauth2_server = MCPServer( server_id="o1", @@ -260,11 +260,13 @@ async def test_fetch_tools_from_gateway_managed_swallows_errors(): mock_client = MagicMock() mock_client.list_tools = AsyncMock(side_effect=upstream_error) - tools = await manager._fetch_tools_with_timeout( - mock_client, oauth2_server.name, server=oauth2_server - ) - assert tools == [] - mock_client.list_tools.assert_awaited_with(raise_on_error=False) + with pytest.raises(MCPUpstreamAuthError) as exc_info: + await manager._fetch_tools_with_timeout(mock_client, oauth2_server.name) + + assert exc_info.value.status_code == 401 + assert exc_info.value.www_authenticate is None + assert exc_info.value.server_name == "keycloak_whoami" + mock_client.list_tools.assert_awaited_with(raise_on_error=True) def _http_server(server_id: str, name: str, **kwargs) -> MCPServer: diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py index c258ed1035fa..b7a7917eac1c 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py @@ -5228,5 +5228,139 @@ async def test_none_with_extra_header_stays_v2_without_clobbering(self): assert client._get_auth_headers()["Authorization"] == "Bearer hook-jwt" +def _upstream_status_error(status_code: int, challenge: str) -> httpx.HTTPStatusError: + request = httpx.Request("POST", "https://upstream.example/mcp") + response = httpx.Response( + status_code, + headers={"WWW-Authenticate": challenge}, + request=request, + ) + return httpx.HTTPStatusError( + "upstream rejected token", request=request, response=response + ) + + +class TestMCPToolsListAuthSurfacing: + """Regression: MCP tools/list 401 auth failures must surface as MCPUpstreamAuthError. + + Previously a missing/expired per-user OAuth token, or an upstream 401 for any + non-carveout auth_type, was swallowed to an empty tool list, so a single-server + client saw a 200 with no tools instead of a 401 challenge. The listing helpers + now raise MCPUpstreamAuthError on a 401 regardless of auth_type; the single-server + routes turn it into a 401 + WWW-Authenticate while the aggregator absorbs it to an + empty list. Only a 401 challenges; a 403 (forbidden) degrades like any other error. + """ + + @pytest.mark.asyncio + async def test_fetch_tools_with_timeout_surfaces_upstream_401(self): + from litellm.proxy._experimental.mcp_server.exceptions import ( + MCPUpstreamAuthError, + ) + + manager = MCPServerManager() + challenge = 'Bearer resource_metadata="https://upstream.example/.well-known/oauth-protected-resource"' + client = MagicMock() + client.list_tools = AsyncMock(side_effect=_upstream_status_error(401, challenge)) + + with pytest.raises(MCPUpstreamAuthError) as exc_info: + await manager._fetch_tools_with_timeout(client, "static-key-server") + + assert exc_info.value.status_code == 401 + assert exc_info.value.www_authenticate == challenge + assert exc_info.value.server_name == "static-key-server" + + @pytest.mark.asyncio + async def test_fetch_tools_with_timeout_absorbs_upstream_403(self): + """Only a 401 drives the re-auth challenge. A 403 (authenticated but + forbidden, e.g. insufficient scope) is not a re-auth signal, so even + with a WWW-Authenticate header it degrades to an empty list rather than + surfacing a challenge.""" + manager = MCPServerManager() + challenge = 'Bearer error="insufficient_scope", scope="read:tools"' + client = MagicMock() + client.list_tools = AsyncMock(side_effect=_upstream_status_error(403, challenge)) + + assert await manager._fetch_tools_with_timeout(client, "forbidden-server") == [] + + @pytest.mark.asyncio + async def test_fetch_tools_with_timeout_returns_empty_on_non_auth_error(self): + manager = MCPServerManager() + client = MagicMock() + client.list_tools = AsyncMock(side_effect=RuntimeError("upstream 500")) + + assert await manager._fetch_tools_with_timeout(client, "srv") == [] + + @pytest.mark.asyncio + async def test_get_tools_from_server_surfaces_unusable_user_token(self): + from litellm.proxy._experimental.mcp_server.exceptions import ( + MCPUpstreamAuthError, + ) + + manager = MCPServerManager() + server = MCPServer( + server_id="oauth-srv", name="oauth-srv", transport=MCPTransport.http + ) + challenge = 'Bearer resource_metadata="/.well-known/oauth-protected-resource/mcp/oauth-srv"' + manager._create_mcp_client = AsyncMock( + side_effect=HTTPException( + status_code=401, + detail="Unauthorized", + headers={"WWW-Authenticate": challenge}, + ) + ) + + with pytest.raises(MCPUpstreamAuthError) as exc_info: + await manager._get_tools_from_server(server) + + assert exc_info.value.status_code == 401 + assert exc_info.value.www_authenticate == challenge + assert exc_info.value.server_name == "oauth-srv" + + @pytest.mark.asyncio + async def test_get_tools_from_server_absorbs_non_challenge_http_error(self): + manager = MCPServerManager() + server = MCPServer( + server_id="stdio-srv", name="stdio-srv", transport=MCPTransport.http + ) + manager._create_mcp_client = AsyncMock( + side_effect=HTTPException( + status_code=403, + detail="MCP stdio command 'foo' is not in the allowlist", + ) + ) + + assert await manager._get_tools_from_server(server) == [] + + @pytest.mark.asyncio + async def test_aggregate_list_tools_absorbs_unauthenticated_server(self): + from litellm.proxy._experimental.mcp_server.exceptions import ( + MCPUpstreamAuthError, + ) + + manager = MCPServerManager() + good = MCPServer(server_id="good", name="good", transport=MCPTransport.http) + bad = MCPServer(server_id="bad", name="bad", transport=MCPTransport.http) + manager.get_allowed_mcp_servers = AsyncMock(return_value=["good", "bad"]) + manager.get_mcp_server_by_id = MagicMock( + side_effect=lambda server_id: {"good": good, "bad": bad}.get(server_id) + ) + good_tool = MCPTool(name="good-do_thing", description="do thing", inputSchema={}) + + async def fake_get_tools(server, **kwargs): + if server.server_id == "bad": + raise MCPUpstreamAuthError( + status_code=401, + www_authenticate='Bearer realm="x"', + server_name="bad", + ) + return [good_tool] + + manager._get_tools_from_server = fake_get_tools + + result = await manager.list_tools() + + assert [t.name for t in result] == ["good-do_thing"] + + if __name__ == "__main__": pytest.main([__file__]) diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py index 46a51f8fe617..fb4eee54e1d1 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py @@ -843,6 +843,76 @@ async def fake_get_tools(*args, **kwargs): assert exc_info.value.status_code == upstream_status assert exc_info.value.headers == {"www-authenticate": challenge} + async def test_aggregate_list_absorbs_one_server_auth_failure(self, monkeypatch): + """The multi-server aggregate listing degrades a server whose upstream + rejects auth to an empty contribution and still returns the healthy + server's tools with a 200, rather than surfacing a 401.""" + from litellm.proxy._experimental.mcp_server.exceptions import ( + MCPUpstreamAuthError, + ) + + class StubServer: + def __init__(self, name): + self.alias = name + self.server_name = name + self.name = name + self.allowed_tools = None + self.mcp_info = {"server_name": name} + self.available_on_public_internet = True + + good = StubServer("good") + bad = StubServer("bad") + + async def fake_contexts(user_api_key_auth): + return [user_api_key_auth] + + async def fake_get_allowed_mcp_servers(*args, **kwargs): + return ["good", "bad"] + + async def fake_get_tools(server, *args, **kwargs): + if server.server_name == "bad": + raise MCPUpstreamAuthError( + status_code=401, + www_authenticate='Bearer realm="x"', + server_name="bad", + ) + return ["good-tool"] + + monkeypatch.setattr( + rest_endpoints, + "build_effective_auth_contexts", + fake_contexts, + raising=False, + ) + monkeypatch.setattr( + rest_endpoints.global_mcp_server_manager, + "get_allowed_mcp_servers", + fake_get_allowed_mcp_servers, + raising=False, + ) + monkeypatch.setattr( + rest_endpoints.global_mcp_server_manager, + "get_mcp_server_by_id", + lambda server_id: {"good": good, "bad": bad}.get(server_id), + raising=False, + ) + monkeypatch.setattr( + rest_endpoints, + "_get_tools_for_single_server", + fake_get_tools, + raising=False, + ) + + request = _build_request(path="/mcp-rest/tools/list", method="GET") + result = await rest_endpoints.list_tool_rest_api( + request, + server_id=None, + user_api_key_dict=UserAPIKeyAuth(), + ) + + assert result["tools"] == ["good-tool"] + assert result["error"] is None + async def test_name_resolution_finds_server_by_uuid(self, monkeypatch): """When server_id is a name string, it should be resolved to its UUID and used for the tools lookup when the UUID is in allowed_server_ids.""" diff --git a/ui/litellm-dashboard/src/components/mcp_tools/mcp_tools.test.tsx b/ui/litellm-dashboard/src/components/mcp_tools/mcp_tools.test.tsx index 72ed39842445..3346bc342f33 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/mcp_tools.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcp_tools.test.tsx @@ -131,6 +131,25 @@ describe("MCPToolsViewer auth gate routing", () => { expect(screen.queryByText(GATE_TEXT)).not.toBeInTheDocument(); }); + it("gates an OBO server whose stored token is expired and the list call 401s (refresh could not mint a token)", async () => { + // has_credential=true but the list call 401s: the server-side refresh could not + // produce a valid token (e.g. expired with no usable refresh token), so the user + // must reauthorize instead of seeing a dead empty list. + vi.mocked(getMCPOAuthUserCredentialStatus).mockResolvedValue( + credStatus({ has_credential: true, is_expired: true }), + ); + vi.mocked(listMCPTools).mockResolvedValue({ + tools: [], + error: "unauthorized", + status: 401, + } as unknown as Awaited>); + + renderViewer({ oauth2_flow: null, delegate_auth_to_upstream: false }); + + expect(await screen.findByText(GATE_TEXT)).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Authorize" })).toBeInTheDocument(); + }); + it("does not gate an M2M server; lists with the LiteLLM key", async () => { renderViewer({ oauth2_flow: "client_credentials", delegate_auth_to_upstream: false }); diff --git a/ui/litellm-dashboard/src/components/mcp_tools/mcp_tools.tsx b/ui/litellm-dashboard/src/components/mcp_tools/mcp_tools.tsx index 0a32bd11a450..54e40fd7ed4c 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/mcp_tools.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcp_tools.tsx @@ -238,9 +238,14 @@ const MCPToolsViewer = ({ const toolsData = mcpToolsResponse?.tools || []; + const oboToolsError = mcpToolsError as (Error & { status?: number; response?: { status?: number } }) | null; + const oboTokenRejected = isObo && (oboToolsError?.status ?? oboToolsError?.response?.status) === 401; + // An auth gate replaces the tool list when the user must authenticate first: - // passthrough needs a browser token, OBO needs a stored DB credential. - const authGateActive = (isPassthrough && !oauthToken) || oboNeedsAuth; + // passthrough needs a browser token; OBO needs a stored DB credential or a + // still-valid one — a 401 from the list call means the backend has none even + // after attempting a refresh, so re-authorization is required. + const authGateActive = (isPassthrough && !oauthToken) || oboNeedsAuth || oboTokenRejected; // Treat OBO credential-status loading as "tools loading" so the empty state // doesn't flash before we know whether the user needs to authorize. const toolsAreaLoading = isLoadingTools || oboStatusLoading; @@ -364,10 +369,12 @@ const MCPToolsViewer = ({ )} - {/* OBO auth gate — only when no credential row exists for this user. - An existing-but-expired token is refreshed server-side on the - list call, so the gate never appears for a stored credential. */} - {oboNeedsAuth && ( + {/* OBO auth gate — shown when there is no credential row for this + user, or when the list call returns 401 (no valid token and the + server-side refresh could not mint one, e.g. an expired token + with no usable refresh token). A refreshable token is refreshed + on the list call and never trips this gate. */} + {(oboNeedsAuth || oboTokenRejected) && (

Authentication required