From 91e6cb798d47dfe16064153a2f5d07269a933d12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20De=20la=20Torre=20Vico?= Date: Wed, 26 Aug 2026 11:06:07 +0200 Subject: [PATCH] feat(mcp): classify shared tool failures and stop relaying upstream bodies (#12531) --- docs/developer-guide/mcp-server.mdx | 70 +++++- mcp_server/AGENTS.md | 3 +- .../mcp-shared-failure-messages.added.md | 1 + .../mcp-upstream-error-bodies.security.md | 1 + mcp_server/prowler_mcp_server/lib/errors.py | 238 ++++++++++++++++++ .../prowler_app/utils/api_client.py | 148 ++++++----- mcp_server/prowler_mcp_server/server.py | 10 +- mcp_server/tests/lib/__init__.py | 0 mcp_server/tests/lib/test_errors.py | 197 +++++++++++++++ .../prowler_app/utils/test_api_client.py | 120 +++++++-- skills/prowler-mcp/SKILL.md | 6 +- 11 files changed, 708 insertions(+), 86 deletions(-) create mode 100644 mcp_server/changelog.d/mcp-shared-failure-messages.added.md create mode 100644 mcp_server/changelog.d/mcp-upstream-error-bodies.security.md create mode 100644 mcp_server/prowler_mcp_server/lib/errors.py create mode 100644 mcp_server/tests/lib/__init__.py create mode 100644 mcp_server/tests/lib/test_errors.py diff --git a/docs/developer-guide/mcp-server.mdx b/docs/developer-guide/mcp-server.mdx index 186786651f..7e1461249e 100644 --- a/docs/developer-guide/mcp-server.mdx +++ b/docs/developer-guide/mcp-server.mdx @@ -120,14 +120,14 @@ class NewFeatureTools(BaseTool): Returns complete feature details including configuration and metadata. """ - try: - response = await self.api_client.get(f"/api/v1/features/{feature_id}") - return DetailedFeature.from_api_response(response["data"]).model_dump() - except Exception as e: - self.logger.error(f"Failed to get feature {feature_id}: {e}") - return {"error": str(e), "status": "failed"} + response = await self.api_client.get(f"/api/v1/features/{feature_id}") + return DetailedFeature.from_api_response(response["data"]).model_dump() ``` +There is no `try`/`except` here on purpose. A failed request raises, and +[Error Handling](#error-handling) explains what turns that raise into a message +the agent can act on. + ### Step 2: Create the Models Create corresponding models in `prowler_app/models/`: @@ -369,18 +369,62 @@ async def search_items(self, status: str = Field(...)) -> dict: ### Error Handling -Return structured error responses instead of raising exceptions: +**Raise, never return.** A returned `{"error": ...}` dict is reported to the +client as `isError: false` -- a *successful* tool call whose payload happens to +mention a failure. Clients and models read that as success. A raised exception +becomes a spec-correct tool execution error instead. + +The common case therefore needs no handler at all: ```python async def get_item(self, item_id: str) -> dict: - try: - response = await self.api_client.get(f"/api/v1/items/{item_id}") - return DetailedItem.from_api_response(response["data"]).model_dump() - except Exception as e: - self.logger.error(f"Failed to get item {item_id}: {e}") - return {"error": str(e), "status": "failed"} + response = await self.api_client.get(f"/api/v1/items/{item_id}") + return DetailedItem.from_api_response(response["data"]).model_dump() ``` +`prowler_mcp_server/lib/errors.py` classifies the failures every tool shares -- +a rejected credential, a missing permission, a rate limit, an outage, an +unreachable API, a bad argument -- and gives each one a message that says what +went wrong and what to do about it. Anything it does not recognise is masked, +because `mask_error_details=True` is set on every sub-server and upstream +response bodies must never be replayed into a model's context. + +Three ways to raise, in the order to reach for them: + +```python +from fastmcp.exceptions import ToolError + +from prowler_mcp_server.lib.errors import InvalidArgument + +# 1. An argument this server rejected before any request went out. The message +# is repeated to the agent verbatim, so write it for one to read. +if not 1 <= page_size <= 1000: + raise InvalidArgument("page_size must be between 1 and 1000.") + +# 2. A request the API answered or never answered: let it propagate untouched. +# `ProwlerAPIError` and `ProwlerAPIUnreachable` are what the classifier keys +# on, and the second one is what stops a retry from duplicating a write. +response = await self.api_client.get(f"/api/v1/items/{item_id}") +data = response["data"] + +# 3. A sentence the classifier cannot know -- a resource name, a precondition, +# the next tool to call. NOTE the absent `from` clause: it is what marks the +# message as already final. With `from e` the classifier would replace it. +if not data: + raise ToolError( + f"No item with the ID {item_id!r} exists. Use prowler_list_items to " + "find a valid one." + ) +``` + +The one thing that still *returns* rather than raises is a write whose outcome is +genuinely unknown. `prowler_send_findings_to_jira` is the worked example: work +items are created one at a time and Prowler cannot delete them, so a dispatch +that stopped halfway answers with a result object carrying +`safe_to_retry: false`. "This may have been applied" is a fact about the world, +not an error, and squashing it into one loses the only thing that stops a retry +from duplicating the write. + ### Parameter Descriptions Use Pydantic `Field()` with clear descriptions. This also helps LLMs understand diff --git a/mcp_server/AGENTS.md b/mcp_server/AGENTS.md index e786f9a5b7..713155bffe 100644 --- a/mcp_server/AGENTS.md +++ b/mcp_server/AGENTS.md @@ -113,7 +113,8 @@ make test-mcp # Run the MCP test suite exactly as CI does - [ ] Models use `MinimalSerializerMixin` - [ ] API responses transformed to simplified models - [ ] No hardcoded secrets -- [ ] Error handling returns structured responses +- [ ] Failures are raised, not returned (see `prowler_mcp_server/lib/errors.py`); + a returned error dict is reported to the client as a success - [ ] Parameter descriptions use Pydantic `Field()` - [ ] Tests added under `mcp_server/tests/`, mirroring the source path below the package root (`prowler_mcp_server/prowler_app/tools/` -> `tests/prowler_app/tools/`), diff --git a/mcp_server/changelog.d/mcp-shared-failure-messages.added.md b/mcp_server/changelog.d/mcp-shared-failure-messages.added.md new file mode 100644 index 0000000000..90097193d7 --- /dev/null +++ b/mcp_server/changelog.d/mcp-shared-failure-messages.added.md @@ -0,0 +1 @@ +Failures shared by every tool - a rejected credential, a missing permission, a rate limit, an outage, an unreachable API, a bad argument - are now explained with a message that says what went wrong and what to do about it diff --git a/mcp_server/changelog.d/mcp-upstream-error-bodies.security.md b/mcp_server/changelog.d/mcp-upstream-error-bodies.security.md new file mode 100644 index 0000000000..edc9dd7db5 --- /dev/null +++ b/mcp_server/changelog.d/mcp-upstream-error-bodies.security.md @@ -0,0 +1 @@ +Stop relaying upstream response bodies to agents: a failed request now reaches the caller as a sentence this server wrote, with the full body kept to the logs, so a gateway error page or a debug traceback can no longer be replayed into a model's context 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 0000000000..29e2db6481 --- /dev/null +++ b/mcp_server/prowler_mcp_server/lib/errors.py @@ -0,0 +1,238 @@ +"""Shared failure classification for every tool in this server.""" + +import json +from typing import Any + +import httpx +from fastmcp.exceptions import McpError, NotFoundError, ToolError +from fastmcp.server.middleware import CallNext, Middleware, MiddlewareContext +from pydantic import ValidationError + +from prowler_mcp_server.lib.logger import logger + +# ------------------------------------------------------------- failure types + + +class ProwlerAPIError(Exception): + """An error response returned by the Prowler API. + + Attributes: + status_code: HTTP status the API answered with + detail: JSON:API `errors[0].detail`, None when there is none to trust + """ + + def __init__( + self, message: str, status_code: int, *, detail: str | None = None + ) -> None: + super().__init__(message) + self.status_code: int = status_code + # Prowler's own JSON:API `errors[0].detail`, which our API writes for a + # caller and we therefore trust. None when the body was not JSON:API -- + # a gateway HTML page or a debug traceback, which is exactly the case + # that must never be repeated to a model -- and None for a 5xx, see + # `jsonapi_detail`. + self.detail: str | None = detail + + +class ProwlerAPIUnreachable(Exception): + """The request never got an answer, so whether it was applied is unknown.""" + + +class ProwlerAPIInvalidResponse(Exception): + """The API answered, but with a body this server could not read as JSON.""" + + +def jsonapi_detail(response: httpx.Response) -> str | None: + """Return the API's own JSON:API error detail, when there is one to trust. + + Args: + response: Error response returned by the Prowler API + + Returns: + `errors[0].detail`, or None if the status is 5xx or the body is not + JSON:API + """ + if response.status_code >= 500: + return None + + try: + errors = response.json().get("errors") + except Exception: + return None + + if not isinstance(errors, list) or not errors: + return None + + detail = errors[0].get("detail") if isinstance(errors[0], dict) else None + return detail if isinstance(detail, str) and detail.strip() else None + + +class InvalidArgument(ValueError): + """An argument this server rejected before any request went out.""" + + +# ------------------------------------------------------------------- messages + + +def _describe_prowler_api_error(exc: ProwlerAPIError) -> str: + """Describe a request the Prowler API answered with an error status.""" + status = exc.status_code + + # The fallback: a status this server has nothing specific to say about. + message = ( + f"Prowler rejected the request with status {status} and gave no reason. " + "Check the arguments against the tool description." + ) + + if status == 401: + message = ( + "Prowler rejected this server's credential: it is missing, malformed " + "or expired. In HTTP mode the request needs an 'Authorization: Bearer " + "' header; in STDIO mode PROWLER_API_KEY must hold a valid key." + ) + elif status == 403: + message = ( + "The credential is valid but not allowed to do this. Use " + "prowler_get_current_user to see which role it holds." + ) + elif status == 429: + message = ( + "Prowler is rate limiting this credential. Wait before retrying, and " + "narrow the request with tighter filters or a smaller page_size." + ) + elif status >= 500: + message = ( + f"Prowler answered {status}: the request failed on Prowler's side, " + "not because of anything in the call." + ) + elif exc.detail: + # Written by the Prowler API for a caller to read, so it is ours to relay. + message = f"Prowler rejected the request ({status}): {exc.detail}" + + return message + + +def _describe_upstream_http_error(exc: httpx.HTTPError) -> str: + """Describe a failure from an upstream this server reads directly.""" + # `.request` raises rather than returning None when it was never set, and + # this runs inside an exception handler, so it is read defensively. + request = getattr(exc, "_request", None) + host = request.url.host if request is not None else "the upstream service" + + if not isinstance(exc, httpx.HTTPStatusError): + return f"{host} could not be reached: {type(exc).__name__}." + + status = exc.response.status_code + if status == 429: + return f"{host} is rate limiting this server. Wait before retrying." + if status >= 500: + return ( + f"{host} answered {status}: the request failed on its side, not " + "because of anything in the call." + ) + return ( + f"{host} rejected the request with status {status}. Check the arguments " + "against the tool description." + ) + + +def _describe_failure(exc: BaseException) -> str | None: + """Describe a failure for a model to read, or return None to leave it masked.""" + # InvalidArgument first: it is the only ValueError here whose message this + # server wrote. The ones below quote the input they rejected, so they are + # matched by type and answered with a message of our own. + if isinstance(exc, InvalidArgument): + return str(exc) + + if isinstance(exc, ProwlerAPIError): + return _describe_prowler_api_error(exc) + + if isinstance(exc, ProwlerAPIInvalidResponse): + return ( + "Prowler answered with a body this server could not read, so the " + "outcome of the call is unknown. If it changes anything, check the " + "current state before sending it again." + ) + + if isinstance(exc, ProwlerAPIUnreachable): + # The only failure a model can turn into a duplicate write by repeating. + return ( + f"Prowler could not be reached: {exc}. Whether the request was " + "applied is unknown, so check the current state before sending it again." + ) + + if isinstance(exc, ValidationError): + problems = [ + f"{'.'.join(str(part) for part in error['loc']) or '(argument)'}: {error['msg']}" + for error in exc.errors(include_url=False) + ] + # Field and expectation only: pydantic quotes the rejected value back. + return f"Invalid arguments -- {'; '.join(problems)}." + + if isinstance(exc, json.JSONDecodeError): + return ( + "An argument that had to be a JSON object could not be parsed. Send " + "it as a real object rather than as a quoted or escaped string." + ) + + # Only the Hub and documentation tools reach here: the Prowler API client + # converts its own httpx failures into the two types matched above. + if isinstance(exc, (httpx.HTTPStatusError, httpx.RequestError)): + return _describe_upstream_http_error(exc) + + return None + + +# ----------------------------------------------------------------- middleware + + +class SharedFailureMiddleware(Middleware): + """Replace the failures many tools share with a message a model can act on.""" + + async def on_call_tool( + self, + context: MiddlewareContext[Any], + call_next: CallNext[Any, Any], + ) -> Any: + """Replace a shared tool failure with the message that describes it. + + Args: + context: Tool call being handled + call_next: Rest of the middleware chain + + Returns: + The tool result when the call succeeded + + Raises: + ToolError: With the classified message when the failure is one this + module recognises + """ + try: + return await call_next(context) + except (NotFoundError, McpError): + # Protocol-level, not a tool failure. Must stay exactly as it is. + raise + except Exception as exc: + # FastMCP wraps whatever the tool raised and records it as __cause__. + # An absent cause means the message is already the final word: a + # ToolError raised deliberately without a `from` clause. + original = exc.__cause__ + message = _describe_failure(original) if original is not None else None + + if message is None: + if original is not None: + logger.warning( + "Tool %s failed with an unclassified error: %s: %s", + getattr(context.message, "name", ""), + type(original).__name__, + original, + ) + raise + + logger.warning( + "Tool %s failed: %s: %s", + getattr(context.message, "name", ""), + type(original).__name__, + original, + ) + raise ToolError(message) from exc diff --git a/mcp_server/prowler_mcp_server/prowler_app/utils/api_client.py b/mcp_server/prowler_mcp_server/prowler_app/utils/api_client.py index 5717a0a3b5..217496a347 100644 --- a/mcp_server/prowler_mcp_server/prowler_app/utils/api_client.py +++ b/mcp_server/prowler_mcp_server/prowler_app/utils/api_client.py @@ -7,28 +7,22 @@ from typing import Any from urllib.parse import urlparse import httpx +from fastmcp.exceptions import ToolError from prowler_mcp_server import __version__ +from prowler_mcp_server.lib.errors import ( + InvalidArgument, + ProwlerAPIError, + ProwlerAPIInvalidResponse, + ProwlerAPIUnreachable, + jsonapi_detail, +) from prowler_mcp_server.lib.logger import logger from prowler_mcp_server.prowler_app.utils.auth import ProwlerAppAuth ALLOWED_EXTERNAL_DOMAINS: frozenset[str] = frozenset({"raw.githubusercontent.com"}) -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) -> None: - super().__init__(message) - self.status_code: int = status_code - - class HTTPMethod(StrEnum): """HTTP methods enum.""" @@ -88,7 +82,8 @@ class ProwlerAPIClient(metaclass=SingletonMeta): Raises: ProwlerAPIError: If the API answered with an error status - Exception: If the request could not be completed + ProwlerAPIUnreachable: If the request got no answer + ProwlerAPIInvalidResponse: If the answer was not readable as JSON """ try: token: str = await self.auth_manager.get_valid_token() @@ -103,31 +98,61 @@ class ProwlerAPIClient(metaclass=SingletonMeta): json=json_data, ) response.raise_for_status() - - if not response.content: - return { - "success": True, - "status_code": response.status_code, - } - else: - return response.json() except httpx.HTTPStatusError as e: - logger.error(f"HTTP error during {method.value} {path}: {e}") - error_detail: str = "" - try: - error_data: dict[str, any] = e.response.json() - error_detail = error_data.get("errors", [{}])[0].get("detail", "") - except Exception: - error_detail = e.response.text - - raise ProwlerAPIError( - f"API request failed: {e.response.status_code} - {error_detail}", - e.response.status_code, + status: int = e.response.status_code + # `jsonapi_detail` returns nothing for a 5xx, so a server error never + # puts upstream text into the exception message either. + detail: str | None = jsonapi_detail(e.response) + # The full body goes to the log and nowhere else. A body that is not + # JSON:API is upstream text of unknown provenance, and the exception + # message is read by a model. + logger.error( + "HTTP error during %s %s: %s %s", + method.value, + path, + status, + (e.response.text or "")[:500], ) + + message = f"API request failed: {status}" + if detail: + message = f"{message} - {detail}" + + raise ProwlerAPIError(message, status, detail=detail) from e + except httpx.RequestError as e: + # No answer came back, so whether the request was applied is unknown. + logger.error(f"Error during {method.value} {path}: {e}") + raise ProwlerAPIUnreachable( + f"{method.value} {path} got no answer: {type(e).__name__}" + ) from e except Exception as e: logger.error(f"Error during {method.value} {path}: {e}") raise + if not response.content: + return { + "success": True, + "status_code": response.status_code, + } + + # Parsed outside the block above so that a body we cannot read is told + # apart from an argument a tool could not parse: both are a + # `JSONDecodeError`, and only the second one is the caller's doing. + try: + return response.json() + except ValueError as e: + logger.error( + "Unreadable response body during %s %s: %s %s", + method.value, + path, + response.status_code, + (response.text or "")[:500], + ) + raise ProwlerAPIInvalidResponse( + f"{method.value} {path} answered {response.status_code} with a " + "body that is not JSON" + ) from e + async def get( self, path: str, params: dict[str, any] | None = None ) -> dict[str, any]: @@ -229,14 +254,14 @@ class ProwlerAPIClient(metaclass=SingletonMeta): Raw text content from the URL Raises: - ValueError: If the URL domain is not in the allowlist - Exception: If the HTTP request fails + InvalidArgument: If the URL scheme or domain is not allowed + ToolError: If the fetch failed """ parsed = urlparse(url) if parsed.scheme != "https": - raise ValueError(f"Only HTTPS URLs are allowed, got '{parsed.scheme}'") + raise InvalidArgument(f"Only HTTPS URLs are allowed, got '{parsed.scheme}'") if parsed.hostname not in ALLOWED_EXTERNAL_DOMAINS: - raise ValueError( + raise InvalidArgument( f"Domain '{parsed.hostname}' is not allowed. " f"Allowed domains: {', '.join(sorted(ALLOWED_EXTERNAL_DOMAINS))}" ) @@ -249,13 +274,20 @@ class ProwlerAPIClient(metaclass=SingletonMeta): response.raise_for_status() return response.text except httpx.HTTPStatusError as e: - logger.error(f"HTTP error fetching external URL {url}: {e}") - raise Exception( - f"Failed to fetch external URL: {e.response.status_code}" - ) from e - except Exception as e: + # The status is ours to report; the body is upstream text and stays + # in the log. No `from` clause: this sentence is the final word. + logger.error( + "HTTP error fetching external URL %s: %s %s", + url, + e.response.status_code, + (e.response.text or "")[:500], + ) + raise ToolError( + f"Fetching {url} failed with status {e.response.status_code}." + ) + except httpx.RequestError as e: logger.error(f"Error fetching external URL {url}: {e}") - raise + raise ToolError(f"Fetching {url} got no answer: {type(e).__name__}.") async def poll_task_until_complete( self, @@ -278,7 +310,7 @@ class ProwlerAPIClient(metaclass=SingletonMeta): The complete task response when terminal state is reached Raises: - Exception: If task fails, is cancelled, or timeout is exceeded + ToolError: If the task fails, is cancelled, or the timeout is exceeded """ terminal_states = {"completed", "failed", "cancelled"} start_time = asyncio.get_event_loop().time() @@ -292,7 +324,7 @@ class ProwlerAPIClient(metaclass=SingletonMeta): # Check if we've exceeded the timeout current_time = asyncio.get_event_loop().time() if current_time >= max_time: - raise Exception( + raise ToolError( f"Task {task_id} polling timed out after {timeout} seconds. " f"The task may still be running. Try increasing the timeout or check task status manually." ) @@ -311,10 +343,14 @@ class ProwlerAPIClient(metaclass=SingletonMeta): logger.info(f"Task {task_id} completed successfully") return response elif state == "failed": - error_msg = task_attrs.get("error", "Unknown error") - raise Exception(f"Task {task_id} failed: {error_msg}") + # The task's own failure text is an upstream body: a celery + # traceback, a provider message. Log it, never relay it. + logger.error( + f"Task {task_id} failed: {task_attrs.get('error', 'no error reported')}" + ) + raise ToolError(f"Task {task_id} failed.") elif state == "cancelled": - raise Exception(f"Task {task_id} was cancelled") + raise ToolError(f"Task {task_id} was cancelled") # Wait before next poll await asyncio.sleep(poll_interval) @@ -330,12 +366,12 @@ class ProwlerAPIClient(metaclass=SingletonMeta): Parsed datetime object Raises: - ValueError: If date format is invalid + InvalidArgument: If date format is invalid """ try: return datetime.strptime(date_str, "%Y-%m-%d") except ValueError: - raise ValueError( + raise InvalidArgument( f"Invalid date format for {param_name}. Expected YYYY-MM-DD (e.g., '2025-01-15'), got '{date_str}'. " f"Full date required - partial dates like '2025' or '2025-01' are not accepted." ) @@ -347,10 +383,10 @@ class ProwlerAPIClient(metaclass=SingletonMeta): page_size: Page size to validate Raises: - ValueError: If page size is out of valid range (1-1000) + InvalidArgument: If page size is out of valid range (1-1000) """ if page_size < 1 or page_size > 1000: - raise ValueError( + raise InvalidArgument( f"Invalid page_size: {page_size}. Must be between 1 and 1000 (inclusive)." ) @@ -373,7 +409,7 @@ class ProwlerAPIClient(metaclass=SingletonMeta): None if no dates provided, otherwise tuple of (date_from, date_to) as strings Raises: - ValueError: If date range exceeds max_days or date format is invalid + InvalidArgument: If date range exceeds max_days or date format is invalid """ if not date_from and not date_to: return None @@ -394,7 +430,7 @@ class ProwlerAPIClient(metaclass=SingletonMeta): # Validate that date_from is before or equal to date_to if from_date > to_date: - raise ValueError( + raise InvalidArgument( f"Invalid date range: date_from must be before or equal to date_to. " f"Got date_from='{from_date.date()}' and date_to='{to_date.date()}'. " f"Please swap the dates or use the correct order." @@ -403,7 +439,7 @@ class ProwlerAPIClient(metaclass=SingletonMeta): # Validate range doesn't exceed max_days delta: int = (to_date - from_date).days + 1 if delta > max_days: - raise ValueError( + raise InvalidArgument( f"Date range cannot exceed {max_days} days. " f"Requested range: {from_date.date()} to {to_date.date()} ({delta} days)" ) diff --git a/mcp_server/prowler_mcp_server/server.py b/mcp_server/prowler_mcp_server/server.py index 1b3271be38..3815047d55 100644 --- a/mcp_server/prowler_mcp_server/server.py +++ b/mcp_server/prowler_mcp_server/server.py @@ -2,9 +2,17 @@ from fastmcp import FastMCP from starlette.responses import JSONResponse from prowler_mcp_server import __version__ +from prowler_mcp_server.lib.errors import SharedFailureMiddleware from prowler_mcp_server.lib.logger import logger -prowler_mcp_server = FastMCP("prowler-mcp-server") +# `mask_error_details` keeps an unhandled failure from relaying text this server +# does not control. It is set on every sub-server as well, because it does not +# reach mounted children -- FastMCP warns about exactly that at mount time. +prowler_mcp_server = FastMCP("prowler-mcp-server", mask_error_details=True) + +# Middleware, unlike masking, does reach mounted children, so the classifier that +# gives the masked failures a sentence back is wired here once. +prowler_mcp_server.add_middleware(SharedFailureMiddleware()) def setup_main_server(): diff --git a/mcp_server/tests/lib/__init__.py b/mcp_server/tests/lib/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/mcp_server/tests/lib/test_errors.py b/mcp_server/tests/lib/test_errors.py new file mode 100644 index 0000000000..5986d883c4 --- /dev/null +++ b/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="gateway timeout") + + 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 diff --git a/mcp_server/tests/prowler_app/utils/test_api_client.py b/mcp_server/tests/prowler_app/utils/test_api_client.py index 41aceedcab..f3ef313bf0 100644 --- a/mcp_server/tests/prowler_app/utils/test_api_client.py +++ b/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="Traceback: 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_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="gateway timeout") + + 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="accepted" + ) + + 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.""" diff --git a/skills/prowler-mcp/SKILL.md b/skills/prowler-mcp/SKILL.md index c98f6a886f..f23243e3ff 100644 --- a/skills/prowler-mcp/SKILL.md +++ b/skills/prowler-mcp/SKILL.md @@ -69,7 +69,11 @@ Use `@mcp.tool()` decorator directly—no BaseTool or models required. - [ ] Tool docstrings describe LLM-relevant behavior - [ ] Models use `MinimalSerializerMixin` - [ ] API responses transformed to simplified models -- [ ] Error handling returns `{"error": str, "status": "failed"}` +- [ ] Failures are **raised**, never returned. A returned error dict is reported + as a success. Raise `InvalidArgument` for a bad argument, let + `ProwlerAPIError`/`ProwlerAPIUnreachable` propagate, and raise `ToolError` + **without a `from` clause** only for a sentence `lib/errors.py` cannot know + (a resource name, a precondition, the next tool to call) - [ ] Parameters use `Field()` with descriptions - [ ] No hardcoded secrets - [ ] Tests added under `mcp_server/tests/`