Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 32 additions & 34 deletions litellm/proxy/_experimental/mcp_server/mcp_server_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 []
Comment thread
tin-berri marked this conversation as resolved.
Comment thread
tin-berri marked this conversation as resolved.
except Exception as e:
verbose_logger.warning(f"Failed to get tools from server {server.name}: {str(e)}")
return []
Expand Down Expand Up @@ -2684,46 +2695,34 @@ 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.

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:
Expand All @@ -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 []

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 == (
Expand Down Expand Up @@ -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 == (
Expand All @@ -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",
Expand All @@ -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
Expand All @@ -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]


Expand Down Expand Up @@ -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",
Expand All @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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__])
Loading
Loading