diff --git a/src/agents/mcp/server.py b/src/agents/mcp/server.py index 35cf10ace8..168a476e12 100644 --- a/src/agents/mcp/server.py +++ b/src/agents/mcp/server.py @@ -105,11 +105,10 @@ class RequireApprovalObject(TypedDict, total=False): _SAFE_EXCEPTION_MESSAGE = "An additional error occurred during the MCP request." -def _safe_transport_cause(http_error: Exception) -> Exception | None: - """Keep a transport exception only when its HTTPX URLs need no sanitization.""" - if not isinstance(http_error, httpx.HTTPStatusError | httpx.RequestError): - return http_error - +def _transport_error_urls_are_safe( + http_error: httpx.HTTPStatusError | httpx.RequestError, +) -> bool: + """Return whether one HTTPX exception contains only credential-safe URLs.""" request_urls: list[str] = [] try: request_urls.append(str(http_error.request.url)) @@ -121,7 +120,7 @@ def _safe_transport_cause(http_error: Exception) -> Exception | None: try: response_url = response.request.url except RuntimeError: - return None + return False request_urls.append(str(response_url)) redirect_location = response.headers.get("location") @@ -129,13 +128,43 @@ def _safe_transport_cause(http_error: Exception) -> Exception | None: try: request_urls.append(str(response_url.join(redirect_location))) except (httpx.InvalidURL, ValueError): - return None + return False - return http_error if all(get_mcp_server_log_name(url) == url for url in request_urls) else None + return all(get_mcp_server_log_name(url) == url for url in request_urls) + + +def _safe_transport_cause(http_error: Exception) -> Exception | None: + """Keep an unchained transport exception only when its HTTPX URLs are credential-safe.""" + if not isinstance(http_error, httpx.HTTPStatusError | httpx.RequestError): + return http_error + + if not _transport_error_urls_are_safe(http_error): + return None + if BaseException.__getattribute__(http_error, "__cause__") is not None: + return None + if BaseException.__getattribute__(http_error, "__context__") is not None: + return None + if BaseException.__getattribute__(http_error, "__dict__").get("__notes__"): + return None + + return http_error def _first_unsafe_transport_error(http_errors: list[Exception]) -> Exception | None: """Return the first transport error whose HTTPX URLs require sanitization.""" + return next( + ( + error + for error in http_errors + if isinstance(error, httpx.HTTPStatusError | httpx.RequestError) + and not _transport_error_urls_are_safe(error) + ), + None, + ) + + +def _first_unretainable_transport_error(http_errors: list[Exception]) -> Exception | None: + """Return the first transport error that cannot be retained as an exception cause.""" return next((error for error in http_errors if _safe_transport_cause(error) is None), None) @@ -184,6 +213,22 @@ def _log_transport_warning(message: str, http_error: Exception) -> None: log_tool_action_warning(logger, message, safe_error) +def _get_cleanup_transport_error_message(http_error: Exception) -> str: + """Return the cleanup warning message for an HTTPX transport failure.""" + if isinstance(http_error, httpx.HTTPStatusError): + return "HTTP error during cleanup of MCP server" + if isinstance(http_error, httpx.ConnectError): + return "Connection error during cleanup of MCP server" + if isinstance(http_error, httpx.TimeoutException): + return "Timeout error during cleanup of MCP server" + return "Request error during cleanup of MCP server" + + +def _log_cleanup_transport_warning(message: str) -> None: + """Log a fixed cleanup warning without retaining the transport exception.""" + logger.warning("%s", message, stacklevel=3) + + def _create_default_streamable_http_client( headers: dict[str, str] | None = None, timeout: httpx.Timeout | None = None, @@ -836,11 +881,45 @@ def _extract_http_errors_from_exception(self, e: BaseException) -> list[Exceptio return [] - def _user_error_for_http_error(self, http_error: Exception) -> UserError: + def _select_cleanup_transport_error(self, error: BaseException) -> Exception | None: + """Select a cleanup transport error for specialized handling.""" + unsafe_http_error = _first_unsafe_transport_error( + self._extract_http_errors_from_exception(error) + ) + if unsafe_http_error is not None: + return unsafe_http_error + + candidates = error.exceptions if isinstance(error, BaseExceptionGroup) else (error,) + for error_type in ( + httpx.HTTPStatusError, + httpx.ConnectError, + httpx.TimeoutException, + ): + selected_http_error = next( + ( + candidate + for candidate in reversed(candidates) + if isinstance(candidate, Exception) and isinstance(candidate, error_type) + ), + None, + ) + if selected_http_error is not None: + return selected_http_error + + return None + + def _user_error_for_http_error( + self, + http_error: Exception, + *, + include_http_reason_phrase: bool = True, + ) -> UserError: """Build a UserError from safe HTTP diagnostics.""" error_message = f"Failed to connect to MCP server '{self._error_name}': " if isinstance(http_error, httpx.HTTPStatusError): - error_message += f"HTTP error {http_error.response.status_code} ({http_error.response.reason_phrase})" # noqa: E501 + error_message += f"HTTP error {http_error.response.status_code}" + if include_http_reason_phrase: + error_message += f" ({http_error.response.reason_phrase})" elif isinstance(http_error, httpx.ConnectError): error_message += "Could not reach the server." @@ -935,6 +1014,8 @@ async def connect(self): connection_succeeded = False connection_error: UserError | None = None connection_cause: Exception | None = None + connection_exception: BaseException | None = None + cleanup_failure: BaseException | None = None try: transport = await self.exit_stack.enter_async_context(self.create_streams()) # streamablehttp_client returns (read, write, get_session_id) @@ -958,56 +1039,69 @@ async def connect(self): self.server_initialize_result = server_result self.session = session connection_succeeded = True - except Exception as e: - http_errors = self._extract_http_errors_from_exception(e) - if not http_errors: - raise + except BaseException as e: + if not isinstance(e, Exception): + connection_exception = e + else: + http_errors = self._extract_http_errors_from_exception(e) + if not http_errors: + connection_exception = e + else: + unsafe_http_error = _first_unretainable_transport_error(http_errors) + http_error = unsafe_http_error or http_errors[0] + connection_cause = _safe_transport_cause(http_error) + maps_safe_error = isinstance( + http_error, + httpx.HTTPStatusError | httpx.ConnectError | httpx.TimeoutException, + ) + if connection_cause is not None and not maps_safe_error: + connection_exception = e + connection_cause = None + else: + connection_error = self._user_error_for_http_error(http_error) + http_errors.clear() + del http_error + del unsafe_http_error + + # Run cleanup after leaving the connection exception handler so a cleanup UserError does + # not retain the pending connection failure as its implicit context. + if not connection_succeeded: + try: + await self.cleanup() + except UserError as e: + cleanup_failure = e + except Exception as cleanup_error: + # Suppress RuntimeError about cancel scopes during cleanup - this is a known + # issue with the MCP library's async generator cleanup and shouldn't mask the + # original error. + if isinstance(cleanup_error, RuntimeError) and "cancel scope" in str(cleanup_error): + logger.debug( + "%s", + get_mcp_server_log_message( + "Ignoring cancel scope error during cleanup of MCP server", self + ), + stacklevel=2, + ) + else: + # Log other cleanup errors but don't raise - original error is more important. + logger.warning( + "%s", + get_mcp_server_log_message("Error during cleanup of MCP server", self), + stacklevel=2, + ) + except BaseException as e: + cleanup_failure = e - unsafe_http_error = _first_unsafe_transport_error(http_errors) - http_error = unsafe_http_error or http_errors[0] - connection_cause = None if unsafe_http_error is not None else http_error - maps_safe_error = isinstance( - http_error, - httpx.HTTPStatusError | httpx.ConnectError | httpx.TimeoutException, - ) - if connection_cause is not None and not maps_safe_error: - raise + if cleanup_failure is not None: + connection_exception = None + connection_error = None + connection_cause = None + if isinstance(cleanup_failure, UserError): + self._raise_mapped_transport_error(cleanup_failure, None) + raise cleanup_failure - connection_error = self._user_error_for_http_error(http_error) - if connection_cause is None: - http_errors.clear() - del http_error - del unsafe_http_error - finally: - # Always attempt cleanup on error, but suppress cleanup errors that mask the original - if not connection_succeeded: - try: - await self.cleanup() - except UserError: - # Re-raise UserError from cleanup (contains the real HTTP error) - raise - except Exception as cleanup_error: - # Suppress RuntimeError about cancel scopes during cleanup - this is a known - # issue with the MCP library's async generator cleanup and shouldn't mask the - # original error - if isinstance(cleanup_error, RuntimeError) and "cancel scope" in str( - cleanup_error - ): - log_tool_action_debug( - logger, - get_mcp_server_log_message( - "Ignoring cancel scope error during cleanup of MCP server", self - ), - cleanup_error, - ) - else: - # Log other cleanup errors but don't raise - original error is more - # important - log_tool_action_warning( - logger, - get_mcp_server_log_message("Error during cleanup of MCP server", self), - cleanup_error, - ) + if connection_exception is not None: + raise connection_exception if connection_error is not None: self._raise_mapped_transport_error(connection_error, connection_cause) @@ -1239,7 +1333,6 @@ async def cleanup(self): # masking the original exception. is_failed_connection_cleanup = self.session is None cleanup_error: UserError | None = None - cleanup_cause: Exception | None = None try: await self.exit_stack.aclose() @@ -1250,70 +1343,60 @@ async def cleanup(self): e, ) raise - except BaseExceptionGroup as eg: - http_errors = self._extract_http_errors_from_exception(eg) - unsafe_http_error = _first_unsafe_transport_error(http_errors) - selected_http_error = unsafe_http_error - - if selected_http_error is None: - # Preserve legacy group diagnostics when HTTP errors are nested but safe. - for error_type in ( - httpx.HTTPStatusError, - httpx.ConnectError, - httpx.TimeoutException, - ): - selected_http_error = next( - ( - error - for error in reversed(eg.exceptions) - if isinstance(error, Exception) and isinstance(error, error_type) - ), - None, - ) - if selected_http_error is not None: - break - + except (BaseExceptionGroup, httpx.HTTPStatusError, httpx.RequestError) as e: + selected_http_error = self._select_cleanup_transport_error(e) if selected_http_error is not None: if is_failed_connection_cleanup: - cleanup_error = self._user_error_for_http_error(selected_http_error) - cleanup_cause = _safe_transport_cause(selected_http_error) - if cleanup_cause is None: - http_errors.clear() - del selected_http_error - del unsafe_http_error - else: - if isinstance(selected_http_error, httpx.HTTPStatusError): - cleanup_message = "HTTP error during cleanup of MCP server" - elif isinstance(selected_http_error, httpx.ConnectError): - cleanup_message = "Connection error during cleanup of MCP server" - elif isinstance(selected_http_error, httpx.TimeoutException): - cleanup_message = "Timeout error during cleanup of MCP server" - else: - cleanup_message = "Request error during cleanup of MCP server" - _log_transport_warning( - get_mcp_server_log_message(cleanup_message, self), + cleanup_error = self._user_error_for_http_error( selected_http_error, + include_http_reason_phrase=False, ) - else: - # No HTTP error found, suppress RuntimeError about cancel scopes - has_cancel_scope_error = any( - isinstance(exc, RuntimeError) and "cancel scope" in str(exc) - for exc in eg.exceptions - ) - if has_cancel_scope_error: - log_tool_action_debug( - logger, + del selected_http_error + else: + _log_cleanup_transport_warning( get_mcp_server_log_message( - "Ignoring cancel scope error during cleanup of MCP server", self - ), - eg, + _get_cleanup_transport_error_message(selected_http_error), self + ) ) - else: + elif isinstance(e, httpx.RequestError): + _log_cleanup_transport_warning( + get_mcp_server_log_message(_get_cleanup_transport_error_message(e), self) + ) + elif isinstance(e, BaseExceptionGroup): + http_errors = self._extract_http_errors_from_exception(e) + if http_errors: + safe_error_group = _credential_safe_exception_group(e) log_tool_action_error( logger, get_mcp_server_log_message("Error cleaning up MCP server", self), - eg, + safe_error_group, + ) + else: + # No HTTP error found, suppress RuntimeError about cancel scopes. + has_cancel_scope_error = any( + isinstance(exc, RuntimeError) and "cancel scope" in str(exc) + for exc in e.exceptions ) + if has_cancel_scope_error: + log_tool_action_debug( + logger, + get_mcp_server_log_message( + "Ignoring cancel scope error during cleanup of MCP server", self + ), + e, + ) + else: + log_tool_action_error( + logger, + get_mcp_server_log_message("Error cleaning up MCP server", self), + e, + ) + else: + log_tool_action_error( + logger, + get_mcp_server_log_message("Error cleaning up MCP server", self), + e, + ) except Exception as e: # Suppress RuntimeError about cancel scopes - this is a known issue with the MCP # library when background tasks fail during async generator cleanup @@ -1336,7 +1419,7 @@ async def cleanup(self): self._get_session_id = None if cleanup_error is not None: - self._raise_mapped_transport_error(cleanup_error, cleanup_cause) + self._raise_mapped_transport_error(cleanup_error, None) class MCPServerStdioParams(TypedDict): @@ -1931,9 +2014,9 @@ async def call_tool( if not http_errors: raise - unsafe_http_error = _first_unsafe_transport_error(http_errors) + unsafe_http_error = _first_unretainable_transport_error(http_errors) http_error = unsafe_http_error or http_errors[0] - transport_cause = None if unsafe_http_error is not None else http_error + transport_cause = _safe_transport_cause(http_error) if isinstance(http_error, httpx.HTTPStatusError): status_code = http_error.response.status_code transport_error = UserError( diff --git a/tests/mcp/test_server_errors.py b/tests/mcp/test_server_errors.py index b5ffc88406..fc4d3f2a56 100644 --- a/tests/mcp/test_server_errors.py +++ b/tests/mcp/test_server_errors.py @@ -126,7 +126,17 @@ def _assert_not_retained_in_log_record( continue seen.add(id(value)) - if isinstance(value, dict): + if isinstance(value, BaseException): + pending.extend(value.args) + if value.__cause__ is not None: + pending.append(value.__cause__) + if value.__context__ is not None: + pending.append(value.__context__) + pending.extend(getattr(value, "__notes__", ())) + pending.append(value.__dict__) + if isinstance(value, BaseExceptionGroup): + pending.extend(value.exceptions) + elif isinstance(value, dict): pending.extend(value.keys()) pending.extend(value.values()) elif isinstance(value, list | tuple | set | frozenset): @@ -487,6 +497,32 @@ def _mixed_request_error_group( return BaseExceptionGroup("mixed failures", [safe_error, nested_group]), safe_error, later_error +def _transport_error_with_sensitive_attachment( + attachment: str, +) -> tuple[httpx.ReadTimeout, object]: + safe_outer_error = httpx.ReadTimeout( + "outer timeout", + request=httpx.Request("GET", _SAFE_URL), + ) + if attachment == "http_context": + http_context = httpx.ReadError( + "inner read failed", + request=httpx.Request("GET", _CREDENTIALED_URL), + ) + sensitive_value: object = http_context + safe_outer_error.__context__ = http_context + elif attachment == "non_http_context": + non_http_context = ValueError(_CREDENTIALED_URL) + sensitive_value = non_http_context + safe_outer_error.__context__ = non_http_context + elif attachment == "note": + sensitive_value = _CREDENTIALED_URL + safe_outer_error.__dict__["__notes__"] = [sensitive_value] + else: + raise AssertionError(f"Unexpected attachment type: {attachment}") + return safe_outer_error, sensitive_value + + @pytest.mark.asyncio async def test_connect_checks_every_request_error_before_preserving_exception_group(): server = MCPServerSse(params={"url": _SAFE_URL}) @@ -519,6 +555,94 @@ async def test_call_tool_checks_every_request_error_before_preserving_exception_ _assert_not_retained_in_traceback_locals(exc_info.value, unsafe_error) +@pytest.mark.asyncio +async def test_connect_group_hides_sensitive_transport_error_context(): + server = MCPServerSse(params={"url": _SAFE_URL}) + transport_error, sensitive_value = _transport_error_with_sensitive_attachment( + "non_http_context" + ) + error_group = BaseExceptionGroup("connection failed", [transport_error]) + + with patch.object(server, "create_streams", side_effect=error_group): + with pytest.raises(UserError) as exc_info: + await server.connect() + + assert "Connection timeout" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, transport_error) + _assert_not_retained_in_exception_graph(exc_info.value, sensitive_value) + _assert_not_retained_in_traceback_locals(exc_info.value, error_group) + _assert_not_retained_in_traceback_locals(exc_info.value, transport_error) + _assert_not_retained_in_traceback_locals(exc_info.value, sensitive_value) + + +@pytest.mark.asyncio +async def test_call_tool_group_hides_sensitive_transport_error_context(): + server = MCPServerStreamableHttp(params={"url": _SAFE_URL}) + server.session = MagicMock() + server.max_retry_attempts = 0 + transport_error, sensitive_value = _transport_error_with_sensitive_attachment( + "non_http_context" + ) + error_group = BaseExceptionGroup("tool call failed", [transport_error]) + + with patch.object(server, "_call_tool_with_isolated_retry", side_effect=error_group): + with pytest.raises(UserError) as exc_info: + await server.call_tool("test_tool", {}) + + assert "Connection timeout" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, transport_error) + _assert_not_retained_in_exception_graph(exc_info.value, sensitive_value) + _assert_not_retained_in_traceback_locals(exc_info.value, error_group) + _assert_not_retained_in_traceback_locals(exc_info.value, transport_error) + _assert_not_retained_in_traceback_locals(exc_info.value, sensitive_value) + + +@pytest.mark.asyncio +async def test_connect_group_checks_every_transport_error_attachment(): + server = MCPServerSse(params={"url": _SAFE_URL}) + error_group, _, later_error = _mixed_request_error_group(_SAFE_URL) + sensitive_value = ValueError(_CREDENTIALED_URL) + later_error.__context__ = sensitive_value + + with patch.object(server, "create_streams", side_effect=error_group): + with pytest.raises(UserError) as exc_info: + await server.connect() + + assert "Could not reach the server" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, error_group) + _assert_not_retained_in_exception_graph(exc_info.value, later_error) + _assert_not_retained_in_exception_graph(exc_info.value, sensitive_value) + _assert_not_retained_in_traceback_locals(exc_info.value, error_group) + _assert_not_retained_in_traceback_locals(exc_info.value, later_error) + _assert_not_retained_in_traceback_locals(exc_info.value, sensitive_value) + + +@pytest.mark.asyncio +async def test_call_tool_group_checks_every_transport_error_attachment(): + server = MCPServerStreamableHttp(params={"url": _SAFE_URL}) + server.session = MagicMock() + server.max_retry_attempts = 0 + error_group, _, later_error = _mixed_request_error_group(_SAFE_URL) + sensitive_value = ValueError(_CREDENTIALED_URL) + later_error.__context__ = sensitive_value + + with patch.object(server, "_call_tool_with_isolated_retry", side_effect=error_group): + with pytest.raises(UserError) as exc_info: + await server.call_tool("test_tool", {}) + + assert "Connection lost" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, error_group) + _assert_not_retained_in_exception_graph(exc_info.value, later_error) + _assert_not_retained_in_exception_graph(exc_info.value, sensitive_value) + _assert_not_retained_in_traceback_locals(exc_info.value, error_group) + _assert_not_retained_in_traceback_locals(exc_info.value, later_error) + _assert_not_retained_in_traceback_locals(exc_info.value, sensitive_value) + + @pytest.mark.asyncio async def test_connect_preserves_exception_group_when_every_request_error_is_safe(): server = MCPServerSse(params={"url": _SAFE_URL}) @@ -810,22 +934,239 @@ async def test_call_tool_request_error_only_maps_credentialed_urls( @pytest.mark.asyncio -async def test_failed_connection_cleanup_hides_url_credentials_from_exception_graph(): +@pytest.mark.parametrize("grouped", [False, True]) +async def test_failed_connection_cleanup_hides_url_credentials_from_exception_graph( + grouped: bool, +): server = MCPServerSse(params={"url": _CREDENTIALED_URL}) request = httpx.Request("GET", _CREDENTIALED_URL) http_error = httpx.HTTPStatusError( "boom", request=request, response=httpx.Response(502, request=request) ) - cleanup_group = BaseExceptionGroup("cleanup failed", [http_error]) + cleanup_error: BaseException = http_error + if grouped: + cleanup_error = BaseExceptionGroup("cleanup failed", [http_error]) - with patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_group)): + with patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)): with pytest.raises(UserError) as exc_info: await server.cleanup() assert "mcp.example.com/sse" in str(exc_info.value) assert "HTTP error 502" in str(exc_info.value) _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, cleanup_error) + _assert_not_retained_in_exception_graph(exc_info.value, http_error) + _assert_not_retained_in_traceback_locals(exc_info.value, http_error) + + +@pytest.mark.asyncio +async def test_connect_preserves_original_error_when_cleanup_has_safe_generic_request_error( + monkeypatch, + caplog, +): + monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False) + server = MCPServerSse(params={"url": _SAFE_URL}) + connection_error = ValueError("original connection failure") + cleanup_error = httpx.ReadError( + "cleanup read failed", + request=httpx.Request("GET", _SAFE_URL), + ) + + with ( + patch.object(server, "create_streams", side_effect=connection_error), + patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), + caplog.at_level(logging.WARNING, logger="openai.agents"), + ): + with pytest.raises(ValueError) as exc_info: + await server.connect() + + assert exc_info.value is connection_error + record = caplog.records[-1] + assert record.exc_info is None + assert record.exc_text is None + _assert_not_retained_in_log_record(record, cleanup_error) + assert server.session is None + assert server._get_session_id is None + + +@pytest.mark.asyncio +async def test_connect_cleanup_mapped_error_omits_pending_connection_failure(): + server = MCPServerSse(params={"url": _SAFE_URL}) + connection_error = ValueError(_CREDENTIALED_URL) + cleanup_error = httpx.ReadTimeout( + "cleanup timed out", + request=httpx.Request("GET", _SAFE_URL), + ) + + with ( + patch.object(server, "create_streams", side_effect=connection_error), + patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), + ): + with pytest.raises(UserError) as exc_info: + await server.connect() + + assert "Connection timeout" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, connection_error) + _assert_not_retained_in_exception_graph(exc_info.value, cleanup_error) + _assert_not_retained_in_traceback_locals(exc_info.value, connection_error) + _assert_not_retained_in_traceback_locals(exc_info.value, cleanup_error) + assert server.session is None + assert server._get_session_id is None + + +@pytest.mark.asyncio +@pytest.mark.parametrize("grouped", [False, True]) +async def test_normal_cleanup_hides_generic_request_error_context_from_log_record( + monkeypatch, + caplog, + grouped: bool, +): + monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False) + server = MCPServerSse(params={"url": _SAFE_URL}) + server.session = MagicMock() + request_error = httpx.ReadError( + "cleanup read failed", + request=httpx.Request("GET", _SAFE_URL), + ) + sensitive_value = ValueError(_CREDENTIALED_URL) + request_error.__context__ = sensitive_value + cleanup_error: BaseException = request_error + if grouped: + cleanup_error = BaseExceptionGroup("cleanup failed", [request_error]) + + with ( + patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), + caplog.at_level(logging.WARNING, logger="openai.agents"), + ): + await server.cleanup() + + record = caplog.records[-1] + if grouped: + assert record.exc_info is not None + assert record.exc_info[1] is not cleanup_error + else: + assert record.exc_info is None + assert record.exc_text is None + _assert_not_retained_in_log_record(record, cleanup_error) + _assert_not_retained_in_log_record(record, request_error) + _assert_not_retained_in_log_record(record, sensitive_value) + _assert_url_credentials_hidden_from_log_record(record) + assert server.session is None + assert server._get_session_id is None + + +@pytest.mark.asyncio +@pytest.mark.parametrize("payload", ["message", "reason_phrase"]) +async def test_normal_cleanup_hides_transport_exception_payload_from_log_record( + monkeypatch, + caplog, + payload: str, +): + monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False) + server = MCPServerSse(params={"url": _SAFE_URL}) + server.session = MagicMock() + request = httpx.Request("GET", _SAFE_URL) + if payload == "message": + cleanup_error: Exception = httpx.ReadTimeout(_CREDENTIALED_URL, request=request) + else: + cleanup_error = httpx.HTTPStatusError( + "cleanup failed", + request=request, + response=httpx.Response( + 502, + request=request, + extensions={"reason_phrase": _CREDENTIALED_URL.encode()}, + ), + ) + + with ( + patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), + caplog.at_level(logging.WARNING, logger="openai.agents"), + ): + await server.cleanup() + + record = caplog.records[-1] + assert record.exc_info is None + assert record.exc_text is None + _assert_not_retained_in_log_record(record, cleanup_error) + _assert_url_credentials_hidden_from_log_record(record) + assert server.session is None + assert server._get_session_id is None + + +@pytest.mark.asyncio +@pytest.mark.parametrize("attachment", ["http_context", "non_http_context", "note"]) +async def test_failed_connection_cleanup_hides_sensitive_exception_attachments( + attachment: str, +): + server = MCPServerSse(params={"url": _SAFE_URL}) + cleanup_error, sensitive_value = _transport_error_with_sensitive_attachment(attachment) + + with patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)): + with pytest.raises(UserError) as exc_info: + await server.cleanup() + + assert "Connection timeout" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, cleanup_error) + _assert_not_retained_in_exception_graph(exc_info.value, sensitive_value) + _assert_not_retained_in_traceback_locals(exc_info.value, cleanup_error) + _assert_not_retained_in_traceback_locals(exc_info.value, sensitive_value) + assert server.session is None + assert server._get_session_id is None + + +@pytest.mark.asyncio +async def test_failed_connection_cleanup_hides_sensitive_transport_error_message(): + server = MCPServerSse(params={"url": _SAFE_URL}) + cleanup_error = httpx.ReadTimeout( + _CREDENTIALED_URL, + request=httpx.Request("GET", _SAFE_URL), + ) + + with patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)): + with pytest.raises(UserError) as exc_info: + await server.cleanup() + + assert "Connection timeout" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, cleanup_error) + _assert_not_retained_in_traceback_locals(exc_info.value, cleanup_error) + assert server.session is None + assert server._get_session_id is None + + +@pytest.mark.asyncio +@pytest.mark.parametrize("grouped", [False, True]) +async def test_failed_connection_cleanup_omits_untrusted_http_reason_phrase(grouped: bool): + server = MCPServerSse(params={"url": _SAFE_URL}) + request = httpx.Request("GET", _SAFE_URL) + http_error = httpx.HTTPStatusError( + "boom", + request=request, + response=httpx.Response( + 502, + request=request, + extensions={"reason_phrase": _CREDENTIALED_URL.encode()}, + ), + ) + cleanup_error: BaseException = http_error + if grouped: + cleanup_error = BaseExceptionGroup("cleanup failed", [http_error]) + + with patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)): + with pytest.raises(UserError) as exc_info: + await server.cleanup() + + assert "HTTP error 502" in str(exc_info.value) + _assert_url_credentials_hidden(exc_info.value) + _assert_not_retained_in_exception_graph(exc_info.value, cleanup_error) + _assert_not_retained_in_exception_graph(exc_info.value, http_error) + _assert_not_retained_in_traceback_locals(exc_info.value, cleanup_error) _assert_not_retained_in_traceback_locals(exc_info.value, http_error) + assert server.session is None + assert server._get_session_id is None @pytest.mark.asyncio @@ -847,7 +1188,7 @@ async def test_failed_connection_cleanup_checks_every_nested_transport_error(): @pytest.mark.asyncio @pytest.mark.parametrize("redacted", [True, False]) -@pytest.mark.parametrize("nested", [False, True]) +@pytest.mark.parametrize("exception_shape", ["direct", "grouped", "nested_group"]) @pytest.mark.parametrize( ("url", "safe_to_attach"), [ @@ -859,7 +1200,7 @@ async def test_normal_cleanup_only_logs_safe_transport_exceptions( monkeypatch, caplog, redacted: bool, - nested: bool, + exception_shape: str, url: str, safe_to_attach: bool, ): @@ -870,31 +1211,30 @@ async def test_normal_cleanup_only_logs_safe_transport_exceptions( "timed out", request=httpx.Request("GET", url), ) - inner_error: BaseException = timeout_error - if nested: - inner_error = BaseExceptionGroup("nested cleanup failed", [inner_error]) - cleanup_group = BaseExceptionGroup("cleanup failed", [inner_error]) + cleanup_error: BaseException = timeout_error + if exception_shape == "grouped": + cleanup_error = BaseExceptionGroup("cleanup failed", [timeout_error]) + elif exception_shape == "nested_group": + inner_group = BaseExceptionGroup("nested cleanup failed", [timeout_error]) + cleanup_error = BaseExceptionGroup("cleanup failed", [inner_group]) with ( - patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_group)), + patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), caplog.at_level(logging.WARNING, logger="openai.agents"), ): await server.cleanup() record = caplog.records[-1] - if not redacted and safe_to_attach: + if not redacted and safe_to_attach and exception_shape == "nested_group": assert record.exc_info is not None - if nested: - assert record.levelno == logging.ERROR - assert record.exc_info[1] is cleanup_group - else: - assert record.levelno == logging.WARNING - assert record.exc_info[1] is timeout_error + assert record.levelno == logging.ERROR + assert record.exc_info[1] is not cleanup_error else: assert record.exc_info is None assert record.exc_text is None - _assert_not_retained_in_log_record(record, cleanup_group) - _assert_not_retained_in_log_record(record, timeout_error) + + _assert_not_retained_in_log_record(record, cleanup_error) + _assert_not_retained_in_log_record(record, timeout_error) if not safe_to_attach: _assert_url_credentials_hidden_from_log_record(record) @@ -904,7 +1244,37 @@ async def test_normal_cleanup_only_logs_safe_transport_exceptions( @pytest.mark.asyncio -async def test_normal_cleanup_preserves_safe_nested_group_diagnostics(monkeypatch, caplog): +@pytest.mark.parametrize("redacted", [True, False]) +@pytest.mark.parametrize("attachment", ["http_context", "non_http_context", "note"]) +async def test_normal_cleanup_hides_sensitive_exception_attachments_from_log_record( + monkeypatch, + caplog, + redacted: bool, + attachment: str, +): + monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", redacted) + server = MCPServerSse(params={"url": _SAFE_URL}) + server.session = MagicMock() + cleanup_error, sensitive_value = _transport_error_with_sensitive_attachment(attachment) + + with ( + patch.object(server.exit_stack, "aclose", AsyncMock(side_effect=cleanup_error)), + caplog.at_level(logging.WARNING, logger="openai.agents"), + ): + await server.cleanup() + + record = caplog.records[-1] + assert record.exc_info is None + assert record.exc_text is None + _assert_not_retained_in_log_record(record, cleanup_error) + _assert_not_retained_in_log_record(record, sensitive_value) + _assert_url_credentials_hidden_from_log_record(record) + assert server.session is None + assert server._get_session_id is None + + +@pytest.mark.asyncio +async def test_normal_cleanup_sanitizes_safe_nested_group_diagnostics(monkeypatch, caplog): monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False) server = MCPServerSse(params={"url": _SAFE_URL}) server.session = MagicMock() @@ -912,10 +1282,11 @@ async def test_normal_cleanup_preserves_safe_nested_group_diagnostics(monkeypatc "timed out", request=httpx.Request("GET", _SAFE_URL), ) + ordinary_error = ValueError("ordinary sibling failure") cleanup_group = BaseExceptionGroup( "cleanup failed", [ - ValueError("ordinary sibling failure"), + ordinary_error, BaseExceptionGroup("nested cleanup failed", [timeout_error]), ], ) @@ -929,8 +1300,13 @@ async def test_normal_cleanup_preserves_safe_nested_group_diagnostics(monkeypatc record = caplog.records[-1] assert record.levelno == logging.ERROR assert record.exc_info is not None - assert record.exc_info[1] is cleanup_group - assert "ordinary sibling failure" in logging.Formatter().format(record) + assert record.exc_info[1] is not cleanup_group + rendered = logging.Formatter().format(record) + assert "An additional error occurred during the MCP request." in rendered + assert "ordinary sibling failure" not in rendered + _assert_not_retained_in_log_record(record, cleanup_group) + _assert_not_retained_in_log_record(record, ordinary_error) + _assert_not_retained_in_log_record(record, timeout_error) assert server.session is None assert server._get_session_id is None