diff --git a/application/mcp/rest_client.py b/application/mcp/rest_client.py index 1e9cc4b3f..9e58f2516 100644 --- a/application/mcp/rest_client.py +++ b/application/mcp/rest_client.py @@ -24,7 +24,7 @@ Set, Tuple, ) -from urllib.parse import quote, urljoin +from urllib.parse import quote, urljoin, urlsplit import requests from jsonschema import Draft7Validator @@ -189,7 +189,11 @@ def call_spec(self, tool: ToolSpec, arguments: Mapping[str, Any]) -> RestResult: # Credentials come from the environment only, never from arguments. headers, token = self._auth_headers(tool) - response = self._send(method, url, query_params, body, headers) + try: + response = self._send(method, url, query_params, body, headers) + except requests.RequestException as exc: + message = _send_failure_message(exc, self._base_url, self._timeout) + raise RestClientError(_scrub(message, token)) from exc # Never retain cookies across tool calls (no credential forwarding). if hasattr(self._session, "cookies"): try: @@ -258,6 +262,23 @@ def _scrub(text: str, token: Optional[str]) -> str: return text.replace(token, "") if token else text +def _send_failure_message( + exc: requests.RequestException, base_url: str, timeout: float +) -> str: + """Say why the request failed without leaking request details or URL secrets.""" + parts = urlsplit(base_url) + # Secrets can sit in user:password@, the query string or the fragment. + netloc = parts.netloc.rpartition("@")[2] + where = parts._replace(netloc=netloc, query="", fragment="").geturl() + reason = type(exc).__name__ + # Timeout first: ConnectTimeout is also a ConnectionError. + if isinstance(exc, requests.Timeout): + return f"OpenCRE at {where} timed out after {timeout:g}s" + if isinstance(exc, requests.ConnectionError): + return f"Could not reach OpenCRE at {where} ({reason})" + return f"Request to OpenCRE at {where} failed ({reason})" + + def flask_test_session(flask_client: Any) -> "_FlaskTestSession": """Adapt Flask's test_client to the requests Session interface.""" return _FlaskTestSession(flask_client) diff --git a/application/tests/mcp_public_tools_test.py b/application/tests/mcp_public_tools_test.py index 5daad8fd7..ce9d4a661 100644 --- a/application/tests/mcp_public_tools_test.py +++ b/application/tests/mcp_public_tools_test.py @@ -8,11 +8,13 @@ import json import os +import socket import unittest from typing import Any, Dict from unittest.mock import patch import networkx as nx +import requests from application import create_app, sqla # type: ignore from application.database import db @@ -30,6 +32,7 @@ ) from application.mcp.rest_client import ( RestClient, + RestClientError, RestRequestError, RestResponseError, _parse_response, @@ -53,6 +56,16 @@ ] +class _FailingSession: + """Stands in for requests.Session when the request gets no response.""" + + def __init__(self, error: Exception) -> None: + self.error = error + + def request(self, *args: Any, **kwargs: Any) -> Any: + raise self.error + + class McpPublicToolsTest(unittest.TestCase): def setUp(self) -> None: self.app = create_app(mode="test") @@ -340,6 +353,32 @@ def test_rest_404_is_tool_failure_not_empty_success(self) -> None: client.call_tool("get_cre_by_id", {"creid": "999-999"}) self.assertEqual(ctx.exception.status_code, 404) + def test_send_failures_say_what_went_wrong(self) -> None: + """Each failure gets its own message, and secrets in the URL never show up.""" + errors = requests.exceptions + site = "OpenCRE at http://127.0.0.1:5999" + unreachable = f"Could not reach {site}" + failed = f"Request to {site} failed" + cases = [ + (errors.ConnectionError(), f"{unreachable} (ConnectionError)"), + (errors.SSLError(), f"{unreachable} (SSLError)"), + (errors.ReadTimeout(), f"{site} timed out after 2.5s"), + # ConnectTimeout is also a ConnectionError, but it reads as a timeout. + (errors.ConnectTimeout(), f"{site} timed out after 2.5s"), + (errors.ChunkedEncodingError(), f"{failed} (ChunkedEncodingError)"), + (errors.InvalidJSONError(), f"{failed} (InvalidJSONError)"), + ] + for error, expected in cases: + with self.subTest(error=type(error).__name__): + client = RestClient( + base_url="http://alice:s3cr3t@127.0.0.1:5999?key=s3cr3t#s3cr3t", + session=_FailingSession(error), + timeout=2.5, + ) + with self.assertRaises(RestClientError) as ctx: + client.call_tool("list_standards", {}) + self.assertEqual(str(ctx.exception), expected) + def test_missing_required_input_fails(self) -> None: client = self._rest() with self.assertRaises(RestRequestError): @@ -469,6 +508,37 @@ async def _run() -> None: with self.assertLogs("application.mcp.server", level="ERROR"): asyncio.run(_run()) + def test_network_errors_are_tool_errors_not_internal_errors(self) -> None: + """MCP clients see why the call failed; the server log keeps the cause.""" + # Pick a free loopback port and close it, so the real session is refused. + with socket.socket() as probe: + probe.bind(("127.0.0.1", 0)) + base_url = f"http://127.0.0.1:{probe.getsockname()[1]}" + server = build_server(rest_client=RestClient(base_url=base_url)) + + async def _run() -> None: + call_handler = server.get_request_handler("tools/call") + assert call_handler is not None + result_raw = await call_handler.handler( + None, + types.CallToolRequestParams(name="list_standards", arguments={}), + ) + assert isinstance(result_raw, types.CallToolResult) + self.assertTrue(result_raw.is_error) + first = result_raw.content[0] + assert isinstance(first, types.TextContent) + self.assertEqual( + first.text, f"Could not reach OpenCRE at {base_url} (ConnectionError)" + ) + + import asyncio + + with self.assertLogs("application.mcp.server", level="ERROR") as logs: + asyncio.run(_run()) + logged = logs.records[0].exc_info + assert logged is not None and logged[1] is not None + self.assertIsInstance(logged[1].__cause__, requests.ConnectionError) + def test_base_url_comes_from_env_not_tool_args(self) -> None: with patch.dict(os.environ, {"OPENCRE_BASE_URL": "http://example.test:9"}): client = RestClient()