diff --git a/litellm/proxy/_experimental/mcp_server/exceptions.py b/litellm/proxy/_experimental/mcp_server/exceptions.py index 8c704c0fe93..a1b3b167a4a 100644 --- a/litellm/proxy/_experimental/mcp_server/exceptions.py +++ b/litellm/proxy/_experimental/mcp_server/exceptions.py @@ -75,6 +75,24 @@ class MCPUpstreamAuthError(Exception): ) +class MCPOpenApiUpstreamError(Exception): + """An OpenAPI-backed MCP tool's upstream answered with a non-2xx that is not a 401. + + Carries the status only. The upstream's response body is deliberately dropped rather than served + as tool content: it crosses a trust boundary and may hold prose, urls, or an error document that + reads as data, which is how these failures came to be reported as successful tool output. This + matches ``outcome_wire_value``'s contract for listing faults, category and status and nothing + else. A 401 is raised as ``MCPUpstreamAuthError`` instead, so the caller learns to + re-authenticate; every other status stays here, mirroring the regular MCP path where a 403 + deliberately does not produce a challenge. + """ + + def __init__(self, status_code: int, server_name: str) -> None: + self.status_code = status_code + self.server_name = server_name + super().__init__(f"upstream returned HTTP {status_code}") + + class MCPToolResultError(Exception): """An MCP tool call completed with ``isError=True`` in its result. diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 65855df89f6..26a6f8d1251 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -2330,7 +2330,15 @@ class MCPServerManager: input_schema = build_input_schema(resolved_operation) # Create tool function with headers using imported function - tool_func = create_tool_function(path, method, resolved_operation, base_url, headers=headers) + tool_func = create_tool_function( + path, + method, + resolved_operation, + base_url, + headers=headers, + server_label=server.name or server.server_name or server.alias or server.server_id, + relays_upstream_auth=server.is_client_forwarded_token, + ) tool_func.__name__ = prefixed_tool_name tool_func.__doc__ = description @@ -4979,6 +4987,12 @@ class MCPServerManager: return result + except MCPUpstreamAuthError: + # The caller must re-authenticate upstream, so this keeps its type all the way to the + # renderers: the streamable path turns it into an isError result naming the status, and + # the REST path relays a real 401 with the upstream's WWW-Authenticate. Flattening it + # into the generic message below would lose both. + raise except Exception as e: error_msg = f"Error calling OpenAPI tool {tool_name}: {e}" verbose_logger.error(error_msg) diff --git a/litellm/proxy/_experimental/mcp_server/openapi_to_mcp_generator.py b/litellm/proxy/_experimental/mcp_server/openapi_to_mcp_generator.py index eb78aaeca0b..083a98cdd36 100644 --- a/litellm/proxy/_experimental/mcp_server/openapi_to_mcp_generator.py +++ b/litellm/proxy/_experimental/mcp_server/openapi_to_mcp_generator.py @@ -15,6 +15,12 @@ from urllib.parse import quote import httpx from typing_extensions import ReadOnly, Required +from litellm.llms.custom_httpx.http_handler import MaskedHTTPStatusError +from litellm.proxy._experimental.mcp_server.exceptions import ( + MCPOpenApiUpstreamError, + MCPUpstreamAuthError, +) + # Tool names emitted from OpenAPI specs must work across all major LLM providers. # OpenAI/Anthropic/Bedrock all enforce a character class roughly equivalent to # ^[a-zA-Z0-9_-]+$ on tool names. Many specs (notably GitHub's REST API) use @@ -392,12 +398,40 @@ def _merge_openapi_tool_request_headers( return effective_headers +def _raise_for_upstream_failure( + response: httpx.Response, + upstream: str, + relays_upstream_auth: bool, +) -> None: + """Turn a non-2xx upstream response into the right typed failure, or return for a 2xx. + + Both call sites feed this: ``get`` hands back the response for a 4xx, while post/put/patch/delete + raise ``MaskedHTTPStatusError`` from inside the HTTP handler, so without one classifier the + non-GET tools would keep serving an error body as tool output. + + Only the client-forwarded modes carry the caller's own upstream token, so only they can act on a + 401 by re-authenticating; ``_call_regular_mcp_tool`` gates its re-auth signal the same way. Every + other status carries the code alone, never the upstream's body, which crosses a trust boundary. + """ + if response.status_code < 400: + return + if response.status_code == 401 and relays_upstream_auth: + raise MCPUpstreamAuthError( + status_code=response.status_code, + www_authenticate=response.headers.get("www-authenticate"), + server_name=upstream, + ) + raise MCPOpenApiUpstreamError(response.status_code, upstream) + + def create_tool_function( path: str, method: str, operation: _OpenAPIOperation, base_url: str, headers: dict[str, str] | None = None, + server_label: str | None = None, + relays_upstream_auth: bool = False, ): """Create a tool function for an OpenAPI operation. @@ -477,20 +511,26 @@ def create_tool_function( json_body = {"data": body_value} client: Final = get_async_httpx_client(llm_provider=httpxSpecialProvider.MCP) + upstream: Final = server_label or f"{original_method.upper()} {path}" - if original_method == "get": - response = await client.get(url, params=params, headers=effective_headers) - elif original_method == "post": - response = await client.post(url, params=params, json=json_body, headers=effective_headers) - elif original_method == "put": - response = await client.put(url, params=params, json=json_body, headers=effective_headers) - elif original_method == "delete": - response = await client.delete(url, params=params, headers=effective_headers) - elif original_method == "patch": - response = await client.patch(url, params=params, json=json_body, headers=effective_headers) - else: - return f"Unsupported HTTP method: {original_method}" + try: + if original_method == "get": + response = await client.get(url, params=params, headers=effective_headers) + elif original_method == "post": + response = await client.post(url, params=params, json=json_body, headers=effective_headers) + elif original_method == "put": + response = await client.put(url, params=params, json=json_body, headers=effective_headers) + elif original_method == "delete": + response = await client.delete(url, params=params, headers=effective_headers) + elif original_method == "patch": + response = await client.patch(url, params=params, json=json_body, headers=effective_headers) + else: + return f"Unsupported HTTP method: {original_method}" + except MaskedHTTPStatusError as e: + _raise_for_upstream_failure(e.response, upstream, relays_upstream_auth) + raise + _raise_for_upstream_failure(response, upstream, relays_upstream_auth) return response.text return tool_function diff --git a/litellm/proxy/_experimental/mcp_server/server.py b/litellm/proxy/_experimental/mcp_server/server.py index 8d69d84e492..0dc85c0318c 100644 --- a/litellm/proxy/_experimental/mcp_server/server.py +++ b/litellm/proxy/_experimental/mcp_server/server.py @@ -407,8 +407,6 @@ if MCP_AVAILABLE: StreamableHTTPSessionManager = None from mcp.types import ( CallToolResult, - EmbeddedResource, - ImageContent, ListToolsResult, Prompt, TextContent, @@ -2861,12 +2859,11 @@ if MCP_AVAILABLE: _extra_token: Final = _request_extra_headers.set(forwarded_headers) _resolved_token: Final = _request_resolved_auth_headers.set(resolved_auth_headers) try: - local_content = await _handle_local_mcp_tool(name, arguments) + response = await _handle_local_mcp_tool(name, arguments) finally: _request_auth_header.reset(_auth_token) _request_extra_headers.reset(_extra_token) _request_resolved_auth_headers.reset(_resolved_token) - response = CallToolResult(content=local_content, isError=False) # Try managed MCP server tool (the name is bare; the prefix boundary was # already resolved above against this server's registered prefixes) @@ -2940,8 +2937,7 @@ if MCP_AVAILABLE: if "arguments" in hook_result: arguments = hook_result["arguments"] # pyright: ignore[reportAny] # hook returns untyped args - local_content = await _handle_local_mcp_tool(original_tool_name, arguments) - response = CallToolResult(content=local_content, isError=False) + response = await _handle_local_mcp_tool(original_tool_name, arguments) return await _run_post_mcp_call_guardrails( result=response, @@ -3319,11 +3315,18 @@ if MCP_AVAILABLE: verbose_logger.debug("CALL TOOL RESULT: %s", call_tool_result) return call_tool_result - async def _handle_local_mcp_tool( - name: str, arguments: dict[str, object] - ) -> list[TextContent | ImageContent | EmbeddedResource]: - """ - Handle tool execution for local registry tools + async def _handle_local_mcp_tool(name: str, arguments: dict[str, object]) -> CallToolResult: + """Execute a local-registry tool and report whether it succeeded. + + Returns the result rather than bare content because the verdict is part of it: the content + alone cannot say whether the handler failed, so callers used to stamp isError=False on every + outcome and an upstream rejection was served as tool output. + + A failure is reported as ``isError=True`` here rather than raised, because the REST surface + turns an unrecognized exception into a 500 and an upstream 403 or 429 is not a gateway crash. + ``MCPUpstreamAuthError`` is the exception: it propagates so the caller is told to + re-authenticate, which both renderers already know how to say. + Note: Local tools don't use prefixes, so we use the original name """ import inspect @@ -3333,15 +3336,16 @@ if MCP_AVAILABLE: raise HTTPException(status_code=404, detail=f"Tool '{name}' not found") try: - # Check if handler is async or sync if inspect.iscoroutinefunction(tool.handler): result = await tool.handler(**arguments) else: result = tool.handler(**arguments) - return [TextContent(text=str(result), type="text")] + except MCPUpstreamAuthError: + raise except Exception as e: verbose_logger.exception("Error executing local tool %s: %s", name, e) - return [TextContent(text=f"Error: {e}", type="text")] + return CallToolResult(content=[TextContent(text=f"Error: {e}", type="text")], isError=True) + return CallToolResult(content=[TextContent(text=str(result), type="text")], isError=False) def _get_mcp_servers_in_path(path: str) -> list[str] | None: """ 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 9064312fd6d..5ee8143fb8e 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 @@ -4395,8 +4395,12 @@ class TestMCPServerManager: captured: dict = {} - def fake_create_tool_function(path, method, operation, base_url, headers=None): + def fake_create_tool_function( + path, method, operation, base_url, headers=None, server_label=None, relays_upstream_auth=False + ): captured["headers"] = headers + captured["server_label"] = server_label + captured["relays_upstream_auth"] = relays_upstream_auth async def tool_func(**kwargs): return "ok" @@ -4425,6 +4429,11 @@ class TestMCPServerManager: assert captured["headers"] is not None assert captured["headers"]["Authorization"] == "STATIC token" + # The label names the server in an upstream-failure error, so registration must thread it; + # without this the fake would simply tolerate the argument and prove nothing about it. + assert captured["server_label"] == "openapi-server" + # auth_type is none here, so a 401 from this upstream must not be dressed up as a re-auth signal + assert captured["relays_upstream_auth"] is False @pytest.mark.asyncio async def test_pre_call_tool_check_allowed_tools_list_allows_tool(self): @@ -10765,3 +10774,61 @@ class TestResolveOpenapiToolAuth: ) assert "Authorization" not in (forwarded or {}) + + +class TestOpenApiHandlerRelaysUpstreamAuth: + """`_call_openapi_tool_handler` must not flatten a re-auth signal into a generic message. + + Its catch-all turned every exception into "Error calling OpenAPI tool ...", which is an isError + result but loses the status, so the REST surface could no longer relay a 401 with the upstream's + WWW-Authenticate and the streamable surface could not name the status the caller must act on. + """ + + @staticmethod + def _server() -> MCPServer: + return MCPServer( + server_id="srv-openapi", + name="report_api", + server_name="report_api", + url="https://api.example.com", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth_delegate, + spec_path="https://api.example.com/openapi.json", + ) + + @pytest.mark.asyncio + async def test_upstream_auth_error_keeps_its_type(self): + from litellm.proxy._experimental.mcp_server.exceptions import MCPUpstreamAuthError + from litellm.proxy._experimental.mcp_server.tool_registry import global_mcp_tool_registry + + manager = MCPServerManager() + server = self._server() + + async def raising_handler(**_kwargs): + raise MCPUpstreamAuthError(status_code=401, www_authenticate="Bearer realm=x", server_name="report_api") + + tool = MagicMock() + tool.handler = raising_handler + with patch.object(global_mcp_tool_registry, "get_tool", return_value=tool): + with pytest.raises(MCPUpstreamAuthError) as exc: + await manager._call_openapi_tool_handler(server, "list_reports", {}) + + assert exc.value.status_code == 401 + assert exc.value.www_authenticate == "Bearer realm=x" + + @pytest.mark.asyncio + async def test_other_failures_still_become_an_error_result(self): + from litellm.proxy._experimental.mcp_server.tool_registry import global_mcp_tool_registry + + manager = MCPServerManager() + + async def raising_handler(**_kwargs): + raise RuntimeError("upstream returned HTTP 503") + + tool = MagicMock() + tool.handler = raising_handler + with patch.object(global_mcp_tool_registry, "get_tool", return_value=tool): + result = await manager._call_openapi_tool_handler(self._server(), "list_reports", {}) + + assert result.isError is True + assert "upstream returned HTTP 503" in result.content[0].text diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_to_mcp_generator.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_to_mcp_generator.py index 1f9316ee9c8..e59616e53c1 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_to_mcp_generator.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_to_mcp_generator.py @@ -27,12 +27,21 @@ from litellm.proxy._experimental.mcp_server.openapi_to_mcp_generator import ( resolve_operation_params, ) +from litellm.proxy._experimental.mcp_server.exceptions import ( + MCPOpenApiUpstreamError, + MCPUpstreamAuthError, +) + GET_ASYNC_CLIENT_TARGET = "litellm.proxy._experimental.mcp_server.openapi_to_mcp_generator.get_async_httpx_client" -def _create_mock_client(method: str, response_text: str) -> AsyncMock: - """Utility to create a mocked async httpx client for the given method.""" - response = SimpleNamespace(text=response_text) +def _create_mock_client(method: str, response_text: str, status_code: int = 200) -> AsyncMock: + """Utility to create a mocked async httpx client for the given method. + + ``status_code`` and ``headers`` are part of the real response the tool function reads, so the + fake carries them too; a fake that omits them cannot observe whether the status is checked. + """ + response = SimpleNamespace(text=response_text, status_code=status_code, headers={}) client = AsyncMock() setattr(client, method, AsyncMock(return_value=response)) return client @@ -1259,3 +1268,113 @@ class TestRequestExtraHeaders: headers_sent = async_client.get.call_args[1]["headers"] assert "Authorization" not in headers_sent + + +class TestUpstreamStatusIsClassified: + """A non-2xx upstream must never be returned as tool output. + + The body used to be returned verbatim whatever the status, so an upstream rejection arrived as a + successful tool result and the request logged as a success. 401 is singled out because it is the + only status the caller can act on by re-authenticating, matching `_call_regular_mcp_tool` where a + 403 deliberately does not produce a challenge. + """ + + @staticmethod + def _tool(status_code: int, text: str = "body", headers: dict | None = None, relays_upstream_auth: bool = True): + response = SimpleNamespace(text=text, status_code=status_code, headers=headers or {}) + client = AsyncMock() + client.get = AsyncMock(return_value=response) + return create_tool_function( + "/reports", + "get", + {"operationId": "list_reports"}, + "https://api.example.com", + server_label="report_api", + relays_upstream_auth=relays_upstream_auth, + ), client + + @pytest.mark.asyncio + async def test_success_still_returns_the_body(self): + tool, client = self._tool(200, text='{"reports": []}') + with patch(GET_ASYNC_CLIENT_TARGET, return_value=client): + assert await tool() == '{"reports": []}' + + @pytest.mark.asyncio + async def test_401_raises_the_reauth_signal_carrying_the_challenge(self): + tool, client = self._tool(401, text='{"error":"invalid_token"}', headers={"www-authenticate": 'Bearer realm="x"'}) + with patch(GET_ASYNC_CLIENT_TARGET, return_value=client): + with pytest.raises(MCPUpstreamAuthError) as exc: + await tool() + + assert exc.value.status_code == 401 + assert exc.value.www_authenticate == 'Bearer realm="x"' + assert exc.value.server_name == "report_api" + + @pytest.mark.asyncio + async def test_401_on_a_non_forwarding_server_is_not_a_reauth_signal(self): + """Only the client-forwarded modes carry the caller's own upstream token, so only they can act + on a 401. `_call_regular_mcp_tool` gates its signal the same way, and without the gate an + api_key server rejecting a token would push clients into an OAuth flow that does not apply.""" + tool, client = self._tool(401, text='{"error":"bad key"}', relays_upstream_auth=False) + with patch(GET_ASYNC_CLIENT_TARGET, return_value=client): + with pytest.raises(MCPOpenApiUpstreamError) as exc: + await tool() + + assert exc.value.status_code == 401 + + @pytest.mark.parametrize("method", ["post", "put", "patch", "delete"]) + @pytest.mark.parametrize("status_code, expected", [(401, "auth"), (500, "other")]) + @pytest.mark.asyncio + async def test_non_get_methods_are_classified_too(self, method: str, status_code: int, expected: str): + """post/put/patch/delete call raise_for_status inside the HTTP handler. + + Only `get` hands a 4xx back to the caller; the others raise `MaskedHTTPStatusError` before any + status check the tool function could do, so classifying the returned response alone would + leave every non-GET tool still serving an upstream error body as successful tool output. A + fake client that simply returns a response cannot observe this, which is why this test builds + the error the real handler raises. + """ + import httpx + + from litellm.llms.custom_httpx.http_handler import MaskedHTTPStatusError + + request = httpx.Request(method.upper(), "https://api.example.com/reports") + raw = httpx.Response( + status_code, + headers={"www-authenticate": 'Bearer realm="x"'}, + text="internal hostname db-prod-7.corp.example.com", + request=request, + ) + masked = MaskedHTTPStatusError(httpx.HTTPStatusError("boom", request=request, response=raw)) + + client = AsyncMock() + setattr(client, method, AsyncMock(side_effect=masked)) + tool = create_tool_function( + "/reports", + method, + {"operationId": "list_reports"}, + "https://api.example.com", + server_label="report_api", + relays_upstream_auth=True, + ) + + expected_type = MCPUpstreamAuthError if expected == "auth" else MCPOpenApiUpstreamError + with patch(GET_ASYNC_CLIENT_TARGET, return_value=client): + with pytest.raises(expected_type) as exc: + await tool() + + assert exc.value.status_code == status_code + assert "db-prod-7" not in str(exc.value) + + @pytest.mark.parametrize("status_code", [403, 404, 429, 500, 503]) + @pytest.mark.asyncio + async def test_other_failures_raise_without_leaking_the_upstream_body(self, status_code: int): + secret_body = "internal hostname db-prod-7.corp.example.com and a stack trace" + tool, client = self._tool(status_code, text=secret_body) + with patch(GET_ASYNC_CLIENT_TARGET, return_value=client): + with pytest.raises(MCPOpenApiUpstreamError) as exc: + await tool() + + assert exc.value.status_code == status_code + assert secret_body not in str(exc.value) + assert str(exc.value) == f"upstream returned HTTP {status_code}" diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_tool_auth.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_tool_auth.py index 7bd846aeda4..bd953dc55f3 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_tool_auth.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_tool_auth.py @@ -652,3 +652,80 @@ async def test_per_server_auth_header_reaches_both_openapi_dispatch_arms(dispatc assert captured["resolver_credential"] == {"Authorization": OPENAPI_PER_SERVER_TOKEN} assert captured["injected"] == OPENAPI_PER_SERVER_TOKEN assert _request_auth_header.get() is None + + +@pytest.mark.parametrize("failure", ["auth", "other"]) +@pytest.mark.asyncio +async def test_local_dispatch_reports_the_outcome_instead_of_success(failure: str): + """A failing local handler must never be reported as a successful tool result, and only an auth + failure may propagate. + + `_handle_local_mcp_tool` used to catch every exception and return it as TextContent, and both of + its callers then stamped `isError=False`, so an upstream rejection was served as tool output and + `extract_mcp_tool_result_error_message` logged the request as a success. + + The two kinds are split by consequence. `MCPUpstreamAuthError` propagates because both renderers + know it: the streamable path names the status and the REST path relays a real 401 with the + upstream's WWW-Authenticate. Anything else is reported as `isError=True` right here, because + `call_tool_rest_api` turns an unrecognized exception into HTTP 500 and an upstream 403 or 429 is + not a gateway crash. + """ + from litellm.proxy._experimental.mcp_server import server as mcp_module + from litellm.proxy._experimental.mcp_server.exceptions import ( + MCPOpenApiUpstreamError, + MCPUpstreamAuthError, + ) + + error = ( + MCPUpstreamAuthError(status_code=401, www_authenticate=None, server_name="report_api") + if failure == "auth" + else MCPOpenApiUpstreamError(429, "report_api") + ) + + async def raising_handler(**_kwargs): + raise error + + fake_tool = MagicMock() + fake_tool.name = "list_reports" + fake_tool.handler = raising_handler + server = MCPServer( + server_id="srv-openapi", + name="report_api", + server_name="report_api", + url="https://api.example.com", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth_delegate, + spec_path="https://api.example.com/openapi.json", + ) + user = UserAPIKeyAuth(api_key="sk-user", user_id="alice", user_role=LitellmUserRoles.INTERNAL_USER.value) + + with ( + patch.object(mcp_module.global_mcp_server_manager, "_get_mcp_server_from_tool_name", return_value=server), + patch.object(mcp_module.global_mcp_server_manager, "pre_call_tool_check", new=AsyncMock(return_value={})), + patch.object(mcp_module.global_mcp_tool_registry, "get_tool", return_value=fake_tool), + patch.object( + mcp_module.global_mcp_server_manager, + "resolve_openapi_upstream_auth", + new=AsyncMock(return_value=(None, None)), + ), + patch( + "litellm.proxy._experimental.mcp_server.server.MCPRequestHandler.is_tool_allowed", + return_value=True, + ), + ): + call = mcp_module.execute_mcp_tool( + name="list_reports", + arguments={}, + allowed_mcp_servers=[server], + start_time=datetime.now(timezone.utc), + user_api_key_auth=user, + ) + if failure == "auth": + with pytest.raises(MCPUpstreamAuthError): + await call + return + result = await call + + # A non-auth upstream failure stays a 200 with isError, so REST does not report it as a gateway 500 + assert result.isError is True + assert "upstream returned HTTP 429" in result.content[0].text