feat(mcp): raise instead of returning error objects in the Prowler Hub tools (#12533)

mcp_server/tests/lib/test_errors.py

@@ -7,6 +7,7 @@ reaches a model is text this server produced.
 
 import json
 
+import httpx
 import pytest
 from fastmcp import Client
 from pydantic import BaseModel, ValidationError
@@ -14,7 +15,9 @@ from pydantic import BaseModel, ValidationError
 from prowler_mcp_server.lib.errors import (
     CredentialError,
     InvalidArgument,
+    UpstreamInvalidResponse,
     _describe_failure,
+    parse_json_response,
 )
 from prowler_mcp_server.prowler_app.utils.api_client import (
     ProwlerAPIError,
@@ -26,6 +29,42 @@ from tests.helpers.jsonapi import jsonapi_error
 LATEST = "/api/v1/findings/latest"
 
 
+# --------------------------------------------------------------- json bodies
+
+
+def _answer(
+    body: str, *, url: str = "https://hub.prowler.com/api/check"
+) -> httpx.Response:
+    """An answer as a client would hand it back, request attached."""
+    return httpx.Response(200, text=body, request=httpx.Request("GET", url))
+
+
+def test_a_json_body_is_returned_as_it_is():
+    """The helper only classifies the failure; the success path is untouched."""
+    assert parse_json_response(_answer('{"id": "s3_bucket_public_access"}')) == {
+        "id": "s3_bucket_public_access"
+    }
+
+
+def test_a_body_that_is_not_json_names_the_host_that_answered():
+    """Which upstream is misbehaving is the one useful fact here, and the shared
+    helper is reached from every sub-server that reads an upstream directly."""
+    with pytest.raises(UpstreamInvalidResponse) as raised:
+        parse_json_response(_answer("<html><body>502 Bad Gateway</body></html>"))
+
+    assert raised.value.host == "hub.prowler.com"
+    assert "Bad Gateway" not in str(raised.value)
+
+
+def test_a_body_that_is_not_json_is_not_a_valueerror():
+    """`JSONDecodeError` is a ValueError, and callers tell an upstream fault from
+    a bad argument by type alone."""
+    with pytest.raises(UpstreamInvalidResponse) as raised:
+        parse_json_response(_answer("not json"))
+
+    assert not isinstance(raised.value, ValueError)
+
+
 # ------------------------------------------------------------ classification
 
 
@@ -94,6 +133,29 @@ def test_an_unreadable_api_answer_is_never_called_safe_to_repeat():
     assert "check the current state" in message
 
 
+def test_an_unreadable_upstream_answer_is_not_blamed_on_the_arguments():
+    """A `JSONDecodeError` from an upstream and one from an argument are the same
+    exception and opposite instructions."""
+    message = _describe_failure(
+        UpstreamInvalidResponse("200 body is not JSON", host="hub.prowler.com")
+    )
+
+    assert "hub.prowler.com" in message
+    assert "could not read as JSON" in message
+    assert "changing them will not help" in message
+
+
+def test_an_unreadable_upstream_answer_never_quotes_the_body():
+    """The body is someone else's text, so only the host and the status leave here."""
+    message = _describe_failure(
+        UpstreamInvalidResponse(
+            "502 body is not JSON", host="raw.githubusercontent.com"
+        )
+    )
+
+    assert "body is not JSON" not 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(
@@ -227,3 +289,19 @@ async def test_a_tool_specific_message_survives_masking(
 
     assert result.isError is True
     assert "prowler_list_integrations" in result.content[0].text
+
+
+async def test_a_hub_tool_failure_says_which_host_refused_it(
+    mcp_root_server, hub_router
+):
+    """Hub failures arrive as raw httpx errors: host and status relayed, body not."""
+    hub_router.add(
+        "GET", "/api/check", status=503, text="<html>upstream nginx 10.1.2.3</html>"
+    )
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp("prowler_hub_list_checks", {})
+
+    assert result.isError is True
+    assert "hub.prowler.com" in result.content[0].text
+    assert "10.1.2.3" not in result.content[0].text

mcp_server/tests/lib/test_urls.py

@@ -0,0 +1,59 @@
+"""Tests for the shared URL path builder.
+
+The bug these pin: an identifier interpolated into a path was resolved away by
+httpx per RFC 3986, so "../" reached an endpoint no tool meant to call.
+"""
+
+import pytest
+
+from prowler_mcp_server.lib.urls import path_segment, url_path
+
+
+@pytest.mark.parametrize(
+    ("value", "expected"),
+    [
+        ("s3_bucket_public_access", "s3_bucket_public_access"),
+        ("cis_4.0_aws", "cis_4.0_aws"),
+        ("../../evil", "..%2F..%2Fevil"),
+        ("....//evil", "....%2F%2Fevil"),
+        ("%2e%2e%2f", "%252e%252e%252f"),
+        ("..;/", "..%3B%2F"),
+        ("s3/../evil", "s3%2F..%2Fevil"),
+        ("evil?fields=all", "evil%3Ffields%3Dall"),
+        ("evil#frag", "evil%23frag"),
+        ("evil\\wrong", "evil%5Cwrong"),
+        ("two words", "two%20words"),
+    ],
+    ids=[
+        "plain",
+        "dots-in-a-name",
+        "traversal",
+        "stripped-filter-bypass",
+        "already-encoded",
+        "path-parameter",
+        "mid-path",
+        "query",
+        "fragment",
+        "backslash",
+        "space",
+    ],
+)
+def test_a_segment_survives_as_a_name_and_never_as_syntax(value, expected):
+    """A real ID passes through untouched; URL syntax comes back as characters."""
+    assert path_segment(value) == expected
+
+
+@pytest.mark.parametrize("value", [".", ".."], ids=["here", "up-one"])
+def test_a_segment_of_nothing_but_dots_is_escaped_rather_than_left_to_resolve(value):
+    """`quote` keeps a dot, so a segment of only dots would still resolve away."""
+    assert path_segment(value) == value.replace(".", "%2E")
+
+
+def test_a_path_is_the_segments_it_was_given_and_no_others():
+    """One argument per segment, so no call site has to encode anything."""
+    assert url_path("users", "../../evil", "roles") == "/users/..%2F..%2Fevil/roles"
+
+
+def test_a_single_segment_path_keeps_its_leading_slash():
+    """Every caller joins this onto a base URL that ends without a slash."""
+    assert url_path("providers") == "/providers"

mcp_server/tests/prowler_hub/test_server.py

@@ -0,0 +1,335 @@
+"""Tests for the Prowler Hub tools.
+
+The Hub sub-server uses its own httpx clients, so its failures never pass through
+the Prowler API client. They still have to arrive as tool errors rather than as a
+result object, which the protocol, the client and the model all read as a success.
+"""
+
+import pytest
+from fastmcp import Client
+
+CHECKS = "/api/check"
+PROVIDERS = "/api/providers"
+COMPLIANCE = "/api/compliance"
+CHECK_ID = "s3_bucket_public_access"
+HUB_CHECK = f"{CHECKS}/{CHECK_ID}"
+
+
+def github_check(provider: str, suffix: str = ".py") -> str:
+    """The raw.githubusercontent path a check artifact is fetched from."""
+    return (
+        f"/prowler-cloud/prowler/refs/heads/master/prowler/providers/{provider}"
+        f"/services/s3/{CHECK_ID}/{CHECK_ID}{suffix}"
+    )
+
+
+GITHUB_CHECK = github_check("aws")
+GITHUB_FIXER = github_check("aws", "_fixer.py")
+
+
+async def test_listing_checks_returns_the_lightweight_shape(
+    mcp_root_server, hub_router
+):
+    """The happy path, so the failure tests below are not the only coverage."""
+    hub_router.add(
+        "GET",
+        CHECKS,
+        json=[
+            {
+                "id": "s3_bucket_public_access",
+                "provider": "aws",
+                "title": "S3 buckets should block public access",
+                "severity": "high",
+            }
+        ],
+    )
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool("prowler_hub_list_checks", {})
+
+    assert result.data["count"] == 1
+    assert result.data["checks"][0]["id"] == "s3_bucket_public_access"
+
+
+async def test_an_unknown_check_fails_and_names_the_tool_that_finds_one(
+    mcp_root_server, hub_router
+):
+    """A 404 is the caller's mistake, and the fix is a different tool."""
+    hub_router.add("GET", f"{CHECKS}/nope", status=404)
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp(
+            "prowler_hub_get_check_details", {"check_id": "nope"}
+        )
+
+    assert result.isError is True
+    assert "prowler_hub_semantic_search_checks" in result.content[0].text
+
+
+async def test_an_unknown_provider_fails_and_lists_the_real_ones(
+    mcp_root_server, hub_router
+):
+    """The valid values are already in hand, so withholding them wastes a call."""
+    hub_router.add(
+        "GET",
+        PROVIDERS,
+        json=[{"id": "aws", "name": "Amazon Web Services", "services": ["s3"]}],
+    )
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp(
+            "prowler_hub_get_provider_services", {"provider_id": "alicloud"}
+        )
+
+    assert result.isError is True
+    assert "aws" in result.content[0].text
+
+
+async def test_a_check_without_a_fixer_says_that_is_normal(mcp_root_server, hub_router):
+    """Most checks have no auto-remediation, so this must not read as a defect."""
+    hub_router.add("GET", GITHUB_FIXER, status=404)
+    hub_router.add("GET", HUB_CHECK, json={"id": CHECK_ID, "provider": "aws"})
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp(
+            "prowler_hub_get_check_fixer",
+            {"provider_id": "aws", "check_id": CHECK_ID},
+        )
+
+    assert result.isError is True
+    message = result.content[0].text
+    assert "normal" in message
+    # The Hub confirmed the check is an aws check, so nothing is left to verify.
+    assert "prowler_hub_get_check_details" not in message
+
+
+async def test_a_check_from_another_provider_names_the_provider_that_has_it(
+    mcp_root_server, hub_router
+):
+    """The ID exists; only the provider is wrong. Saying otherwise sends the
+    caller off to search for an ID they already hold."""
+    hub_router.add("GET", github_check("azure"), status=404)
+    hub_router.add("GET", HUB_CHECK, json={"id": CHECK_ID, "provider": "aws"})
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp(
+            "prowler_hub_get_check_code",
+            {"provider_id": "azure", "check_id": CHECK_ID},
+        )
+
+    assert result.isError is True
+    message = result.content[0].text
+    assert "provider_id='aws'" in message
+    assert "No check with the ID" not in message
+
+
+async def test_a_fixer_from_another_provider_is_not_reported_as_a_missing_fixer(
+    mcp_root_server, hub_router
+):
+    """'That check has no fixer' about a check the provider never had is a lie
+    the caller cannot act on."""
+    hub_router.add("GET", github_check("azure", "_fixer.py"), status=404)
+    hub_router.add("GET", HUB_CHECK, json={"id": CHECK_ID, "provider": "aws"})
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp(
+            "prowler_hub_get_check_fixer",
+            {"provider_id": "azure", "check_id": CHECK_ID},
+        )
+
+    assert result.isError is True
+    message = result.content[0].text
+    assert "provider_id='aws'" in message
+    assert "auto-remediation" not in message
+
+
+async def test_a_check_id_that_exists_nowhere_is_still_reported_as_unknown(
+    mcp_root_server, hub_router
+):
+    """The Hub not having the ID either is the one case that does justify the
+    original message."""
+    hub_router.add("GET", github_check("azure"), status=404)
+    hub_router.add("GET", HUB_CHECK, status=404)
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp(
+            "prowler_hub_get_check_code",
+            {"provider_id": "azure", "check_id": CHECK_ID},
+        )
+
+    assert result.isError is True
+    assert "No check with the ID" in result.content[0].text
+
+
+async def test_an_unreachable_hub_leaves_the_cause_open_rather_than_guessing(
+    mcp_root_server, hub_router
+):
+    """With nothing to distinguish the causes, naming one of them is a guess."""
+    hub_router.add("GET", github_check("azure"), status=404)
+    hub_router.add("GET", HUB_CHECK, status=503)
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp(
+            "prowler_hub_get_check_code",
+            {"provider_id": "azure", "check_id": CHECK_ID},
+        )
+
+    assert result.isError is True
+    message = result.content[0].text
+    assert "No check with the ID" not in message
+    assert "prowler_hub_get_check_details" in message
+
+
+async def test_a_check_code_hit_never_asks_the_hub(mcp_root_server, hub_router):
+    """The Hub lookup exists to explain a 404. On the happy path it is dead
+    weight -- a second round trip for every call that already succeeded."""
+    hub_router.add("GET", GITHUB_CHECK, text="class s3_bucket_public_access: ...")
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool(
+            "prowler_hub_get_check_code",
+            {"provider_id": "aws", "check_id": CHECK_ID},
+        )
+
+    assert "class s3_bucket_public_access" in result.data["content"]
+    assert hub_router.paths() == [f"GET {GITHUB_CHECK}"]
+
+
+async def test_a_hub_outage_is_reported_rather_than_returned_as_an_empty_list(
+    mcp_root_server, hub_router
+):
+    """An empty result set and a failed request are different answers."""
+    hub_router.add("GET", CHECKS, status=500, json={"detail": "boom"})
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp("prowler_hub_list_checks", {})
+
+    assert result.isError is True
+    assert result.structuredContent is None
+
+
+@pytest.mark.parametrize(
+    ("check_id", "routed_as", "sent_as"),
+    [
+        ("../../evil", f"{CHECKS}/../../evil", b"/api/check/..%2F..%2Fevil"),
+        ("s3/../evil", f"{CHECKS}/s3/../evil", b"/api/check/s3%2F..%2Fevil"),
+        ("..", f"{CHECKS}/..", b"/api/check/%2E%2E"),
+        (
+            "s3_x?fields=all",
+            f"{CHECKS}/s3_x?fields=all",
+            b"/api/check/s3_x%3Ffields%3Dall",
+        ),
+        ("s3_x#frag", f"{CHECKS}/s3_x#frag", b"/api/check/s3_x%23frag"),
+    ],
+    ids=["traversal", "mid-path", "dot-segment", "query", "fragment"],
+)
+async def test_an_id_names_a_check_and_cannot_name_an_endpoint(
+    mcp_root_server, hub_router, check_id, routed_as, sent_as
+):
+    """The bug this pins: httpx resolved "../.." away and the request left
+    /api/check for another endpoint of the Hub."""
+    hub_router.add("GET", routed_as, status=404)
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp(
+            "prowler_hub_get_check_details", {"check_id": check_id}
+        )
+
+    assert hub_router.requests[0].url.raw_path == sent_as
+    assert result.isError is True
+    assert "No check with the ID" in result.content[0].text
+
+
+async def test_a_compliance_id_cannot_name_an_endpoint_either(
+    mcp_root_server, hub_router
+):
+    """Every Hub path is built by the same helper, so this holds without its own
+    guard."""
+    hub_router.add("GET", f"{COMPLIANCE}/../../evil", status=404)
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp(
+            "prowler_hub_get_compliance_details", {"compliance_id": "../../evil"}
+        )
+
+    assert hub_router.requests[0].url.raw_path == b"/api/compliance/..%2F..%2Fevil"
+    assert result.isError is True
+    assert "No compliance framework with the ID" in result.content[0].text
+
+
+async def test_a_check_source_url_confines_the_provider_and_the_check_alike(
+    mcp_root_server, hub_router
+):
+    """Both halves of the GitHub raw URL come from the caller, so both are
+    confined."""
+    hub_router.add("GET", github_check("../../../../evil"), status=404)
+    hub_router.add("GET", HUB_CHECK, json={"id": CHECK_ID, "provider": "aws"})
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp(
+            "prowler_hub_get_check_code",
+            {"provider_id": "../../../../evil", "check_id": CHECK_ID},
+        )
+
+    assert (
+        hub_router.requests[0].url.raw_path
+        == github_check("..%2F..%2F..%2F..%2Fevil").encode()
+    )
+    assert result.isError is True
+
+
+async def test_a_hub_body_that_is_not_json_is_not_blamed_on_the_arguments(
+    mcp_root_server, hub_router
+):
+    """An edge answering 200 with an HTML page decodes to the same
+    `JSONDecodeError` a malformed argument does, and the two mean opposite
+    things: nothing in this call can be corrected."""
+    hub_router.add("GET", CHECKS, text="<html><body>502 Bad Gateway</body></html>")
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp("prowler_hub_list_checks", {})
+
+    assert result.isError is True
+    message = result.content[0].text
+    assert "hub.prowler.com" in message
+    assert "could not read as JSON" in message
+    assert "Bad Gateway" not in message
+    assert "Send it as a real object" not in message
+
+
+async def test_an_unreadable_hub_answer_does_not_become_an_unknown_check(
+    mcp_root_server, hub_router
+):
+    """The 404 branch is the only one that may claim the ID does not exist. A
+    body that could not be read says nothing about the ID."""
+    hub_router.add("GET", HUB_CHECK, text="<html>not json</html>")
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp(
+            "prowler_hub_get_check_details", {"check_id": CHECK_ID}
+        )
+
+    assert result.isError is True
+    message = result.content[0].text
+    assert "could not read as JSON" in message
+    assert "No check with the ID" not in message
+
+
+async def test_an_unreadable_hub_answer_leaves_a_missing_check_file_unexplained(
+    mcp_root_server, hub_router
+):
+    """The Hub is asked which provider owns the check. A body it could not read
+    is no more of an answer than an outage, so it hedges the same way."""
+    hub_router.add("GET", github_check("azure"), status=404)
+    hub_router.add("GET", HUB_CHECK, text="<html>not json</html>")
+
+    async with Client(mcp_root_server) as client:
+        result = await client.call_tool_mcp(
+            "prowler_hub_get_check_code",
+            {"provider_id": "azure", "check_id": CHECK_ID},
+        )
+
+    assert result.isError is True
+    message = result.content[0].text
+    assert "No check with the ID" not in message
+    assert "prowler_hub_get_check_details" in message