From fde37a149fc942dd107666441a1989f187d30c3b Mon Sep 17 00:00:00 2001 From: alex s <46074070+bearsyankees@users.noreply.github.com> Date: Tue, 20 Oct 2026 23:36:38 -0400 Subject: [PATCH] fix(reporting): keep HTTP exchange evidence when the proxy is slow to verify ids (#1551) Verifying http_exchange_ids fetched every request with its full raw request and response bodies, serially, and timed out on busy proxy projects; the ids were then dropped and the finding lost its evidence. Check existence with an id-only GraphQL query, retry once, and when the proxy still does not answer keep the ids with a warning instead of discarding them. --- strix/tools/proxy/tools.py | 33 ++++++++++++++----- strix/tools/reporting/tool.py | 14 ++++---- tests/test_proxy_client.py | 60 +++++++++++++++++++++++++--------- tests/test_reporting_fields.py | 17 +++++----- 4 files changed, 85 insertions(+), 39 deletions(-) diff --git a/strix/tools/proxy/tools.py b/strix/tools/proxy/tools.py index d6b09e7e..839b5de8 100644 --- a/strix/tools/proxy/tools.py +++ b/strix/tools/proxy/tools.py @@ -69,6 +69,15 @@ async def _call[T](client: Client, fn: Callable[[Client], Awaitable[T]]) -> T: return await fn(client) +_REQUEST_EXISTS_QUERY = "query RequestExists($id: ID!) { request(id: $id) { id } }" +_REQUEST_EXISTS_ATTEMPTS = 2 + + +async def _request_exists(client: Client, request_id: str) -> bool: + data = await client.graphql.query(_REQUEST_EXISTS_QUERY, {"id": request_id}) + return bool(data) and data.get("request") is not None + + async def existing_request_ids( ctx: RunContextWrapper, request_ids: list[str], @@ -81,16 +90,24 @@ async def existing_request_ids( if client is None: raise RuntimeError("Caido client is not available") - # Request IDs are not an HTTPQL field. Resolve each ID through the same - # project-bound lookup as view_request rather than constructing a filter. + # Request IDs are not an HTTPQL field, so each one is resolved by id. Only + # the id is selected: pulling the raw request and response bodies just to + # learn whether a row exists is what made this time out on busy projects. existing: set[str] = set() for request_id in request_ids: - result = await _call( - client, - functools.partial(caido_api.get_request_with_client, request_id=request_id), - ) - if result is not None: - existing.add(str(result.request.id)) + for attempt in range(1, _REQUEST_EXISTS_ATTEMPTS + 1): + try: + exists = await _call( + client, + functools.partial(_request_exists, request_id=request_id), + ) + except Exception: + if attempt == _REQUEST_EXISTS_ATTEMPTS: + raise + continue + if exists: + existing.add(request_id) + break return existing diff --git a/strix/tools/reporting/tool.py b/strix/tools/reporting/tool.py index 791bae79..3faf7fbd 100644 --- a/strix/tools/reporting/tool.py +++ b/strix/tools/reporting/tool.py @@ -222,9 +222,9 @@ def _normalize_http_exchange_ids(raw: Any) -> tuple[list[str] | None, list[str]] return normalized, errors -_HTTP_EXCHANGE_DROPPED_WARNING = ( - "http_exchange_ids were not stored: the proxy project could not be reached to verify " - "them. Attach them with update_vulnerability_report when the proxy responds again." +_HTTP_EXCHANGE_UNVERIFIED_WARNING = ( + "http_exchange_ids were stored without verification because the proxy project did " + "not respond in time. Ids that do not exist in the project will not render as evidence." ) @@ -235,9 +235,9 @@ async def _verify_http_exchange_ids( """Verify proxy exchange IDs against the current Caido project. IDs the project does not know are rejected. When the proxy itself cannot be - queried the IDs are dropped and a warning is returned instead, so a proxy - outage never blocks a finding and unverified IDs are never recorded as - evidence. + queried the IDs are kept and a warning is returned instead: a slow or + unreachable proxy must never strip evidence from a finding, and consumers + resolve each id against the recorded traffic before rendering it. """ request_ids, errors = _normalize_http_exchange_ids(raw) if request_ids is None or errors or not request_ids: @@ -250,7 +250,7 @@ async def _verify_http_exchange_ids( "Could not verify HTTP exchange IDs against the current Caido project", exc_info=True, ) - return None, [], _HTTP_EXCHANGE_DROPPED_WARNING + return request_ids, [], _HTTP_EXCHANGE_UNVERIFIED_WARNING missing_ids = [request_id for request_id in request_ids if request_id not in existing_ids] if missing_ids: diff --git a/tests/test_proxy_client.py b/tests/test_proxy_client.py index fba20787..d9145ae2 100644 --- a/tests/test_proxy_client.py +++ b/tests/test_proxy_client.py @@ -9,7 +9,6 @@ call at a time against the shared client. from __future__ import annotations import asyncio -from types import SimpleNamespace from typing import TYPE_CHECKING, Any, cast import pytest @@ -27,6 +26,7 @@ class _FakeClient: def __init__(self, name: str) -> None: self.name = name self.closed = False + self.graphql: Any = None async def aclose(self) -> None: self.closed = True @@ -231,20 +231,23 @@ async def test_ctx_client_degrades_when_bootstrap_failed() -> None: assert await tools._ctx_client(cast("Any", _Ctx({"caido_client": handle}))) is None -async def test_existing_request_ids_queries_current_project( - monkeypatch: pytest.MonkeyPatch, -) -> None: +class _FakeGraphQL: + def __init__(self, answers: dict[str, list[Any]]) -> None: + self.answers = answers + self.queries: list[tuple[str, dict[str, Any]]] = [] + + async def query(self, document: str, variables: dict[str, Any]) -> dict[str, Any]: + self.queries.append((document, dict(variables))) + answer = self.answers[variables["id"]].pop(0) + if isinstance(answer, Exception): + raise answer + return {"request": answer} + + +async def test_existing_request_ids_select_only_the_id() -> None: client = _FakeClient("host") - looked_up: list[str] = [] - - async def get_request_with_client(passed_client: Any, request_id: str) -> Any: - assert passed_client is client - looked_up.append(request_id) - if request_id == "1042": - return SimpleNamespace(request=SimpleNamespace(id="1042")) - return None - - monkeypatch.setattr(caido_api, "get_request_with_client", get_request_with_client) + graphql = _FakeGraphQL({"1042": [{"id": "1042"}], "1088": [None]}) + client.graphql = graphql existing = await tools.existing_request_ids( cast("Any", _Ctx({"caido_client": client})), @@ -252,7 +255,34 @@ async def test_existing_request_ids_queries_current_project( ) assert existing == {"1042"} - assert looked_up == ["1042", "1088"] + assert [variables["id"] for _, variables in graphql.queries] == ["1042", "1088"] + for document, _ in graphql.queries: + assert "raw" not in document + assert "{ id }" in document + + +async def test_existing_request_ids_retry_once_before_giving_up() -> None: + client = _FakeClient("host") + graphql = _FakeGraphQL( + { + "1042": [TimeoutError("deadline exceeded"), {"id": "1042"}], + "1088": [TimeoutError("deadline exceeded"), TimeoutError("deadline exceeded")], + } + ) + client.graphql = graphql + + with pytest.raises(TimeoutError): + await tools.existing_request_ids( + cast("Any", _Ctx({"caido_client": client})), + ["1042", "1088"], + ) + + assert [variables["id"] for _, variables in graphql.queries] == [ + "1042", + "1042", + "1088", + "1088", + ] def test_err_logs_caido_errors_as_warnings(caplog: pytest.LogCaptureFixture) -> None: diff --git a/tests/test_reporting_fields.py b/tests/test_reporting_fields.py index 558a2b64..c895847c 100644 --- a/tests/test_reporting_fields.py +++ b/tests/test_reporting_fields.py @@ -1640,7 +1640,7 @@ async def test_http_exchange_ids_must_exist_in_current_proxy_project( assert warning is None -async def test_http_exchange_ids_are_dropped_when_proxy_cannot_be_queried( +async def test_http_exchange_ids_are_kept_when_proxy_cannot_be_queried( monkeypatch: pytest.MonkeyPatch, ) -> None: async def existing_request_ids( @@ -1656,11 +1656,10 @@ async def test_http_exchange_ids_are_dropped_when_proxy_cannot_be_queried( ["1042", "1042", "1088"], ) - assert request_ids is None + assert request_ids == ["1042", "1088"] assert errors == [] assert warning is not None - assert "not stored" in warning - assert "update_vulnerability_report" in warning + assert "without verification" in warning async def test_create_reports_persistence_failure_as_tool_error( @@ -1679,7 +1678,7 @@ async def test_create_reports_persistence_failure_as_tool_error( assert report_state.vulnerability_reports == [] -async def test_evidence_only_update_reports_proxy_outage_as_retryable( +async def test_evidence_only_update_keeps_ids_when_proxy_cannot_be_queried( report_state: ReportState, monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -1712,11 +1711,11 @@ async def test_evidence_only_update_reports_proxy_outage_as_retryable( ) result = json.loads(raw) - assert result["success"] is False - assert "No fields to update" not in result["error"] - assert "update_vulnerability_report" in result["error"] + assert result["success"] is True + assert "without verification" in result["warning"] assert result["report_id"] == "vuln-0009" - assert report_state.vulnerability_reports[0] == original + assert report_state.vulnerability_reports[0]["http_exchange_ids"] == ["204"] + assert report_state.vulnerability_reports[0] != original def test_update_reports_persistence_failure_as_tool_error(report_state: ReportState) -> None: