diff --git a/strix/runtime/backends.py b/strix/runtime/backends.py index d7eba3357..5cdbba884 100644 --- a/strix/runtime/backends.py +++ b/strix/runtime/backends.py @@ -32,15 +32,17 @@ async def _docker_backend( backend don't need the docker-py library installed. ``session.start()`` is what materializes the manifest entries - (LocalDir copies and manifest-declared volume/FUSE mounts) into the - running container — the SDK's ``client.create()`` only builds the inner - session object without applying the manifest. ``async with session:`` - would call it too, but Strix manages session lifetime explicitly via - ``client.delete()`` so we trigger ``start()`` ourselves. + (manifest-declared volume/FUSE mounts) into the running container — the + SDK's ``client.create()`` only builds the inner session object without + applying the manifest. ``async with session:`` would call it too, but + Strix manages session lifetime explicitly via ``client.delete()`` so we + trigger ``start()`` ourselves. Local source trees are copied in separately + after ``start()`` via a single tar ``put_archive`` (see + ``session_manager._import_local_sources``), not through the manifest. ``bind_mounts`` are host directories (e.g. large repos passed via - ``--mount``) bind-mounted read-only; unlike manifest entries they are - applied by Docker at container-create time, not by ``start()``. + ``--mount``) bind-mounted read-only; unlike copied sources they are + applied by Docker at container-create time, not after ``start()``. """ import docker from agents.sandbox.sandboxes.docker import DockerSandboxClientOptions diff --git a/strix/runtime/docker_client.py b/strix/runtime/docker_client.py index 497ae2f21..0979f6a4c 100644 --- a/strix/runtime/docker_client.py +++ b/strix/runtime/docker_client.py @@ -117,7 +117,7 @@ async def _create_container( extra_hosts["host.docker.internal"] = "host-gateway" # Strix injection: host bind mounts (e.g. large repos passed via --mount) - # that bypass the SDK's file-by-file LocalDir copy. + # that bypass the in-container source copy entirely. bind_mounts = getattr(self, "strix_bind_mounts", ()) if bind_mounts: mounts = create_kwargs.setdefault("mounts", []) diff --git a/strix/runtime/session_manager.py b/strix/runtime/session_manager.py index a19495cc5..95eaf2a19 100644 --- a/strix/runtime/session_manager.py +++ b/strix/runtime/session_manager.py @@ -2,11 +2,14 @@ from __future__ import annotations +import asyncio +import io import logging +import os +import tarfile from pathlib import Path from typing import Any -from agents.sandbox.entries import BaseEntry, LocalDir from agents.sandbox.manifest import Environment, Manifest from strix.config import load_settings @@ -23,27 +26,46 @@ _SESSION_CACHE: dict[str, dict[str, Any]] = {} -# Manifest root inside the container; entry keys hang off this path. +# Workspace root inside the container; sources land at ``/``. _WORKSPACE_ROOT = "/workspace" +# Container user that runs the agent / shell tools (from the image). +_CONTAINER_USER = "pentester" -def build_session_entries( + +def _is_safe_workspace_subdir(ws_subdir: str) -> bool: + """Reject subdirs that would escape ``/workspace`` when joined. + + ``ws_subdir`` becomes both a tar member prefix and a ``chown`` target, so a + value containing ``..`` (or an absolute path) could write outside the + intended ``/workspace/`` tree. Callers pass values with surrounding + slashes already stripped. + """ + if not ws_subdir: + return False + return not any(part in ("..", "") for part in ws_subdir.split("/")) + + +def split_local_sources( local_sources: list[dict[str, Any]], -) -> tuple[dict[str | Path, BaseEntry], list[dict[str, Any]]]: - """Split local sources into copied manifest entries and host bind mounts. +) -> tuple[list[dict[str, str]], list[dict[str, Any]]]: + """Split local sources into tar-copied entries and host bind mounts. Sources flagged ``mount`` are bind-mounted read-only at - ``/workspace/`` (not added to the manifest, so the SDK - does not stream them in file-by-file). Every other source becomes a - ``LocalDir`` entry copied into the container as before. + ``/workspace/`` (applied by Docker at container-create + time). Every other source is copied into the container after start via a + single tar ``put_archive`` (see :func:`_import_local_sources`). """ - entries: dict[str | Path, BaseEntry] = {} + copied: list[dict[str, str]] = [] bind_mounts: list[dict[str, Any]] = [] for src in local_sources: - ws_subdir = src.get("workspace_subdir") or "" + ws_subdir = (src.get("workspace_subdir") or "").strip("/") host_path = src.get("source_path") or "" if not ws_subdir or not host_path: continue + if not _is_safe_workspace_subdir(ws_subdir): + logger.warning("Skipping local source with unsafe workspace_subdir: %r", ws_subdir) + continue resolved = Path(host_path).expanduser().resolve() if src.get("mount"): bind_mounts.append( @@ -54,8 +76,134 @@ def build_session_entries( } ) else: - entries[ws_subdir] = LocalDir(src=resolved) - return entries, bind_mounts + copied.append({"source_path": str(resolved), "workspace_subdir": ws_subdir}) + return copied, bind_mounts + + +def _build_source_tar(src_root: Path, arc_prefix: str) -> tuple[bytes, int, int]: + """Pack ``src_root`` into an in-memory tar rooted at ``arc_prefix``. + + Returns ``(tar_bytes, added, skipped)``. Regular files and directories — + including dotfiles such as ``.git`` and directories with no files — are + packed as-is so source-aware and git-diff analysis keep working and + committed empty dirs survive. Symlinks are skipped (and counted) rather + than followed, avoiding host path escapes and the dangling links a naive + copy would create inside the container. + """ + buf = io.BytesIO() + added = 0 + skipped = 0 + with tarfile.open(fileobj=buf, mode="w") as tar: + for dirpath, dirnames, filenames in os.walk(src_root, followlinks=False): + kept_dirs: list[str] = [] + for name in dirnames: + if Path(dirpath, name).is_symlink(): + skipped += 1 + continue + kept_dirs.append(name) + dirnames[:] = kept_dirs + + dir_abs = Path(dirpath) + rel = dir_abs.relative_to(src_root).as_posix() + dir_arcname = arc_prefix if rel == "." else f"{arc_prefix}/{rel}" + tar.add(str(dir_abs), arcname=dir_arcname, recursive=False) + + for name in filenames: + full = dir_abs / name + if full.is_symlink() or not full.is_file(): + skipped += 1 + continue + arcname = f"{arc_prefix}/{full.relative_to(src_root).as_posix()}" + tar.add(str(full), arcname=arcname, recursive=False) + added += 1 + return buf.getvalue(), added, skipped + + +def _container_of(session: Any) -> Any: + """Reach the underlying docker-py ``Container`` from an SDK session. + + The SDK wraps the backend session in an outer object; the docker backend + exposes the real container as ``_inner._container``. Pinned to + openai-agents==0.14.6 — re-check these private attrs on SDK bumps. + """ + inner = getattr(session, "_inner", session) + container = getattr(inner, "_container", None) + if container is None: + raise RuntimeError("could not locate docker container on sandbox session") + return container + + +def _run_checked(container: Any, cmd: list[str]) -> None: + """Run ``cmd`` in the container as root and raise on non-zero exit. + + ``exec_run`` reports failures only via its result; an ignored ``chown`` + failure would leave sources root-owned and unwritable while session setup + still "succeeds", so surface it here with the command's own diagnostics. + """ + result = container.exec_run(cmd, user="root") + if result.exit_code: + output = result.output + detail = output.decode("utf-8", "replace").strip() if isinstance(output, bytes) else output + raise RuntimeError(f"container command {cmd!r} failed (exit {result.exit_code}): {detail}") + + +async def _import_local_sources( + session: Any, + copied_sources: list[dict[str, str]], +) -> None: + """Copy each host source tree into the container via a single tar import. + + Replaces the SDK's per-file ``LocalDir`` materialization, which issues + several ``docker exec`` calls per file. On large trees that serializes into + thousands of exec round-trips against a non-thread-safe docker-py client, + causing multi-minute hangs (the "stuck on loading" symptom) and occasional + ``ExecTransportError``. ``put_archive`` lands the whole tree in one shot + (sub-second for thousands of files). + + Safe here because the copy path attaches no Docker volume-driver mounts — + the SDK avoids ``put_archive`` only to sidestep volume plugins that re-run + mount setup during archive ops (docker.py:709). ``--mount`` sources use + plain read-only bind mounts at a different subdir, which have no such + driver. + """ + container = _container_of(session) + loop = asyncio.get_running_loop() + + for src in copied_sources: + ws_subdir = src["workspace_subdir"] + src_root = Path(src["source_path"]) + if not src_root.is_dir(): + logger.warning("Skipping non-directory local source: %s", src_root) + continue + + tar_bytes, added, skipped = await loop.run_in_executor( + None, _build_source_tar, src_root, ws_subdir + ) + logger.info( + "Importing %d files into %s/%s (skipped %d symlink entries)", + added, + _WORKSPACE_ROOT, + ws_subdir, + skipped, + ) + + def _put(tar_bytes: bytes = tar_bytes, ws_subdir: str = ws_subdir) -> None: + _run_checked(container, ["mkdir", "-p", _WORKSPACE_ROOT]) + if not container.put_archive(_WORKSPACE_ROOT, tar_bytes): + raise RuntimeError(f"put_archive failed for {_WORKSPACE_ROOT}/{ws_subdir}") + # put_archive unpacks as root with the tar's host uids; hand the + # tree to the agent's runtime user so tools can read/write it. + _run_checked( + container, + [ + "chown", + "-R", + f"{_CONTAINER_USER}:{_CONTAINER_USER}", + f"{_WORKSPACE_ROOT}/{ws_subdir}", + ], + ) + + await loop.run_in_executor(None, _put) async def create_or_reuse( @@ -67,16 +215,21 @@ async def create_or_reuse( """Return the existing session bundle for ``scan_id`` or create a new one. Each ``local_sources`` entry exposes its host ``source_path`` at - ``/workspace/`` inside the container — copied in, or - bind-mounted read-only when the entry is flagged ``mount``. + ``/workspace/`` inside the container — copied in via a + single tar ``put_archive`` after start, or bind-mounted read-only when the + entry is flagged ``mount``. """ cached = _SESSION_CACHE.get(scan_id) if cached is not None: logger.info("Reusing existing sandbox session for scan %s", scan_id) return cached - entries, bind_mounts = build_session_entries(local_sources) + copied_sources, bind_mounts = split_local_sources(local_sources) + # Copied source trees are imported after start() via a single tar + # ``put_archive`` (see ``_import_local_sources``) instead of the SDK's + # per-file ``LocalDir`` exec loop, which hangs on large trees. So the + # manifest itself carries no source entries. # Caido runs as an in-container sidecar; HTTP(S) traffic from any # process started via ``session.exec`` (the SDK's Shell tool, etc.) # picks up these env vars automatically. ``NO_PROXY`` keeps the @@ -84,7 +237,7 @@ async def create_or_reuse( # through Caido. container_caido_url = f"http://127.0.0.1:{_CONTAINER_CAIDO_PORT}" manifest = Manifest( - entries=entries, + entries={}, environment=Environment( value={ "PYTHONUNBUFFERED": "1", @@ -113,6 +266,8 @@ async def create_or_reuse( bind_mounts=bind_mounts, ) + await _import_local_sources(session, copied_sources) + caido_endpoint = await session.resolve_exposed_port(_CONTAINER_CAIDO_PORT) host_caido_url = f"http://{caido_endpoint.host}:{caido_endpoint.port}" logger.debug("Caido host endpoint resolved: %s", host_caido_url) diff --git a/tests/test_session_entries.py b/tests/test_session_entries.py index 8288c1d08..d7597cd74 100644 --- a/tests/test_session_entries.py +++ b/tests/test_session_entries.py @@ -1,34 +1,59 @@ -"""Tests for build_session_entries: splitting copied vs bind-mounted sources.""" +"""Tests for local-source handling in session_manager. + +Covers splitting copied vs bind-mounted sources (``split_local_sources``) and +the in-memory tar builder (``_build_source_tar``) that replaces the SDK's +per-file ``LocalDir`` copy. +""" from __future__ import annotations +import io +import os +import tarfile from typing import TYPE_CHECKING, Any -from agents.sandbox.entries import LocalDir +import pytest -from strix.runtime.session_manager import build_session_entries +from strix.runtime.session_manager import _build_source_tar, split_local_sources if TYPE_CHECKING: from pathlib import Path +_HAS_SYMLINK = hasattr(os, "symlink") + + def _source(subdir: str, path: str, *, mount: bool = False) -> dict[str, Any]: return {"source_path": path, "workspace_subdir": subdir, "mount": mount} -def test_copied_source_becomes_localdir_entry(tmp_path: Path) -> None: - entries, bind_mounts = build_session_entries([_source("repo", str(tmp_path))]) +def _tar_names(tar_bytes: bytes) -> set[str]: + with tarfile.open(fileobj=io.BytesIO(tar_bytes), mode="r") as tar: + return set(tar.getnames()) + + +def _tar_file_names(tar_bytes: bytes) -> set[str]: + with tarfile.open(fileobj=io.BytesIO(tar_bytes), mode="r") as tar: + return {m.name for m in tar.getmembers() if m.isfile()} + + +def _tar_dir_names(tar_bytes: bytes) -> set[str]: + with tarfile.open(fileobj=io.BytesIO(tar_bytes), mode="r") as tar: + return {m.name for m in tar.getmembers() if m.isdir()} + + +def test_copied_source_is_returned_for_import(tmp_path: Path) -> None: + copied, bind_mounts = split_local_sources([_source("repo", str(tmp_path))]) assert bind_mounts == [] - assert isinstance(entries["repo"], LocalDir) - assert entries["repo"].src == tmp_path.resolve() + assert copied == [{"source_path": str(tmp_path.resolve()), "workspace_subdir": "repo"}] def test_mounted_source_becomes_bind_mount(tmp_path: Path) -> None: - entries, bind_mounts = build_session_entries([_source("repo", str(tmp_path), mount=True)]) + copied, bind_mounts = split_local_sources([_source("repo", str(tmp_path), mount=True)]) - assert entries == {} + assert copied == [] assert bind_mounts == [ { "source": str(tmp_path.resolve()), @@ -39,29 +64,134 @@ def test_mounted_source_becomes_bind_mount(tmp_path: Path) -> None: def test_mixed_sources_split_correctly(tmp_path: Path) -> None: - copied = tmp_path / "copied" - mounted = tmp_path / "mounted" - copied.mkdir() - mounted.mkdir() + copied_dir = tmp_path / "copied" + mounted_dir = tmp_path / "mounted" + copied_dir.mkdir() + mounted_dir.mkdir() - entries, bind_mounts = build_session_entries( + copied, bind_mounts = split_local_sources( [ - _source("copied", str(copied)), - _source("mounted", str(mounted), mount=True), + _source("copied", str(copied_dir)), + _source("mounted", str(mounted_dir), mount=True), ] ) - assert list(entries) == ["copied"] - assert isinstance(entries["copied"], LocalDir) + assert [c["workspace_subdir"] for c in copied] == ["copied"] assert [m["target"] for m in bind_mounts] == ["/workspace/mounted"] def test_incomplete_sources_are_skipped() -> None: - entries, bind_mounts = build_session_entries( + copied, bind_mounts = split_local_sources( [ {"source_path": "", "workspace_subdir": "x"}, {"source_path": "/p", "workspace_subdir": ""}, ] ) - assert entries == {} + assert copied == [] + assert bind_mounts == [] + + +def test_workspace_subdir_slashes_are_stripped(tmp_path: Path) -> None: + copied, _ = split_local_sources([_source("/repo/", str(tmp_path))]) + assert copied[0]["workspace_subdir"] == "repo" + + +def test_tar_packs_files_under_arc_prefix(tmp_path: Path) -> None: + (tmp_path / "a.txt").write_text("a") + nested = tmp_path / "sub" + nested.mkdir() + (nested / "b.txt").write_text("b") + + tar_bytes, added, skipped = _build_source_tar(tmp_path, "repo") + + assert added == 2 + assert skipped == 0 + assert _tar_file_names(tar_bytes) == {"repo/a.txt", "repo/sub/b.txt"} + # Directory entries (incl. the arc-prefix root) are packed too. + assert {"repo", "repo/sub"} <= _tar_dir_names(tar_bytes) + + +def test_tar_preserves_dotfiles_and_git(tmp_path: Path) -> None: + # .git and other dotfiles must survive so source-aware / git-diff analysis + # keeps working — unlike a naive "skip hidden" copy. + (tmp_path / ".env").write_text("SECRET=1") + git_dir = tmp_path / ".git" + git_dir.mkdir() + (git_dir / "HEAD").write_text("ref: refs/heads/main") + + tar_bytes, added, _ = _build_source_tar(tmp_path, "repo") + + assert added == 2 + assert _tar_file_names(tar_bytes) == {"repo/.env", "repo/.git/HEAD"} + + +@pytest.mark.skipif(not _HAS_SYMLINK, reason="requires symlink support") +def test_tar_skips_file_symlinks(tmp_path: Path) -> None: + (tmp_path / "real.txt").write_text("real") + (tmp_path / "link.txt").symlink_to(tmp_path / "real.txt") + + tar_bytes, added, skipped = _build_source_tar(tmp_path, "repo") + + assert added == 1 + assert skipped == 1 + assert _tar_file_names(tar_bytes) == {"repo/real.txt"} + + +@pytest.mark.skipif(not _HAS_SYMLINK, reason="requires symlink support") +def test_tar_skips_dir_symlinks_without_descending(tmp_path: Path) -> None: + outside = tmp_path / "outside" + outside.mkdir() + (outside / "secret.txt").write_text("secret") + + src = tmp_path / "src" + src.mkdir() + (src / "keep.txt").write_text("keep") + (src / "escape").symlink_to(outside) + + tar_bytes, added, skipped = _build_source_tar(src, "repo") + + # The symlinked directory is not followed, so nothing under ``outside`` + # leaks into the tar. + assert added == 1 + assert skipped == 1 + assert _tar_file_names(tar_bytes) == {"repo/keep.txt"} + assert "repo/escape" not in _tar_names(tar_bytes) + + +def test_tar_preserves_empty_dirs(tmp_path: Path) -> None: + # Committed empty directories (cache/output scaffolding) must survive. + (tmp_path / "empty").mkdir() + (tmp_path / "keep.txt").write_text("k") + + tar_bytes, added, skipped = _build_source_tar(tmp_path, "repo") + + assert added == 1 + assert skipped == 0 + assert _tar_file_names(tar_bytes) == {"repo/keep.txt"} + assert {"repo", "repo/empty"} <= _tar_dir_names(tar_bytes) + + +def test_tar_empty_root_still_packs_prefix(tmp_path: Path) -> None: + tar_bytes, added, skipped = _build_source_tar(tmp_path, "repo") + assert added == 0 + assert skipped == 0 + assert _tar_file_names(tar_bytes) == set() + # The root prefix dir is still created so the workspace subdir exists. + assert _tar_dir_names(tar_bytes) == {"repo"} + + +def test_unsafe_workspace_subdir_is_skipped(tmp_path: Path) -> None: + copied, bind_mounts = split_local_sources( + [ + _source("../escape", str(tmp_path)), + _source("ok/../../escape", str(tmp_path)), + ] + ) + assert copied == [] + assert bind_mounts == [] + + +def test_unsafe_workspace_subdir_skipped_for_mount(tmp_path: Path) -> None: + copied, bind_mounts = split_local_sources([_source("../escape", str(tmp_path), mount=True)]) + assert copied == [] assert bind_mounts == []