fix(mcp): stop reporting failed OpenAPI tool calls as successes (#37496)

An OpenAPI-backed MCP tool whose upstream answered 401 came back as a
successful tool result carrying the upstream's rejection as its content, so a
caller saw {"error":"invalid_token"} presented as data and the gateway recorded
the request in its own spend log as call_mcp_tool | success.

Three layers each erased the outcome. The request function returned
response.text whatever the status, _handle_local_mcp_tool caught every exception
and returned it as ordinary TextContent, and both dispatch sites then stamped
isError=False unconditionally. Fixing only the first, which is the obvious fix,
changes nothing, because the two above it still map failure onto the
success-shaped value.

The status is now classified where the response is held: a 401 becomes
MCPUpstreamAuthError so the caller is told to re-authenticate, and every other
non-2xx becomes MCPOpenApiUpstreamError, which carries the status and drops the
upstream body rather than serving it as tool content. _handle_local_mcp_tool no
longer swallows, and the call_tool arm keeps the auth error's type. Nothing new
renders these: call_mcp_tool and call_tool_rest_api already turn them into an
isError result naming the status and into a real 401 with WWW-Authenticate, and
the OpenAPI path simply never reached them.

The result is now byte-identical to the regular MCP path for the same failure.
This commit is contained in:
tin-berri 2026-08-19 14:26:02 -07:00 committed by GitHub
parent afbfc3f8fa
commit 0de60a2ff2
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 370 additions and 31 deletions

View file

@ -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.

View file

@ -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)

View file

@ -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

View file

@ -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:
"""

View file

@ -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

View file

@ -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}"

View file

@ -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