feat(mcp): classify shared tool failures and stop relaying upstream bodies (#12531)

mcp_server/tests/lib/test_errors.py

@@ -0,0 +1,197 @@
+"""Tests for the shared failure classifier.
+
+Two properties are pinned here: a failed tool call answers with ``isError: true``
+rather than a result object the client reads as a success, and the only text that
+reaches a model is text this server produced.
+"""
+
+import json
+
+import pytest
+from fastmcp import Client
+from pydantic import BaseModel, ValidationError
+
+from prowler_mcp_server.lib.errors import InvalidArgument, _describe_failure
+from prowler_mcp_server.prowler_app.utils.api_client import (
+    ProwlerAPIError,
+    ProwlerAPIInvalidResponse,
+    ProwlerAPIUnreachable,
+)
+from tests.helpers.jsonapi import jsonapi_error
+
+LATEST = "/api/v1/findings/latest"
+
+
+# ------------------------------------------------------------ classification
+
+
+@pytest.mark.parametrize(
+    ("status", "expected"),
+    [
+        (401, "missing, malformed or expired"),
+        (403, "prowler_get_current_user"),
+        (429, "rate limiting"),
+        (503, "failed on Prowler's side"),
+    ],
+    ids=["unauthorized", "forbidden", "rate-limited", "unavailable"],
+)
+def test_the_failures_every_authenticated_tool_shares_get_one_message(status, expected):
+    """These four mean the same thing whichever tool hit them."""
+    message = _describe_failure(ProwlerAPIError("failed", status))
+
+    assert expected in message
+
+
+def test_a_rejection_relays_the_apis_own_reason():
+    """`errors[].detail` is written by Prowler for a caller, so it is ours to relay."""
+    message = _describe_failure(
+        ProwlerAPIError("failed", 400, detail="scan_id is not a valid UUID.")
+    )
+
+    assert "scan_id is not a valid UUID." in message
+
+
+def test_a_rejection_with_no_trustworthy_reason_says_so_instead_of_guessing():
+    """A body that was not JSON:API leaves `detail` unset, and it stays unrelayed."""
+    message = _describe_failure(ProwlerAPIError("failed", 400))
+
+    assert "gave no reason" in message
+
+
+def test_a_request_that_got_no_answer_says_the_outcome_is_unknown():
+    """An unanswered write may well have landed, so repeating it can duplicate it."""
+    message = _describe_failure(ProwlerAPIUnreachable("POST /providers got no answer"))
+
+    assert "could not be reached" in message
+    assert "unknown" in message
+
+
+def test_an_unreadable_api_answer_is_not_blamed_on_the_arguments():
+    """The same `JSONDecodeError` means opposite things on the two sides."""
+    message = _describe_failure(
+        ProwlerAPIInvalidResponse(
+            "GET /findings answered 200 with a body that is not JSON"
+        )
+    )
+
+    assert "could not read" in message
+    assert "argument" not in message
+
+
+def test_an_unreadable_api_answer_is_never_called_safe_to_repeat():
+    """`post`, `patch` and `delete` reach this too, and a write may have landed."""
+    message = _describe_failure(
+        ProwlerAPIInvalidResponse(
+            "POST /providers answered 201 with a body that is not JSON"
+        )
+    )
+
+    assert "unknown" in message
+    assert "check the current state" in message
+
+
+def test_an_argument_this_server_rejected_is_repeated_verbatim():
+    """`InvalidArgument` exists to mark a message as one we wrote."""
+    message = _describe_failure(
+        InvalidArgument("page_size must be between 1 and 1000.")
+    )
+
+    assert message == "page_size must be between 1 and 1000."
+
+
+def test_a_pydantic_rejection_names_the_field_without_echoing_the_value():
+    """Pydantic quotes `input_value` back, and these tools take credentials."""
+
+    class Credentials(BaseModel):
+        api_token: int
+
+    with pytest.raises(ValidationError) as raised:
+        Credentials(api_token="hunter2-the-real-secret")
+
+    message = _describe_failure(raised.value)
+
+    assert "api_token" in message
+    assert "hunter2-the-real-secret" not in message
+
+
+def test_unparseable_json_is_reported_without_quoting_the_input():
+    """`JSONDecodeError` is a ValueError whose message quotes what it was given."""
+    with pytest.raises(json.JSONDecodeError) as raised:
+        json.loads('{"api_token": "hunter2-the-real-secret"')
+
+    message = _describe_failure(raised.value)
+
+    assert "could not be parsed" in message
+    assert "hunter2" not in message
+
+
+def test_an_unrecognised_failure_is_left_masked():
+    """Saying nothing is the safe default; the alternative is relaying anything."""
+    assert (
+        _describe_failure(RuntimeError("connection pool exhausted at 10.0.0.4:5432"))
+        is None
+    )
+
+
+# --------------------------------------------------------- through the server
+
+
+async def test_a_failing_tool_answers_with_is_error_not_a_result(
+    mcp_root_server, mock_api_client, mock_router
+):
+    """An error dict would arrive as `isError: false` and read as a success."""
+    mock_router.add("GET", LATEST, status=403, json=jsonapi_error(403, "Denied."))
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp("prowler_search_security_findings", {})
+
+    assert result.isError is True
+    assert result.structuredContent is None
+    assert "prowler_get_current_user" in result.content[0].text
+
+
+async def test_an_upstream_body_never_reaches_the_agent(
+    mcp_root_server, mock_api_client, mock_router
+):
+    """A body this server did not write is logged and replaced, never relayed."""
+    mock_router.add(
+        "GET",
+        LATEST,
+        status=500,
+        text="Traceback: psycopg2 could not connect to internal-db:5432",
+    )
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp("prowler_search_security_findings", {})
+
+    assert result.isError is True
+    assert "internal-db" not in result.content[0].text
+    assert "failed on Prowler's side" in result.content[0].text
+
+
+async def test_a_bad_argument_is_rejected_before_any_request_goes_out(
+    mcp_root_server, mock_api_client, mock_router
+):
+    """Local validation saves a round trip, and its message is safe to repeat."""
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp(
+            "prowler_search_security_findings", {"page_size": 5000}
+        )
+
+    assert result.isError is True
+    assert "Must be between 1 and 1000" in result.content[0].text
+    assert mock_router.requests == []
+
+
+async def test_an_unreadable_api_answer_does_not_reach_the_agent_as_a_bad_argument(
+    mcp_root_server, mock_api_client, mock_router
+):
+    """Told apart by type, so the agent is not sent to fix an argument that is fine."""
+    mock_router.add("GET", LATEST, text="<html>gateway timeout</html>")
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp("prowler_search_security_findings", {})
+
+    assert result.isError is True
+    assert "gateway timeout" not in result.content[0].text
+    assert "argument" not in result.content[0].text

mcp_server/tests/prowler_app/utils/test_api_client.py

@@ -7,9 +7,19 @@ query encoding and header assembly stay covered.
 
 import httpx
 import pytest
+from fastmcp.exceptions import ToolError
 
-from prowler_mcp_server.prowler_app.utils.api_client import ProwlerAPIError
-from tests.helpers.jsonapi import jsonapi_collection, jsonapi_error, jsonapi_resource
+from prowler_mcp_server.prowler_app.utils.api_client import (
+    ProwlerAPIError,
+    ProwlerAPIInvalidResponse,
+    ProwlerAPIUnreachable,
+)
+from tests.helpers.jsonapi import (
+    jsonapi_collection,
+    jsonapi_error,
+    jsonapi_resource,
+    task_document,
+)
 from tests.helpers.tokens import FAKE_API_KEY
 
 
@@ -46,11 +56,7 @@ async def test_get_forwards_query_parameters(mock_api_client, mock_router):
 
 
 async def test_error_response_surfaces_the_jsonapi_detail(mock_api_client, mock_router):
-    """A failed request is raised with the API's own `errors[].detail` message.
-
-    Tools relay this text straight to the model, so losing it turns an actionable
-    error into an opaque one.
-    """
+    """A failed request carries the API's own `errors[].detail`."""
     mock_router.add(
         "GET",
         "/api/v1/findings/nope",
@@ -64,26 +70,112 @@ async def test_error_response_surfaces_the_jsonapi_detail(mock_api_client, mock_
         await mock_api_client.get("/findings/nope")
 
     assert raised.value.status_code == 404
+    assert raised.value.detail == "Not found."
+
+
+async def test_a_body_that_is_not_jsonapi_is_never_repeated(
+    mock_api_client, mock_router
+):
+    """A body that is not JSON:API leaves `detail` unset, so nothing is relayed."""
+    mock_router.add(
+        "GET",
+        "/api/v1/findings",
+        status=502,
+        text="<html><body>Traceback: secret-internal-host:5432</body></html>",
+    )
+
+    with pytest.raises(ProwlerAPIError) as raised:
+        await mock_api_client.get("/findings")
+
+    assert raised.value.detail is None
+    assert "secret-internal-host" not in str(raised.value)
+
+
+async def test_a_server_error_detail_never_reaches_the_exception_text(
+    mock_api_client, mock_router
+):
+    """On a 5xx `errors[].detail` carries the failure, not a reason for a caller.
+
+    Tools that answer with `str(exc)` bypass the shared classifier, so the check
+    is on the exception itself rather than on the message the classifier builds.
+    """
+    mock_router.add(
+        "GET",
+        "/api/v1/findings",
+        status=500,
+        json=jsonapi_error(
+            500, "OperationalError: could not connect to secret-internal-host:5432"
+        ),
+    )
+
+    with pytest.raises(ProwlerAPIError) as raised:
+        await mock_api_client.get("/findings")
+
+    assert raised.value.detail is None
+    assert "secret-internal-host" not in str(raised.value)
+
+
+async def test_an_unreadable_body_is_not_an_argument_failure(
+    mock_api_client, mock_router
+):
+    """A `JSONDecodeError` here is the API's doing, and must not read as ours."""
+    mock_router.add("GET", "/api/v1/findings", text="<html>gateway timeout</html>")
+
+    with pytest.raises(ProwlerAPIInvalidResponse) as raised:
+        await mock_api_client.get("/findings")
+
+    assert not isinstance(raised.value, ValueError)
+    assert "gateway timeout" not in str(raised.value)
+
+
+async def test_a_mutation_with_an_unreadable_answer_still_raises(
+    mock_api_client, mock_router
+):
+    """`post` shares the parse, so a write cannot answer with an unread body."""
+    mock_router.add(
+        "POST", "/api/v1/providers", status=201, text="<html>accepted</html>"
+    )
+
+    with pytest.raises(ProwlerAPIInvalidResponse):
+        await mock_api_client.post("/providers", json_data={"data": {}})
 
 
 async def test_a_request_that_got_no_answer_is_not_an_api_error(
     mock_api_client, mock_router
 ):
-    """`ProwlerAPIError` means the API answered, and callers act on that.
-
-    A write tool tells a rejected request -- which changed nothing -- from one
-    that may have been processed by the type of the failure, so a timeout must
-    not be dressed up as a rejection.
-    """
+    """`ProwlerAPIError` means the API answered, so a timeout must not use it."""
 
     def timed_out(request):
         raise httpx.ReadTimeout("Timed out reading the response", request=request)
 
     mock_router.add_handler("GET", "/api/v1/findings", timed_out)
 
-    with pytest.raises(httpx.ReadTimeout):
+    with pytest.raises(ProwlerAPIUnreachable) as raised:
         await mock_api_client.get("/findings")
 
+    assert not isinstance(raised.value, ProwlerAPIError)
+
+
+async def test_a_failed_task_does_not_relay_its_own_error_text(
+    mock_api_client, mock_router
+):
+    """A failed task's error is an upstream body, so polling must not repeat it."""
+    mock_router.add(
+        "GET",
+        "/api/v1/tasks/t1",
+        json=task_document(
+            "t1",
+            "failed",
+            error="Traceback: connection to secret-internal-host:5432 refused",
+        ),
+    )
+
+    with pytest.raises(ToolError) as raised:
+        await mock_api_client.poll_task_until_complete(task_id="t1", timeout=5)
+
+    assert "secret-internal-host" not in str(raised.value)
+    assert "t1" in str(raised.value)
+
 
 def test_build_filter_params_normalises_types_for_the_api(mock_api_client):
     """Booleans become lowercase strings, sequences become CSV, `None` is dropped."""