From e4548cb28c4e7e996d0226a4c8fdb734a180b18c Mon Sep 17 00:00:00 2001 From: "devin-ai-integration[bot]" <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Fri, 17 Jul 2026 13:31:57 -0400 Subject: [PATCH] fix(proxy,tooling): serialize+reconnect Caido client, actionable HTTPQL errors, sandbox tool guidance (#794) * fix(proxy,tooling): serialize+reconnect Caido client, actionable HTTPQL errors, sandbox tool guidance Addresses the top recurring agent tool-call failures observed in telemetry: - proxy: the shared Caido client had no locking or reconnect, so concurrent agent calls raced ("Transport is already connected") and a dead transport poisoned the rest of the run ("Connector is closed"/"Server disconnected"). Add an asyncio lock + bounded reconnect in caido_api.call_with_client (sandbox path) and a scan-wide caido_lock in the run context that host-side proxy tools hold around every call. Deterministic errors are not retried. - proxy: list_requests now returns Caido's exact parser message, echoes the offending query, and includes a corrected-syntax hint so agents self-correct instead of retrying a broken HTTPQL filter. - shell/prompt: document that write_stdin requires a process started with tty=true; nudge toward writing Python to a file over deeply-nested one-liners; note the venv pre-installs common libs. - agent-browser: distinguish daemon/connection failures (run doctor, don't loop) from malformed commands; invoke directly (no sh -c wrapper). - containers: use POSIX '.' instead of the bashism 'source' in generated rc files (fixes 'sh: source: not found'); add file + xxd and pre-install requests/httpx/beautifulsoup4/lxml/pyjwt/cryptography in the sandbox venv. - tests: cover proxy serialization/reconnect/no-retry and HTTPQL errors. * fix(proxy): host-side reconnect, close stale clients, don't retry mutations Addresses Greptile review on the reconnect logic: - Host path had no reconnect: a dead shared context client (Caido restart / network blip) previously disabled proxy tools for the rest of the scan. Add SharedCaidoClient, a serialized reconnect-safe holder stored once per scan in the run context and shared across agents. On a dead transport it rebuilds via reconnect_caido, which re-selects the SAME Caido project (preserving captured traffic) instead of creating a new empty one. - Don't repeat completed mutations: call_with_client / SharedCaidoClient.call take idempotent=. Reads retry once on reconnect; replay + scope create/update/delete heal the client but re-raise instead of risking a double-apply. - Don't leak replaced clients: the stale client is aclose()d (best-effort) on every reconnect. - Extend tests to cover close-on-reconnect, non-idempotent re-raise, and the SharedCaidoClient holder. * fix(proxy): close replacement Caido client when project.select fails Addresses Greptile P1: in reconnect_caido (and bootstrap_caido) a successful connect() followed by a failing project.select()/create() discarded the connected client without closing it, so a missing/unavailable project could leak a transport on every retry. Close the client before re-raising. --------- Co-authored-by: Alex Schapiro --- containers/Dockerfile | 3 + containers/docker-entrypoint.sh | 9 +- strix/agents/prompts/system_prompt.jinja | 19 +- strix/core/runner.py | 4 + strix/runtime/caido_bootstrap.py | 68 ++++- strix/runtime/session_manager.py | 21 +- strix/skills/tooling/agent_browser.md | 17 ++ strix/skills/tooling/python.md | 14 +- strix/tools/proxy/caido_api.py | 231 +++++++++++++--- strix/tools/proxy/tools.py | 169 ++++++++---- strix/tools/shell/README.md | 17 ++ tests/test_proxy_client.py | 321 +++++++++++++++++++++++ 12 files changed, 784 insertions(+), 109 deletions(-) create mode 100644 tests/test_proxy_client.py diff --git a/containers/Dockerfile b/containers/Dockerfile index 16c1c540..4c54ddc2 100644 --- a/containers/Dockerfile +++ b/containers/Dockerfile @@ -24,6 +24,7 @@ RUN apt-get update && \ python3 python3-pip python3-dev python3-venv python3-setuptools \ golang-go \ net-tools dnsutils whois \ + file xxd \ jq parallel ripgrep grep \ less man-db procps htop \ iproute2 iputils-ping netcat-traditional \ @@ -192,6 +193,8 @@ RUN mkdir -p /workspace && chown -R pentester:pentester /workspace /app USER pentester RUN python3 -m venv /app/.venv && \ /app/.venv/bin/pip install --no-cache-dir caido-sdk-client && \ + /app/.venv/bin/pip install --no-cache-dir \ + requests httpx beautifulsoup4 lxml pyjwt cryptography && \ /app/.venv/bin/pip install --no-cache-dir -r /home/pentester/tools/jwt_tool/requirements.txt && \ printf '%s\n' \ '#!/bin/bash' \ diff --git a/containers/docker-entrypoint.sh b/containers/docker-entrypoint.sh index a12390df..d22cefdf 100644 --- a/containers/docker-entrypoint.sh +++ b/containers/docker-entrypoint.sh @@ -91,10 +91,13 @@ http_proxy=http://127.0.0.1:${CAIDO_PORT} https_proxy=http://127.0.0.1:${CAIDO_PORT} EOF -echo "source /etc/profile.d/proxy.sh" >> ~/.bashrc -echo "source /etc/profile.d/proxy.sh" >> ~/.zshrc +# Use POSIX `.` (not the bashism `source`) so these lines are safe when the rc +# files are read by a POSIX shell (e.g. `sh -lc`), which otherwise fails with +# "source: not found". `.` is understood by bash, zsh, and dash alike. +echo ". /etc/profile.d/proxy.sh" >> ~/.bashrc +echo ". /etc/profile.d/proxy.sh" >> ~/.zshrc -source /etc/profile.d/proxy.sh +. /etc/profile.d/proxy.sh echo "✅ System-wide proxy configuration complete" diff --git a/strix/agents/prompts/system_prompt.jinja b/strix/agents/prompts/system_prompt.jinja index e6b81611..fd3b66d6 100644 --- a/strix/agents/prompts/system_prompt.jinja +++ b/strix/agents/prompts/system_prompt.jinja @@ -169,8 +169,23 @@ EFFICIENCY TACTICS: - Run multiple scans in parallel when possible - Load the most relevant skill before starting a specialized testing workflow if doing so will improve accuracy, speed, or tool usage - Use `exec_command` for Python code: write reusable scripts to a file and - run them with `python3`. For one-off snippets, `python3 -c` or a - here-document is acceptable. + run them with `python3 script.py`. For one-off snippets, `python3 -c` or a + here-document is acceptable, but avoid deeply nested quotes/parentheses — if + a snippet needs complex quoting or is more than a few lines, write it to a + file first to prevent syntax errors. +- Before importing a third-party Python library, make sure it is installed. The + sandbox's `python3` runs inside a preconfigured virtualenv that ships + `requests`, `httpx`, `beautifulsoup4` (bs4), `lxml`, `pyjwt`, and + `cryptography`; for anything else prefer the stdlib or run `pip install ` + (it installs into that active venv) before importing, rather than letting the + script fail with `ModuleNotFoundError`. +- `exec_command` runs each command in a fresh non-interactive shell (plain + pipes, no TTY). To drive an interactive or long-running process with + `write_stdin` — REPLs, `ssh`/`nc`/`ftp`, `msfconsole`, or to send Ctrl-C — + you MUST start it with `exec_command(cmd="...", tty=true)` and then + `write_stdin(session_id=, chars="...")`. Calling `write_stdin` on a + default (non-TTY) command or on a process that has already exited fails with + "stdin is not available". - For Caido proxy automation inside Python, explicitly import from `caido_api`: `from caido_api import list_requests, view_request, repeat_request, list_sitemap, view_sitemap_entry, scope_rules` diff --git a/strix/core/runner.py b/strix/core/runner.py index e6a15dc3..1e5d66b4 100644 --- a/strix/core/runner.py +++ b/strix/core/runner.py @@ -282,6 +282,10 @@ async def run_strix_scan( context: dict[str, Any] = { "coordinator": coordinator, "sandbox_session": bundle["session"], + # One ``SharedCaidoClient`` is reused by every agent in the scan + # (child contexts are shallow copies via ``dict(parent_ctx)``). It + # serializes access to the non-concurrency-safe GraphQL transport + # and rebuilds it if it dies mid-scan. "caido_client": bundle["caido_client"], "agent_id": root_id, "parent_id": None, diff --git a/strix/runtime/caido_bootstrap.py b/strix/runtime/caido_bootstrap.py index 7057d228..ae81f7f3 100644 --- a/strix/runtime/caido_bootstrap.py +++ b/strix/runtime/caido_bootstrap.py @@ -10,6 +10,7 @@ exposed-port URL for all subsequent SDK calls. from __future__ import annotations import asyncio +import contextlib import json import logging from typing import TYPE_CHECKING @@ -79,23 +80,72 @@ async def _login_as_guest( raise RuntimeError(f"loginAsGuest failed after {attempts} attempts: {last_err}") -async def bootstrap_caido( +async def _aclose_quietly(client: Client) -> None: + """Best-effort close of a client whose setup failed; never raises.""" + with contextlib.suppress(Exception): + await client.aclose() + + +async def _connect_client( session: BaseSandboxSession, *, host_url: str, container_url: str, ) -> Client: - """Connect to the in-container Caido sidecar and select a fresh project.""" - logger.info("Bootstrapping Caido client (host=%s, container=%s)", host_url, container_url) - access_token = await _login_as_guest(session, container_url=container_url) - client = Client(host_url, auth=TokenAuthOptions(token=access_token)) await client.connect() + return client - project = await client.project.create( - CreateProjectOptions(name="sandbox", temporary=True), - ) - await client.project.select(project.id) + +async def bootstrap_caido( + session: BaseSandboxSession, + *, + host_url: str, + container_url: str, +) -> tuple[Client, str]: + """Connect to the in-container Caido sidecar and select a fresh project. + + Returns the connected client and the id of the temporary project it + selected. The project id lets :func:`reconnect_caido` rebuild a dead + transport while staying on the *same* project (and its captured traffic) + instead of creating a new empty one. + """ + logger.info("Bootstrapping Caido client (host=%s, container=%s)", host_url, container_url) + + client = await _connect_client(session, host_url=host_url, container_url=container_url) + try: + project = await client.project.create( + CreateProjectOptions(name="sandbox", temporary=True), + ) + await client.project.select(project.id) + except BaseException: + # Don't leak the connected transport if project setup fails. + await _aclose_quietly(client) + raise logger.info("Caido project selected: %s", project.id) + return client, str(project.id) + + +async def reconnect_caido( + session: BaseSandboxSession, + *, + host_url: str, + container_url: str, + project_id: str, +) -> Client: + """Rebuild a Caido client after its transport died, keeping the project. + + Re-authenticates, reconnects, and re-selects the existing project so the + caller keeps access to the traffic captured before the disconnect. + """ + logger.info("Reconnecting Caido client (host=%s, project=%s)", host_url, project_id) + client = await _connect_client(session, host_url=host_url, container_url=container_url) + try: + await client.project.select(project_id) + except BaseException: + # A missing/unavailable project must not leave the freshly-connected + # transport dangling — otherwise every retry leaks another one. + await _aclose_quietly(client) + raise return client diff --git a/strix/runtime/session_manager.py b/strix/runtime/session_manager.py index cdc6d955..84950666 100644 --- a/strix/runtime/session_manager.py +++ b/strix/runtime/session_manager.py @@ -5,15 +5,20 @@ from __future__ import annotations import logging import shutil from pathlib import Path -from typing import Any +from typing import TYPE_CHECKING, Any from agents.sandbox.entries import BaseEntry, LocalDir from agents.sandbox.manifest import Environment, Manifest from strix.config import load_settings from strix.runtime.backends import get_backend -from strix.runtime.caido_bootstrap import bootstrap_caido +from strix.runtime.caido_bootstrap import bootstrap_caido, reconnect_caido from strix.runtime.local_dir_staging import stage_symlink_safe_dir +from strix.tools.proxy.caido_api import SharedCaidoClient + + +if TYPE_CHECKING: + from caido_sdk_client import Client as CaidoClient logger = logging.getLogger(__name__) @@ -131,16 +136,24 @@ async def create_or_reuse( host_caido_url = f"{scheme}://{caido_endpoint.host}:{caido_endpoint.port}" logger.debug("Caido host endpoint resolved: %s", host_caido_url) - caido_client = await bootstrap_caido( + caido_client, caido_project_id = await bootstrap_caido( session, host_url=host_caido_url, container_url=container_caido_url, ) + async def _reconnect_caido() -> CaidoClient: + return await reconnect_caido( + session, + host_url=host_caido_url, + container_url=container_caido_url, + project_id=caido_project_id, + ) + bundle = { "client": client, "session": session, - "caido_client": caido_client, + "caido_client": SharedCaidoClient(caido_client, _reconnect_caido), } _SESSION_CACHE[scan_id] = bundle logger.info("Sandbox session for scan %s ready and cached", scan_id) diff --git a/strix/skills/tooling/agent_browser.md b/strix/skills/tooling/agent_browser.md index 9ef810d6..db074e86 100644 --- a/strix/skills/tooling/agent_browser.md +++ b/strix/skills/tooling/agent_browser.md @@ -365,6 +365,23 @@ agent-browser dialog accept "text" # accept with prompt input agent-browser dialog dismiss # cancel ``` +## Readiness & recovery + +The first `agent-browser open` in a session launches the headless-Chrome +daemon; later commands reuse it. Distinguish the two failure modes and react +differently — do **not** blindly re-run the same failing command in a loop: + +- **Daemon / connection failure** (`Failed to connect`, `connection refused`, + socket missing, `browser not running`): the daemon isn't up or has died. Run + `agent-browser doctor` (add `--fix` if it reports repairable problems), then + re-open the page. Retrying the original command unchanged will keep failing. +- **Malformed command** (`Unknown command`, `Ref not found`, bad flag): fix the + command itself — re-snapshot for fresh refs, or correct the syntax. + +Invoke `agent-browser` directly through `exec_command`; there is no need to wrap +it in an extra `sh -c "..."` / `bash -lc "..."` layer, which only adds shell +quoting and startup-file pitfalls. + ## Diagnosing install issues If a command fails unexpectedly (`Unknown command`, `Failed to connect`, diff --git a/strix/skills/tooling/python.md b/strix/skills/tooling/python.md index d80aa69f..85a53d9f 100644 --- a/strix/skills/tooling/python.md +++ b/strix/skills/tooling/python.md @@ -92,10 +92,18 @@ For iterative exploit work, put code in a file: ## Installing extra packages -The sandbox's Python lives in `/app/.venv`. To add a one-off dependency -for an exploit script, use `uv` (already in the image and much faster -than pip): +The sandbox's Python lives in `/app/.venv`, and it is the active virtualenv +(`python3` / `pip` already resolve to it). The following common libraries are +**pre-installed** — import them directly, no install step needed: +`requests`, `httpx`, `beautifulsoup4` (`bs4`), `lxml`, `pyjwt` (`jwt`), +`cryptography`. + +To add a one-off dependency for an exploit script, use `uv` (already in the +image and much faster than pip): ```bash uv pip install --python /app/.venv/bin/python ``` + +Plain `pip install ` also works because the venv is active. Install +before you import, so scripts don't fail with `ModuleNotFoundError`. diff --git a/strix/tools/proxy/caido_api.py b/strix/tools/proxy/caido_api.py index 926d5c74..11e26859 100644 --- a/strix/tools/proxy/caido_api.py +++ b/strix/tools/proxy/caido_api.py @@ -3,7 +3,9 @@ from __future__ import annotations import asyncio +import contextlib import json +import logging import os import time import urllib.request @@ -21,9 +23,14 @@ from caido_sdk_client.types import ( if TYPE_CHECKING: + from collections.abc import Awaitable, Callable + from caido_sdk_client import Client as CaidoClient +logger = logging.getLogger(__name__) + + RequestPart = Literal["request", "response"] SortBy = Literal[ "timestamp", @@ -42,6 +49,19 @@ _SITEMAP_PAGE_SIZE = 30 _DEFAULT_CAIDO_URL = "http://127.0.0.1:48080" _CLIENT_CACHE: dict[str, Client] = {} +_CLIENT_LOCK = asyncio.Lock() + +# Substrings that mean the shared client's transport has died or is being used +# concurrently — recoverable by rebuilding the client and retrying once. +_CONNECTION_ERROR_MARKERS = ( + "transport is already connected", + "connector is closed", + "server disconnected", + "session is closed", + "cannot write to closing transport", + "connection reset", + "connection closed", +) _REQ_FIELD_MAP: dict[SortBy, tuple[str, str]] = { "timestamp": ("req", "created_at"), "host": ("req", "host"), @@ -81,19 +101,116 @@ def _login_as_guest() -> str: return str(payload["data"]["loginAsGuest"]["token"]["accessToken"]) -async def get_client() -> Client: - if client := _CLIENT_CACHE.get("default"): - return client - +async def _new_client() -> Client: token = await asyncio.to_thread(_login_as_guest) client = Client(caido_url(), auth=TokenAuthOptions(token=token)) await client.connect() - _CLIENT_CACHE["default"] = client return client +async def _safe_aclose(client: Client | None) -> None: + """Close a (possibly dead) client without letting teardown errors escape.""" + if client is None: + return + with contextlib.suppress(Exception): + await client.aclose() + + +def _is_connection_error(exc: BaseException) -> bool: + message = str(exc).lower() + if any(marker in message for marker in _CONNECTION_ERROR_MARKERS): + return True + cause = exc.__cause__ or exc.__context__ + return cause is not None and cause is not exc and _is_connection_error(cause) + + +async def get_client() -> Client: + """Return the shared Caido client, creating it under a lock if needed. + + The lock prevents two concurrent callers from each building a client and + racing ``connect()`` on the same transport ("Transport is already + connected"). + """ + async with _CLIENT_LOCK: + client = _CLIENT_CACHE.get("default") + if client is None: + client = await _new_client() + _CLIENT_CACHE["default"] = client + return client + + +async def call_with_client[T]( + fn: Callable[[Client], Awaitable[T]], *, idempotent: bool = True +) -> T: + """Run ``fn`` against the shared client, serialized and reconnect-safe. + + The Caido GraphQL transport is not safe for concurrent use: two in-flight + requests race and raise "Transport is already connected". All proxy calls + are therefore serialized through ``_CLIENT_LOCK``. If the cached client's + transport has since died ("Connector is closed" / "Server disconnected"), + the stale client is closed and rebuilt so subsequent calls stop failing + against a dead client. + + ``fn`` is only re-run automatically when ``idempotent`` is true. For + mutations (replay, scope create/update/delete) a connection error may + arrive *after* Caido applied the change, so we heal the client for future + calls but re-raise instead of risking a double-apply. + """ + async with _CLIENT_LOCK: + client = _CLIENT_CACHE.get("default") + if client is None: + client = await _new_client() + _CLIENT_CACHE["default"] = client + try: + return await fn(client) + except Exception as exc: + if not _is_connection_error(exc): + raise + new_client = await _new_client() + _CLIENT_CACHE["default"] = new_client + await _safe_aclose(client) + if not idempotent: + raise + return await fn(new_client) + + +class SharedCaidoClient: + """Serialized, reconnect-safe wrapper around one host-side Caido client. + + Every agent in a scan shares a single instance (propagated through the + shallow-copied run context). ``call`` serializes access — the SDK transport + is not concurrency-safe — and, when the transport dies, rebuilds the client + via ``reconnect`` (which preserves the Caido project) and closes the dead + one, so a transient Caido restart no longer disables proxy tools for the + rest of the scan. + """ + + def __init__(self, client: Client, reconnect: Callable[[], Awaitable[Client]]) -> None: + self._client = client + self._reconnect = reconnect + self._lock = asyncio.Lock() + + async def call[T](self, fn: Callable[[Client], Awaitable[T]], *, idempotent: bool = True) -> T: + async with self._lock: + try: + return await fn(self._client) + except Exception as exc: + if not _is_connection_error(exc): + raise + dead, self._client = self._client, await self._reconnect() + await _safe_aclose(dead) + if not idempotent: + raise + return await fn(self._client) + + async def aclose(self) -> None: + async with self._lock: + await _safe_aclose(self._client) + + async def close_client() -> None: - client = _CLIENT_CACHE.pop("default", None) + async with _CLIENT_LOCK: + client = _CLIENT_CACHE.pop("default", None) if client is None: return await client.aclose() @@ -385,19 +502,23 @@ async def list_requests( sort_order: SortOrder = "desc", scope_id: str | None = None, ) -> Any: - return await list_requests_with_client( - await get_client(), - httpql_filter=httpql_filter, - first=first, - after=after, - sort_by=sort_by, - sort_order=sort_order, - scope_id=scope_id, + return await call_with_client( + lambda client: list_requests_with_client( + client, + httpql_filter=httpql_filter, + first=first, + after=after, + sort_by=sort_by, + sort_order=sort_order, + scope_id=scope_id, + ) ) async def view_request(request_id: str, *, part: RequestPart = "request") -> Any: - return await get_request_with_client(await get_client(), request_id, part=part) + return await call_with_client( + lambda client: get_request_with_client(client, request_id, part=part) + ) async def repeat_request( @@ -406,22 +527,28 @@ async def repeat_request( modifications: dict[str, Any] | None = None, ) -> dict[str, Any]: mods = modifications or {} - result = await get_request_with_client(await get_client(), request_id, part="request") - if result is None or result.request.raw is None: - raise ValueError(f"Request {request_id} not found") - original = result.request - raw_str = result.request.raw.decode("utf-8", errors="replace") - components = parse_raw_request(raw_str) - full_url = full_url_from_components(original, components, mods) - modified = apply_modifications(components, mods, full_url) - connection, raw = build_raw_request( - method=modified["method"], - url=modified["url"], - headers=modified["headers"], - body=modified["body"], - ) - return await replay_send_raw(await get_client(), raw=raw, connection=connection) + async def _run(client: CaidoClient) -> dict[str, Any]: + result = await get_request_with_client(client, request_id, part="request") + if result is None or result.request.raw is None: + raise ValueError(f"Request {request_id} not found") + + original = result.request + raw_str = result.request.raw.decode("utf-8", errors="replace") + components = parse_raw_request(raw_str) + full_url = full_url_from_components(original, components, mods) + modified = apply_modifications(components, mods, full_url) + connection, raw = build_raw_request( + method=modified["method"], + url=modified["url"], + headers=modified["headers"], + body=modified["body"], + ) + return await replay_send_raw(client, raw=raw, connection=connection) + + # A replay mutates server state; don't auto-retry if the transport dies + # mid-send (the request may already have been sent). + return await call_with_client(_run, idempotent=False) async def scope_rules( @@ -432,7 +559,29 @@ async def scope_rules( scope_id: str | None = None, scope_name: str | None = None, ) -> Any: - client = await get_client() + async def _run(client: CaidoClient) -> Any: + return await _scope_rules_with_client( + client, + action, + allowlist=allowlist, + denylist=denylist, + scope_id=scope_id, + scope_name=scope_name, + ) + + # get/list are read-only and safe to retry; create/update/delete mutate. + return await call_with_client(_run, idempotent=action in {"get", "list"}) + + +async def _scope_rules_with_client( + client: CaidoClient, + action: ScopeAction, + *, + allowlist: list[str] | None = None, + denylist: list[str] | None = None, + scope_id: str | None = None, + scope_name: str | None = None, +) -> Any: if action == "list": result = await scope_list(client) elif action == "get": @@ -651,26 +800,30 @@ async def list_sitemap( page: int = 1, page_size: int = _SITEMAP_PAGE_SIZE, ) -> dict[str, Any]: - return await list_sitemap_with_client( - await get_client(), - scope_id=scope_id, - parent_id=parent_id, - depth=depth, - page=page, - page_size=page_size, + return await call_with_client( + lambda client: list_sitemap_with_client( + client, + scope_id=scope_id, + parent_id=parent_id, + depth=depth, + page=page, + page_size=page_size, + ) ) async def view_sitemap_entry(entry_id: str) -> dict[str, Any]: - return await view_sitemap_entry_with_client(await get_client(), entry_id) + return await call_with_client(lambda client: view_sitemap_entry_with_client(client, entry_id)) __all__ = [ "RequestPart", "ScopeAction", + "SharedCaidoClient", "SitemapDepth", "SortBy", "SortOrder", + "call_with_client", "close_client", "get_client", "list_requests", diff --git a/strix/tools/proxy/tools.py b/strix/tools/proxy/tools.py index db8a6b24..4fdb850a 100644 --- a/strix/tools/proxy/tools.py +++ b/strix/tools/proxy/tools.py @@ -13,6 +13,7 @@ from typing import TYPE_CHECKING, Any, Literal from agents import RunContextWrapper, function_tool from strix.tools.proxy import caido_api +from strix.tools.proxy.caido_api import SharedCaidoClient logger = logging.getLogger(__name__) @@ -28,7 +29,7 @@ if TYPE_CHECKING: SortOrder, ) else: - from strix.tools.proxy.caido_api import ( # noqa: TC001 + from strix.tools.proxy.caido_api import ( RequestPart, SitemapDepth, SortBy, @@ -39,9 +40,18 @@ else: ScopeAction = Literal["get", "list", "create", "update", "delete"] -def _ctx_client(ctx: RunContextWrapper) -> Client | None: +def _ctx_proxy(ctx: RunContextWrapper) -> SharedCaidoClient | None: + """Return the scan-wide serialized, reconnect-safe Caido client holder. + + All agents in a scan share one :class:`SharedCaidoClient` whose GraphQL + transport is not concurrency-safe (parallel calls raise "Transport is + already connected"). ``SharedCaidoClient.call`` serializes access and + rebuilds the transport if it dies mid-scan. Returns ``None`` when no holder + is present (e.g. standalone tool invocation outside a scan run). + """ inner = ctx.context if isinstance(ctx.context, dict) else {} - return inner.get("caido_client") + proxy = inner.get("caido_client") + return proxy if isinstance(proxy, SharedCaidoClient) else None def _to_tool_json(value: Any) -> Any: @@ -83,6 +93,39 @@ def _err(name: str, exc: Exception) -> str: ) +_HTTPQL_HINT = ( + "HTTPQL syntax: quote string values and leave integers unquoted; combine " + "terms with AND / OR (there is no NOT). Numeric fields (resp.code, req.port, " + "id, roundtrip) use eq/ne/gt/gte/lt/lte; text/byte fields (req.host, req.path, " + "req.method, req.raw, resp.raw) use cont/ncont/eq/ne/like/nlike/regex/nregex. " + "Example: 'resp.code.gte:200 AND resp.code.lt:300 AND req.host.cont:\"api\"'." +) + + +def _is_httpql_error(exc: Exception) -> bool: + message = str(exc).lower() + return "httpql" in message or ("filter" in message and "pars" in message) + + +def _httpql_error(exc: Exception, httpql_filter: str | None) -> str: + """Return an actionable error for a rejected HTTPQL filter. + + Preserves Caido's exact parser message and echoes the offending query so + the agent can self-correct instead of retrying the same broken filter. + """ + logger.info("list_requests rejected HTTPQL filter %r: %s", httpql_filter, exc) + return json.dumps( + { + "success": False, + "error": f"Invalid HTTPQL filter: {exc}", + "httpql_filter": httpql_filter, + "hint": _HTTPQL_HINT, + }, + ensure_ascii=False, + default=str, + ) + + @function_tool(timeout=120) async def list_requests( ctx: RunContextWrapper, @@ -141,19 +184,21 @@ async def list_requests( sort_order: ``asc`` or ``desc``. scope_id: Restrict to a Caido scope (managed via ``scope_rules``). """ - client = _ctx_client(ctx) - if client is None: + proxy = _ctx_proxy(ctx) + if proxy is None: return _no_client() try: - connection = await caido_api.list_requests_with_client( - client, - httpql_filter=httpql_filter, - first=first, - after=after, - sort_by=sort_by, - sort_order=sort_order, - scope_id=scope_id, + connection = await proxy.call( + lambda client: caido_api.list_requests_with_client( + client, + httpql_filter=httpql_filter, + first=first, + after=after, + sort_by=sort_by, + sort_order=sort_order, + scope_id=scope_id, + ) ) entries = [] @@ -207,6 +252,8 @@ async def list_requests( default=str, ) except Exception as exc: # noqa: BLE001 + if httpql_filter and _is_httpql_error(exc): + return _httpql_error(exc, httpql_filter) return _err("list_requests", exc) @@ -244,12 +291,14 @@ async def view_request( page: 1-indexed page number (only when no ``search_pattern``). page_size: Lines per page. """ - client = _ctx_client(ctx) - if client is None: + proxy = _ctx_proxy(ctx) + if proxy is None: return _no_client() try: - result = await caido_api.get_request_with_client(client, request_id, part=part) + result = await proxy.call( + lambda client: caido_api.get_request_with_client(client, request_id, part=part) + ) if result is None: return json.dumps( {"success": False, "error": f"Request {request_id} not found"}, @@ -359,20 +408,15 @@ async def repeat_request( - ``body`` — replace the body string entirely. - ``cookies`` — dict of cookies to add/update. """ - client = _ctx_client(ctx) - if client is None: + proxy = _ctx_proxy(ctx) + if proxy is None: return _no_client() mods = modifications or {} - try: + async def _do(client: Client) -> dict[str, Any] | None: result = await caido_api.get_request_with_client(client, request_id, part="request") if result is None or result.request.raw is None: - return json.dumps( - {"success": False, "error": f"Request {request_id} not found"}, - ensure_ascii=False, - default=str, - ) - + return None original = result.request raw_str = result.request.raw.decode("utf-8", errors="replace") components = caido_api.parse_raw_request(raw_str) @@ -384,7 +428,18 @@ async def repeat_request( headers=modified["headers"], body=modified["body"], ) - replay = await caido_api.replay_send_raw(client, raw=raw, connection=connection) + return await caido_api.replay_send_raw(client, raw=raw, connection=connection) + + try: + # A replay mutates target state, so don't auto-retry on a mid-send + # transport failure (the request may already have been sent). + replay = await proxy.call(_do, idempotent=False) + if replay is None: + return json.dumps( + {"success": False, "error": f"Request {request_id} not found"}, + ensure_ascii=False, + default=str, + ) return _format_replay_tool_result(replay) except Exception as exc: # noqa: BLE001 return _err("repeat_request", exc) @@ -437,16 +492,18 @@ async def list_sitemap( (recursive subtree). Only meaningful with ``parent_id``. page: 1-indexed page (30 entries per page). """ - client = _ctx_client(ctx) - if client is None: + proxy = _ctx_proxy(ctx) + if proxy is None: return _no_client() try: - payload = await caido_api.list_sitemap_with_client( - client, - scope_id=scope_id, - parent_id=parent_id, - depth=depth, - page=page, + payload = await proxy.call( + lambda client: caido_api.list_sitemap_with_client( + client, + scope_id=scope_id, + parent_id=parent_id, + depth=depth, + page=page, + ) ) return json.dumps(payload, ensure_ascii=False, default=str) except Exception as exc: # noqa: BLE001 @@ -468,11 +525,13 @@ async def view_sitemap_entry( Args: entry_id: ID from ``list_sitemap`` (or any nested entry). """ - client = _ctx_client(ctx) - if client is None: + proxy = _ctx_proxy(ctx) + if proxy is None: return _no_client() try: - payload = await caido_api.view_sitemap_entry_with_client(client, entry_id) + payload = await proxy.call( + lambda client: caido_api.view_sitemap_entry_with_client(client, entry_id) + ) return json.dumps(payload, ensure_ascii=False, default=str) except Exception as exc: # noqa: BLE001 return _err("view_sitemap_entry", exc) @@ -524,13 +583,13 @@ async def scope_rules( scope_id: Required for ``get`` / ``update`` / ``delete``. scope_name: Required for ``create`` / ``update``. """ - client = _ctx_client(ctx) - if client is None: + proxy = _ctx_proxy(ctx) + if proxy is None: return _no_client() try: if action == "list": - scopes = await caido_api.scope_list(client) + scopes = await proxy.call(caido_api.scope_list) return json.dumps( {"success": True, "scopes": [_to_tool_json(s) for s in scopes]}, ensure_ascii=False, @@ -543,9 +602,11 @@ async def scope_rules( ensure_ascii=False, default=str, ) - scope = await caido_api.scope_get(client, scope_id) + scope = await proxy.call(lambda client: caido_api.scope_get(client, scope_id)) return json.dumps( - {"success": True, "scope": _to_tool_json(scope)}, ensure_ascii=False, default=str + {"success": True, "scope": _to_tool_json(scope)}, + ensure_ascii=False, + default=str, ) if action == "create": if not scope_name: @@ -554,11 +615,16 @@ async def scope_rules( ensure_ascii=False, default=str, ) - scope = await caido_api.scope_create( - client, name=scope_name, allowlist=allowlist, denylist=denylist + scope = await proxy.call( + lambda client: caido_api.scope_create( + client, name=scope_name, allowlist=allowlist, denylist=denylist + ), + idempotent=False, ) return json.dumps( - {"success": True, "scope": _to_tool_json(scope)}, ensure_ascii=False, default=str + {"success": True, "scope": _to_tool_json(scope)}, + ensure_ascii=False, + default=str, ) if action == "update": if not scope_id or not scope_name: @@ -570,11 +636,16 @@ async def scope_rules( ensure_ascii=False, default=str, ) - scope = await caido_api.scope_update( - client, scope_id, name=scope_name, allowlist=allowlist, denylist=denylist + scope = await proxy.call( + lambda client: caido_api.scope_update( + client, scope_id, name=scope_name, allowlist=allowlist, denylist=denylist + ), + idempotent=False, ) return json.dumps( - {"success": True, "scope": _to_tool_json(scope)}, ensure_ascii=False, default=str + {"success": True, "scope": _to_tool_json(scope)}, + ensure_ascii=False, + default=str, ) if not scope_id: return json.dumps( @@ -582,7 +653,7 @@ async def scope_rules( ensure_ascii=False, default=str, ) - await caido_api.scope_delete(client, scope_id) + await proxy.call(lambda client: caido_api.scope_delete(client, scope_id), idempotent=False) return json.dumps( { "success": True, diff --git a/strix/tools/shell/README.md b/strix/tools/shell/README.md index 204c9256..b842e317 100644 --- a/strix/tools/shell/README.md +++ b/strix/tools/shell/README.md @@ -5,6 +5,23 @@ invocation the agent makes (nmap, ffuf, agent-browser, python3, …) goes through `exec_command`. `write_stdin` streams input to a still-running process started by an earlier `exec_command` (for interactive prompts). +## `write_stdin` requires a TTY-backed process + +`exec_command` runs each command in a fresh **non-interactive** shell (plain +pipes, no TTY) by default. `write_stdin` only works against a process that is +still running **and** was started with a PTY. The canonical sequence is: + +```text +exec_command(cmd="python3", tty=true) # start a PTY-backed process +write_stdin(session_id=, chars="print(1)\n") +``` + +Calling `write_stdin` on a command started with the default `tty=false`, or on +a process that has already exited, fails with +`stdin is not available for this process. Start the command with 'tty=true' in +'exec_command' before using 'write_stdin'.` Use `tty=true` for REPLs, +`ssh`/`nc`/`ftp`, `msfconsole`, or to deliver a Ctrl-C to a long-running job. + - **Implementation:** `agents.sandbox.capabilities.tools.shell_tool.ShellTool` (in the upstream `agents` SDK) - **Wired in:** `strix/agents/factory.py` — added per-run via the SDK diff --git a/tests/test_proxy_client.py b/tests/test_proxy_client.py new file mode 100644 index 00000000..64436583 --- /dev/null +++ b/tests/test_proxy_client.py @@ -0,0 +1,321 @@ +"""Tests for the shared Caido client lifecycle and proxy error handling. + +Covers the concurrency/reconnect guarantees of ``caido_api.call_with_client`` +(the sandbox-imported path) and ``caido_api.SharedCaidoClient`` (the host-side +holder), plus the actionable HTTPQL errors in ``proxy.tools``. +""" + +from __future__ import annotations + +import asyncio +import json +from typing import TYPE_CHECKING, Any, cast + +import pytest + +from strix.tools.proxy import caido_api, tools +from strix.tools.proxy.caido_api import SharedCaidoClient + + +if TYPE_CHECKING: + from collections.abc import Iterator + + +class _FakeClient: + def __init__(self, name: str) -> None: + self.name = name + self.closed = False + + async def aclose(self) -> None: + self.closed = True + + +@pytest.fixture(autouse=True) +def _clear_cache() -> Iterator[None]: + caido_api._CLIENT_CACHE.clear() + yield + caido_api._CLIENT_CACHE.clear() + + +async def test_call_with_client_reuses_cached_client(monkeypatch: pytest.MonkeyPatch) -> None: + cached = _FakeClient("cached") + caido_api._CLIENT_CACHE["default"] = cast("Any", cached) + + async def _new() -> Any: + raise AssertionError("_new_client must not run when a client is cached") + + monkeypatch.setattr(caido_api, "_new_client", _new) + + seen: dict[str, Any] = {} + + async def fn(client: Any) -> str: + seen["client"] = client + return "ok" + + assert await caido_api.call_with_client(fn) == "ok" + assert seen["client"] is cached + + +async def test_call_with_client_creates_and_caches_when_empty( + monkeypatch: pytest.MonkeyPatch, +) -> None: + created = _FakeClient("fresh") + + async def _new() -> Any: + return created + + monkeypatch.setattr(caido_api, "_new_client", _new) + + seen: dict[str, Any] = {} + + async def fn(client: Any) -> str: + seen["client"] = client + return "ok" + + assert await caido_api.call_with_client(fn) == "ok" + assert seen["client"] is created + assert caido_api._CLIENT_CACHE["default"] is created + + +async def test_failed_init_does_not_poison_cache(monkeypatch: pytest.MonkeyPatch) -> None: + async def _new() -> Any: + raise ConnectionRefusedError("caido not up yet") + + monkeypatch.setattr(caido_api, "_new_client", _new) + + async def fn(_client: Any) -> str: + return "unreachable" + + with pytest.raises(ConnectionRefusedError): + await caido_api.call_with_client(fn) + assert "default" not in caido_api._CLIENT_CACHE + + +async def test_call_with_client_reconnects_and_closes_dead_transport( + monkeypatch: pytest.MonkeyPatch, +) -> None: + dead = _FakeClient("dead") + fresh = _FakeClient("fresh") + caido_api._CLIENT_CACHE["default"] = cast("Any", dead) + + new_calls = {"n": 0} + + async def _new() -> Any: + new_calls["n"] += 1 + return fresh + + monkeypatch.setattr(caido_api, "_new_client", _new) + + attempts: list[Any] = [] + + async def fn(client: Any) -> str: + attempts.append(client) + if len(attempts) == 1: + raise RuntimeError("Transport is already connected") + return "ok" + + assert await caido_api.call_with_client(fn) == "ok" + assert attempts == [dead, fresh] + assert new_calls["n"] == 1 + assert caido_api._CLIENT_CACHE["default"] is fresh + assert dead.closed is True # stale transport is not leaked + + +async def test_call_with_client_non_idempotent_rebuilds_but_reraises( + monkeypatch: pytest.MonkeyPatch, +) -> None: + dead = _FakeClient("dead") + fresh = _FakeClient("fresh") + caido_api._CLIENT_CACHE["default"] = cast("Any", dead) + + async def _new() -> Any: + return fresh + + monkeypatch.setattr(caido_api, "_new_client", _new) + + calls = {"n": 0} + + async def fn(_client: Any) -> str: + calls["n"] += 1 + raise RuntimeError("Server disconnected") + + # A mutation must not be auto-retried (it may already have applied), but the + # dead client is still healed so later calls succeed. + with pytest.raises(RuntimeError, match="Server disconnected"): + await caido_api.call_with_client(fn, idempotent=False) + assert calls["n"] == 1 + assert caido_api._CLIENT_CACHE["default"] is fresh + assert dead.closed is True + + +async def test_call_with_client_does_not_retry_application_errors( + monkeypatch: pytest.MonkeyPatch, +) -> None: + cached = _FakeClient("cached") + caido_api._CLIENT_CACHE["default"] = cast("Any", cached) + + async def _new() -> Any: + raise AssertionError("deterministic errors must not trigger a reconnect") + + monkeypatch.setattr(caido_api, "_new_client", _new) + + calls = {"n": 0} + + async def fn(_client: Any) -> str: + calls["n"] += 1 + raise ValueError("Invalid HTTPQL filter") + + with pytest.raises(ValueError, match="Invalid HTTPQL"): + await caido_api.call_with_client(fn) + assert calls["n"] == 1 + assert caido_api._CLIENT_CACHE["default"] is cached + + +async def test_call_with_client_serializes_concurrent_calls( + monkeypatch: pytest.MonkeyPatch, +) -> None: + caido_api._CLIENT_CACHE["default"] = cast("Any", _FakeClient("shared")) + + async def _new() -> Any: + raise AssertionError("no reconnect expected") + + monkeypatch.setattr(caido_api, "_new_client", _new) + + state = {"active": 0, "max": 0} + + async def fn(_client: Any) -> str: + state["active"] += 1 + state["max"] = max(state["max"], state["active"]) + await asyncio.sleep(0.01) + state["active"] -= 1 + return "ok" + + await asyncio.gather(*(caido_api.call_with_client(fn) for _ in range(6))) + assert state["max"] == 1 + + +async def test_shared_client_reconnects_and_closes_dead_transport() -> None: + dead = _FakeClient("dead") + fresh = _FakeClient("fresh") + + async def _reconnect() -> Any: + return fresh + + holder = SharedCaidoClient(cast("Any", dead), _reconnect) + + attempts: list[Any] = [] + + async def fn(client: Any) -> str: + attempts.append(client) + if len(attempts) == 1: + raise RuntimeError("Connector is closed") + return "ok" + + assert await holder.call(fn) == "ok" + assert attempts == [dead, fresh] + assert dead.closed is True + + +async def test_shared_client_non_idempotent_rebuilds_but_reraises() -> None: + dead = _FakeClient("dead") + fresh = _FakeClient("fresh") + + async def _reconnect() -> Any: + return fresh + + holder = SharedCaidoClient(cast("Any", dead), _reconnect) + + calls = {"n": 0} + + async def fn(_client: Any) -> str: + calls["n"] += 1 + raise RuntimeError("Server disconnected") + + with pytest.raises(RuntimeError, match="Server disconnected"): + await holder.call(fn, idempotent=False) + assert calls["n"] == 1 + assert dead.closed is True + # The healthy client remains for the next call. + assert await holder.call(lambda _c: _ok()) == "ok" + + +async def _ok() -> str: + return "ok" + + +async def test_shared_client_serializes_concurrent_calls() -> None: + async def _reconnect() -> Any: + raise AssertionError("no reconnect expected") + + holder = SharedCaidoClient(cast("Any", _FakeClient("shared")), _reconnect) + + state = {"active": 0, "max": 0} + + async def fn(_client: Any) -> str: + state["active"] += 1 + state["max"] = max(state["max"], state["active"]) + await asyncio.sleep(0.01) + state["active"] -= 1 + return "ok" + + await asyncio.gather(*(holder.call(fn) for _ in range(6))) + assert state["max"] == 1 + + +async def test_shared_client_passes_through_application_errors() -> None: + async def _reconnect() -> Any: + raise AssertionError("deterministic errors must not trigger a reconnect") + + holder = SharedCaidoClient(cast("Any", _FakeClient("c")), _reconnect) + + async def fn(_client: Any) -> str: + raise ValueError("Invalid HTTPQL filter") + + with pytest.raises(ValueError, match="Invalid HTTPQL"): + await holder.call(fn) + + +def test_is_connection_error_matches_markers_and_causes() -> None: + assert caido_api._is_connection_error(RuntimeError("Transport is already connected")) + assert caido_api._is_connection_error(RuntimeError("Connector is closed")) + assert caido_api._is_connection_error(RuntimeError("Server disconnected")) + assert not caido_api._is_connection_error(ValueError("Invalid HTTPQL filter")) + + nested = RuntimeError("wrapper") + nested.__cause__ = RuntimeError("connection reset by peer") + assert caido_api._is_connection_error(nested) + + +class _Ctx: + def __init__(self, context: Any) -> None: + self.context = context + + +def test_ctx_proxy_returns_holder_when_present() -> None: + async def _reconnect() -> Any: + raise AssertionError("unused") + + holder = SharedCaidoClient(cast("Any", _FakeClient("c")), _reconnect) + got = tools._ctx_proxy(cast("Any", _Ctx({"caido_client": holder}))) + assert got is holder + + +def test_ctx_proxy_returns_none_without_holder() -> None: + assert tools._ctx_proxy(cast("Any", _Ctx({}))) is None + assert tools._ctx_proxy(cast("Any", _Ctx(None))) is None + assert tools._ctx_proxy(cast("Any", _Ctx({"caido_client": object()}))) is None + + +def test_is_httpql_error_detection() -> None: + assert tools._is_httpql_error(RuntimeError("HTTPQL parse error at column 4")) + assert tools._is_httpql_error(RuntimeError("failed to parse filter")) + assert not tools._is_httpql_error(RuntimeError("Transport is already connected")) + + +def test_httpql_error_preserves_message_and_query() -> None: + exc = RuntimeError("HTTPQL parse error: unexpected token at column 12") + payload = json.loads(tools._httpql_error(exc, 'resp.code.eq:"200"')) + assert payload["success"] is False + assert "unexpected token at column 12" in payload["error"] + assert payload["httpql_filter"] == 'resp.code.eq:"200"' + assert "AND / OR" in payload["hint"]