From a00a5139ad37ee2cb0f0ecbb5e2e3ee02e7cfd6a Mon Sep 17 00:00:00 2001 From: ShaanGS <254507949+ShaanGS@users.noreply.github.com> Date: Tue, 20 Oct 2026 14:40:14 +0530 Subject: [PATCH 1/2] fix(mcp): report connection errors and timeouts to MCP clients RestClient didn't catch requests exceptions, so a refused connection or a read timeout fell through to the catch-all in server.py, and MCP clients only got "Internal MCP tool error.". The real cause was only in the server's stderr. Catch them around the _send() call in call_spec() and raise RestClientError with a message that says what happened: a timeout, OpenCRE not reachable, or some other request failure, plus the base URL and the exception class. Credentials in the base URL are left out, the message goes through _scrub() like REST errors do, and the original exception stays chained so the server log still has the details. Fixes #1163. --- application/mcp/rest_client.py | 23 ++++++- application/tests/mcp_public_tools_test.py | 71 ++++++++++++++++++++++ 2 files changed, 92 insertions(+), 2 deletions(-) diff --git a/application/mcp/rest_client.py b/application/mcp/rest_client.py index 1e9cc4b3f..50db89a34 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,21 @@ 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, leaving out request details and URL credentials.""" + parts = urlsplit(base_url) + where = parts._replace(netloc=parts.netloc.rpartition("@")[2]).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..918c62520 100644 --- a/application/tests/mcp_public_tools_test.py +++ b/application/tests/mcp_public_tools_test.py @@ -13,6 +13,7 @@ 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 +31,7 @@ ) from application.mcp.rest_client import ( RestClient, + RestClientError, RestRequestError, RestResponseError, _parse_response, @@ -53,6 +55,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 +352,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 URL credentials 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", + 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 +507,39 @@ 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.""" + error = requests.ConnectionError("Connection refused") + server = build_server( + rest_client=RestClient( + base_url="http://127.0.0.1:5999", session=_FailingSession(error) + ) + ) + + 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, + "Could not reach OpenCRE at http://127.0.0.1:5999 (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.assertIs(logged[1].__cause__, error) + 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() From ea071ef48dcb9e6bfab34f45a981ef95e19dcd92 Mon Sep 17 00:00:00 2001 From: ShaanGS <254507949+ShaanGS@users.noreply.github.com> Date: Wed, 21 Oct 2026 18:05:34 +0530 Subject: [PATCH 2/2] fix(mcp): leave the query and fragment out of send errors A secret in the query string or fragment of OPENCRE_BASE_URL would still show up in the error text, so drop both along with user:password@. The path stays, because a base URL with no scheme (like "opencre.org") ends up there. The build_server test now uses a real requests.Session against a closed loopback port instead of a fake session, so it covers the real connection refusal as well as what the MCP client gets back. --- application/mcp/rest_client.py | 6 ++++-- application/tests/mcp_public_tools_test.py | 21 ++++++++++----------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/application/mcp/rest_client.py b/application/mcp/rest_client.py index 50db89a34..9e58f2516 100644 --- a/application/mcp/rest_client.py +++ b/application/mcp/rest_client.py @@ -265,9 +265,11 @@ def _scrub(text: str, token: Optional[str]) -> str: def _send_failure_message( exc: requests.RequestException, base_url: str, timeout: float ) -> str: - """Say why the request failed, leaving out request details and URL credentials.""" + """Say why the request failed without leaking request details or URL secrets.""" parts = urlsplit(base_url) - where = parts._replace(netloc=parts.netloc.rpartition("@")[2]).geturl() + # 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): diff --git a/application/tests/mcp_public_tools_test.py b/application/tests/mcp_public_tools_test.py index 918c62520..ce9d4a661 100644 --- a/application/tests/mcp_public_tools_test.py +++ b/application/tests/mcp_public_tools_test.py @@ -8,6 +8,7 @@ import json import os +import socket import unittest from typing import Any, Dict from unittest.mock import patch @@ -353,7 +354,7 @@ def test_rest_404_is_tool_failure_not_empty_success(self) -> None: self.assertEqual(ctx.exception.status_code, 404) def test_send_failures_say_what_went_wrong(self) -> None: - """Each failure gets its own message, and URL credentials never show up.""" + """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}" @@ -370,7 +371,7 @@ def test_send_failures_say_what_went_wrong(self) -> None: for error, expected in cases: with self.subTest(error=type(error).__name__): client = RestClient( - base_url="http://alice:s3cr3t@127.0.0.1:5999", + base_url="http://alice:s3cr3t@127.0.0.1:5999?key=s3cr3t#s3cr3t", session=_FailingSession(error), timeout=2.5, ) @@ -509,12 +510,11 @@ async def _run() -> None: 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.""" - error = requests.ConnectionError("Connection refused") - server = build_server( - rest_client=RestClient( - base_url="http://127.0.0.1:5999", session=_FailingSession(error) - ) - ) + # 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") @@ -528,8 +528,7 @@ async def _run() -> None: first = result_raw.content[0] assert isinstance(first, types.TextContent) self.assertEqual( - first.text, - "Could not reach OpenCRE at http://127.0.0.1:5999 (ConnectionError)", + first.text, f"Could not reach OpenCRE at {base_url} (ConnectionError)" ) import asyncio @@ -538,7 +537,7 @@ async def _run() -> None: asyncio.run(_run()) logged = logs.records[0].exc_info assert logged is not None and logged[1] is not None - self.assertIs(logged[1].__cause__, error) + 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"}):