diff --git a/mcp_server/prowler_mcp_server/lib/errors.py b/mcp_server/prowler_mcp_server/lib/errors.py new file mode 100644 index 00000000000..1c11ebf63ce --- /dev/null +++ b/mcp_server/prowler_mcp_server/lib/errors.py @@ -0,0 +1,288 @@ +"""What a Prowler MCP Server failure is, and the sentence it is described with. + +MCP draws a line this server used to blur. A tool that *returns* `{"error": ...}` +produces a successful result (`isError: false`) whose failure is only discoverable by +guessing which key to look at; a tool that *raises* produces `isError: true`, which +every client and model already understands as "this call did not work". Moving the +server onto the second of those needs one vocabulary of failures and one way to render +them, which is what this module is; raising them is the sub-servers' job. + +`render_tool_error` is the single place an exception becomes text a client reads, so +the same failure can never get described two different ways. + +The exception types live here rather than next to the API client because the hub and the +documentation sub-servers must be able to raise and render them without taking a +dependency on `prowler_app`. +""" + +from __future__ import annotations + +import json +from dataclasses import dataclass +from typing import Any + +import httpx + +# Upstream bodies are not ours and may be large or HTML; enough to diagnose, not enough +# to flood the model's context. +_MAX_UPSTREAM_BODY = 500 + +# The one thing a status code does not say. Appended only when a request that could have +# changed something did not come back with a verdict, because an agent reads a failure as +# "nothing happened" and will happily send the write again. +_UNKNOWN_OUTCOME = ( + " It may have been carried out anyway, so check the current state before retrying." +) + + +@dataclass(frozen=True, slots=True) +class ApiErrorDetail: + """One entry of a JSON:API `errors` array. + + The API answers a rejected write with one error *per invalid field*, each naming the + field in `source`. Keeping the whole shape is what turns "the request was invalid" + into "these two fields were invalid, and here is which". + + Field names are JSON:API's own. `source.pointer` and `source.parameter` are not two + spellings of one thing: a pointer is a JSON Pointer into the request *document* + (`/data/attributes/provider_id`, as `alerts/errors.py` and `tasks/beat.py` send), + while a parameter is a *query parameter* name (`page[size]`, `lookback_days`, as + `api/v1/views.py` sends). The spec has a third, `source.header`; the Prowler API + never emits one, so there is nothing here to read it into. + """ + + detail: str | None = None + title: str | None = None + pointer: str | None = None + """JSON:API `source.pointer`: a JSON Pointer into the request document.""" + parameter: str | None = None + """JSON:API `source.parameter`: the query parameter that caused the error.""" + + @classmethod + def from_jsonapi(cls, error: dict[str, Any]) -> ApiErrorDetail: + """Build from a single member of a JSON:API `errors` array. + + `source` is optional and most errors omit it, so it supplies the location only, + never whether there is a detail worth reporting. + """ + source = error.get("source", {}) + return cls( + detail=error.get("detail"), + title=error.get("title"), + pointer=source.get("pointer"), + parameter=source.get("parameter"), + ) + + def render(self) -> str: + """The error text, and where the API said it is. + + A pointer is left as-is because a leading `/` already reads as a path into the + body. A parameter is labelled, since `(page[size])` on its own would read like + one. + """ + text = self.detail or self.title or "" + if not text: + return "" + if self.pointer: + return f"{text} ({self.pointer})" + if self.parameter: + return f"{text} (parameter {self.parameter})" + return text + + +def parse_jsonapi_errors(payload: Any) -> tuple[ApiErrorDetail, ...]: + """Extract every error from a JSON:API error document. + + Tolerant on purpose: this runs while handling a failure, and a body that is not the + document it should be must not turn a useful API error into a parsing traceback. + """ + if not isinstance(payload, dict): + return () + errors = payload.get("errors") + if not isinstance(errors, list): + return () + return tuple( + ApiErrorDetail.from_jsonapi(error) + for error in errors + if isinstance(error, dict) + ) + + +class ProwlerAPIError(Exception): + """An error response returned by the Prowler API. + + Raised only when the API answered with an error status, which tells a caller + something no plain exception can: the request reached Prowler and was + rejected, so it changed nothing. A timeout or a dropped connection stays a + bare exception because the request may well have been processed. + """ + + def __init__( + self, + message: str, + status_code: int, + *, + method: str | None = None, + path: str | None = None, + errors: tuple[ApiErrorDetail, ...] = (), + ) -> None: + super().__init__(message) + self.status_code: int = status_code + # Stored as plain strings so this module stays independent of the API client's + # HTTPMethod enum; StrEnum members compare equal to their value either way. + self.method: str | None = str(method) if method is not None else None + self.path: str | None = path + self.errors: tuple[ApiErrorDetail, ...] = tuple(errors) + + @property + def rejected(self) -> bool: + """The request reached Prowler and was refused, so it changed nothing.""" + return 400 <= self.status_code < 500 + + +class ProwlerTaskError(Exception): + """A background task this server was waiting on did not complete. + + Separate from `ProwlerAPIError` because the API already accepted the work: the + task exists and may still be running, so the outcome is unknown rather than refused. + """ + + def __init__(self, message: str, *, task_id: str, state: str) -> None: + super().__init__(message) + self.task_id: str = task_id + self.state: str = state + """One of `timeout`, `failed` or `cancelled`.""" + + +class ProwlerAuthError(ValueError): + """The credentials are missing, malformed or expired. + + Subclasses `ValueError` so that the handlers which already treat an + authentication failure as a refusal-before-send keep working. It is matched by name + in `render_tool_error` rather than by that base class, so it is described as the + credential problem it is instead of falling through to the bug branch. + """ + + +class ProwlerHubError(Exception): + """The Prowler Hub answered with an error status. + + The Hub is a separate public service with its own client, so its failures cannot be + `ProwlerAPIError`. Everything the Hub exposes is a read. + """ + + def __init__( + self, + message: str, + *, + status_code: int, + path: str, + body: str | None = None, + ) -> None: + super().__init__(message) + self.status_code: int = status_code + self.path: str = path + self.body: str | None = body + + +def _upstream_detail(body: str | None) -> str: + """A readable line from an error body that is not JSON:API. + + The Prowler API answers with a JSON:API document, which is parsed into + `ApiErrorDetail`. Every other host this server talks to has its own shape: the Hub + answers `{"error": "Not found"}`, GitHub answers plain text, a proxy in between may + answer HTML. Relaying any of those verbatim puts braces and markup in front of the + model, so the message is pulled out when there is one and truncated when there is + not. + """ + if not body: + return "" + text = body.strip() + try: + parsed = json.loads(text) + except ValueError: + parsed = None + if isinstance(parsed, dict): + for key in ("error", "message", "detail"): + value = parsed.get(key) + if isinstance(value, str) and value: + return value + return f"{text[:_MAX_UPSTREAM_BODY]}..." if len(text) > _MAX_UPSTREAM_BODY else text + + +def render_tool_error(error: Exception, *, warn: bool = True) -> str: + """Describe an exception in the plainest sentence that keeps every useful detail. + + Callers that surface a failure from anywhere other than a raised exception -- a + message they write themselves, a field of a structured result -- go through here + too, which is what keeps one failure from being described two different ways. + + Pass `warn=False` when the caller already states the outcome, which a structured + result reporting `status="unknown"` does by definition. Otherwise the generic + warning lands next to a more specific one saying the same thing. + + Ordered most specific first: `ProwlerAuthError` is a `ValueError` and + `httpx.TimeoutException` is a `RequestError`, so the general branches come last. + """ + unknown = _UNKNOWN_OUTCOME if warn else "" + + if isinstance(error, ProwlerAPIError): + operation = ( + " ".join(p for p in (error.method, error.path) if p) or "The request" + ) + details = "; ".join(text for text in (d.render() for d in error.errors) if text) + message = f"{operation} failed with HTTP {error.status_code}." + if details: + message = f"{message} {details}" + # A 4xx is a refusal, so it changed nothing and needs no warning. + if error.rejected or error.method == "GET": + return message + return message + unknown + + if isinstance(error, ProwlerTaskError): + # The API accepted the work before the wait failed, so the outcome is open + # whichever way the task ended. + return f"{error}{unknown}" + + if isinstance(error, ProwlerAuthError): + return f"Prowler authentication failed: {error}" + + if isinstance(error, ProwlerHubError): + # Same shape as the API branch, with the service named because the Hub can be + # down while the API is fine. Everything the Hub exposes is a GET. + message = f"Prowler Hub GET {error.path} failed with HTTP {error.status_code}." + detail = _upstream_detail(error.body) + return f"{message} {detail}" if detail else message + + if isinstance(error, httpx.HTTPStatusError): + # An upstream that is not the Prowler API, such as the external-URL fetch. + request = error.request + message = ( + f"{request.method} {request.url} failed with HTTP " + f"{error.response.status_code}." + ) + detail = _upstream_detail(error.response.text) + return f"{message} {detail}" if detail else message + + if isinstance(error, httpx.RequestError): + # No answer at all: a timeout, a dropped connection, a DNS failure. + try: + operation = f"{error.request.method} {error.request.url}" + method = error.request.method + except RuntimeError: + # httpx only attaches the request once it has one, and reading it before + # then raises. Never let that hide the failure being reported. + operation, method = "The request", None + suffix = "" if method == "GET" else unknown + return f"{operation} got no answer ({type(error).__name__}: {error}).{suffix}" + + # No `ValueError` branch, deliberately. A message written for the caller is raised + # as a `ToolError`, which never reaches here. What is left -- a model factory + # rejecting an API payload, a pydantic `ValidationError`, an `int()` on something + # that is not a number -- is this server or the API breaking its own contract, and + # saying so is the only useful thing to tell a caller who cannot fix it. + return ( + f"The Prowler MCP Server hit an unexpected {type(error).__name__}: {error}. " + "This is a bug in the server, not something you can fix by changing the " + "arguments." + ) diff --git a/mcp_server/tests/lib/__init__.py b/mcp_server/tests/lib/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/mcp_server/tests/lib/test_errors.py b/mcp_server/tests/lib/test_errors.py new file mode 100644 index 00000000000..8e96667a256 --- /dev/null +++ b/mcp_server/tests/lib/test_errors.py @@ -0,0 +1,311 @@ +"""Tests for the sentence a failure is described with. + +These assert on the *text* a model reads, because that text is the whole contract: a +`ToolError` carries nothing else. What matters is that the API's own words survive +intact, and that a write whose outcome nobody can report says so. + +How that sentence reaches a client -- and that no tool can escape it -- is +`test_server.py`. +""" + +import httpx +import pytest + +from prowler_mcp_server.lib.errors import ( + ApiErrorDetail, + ProwlerAPIError, + ProwlerAuthError, + ProwlerHubError, + ProwlerTaskError, + parse_jsonapi_errors, + render_tool_error, +) + +MAY_HAVE_LANDED = "It may have been carried out anyway" + + +# --------------------------------------------------------------------------- parsing + + +def test_every_error_of_the_document_is_preserved(): + """A rejected write names one error per invalid field; all of them matter.""" + errors = parse_jsonapi_errors( + { + "errors": [ + {"status": "400", "detail": "This field may not be blank."}, + {"status": "400", "detail": "Enter a valid URL."}, + ] + } + ) + + assert [error.detail for error in errors] == [ + "This field may not be blank.", + "Enter a valid URL.", + ] + + +def test_a_query_parameter_error_is_not_dressed_up_as_a_body_path(): + """`source.parameter` and `source.pointer` are different places, per JSON:API. + + The API sends a parameter for a bad query string (`api/v1/views.py` answers + `page[size]` and `lookback_days` that way) and a pointer for a bad body field. + Rendering a parameter bare would read as though `page[size]` were a path into the + document, which is somewhere the caller never put it. + """ + (error,) = parse_jsonapi_errors( + { + "errors": [ + { + "detail": "invalid parameter 'page[size]'", + "source": {"parameter": "page[size]"}, + } + ] + } + ) + + assert error.parameter == "page[size]" + assert error.pointer is None + assert error.render() == "invalid parameter 'page[size]' (parameter page[size])" + + +def test_the_field_an_error_points_at_is_kept(): + """`source.pointer` is what turns "invalid" into "this field is invalid".""" + (error,) = parse_jsonapi_errors( + { + "errors": [ + { + "detail": "This field may not be blank.", + "source": {"pointer": "/data/attributes/name"}, + } + ] + } + ) + + assert error.render() == ("This field may not be blank. (/data/attributes/name)") + + +def test_an_error_with_only_a_title_still_says_something(): + """`detail` is the useful field, but the API does not always send one.""" + assert ApiErrorDetail(title="Not Found").render() == "Not Found" + + +@pytest.mark.parametrize( + "payload", + [None, "not a document", {}, {"errors": "not a list"}, {"errors": [None]}], + ids=["none", "text", "empty", "errors-not-a-list", "member-not-a-dict"], +) +def test_a_body_that_is_not_an_error_document_is_tolerated(payload): + """Parsing runs while handling a failure; it must not become the failure.""" + assert parse_jsonapi_errors(payload) == () + + +def test_an_error_with_nothing_in_it_renders_empty(): + """Rendered to nothing rather than to punctuation, so composition can skip it.""" + assert ApiErrorDetail().render() == "" + + +# --------------------------------------------------------------------- api failures + + +def test_a_failed_read_names_the_call_and_the_reason(): + message = render_tool_error( + ProwlerAPIError( + "API request failed: 404 - Not found.", + 404, + method="GET", + path="/findings/nope", + errors=parse_jsonapi_errors( + {"errors": [{"detail": "No Finding matches the given query."}]} + ), + ) + ) + + assert message == ( + "GET /findings/nope failed with HTTP 404. No Finding matches the given query." + ) + + +def test_every_error_of_a_rejected_write_reaches_the_client(): + """The API rejects a write with one error per invalid field, and all of them help.""" + message = render_tool_error( + ProwlerAPIError( + "API request failed: 400 - blank", + 400, + method="POST", + path="/integrations", + errors=parse_jsonapi_errors( + { + "errors": [ + { + "detail": "This field may not be blank.", + "source": {"pointer": "/data/attributes/bucket_name"}, + }, + {"detail": "Enter a valid URL."}, + ] + } + ), + ) + ) + + assert message == ( + "POST /integrations failed with HTTP 400. " + "This field may not be blank. (/data/attributes/bucket_name); " + "Enter a valid URL." + ) + + +def test_a_rejected_write_gets_no_warning(): + """A 4xx changed nothing, so there is nothing to warn about.""" + message = render_tool_error( + ProwlerAPIError("boom", 400, method="POST", path="/scans") + ) + + assert MAY_HAVE_LANDED not in message + + +def test_a_write_that_hit_a_server_error_warns_it_may_have_landed(): + """The API validates and queues before answering, so a 500 may have gone through.""" + message = render_tool_error( + ProwlerAPIError("boom", 500, method="DELETE", path="/integrations/i1") + ) + + assert message.startswith("DELETE /integrations/i1 failed with HTTP 500.") + assert MAY_HAVE_LANDED in message + + +def test_a_failed_read_never_warns(): + """A read cannot have changed anything, whatever went wrong.""" + message = render_tool_error( + ProwlerAPIError("boom", 500, method="GET", path="/scans") + ) + + assert MAY_HAVE_LANDED not in message + + +# ------------------------------------------------------------------ no answer at all + + +def test_a_write_that_got_no_answer_warns_it_may_have_landed(): + """A timeout is the case the warning exists for.""" + request = httpx.Request("POST", "https://api.testing.invalid/api/v1/scans") + message = render_tool_error(httpx.ReadTimeout("timed out", request=request)) + + assert "POST https://api.testing.invalid/api/v1/scans got no answer" in message + assert "ReadTimeout" in message + assert MAY_HAVE_LANDED in message + + +def test_a_read_that_got_no_answer_does_not_warn(): + request = httpx.Request("GET", "https://api.testing.invalid/api/v1/findings") + message = render_tool_error(httpx.ReadTimeout("timed out", request=request)) + + assert MAY_HAVE_LANDED not in message + + +def test_a_dropped_connection_names_what_went_wrong(): + request = httpx.Request("POST", "https://api.testing.invalid/api/v1/scans") + message = render_tool_error(httpx.ConnectError("connection reset", request=request)) + + assert "ConnectError: connection reset" in message + + +def test_an_unfinished_task_warns_it_may_have_landed(): + """The API already accepted the work, so the outcome is open, not refused.""" + message = render_tool_error( + ProwlerTaskError( + "Task t1 polling timed out after 60 seconds.", task_id="t1", state="timeout" + ) + ) + + assert message.startswith("Task t1 polling timed out after 60 seconds.") + assert MAY_HAVE_LANDED in message + + +# -------------------------------------------------------------- refusals before send + + +def test_a_stray_value_error_is_reported_as_a_bug(): + """The distinction the previous passthrough branch could not make. + + A model factory rejecting an API payload, or an `int()` on something that is not a + number, is not the caller's mistake. Describing it like a validation message sends + an agent off rewriting arguments that were never the problem. + """ + message = render_tool_error( + ValueError("Missing pagination metadata in API response") + ) + + assert "unexpected ValueError" in message + assert "bug in the server" in message + + +def test_an_authentication_failure_says_so(): + """A `ValueError` subclass, so it must be recognised before the generic branch.""" + assert render_tool_error(ProwlerAuthError("Token has expired")) == ( + "Prowler authentication failed: Token has expired" + ) + + +# ---------------------------------------------------------------------- other hosts + + +def test_a_hub_failure_reads_like_an_api_failure(): + """Same sentence as the Prowler API, with the service named. + + The Hub can be down while the API is fine, so which one failed is worth the two + extra words -- but the shape must not differ, or the two look like two contracts. + """ + message = render_tool_error( + ProwlerHubError( + "hub failed", + status_code=404, + path="/check/test", + body='{"error": "Not found"}', + ) + ) + + assert message == "Prowler Hub GET /check/test failed with HTTP 404. Not found" + + +@pytest.mark.parametrize( + ("body", "expected"), + [ + ('{"error": "Not found"}', "Not found"), + ('{"message": "Bad gateway"}', "Bad gateway"), + ('{"detail": "Rate limited"}', "Rate limited"), + ("Service Unavailable", "Service Unavailable"), + ('{"unexpected": "shape"}', '{"unexpected": "shape"}'), + ], + ids=["error", "message", "detail", "plain-text", "unknown-json"], +) +def test_an_upstream_message_is_pulled_out_of_whatever_shape_it_came_in(body, expected): + """Hosts that are not the Prowler API each have their own error shape. + + Relaying the raw body puts JSON braces, or a whole HTML page, in front of the model. + """ + message = render_tool_error( + ProwlerHubError("hub failed", status_code=500, path="/checks", body=body) + ) + + assert message.endswith(expected) + + +def test_an_upstream_body_is_truncated(): + """An HTML error page must not flood the model's context.""" + message = render_tool_error( + ProwlerHubError("hub failed", status_code=500, path="/checks", body="x" * 2000) + ) + + assert "x" * 500 in message + assert "x" * 501 not in message + + +# --------------------------------------------------------------------- server bugs + + +def test_a_bug_in_this_server_is_reported_as_a_bug(): + """Named as ours, so the caller stops trying to fix it by changing arguments.""" + message = render_tool_error(KeyError("attributes")) + + assert "unexpected KeyError" in message + assert "bug in the server" in message