diff --git a/plugins/communication_protocols/http/src/utcp_http/_errors.py b/plugins/communication_protocols/http/src/utcp_http/_errors.py new file mode 100644 index 0000000..b3a0e90 --- /dev/null +++ b/plugins/communication_protocols/http/src/utcp_http/_errors.py @@ -0,0 +1,104 @@ +"""Surface the server's error body on failed HTTP calls. + +``aiohttp.ClientResponse.raise_for_status()`` raises a ``ClientResponseError`` +whose ``message`` is only the reason phrase ("Forbidden"). Servers put the +real reason in the response body, typically ``{"error": "..."}``, and that +was discarded, so a refused call or discovery surfaced as nothing more than a +status code. Mirrors the TypeScript SDK's ``_normalizeToolError``. +""" +import json +from typing import Optional + +import aiohttp + +# Bodies are folded into an exception message; keep pathological ones bounded. +MAX_DETAIL_CHARS = 2000 + +# How much of an error body is read at all. The response may come from an +# attacker-controlled endpoint (discovery URLs are exactly that trust +# surface), so the read is bounded up front rather than buffered in full and +# truncated afterwards. Comfortably larger than MAX_DETAIL_CHARS so a JSON +# body with a long ``error`` field still parses. +MAX_BODY_READ_BYTES = 64 * 1024 + +_DETAIL_KEYS = ("error", "message", "detail") + + +def error_detail_from_body(text: str) -> Optional[str]: + """Extract a human-readable reason from an error response body. + + When the body is a JSON object, the first of ``error`` / ``message`` / + ``detail`` that is present decides: a non-empty string is returned as the + reason; anything else (an object, a list, a number) means the server sent a + structured error, so the raw JSON is returned to keep that structure + visible rather than skipping ahead to a lower-priority generic string. + A non-JSON body is returned as-is. Returns ``None`` for an empty body. + """ + body = text.strip() + if not body: + return None + try: + data = json.loads(body) + except ValueError: + return body[:MAX_DETAIL_CHARS] + if isinstance(data, dict): + for key in _DETAIL_KEYS: + if key not in data or data[key] is None: + continue + value = data[key] + if isinstance(value, str): + if value.strip(): + return value.strip()[:MAX_DETAIL_CHARS] + continue + # Structured error: show it rather than a later generic string. + return body[:MAX_DETAIL_CHARS] + return body[:MAX_DETAIL_CHARS] + + +async def _read_body_bounded(response: aiohttp.ClientResponse, limit: int) -> str: + """Read at most ``limit`` bytes of the body and decode them leniently.""" + chunks = [] + total = 0 + async for chunk in response.content.iter_chunked(8192): + chunks.append(chunk) + total += len(chunk) + if total >= limit: + break + raw = b"".join(chunks)[:limit] + # ``charset`` only parses the Content-Type header, but stay defensive: the + # body was read directly, so aiohttp's own buffered-body machinery must not + # be relied on, and an unknown charset name must not lose the detail. + try: + encoding = response.charset or "utf-8" + raw.decode(encoding, errors="replace") + except (LookupError, RuntimeError, ValueError): + encoding = "utf-8" + return raw.decode(encoding, errors="replace") + + +async def raise_for_status_with_body(response: aiohttp.ClientResponse) -> None: + """Like ``response.raise_for_status()``, but with the response body in the error. + + On a 4xx/5xx, reads up to ``MAX_BODY_READ_BYTES`` of the body and raises a + ``ClientResponseError`` of the same status and headers whose ``message`` is + ``": "``. The text that was read is attached as ``body`` + for callers that want the structure. Does nothing on a 2xx/3xx. + """ + if response.status < 400: + return + try: + text = await _read_body_bounded(response, MAX_BODY_READ_BYTES) + except Exception: + text = "" + detail = error_detail_from_body(text) + reason = response.reason or "" + message = f"{reason}: {detail}" if detail else reason + error = aiohttp.ClientResponseError( + response.request_info, + response.history, + status=response.status, + message=message, + headers=response.headers, + ) + error.body = text # type: ignore[attr-defined] + raise error diff --git a/plugins/communication_protocols/http/src/utcp_http/http_communication_protocol.py b/plugins/communication_protocols/http/src/utcp_http/http_communication_protocol.py index 8a3a187..99d5de2 100644 --- a/plugins/communication_protocols/http/src/utcp_http/http_communication_protocol.py +++ b/plugins/communication_protocols/http/src/utcp_http/http_communication_protocol.py @@ -34,6 +34,7 @@ from aiohttp import ClientSession, BasicAuth as AiohttpBasicAuth from utcp_http.openapi_converter import OpenApiConverter from utcp_http._security import ensure_secure_url, safe_request_with_redirects +from utcp_http._errors import raise_for_status_with_body import logging logging.basicConfig( @@ -210,7 +211,7 @@ async def register_manual(self, caller, manual_call_template: CallTemplate) -> R timeout=aiohttp.ClientTimeout(total=10.0), auth_header_names=auth_header_names, ) as response: - response.raise_for_status() # Raise exception for 4XX/5XX responses + await raise_for_status_with_body(response) # 4XX/5XX, with the server's body in the message # Check content type to determine how to parse the response content_type = response.headers.get('Content-Type', '') @@ -359,7 +360,7 @@ async def call_tool(self, caller, tool_name: str, tool_args: Dict[str, Any], too timeout=aiohttp.ClientTimeout(total=30.0), auth_header_names=auth_header_names, ) as response: - response.raise_for_status() + await raise_for_status_with_body(response) content_type = response.headers.get('Content-Type', '').lower() if 'application/json' in content_type: diff --git a/plugins/communication_protocols/http/src/utcp_http/sse_communication_protocol.py b/plugins/communication_protocols/http/src/utcp_http/sse_communication_protocol.py index a4d284f..fdb2ff5 100644 --- a/plugins/communication_protocols/http/src/utcp_http/sse_communication_protocol.py +++ b/plugins/communication_protocols/http/src/utcp_http/sse_communication_protocol.py @@ -18,6 +18,7 @@ from utcp.data.auth_implementations.oauth2_auth import OAuth2Auth from utcp_http.sse_call_template import SseCallTemplate from aiohttp import ClientSession, BasicAuth as AiohttpBasicAuth +from utcp_http._errors import raise_for_status_with_body from utcp_http._security import ensure_secure_url, safe_request_with_redirects import traceback import logging @@ -170,7 +171,7 @@ async def register_manual(self, caller, manual_call_template: CallTemplate) -> R timeout=aiohttp.ClientTimeout(total=10.0), auth_header_names=auth_header_names, ) as response: - response.raise_for_status() + await raise_for_status_with_body(response) response_data = await response.json() utcp_manual = UtcpManualSerializer().validate_dict(response_data) return RegisterManualResult( diff --git a/plugins/communication_protocols/http/src/utcp_http/streamable_http_communication_protocol.py b/plugins/communication_protocols/http/src/utcp_http/streamable_http_communication_protocol.py index fde6cb5..df4b94f 100644 --- a/plugins/communication_protocols/http/src/utcp_http/streamable_http_communication_protocol.py +++ b/plugins/communication_protocols/http/src/utcp_http/streamable_http_communication_protocol.py @@ -15,6 +15,7 @@ from utcp.data.auth_implementations import OAuth2Auth from utcp_http.streamable_http_call_template import StreamableHttpCallTemplate from aiohttp import ClientSession, BasicAuth as AiohttpBasicAuth, ClientResponse +from utcp_http._errors import raise_for_status_with_body from utcp_http._security import ensure_secure_url, safe_request_with_redirects import logging @@ -149,7 +150,7 @@ async def register_manual(self, caller, manual_call_template: CallTemplate) -> R timeout=aiohttp.ClientTimeout(total=10.0), auth_header_names=auth_header_names, ) as response: - response.raise_for_status() + await raise_for_status_with_body(response) response_data = await response.json() utcp_manual = UtcpManualSerializer().validate_dict(response_data) return RegisterManualResult( diff --git a/plugins/communication_protocols/http/tests/test_http_communication_protocol.py b/plugins/communication_protocols/http/tests/test_http_communication_protocol.py index 518b8df..0dd25e5 100644 --- a/plugins/communication_protocols/http/tests/test_http_communication_protocol.py +++ b/plugins/communication_protocols/http/tests/test_http_communication_protocol.py @@ -139,6 +139,42 @@ async def error_handler(request): app.router.add_post('/token', token_handler) app.router.add_post('/token_header_auth', token_header_auth_handler) app.router.add_get('/error', error_handler) + + # Non-2xx with a descriptive body, like a real API refusing a call. + async def forbidden_handler(request): + return web.json_response({"error": "You are not allowed to do that, and here is exactly why."}, status=403) + + # Some APIs nest an object under `error`; the message must show its JSON. + async def forbidden_object_handler(request): + return web.json_response({"error": {"code": "INVALID_FIELD", "reason": "value out of range"}}, status=422) + + app.router.add_route('*', '/forbidden', forbidden_handler) + app.router.add_route('*', '/forbidden-object', forbidden_object_handler) + + # A structured `error` next to a generic `message`: the structure must win. + async def forbidden_object_then_message_handler(request): + return web.json_response( + {"error": {"code": "INVALID_FIELD", "reason": "value out of range"}, "message": "Request failed"}, + status=422, + ) + + # A huge error body (think an HTML stack trace): the read itself is bounded. + async def forbidden_huge_handler(request): + return web.Response(status=403, text="x" * (1024 * 1024)) + + app.router.add_route('*', '/forbidden-object-then-message', forbidden_object_then_message_handler) + app.router.add_route('*', '/forbidden-huge', forbidden_huge_handler) + + # No Content-Type at all, so no charset to decode with. + async def forbidden_no_charset_handler(request): + return web.Response(status=403, body=b'{"error": "no charset here"}') + + # A charset Python does not know. + async def forbidden_bad_charset_handler(request): + return web.Response(status=403, body=b'{"error": "odd charset"}', content_type="application/json", charset="x-unknown-charset") + + app.router.add_route('*', '/forbidden-no-charset', forbidden_no_charset_handler) + app.router.add_route('*', '/forbidden-bad-charset', forbidden_bad_charset_handler) return app @@ -736,3 +772,97 @@ def test_auth_tools_integration(): serialized = serializer.to_dict(call_template) assert "auth_tools" in serialized assert serialized["auth_tools"]["auth_type"] == "api_key" + + +# --- Server error bodies are surfaced, not just status codes --- + +@pytest.mark.asyncio +async def test_call_tool_surfaces_server_error_body(http_transport, aiohttp_client, app): + """A refused call carries the server's reason, not only "403, message='Forbidden'".""" + client = await aiohttp_client(app) + call_template = HttpCallTemplate(name="t", url=f"http://localhost:{client.port}/forbidden", http_method="POST") + with pytest.raises(aiohttp.ClientResponseError) as excinfo: + await http_transport.call_tool(None, "t.tool", {"param1": "value1"}, call_template) + assert excinfo.value.status == 403 + assert "You are not allowed to do that, and here is exactly why." in str(excinfo.value) + assert '"error"' in excinfo.value.body + + +@pytest.mark.asyncio +async def test_call_tool_surfaces_object_valued_error_field(http_transport, aiohttp_client, app): + """An object under `error` shows its JSON structure in the message.""" + client = await aiohttp_client(app) + call_template = HttpCallTemplate(name="t", url=f"http://localhost:{client.port}/forbidden-object", http_method="POST") + with pytest.raises(aiohttp.ClientResponseError) as excinfo: + await http_transport.call_tool(None, "t.tool", {}, call_template) + assert excinfo.value.status == 422 + assert "INVALID_FIELD" in str(excinfo.value) + assert "value out of range" in str(excinfo.value) + + +@pytest.mark.asyncio +async def test_register_manual_surfaces_server_error_body(http_transport, aiohttp_client, app): + """A refused discovery reports the server's reason in errors[].""" + client = await aiohttp_client(app) + call_template = HttpCallTemplate(name="t", url=f"http://localhost:{client.port}/forbidden", http_method="GET") + result = await http_transport.register_manual(None, call_template) + assert result.success is False + assert "You are not allowed to do that, and here is exactly why." in result.errors[0] + assert "403" in result.errors[0] + + +@pytest.mark.asyncio +async def test_structured_error_wins_over_generic_message(http_transport, aiohttp_client, app): + """An object under `error` is shown even when a lower-priority string field exists.""" + client = await aiohttp_client(app) + call_template = HttpCallTemplate(name="t", url=f"http://localhost:{client.port}/forbidden-object-then-message", http_method="POST") + with pytest.raises(aiohttp.ClientResponseError) as excinfo: + await http_transport.call_tool(None, "t.tool", {}, call_template) + assert "INVALID_FIELD" in excinfo.value.message + # The detail is the whole structured body, not the generic "Request failed". + from utcp_http._errors import error_detail_from_body + import json as _json + assert _json.loads(error_detail_from_body(excinfo.value.body)) == { + "error": {"code": "INVALID_FIELD", "reason": "value out of range"}, + "message": "Request failed", + } + + +@pytest.mark.asyncio +async def test_huge_error_body_is_read_bounded(http_transport, aiohttp_client, app): + """A 1 MiB error body is neither buffered in full nor folded into the message in full.""" + from utcp_http._errors import MAX_BODY_READ_BYTES, MAX_DETAIL_CHARS + client = await aiohttp_client(app) + call_template = HttpCallTemplate(name="t", url=f"http://localhost:{client.port}/forbidden-huge", http_method="POST") + with pytest.raises(aiohttp.ClientResponseError) as excinfo: + await http_transport.call_tool(None, "t.tool", {}, call_template) + assert excinfo.value.status == 403 + assert len(excinfo.value.body) <= MAX_BODY_READ_BYTES + assert len(excinfo.value.message) <= MAX_DETAIL_CHARS + 50 + + +def test_error_detail_from_body_precedence_and_fallbacks(): + from utcp_http._errors import error_detail_from_body + assert error_detail_from_body("") is None + assert error_detail_from_body(" ") is None + assert error_detail_from_body("plain text") == "plain text" + assert error_detail_from_body('{"error": "nope"}') == "nope" + assert error_detail_from_body('{"message": "nope"}') == "nope" + # Structured error beats a later generic string. + assert error_detail_from_body('{"error": {"code": "X"}, "message": "generic"}') == '{"error": {"code": "X"}, "message": "generic"}' + # An explicit null or blank string is skipped, not treated as structured. + assert error_detail_from_body('{"error": null, "message": "generic"}') == "generic" + assert error_detail_from_body('{"error": " ", "detail": "specific"}') == "specific" + # Non-object JSON falls back to the raw body. + assert error_detail_from_body('["a", "b"]') == '["a", "b"]' + + +@pytest.mark.asyncio +@pytest.mark.parametrize("path,expected", [("/forbidden-no-charset", "no charset here"), ("/forbidden-bad-charset", "odd charset")]) +async def test_error_body_is_surfaced_without_a_usable_charset(http_transport, aiohttp_client, app, path, expected): + """A missing or unknown charset must not lose the body; decode as UTF-8.""" + client = await aiohttp_client(app) + call_template = HttpCallTemplate(name="t", url=f"http://localhost:{client.port}{path}", http_method="POST") + with pytest.raises(aiohttp.ClientResponseError) as excinfo: + await http_transport.call_tool(None, "t.tool", {}, call_template) + assert expected in excinfo.value.message diff --git a/plugins/communication_protocols/http/tests/test_sse_communication_protocol.py b/plugins/communication_protocols/http/tests/test_sse_communication_protocol.py index 48f73c6..96d5351 100644 --- a/plugins/communication_protocols/http/tests/test_sse_communication_protocol.py +++ b/plugins/communication_protocols/http/tests/test_sse_communication_protocol.py @@ -106,6 +106,10 @@ async def error_handler(request): return web.Response(status=500, text="Internal Server Error") +async def forbidden_discovery_handler(request): + return web.Response(status=403, text="discovery refused: tenant is not provisioned for streaming") + + async def crlf_split_events_handler(request): """One multi-line CRLF event whose CRLF is split across two writes.""" response = web.StreamResponse(status=200, headers={'Content-Type': 'text/event-stream'}) @@ -202,6 +206,7 @@ def app(): app.router.add_post("/token", token_handler) app.router.add_post("/token_header_auth", token_header_auth_handler) app.router.add_get("/error", error_handler) + app.router.add_get("/forbidden-discovery", forbidden_discovery_handler) app.router.add_get("/flaky_events", flaky_events_handler) app["flaky"] = {"connections": 0, "last_event_ids": [], "always_drop": False} app.router.add_get("/crlf_split_events", crlf_split_events_handler) @@ -580,3 +585,14 @@ async def test_reconnect_delay_is_capped(sse_transport, aiohttp_client, app, mon assert results == [{"seq": 1}, {"seq": 2}] assert app["huge_retry"]["connections"] == 2 assert time.monotonic() - started < 3 + + +@pytest.mark.asyncio +async def test_register_manual_surfaces_server_error_body(sse_transport, aiohttp_client, app): + """A refused discovery reports the server's body in errors[], not just the status.""" + client = await aiohttp_client(app) + call_template = SseCallTemplate(name="test-sse", url=str(client.make_url("/forbidden-discovery"))) + result = await sse_transport.register_manual(None, call_template) + assert result.success is False + assert "discovery refused: tenant is not provisioned for streaming" in result.errors[0] + assert "403" in result.errors[0] diff --git a/plugins/communication_protocols/http/tests/test_streamable_http_communication_protocol.py b/plugins/communication_protocols/http/tests/test_streamable_http_communication_protocol.py index d86a44c..e026f45 100644 --- a/plugins/communication_protocols/http/tests/test_streamable_http_communication_protocol.py +++ b/plugins/communication_protocols/http/tests/test_streamable_http_communication_protocol.py @@ -109,6 +109,9 @@ async def check_oauth(request): async def error_endpoint(request): return web.Response(status=500, text="Internal Server Error") + async def forbidden_discovery(request): + return web.Response(status=403, text="discovery refused: tenant is not provisioned for streaming") + app = web.Application() app.add_routes([ web.get('/discover', discover), @@ -120,6 +123,7 @@ async def error_endpoint(request): web.post('/token', oauth_token_handler), web.post('/token-header', oauth_token_header_handler), web.get('/error', error_endpoint), + web.get('/forbidden-discovery', forbidden_discovery), ]) return app @@ -341,3 +345,14 @@ async def test_call_tool_with_oauth2_header_fallback_nonstream(streamable_http_t result = await streamable_http_transport.call_tool(None, "test_tool", {}, call_template) assert result == SAMPLE_NDJSON_RESPONSE + + +@pytest.mark.asyncio +async def test_register_manual_surfaces_server_error_body(streamable_http_transport, aiohttp_client, app): + """A refused discovery reports the server's body in errors[], not just the status.""" + client = await aiohttp_client(app) + call_template = StreamableHttpCallTemplate(name="test-provider", url=f"{client.make_url('/forbidden-discovery')}") + result = await streamable_http_transport.register_manual(None, call_template) + assert result.success is False + assert "discovery refused: tenant is not provisioned for streaming" in result.errors[0] + assert "403" in result.errors[0]