From 4cd0c8f886e3cef76f38fe3084dff850ce5463e2 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Mon, 26 Oct 2026 01:43:00 -0700 Subject: [PATCH 1/6] fix(security): return fixed public MCP JSON-RPC errors, log detail with request id (code-scanning #2-#9) --- gateway/app/routers/mcp.py | 262 ++++++++++++------ gateway/tests/test_adversarial_five_claims.py | 15 +- .../test_mcp_error_exposure_alerts_2_9.py | 198 +++++++++++++ gateway/tests/test_mcp_generator.py | 28 +- .../test_mcp_internal_error_disclosure.py | 3 +- gateway/tests/test_mcp_trust.py | 57 +++- gateway/tests/test_mcp_trust_mode.py | 8 +- gateway/tests/test_mcp_upstream_governed.py | 26 +- 8 files changed, 471 insertions(+), 126 deletions(-) create mode 100644 gateway/tests/test_mcp_error_exposure_alerts_2_9.py diff --git a/gateway/app/routers/mcp.py b/gateway/app/routers/mcp.py index 49a9727cc..4b7153376 100644 --- a/gateway/app/routers/mcp.py +++ b/gateway/app/routers/mcp.py @@ -21,6 +21,7 @@ import json import logging import math +import re import uuid from dataclasses import asdict from decimal import Decimal @@ -33,6 +34,7 @@ from ..audit.lightweight import record_audit from ..core.config import DuplicateGuardMode, get_settings +from ..core.reason_codes import REFUSAL_REASON_CODES from ..policy.jev_guard import evaluate_jev_guard from ..services.jev_guard_metadata import jev_audit_id, load_jev_guard_metadata from ..core.auth import ( @@ -407,18 +409,129 @@ def _safe_jsonrpc_id(value: Any) -> str | int | float | None: return None +INTERNAL_ERROR_MESSAGE = "internal_error" + +# Public JSON-RPC messages are canned constants. Exception text is logged +# under a request id and never copied into the client payload. +_MCP_REQUEST_ID_RE = re.compile(r"^[A-Za-z0-9._-]{1,64}$") +_JSONRPC_PUBLIC_MESSAGES: dict[int, str] = { + -32600: "Invalid Request", + -32601: "Method not found", + -32602: "Invalid params", + -32603: INTERNAL_ERROR_MESSAGE, + -32001: "Tool not found", + -32002: "Tool not executable", + -32003: "Permission denied", + -32004: "insufficient_funds", + -32005: "idempotency_in_progress", + -32006: "upstream_error", +} +_PUBLIC_REASON_MESSAGES: dict[str, str] = { + code: code + for code in ( + *REFUSAL_REASON_CODES, + "Missing tool name", + "Missing wallet_id in mcpContext", + "idempotency_key_required", + "idempotency_in_progress", + "invalid_idempotency_key", + "permit_required", + "quote_already_consumed", + "human_approval_pending", + "human_approval_unavailable", + "ledger_write_contended", + "audit_chain_head_contention", + "receipt_write_contended", + "permit_write_contended", + "tool_execution_failed", + ) +} +_PUBLIC_MESSAGE_BY_EXC_TYPE: dict[type[BaseException], str] = { + InvalidIdempotencyKeyError: "invalid_idempotency_key", + GovernedRequestInvalid: "Invalid params", + HumanApprovalPendingSignal: "human_approval_pending", + IdempotencyInProgressError: "idempotency_in_progress", + LedgerWriteContendedError: "ledger_write_contended", + AuditChainContendedError: "audit_chain_head_contention", + ReceiptWriteContendedError: "receipt_write_contended", + PermitWriteContendedError: "permit_write_contended", + ToolPermissionDenied: "Permission denied", + PermissionError: "Permission denied", + ToolExecutionError: "tool_execution_failed", +} + + +def _mcp_request_id(request: Request) -> str: + """Return a short correlation id from X-Request-ID, or a fresh uuid4 hex. + + The header is accepted only when it is short and charset-limited so a + client cannot inject log or JSON delimiters through it. + """ + raw = request.headers.get("x-request-id") + if raw is not None and _MCP_REQUEST_ID_RE.fullmatch(raw): + return raw + return uuid.uuid4().hex + + +def _public_jsonrpc_message(exc: BaseException, *, code: int) -> str: + """Canned client message for ``exc``. Never returns ``str(exc)``.""" + text = str(exc) + canned = _PUBLIC_REASON_MESSAGES.get(text) + if canned is not None: + return canned + if text.startswith("Invalid params"): + return "Invalid params" + if text.startswith("invalid_idempotency_key"): + return "invalid_idempotency_key" + if text.startswith("Tool not found"): + return "Tool not found" + if text.startswith("Tool not executable"): + return "Tool not executable" + if text.startswith("refund_failed"): + return "refund_failed" + for exc_type, message in _PUBLIC_MESSAGE_BY_EXC_TYPE.items(): + if isinstance(exc, exc_type): + return message + return _JSONRPC_PUBLIC_MESSAGES.get(code, INTERNAL_ERROR_MESSAGE) + + def _jsonrpc_error_response( request_id: str | int | float | None, code: int, message: str, data: dict[str, Any] | None = None, ) -> JSONResponse: + """Build a JSON-RPC error. ``message`` must be a canned public string.""" error_payload: dict[str, Any] = {"code": code, "message": message} if data: - error_payload["data"] = data + error_payload["data"] = dict(data) return JSONResponse({"jsonrpc": "2.0", "id": request_id, "error": error_payload}) +def _public_jsonrpc_error( + exc: BaseException, + *, + request: Request, + rpc_id: str | int | float | None, + code: int, + data: dict[str, Any] | None = None, +) -> JSONResponse: + """Log ``str(exc)`` with a request id and return a canned JSON-RPC error.""" + request_id = _mcp_request_id(request) + logger.error( + "mcp_jsonrpc_error surface=/mcp/messages request_id=%s code=%s class=%s error=%s", + request_id, + code, + type(exc).__name__, + exc, + ) + payload_data = dict(data or {}) + payload_data["request_id"] = request_id + return _jsonrpc_error_response( + rpc_id, code, _public_jsonrpc_message(exc, code=code), payload_data + ) + + class McpContext(BaseModel): """MCP execution context passed in tool calls.""" @@ -592,11 +705,17 @@ async def handle_messages( ] ) except InvalidIdempotencyKeyError as e: - return _jsonrpc_error_response( - request_id, -32602, str(e), e.as_error_data() + return _public_jsonrpc_error( + e, + request=request, + rpc_id=request_id, + code=-32602, + data=e.as_error_data(), ) except GovernedRequestInvalid as e: - return _jsonrpc_error_response(request_id, -32602, str(e)) + return _public_jsonrpc_error( + e, request=request, rpc_id=request_id, code=-32602 + ) try: result = await _handle_tools_call( call_params, @@ -616,18 +735,12 @@ async def handle_messages( } ) except HumanApprovalPendingSignal as e: - error_payload: dict[str, Any] = { - "code": e.jsonrpc_code, - "message": str(e), - } - if e.data: - error_payload["data"] = e.data - return JSONResponse( - { - "jsonrpc": "2.0", - "id": request_id, - "error": error_payload, - } + return _public_jsonrpc_error( + e, + request=request, + rpc_id=request_id, + code=e.jsonrpc_code, + data=dict(e.data) if e.data else None, ) except ( IdempotencyInProgressError, @@ -638,11 +751,12 @@ async def handle_messages( ) as e: # All five mean the same thing to a caller: nothing terminal was # recorded, retry the same idempotency key. -32005 is this - # surface's retryable code and str(e) carries which one it was, so - # a client can distinguish "a winner is mid-flight" from "the write - # lost its snapshot", "the audit chain stayed busy", "the receipt - # insert did" or "the permit write did" without any of them landing - # as internal_error. + # surface's retryable code. The public message is a canned string + # per exception class so a client can distinguish "a winner is + # mid-flight" from "the write lost its snapshot", "the audit chain + # stayed busy", "the receipt insert did" or "the permit write did" + # without any of them landing as internal_error, and without + # exception text reaching the wire. # # Only a refusal that ran nothing reaches here as # AuditChainContendedError, ReceiptWriteContendedError or @@ -655,15 +769,8 @@ async def handle_messages( # the base also carries permit_not_found, # dispatch_attempt_not_found and the budget denials, none of which # are retryable contention. - return JSONResponse( - { - "jsonrpc": "2.0", - "id": request_id, - "error": { - "code": -32005, - "message": str(e), - }, - } + return _public_jsonrpc_error( + e, request=request, rpc_id=request_id, code=-32005 ) except TerminalRecordContendedError as e: # The same contention, on the other side of the charge. Its own @@ -682,79 +789,54 @@ async def handle_messages( } ) except ToolPermissionDenied as e: - error_payload = { - "code": -32003, - "message": str(e), - } data: dict[str, Any] = {} if e.receipt: data["receipt"] = e.receipt if e.details: data["details"] = e.details - if data: - error_payload["data"] = data - return JSONResponse( - { - "jsonrpc": "2.0", - "id": request_id, - "error": error_payload, - } + return _public_jsonrpc_error( + e, + request=request, + rpc_id=request_id, + code=-32003, + data=data or None, ) except GovernedToolError as e: - error_payload = { - "code": e.jsonrpc_code, - "message": str(e), - } error_data = dict(e.extra_data) if e.receipt: error_data["receipt"] = e.receipt - if error_data: - error_payload["data"] = error_data - return JSONResponse( - { - "jsonrpc": "2.0", - "id": request_id, - "error": error_payload, - } + return _public_jsonrpc_error( + e, + request=request, + rpc_id=request_id, + code=e.jsonrpc_code, + data=error_data or None, ) except PermissionError as e: - return JSONResponse( - { - "jsonrpc": "2.0", - "id": request_id, - "error": { - "code": -32003, - "message": str(e), - }, - } + return _public_jsonrpc_error( + e, request=request, rpc_id=request_id, code=-32003 ) except ToolExecutionError as e: - # Legacy (unpermitted) tool failure: compensation is complete and, - # as on the governed path, the message is the tool's own error. - return JSONResponse( - { - "jsonrpc": "2.0", - "id": request_id, - "error": { - "code": -32603, - "message": str(e), - }, - } + # Legacy (unpermitted) tool failure: compensation is complete. + # The tool's own error stays in the server log; the client gets + # a canned message and a request id. + return _public_jsonrpc_error( + e, request=request, rpc_id=request_id, code=-32603 ) except ValueError as e: code = _value_error_jsonrpc_code(str(e)) if code is None: - error_payload = _internal_error( - e, surface="/mcp/messages", request_id=request_id + return JSONResponse( + { + "jsonrpc": "2.0", + "id": request_id, + "error": _internal_error( + e, surface="/mcp/messages", request_id=request_id + ), + } ) - else: - error_payload = {"code": code, "message": str(e)} - return JSONResponse( - { - "jsonrpc": "2.0", - "id": request_id, - "error": error_payload, - } + return _public_jsonrpc_error( + e, request=request, rpc_id=request_id, code=code ) except Exception as e: return JSONResponse( @@ -4336,10 +4418,11 @@ def _trust_metadata( def _value_error_jsonrpc_code(message: str) -> int | None: """JSON-RPC code for a ``ValueError`` the pipeline raised on purpose. - Every message here is contract: the Python SDK and existing clients match - these strings. ``None`` means the text is not one of ours — a library or - driver error that happens to subclass ``ValueError`` — and the route must - answer with ``internal_error`` instead of echoing it. + Classification inspects the exception text; the client-facing message is a + canned constant from ``_public_jsonrpc_message``. ``None`` means the text + is not one of ours — a library or driver error that happens to subclass + ``ValueError`` — and the route must answer with ``internal_error`` instead + of echoing it. """ if message in {"Missing tool name", "Missing wallet_id in mcpContext"}: return -32602 @@ -4379,9 +4462,6 @@ def _receipt_contention_after_effects() -> TerminalRecordContendedError: return TerminalRecordContendedError(RECEIPT_WRITE_CONTENDED_AFTER_EFFECTS) -INTERNAL_ERROR_MESSAGE = "internal_error" - - def _internal_error( exc: BaseException, *, surface: str, request_id: object ) -> dict[str, Any]: diff --git a/gateway/tests/test_adversarial_five_claims.py b/gateway/tests/test_adversarial_five_claims.py index 1ca62f67a..83af40026 100644 --- a/gateway/tests/test_adversarial_five_claims.py +++ b/gateway/tests/test_adversarial_five_claims.py @@ -75,6 +75,13 @@ ) from tests.test_trust_helpers import create_tool_permit, provision_agent_wallet + +def _error_without_request_id(error: dict) -> dict: + data = dict(error.get("data") or {}) + data.pop("request_id", None) + return {**error, "data": data} + + TOOL_COST = 2.0 WALLET_START = Decimal("1000") # provision_agent_wallet seeds budget_credits=1000 @@ -702,7 +709,9 @@ async def test_claim3_induced_timeout_stays_charged_and_never_redispatches( assert Decimal(str(receipt["credits_charged"])) == Decimal("2") # Replay is byte-identical and did NOT redispatch. - assert replay.json()["error"] == error + assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( + error + ) assert executor.dispatch_count == 1 assert len(executor.calls) == 1 @@ -761,7 +770,9 @@ async def test_claim3_pre_dispatch_death_refunds_to_net_zero_without_executing( assert Decimal(str(receipt["credits_charged"])) == Decimal("0") # Replay is identical and still never dispatched. - assert replay.json()["error"] == error + assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( + error + ) assert executor.dispatch_count == 0 # Debit and its refund both landed; the net effect on spend is zero. diff --git a/gateway/tests/test_mcp_error_exposure_alerts_2_9.py b/gateway/tests/test_mcp_error_exposure_alerts_2_9.py new file mode 100644 index 000000000..902c69fa0 --- /dev/null +++ b/gateway/tests/test_mcp_error_exposure_alerts_2_9.py @@ -0,0 +1,198 @@ +"""JSON-RPC /mcp/messages must not echo exception text (alerts 2-9). + +Each tools/call except branch, plus ``_jsonrpc_error_response``, used to put +``str(exc)`` on ``error.message``. Clients now get a canned public message and +a ``request_id``; the sentinel exception text is logged with that same id. +""" + +from __future__ import annotations + +import logging +import re +from collections.abc import Callable +from typing import Any + +import pytest +from httpx import ASGITransport, AsyncClient + +from app.main import app +from app.routers import mcp as mcp_router +from app.services.billing_engine import LedgerWriteContendedError +from app.services.idempotency import InvalidIdempotencyKeyError +from app.trust.adapters import GovernedRequestInvalid +from tests.test_trust_helpers import provision_agent_wallet + +LOGGER = "app.routers.mcp" +REQUEST_ID = "alert29-req-1" +SENTINEL = "ALERT29_SENTINEL_stack_trace_leak" +GENERATED_REQUEST_ID = re.compile(r"^[0-9a-f]{32}$") +MCP_CALL = { + "jsonrpc": "2.0", + "id": "alert-29", + "method": "tools/call", + "params": { + "name": "exposure-probe", + "arguments": {}, + "mcpContext": {"wallet_id": "unused", "idempotency_key": "alert-29-key"}, + }, +} + + +@pytest.fixture +async def client(): + transport = ASGITransport(app=app, raise_app_exceptions=False) + async with AsyncClient(transport=transport, base_url="http://test") as c: + yield c + + +def _patch_handle(monkeypatch: pytest.MonkeyPatch, exc: BaseException) -> None: + async def explode(*_args: Any, **_kwargs: Any) -> Any: + raise exc + + monkeypatch.setattr(mcp_router, "_handle_tools_call", explode) + + +def _patch_validate(monkeypatch: pytest.MonkeyPatch, exc: BaseException) -> None: + def explode(*_args: Any, **_kwargs: Any) -> Any: + raise exc + + monkeypatch.setattr(mcp_router, "validate_tools_call_params", explode) + + +def _patch_resolve_key(monkeypatch: pytest.MonkeyPatch, exc: BaseException) -> None: + def explode(*_args: Any, **_kwargs: Any) -> Any: + raise exc + + monkeypatch.setattr(mcp_router, "resolve_client_idempotency_key", explode) + + +BRANCHES: list[tuple[str, Callable[[pytest.MonkeyPatch], None]]] = [ + ( + "jsonrpc_error_response_invalid_key", + lambda monkeypatch: _patch_resolve_key( + monkeypatch, + InvalidIdempotencyKeyError( + "idempotency_key_blank", SENTINEL, source="test" + ), + ), + ), + ( + "jsonrpc_error_response_governed_invalid", + lambda monkeypatch: _patch_validate( + monkeypatch, GovernedRequestInvalid(SENTINEL) + ), + ), + ( + "human_approval_pending", + lambda monkeypatch: _patch_handle( + monkeypatch, + mcp_router.HumanApprovalPendingSignal( + SENTINEL, data={"approval_id": "apr-1"} + ), + ), + ), + ( + "retryable_contention", + lambda monkeypatch: _patch_handle( + monkeypatch, LedgerWriteContendedError(SENTINEL) + ), + ), + ( + "tool_permission_denied", + lambda monkeypatch: _patch_handle( + monkeypatch, + mcp_router.ToolPermissionDenied( + SENTINEL, receipt={"receipt_id": "rcpt-1"}, details={"limit": 1} + ), + ), + ), + ( + "governed_tool_error", + lambda monkeypatch: _patch_handle( + monkeypatch, + mcp_router.GovernedToolError( + SENTINEL, extra_data={"dispatch": {"status": "failed"}} + ), + ), + ), + ( + "permission_error", + lambda monkeypatch: _patch_handle(monkeypatch, PermissionError(SENTINEL)), + ), + ( + "tool_execution_error", + lambda monkeypatch: _patch_handle( + monkeypatch, mcp_router.ToolExecutionError(SENTINEL) + ), + ), + ( + "classified_value_error", + lambda monkeypatch: _patch_handle( + monkeypatch, ValueError(f"Tool not found: {SENTINEL}") + ), + ), +] + + +@pytest.mark.anyio +@pytest.mark.parametrize(("branch", "patch"), BRANCHES, ids=[row[0] for row in BRANCHES]) +async def test_tools_call_except_branch_hides_exception_text( + client, clean_database, monkeypatch, caplog, branch, patch +): + provisioned = await provision_agent_wallet(client) + patch(monkeypatch) + + with caplog.at_level(logging.ERROR, logger=LOGGER): + resp = await client.post( + "/mcp/messages", + json=MCP_CALL, + headers={**provisioned["agent_headers"], "X-Request-ID": REQUEST_ID}, + ) + + assert resp.status_code == 200, resp.text + assert SENTINEL not in resp.text + payload = resp.json() + assert "error" in payload, payload + error = payload["error"] + assert error["data"]["request_id"] == REQUEST_ID + assert SENTINEL not in error["message"] + records = [r for r in caplog.records if r.name == LOGGER] + assert records, caplog.text + matched = [ + r + for r in records + if REQUEST_ID in r.getMessage() and SENTINEL in r.getMessage() + ] + assert matched, f"{branch}: {caplog.text}" + + +@pytest.mark.anyio +async def test_malformed_request_id_header_is_replaced_with_generated_hex( + client, clean_database, monkeypatch, caplog +): + provisioned = await provision_agent_wallet(client) + _patch_handle(monkeypatch, mcp_router.ToolExecutionError(SENTINEL)) + + with caplog.at_level(logging.ERROR, logger=LOGGER): + resp = await client.post( + "/mcp/messages", + json=MCP_CALL, + headers={ + **provisioned["agent_headers"], + "X-Request-ID": "not a valid id / has spaces", + }, + ) + + assert resp.status_code == 200, resp.text + assert SENTINEL not in resp.text + request_id = resp.json()["error"]["data"]["request_id"] + assert GENERATED_REQUEST_ID.fullmatch(request_id) + assert "not a valid id" not in resp.text + matched = [ + r + for r in caplog.records + if r.name == LOGGER + and request_id in r.getMessage() + and SENTINEL in r.getMessage() + ] + assert matched, caplog.text diff --git a/gateway/tests/test_mcp_generator.py b/gateway/tests/test_mcp_generator.py index 00ccdf437..b9283051e 100644 --- a/gateway/tests/test_mcp_generator.py +++ b/gateway/tests/test_mcp_generator.py @@ -547,15 +547,19 @@ async def test_messages_tools_call_maps_missing_wallet_to_invalid_params(self): payload = response.json() assert payload["error"]["code"] == -32602 assert payload["error"]["message"] == "Missing wallet_id in mcpContext" - assert payload["error"]["data"] == { - "code": "missing_wallet_id", - "message": ( - "This call needs a wallet. Pass wallet_id from your demo key " - "response, or use a key with one wallet." - ), - "fix": "Pass wallet_id from your demo key response, or use a key with one wallet.", - "docs_url": "https://www.agentmiddleware.dev/docs/errors#missing_wallet_id", - } + data = payload["error"]["data"] + assert data["code"] == "missing_wallet_id" + assert data["message"] == ( + "This call needs a wallet. Pass wallet_id from your demo key " + "response, or use a key with one wallet." + ) + assert data["fix"] == ( + "Pass wallet_id from your demo key response, or use a key with one wallet." + ) + assert data["docs_url"] == ( + "https://www.agentmiddleware.dev/docs/errors#missing_wallet_id" + ) + assert "request_id" in data @pytest.mark.anyio async def test_messages_tools_call_maps_tool_not_found(self, clean_database): @@ -584,7 +588,7 @@ async def test_messages_tools_call_maps_tool_not_found(self, clean_database): assert response.status_code == 200 payload = response.json() assert payload["error"]["code"] == -32001 - assert payload["error"]["message"] == "Tool not found: missing-jsonrpc-tool" + assert payload["error"]["message"] == "Tool not found" @pytest.mark.anyio async def test_messages_tools_list_requires_api_key_header(self): @@ -815,7 +819,9 @@ def failing_tool() -> dict: assert response.status_code == 200 payload = response.json() assert payload["error"]["code"] == -32603 - assert "boom" in payload["error"]["message"] + assert payload["error"]["message"] == "tool_execution_failed" + assert "boom" not in payload["error"]["message"] + assert "boom" not in response.text assert ledger_resp.status_code == 200 tool_entries = [ entry diff --git a/gateway/tests/test_mcp_internal_error_disclosure.py b/gateway/tests/test_mcp_internal_error_disclosure.py index 5d29c8c4b..bec3119f7 100644 --- a/gateway/tests/test_mcp_internal_error_disclosure.py +++ b/gateway/tests/test_mcp_internal_error_disclosure.py @@ -320,9 +320,10 @@ async def failing_refund(self, **_kwargs): assert resp.status_code == 200 error = resp.json()["error"] assert error["code"] == -32603 - assert error["message"] == "refund_failed; tool_error:tool exploded" + assert error["message"] == "refund_failed" assert error["data"]["receipt"]["outcome"] == "failed_unrefunded" assert "SELECT" not in resp.text + assert "tool exploded" not in resp.text @pytest.mark.anyio diff --git a/gateway/tests/test_mcp_trust.py b/gateway/tests/test_mcp_trust.py index 71438678a..268f4b51e 100644 --- a/gateway/tests/test_mcp_trust.py +++ b/gateway/tests/test_mcp_trust.py @@ -33,6 +33,13 @@ ) +def _error_without_request_id(error: dict) -> dict: + """Replay identity ignores the per-call correlation id.""" + data = dict(error.get("data") or {}) + data.pop("request_id", None) + return {**error, "data": data} + + @pytest.fixture async def client(): transport = ASGITransport(app=app) @@ -188,11 +195,16 @@ async def test_legacy_mcp_wallet_defaults_only_when_omitted( assert response.status_code == 200 error = response.json()["error"] assert error["code"] == -32602 - assert error["message"] == ( - "Invalid params: mcpContext.wallet_id must be a string" - ) + assert error["message"] == "Invalid params" assert error["data"]["code"] == "invalid_params" - assert set(error["data"]) == {"code", "message", "fix", "docs_url"} + assert "request_id" in error["data"] + assert set(error["data"]) == { + "code", + "message", + "fix", + "docs_url", + "request_id", + } else: assert response.status_code == 422 elif wallet_case == "foreign": @@ -280,7 +292,14 @@ async def test_legacy_mcp_missing_wallet_still_rejected_without_bound_key( assert error["code"] == -32602 assert error["message"] == "Missing wallet_id in mcpContext" assert error["data"]["code"] == "missing_wallet_id" - assert set(error["data"]) == {"code", "message", "fix", "docs_url"} + assert "request_id" in error["data"] + assert set(error["data"]) == { + "code", + "message", + "fix", + "docs_url", + "request_id", + } else: response = await client.post( "/mcp/tools/anything/invoke", @@ -372,7 +391,9 @@ def frozen_guard_tool() -> dict: assert receipt["credits_charged"] == "0" assert replay.status_code == 200 - assert replay.json()["error"] == first_error + assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( + first_error + ) assert calls["count"] == 0 stored_permit = await get_permit_service().get_permit(permit["permit_id"]) @@ -476,7 +497,9 @@ def auto_freeze_guard_tool() -> dict: assert receipt["ledger_entry_id"] is None assert receipt["credits_charged"] == "0" assert replay.status_code == 200 - assert replay.json()["error"] == first_error + assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( + first_error + ) assert calls["count"] == 0 stored_permit = await get_permit_service().get_permit(permit["permit_id"]) @@ -895,7 +918,8 @@ def failing_tool() -> dict: assert resp.status_code == 200 payload = resp.json() - assert payload["error"]["message"] == "tool exploded" + assert payload["error"]["message"] == "internal_error" + assert "tool exploded" not in resp.text receipt = payload["error"]["data"]["receipt"] assert receipt["outcome"] == "failed_refunded" assert receipt["ledger_entry_id"] @@ -980,11 +1004,18 @@ def body(idempotency_key: str, request_id: str) -> dict: assert first.status_code == 200 assert first.json()["error"]["code"] == -32603 - # The reason names the tool's own error; the refund store's text stays + # The public message is canned; the tool error and refund store text stay # server-side (server log and audit event), as on the upstream path. - assert first.json()["error"]["message"] == "refund_failed; tool_error:tool exploded" + assert first.json()["error"]["message"] == "refund_failed" assert "refund down" not in first.text - assert replay.json()["error"] == first.json()["error"] + assert "tool exploded" not in first.text + first_error = first.json()["error"] + replay_error = replay.json()["error"] + assert replay_error["code"] == first_error["code"] + assert replay_error["message"] == first_error["message"] + first_data = {k: v for k, v in first_error["data"].items() if k != "request_id"} + replay_data = {k: v for k, v in replay_error["data"].items() if k != "request_id"} + assert replay_data == first_data receipt = first.json()["error"]["data"]["receipt"] assert receipt["outcome"] == "failed_unrefunded" assert receipt["ledger_entry_id"] @@ -1491,7 +1522,9 @@ def broke_guard_tool() -> dict: assert receipt["credits_charged"] == "0" assert replay.status_code == 200 - assert replay.json()["error"] == first_error + assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( + first_error + ) assert calls["count"] == 0 stored_permit = await get_permit_service().get_permit(permit["permit_id"]) diff --git a/gateway/tests/test_mcp_trust_mode.py b/gateway/tests/test_mcp_trust_mode.py index 36a802c5d..056339b47 100644 --- a/gateway/tests/test_mcp_trust_mode.py +++ b/gateway/tests/test_mcp_trust_mode.py @@ -426,10 +426,10 @@ async def test_strict_mode_replays_missing_tool_lookup_error_with_idempotency_ke assert first.status_code == 200 assert first.json()["error"]["code"] == -32001 - assert first.json()["error"]["message"] == f"Tool not found: {tool_name}" + assert first.json()["error"]["message"] == "Tool not found" assert replay.status_code == 200 assert replay.json()["error"]["code"] == -32001 - assert replay.json()["error"]["message"] == f"Tool not found: {tool_name}" + assert replay.json()["error"]["message"] == "Tool not found" assert replay.json()["error"]["message"] != "idempotency_in_progress" # docs/failure-semantics.md: an unknown tool completes a terminal record. assert ( @@ -483,10 +483,10 @@ async def test_strict_mode_replays_non_executable_tool_error_with_idempotency_ke assert first.status_code == 200 assert first.json()["error"]["code"] == -32002 - assert first.json()["error"]["message"] == f"Tool not executable: {tool_name}" + assert first.json()["error"]["message"] == "Tool not executable" assert replay.status_code == 200 assert replay.json()["error"]["code"] == -32002 - assert replay.json()["error"]["message"] == f"Tool not executable: {tool_name}" + assert replay.json()["error"]["message"] == "Tool not executable" assert replay.json()["error"]["message"] != "idempotency_in_progress" # docs/failure-semantics.md: a resolved but non-executable tool is # rejected before the governed record is begun, so nothing is recorded. diff --git a/gateway/tests/test_mcp_upstream_governed.py b/gateway/tests/test_mcp_upstream_governed.py index bb5f9fb33..4c11ee97c 100644 --- a/gateway/tests/test_mcp_upstream_governed.py +++ b/gateway/tests/test_mcp_upstream_governed.py @@ -80,6 +80,12 @@ async def client() -> AsyncClient: ] +def _error_without_request_id(error: dict) -> dict: + data = dict(error.get("data") or {}) + data.pop("request_id", None) + return {**error, "data": data} + + def _upstream_result(payload: dict[str, Any]) -> UpstreamMcpResult: canonical = json.dumps( payload, @@ -970,7 +976,9 @@ async def test_confirmed_result_rejected_by_persistence_is_terminal_and_replayab first_error = first.json()["error"] assert first_error["code"] == -32006 assert first_error["message"] == "response_rejected" - assert replay.json()["error"] == first_error + assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( + first_error + ) assert executor.dispatch_count == 1 persisted = await _load_persisted_invocation( wallet_id=provisioned["agent_wallet_id"], @@ -1047,7 +1055,9 @@ async def test_frozen_wallet_is_denied_before_upstream_dispatch_and_replays( first_error = first.json()["error"] assert first_error["code"] == -32003 assert first_error["message"] == "wallet_frozen" - assert replay.json()["error"] == first_error + assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( + first_error + ) receipt_payload = first_error["data"]["receipt"] assert receipt_payload["outcome"] == "failed_refunded" assert receipt_payload["ledger_entry_id"] is None @@ -2247,7 +2257,9 @@ async def test_confirmed_upstream_failures_refund_and_replay_without_redispatch( ) assert first.json()["error"]["message"] == expected_reason - assert replay.json()["error"] == first.json()["error"] + assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( + first.json()["error"] + ) receipt = first.json()["error"]["data"]["receipt"] assert receipt["outcome"] == "failed_refunded" assert receipt["credits_charged"] == "0" @@ -2423,7 +2435,9 @@ async def test_expired_child_wallet_denial_is_signed_released_and_replayable( assert first.status_code == 200 assert replay.status_code == 200 - assert replay.json()["error"] == first.json()["error"] + assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( + first.json()["error"] + ) error = first.json()["error"] assert error["code"] == -32003 assert error["message"] == "wallet_expired" @@ -2545,7 +2559,9 @@ async def test_ambiguous_or_rejected_upstream_response_stays_charged_and_replays first_error = first.json()["error"] assert first_error["code"] == expected_code assert first_error["message"] == expected_reason - assert replay.json()["error"] == first_error + assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( + first_error + ) receipt = first_error["data"]["receipt"] assert receipt["outcome"] == expected_state assert Decimal(receipt["credits_charged"]) == Decimal("2") From f485e3fe642c075334f8c350272a8fdd48637cf4 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Mon, 26 Oct 2026 04:03:39 -0700 Subject: [PATCH 2/6] fix(security): sanitize REST MCP invoke errors, log detail with request id (#242) Signed-off-by: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> --- gateway/app/routers/mcp.py | 128 +++++++++++++--- gateway/tests/test_adversarial_five_claims.py | 12 +- .../tests/test_cross_tenant_refusal_replay.py | 13 +- .../tests/test_delivery_uncertain_replay.py | 12 +- gateway/tests/test_jev_risk_guard.py | 6 +- .../test_mcp_error_exposure_alerts_2_9.py | 142 +++++++++++++++++- gateway/tests/test_mcp_generator.py | 6 +- .../tests/test_mcp_permit_denial_replay.py | 14 +- gateway/tests/test_mcp_trust.py | 18 +-- gateway/tests/test_mcp_upstream_governed.py | 40 ++--- .../test_permit_budget_drift_visibility.py | 7 +- .../test_permit_write_contention_surface.py | 12 +- 12 files changed, 346 insertions(+), 64 deletions(-) diff --git a/gateway/app/routers/mcp.py b/gateway/app/routers/mcp.py index 4b7153376..9a8d3c301 100644 --- a/gateway/app/routers/mcp.py +++ b/gateway/app/routers/mcp.py @@ -34,7 +34,7 @@ from ..audit.lightweight import record_audit from ..core.config import DuplicateGuardMode, get_settings -from ..core.reason_codes import REFUSAL_REASON_CODES +from ..core.reason_codes import REFUSAL_REASON_CODES, is_refusal_reason_code from ..policy.jev_guard import evaluate_jev_guard from ..services.jev_guard_metadata import jev_audit_id, load_jev_guard_metadata from ..core.auth import ( @@ -435,8 +435,14 @@ def _safe_jsonrpc_id(value: Any) -> str | int | float | None: "idempotency_key_required", "idempotency_in_progress", "invalid_idempotency_key", + "jev_risk_review_required", "permit_required", "quote_already_consumed", + "quote_expired", + "quote_not_found", + "quote_tool_mismatch", + "quote_wallet_mismatch", + "refund_reconciliation_resolution_invalid", "human_approval_pending", "human_approval_unavailable", "ledger_write_contended", @@ -474,7 +480,15 @@ def _mcp_request_id(request: Request) -> str: def _public_jsonrpc_message(exc: BaseException, *, code: int) -> str: - """Canned client message for ``exc``. Never returns ``str(exc)``.""" + """Client message for ``exc``: stable reason codes pass through, rest is canned. + + Governed denial reason codes (``delivery_uncertain``, + ``permit_forbidden_field:``, ...) are contract, not exception + text: clients match them and receipts sign them, so they reach the wire + unchanged, including a ``code: detail`` suffix. Anything else that is not + one of the fixed public strings below is replaced by a canned message and + the original text stays server-side in the log. + """ text = str(exc) canned = _PUBLIC_REASON_MESSAGES.get(text) if canned is not None: @@ -489,6 +503,8 @@ def _public_jsonrpc_message(exc: BaseException, *, code: int) -> str: return "Tool not executable" if text.startswith("refund_failed"): return "refund_failed" + if is_refusal_reason_code(text): + return text for exc_type, message in _PUBLIC_MESSAGE_BY_EXC_TYPE.items(): if isinstance(exc, exc_type): return message @@ -516,9 +532,9 @@ def _public_jsonrpc_error( code: int, data: dict[str, Any] | None = None, ) -> JSONResponse: - """Log ``str(exc)`` with a request id and return a canned JSON-RPC error.""" + """Log ``str(exc)`` with a request id and return a public JSON-RPC error.""" request_id = _mcp_request_id(request) - logger.error( + logger.exception( "mcp_jsonrpc_error surface=/mcp/messages request_id=%s code=%s class=%s error=%s", request_id, code, @@ -532,6 +548,19 @@ def _public_jsonrpc_error( ) +def _log_rest_error(exc: BaseException, *, request: Request, status_code: int) -> str: + """Log a REST invoke failure with a request id; return the id for the body.""" + request_id = _mcp_request_id(request) + logger.exception( + "mcp_rest_error surface=/mcp/tools/{service_id}/invoke request_id=%s status=%s class=%s error=%s", + request_id, + status_code, + type(exc).__name__, + exc, + ) + return request_id + + class McpContext(BaseModel): """MCP execution context passed in tool calls.""" @@ -776,7 +805,19 @@ async def handle_messages( # The same contention, on the other side of the charge. Its own # code so a client cannot read it as either neighbour: -32005 would # invite the retry that runs a paid call again, and -32603 would - # bury a deliberated state in the unclassified channel. + # bury a deliberated state in the unclassified channel. The reason + # is a stable code and stays on the wire; the request id joins it + # so the operator can find this log line from the client payload. + terminal_request_id = _mcp_request_id(request) + logger.exception( + "mcp_jsonrpc_error surface=/mcp/messages request_id=%s code=%s class=%s error=%s", + terminal_request_id, + e.jsonrpc_code, + type(e).__name__, + e, + ) + terminal_data = _terminal_record_contended_data(e.reason) + terminal_data["request_id"] = terminal_request_id return JSONResponse( { "jsonrpc": "2.0", @@ -784,7 +825,7 @@ async def handle_messages( "error": { "code": e.jsonrpc_code, "message": e.reason, - "data": _terminal_record_contended_data(e.reason), + "data": terminal_data, }, } ) @@ -4763,9 +4804,14 @@ async def invoke_tool( ] ) except InvalidIdempotencyKeyError as exc: + rest_request_id = _log_rest_error(exc, request=http_request, status_code=400) raise HTTPException( status_code=400, - detail={"message": str(exc), **exc.as_error_data()}, + detail={ + "message": "invalid_idempotency_key", + **exc.as_error_data(), + "request_id": rest_request_id, + }, ) from exc mcp_payload = { @@ -4796,7 +4842,13 @@ async def invoke_tool( response["permit_id"] = auto_permit_id return ToolCallResponse(**response) except HumanApprovalPendingSignal as exc: - detail: dict[str, Any] = {"error": str(exc)} + rest_request_id = _log_rest_error( + exc, request=http_request, status_code=exc.status_code + ) + detail: dict[str, Any] = { + "error": _public_jsonrpc_message(exc, code=exc.jsonrpc_code), + "request_id": rest_request_id, + } if exc.data: detail["approval"] = exc.data raise HTTPException(status_code=exc.status_code, detail=detail) @@ -4831,9 +4883,13 @@ async def invoke_tool( # A contention loss whose effects committed does not arrive here at # all: the five receipt sites and the audit site re-raise it as # TerminalRecordContendedError, which the next branch answers. + rest_request_id = _log_rest_error(exc, request=http_request, status_code=409) raise HTTPException( status_code=409, - detail={"error": str(exc)}, + detail={ + "error": _public_jsonrpc_message(exc, code=-32005), + "request_id": rest_request_id, + }, ) from exc except TerminalRecordContendedError as exc: # Stays 500 -- the server did fail to record the call, and this surface @@ -4841,30 +4897,57 @@ async def invoke_tool( # What changes is the body: a named reason and its remediation instead # of an opaque internal_error, matching the -32007 the JSON-RPC # surfaces return for the identical state. + rest_request_id = _log_rest_error( + exc, request=http_request, status_code=exc.status_code + ) + terminal_detail = _terminal_record_contended_data(exc.reason) + terminal_detail["request_id"] = rest_request_id raise HTTPException( status_code=exc.status_code, - detail=_terminal_record_contended_data(exc.reason), + detail=terminal_detail, ) from exc except ToolPermissionDenied as exc: - detail = {"error": str(exc)} + rest_request_id = _log_rest_error(exc, request=http_request, status_code=403) + detail = { + "error": _public_jsonrpc_message(exc, code=-32003), + "request_id": rest_request_id, + } if exc.receipt: detail["receipt"] = exc.receipt if exc.details: detail["details"] = exc.details raise HTTPException(status_code=403, detail=detail) except GovernedToolError as exc: - detail = {"error": str(exc)} + rest_request_id = _log_rest_error( + exc, request=http_request, status_code=exc.status_code + ) + detail = {"error": _public_jsonrpc_message(exc, code=exc.jsonrpc_code)} if exc.receipt: detail["receipt"] = exc.receipt detail.update(exc.extra_data) + detail["request_id"] = rest_request_id raise HTTPException(status_code=exc.status_code, detail=detail) except PermissionError as exc: - raise HTTPException(status_code=403, detail=str(exc)) + rest_request_id = _log_rest_error(exc, request=http_request, status_code=403) + permission_message = _public_jsonrpc_message(exc, code=-32003) + if permission_message == str(exc): + # A stable denial string keeps its contract shape. + raise HTTPException(status_code=403, detail=str(exc)) + raise HTTPException( + status_code=403, + detail={"error": permission_message, "request_id": rest_request_id}, + ) except ToolExecutionError as exc: - # Legacy (unpermitted) tool failure: compensation is complete and the - # message is the tool's own error, as on the governed path. + # Legacy (unpermitted) tool failure: compensation is complete. The + # tool's own error stays in the server log; the client gets the + # classified message and a request id, as on the JSON-RPC path. + rest_request_id = _log_rest_error(exc, request=http_request, status_code=200) return ToolCallResponse( - content=[{"type": "text", "text": f"Error: {exc}"}], + content=[{"type": "text", "text": "Error: tool_execution_failed"}], + structuredContent={ + "error": "tool_execution_failed", + "request_id": rest_request_id, + }, isError=True, ) except ValueError as exc: @@ -4874,15 +4957,26 @@ async def invoke_tool( exc, surface="/mcp/tools/{service_id}/invoke" ) if message == "insufficient_funds": + _log_rest_error(exc, request=http_request, status_code=402) raise HTTPException(status_code=402, detail=message) if message == "idempotency_key_reused": + _log_rest_error(exc, request=http_request, status_code=409) raise HTTPException(status_code=409, detail=message) if message in {"wallet_frozen", "wallet_expired"}: - raise HTTPException(status_code=403, detail={"error": message}) + rest_request_id = _log_rest_error( + exc, request=http_request, status_code=403 + ) + raise HTTPException( + status_code=403, + detail={"error": message, "request_id": rest_request_id}, + ) if message.startswith("Tool not found"): + _log_rest_error(exc, request=http_request, status_code=404) raise HTTPException(status_code=404, detail=message) if message.startswith("Tool not executable"): + _log_rest_error(exc, request=http_request, status_code=501) raise HTTPException(status_code=501, detail=message) + _log_rest_error(exc, request=http_request, status_code=400) raise HTTPException(status_code=400, detail=message) except Exception as exc: return _internal_error_tool_result( diff --git a/gateway/tests/test_adversarial_five_claims.py b/gateway/tests/test_adversarial_five_claims.py index 83af40026..6ca2acc8f 100644 --- a/gateway/tests/test_adversarial_five_claims.py +++ b/gateway/tests/test_adversarial_five_claims.py @@ -709,9 +709,9 @@ async def test_claim3_induced_timeout_stays_charged_and_never_redispatches( assert Decimal(str(receipt["credits_charged"])) == Decimal("2") # Replay is byte-identical and did NOT redispatch. - assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( - error - ) + assert _error_without_request_id( + replay.json()["error"] + ) == _error_without_request_id(error) assert executor.dispatch_count == 1 assert len(executor.calls) == 1 @@ -770,9 +770,9 @@ async def test_claim3_pre_dispatch_death_refunds_to_net_zero_without_executing( assert Decimal(str(receipt["credits_charged"])) == Decimal("0") # Replay is identical and still never dispatched. - assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( - error - ) + assert _error_without_request_id( + replay.json()["error"] + ) == _error_without_request_id(error) assert executor.dispatch_count == 0 # Debit and its refund both landed; the net effect on spend is zero. diff --git a/gateway/tests/test_cross_tenant_refusal_replay.py b/gateway/tests/test_cross_tenant_refusal_replay.py index 670611b89..4b02a3c5b 100644 --- a/gateway/tests/test_cross_tenant_refusal_replay.py +++ b/gateway/tests/test_cross_tenant_refusal_replay.py @@ -95,7 +95,13 @@ async def _call_twice(client, tool_name, tenant_b, mcp_context): "arguments": {"text": "one"}, "mcp_context": {**mcp_context, "idempotency_key": TOOL_CALL_ID}, } - headers = {**tenant_b["agent_headers"], "Idempotency-Key": TOOL_CALL_ID} + # A caller-provided correlation id is echoed into each refusal. Both calls + # carry the same one, so byte-identical refusals stay byte-identical. + headers = { + **tenant_b["agent_headers"], + "Idempotency-Key": TOOL_CALL_ID, + "X-Request-ID": "sel475-fixed-request-id", + } url = f"/mcp/tools/{tool_name}/invoke" first = await client.post(url, json=body, headers=headers) replay = await client.post(url, json=body, headers=headers) @@ -130,7 +136,10 @@ async def test_foreign_wallet_refusal_replays_identically( ) _assert_refusal_envelope(first, "wallet_access_denied") - assert first.json()["detail"] == {"error": "wallet_access_denied"} + assert first.json()["detail"] == { + "error": "wallet_access_denied", + "request_id": "sel475-fixed-request-id", + } # Same status, same headers that matter, same bytes. assert replay.status_code == first.status_code assert replay.headers["content-type"] == first.headers["content-type"] diff --git a/gateway/tests/test_delivery_uncertain_replay.py b/gateway/tests/test_delivery_uncertain_replay.py index dfd2e6af3..516688bd9 100644 --- a/gateway/tests/test_delivery_uncertain_replay.py +++ b/gateway/tests/test_delivery_uncertain_replay.py @@ -51,6 +51,14 @@ def _upstream_result(payload: dict[str, Any]) -> UpstreamMcpResult: ) +def _without_request_id(payload: dict[str, Any]) -> dict[str, Any]: + """Replay identity ignores the per-call correlation id.""" + error = dict(payload.get("error") or {}) + data = dict(error.get("data") or {}) + data.pop("request_id", None) + return {**payload, "error": {**error, "data": data}} + + @dataclass class AmbiguousExecutor: """Executor that always raises delivery_uncertain after the dispatch checkpoint.""" @@ -318,8 +326,8 @@ async def test_delivery_uncertain_replay_never_redispatches( assert replay.status_code == 200 replay_data = replay.json() - # Should be identical - assert replay_data == first_data + # Should be identical apart from the per-call request id. + assert _without_request_id(replay_data) == _without_request_id(first_data) # Verify NO additional dispatch assert executor.dispatch_count == 1, "Should not redispatch on replay" diff --git a/gateway/tests/test_jev_risk_guard.py b/gateway/tests/test_jev_risk_guard.py index 4b9118e89..4aec0618e 100644 --- a/gateway/tests/test_jev_risk_guard.py +++ b/gateway/tests/test_jev_risk_guard.py @@ -166,6 +166,10 @@ async def _setup( async def _invoke(client, agent, permit, key="jev-invoke", *, headers=None): + merged = dict(headers or agent["agent_headers"]) + # Refusals echo the caller correlation id. One fixed id keeps + # re-evaluated refusals byte-identical across first and replay. + merged.setdefault("X-Request-ID", "jev-fixed-request-id") return await client.post( f"/mcp/tools/{TOOL}/invoke", json={ @@ -177,7 +181,7 @@ async def _invoke(client, agent, permit, key="jev-invoke", *, headers=None): "idempotency_key": key, }, }, - headers=headers or agent["agent_headers"], + headers=merged, ) diff --git a/gateway/tests/test_mcp_error_exposure_alerts_2_9.py b/gateway/tests/test_mcp_error_exposure_alerts_2_9.py index 902c69fa0..2fceaf238 100644 --- a/gateway/tests/test_mcp_error_exposure_alerts_2_9.py +++ b/gateway/tests/test_mcp_error_exposure_alerts_2_9.py @@ -66,6 +66,13 @@ def explode(*_args: Any, **_kwargs: Any) -> Any: monkeypatch.setattr(mcp_router, "resolve_client_idempotency_key", explode) +def _patch_adapter_invoke(monkeypatch: pytest.MonkeyPatch, exc: BaseException) -> None: + async def explode(_request: Any) -> Any: + raise exc + + monkeypatch.setattr(mcp_router._mcp_adapter, "invoke", explode) + + BRANCHES: list[tuple[str, Callable[[pytest.MonkeyPatch], None]]] = [ ( "jsonrpc_error_response_invalid_key", @@ -135,7 +142,9 @@ def explode(*_args: Any, **_kwargs: Any) -> Any: @pytest.mark.anyio -@pytest.mark.parametrize(("branch", "patch"), BRANCHES, ids=[row[0] for row in BRANCHES]) +@pytest.mark.parametrize( + ("branch", "patch"), BRANCHES, ids=[row[0] for row in BRANCHES] +) async def test_tools_call_except_branch_hides_exception_text( client, clean_database, monkeypatch, caplog, branch, patch ): @@ -196,3 +205,134 @@ async def test_malformed_request_id_header_is_replaced_with_generated_hex( and SENTINEL in r.getMessage() ] assert matched, caplog.text + + +# Fixed-value regression coverage for the REST invoke route and the governed +# error path: fixed X-Request-ID headers, fixed marker strings, no randomness. +REQUEST_ID_242_A = "req-fixed-242-0001" +REQUEST_ID_242_B = "req-fixed-242-0002" +REQUEST_ID_242_C = "req-fixed-242-0003" +SENTINEL_242 = "REQ242_FIXED_SENTINEL_MUST_NOT_LEAK" +SENTINEL_242_B = "REQ242_FIXED_SENTINEL_B_MUST_NOT_LEAK" +TOOL_242 = "exposure-probe-242" + + +def _rest_body(wallet_id: str, idempotency_key: str) -> dict[str, Any]: + return { + "name": TOOL_242, + "arguments": {}, + "mcp_context": { + "wallet_id": wallet_id, + "idempotency_key": idempotency_key, + }, + } + + +@pytest.mark.anyio +async def test_rest_invoke_permission_denied_hides_exception_text( + client, clean_database, monkeypatch, caplog +): + provisioned = await provision_agent_wallet(client) + _patch_adapter_invoke( + monkeypatch, + mcp_router.ToolPermissionDenied( + SENTINEL_242, + receipt={"receipt_id": "rcpt-fixed-242-0001"}, + details={"reason_code": "fixed-242-probe"}, + ), + ) + + with caplog.at_level(logging.ERROR, logger=LOGGER): + resp = await client.post( + f"/mcp/tools/{TOOL_242}/invoke", + json=_rest_body(provisioned["agent_wallet_id"], "req-242-fixed-key-0001"), + headers={ + **provisioned["agent_headers"], + "X-Request-ID": REQUEST_ID_242_A, + }, + ) + + assert resp.status_code == 403, resp.text + assert SENTINEL_242 not in resp.text + detail = resp.json()["detail"] + assert detail["error"] == "Permission denied" + assert detail["request_id"] == REQUEST_ID_242_A + assert detail["receipt"]["receipt_id"] == "rcpt-fixed-242-0001" + assert detail["details"] == {"reason_code": "fixed-242-probe"} + matched = [ + r + for r in caplog.records + if r.name == LOGGER + and REQUEST_ID_242_A in r.getMessage() + and SENTINEL_242 in r.getMessage() + ] + assert matched, caplog.text + assert matched[0].exc_info is not None + + +@pytest.mark.anyio +async def test_rest_invoke_tool_failure_hides_exception_text( + client, clean_database, monkeypatch, caplog +): + provisioned = await provision_agent_wallet(client) + _patch_adapter_invoke(monkeypatch, mcp_router.ToolExecutionError(SENTINEL_242_B)) + + with caplog.at_level(logging.ERROR, logger=LOGGER): + resp = await client.post( + f"/mcp/tools/{TOOL_242}/invoke", + json=_rest_body(provisioned["agent_wallet_id"], "req-242-fixed-key-0002"), + headers={ + **provisioned["agent_headers"], + "X-Request-ID": REQUEST_ID_242_B, + }, + ) + + assert resp.status_code == 200, resp.text + assert SENTINEL_242_B not in resp.text + body = resp.json() + assert body["isError"] is True + assert body["content"] == [{"type": "text", "text": "Error: tool_execution_failed"}] + assert body["structuredContent"]["error"] == "tool_execution_failed" + assert body["structuredContent"]["request_id"] == REQUEST_ID_242_B + matched = [ + r + for r in caplog.records + if r.name == LOGGER + and REQUEST_ID_242_B in r.getMessage() + and SENTINEL_242_B in r.getMessage() + ] + assert matched, caplog.text + assert matched[0].exc_info is not None + + +@pytest.mark.anyio +async def test_messages_governed_tool_error_hides_exception_text_fixed_id( + client, clean_database, monkeypatch, caplog +): + provisioned = await provision_agent_wallet(client) + _patch_handle(monkeypatch, mcp_router.GovernedToolError(SENTINEL_242)) + + with caplog.at_level(logging.ERROR, logger=LOGGER): + resp = await client.post( + "/mcp/messages", + json=MCP_CALL, + headers={ + **provisioned["agent_headers"], + "X-Request-ID": REQUEST_ID_242_C, + }, + ) + + assert resp.status_code == 200, resp.text + assert SENTINEL_242 not in resp.text + error = resp.json()["error"] + assert error["message"] == "internal_error" + assert error["data"]["request_id"] == REQUEST_ID_242_C + matched = [ + r + for r in caplog.records + if r.name == LOGGER + and REQUEST_ID_242_C in r.getMessage() + and SENTINEL_242 in r.getMessage() + ] + assert matched, caplog.text + assert matched[0].exc_info is not None diff --git a/gateway/tests/test_mcp_generator.py b/gateway/tests/test_mcp_generator.py index b9283051e..41bd08155 100644 --- a/gateway/tests/test_mcp_generator.py +++ b/gateway/tests/test_mcp_generator.py @@ -958,7 +958,11 @@ def failing_tool() -> dict: payload = response.json() assert payload["isError"] is True assert payload["code"] == "tool_execution_failed" - assert "insufficient_funds" in payload["content"][0]["text"] + # The tool's own error stays server-side; the client gets the + # classified message and a request id. + assert payload["content"][0]["text"] == "Error: tool_execution_failed" + assert "insufficient_funds" not in response.text + assert "request_id" in payload["structuredContent"] finally: registry.unregister_local("http-value-error-echo") diff --git a/gateway/tests/test_mcp_permit_denial_replay.py b/gateway/tests/test_mcp_permit_denial_replay.py index b1939df92..e4967bcfc 100644 --- a/gateway/tests/test_mcp_permit_denial_replay.py +++ b/gateway/tests/test_mcp_permit_denial_replay.py @@ -51,7 +51,13 @@ async def test_foreign_permit_denial_replays_original_403( "idempotency_key": key, }, } - headers = {**tenant_b["agent_headers"], "Idempotency-Key": key} + headers = { + **tenant_b["agent_headers"], + "Idempotency-Key": key, + # Same caller correlation id on first and replay: refusals that + # echo it stay byte-identical. + "X-Request-ID": "sel482-fixed-request-id", + } path = f"/mcp/tools/{counted_tool.name}/invoke" first = await client.post(path, json=request, headers=headers) @@ -82,6 +88,7 @@ async def test_foreign_permit_denial_replays_original_403( headers={ "X-API-Key": other_key.json()["api_key"], "Idempotency-Key": key, + "X-Request-ID": "sel482-fixed-request-id-other", }, ) @@ -101,7 +108,10 @@ async def test_foreign_permit_denial_replays_original_403( assert set(replay.json()["detail"]) == set(detail) assert replay.content == first.content assert refused_other_key.status_code == 403 - assert refused_other_key.json()["detail"] == {"error": "permit_wallet_mismatch"} + assert refused_other_key.json()["detail"] == { + "error": "permit_wallet_mismatch", + "request_id": "sel482-fixed-request-id-other", + } assert tenant_a["agent_wallet_id"] not in first.text assert counted_tool.effects == [] diff --git a/gateway/tests/test_mcp_trust.py b/gateway/tests/test_mcp_trust.py index 268f4b51e..0a7497238 100644 --- a/gateway/tests/test_mcp_trust.py +++ b/gateway/tests/test_mcp_trust.py @@ -391,9 +391,9 @@ def frozen_guard_tool() -> dict: assert receipt["credits_charged"] == "0" assert replay.status_code == 200 - assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( - first_error - ) + assert _error_without_request_id( + replay.json()["error"] + ) == _error_without_request_id(first_error) assert calls["count"] == 0 stored_permit = await get_permit_service().get_permit(permit["permit_id"]) @@ -497,9 +497,9 @@ def auto_freeze_guard_tool() -> dict: assert receipt["ledger_entry_id"] is None assert receipt["credits_charged"] == "0" assert replay.status_code == 200 - assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( - first_error - ) + assert _error_without_request_id( + replay.json()["error"] + ) == _error_without_request_id(first_error) assert calls["count"] == 0 stored_permit = await get_permit_service().get_permit(permit["permit_id"]) @@ -1522,9 +1522,9 @@ def broke_guard_tool() -> dict: assert receipt["credits_charged"] == "0" assert replay.status_code == 200 - assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( - first_error - ) + assert _error_without_request_id( + replay.json()["error"] + ) == _error_without_request_id(first_error) assert calls["count"] == 0 stored_permit = await get_permit_service().get_permit(permit["permit_id"]) diff --git a/gateway/tests/test_mcp_upstream_governed.py b/gateway/tests/test_mcp_upstream_governed.py index 5e20ef4fb..824e5f52c 100644 --- a/gateway/tests/test_mcp_upstream_governed.py +++ b/gateway/tests/test_mcp_upstream_governed.py @@ -1139,9 +1139,9 @@ async def test_confirmed_result_rejected_by_persistence_is_terminal_and_replayab first_error = first.json()["error"] assert first_error["code"] == -32006 assert first_error["message"] == "response_rejected" - assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( - first_error - ) + assert _error_without_request_id( + replay.json()["error"] + ) == _error_without_request_id(first_error) assert executor.dispatch_count == 1 persisted = await _load_persisted_invocation( wallet_id=provisioned["agent_wallet_id"], @@ -1218,9 +1218,9 @@ async def test_frozen_wallet_is_denied_before_upstream_dispatch_and_replays( first_error = first.json()["error"] assert first_error["code"] == -32003 assert first_error["message"] == "wallet_frozen" - assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( - first_error - ) + assert _error_without_request_id( + replay.json()["error"] + ) == _error_without_request_id(first_error) receipt_payload = first_error["data"]["receipt"] assert receipt_payload["outcome"] == "failed_refunded" assert receipt_payload["ledger_entry_id"] is None @@ -1461,10 +1461,16 @@ async def commit_then_report_uncertain( permit_id=permit["permit_id"], idempotency_key=idempotency_key, ), - headers=provisioned["agent_headers"], + headers={ + **provisioned["agent_headers"], + "X-Request-ID": "prepare-uncertain-fixed-req", + }, ) assert uncertain.status_code == 409 - assert uncertain.json()["detail"] == {"error": "idempotency_in_progress"} + assert uncertain.json()["detail"] == { + "error": "idempotency_in_progress", + "request_id": "prepare-uncertain-fixed-req", + } assert committed_attempt_id is not None assert executor.calls == [] @@ -2420,9 +2426,9 @@ async def test_confirmed_upstream_failures_refund_and_replay_without_redispatch( ) assert first.json()["error"]["message"] == expected_reason - assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( - first.json()["error"] - ) + assert _error_without_request_id( + replay.json()["error"] + ) == _error_without_request_id(first.json()["error"]) receipt = first.json()["error"]["data"]["receipt"] assert receipt["outcome"] == "failed_refunded" assert receipt["credits_charged"] == "0" @@ -2598,9 +2604,9 @@ async def test_expired_child_wallet_denial_is_signed_released_and_replayable( assert first.status_code == 200 assert replay.status_code == 200 - assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( - first.json()["error"] - ) + assert _error_without_request_id( + replay.json()["error"] + ) == _error_without_request_id(first.json()["error"]) error = first.json()["error"] assert error["code"] == -32003 assert error["message"] == "wallet_expired" @@ -2722,9 +2728,9 @@ async def test_ambiguous_or_rejected_upstream_response_stays_charged_and_replays first_error = first.json()["error"] assert first_error["code"] == expected_code assert first_error["message"] == expected_reason - assert _error_without_request_id(replay.json()["error"]) == _error_without_request_id( - first_error - ) + assert _error_without_request_id( + replay.json()["error"] + ) == _error_without_request_id(first_error) receipt = first_error["data"]["receipt"] assert receipt["outcome"] == expected_state assert Decimal(receipt["credits_charged"]) == Decimal("2") diff --git a/gateway/tests/test_permit_budget_drift_visibility.py b/gateway/tests/test_permit_budget_drift_visibility.py index f55fbc47d..8c18ba307 100644 --- a/gateway/tests/test_permit_budget_drift_visibility.py +++ b/gateway/tests/test_permit_budget_drift_visibility.py @@ -215,9 +215,12 @@ async def test_a_lost_drift_alert_never_fails_the_call_it_reports_on( ) assert runs["count"] == 1 - # The caller's real answer survived, receipt and all. + # The caller's real answer survived, receipt and all. The + # tool's own error stays server-side; the client gets the + # classified message. detail = resp.json()["detail"] - assert "tool blew up" in detail["error"], detail + assert detail["error"] == "internal_error", detail + assert "tool blew up" not in resp.text assert detail["receipt"]["outcome"] == "failed_refunded", detail factory = get_session_factory() diff --git a/gateway/tests/test_permit_write_contention_surface.py b/gateway/tests/test_permit_write_contention_surface.py index d618c67ea..d0f33bfa9 100644 --- a/gateway/tests/test_permit_write_contention_surface.py +++ b/gateway/tests/test_permit_write_contention_surface.py @@ -520,8 +520,10 @@ async def test_a_call_that_already_ran_is_never_told_to_retry( assert PermitWriteContendedError.reason not in str(detail), detail # The caller's real answer survived the contention, and so did the receipt - # -- the whole reason the release absorbs rather than escalates. - assert "tool blew up" in detail["error"], detail + # -- the whole reason the release absorbs rather than escalates. The + # tool's own error stays server-side. + assert detail["error"] == "internal_error", detail + assert "tool blew up" not in resp.text assert detail["receipt"]["outcome"] == "failed_refunded", detail assert await _count_receipts() == 1 @@ -744,8 +746,10 @@ async def test_the_messages_surface_never_tells_a_charged_call_to_retry( error = resp.json()["error"] assert error["code"] != -32005, error assert error["message"] != PermitWriteContendedError.reason, error - # The caller's real answer and its receipt both survived. - assert "tool blew up" in error["message"], error + # The caller's real answer and its receipt both survived. The + # tool's own error stays server-side. + assert error["message"] == "internal_error", error + assert "tool blew up" not in resp.text assert await _count_receipts() == 1 From f4ce24f05ecb4648cf0af20797708840dffaaa79 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Mon, 26 Oct 2026 04:24:02 -0700 Subject: [PATCH 3/6] chore: add DCO sign-off for main merge 79dbee9 Signed-off-by: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> From cf766ea13fc3f07c5fe99e02a1dd490f313630e4 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Mon, 26 Oct 2026 05:17:22 -0700 Subject: [PATCH 4/6] fix(gateway): classify tool failures first and strip refusal suffixes in MCP errors Signed-off-by: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> --- gateway/app/routers/mcp.py | 33 ++- .../test_mcp_error_exposure_alerts_2_9.py | 202 ++++++++++++++++++ gateway/tests/test_mcp_generator.py | 5 +- 3 files changed, 233 insertions(+), 7 deletions(-) diff --git a/gateway/app/routers/mcp.py b/gateway/app/routers/mcp.py index 9a8d3c301..6af10ecbf 100644 --- a/gateway/app/routers/mcp.py +++ b/gateway/app/routers/mcp.py @@ -466,6 +466,16 @@ def _safe_jsonrpc_id(value: Any) -> str | int | float | None: ToolExecutionError: "tool_execution_failed", } +# Refusal codes whose ``code: detail`` suffix may reach the wire, and only on +# the denial path (a ToolPermissionDenied/PermissionError). ``permit_forbidden_field`` +# is the one documented producer-controlled detail: the suffix names the entry +# of the permit's own forbidden-fields list that the caller's arguments hit +# (docs/denial-details.md), and the field name already travels in the denial's +# details payload. Every other refusal code is answered bare. In particular a +# wrapped tool failure must never smuggle arbitrary tool output onto the wire +# by mimicking a ``code: detail`` prefix. +_REFUSAL_DETAIL_ALLOWLIST: frozenset[str] = frozenset({"permit_forbidden_field"}) + def _mcp_request_id(request: Request) -> str: """Return a short correlation id from X-Request-ID, or a fresh uuid4 hex. @@ -483,12 +493,20 @@ def _public_jsonrpc_message(exc: BaseException, *, code: int) -> str: """Client message for ``exc``: stable reason codes pass through, rest is canned. Governed denial reason codes (``delivery_uncertain``, - ``permit_forbidden_field:``, ...) are contract, not exception - text: clients match them and receipts sign them, so they reach the wire - unchanged, including a ``code: detail`` suffix. Anything else that is not - one of the fixed public strings below is replaced by a canned message and - the original text stays server-side in the log. + ``permit_forbidden_field``, ...) are contract, not exception text: + clients match them and receipts sign them. A ``code: detail`` suffix + reaches the wire only for a code in ``_REFUSAL_DETAIL_ALLOWLIST`` carried + by a denial exception; every other refusal code is answered bare (text + before the first colon), so a wrapped tool failure can neither select the + denial contract nor smuggle arbitrary tool output through it. Anything + else that is not one of the fixed public strings below is replaced by a + canned message and the original text stays server-side in the log. """ + if isinstance(exc, ToolExecutionError): + # A wrapped tool failure, classified by type before any text matching: + # the tool's own text may mimic a refusal code or a denial prefix, and + # must never select the message. + return "tool_execution_failed" text = str(exc) canned = _PUBLIC_REASON_MESSAGES.get(text) if canned is not None: @@ -504,7 +522,10 @@ def _public_jsonrpc_message(exc: BaseException, *, code: int) -> str: if text.startswith("refund_failed"): return "refund_failed" if is_refusal_reason_code(text): - return text + bare_code = text.split(":", 1)[0].strip() + if bare_code in _REFUSAL_DETAIL_ALLOWLIST and isinstance(exc, PermissionError): + return text + return bare_code for exc_type, message in _PUBLIC_MESSAGE_BY_EXC_TYPE.items(): if isinstance(exc, exc_type): return message diff --git a/gateway/tests/test_mcp_error_exposure_alerts_2_9.py b/gateway/tests/test_mcp_error_exposure_alerts_2_9.py index 2fceaf238..1be76d3e8 100644 --- a/gateway/tests/test_mcp_error_exposure_alerts_2_9.py +++ b/gateway/tests/test_mcp_error_exposure_alerts_2_9.py @@ -212,6 +212,11 @@ async def test_malformed_request_id_header_is_replaced_with_generated_hex( REQUEST_ID_242_A = "req-fixed-242-0001" REQUEST_ID_242_B = "req-fixed-242-0002" REQUEST_ID_242_C = "req-fixed-242-0003" +REQUEST_ID_242_D = "req-fixed-242-0004" +REQUEST_ID_242_E = "req-fixed-242-0005" +REQUEST_ID_242_F = "req-fixed-242-0006" +REQUEST_ID_242_G = "req-fixed-242-0007" +REQUEST_ID_242_H = "req-fixed-242-0008" SENTINEL_242 = "REQ242_FIXED_SENTINEL_MUST_NOT_LEAK" SENTINEL_242_B = "REQ242_FIXED_SENTINEL_B_MUST_NOT_LEAK" TOOL_242 = "exposure-probe-242" @@ -336,3 +341,200 @@ async def test_messages_governed_tool_error_hides_exception_text_fixed_id( ] assert matched, caplog.text assert matched[0].exc_info is not None + + +# Fixed-value coverage for the refusal-code sanitizer: a wrapped tool failure +# is classified by type before any text matching, a refusal suffix is stripped +# to the bare code unless allowlisted on the denial path, and the fixed +# quote/refund denial reasons pass through unchanged. + + +@pytest.mark.anyio +async def test_messages_tool_execution_error_mimicking_refusal_is_classified( + client, clean_database, monkeypatch, caplog +): + provisioned = await provision_agent_wallet(client) + _patch_handle( + monkeypatch, + mcp_router.ToolExecutionError(f"wallet_frozen: {SENTINEL_242}"), + ) + + with caplog.at_level(logging.ERROR, logger=LOGGER): + resp = await client.post( + "/mcp/messages", + json=MCP_CALL, + headers={ + **provisioned["agent_headers"], + "X-Request-ID": REQUEST_ID_242_D, + }, + ) + + assert resp.status_code == 200, resp.text + assert SENTINEL_242 not in resp.text + error = resp.json()["error"] + assert error["message"] == "tool_execution_failed" + assert error["data"]["request_id"] == REQUEST_ID_242_D + matched = [ + r + for r in caplog.records + if r.name == LOGGER + and REQUEST_ID_242_D in r.getMessage() + and SENTINEL_242 in r.getMessage() + ] + assert matched, caplog.text + assert matched[0].exc_info is not None + + +@pytest.mark.anyio +async def test_messages_refusal_suffix_is_stripped_to_bare_code( + client, clean_database, monkeypatch, caplog +): + provisioned = await provision_agent_wallet(client) + _patch_handle( + monkeypatch, + mcp_router.ToolPermissionDenied(f"wallet_frozen: {SENTINEL_242}"), + ) + + with caplog.at_level(logging.ERROR, logger=LOGGER): + resp = await client.post( + "/mcp/messages", + json=MCP_CALL, + headers={ + **provisioned["agent_headers"], + "X-Request-ID": REQUEST_ID_242_E, + }, + ) + + assert resp.status_code == 200, resp.text + assert SENTINEL_242 not in resp.text + error = resp.json()["error"] + assert error["message"] == "wallet_frozen" + assert error["data"]["request_id"] == REQUEST_ID_242_E + matched = [ + r + for r in caplog.records + if r.name == LOGGER + and REQUEST_ID_242_E in r.getMessage() + and SENTINEL_242 in r.getMessage() + ] + assert matched, caplog.text + + +@pytest.mark.anyio +async def test_messages_allowlisted_forbidden_field_detail_passes_through( + client, clean_database, monkeypatch, caplog +): + provisioned = await provision_agent_wallet(client) + _patch_handle( + monkeypatch, + mcp_router.ToolPermissionDenied("permit_forbidden_field:secret_token"), + ) + + with caplog.at_level(logging.ERROR, logger=LOGGER): + resp = await client.post( + "/mcp/messages", + json=MCP_CALL, + headers={ + **provisioned["agent_headers"], + "X-Request-ID": REQUEST_ID_242_F, + }, + ) + + assert resp.status_code == 200, resp.text + error = resp.json()["error"] + assert error["message"] == "permit_forbidden_field:secret_token" + assert error["data"]["request_id"] == REQUEST_ID_242_F + matched = [ + r + for r in caplog.records + if r.name == LOGGER and REQUEST_ID_242_F in r.getMessage() + ] + assert matched, caplog.text + + +@pytest.mark.anyio +async def test_messages_wrapped_tool_mimic_of_allowlisted_code_is_bare( + client, clean_database, monkeypatch, caplog +): + provisioned = await provision_agent_wallet(client) + _patch_handle( + monkeypatch, + mcp_router.GovernedToolError(f"permit_forbidden_field:{SENTINEL_242}"), + ) + + with caplog.at_level(logging.ERROR, logger=LOGGER): + resp = await client.post( + "/mcp/messages", + json=MCP_CALL, + headers={ + **provisioned["agent_headers"], + "X-Request-ID": REQUEST_ID_242_G, + }, + ) + + assert resp.status_code == 200, resp.text + assert SENTINEL_242 not in resp.text + error = resp.json()["error"] + assert error["message"] == "permit_forbidden_field" + assert error["data"]["request_id"] == REQUEST_ID_242_G + matched = [ + r + for r in caplog.records + if r.name == LOGGER + and REQUEST_ID_242_G in r.getMessage() + and SENTINEL_242 in r.getMessage() + ] + assert matched, caplog.text + + +def _permission_error(reason: str) -> PermissionError: + return PermissionError(reason) + + +def _governed_error(reason: str) -> mcp_router.GovernedToolError: + return mcp_router.GovernedToolError(reason) + + +FIXED_DENIAL_REASONS: list[tuple[Callable[[str], BaseException], str]] = [ + (_permission_error, "quote_expired"), + (_permission_error, "quote_not_found"), + (_permission_error, "quote_wallet_mismatch"), + (_permission_error, "quote_tool_mismatch"), + (_governed_error, "refund_reconciliation_resolution_invalid"), +] + + +@pytest.mark.anyio +@pytest.mark.parametrize( + ("make_exc", "reason"), + FIXED_DENIAL_REASONS, + ids=[row[1] for row in FIXED_DENIAL_REASONS], +) +async def test_messages_fixed_denial_reasons_are_preserved( + client, clean_database, monkeypatch, caplog, make_exc, reason +): + provisioned = await provision_agent_wallet(client) + _patch_handle(monkeypatch, make_exc(reason)) + + with caplog.at_level(logging.ERROR, logger=LOGGER): + resp = await client.post( + "/mcp/messages", + json=MCP_CALL, + headers={ + **provisioned["agent_headers"], + "X-Request-ID": REQUEST_ID_242_H, + }, + ) + + assert resp.status_code == 200, resp.text + error = resp.json()["error"] + assert error["message"] == reason + assert error["data"]["request_id"] == REQUEST_ID_242_H + matched = [ + r + for r in caplog.records + if r.name == LOGGER + and REQUEST_ID_242_H in r.getMessage() + and reason in r.getMessage() + ] + assert matched, caplog.text diff --git a/gateway/tests/test_mcp_generator.py b/gateway/tests/test_mcp_generator.py index 41bd08155..403a56f0c 100644 --- a/gateway/tests/test_mcp_generator.py +++ b/gateway/tests/test_mcp_generator.py @@ -894,7 +894,10 @@ def failing_tool() -> dict: assert response.status_code == 200 payload = response.json() assert payload["error"]["code"] == -32603 - assert "insufficient_funds" in payload["error"]["message"] + # The tool's own text mimics a refusal code; the wrapped failure + # is classified by type, so the mimic must not select the message. + assert payload["error"]["message"] == "tool_execution_failed" + assert "insufficient_funds" not in response.text assert ledger_resp.status_code == 200 tool_entries = [ entry From ee98dfef6aac1bdaa17557c2bcc08ebc335d03a2 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Mon, 26 Oct 2026 06:45:56 -0700 Subject: [PATCH 5/6] fix(gateway): allow quote_invalid reason and repr-format MCP error logs (CWE-117) Signed-off-by: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> --- gateway/app/routers/mcp.py | 7 ++++--- gateway/tests/test_mcp_error_exposure_alerts_2_9.py | 1 + 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/gateway/app/routers/mcp.py b/gateway/app/routers/mcp.py index 6af10ecbf..323d1f3d9 100644 --- a/gateway/app/routers/mcp.py +++ b/gateway/app/routers/mcp.py @@ -439,6 +439,7 @@ def _safe_jsonrpc_id(value: Any) -> str | int | float | None: "permit_required", "quote_already_consumed", "quote_expired", + "quote_invalid", "quote_not_found", "quote_tool_mismatch", "quote_wallet_mismatch", @@ -556,7 +557,7 @@ def _public_jsonrpc_error( """Log ``str(exc)`` with a request id and return a public JSON-RPC error.""" request_id = _mcp_request_id(request) logger.exception( - "mcp_jsonrpc_error surface=/mcp/messages request_id=%s code=%s class=%s error=%s", + "mcp_jsonrpc_error surface=/mcp/messages request_id=%s code=%s class=%s error=%r", request_id, code, type(exc).__name__, @@ -573,7 +574,7 @@ def _log_rest_error(exc: BaseException, *, request: Request, status_code: int) - """Log a REST invoke failure with a request id; return the id for the body.""" request_id = _mcp_request_id(request) logger.exception( - "mcp_rest_error surface=/mcp/tools/{service_id}/invoke request_id=%s status=%s class=%s error=%s", + "mcp_rest_error surface=/mcp/tools/{service_id}/invoke request_id=%s status=%s class=%s error=%r", request_id, status_code, type(exc).__name__, @@ -831,7 +832,7 @@ async def handle_messages( # so the operator can find this log line from the client payload. terminal_request_id = _mcp_request_id(request) logger.exception( - "mcp_jsonrpc_error surface=/mcp/messages request_id=%s code=%s class=%s error=%s", + "mcp_jsonrpc_error surface=/mcp/messages request_id=%s code=%s class=%s error=%r", terminal_request_id, e.jsonrpc_code, type(e).__name__, diff --git a/gateway/tests/test_mcp_error_exposure_alerts_2_9.py b/gateway/tests/test_mcp_error_exposure_alerts_2_9.py index 1be76d3e8..1daa02408 100644 --- a/gateway/tests/test_mcp_error_exposure_alerts_2_9.py +++ b/gateway/tests/test_mcp_error_exposure_alerts_2_9.py @@ -497,6 +497,7 @@ def _governed_error(reason: str) -> mcp_router.GovernedToolError: FIXED_DENIAL_REASONS: list[tuple[Callable[[str], BaseException], str]] = [ (_permission_error, "quote_expired"), + (_permission_error, "quote_invalid"), (_permission_error, "quote_not_found"), (_permission_error, "quote_wallet_mismatch"), (_permission_error, "quote_tool_mismatch"), From e83256963297d4812f67100233092c3864099b1a Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Mon, 26 Oct 2026 07:28:12 -0700 Subject: [PATCH 6/6] fix(gateway): sanitize #242 error logs and stop raw error text reaching clients Replace logger.exception with logger.error (request id, exception type, sanitized capped %r message, no traceback) to close CWE-117. Return fixed messages for invoke_tool ValueError paths and permit_forbidden_field. Generate test request ids at runtime. Signed-off-by: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> --- gateway/app/routers/mcp.py | 134 ++++++-- .../test_mcp_error_exposure_alerts_2_9.py | 300 +++++++++++++----- .../test_mcp_idempotency_key_validation.py | 3 +- gateway/tests/test_mcp_upstream_governed.py | 3 +- gateway/tests/test_permit_v2_constraints.py | 4 +- gateway/tests/test_receipts.py | 3 +- 6 files changed, 341 insertions(+), 106 deletions(-) diff --git a/gateway/app/routers/mcp.py b/gateway/app/routers/mcp.py index 323d1f3d9..a41448acb 100644 --- a/gateway/app/routers/mcp.py +++ b/gateway/app/routers/mcp.py @@ -22,6 +22,7 @@ import logging import math import re +import unicodedata import uuid from dataclasses import asdict from decimal import Decimal @@ -467,14 +468,15 @@ def _safe_jsonrpc_id(value: Any) -> str | int | float | None: ToolExecutionError: "tool_execution_failed", } -# Refusal codes whose ``code: detail`` suffix may reach the wire, and only on -# the denial path (a ToolPermissionDenied/PermissionError). ``permit_forbidden_field`` +# Refusal codes whose ``code: detail`` suffix is recognized on the denial +# path (a ToolPermissionDenied/PermissionError). ``permit_forbidden_field`` # is the one documented producer-controlled detail: the suffix names the entry # of the permit's own forbidden-fields list that the caller's arguments hit # (docs/denial-details.md), and the field name already travels in the denial's -# details payload. Every other refusal code is answered bare. In particular a -# wrapped tool failure must never smuggle arbitrary tool output onto the wire -# by mimicking a ``code: detail`` prefix. +# details payload and the server log. The wire always gets the bare reason +# code; the suffix never reaches the client. In particular a wrapped tool +# failure must never smuggle arbitrary tool output onto the wire by mimicking +# a ``code: detail`` prefix. _REFUSAL_DETAIL_ALLOWLIST: frozenset[str] = frozenset({"permit_forbidden_field"}) @@ -495,13 +497,14 @@ def _public_jsonrpc_message(exc: BaseException, *, code: int) -> str: Governed denial reason codes (``delivery_uncertain``, ``permit_forbidden_field``, ...) are contract, not exception text: - clients match them and receipts sign them. A ``code: detail`` suffix - reaches the wire only for a code in ``_REFUSAL_DETAIL_ALLOWLIST`` carried - by a denial exception; every other refusal code is answered bare (text - before the first colon), so a wrapped tool failure can neither select the - denial contract nor smuggle arbitrary tool output through it. Anything - else that is not one of the fixed public strings below is replaced by a - canned message and the original text stays server-side in the log. + clients match them and receipts sign them. Every refusal code is + answered bare (text before the first colon); a ``code: detail`` suffix + never reaches the wire, even for a code in + ``_REFUSAL_DETAIL_ALLOWLIST`` carried by a denial exception, so a + wrapped tool failure can neither select the denial contract nor smuggle + arbitrary tool output through it. Anything else that is not one of the + fixed public strings below is replaced by a canned message and the + original text stays server-side in the log. """ if isinstance(exc, ToolExecutionError): # A wrapped tool failure, classified by type before any text matching: @@ -525,7 +528,8 @@ def _public_jsonrpc_message(exc: BaseException, *, code: int) -> str: if is_refusal_reason_code(text): bare_code = text.split(":", 1)[0].strip() if bare_code in _REFUSAL_DETAIL_ALLOWLIST and isinstance(exc, PermissionError): - return text + # Allowlisted detail stays server-side; the wire gets the fixed code. + return bare_code return bare_code for exc_type, message in _PUBLIC_MESSAGE_BY_EXC_TYPE.items(): if isinstance(exc, exc_type): @@ -546,6 +550,41 @@ def _jsonrpc_error_response( return JSONResponse({"jsonrpc": "2.0", "id": request_id, "error": error_payload}) +def _sanitize_log_message(value: object, limit: int = 500) -> str: + """Make exception text safe for a single log line, capped at ``limit`` chars. + + Every ``Cc`` control character (which includes CR and LF) plus U+2028 and + U+2029 is replaced with an escaped form or a space so a crafted message + cannot inject log lines or break line-oriented collectors. Over-long + messages are truncated with a ``...[truncated]`` suffix. + """ + text = str(value) + parts: list[str] = [] + for char in text: + if char == "\n": + parts.append("\\n") + elif char == "\r": + parts.append("\\r") + elif char == "\t": + parts.append("\\t") + elif char in ("\u2028", "\u2029"): + parts.append(" ") + elif unicodedata.category(char) == "Cc": + code_point = ord(char) + if code_point < 0x100: + parts.append(f"\\x{code_point:02x}") + elif code_point < 0x10000: + parts.append(f"\\u{code_point:04x}") + else: + parts.append(f"\\U{code_point:08x}") + else: + parts.append(char) + sanitized = "".join(parts) + if len(sanitized) > limit: + return sanitized[:limit] + "...[truncated]" + return sanitized + + def _public_jsonrpc_error( exc: BaseException, *, @@ -556,12 +595,12 @@ def _public_jsonrpc_error( ) -> JSONResponse: """Log ``str(exc)`` with a request id and return a public JSON-RPC error.""" request_id = _mcp_request_id(request) - logger.exception( + logger.error( "mcp_jsonrpc_error surface=/mcp/messages request_id=%s code=%s class=%s error=%r", request_id, code, type(exc).__name__, - exc, + _sanitize_log_message(exc), ) payload_data = dict(data or {}) payload_data["request_id"] = request_id @@ -573,12 +612,12 @@ def _public_jsonrpc_error( def _log_rest_error(exc: BaseException, *, request: Request, status_code: int) -> str: """Log a REST invoke failure with a request id; return the id for the body.""" request_id = _mcp_request_id(request) - logger.exception( + logger.error( "mcp_rest_error surface=/mcp/tools/{service_id}/invoke request_id=%s status=%s class=%s error=%r", request_id, status_code, type(exc).__name__, - exc, + _sanitize_log_message(exc), ) return request_id @@ -831,12 +870,12 @@ async def handle_messages( # is a stable code and stays on the wire; the request id joins it # so the operator can find this log line from the client payload. terminal_request_id = _mcp_request_id(request) - logger.exception( + logger.error( "mcp_jsonrpc_error surface=/mcp/messages request_id=%s code=%s class=%s error=%r", terminal_request_id, e.jsonrpc_code, type(e).__name__, - e, + _sanitize_log_message(e), ) terminal_data = _terminal_record_contended_data(e.reason) terminal_data["request_id"] = terminal_request_id @@ -4979,11 +5018,27 @@ async def invoke_tool( exc, surface="/mcp/tools/{service_id}/invoke" ) if message == "insufficient_funds": - _log_rest_error(exc, request=http_request, status_code=402) - raise HTTPException(status_code=402, detail=message) + rest_request_id = _log_rest_error( + exc, request=http_request, status_code=402 + ) + raise HTTPException( + status_code=402, + detail={ + "error": "insufficient_funds", + "request_id": rest_request_id, + }, + ) if message == "idempotency_key_reused": - _log_rest_error(exc, request=http_request, status_code=409) - raise HTTPException(status_code=409, detail=message) + rest_request_id = _log_rest_error( + exc, request=http_request, status_code=409 + ) + raise HTTPException( + status_code=409, + detail={ + "error": "idempotency_key_reused", + "request_id": rest_request_id, + }, + ) if message in {"wallet_frozen", "wallet_expired"}: rest_request_id = _log_rest_error( exc, request=http_request, status_code=403 @@ -4993,13 +5048,34 @@ async def invoke_tool( detail={"error": message, "request_id": rest_request_id}, ) if message.startswith("Tool not found"): - _log_rest_error(exc, request=http_request, status_code=404) - raise HTTPException(status_code=404, detail=message) + rest_request_id = _log_rest_error( + exc, request=http_request, status_code=404 + ) + raise HTTPException( + status_code=404, + detail={"error": "Tool not found", "request_id": rest_request_id}, + ) if message.startswith("Tool not executable"): - _log_rest_error(exc, request=http_request, status_code=501) - raise HTTPException(status_code=501, detail=message) - _log_rest_error(exc, request=http_request, status_code=400) - raise HTTPException(status_code=400, detail=message) + rest_request_id = _log_rest_error( + exc, request=http_request, status_code=501 + ) + raise HTTPException( + status_code=501, + detail={ + "error": "Tool not executable", + "request_id": rest_request_id, + }, + ) + rest_request_id = _log_rest_error(exc, request=http_request, status_code=400) + raise HTTPException( + status_code=400, + detail={ + "error": _public_jsonrpc_message( + exc, code=_value_error_jsonrpc_code(message) or -32602 + ), + "request_id": rest_request_id, + }, + ) except Exception as exc: return _internal_error_tool_result( exc, surface="/mcp/tools/{service_id}/invoke" diff --git a/gateway/tests/test_mcp_error_exposure_alerts_2_9.py b/gateway/tests/test_mcp_error_exposure_alerts_2_9.py index 1daa02408..a59a782cb 100644 --- a/gateway/tests/test_mcp_error_exposure_alerts_2_9.py +++ b/gateway/tests/test_mcp_error_exposure_alerts_2_9.py @@ -9,6 +9,7 @@ import logging import re +import uuid from collections.abc import Callable from typing import Any @@ -141,6 +142,19 @@ async def explode(_request: Any) -> Any: ] +BRANCH_EXC_TYPE_NAMES: dict[str, str] = { + "jsonrpc_error_response_invalid_key": "InvalidIdempotencyKeyError", + "jsonrpc_error_response_governed_invalid": "GovernedRequestInvalid", + "human_approval_pending": "HumanApprovalPendingSignal", + "retryable_contention": "LedgerWriteContendedError", + "tool_permission_denied": "ToolPermissionDenied", + "governed_tool_error": "GovernedToolError", + "permission_error": "PermissionError", + "tool_execution_error": "ToolExecutionError", + "classified_value_error": "ValueError", +} + + @pytest.mark.anyio @pytest.mark.parametrize( ("branch", "patch"), BRANCHES, ids=[row[0] for row in BRANCHES] @@ -173,6 +187,10 @@ async def test_tools_call_except_branch_hides_exception_text( if REQUEST_ID in r.getMessage() and SENTINEL in r.getMessage() ] assert matched, f"{branch}: {caplog.text}" + assert matched[0].exc_info is None + assert BRANCH_EXC_TYPE_NAMES[branch] in matched[0].getMessage() + assert "\n" not in matched[0].getMessage() + assert "\r" not in matched[0].getMessage() @pytest.mark.anyio @@ -205,23 +223,40 @@ async def test_malformed_request_id_header_is_replaced_with_generated_hex( and SENTINEL in r.getMessage() ] assert matched, caplog.text + assert matched[0].exc_info is None + assert "ToolExecutionError" in matched[0].getMessage() + assert "\n" not in matched[0].getMessage() + assert "\r" not in matched[0].getMessage() -# Fixed-value regression coverage for the REST invoke route and the governed -# error path: fixed X-Request-ID headers, fixed marker strings, no randomness. -REQUEST_ID_242_A = "req-fixed-242-0001" -REQUEST_ID_242_B = "req-fixed-242-0002" -REQUEST_ID_242_C = "req-fixed-242-0003" -REQUEST_ID_242_D = "req-fixed-242-0004" -REQUEST_ID_242_E = "req-fixed-242-0005" -REQUEST_ID_242_F = "req-fixed-242-0006" -REQUEST_ID_242_G = "req-fixed-242-0007" -REQUEST_ID_242_H = "req-fixed-242-0008" -SENTINEL_242 = "REQ242_FIXED_SENTINEL_MUST_NOT_LEAK" -SENTINEL_242_B = "REQ242_FIXED_SENTINEL_B_MUST_NOT_LEAK" +# Regression coverage for the REST invoke route and the governed error path. +# Request ids, marker strings, and idempotency keys are generated with uuid4 +# at runtime; each test asserts against the values it generated. TOOL_242 = "exposure-probe-242" +def _fresh_request_id() -> str: + return uuid.uuid4().hex + + +def _fresh_sentinel(tag: str = "REQ242") -> str: + return f"{tag}_{uuid.uuid4().hex}_MUST_NOT_LEAK" + + +def _fresh_idempotency_key(tag: str = "req-242") -> str: + return f"{tag}-{uuid.uuid4().hex}" + + +def _assert_clean_error_log(record: logging.LogRecord, *, exc_type: str) -> str: + """Shared log-record checks: no traceback, typed, single-line.""" + message = record.getMessage() + assert record.exc_info is None + assert exc_type in message + assert "\n" not in message + assert "\r" not in message + return message + + def _rest_body(wallet_id: str, idempotency_key: str) -> dict[str, Any]: return { "name": TOOL_242, @@ -238,11 +273,14 @@ async def test_rest_invoke_permission_denied_hides_exception_text( client, clean_database, monkeypatch, caplog ): provisioned = await provision_agent_wallet(client) + request_id = _fresh_request_id() + sentinel = _fresh_sentinel() + receipt_id = f"rcpt-{uuid.uuid4().hex}" _patch_adapter_invoke( monkeypatch, mcp_router.ToolPermissionDenied( - SENTINEL_242, - receipt={"receipt_id": "rcpt-fixed-242-0001"}, + sentinel, + receipt={"receipt_id": receipt_id}, details={"reason_code": "fixed-242-probe"}, ), ) @@ -250,29 +288,29 @@ async def test_rest_invoke_permission_denied_hides_exception_text( with caplog.at_level(logging.ERROR, logger=LOGGER): resp = await client.post( f"/mcp/tools/{TOOL_242}/invoke", - json=_rest_body(provisioned["agent_wallet_id"], "req-242-fixed-key-0001"), + json=_rest_body(provisioned["agent_wallet_id"], _fresh_idempotency_key()), headers={ **provisioned["agent_headers"], - "X-Request-ID": REQUEST_ID_242_A, + "X-Request-ID": request_id, }, ) assert resp.status_code == 403, resp.text - assert SENTINEL_242 not in resp.text + assert sentinel not in resp.text detail = resp.json()["detail"] assert detail["error"] == "Permission denied" - assert detail["request_id"] == REQUEST_ID_242_A - assert detail["receipt"]["receipt_id"] == "rcpt-fixed-242-0001" + assert detail["request_id"] == request_id + assert detail["receipt"]["receipt_id"] == receipt_id assert detail["details"] == {"reason_code": "fixed-242-probe"} matched = [ r for r in caplog.records if r.name == LOGGER - and REQUEST_ID_242_A in r.getMessage() - and SENTINEL_242 in r.getMessage() + and request_id in r.getMessage() + and sentinel in r.getMessage() ] assert matched, caplog.text - assert matched[0].exc_info is not None + _assert_clean_error_log(matched[0], exc_type="ToolPermissionDenied") @pytest.mark.anyio @@ -280,42 +318,46 @@ async def test_rest_invoke_tool_failure_hides_exception_text( client, clean_database, monkeypatch, caplog ): provisioned = await provision_agent_wallet(client) - _patch_adapter_invoke(monkeypatch, mcp_router.ToolExecutionError(SENTINEL_242_B)) + request_id = _fresh_request_id() + sentinel = _fresh_sentinel(tag="REQ242B") + _patch_adapter_invoke(monkeypatch, mcp_router.ToolExecutionError(sentinel)) with caplog.at_level(logging.ERROR, logger=LOGGER): resp = await client.post( f"/mcp/tools/{TOOL_242}/invoke", - json=_rest_body(provisioned["agent_wallet_id"], "req-242-fixed-key-0002"), + json=_rest_body(provisioned["agent_wallet_id"], _fresh_idempotency_key()), headers={ **provisioned["agent_headers"], - "X-Request-ID": REQUEST_ID_242_B, + "X-Request-ID": request_id, }, ) assert resp.status_code == 200, resp.text - assert SENTINEL_242_B not in resp.text + assert sentinel not in resp.text body = resp.json() assert body["isError"] is True assert body["content"] == [{"type": "text", "text": "Error: tool_execution_failed"}] assert body["structuredContent"]["error"] == "tool_execution_failed" - assert body["structuredContent"]["request_id"] == REQUEST_ID_242_B + assert body["structuredContent"]["request_id"] == request_id matched = [ r for r in caplog.records if r.name == LOGGER - and REQUEST_ID_242_B in r.getMessage() - and SENTINEL_242_B in r.getMessage() + and request_id in r.getMessage() + and sentinel in r.getMessage() ] assert matched, caplog.text - assert matched[0].exc_info is not None + _assert_clean_error_log(matched[0], exc_type="ToolExecutionError") @pytest.mark.anyio -async def test_messages_governed_tool_error_hides_exception_text_fixed_id( +async def test_messages_governed_tool_error_hides_exception_text( client, clean_database, monkeypatch, caplog ): provisioned = await provision_agent_wallet(client) - _patch_handle(monkeypatch, mcp_router.GovernedToolError(SENTINEL_242)) + request_id = _fresh_request_id() + sentinel = _fresh_sentinel() + _patch_handle(monkeypatch, mcp_router.GovernedToolError(sentinel)) with caplog.at_level(logging.ERROR, logger=LOGGER): resp = await client.post( @@ -323,30 +365,30 @@ async def test_messages_governed_tool_error_hides_exception_text_fixed_id( json=MCP_CALL, headers={ **provisioned["agent_headers"], - "X-Request-ID": REQUEST_ID_242_C, + "X-Request-ID": request_id, }, ) assert resp.status_code == 200, resp.text - assert SENTINEL_242 not in resp.text + assert sentinel not in resp.text error = resp.json()["error"] assert error["message"] == "internal_error" - assert error["data"]["request_id"] == REQUEST_ID_242_C + assert error["data"]["request_id"] == request_id matched = [ r for r in caplog.records if r.name == LOGGER - and REQUEST_ID_242_C in r.getMessage() - and SENTINEL_242 in r.getMessage() + and request_id in r.getMessage() + and sentinel in r.getMessage() ] assert matched, caplog.text - assert matched[0].exc_info is not None + _assert_clean_error_log(matched[0], exc_type="GovernedToolError") -# Fixed-value coverage for the refusal-code sanitizer: a wrapped tool failure -# is classified by type before any text matching, a refusal suffix is stripped -# to the bare code unless allowlisted on the denial path, and the fixed -# quote/refund denial reasons pass through unchanged. +# Coverage for the refusal-code sanitizer: a wrapped tool failure is +# classified by type before any text matching, every refusal suffix is +# stripped to the bare code, and the fixed quote/refund denial reasons pass +# through unchanged. @pytest.mark.anyio @@ -354,9 +396,11 @@ async def test_messages_tool_execution_error_mimicking_refusal_is_classified( client, clean_database, monkeypatch, caplog ): provisioned = await provision_agent_wallet(client) + request_id = _fresh_request_id() + sentinel = _fresh_sentinel() _patch_handle( monkeypatch, - mcp_router.ToolExecutionError(f"wallet_frozen: {SENTINEL_242}"), + mcp_router.ToolExecutionError(f"wallet_frozen: {sentinel}"), ) with caplog.at_level(logging.ERROR, logger=LOGGER): @@ -365,24 +409,24 @@ async def test_messages_tool_execution_error_mimicking_refusal_is_classified( json=MCP_CALL, headers={ **provisioned["agent_headers"], - "X-Request-ID": REQUEST_ID_242_D, + "X-Request-ID": request_id, }, ) assert resp.status_code == 200, resp.text - assert SENTINEL_242 not in resp.text + assert sentinel not in resp.text error = resp.json()["error"] assert error["message"] == "tool_execution_failed" - assert error["data"]["request_id"] == REQUEST_ID_242_D + assert error["data"]["request_id"] == request_id matched = [ r for r in caplog.records if r.name == LOGGER - and REQUEST_ID_242_D in r.getMessage() - and SENTINEL_242 in r.getMessage() + and request_id in r.getMessage() + and sentinel in r.getMessage() ] assert matched, caplog.text - assert matched[0].exc_info is not None + _assert_clean_error_log(matched[0], exc_type="ToolExecutionError") @pytest.mark.anyio @@ -390,9 +434,11 @@ async def test_messages_refusal_suffix_is_stripped_to_bare_code( client, clean_database, monkeypatch, caplog ): provisioned = await provision_agent_wallet(client) + request_id = _fresh_request_id() + sentinel = _fresh_sentinel() _patch_handle( monkeypatch, - mcp_router.ToolPermissionDenied(f"wallet_frozen: {SENTINEL_242}"), + mcp_router.ToolPermissionDenied(f"wallet_frozen: {sentinel}"), ) with caplog.at_level(logging.ERROR, logger=LOGGER): @@ -401,33 +447,36 @@ async def test_messages_refusal_suffix_is_stripped_to_bare_code( json=MCP_CALL, headers={ **provisioned["agent_headers"], - "X-Request-ID": REQUEST_ID_242_E, + "X-Request-ID": request_id, }, ) assert resp.status_code == 200, resp.text - assert SENTINEL_242 not in resp.text + assert sentinel not in resp.text error = resp.json()["error"] assert error["message"] == "wallet_frozen" - assert error["data"]["request_id"] == REQUEST_ID_242_E + assert error["data"]["request_id"] == request_id matched = [ r for r in caplog.records if r.name == LOGGER - and REQUEST_ID_242_E in r.getMessage() - and SENTINEL_242 in r.getMessage() + and request_id in r.getMessage() + and sentinel in r.getMessage() ] assert matched, caplog.text + _assert_clean_error_log(matched[0], exc_type="ToolPermissionDenied") @pytest.mark.anyio -async def test_messages_allowlisted_forbidden_field_detail_passes_through( +async def test_messages_allowlisted_forbidden_field_detail_is_stripped_to_bare_code( client, clean_database, monkeypatch, caplog ): provisioned = await provision_agent_wallet(client) + request_id = _fresh_request_id() + field_name = f"field_{uuid.uuid4().hex}" _patch_handle( monkeypatch, - mcp_router.ToolPermissionDenied("permit_forbidden_field:secret_token"), + mcp_router.ToolPermissionDenied(f"permit_forbidden_field:{field_name}"), ) with caplog.at_level(logging.ERROR, logger=LOGGER): @@ -436,20 +485,21 @@ async def test_messages_allowlisted_forbidden_field_detail_passes_through( json=MCP_CALL, headers={ **provisioned["agent_headers"], - "X-Request-ID": REQUEST_ID_242_F, + "X-Request-ID": request_id, }, ) assert resp.status_code == 200, resp.text + assert field_name not in resp.text error = resp.json()["error"] - assert error["message"] == "permit_forbidden_field:secret_token" - assert error["data"]["request_id"] == REQUEST_ID_242_F + assert error["message"] == "permit_forbidden_field" + assert error["data"]["request_id"] == request_id matched = [ - r - for r in caplog.records - if r.name == LOGGER and REQUEST_ID_242_F in r.getMessage() + r for r in caplog.records if r.name == LOGGER and request_id in r.getMessage() ] assert matched, caplog.text + message = _assert_clean_error_log(matched[0], exc_type="ToolPermissionDenied") + assert field_name in message @pytest.mark.anyio @@ -457,9 +507,11 @@ async def test_messages_wrapped_tool_mimic_of_allowlisted_code_is_bare( client, clean_database, monkeypatch, caplog ): provisioned = await provision_agent_wallet(client) + request_id = _fresh_request_id() + sentinel = _fresh_sentinel() _patch_handle( monkeypatch, - mcp_router.GovernedToolError(f"permit_forbidden_field:{SENTINEL_242}"), + mcp_router.GovernedToolError(f"permit_forbidden_field:{sentinel}"), ) with caplog.at_level(logging.ERROR, logger=LOGGER): @@ -468,23 +520,24 @@ async def test_messages_wrapped_tool_mimic_of_allowlisted_code_is_bare( json=MCP_CALL, headers={ **provisioned["agent_headers"], - "X-Request-ID": REQUEST_ID_242_G, + "X-Request-ID": request_id, }, ) assert resp.status_code == 200, resp.text - assert SENTINEL_242 not in resp.text + assert sentinel not in resp.text error = resp.json()["error"] assert error["message"] == "permit_forbidden_field" - assert error["data"]["request_id"] == REQUEST_ID_242_G + assert error["data"]["request_id"] == request_id matched = [ r for r in caplog.records if r.name == LOGGER - and REQUEST_ID_242_G in r.getMessage() - and SENTINEL_242 in r.getMessage() + and request_id in r.getMessage() + and sentinel in r.getMessage() ] assert matched, caplog.text + _assert_clean_error_log(matched[0], exc_type="GovernedToolError") def _permission_error(reason: str) -> PermissionError: @@ -515,6 +568,7 @@ async def test_messages_fixed_denial_reasons_are_preserved( client, clean_database, monkeypatch, caplog, make_exc, reason ): provisioned = await provision_agent_wallet(client) + request_id = _fresh_request_id() _patch_handle(monkeypatch, make_exc(reason)) with caplog.at_level(logging.ERROR, logger=LOGGER): @@ -523,19 +577,121 @@ async def test_messages_fixed_denial_reasons_are_preserved( json=MCP_CALL, headers={ **provisioned["agent_headers"], - "X-Request-ID": REQUEST_ID_242_H, + "X-Request-ID": request_id, }, ) assert resp.status_code == 200, resp.text error = resp.json()["error"] assert error["message"] == reason - assert error["data"]["request_id"] == REQUEST_ID_242_H + assert error["data"]["request_id"] == request_id matched = [ r for r in caplog.records if r.name == LOGGER - and REQUEST_ID_242_H in r.getMessage() + and request_id in r.getMessage() and reason in r.getMessage() ] assert matched, caplog.text + _assert_clean_error_log(matched[0], exc_type=type(make_exc(reason)).__name__) + + +def test_sanitize_log_message_escapes_controls_and_caps() -> None: + sanitize = mcp_router._sanitize_log_message + assert sanitize("plain") == "plain" + assert sanitize("a\nb\rc\td") == "a\\nb\\rc\\td" + assert sanitize("a\x00b\x1bb") == "a\\x00b\\x1bb" + assert sanitize("a\u2028b\u2029c") == "a b c" + long_text = "A" * 1000 + capped = sanitize(long_text) + assert capped == "A" * 500 + "...[truncated]" + assert len(capped) == 514 + short = sanitize("B" * 500) + assert short == "B" * 500 + assert "...[truncated]" not in short + + +@pytest.mark.anyio +async def test_messages_log_injection_is_escaped_capped_and_typed( + client, clean_database, monkeypatch, caplog +): + """CR/LF/control bytes in exception text must not reach the log raw.""" + provisioned = await provision_agent_wallet(client) + request_id = _fresh_request_id() + core = _fresh_sentinel(tag="INJECT") + payload = core + "\nINJECTED_LINE\r\nTAIL\x00\x1b\u2028" + "P" * 1000 + _patch_handle(monkeypatch, mcp_router.ToolExecutionError(payload)) + + with caplog.at_level(logging.ERROR, logger=LOGGER): + resp = await client.post( + "/mcp/messages", + json=MCP_CALL, + headers={ + **provisioned["agent_headers"], + "X-Request-ID": request_id, + }, + ) + + assert resp.status_code == 200, resp.text + assert core not in resp.text + assert "INJECTED_LINE" not in resp.text + error = resp.json()["error"] + assert error["message"] == "tool_execution_failed" + assert error["data"]["request_id"] == request_id + matched = [ + r for r in caplog.records if r.name == LOGGER and request_id in r.getMessage() + ] + assert matched, caplog.text + message = _assert_clean_error_log(matched[0], exc_type="ToolExecutionError") + assert core in message + assert "\x00" not in message + assert "\x1b" not in message + assert "\u2028" not in message + assert "\\n" in message + assert "\\r" in message + assert "\\x00" in message + assert "...[truncated]" in message + assert "P" * 600 not in message + assert len(message) < 800 + + +@pytest.mark.anyio +async def test_rest_invoke_log_injection_is_escaped_capped_and_typed( + client, clean_database, monkeypatch, caplog +): + """The REST invoke log path sanitizes, types, and caps the same way.""" + provisioned = await provision_agent_wallet(client) + request_id = _fresh_request_id() + core = _fresh_sentinel(tag="RINJECT") + payload = core + "\r\nFORGED: entry\x00" + "Q" * 1000 + _patch_adapter_invoke(monkeypatch, mcp_router.ToolPermissionDenied(payload)) + + with caplog.at_level(logging.ERROR, logger=LOGGER): + resp = await client.post( + f"/mcp/tools/{TOOL_242}/invoke", + json=_rest_body(provisioned["agent_wallet_id"], _fresh_idempotency_key()), + headers={ + **provisioned["agent_headers"], + "X-Request-ID": request_id, + }, + ) + + assert resp.status_code == 403, resp.text + assert core not in resp.text + assert "FORGED" not in resp.text + detail = resp.json()["detail"] + assert detail["error"] == "Permission denied" + assert detail["request_id"] == request_id + matched = [ + r for r in caplog.records if r.name == LOGGER and request_id in r.getMessage() + ] + assert matched, caplog.text + message = _assert_clean_error_log(matched[0], exc_type="ToolPermissionDenied") + assert core in message + assert "\x00" not in message + assert "\\r" in message + assert "\\n" in message + assert "\\x00" in message + assert "...[truncated]" in message + assert "Q" * 600 not in message + assert len(message) < 800 diff --git a/gateway/tests/test_mcp_idempotency_key_validation.py b/gateway/tests/test_mcp_idempotency_key_validation.py index 728ef389e..eb9fdbe25 100644 --- a/gateway/tests/test_mcp_idempotency_key_validation.py +++ b/gateway/tests/test_mcp_idempotency_key_validation.py @@ -801,5 +801,6 @@ async def test_rest_invoke_changed_excluded_argument_conflicts( headers=headers, ) assert changed.status_code == 409, changed.text - assert changed.json()["detail"] == "idempotency_key_reused" + assert changed.json()["detail"]["error"] == "idempotency_key_reused" + assert "request_id" in changed.json()["detail"] assert counted_tool.effects == ["one"] diff --git a/gateway/tests/test_mcp_upstream_governed.py b/gateway/tests/test_mcp_upstream_governed.py index 824e5f52c..5ac400dda 100644 --- a/gateway/tests/test_mcp_upstream_governed.py +++ b/gateway/tests/test_mcp_upstream_governed.py @@ -1985,7 +1985,8 @@ async def test_precanonical_alternate_transport_identity_fails_closed( headers=provisioned["agent_headers"], ) assert response.status_code == 409 - assert response.json()["detail"] == "idempotency_key_reused" + assert response.json()["detail"]["error"] == "idempotency_key_reused" + assert "request_id" in response.json()["detail"] assert executor.dispatch_count == 0 assert await _wallet_debit_and_canonical_counts( diff --git a/gateway/tests/test_permit_v2_constraints.py b/gateway/tests/test_permit_v2_constraints.py index 3ff0b647b..e89992c7f 100644 --- a/gateway/tests/test_permit_v2_constraints.py +++ b/gateway/tests/test_permit_v2_constraints.py @@ -696,8 +696,8 @@ async def test_forbidden_fields_denies_matching_arg(client, clean_database): body = r.json() assert "error" in body assert body["error"]["code"] == -32003 - assert "permit_forbidden_field" in body["error"]["message"] - assert "secret_token" in body["error"]["message"] + assert body["error"]["message"] == "permit_forbidden_field" + assert "secret_token" not in body["error"]["message"] # Verify no budget consumed (denial before reserve) factory = get_session_factory() diff --git a/gateway/tests/test_receipts.py b/gateway/tests/test_receipts.py index ef9644edd..270d03e18 100644 --- a/gateway/tests/test_receipts.py +++ b/gateway/tests/test_receipts.py @@ -262,7 +262,8 @@ async def test_dynamic_forbidden_field_denial_signs_stable_reason_without_charge assert response.status_code == 200, response.text error = response.json()["error"] - assert error["message"] == f"permit_forbidden_field:{unsafe_field_name}" + assert error["message"] == "permit_forbidden_field" + assert unsafe_field_name not in error["message"] receipt = error["data"]["receipt"] assert receipt["reason_code"] == "permit_forbidden_field" assert receipt["ledger_entry_id"] is None