diff --git a/backend/api/tools.py b/backend/api/tools.py index 94366ba61..b88fea79e 100644 --- a/backend/api/tools.py +++ b/backend/api/tools.py @@ -467,8 +467,8 @@ async def handler(params: Dict[str, Any]) -> Any: ) response.raise_for_status() return response.json() - except httpx.HTTPError as e: - raise ValueError(f"Webhook execution failed: {str(e)}") + except httpx.HTTPError: + raise ValueError("Webhook execution failed") return handler @@ -603,8 +603,8 @@ async def base64_decoder_handler(params: Dict[str, Any]) -> Dict[str, str]: "utf-8" ) } - except Exception as e: - raise ValueError(f"Invalid Base64 string: {e}") + except Exception: + raise ValueError("Invalid Base64 string") registry.register( diff --git a/backend/tests/test_tool_error_redaction.py b/backend/tests/test_tool_error_redaction.py new file mode 100644 index 000000000..ee85fdab2 --- /dev/null +++ b/backend/tests/test_tool_error_redaction.py @@ -0,0 +1,82 @@ +"""Regression tests for public tool failure-detail redaction.""" + +import httpx +import pytest + +from api import tools + + +class _FailingWebhookClient: + """Minimal async client that exposes a transport detail only through its exception.""" + + def __init__(self, detail: str): + self._detail = detail + + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, traceback): + return False + + async def post(self, *args, **kwargs): + raise httpx.HTTPError(self._detail) + + +@pytest.mark.asyncio +async def test_webhook_failure_response_does_not_expose_transport_detail(monkeypatch): + secret_detail = "provider detail: token=do-not-return" + monkeypatch.setattr( + tools, + "_resolve_global_addresses", + lambda *args, **kwargs: ("93.184.216.34",), + ) + monkeypatch.setattr( + tools, + "build_pinned_https_async_client", + lambda **kwargs: _FailingWebhookClient(secret_detail), + ) + + tool_code = "webhook_error_redaction_contract" + handler = tools.make_webhook_handler("https://example.com/webhook") + tools.registry.register( + tools.ToolInfo( + code=tool_code, + name="Webhook error redaction contract", + description="Test-only webhook failure contract", + category="Test", + parameters={"input": "string"}, + ), + handler, + ) + try: + response = await tools.execute_tool( + tool_code, + tools.ExecuteRequest(parameters={"input": "hello"}), + ) + finally: + tools.registry.unregister(tool_code) + + assert response.status == "failed" + assert response.result is None + assert response.message == "Webhook execution failed" + assert secret_detail not in response.message + + +@pytest.mark.asyncio +async def test_base64_failure_response_does_not_expose_decoder_detail(monkeypatch): + secret_detail = "decoder detail: source=/srv/private/input" + + def _raise_decoder_error(*args, **kwargs): + raise ValueError(secret_detail) + + monkeypatch.setattr(tools.base64, "b64decode", _raise_decoder_error) + + response = await tools.execute_tool( + "base64_decoder", + tools.ExecuteRequest(parameters={"encoded_text": "YQ=="}), + ) + + assert response.status == "failed" + assert response.result is None + assert response.message == "Invalid Base64 string" + assert secret_detail not in response.message diff --git a/backend/tests/test_tools_api.py b/backend/tests/test_tools_api.py index 50ac63c44..87fe97762 100644 --- a/backend/tests/test_tools_api.py +++ b/backend/tests/test_tools_api.py @@ -849,7 +849,7 @@ async def test_webhook_handler_http_error(): data = response.json() assert data["status"] == "failed" assert ( - "Webhook execution failed: Simulated HTTP Error" in data["message"] + "Webhook execution failed" in data["message"] ) finally: diff --git a/docs/doctoring/tool-error-redaction-boundary.md b/docs/doctoring/tool-error-redaction-boundary.md new file mode 100644 index 000000000..4b7224d44 --- /dev/null +++ b/docs/doctoring/tool-error-redaction-boundary.md @@ -0,0 +1,43 @@ +# Tool error-redaction boundary + +Date: 2026-09-16 +Status: Proposed security doctoring for PR #1696 + +## Finding + +`POST /api/tools/{code}/execute` returns handler failures through `ExecuteResponse.message`. The shared `_safe_tool_failure_message()` bounds and escapes exception text, but it does not remove credentials, provider responses, paths, connection details, or other sensitive values that may be present in the exception string. A handler that embeds a downstream exception into its own `ValueError` can therefore expose that detail to an authenticated API caller. + +The reproduced boundary is narrow. `make_webhook_handler()` previously included the `httpx.HTTPError` text in `ValueError("Webhook execution failed: ...")`; `base64_decoder_handler()` likewise propagated arbitrary decoder exception text. The repair keeps the existing common failure envelope and substitutes stable public messages at those two handlers. Regression tests inject sensitive-looking exception details and require the final `ExecuteResponse` to contain only the stable message. + +## Decision + +Do not generalize this finding into a repository-wide rule that every caught exception must be removed from logs or replaced by `exc_info=True`. + +`exc_info=True` preserves the traceback and the exception value for diagnostics. It is therefore not, by itself, a sensitive-log redaction mechanism. Log safety depends on the trust boundary, data classification, access controls, retention, and what the exception contains. Repository-wide logging changes require sink-specific evidence under the logging/security owner rather than a generated blanket rewrite. + +Likewise, internal exception types that are already converted to generic HTTP responses are not changed merely to make their internal messages shorter. The security invariant owned here is the public tool response: untrusted downstream exception text must not reach `ExecuteResponse.message` through the retained webhook and Base64 handlers. + +## Rejected alternatives + +- Logging every exception with `exc_info=True` as a universal fix: rejected because traceback output still contains exception details and can itself become a CWE-532 sink. +- Replacing `_safe_tool_failure_message()` with one global generic string: rejected because existing domain validation errors intentionally communicate bounded, useful failure reasons; a global behavior change needs a separate API-contract decision. +- Keeping broad changes to email, import, archive, SMTP, parser, embedding, worker, and LLM code in this PR: rejected because the generated sweep mixed independent boundaries and surfaced unrelated findings. Those components require their own owner evidence. + +## Verification + +`backend/tests/test_tool_error_redaction.py` exercises the API execution envelope directly. It injects downstream detail strings that resemble secrets/internal paths and requires: + +- `status == "failed"`; +- `result is None`; +- an exact stable public failure message; and +- absence of the injected detail from the returned message. + +The PR remains Draft until the final integrated head receives all live required checks and a qualifying independent review. Because PR #1695 also writes `backend/api/tools.py`, neither branch should be independently merged over the other; the later validated lane must ordinary-adopt the other owner or a verified successor without force rewrite. + +## References + +MITRE. (2026). *CWE-209: Generation of error message containing sensitive information* (CWE Version 4.20). https://cwe.mitre.org/data/definitions/209.html + +MITRE. (2026). *CWE-532: Insertion of sensitive information into log file* (CWE Version 4.20). https://cwe.mitre.org/data/definitions/532.html + +OWASP Foundation. (n.d.). *Logging cheat sheet*. OWASP Cheat Sheet Series. Retrieved September 16, 2026, from https://cheatsheetseries.owasp.org/cheatsheets/Logging_Cheat_Sheet.html