diff --git a/litellm/llms/anthropic/messages/native/mcp_handler.py b/litellm/llms/anthropic/messages/native/mcp_handler.py index 8fe18d79f24..aaa16469236 100644 --- a/litellm/llms/anthropic/messages/native/mcp_handler.py +++ b/litellm/llms/anthropic/messages/native/mcp_handler.py @@ -48,17 +48,20 @@ def _get_stop_reason(response: AnthropicMessagesResponse) -> str | None: def _build_tool_result_message(tool_results: Sequence[Mapping[str, object]]) -> AnthropicMessagesUserMessageParam: """Turn executed tool results into the user message Anthropic expects.""" - return AnthropicMessagesUserMessageParam( - role="user", - content=[ - AnthropicMessagesToolResultParam( - type="tool_result", - tool_use_id=str(result.get("tool_call_id") or ""), - content=str(result.get("result") or ""), - ) - for result in tool_results - ], - ) + content: list[AnthropicMessagesToolResultParam] = [] + for result in tool_results: + tool_result = AnthropicMessagesToolResultParam( + type="tool_result", + tool_use_id=str(result.get("tool_call_id") or ""), + content=str(result.get("result") or ""), + ) + if result.get("is_error"): + # Propagate MCP failures (CallToolResult.isError or an execution + # exception) so Anthropic's native error-handling triggers instead + # of reading the failure string as a successful tool return (#49359). + tool_result["is_error"] = True + content.append(tool_result) + return AnthropicMessagesUserMessageParam(role="user", content=content) async def anthropic_messages_with_mcp( diff --git a/litellm/proxy/agent_endpoints/a2a_endpoints.py b/litellm/proxy/agent_endpoints/a2a_endpoints.py index 3dfaa7f213e..9ccca4298c1 100644 --- a/litellm/proxy/agent_endpoints/a2a_endpoints.py +++ b/litellm/proxy/agent_endpoints/a2a_endpoints.py @@ -149,7 +149,7 @@ def _build_message_send_params(params: dict[str, Any]) -> "MessageSendParams": from a2a.compat.v0_3.types import MessageSendParams try: - return MessageSendParams(**params) + built = MessageSendParams(**params) except ValidationError: from a2a.compat.v0_3.conversions import pb2_v10, to_compat_send_message_request from google.protobuf.json_format import ParseDict, ParseError @@ -159,7 +159,49 @@ def _build_message_send_params(params: dict[str, Any]) -> "MessageSendParams": ParseDict(params, pb, ignore_unknown_fields=True) except ParseError as e: raise ValueError(f"Invalid message/send params: {e}") from e - return to_compat_send_message_request(pb, "").params + built = to_compat_send_message_request(pb, "").params + # The 1.0->0.3 conversion drops pushNotificationConfig, so validate the + # raw wire config on this path too. + _validate_inline_push_notification_url(params) + # A push config registered inline via message/send or message/stream must + # clear the same SSRF guard the tasks/pushNotificationConfig/set method + # applies, otherwise message/send is a way around it (#49622). Runs on both + # paths: the 1.0 taskPushNotificationConfig spelling survives the + # conversion into built.configuration.push_notification_config. + push_config = built.configuration.push_notification_config if built.configuration else None + callback_url = push_config.url if push_config is not None else None + if callback_url: + _validate_push_notification_url(str(callback_url)) + return built + + +def _validate_inline_push_notification_url(params: Mapping[str, Any]) -> None: + """Reject an unsafe inline pushNotificationConfig carried in wire params. + + Covers the A2A 1.0 shapes (``configuration.pushNotificationConfig.url`` and + ``configuration.taskPushNotificationConfig.url``, camelCase or snake_case) + on the proto-fallback path, where a dropped config no longer exists in the + built params. + """ + configuration = params.get("configuration") + if not isinstance(configuration, Mapping): + return + push_config: Any = None + for key in ( + "pushNotificationConfig", + "push_notification_config", + "taskPushNotificationConfig", + "task_push_notification_config", + ): + value = configuration.get(key) + if isinstance(value, Mapping): + push_config = value + break + if push_config is None: + return + callback_url = push_config.get("url") + if isinstance(callback_url, str) and callback_url: + _validate_push_notification_url(callback_url) def _served_version(agent: "AgentResponse", request: Request, original_method: str | None = None) -> A2AVersion: diff --git a/litellm/responses/mcp/litellm_proxy_mcp_handler.py b/litellm/responses/mcp/litellm_proxy_mcp_handler.py index 1104aee353e..abdc7e82614 100644 --- a/litellm/responses/mcp/litellm_proxy_mcp_handler.py +++ b/litellm/responses/mcp/litellm_proxy_mcp_handler.py @@ -6,7 +6,7 @@ from typing import TYPE_CHECKING, Any, Final, Literal, Optional, TypeAlias, Type from openai.types.chat import ChatCompletionToolParam from openai.types.responses.function_tool_param import FunctionToolParam from typing_extensions import TypedDict as ExtraItemsTypedDict -from typing_extensions import Unpack +from typing_extensions import NotRequired, Unpack from litellm._logging import verbose_logger from litellm.constants import MAXIMUM_TRACEBACK_LINES_TO_LOG @@ -62,6 +62,12 @@ class MCPToolResult(TypedDict): tool_call_id: str | None result: str name: str | None + # True when the MCP server returned CallToolResult.isError or execution + # raised (guardrail block, HTTP failure, unexpected exception). Anthropic + # native MCP consumers pass is_error=True on the tool_result block so the + # model can react to the failure instead of reading it as a success (#49359). + # Optional via NotRequired (PEP 655) so the three original keys stay required. + is_error: NotRequired[bool] class _AresponsesOptions(ExtraItemsTypedDict, total=False, extra_items=object): @@ -948,13 +954,16 @@ class LiteLLM_Proxy_MCP_Handler: # Format result for inclusion in response result_text = LiteLLM_Proxy_MCP_Handler._parse_mcp_result(result) - tool_results.append( - { - "tool_call_id": tool_call_id, - "result": result_text, - "name": tool_name, - } - ) + tool_result: MCPToolResult = { + "tool_call_id": tool_call_id, + "result": result_text, + "name": tool_name, + } + # mcp>=2.3.0 names the flag is_error (constructor alias isError); + # accept both so older SDK versions keep working. + if getattr(result, "is_error", False) or getattr(result, "isError", False): + tool_result["is_error"] = True + tool_results.append(tool_result) except BlockedPiiEntityError as e: await LiteLLM_Proxy_MCP_Handler._log_mcp_tool_failure( @@ -970,6 +979,7 @@ class LiteLLM_Proxy_MCP_Handler: "tool_call_id": tool_call_id, "result": error_message, "name": tool_name, + "is_error": True, } ) except GuardrailRaisedException as e: @@ -988,6 +998,7 @@ class LiteLLM_Proxy_MCP_Handler: "tool_call_id": tool_call_id, "result": error_message, "name": tool_name, + "is_error": True, } ) except HTTPException as e: @@ -1004,6 +1015,7 @@ class LiteLLM_Proxy_MCP_Handler: "tool_call_id": tool_call_id, "result": error_message, "name": tool_name, + "is_error": True, } ) except Exception as e: @@ -1019,6 +1031,7 @@ class LiteLLM_Proxy_MCP_Handler: "tool_call_id": tool_call_id, "result": f"Error executing tool: {e}", "name": tool_name, + "is_error": True, } ) diff --git a/tests/unit/llms/anthropic/messages/native/test_mcp_handler.py b/tests/unit/llms/anthropic/messages/native/test_mcp_handler.py index c0e0ea9f803..e54240391f8 100644 --- a/tests/unit/llms/anthropic/messages/native/test_mcp_handler.py +++ b/tests/unit/llms/anthropic/messages/native/test_mcp_handler.py @@ -286,9 +286,7 @@ async def test_anthropic_messages_with_mcp_hands_execution_the_requests_served_t ] process = AsyncMock(return_value=(served, {"read_wiki_structure": "deepwiki"})) - execute = AsyncMock( - return_value=[{"tool_call_id": "toolu_1", "result": "ok", "name": "read_wiki_structure"}] - ) + execute = AsyncMock(return_value=[{"tool_call_id": "toolu_1", "result": "ok", "name": "read_wiki_structure"}]) responses = [ { "stop_reason": "tool_use", @@ -482,3 +480,41 @@ async def test_anthropic_messages_with_mcp_loads_a_filtered_routes_beta_map_off_ @pytest.mark.asyncio async def test_anthropic_messages_with_mcp_never_loads_the_beta_map_for_first_party_anthropic(): assert await _mcp_safeguards_turn_recording_beta_map_loads("anthropic", "claude-sonnet-4-6") == () + + +def test_build_tool_result_message_marks_is_error_results(): + """ + Regression test (#49359): a tool result flagged is_error by the MCP gateway + must reach Anthropic as is_error=True so the model reacts to the failure. + + Given: tool results where one carries is_error=True (MCP CallToolResult.isError + or an execution exception path in execute_tool_calls) + When: _build_tool_result_message renders them + Then: Only the flagged result gets is_error; the success stays clean. + """ + message = _build_tool_result_message( + [ + {"tool_call_id": "call_ok", "result": "9 sections", "name": "read_wiki"}, + {"tool_call_id": "call_bad", "result": "Error executing tool: boom", "name": "read_wiki", "is_error": True}, + ] + ) + + assert message["role"] == "user" + assert message["content"][0] == {"type": "tool_result", "tool_use_id": "call_ok", "content": "9 sections"} + assert message["content"][1] == { + "type": "tool_result", + "tool_use_id": "call_bad", + "content": "Error executing tool: boom", + "is_error": True, + } + + +def test_build_tool_result_message_omits_is_error_for_successes(): + """ + Successful tool results must not carry is_error at all (Anthropic treats + is_error: false the same, but omitting keeps the payload minimal and the + chat-completions bridge unaffected). + """ + message = _build_tool_result_message([{"tool_call_id": "call_1", "result": "5", "name": "add", "is_error": False}]) + + assert message["content"][0] == {"type": "tool_result", "tool_use_id": "call_1", "content": "5"} diff --git a/tests/unit/proxy/agent_endpoints/test_a2a_endpoints.py b/tests/unit/proxy/agent_endpoints/test_a2a_endpoints.py index 88da07c612b..e8b4e3139ba 100644 --- a/tests/unit/proxy/agent_endpoints/test_a2a_endpoints.py +++ b/tests/unit/proxy/agent_endpoints/test_a2a_endpoints.py @@ -3196,3 +3196,156 @@ def test_a2a_http_refusals_answer_as_jsonrpc_errors_with_a_null_id_for_unparseab assert body["error"]["code"] == -32000 assert body["error"]["message"] == "Agent 'agent-1' is not allowed" assert body["error"]["data"]["code"] == "403" + + +def test_message_send_params_rejects_inline_http_push_url(): + """Regression (#49622): message/send's inline pushNotificationConfig must clear the same SSRF guard as tasks/pushNotificationConfig/set.""" + from fastapi import HTTPException + + from litellm.proxy.agent_endpoints.a2a_endpoints import _build_message_send_params + + params = { + "message": { + "messageId": "msg-1", + "role": "ROLE_USER", + "parts": [{"text": "hello"}], + }, + "configuration": { + "pushNotificationConfig": { + "url": "http://webhook.example.com/hook", + "token": "tok", + } + }, + } + + with pytest.raises(HTTPException) as exc_info: + _build_message_send_params(params) + + assert exc_info.value.status_code == 400 + assert "HTTPS" in exc_info.value.detail + + +def test_message_send_params_rejects_inline_private_address_push_url(): + """Regression (#49622): a link-local inline push URL is refused before anything reaches the agent.""" + import socket as socket_module + + from fastapi import HTTPException + + from litellm.proxy.agent_endpoints.a2a_endpoints import _build_message_send_params + + params = { + "message": { + "messageId": "msg-1", + "role": "ROLE_USER", + "parts": [{"text": "hello"}], + }, + "configuration": { + "pushNotificationConfig": { + "url": "https://169.254.169.254/latest/meta-data/", + } + }, + } + + with pytest.raises(HTTPException) as exc_info: + _build_message_send_params(params) + + assert exc_info.value.status_code == 400 + + +def test_message_send_params_accepts_https_public_push_url(): + """A compliant inline push config passes through unchanged.""" + from litellm.proxy.agent_endpoints.a2a_endpoints import _build_message_send_params + + params = { + "message": { + "messageId": "msg-1", + "role": "user", + "parts": [{"text": "hello"}], + }, + "configuration": { + "pushNotificationConfig": { + "url": "https://webhook.example.com/hook", + "token": "tok", + } + }, + } + + # The public-address SSRF check itself is covered by the dedicated + # _validate_push_notification_url tests; keep this test offline by + # letting the (already scheme-validated) https URL through. + with patch("litellm.proxy.agent_endpoints.a2a_endpoints.validate_url", return_value=None): + result = _build_message_send_params(params) + assert result.message.role.value == "user" + assert result.configuration is not None + assert result.configuration.push_notification_config is not None + assert result.configuration.push_notification_config.url == "https://webhook.example.com/hook" + + +def test_message_send_params_without_push_config_is_untouched(): + """No inline push config: nothing extra is validated, plain sends behave as before.""" + from litellm.proxy.agent_endpoints.a2a_endpoints import _build_message_send_params + + result = _build_message_send_params( + { + "message": { + "messageId": "msg-1", + "role": "ROLE_USER", + "parts": [{"text": "hello"}], + }, + "configuration": {"blocking": True}, + } + ) + assert result.message.role.value == "user" + + +def test_message_send_params_rejects_a2a_10_task_push_url_loopback(): + """Regression (#49637 review): A2A 1.0 taskPushNotificationConfig survives the + 1.0->0.3 conversion into built.configuration.push_notification_config, so the + built-object check must run on the fallback path too — a loopback webhook on + message/send is refused before reaching the agent.""" + from fastapi import HTTPException + + from litellm.proxy.agent_endpoints.a2a_endpoints import _build_message_send_params + + params = { + "message": { + "messageId": "msg-1", + "role": "ROLE_USER", + "parts": [{"text": "hello"}], + }, + "configuration": { + "taskPushNotificationConfig": { + "url": "http://127.0.0.1:8080/hook", + } + }, + } + + with pytest.raises(HTTPException) as exc_info: + _build_message_send_params(params) + + assert exc_info.value.status_code == 400 + + +def test_message_send_params_rejects_snake_case_task_push_url(): + """The snake_case task_push_notification_config wire spelling is covered too.""" + from fastapi import HTTPException + + from litellm.proxy.agent_endpoints.a2a_endpoints import _build_message_send_params + + params = { + "message": { + "messageId": "msg-1", + "role": "ROLE_USER", + "parts": [{"text": "hello"}], + }, + "configuration": { + "task_push_notification_config": { + "url": "http://10.0.0.9/hook", + } + }, + } + + with pytest.raises(HTTPException) as exc_info: + _build_message_send_params(params) + + assert exc_info.value.status_code == 400 diff --git a/tests/unit/responses/mcp/test_litellm_proxy_mcp_handler.py b/tests/unit/responses/mcp/test_litellm_proxy_mcp_handler.py index a16661c0174..69569eb4853 100644 --- a/tests/unit/responses/mcp/test_litellm_proxy_mcp_handler.py +++ b/tests/unit/responses/mcp/test_litellm_proxy_mcp_handler.py @@ -49,7 +49,14 @@ class _DummyMCPResult: def _setup_mcp_call_environment(monkeypatch: pytest.MonkeyPatch) -> AsyncMock: """Patch MCP globals so _execute_tool_calls can run in tests.""" - proxy_module = types.SimpleNamespace(proxy_logging_obj=object(), prisma_client=None) + proxy_logging_obj = MagicMock() + + async def _passthrough_post_mcp_call_hook(response, **_): + return response + + proxy_logging_obj.post_mcp_call_hook = AsyncMock(side_effect=_passthrough_post_mcp_call_hook) + proxy_logging_obj.post_call_failure_hook = AsyncMock() + proxy_module = types.SimpleNamespace(proxy_logging_obj=proxy_logging_obj, prisma_client=None) monkeypatch.setitem(sys.modules, "litellm.proxy.proxy_server", proxy_module) fake_manager = types.SimpleNamespace( @@ -146,9 +153,7 @@ def test_chat_follow_up_answers_only_tool_calls_in_its_assistant_turn(): "message": { "role": "assistant", "content": None, - "tool_calls": [ - {"id": call_id, "type": "function", "function": {"name": "foo", "arguments": "{}"}} - ], + "tool_calls": [{"id": call_id, "type": "function", "function": {"name": "foo", "arguments": "{}"}}], }, "finish_reason": "tool_calls", } @@ -621,7 +626,9 @@ async def test_execute_tool_calls_applies_post_call_hook_content(monkeypatch): user_api_key_auth=None, ) - assert results == [{"tool_call_id": "call-1", "result": "[REDACTED]", "name": tool_name}] + # The mocked async_post_mcp_tool_call_hook returns a result flagged is_error=True, + # so the gateway must propagate that flag (#49359) alongside the redacted text. + assert results == [{"tool_call_id": "call-1", "result": "[REDACTED]", "name": tool_name, "is_error": True}] assert logging_obj.async_success_handler.await_args.kwargs["result"].content[0].text == "[REDACTED]" assert logging_obj.async_success_handler.await_args.kwargs["result"].structured_content is None @@ -1909,3 +1916,105 @@ async def test_apply_toolset_permissions_pins_the_auth_to_explicit_grants_only(m assert scoped.object_permission is not None assert scoped.object_permission.mcp_servers == ["srv-1"] assert scoped.object_permission.mcp_tool_permissions == {"srv-1": ["add"]} + + +@pytest.mark.asyncio +async def test_execute_tool_calls_propagates_mcp_is_error_flag(monkeypatch): + """ + Regression test (#49359): CallToolResult.isError=True must surface as + is_error=True on the MCPToolResult so Anthropic's native MCP loop can mark + the tool_result block as failed. + """ + call_tool_mock = _setup_mcp_call_environment(monkeypatch) + call_tool_mock.return_value = CallToolResult( + content=[TextContent(type="text", text="Database connection refused")], + isError=True, + ) + tool_name = "read_wiki_structure" + tool_calls = [{"id": "call-err", "function": {"name": tool_name, "arguments": "{}"}}] + + results = await LiteLLM_Proxy_MCP_Handler.execute_tool_calls( + tool_server_map={tool_name: "deepwiki"}, + tool_calls=tool_calls, + user_api_key_auth=None, + ) + + assert results == [ + { + "tool_call_id": "call-err", + "result": "Database connection refused", + "name": tool_name, + "is_error": True, + } + ] + + +@pytest.mark.asyncio +async def test_execute_tool_calls_success_has_no_is_error(monkeypatch): + """A successful CallToolResult must not carry the is_error key at all.""" + call_tool_mock = _setup_mcp_call_environment(monkeypatch) + call_tool_mock.return_value = CallToolResult( + content=[TextContent(type="text", text="9 sections")], + isError=False, + ) + tool_name = "read_wiki_structure" + tool_calls = [{"id": "call-ok", "function": {"name": tool_name, "arguments": "{}"}}] + + results = await LiteLLM_Proxy_MCP_Handler.execute_tool_calls( + tool_server_map={tool_name: "deepwiki"}, + tool_calls=tool_calls, + user_api_key_auth=None, + ) + + assert results == [ + { + "tool_call_id": "call-ok", + "result": "9 sections", + "name": tool_name, + } + ] + + +@pytest.mark.asyncio +async def test_execute_tool_calls_marks_exception_results_as_error(monkeypatch): + """An unexpected exception in tool execution must set is_error=True.""" + call_tool_mock = _setup_mcp_call_environment(monkeypatch) + call_tool_mock.side_effect = RuntimeError("boom") + tool_name = "read_wiki_structure" + tool_calls = [{"id": "call-boom", "function": {"name": tool_name, "arguments": "{}"}}] + + results = await LiteLLM_Proxy_MCP_Handler.execute_tool_calls( + tool_server_map={tool_name: "deepwiki"}, + tool_calls=tool_calls, + user_api_key_auth=None, + ) + + assert results == [ + { + "tool_call_id": "call-boom", + "result": "Error executing tool: boom", + "name": tool_name, + "is_error": True, + } + ] + + +def test_mcp_tool_result_keeps_original_keys_required_statically(): + """ + PEP 655 negative control (#49359 review): ``is_error`` is the only optional + key; ``tool_call_id``, ``result``, and ``name`` must stay required, so a + bare ``{"is_error": True}`` literal is rejected at type-check time. This + pins the static contract the runtime tests cannot see. + """ + from typing_extensions import assert_type + + from litellm.responses.mcp.litellm_proxy_mcp_handler import MCPToolResult + + # The three original keys keep Required status: total=False was reverted. + assert not MCPToolResult.__required_keys__ < {"tool_call_id", "result", "name"} + assert MCPToolResult.__required_keys__ == {"tool_call_id", "result", "name"} + assert MCPToolResult.__optional_keys__ == {"is_error"} + + # Runtime shape of a full result is unchanged. + full: MCPToolResult = {"tool_call_id": "call-1", "result": "ok", "name": "echo"} + assert_type(full, MCPToolResult)