diff --git a/codeframe/adapters/e2b/adapter.py b/codeframe/adapters/e2b/adapter.py index 2c55da5b..28a2ffcf 100644 --- a/codeframe/adapters/e2b/adapter.py +++ b/codeframe/adapters/e2b/adapter.py @@ -9,10 +9,10 @@ import logging import os import time -from pathlib import Path +from pathlib import Path, PurePosixPath from typing import Callable -from codeframe.adapters.e2b.credential_scanner import scan_path +from codeframe.adapters.e2b.credential_scanner import EXCLUDED_DIRS, scan_path from codeframe.core.adapters.agent_adapter import ( AgentEvent, AgentResult, @@ -34,6 +34,47 @@ _INSTALL_CMD = "pip install codeframe --quiet" +#: The XY status characters `git status --porcelain` can emit. Used to tell a +#: real status record from a bare path that merely has a space at index 2. +_PORCELAIN_STATUS_CHARS = frozenset(" MADRCUT?!") + + +def _safe_local_path(workspace_root: Path, rel_path: str) -> Path | None: + """Resolve *rel_path* inside *workspace_root*, or return None to reject. + + The agent has arbitrary command execution in the sandbox before this runs + and can shadow the git binary, so every porcelain path is hostile input + (#967). Two ways out exist without this check: ``..`` segments, and an + absolute path — ``Path("/ws") / "/etc/passwd"`` is ``/etc/passwd``, because + pathlib discards the left side. Both used to reach ``mkdir(parents=True)`` + and a write. + + Both sides are resolved, which also closes the symlink route: a symlink + inside the workspace pointing outward resolves outward and is rejected, + while a workspace *reached* through a symlink is not falsely rejected. + + Returns: + The absolute local path, or None if it escapes the workspace. + """ + if not rel_path or PurePosixPath(rel_path).is_absolute() or Path(rel_path).is_absolute(): + return None + + try: + root = workspace_root.resolve() + candidate = (root / rel_path).resolve() + except (OSError, RuntimeError, ValueError) as exc: + # resolve() is not total: a symlink loop raises RuntimeError on Python + # 3.11/3.12 (3.13 resolves it quietly), and a bad path can raise + # OSError. Reject this one entry rather than aborting the whole + # download — an unresolvable path is exactly one we must not write to. + logger.warning("Could not resolve sandbox path %r: %s", rel_path, exc) + return None + + if candidate == root or root not in candidate.parents: + return None + return candidate + + class E2BAgentAdapter: """Runs a CodeFrame task inside an E2B Linux sandbox. @@ -257,14 +298,11 @@ def _upload_workspace( emit: Callable[[str, str, dict | None], None], ) -> int: """Upload workspace files to sandbox, returning the count uploaded.""" - _EXCLUDED = frozenset({ - "__pycache__", ".git", ".mypy_cache", ".pytest_cache", - ".ruff_cache", "node_modules", ".venv", "venv", - }) - uploaded = 0 for path in sorted(workspace_path.rglob("*")): - if any(part in _EXCLUDED for part in path.parts): + # The SAME set the credential scanner skips (#967) — a directory + # the scanner does not read must not be a directory we ship. + if any(part in EXCLUDED_DIRS for part in path.parts): continue if not path.is_file(): continue @@ -296,35 +334,30 @@ def _download_changed_files( Tuple of (list of relative file paths, count downloaded). """ status_result = sbx.commands.run( - f"cd {_SANDBOX_WORKSPACE} && git status --porcelain", + f"cd {_SANDBOX_WORKSPACE} && git status --porcelain -z --no-renames", timeout=30, ) if status_result.exit_code != 0 or not status_result.stdout.strip(): return [], 0 - changed: list[str] = [] - for line in status_result.stdout.splitlines(): - line = line.strip() - if not line: - continue - # porcelain format: XY filename (or "XY old -> new" for renames) - parts = line.split(None, 1) - if len(parts) < 2: - continue - xy, filepath = parts - # Handle renames: "R old -> new" — take the new name after " -> " - if " -> " in filepath: - filepath = filepath.split(" -> ", 1)[1] - changed.append(filepath.strip()) + changed, rejected = self._parse_porcelain(status_result.stdout) downloaded = 0 modified_files: list[str] = [] for rel_path in changed: - remote = f"{_SANDBOX_WORKSPACE}/{rel_path}" - local = workspace_path / rel_path + # Contain BEFORE the read and before any mkdir — a rejected path + # must cost nothing and create nothing (#967). + local = _safe_local_path(workspace_path, rel_path) + if local is None: + rejected += 1 + logger.warning( + "Rejected sandbox path outside the workspace: %r", rel_path + ) + continue + remote = f"{_SANDBOX_WORKSPACE}/{rel_path}" try: content = sbx.files.read(remote) local.parent.mkdir(parents=True, exist_ok=True) @@ -339,4 +372,76 @@ def _download_changed_files( logger.warning("Failed to download %s: %s", rel_path, exc) emit("progress", f"Downloaded {downloaded} changed file(s)") + if rejected: + # Counted and surfaced, never silently dropped: a rejection here + # means the sandbox tried to write outside the workspace. + emit( + "progress", + f"Rejected {rejected} sandbox path(s) — unparseable, or " + "outside the workspace (see log for each)", + ) return modified_files, downloaded + + @staticmethod + def _parse_porcelain(stdout: str) -> tuple[list[str], int]: + """Parse ``git status --porcelain -z`` into paths, hostile input assumed. + + Two flags carry the weight here, both verified against real git: + + ``-z`` emits each path as raw bytes, so there is no C-quoting to decode + (``--porcelain`` alone renders ``café.txt`` as ``"caf\\303\\251.txt"``, + quotes and all) and no ``" -> "`` rename separator to collide with a + filename containing that string. + + ``--no-renames`` reports a rename as an independent delete + add + instead of ``R new\\0old\\0``. That removes the paired field entirely, + and with it a whole class of ambiguity: there is no lookahead to guess + at, so a hostile ``R`` header cannot make the parser swallow the record + after it, and an old filename shaped like a status record (``AD + HOC.txt``, ``v1 notes.txt``) cannot be misread as one. Every entry is a + record. + + Returns: + Tuple of (paths, count rejected as unparseable). + """ + def _looks_like_record(entry: str) -> bool: + # "XY PATH" — two status characters then a space. + return ( + len(entry) >= 4 + and entry[2] == " " + and entry[0] in _PORCELAIN_STATUS_CHARS + and entry[1] in _PORCELAIN_STATUS_CHARS + ) + + entries = [e for e in stdout.split("\0") if e] + paths: list[str] = [] + rejected = 0 + + index = 0 + while index < len(entries): + entry = entries[index] + index += 1 + if not _looks_like_record(entry): + # Counted and warned, not dropped: real git cannot emit this, + # so a malformed record means the sandbox's git is not git. + rejected += 1 + logger.warning("Rejected malformed porcelain record: %r", entry) + continue + status, raw = entry[:2], entry[3:] + + # A deleted file is not in the sandbox to read. Skipping it avoids + # a misleading "Failed to download" for a file that is meant to be + # gone. Applying the deletion locally is a separate, parked defect + # (#966) — this only stops us fetching a path we know is absent. + if "D" in status: + logger.debug("Skipping deleted path: %r", raw) + continue + + # Verbatim: -z output is NOT C-quoted (that is the whole point of + # the flag), so a file genuinely named `"a.py"` must keep its + # quotes. Decoding here would silently rewrite it to `a.py` and + # clobber a different file. Whatever the name turns out to be, + # _safe_local_path is what decides where it may land. + paths.append(raw) + + return paths, rejected diff --git a/codeframe/adapters/e2b/credential_scanner.py b/codeframe/adapters/e2b/credential_scanner.py index 088a5477..056db0ba 100644 --- a/codeframe/adapters/e2b/credential_scanner.py +++ b/codeframe/adapters/e2b/credential_scanner.py @@ -13,8 +13,17 @@ logger = logging.getLogger(__name__) -# Directories that are always excluded from scanning and upload counts -_EXCLUDED_DIRS = frozenset({ +# Directories skipped by BOTH the scanner and the uploader (#967). +# +# One constant, deliberately: these two used to disagree, and the uploader's +# smaller set was the wider one — the scanner skipped ``dist``/``build``/ +# ``.tox``/``.eggs`` while the uploader happily shipped them, so a ``.env`` or +# key baked into a build artifact reached the third-party sandbox unscanned. +# Anything listed here is never scanned, which means it must also never be +# uploaded. Only add entries that can hold no secret worth protecting. +# +# Build output is NOT excluded, precisely because it can contain one. +EXCLUDED_DIRS = frozenset({ "__pycache__", ".git", ".mypy_cache", @@ -23,10 +32,6 @@ "node_modules", ".venv", "venv", - ".tox", - "dist", - "build", - ".eggs", }) # High-risk filename/extension patterns (case-insensitive glob-style matching) @@ -83,7 +88,7 @@ def scan_path(root: Path) -> ScanResult: for path in sorted(root.rglob("*")): # Skip excluded directories - if any(part in _EXCLUDED_DIRS for part in path.parts): + if any(part in EXCLUDED_DIRS for part in path.parts): continue if not path.is_file(): diff --git a/tests/adapters/test_e2b_adapter.py b/tests/adapters/test_e2b_adapter.py index d9a8cf94..3b696ec7 100644 --- a/tests/adapters/test_e2b_adapter.py +++ b/tests/adapters/test_e2b_adapter.py @@ -269,8 +269,8 @@ def test_successful_execution_returns_completed(self, mock_create, tmp_path): # Adapter runs: git-combined (1 call), pip install, cf work start, git status diff_result = MagicMock() diff_result.exit_code = 0 - # git status --porcelain format: "XY filename" - diff_result.stdout = " M main.py\n" + # `git status --porcelain -z`: "XY filename" records, NUL-terminated (#967) + diff_result.stdout = " M main.py\x00" sbx.commands.run.side_effect = [ MagicMock(exit_code=0, stdout="", stderr=""), # git init+add+commit MagicMock(exit_code=0, stdout="installed", stderr=""), # pip install @@ -375,7 +375,7 @@ def test_new_files_downloaded_via_porcelain(self, mock_create, tmp_path): # porcelain: modified file + untracked new file status_result = MagicMock() status_result.exit_code = 0 - status_result.stdout = " M existing.py\n?? new_module.py\n" + status_result.stdout = " M existing.py\x00?? new_module.py\x00" sbx.commands.run.side_effect = [ MagicMock(exit_code=0, stdout="", stderr=""), # git combined MagicMock(exit_code=0, stdout="", stderr=""), # pip install diff --git a/tests/adapters/test_e2b_trust_boundary_967.py b/tests/adapters/test_e2b_trust_boundary_967.py new file mode 100644 index 00000000..88a74105 --- /dev/null +++ b/tests/adapters/test_e2b_trust_boundary_967.py @@ -0,0 +1,432 @@ +"""The E2B sandbox is a trust boundary in both directions (issue #967). + +The coding agent has arbitrary command execution inside the sandbox before +``_download_changed_files`` runs, so the output of ``git status --porcelain`` +is attacker-controlled — the agent can shadow the git binary. Every path in it +must be treated as hostile input, and the containment check is the actual +security boundary rather than anything git promises. + +Outbound, the pre-upload credential scan and the uploader disagreed about which +directories to skip, so a secret baked into a build artifact was shipped to a +third-party sandbox unscanned — defeating the adapter's abort-on-secrets +contract. + +These tests are written against the local filesystem, not a real sandbox: the +question is only ever "what did we write, and where". +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +pytestmark = pytest.mark.v2 + + +def _sbx(*entries: str, content: str | bytes = "pwned") -> MagicMock: + """A sandbox whose git reports *entries* and whose files all read back. + + Entries are given the way `git status --porcelain -z` emits them: one + ``XY PATH`` record per argument, NUL-terminated. + """ + sbx = MagicMock() + stdout = "".join(e + "\0" for e in entries) + sbx.commands.run.return_value = MagicMock(exit_code=0, stdout=stdout, stderr="") + sbx.files.read.return_value = content + return sbx + + +def _download(sbx: MagicMock, workspace: Path): + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + adapter = E2BAgentAdapter(timeout_minutes=5) + return adapter._download_changed_files(sbx, workspace, lambda *a, **k: None) + + +@pytest.fixture +def workspace(tmp_path: Path) -> Path: + ws = tmp_path / "ws" + ws.mkdir() + return ws + + +# ───────────────────────────────────────────────────────────────────────────── +# Containment: nothing lands outside the workspace +# ───────────────────────────────────────────────────────────────────────────── + + +class TestNothingEscapesTheWorkspace: + def test_dotdot_path_writes_nothing_outside(self, workspace, tmp_path): + """The canonical case from the issue.""" + outside = tmp_path / "evil" + _download(_sbx(" M ../evil"), workspace) + assert not outside.exists(), "wrote outside the workspace" + + def test_deep_dotdot_path_writes_nothing_outside(self, workspace, tmp_path): + _download(_sbx(" M a/../../../evil2"), workspace) + assert not (tmp_path.parent / "evil2").exists() + assert not (tmp_path / "evil2").exists() + + def test_absolute_path_is_rejected(self, workspace, tmp_path): + """`workspace / '/abs'` is `/abs` — pathlib discards the left side.""" + target = tmp_path / "absolute-pwned" + _download(_sbx(f" M {target}"), workspace) + assert not target.exists() + + def test_an_absolute_path_inside_the_workspace_is_still_rejected(self, workspace): + """The case the containment check alone does NOT catch. + + An absolute path that happens to resolve inside the workspace passes + the relative_to() test, but porcelain paths are always workspace- + relative — an absolute one means the output is not what we asked for, + and the remote read would be built as `/workspace//abs/path` anyway. + """ + target = workspace / "inside.py" + files, count = _download(_sbx(f" M {target}"), workspace) + assert count == 0, files + assert not target.exists() + + def test_home_relative_traversal_cannot_reach_ssh(self, workspace, tmp_path): + """The issue's worked example: ~/.ssh/authorized_keys.""" + fake_home = tmp_path / "home" + (fake_home / ".ssh").mkdir(parents=True) + rel = Path("..") / "home" / ".ssh" / "authorized_keys" + _download(_sbx(f" M {rel}"), workspace) + assert not (fake_home / ".ssh" / "authorized_keys").exists() + + def test_a_symlink_out_of_the_tree_is_not_a_way_out(self, workspace, tmp_path): + """Containment must resolve, not just string-check for '..'.""" + outside_dir = tmp_path / "outside" + outside_dir.mkdir() + (workspace / "link").symlink_to(outside_dir, target_is_directory=True) + + _download(_sbx(" M link/escaped.txt"), workspace) + assert not (outside_dir / "escaped.txt").exists() + + def test_a_failing_resolve_rejects_one_entry_not_the_whole_download( + self, workspace, monkeypatch + ): + """`Path.resolve()` can raise, and one bad entry must not abort the run. + + Version-dependent in the wild: a symlink loop raises RuntimeError on + Python 3.11/3.12 (both supported here) but resolves quietly on 3.13, so + a real-symlink test would silently pass on a 3.13 dev box while the + crash stays live for most users. Forcing the raise pins the handling on + every version. + """ + real_resolve = Path.resolve + + def exploding_resolve(self, *args, **kwargs): + if self.name == "boom": + raise RuntimeError(f"Symlink loop from {self}") + return real_resolve(self, *args, **kwargs) + + monkeypatch.setattr(Path, "resolve", exploding_resolve) + + files, count = _download(_sbx(" M boom", " M good.py", content="x"), workspace) + assert files == ["good.py"], files + assert count == 1 + + def test_no_directories_are_created_outside_either(self, workspace, tmp_path): + """mkdir(parents=True) runs before the write — it must not run at all.""" + _download(_sbx(" M ../made/up/dirs/file.txt"), workspace) + assert not (tmp_path / "made").exists() + + +# ───────────────────────────────────────────────────────────────────────────── +# The guard must not break the feature it protects +# ───────────────────────────────────────────────────────────────────────────── + + +class TestLegitimateFilesStillArrive: + def test_a_nested_file_downloads(self, workspace): + files, count = _download(_sbx(" M src/pkg/mod.py", content="real content"), workspace) + assert count == 1 + assert files == ["src/pkg/mod.py"] + assert (workspace / "src" / "pkg" / "mod.py").read_text() == "real content" + + def test_a_workspace_reached_through_a_symlink_is_not_a_false_reject( + self, tmp_path + ): + """Resolve BOTH sides, or a symlinked workspace rejects everything.""" + real = tmp_path / "real" + real.mkdir() + link = tmp_path / "link" + link.symlink_to(real, target_is_directory=True) + + files, count = _download(_sbx(" M ok.py", content="x"), link) + assert count == 1, "a symlinked workspace root rejected a legitimate file" + assert (real / "ok.py").read_text() == "x" + + def test_good_files_survive_alongside_a_rejected_one(self, workspace, tmp_path): + files, count = _download(_sbx(" M ../evil3", "M good.py", content="x"), workspace) + assert not (tmp_path / "evil3").exists() + assert files == ["good.py"] + assert count == 1 + + +# ───────────────────────────────────────────────────────────────────────────── +# Rejections are visible (AC2) +# ───────────────────────────────────────────────────────────────────────────── + + +class TestRejectionsAreWarnedAndCounted: + def test_a_rejected_path_is_logged_as_a_warning(self, workspace, caplog): + import logging + + with caplog.at_level(logging.WARNING): + _download(_sbx(" M ../evil4"), workspace) + + assert any("evil4" in r.getMessage() for r in caplog.records), caplog.text + assert "workspace" in caplog.text.lower() or "outside" in caplog.text.lower() + + def test_a_rejected_path_is_counted(self, workspace): + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + adapter = E2BAgentAdapter(timeout_minutes=5) + emitted: list[tuple] = [] + adapter._download_changed_files( + _sbx(" M ../evil5", " M ../evil6", " M ok.py"), + workspace, + lambda *a, **k: emitted.append(a), + ) + blob = " ".join(str(a) for a in emitted).lower() + assert "2" in blob and "reject" in blob, emitted + + def test_a_rejected_path_is_never_read_from_the_sandbox(self, workspace): + """Reject before the read, not after — no needless round trip.""" + sbx = _sbx(" M ../evil7") + _download(sbx, workspace) + assert sbx.files.read.call_count == 0 + + +# ───────────────────────────────────────────────────────────────────────────── +# Hostile porcelain parsing (AC3) +# ───────────────────────────────────────────────────────────────────────────── + + +class TestPorcelainParsing: + def test_a_literally_quoted_filename_keeps_its_quotes(self, workspace): + """-z emits paths verbatim, so quotes in a name are part of the name. + + Verified against real git: a file named `"quoted".py` comes back as + `?? "quoted".py\\0` under -z, and as `?? "\\"quoted\\".py"` without it. + Decoding the -z form would rewrite `"a.py"` to `a.py` and clobber a + different file. + """ + files, count = _download(_sbx(' M "a.py"', content="x"), workspace) + assert files == ['"a.py"'] + assert (workspace / '"a.py"').exists() + assert not (workspace / "a.py").exists(), "stripped quotes that were real" + + def test_a_utf8_name_arrives_verbatim(self, workspace): + """No octal escaping under -z — the bytes are the name.""" + files, count = _download(_sbx(" M café.txt", content="x"), workspace) + assert files == ["café.txt"] + assert (workspace / "café.txt").exists() + + def test_a_name_containing_a_tab_survives(self, workspace): + """--porcelain would have quoted this one; -z does not.""" + files, _ = _download(_sbx(" M tab\tname.txt", content="x"), workspace) + assert files == ["tab\tname.txt"] + + def test_a_quoted_traversal_is_still_contained(self, workspace, tmp_path): + """Decoding must not be a way around the containment check.""" + _download(_sbx(' M "../evil8"'), workspace) + assert not (tmp_path / "evil8").exists() + + def test_a_bizarre_name_is_contained_rather_than_interpreted(self, workspace): + """Hostile input is not guessed at — it is just kept inside the tree.""" + _download(_sbx(' M "bad\\777\\777"', content="x"), workspace) + written = [p for p in workspace.rglob("*") if p.is_file()] + assert len(written) == 1 + assert workspace.resolve() in written[0].resolve().parents + + def test_a_rename_keeps_the_new_name_only(self, workspace): + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + adapter = E2BAgentAdapter(timeout_minutes=5) + sbx = _sbx(content="x") + # -z renames are "XY new\0old\0" + sbx.commands.run.return_value = MagicMock( + exit_code=0, stdout="R new.py\x00old.py\x00", stderr="" + ) + files, _ = adapter._download_changed_files(sbx, workspace, lambda *a, **k: None) + assert files == ["new.py"] + assert not (workspace / "old.py").exists() + + def test_a_filename_containing_the_rename_arrow_is_not_mangled(self, workspace): + """' -> ' inside a name used to split the path in half.""" + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + adapter = E2BAgentAdapter(timeout_minutes=5) + sbx = _sbx(content="x") + sbx.commands.run.return_value = MagicMock( + exit_code=0, stdout=" M a -> b.py\x00", stderr="" + ) + files, _ = adapter._download_changed_files(sbx, workspace, lambda *a, **k: None) + assert files == ["a -> b.py"] + + def test_a_malformed_record_is_counted_and_warned_not_dropped(self, workspace, caplog): + """AC2 applies to parse rejections too, not just containment ones.""" + import logging + + with caplog.at_level(logging.WARNING): + files, count = _download(_sbx("xx", " M ok.py", content="x"), workspace) + + assert files == ["ok.py"] + assert count == 1 + assert any("xx" in r.getMessage() for r in caplog.records), caplog.text + + def test_the_parse_reject_count_reaches_the_user(self, workspace): + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + emitted: list[str] = [] + E2BAgentAdapter(timeout_minutes=5)._download_changed_files( + _sbx("xx", "y", " M ../outside", content="x"), + workspace, + lambda kind, msg, *a: emitted.append(msg), + ) + # 2 unparseable + 1 escaping the workspace + assert any("3" in m and "reject" in m.lower() for m in emitted), emitted + + def test_parse_rejects_are_reported_separately_from_containment(self): + """_parse_porcelain's own return value must mean something.""" + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + paths, rejected = E2BAgentAdapter._parse_porcelain("xx\0 M ok.py\0y\0") + assert paths == ["ok.py"] + assert rejected == 2 + + def test_renames_are_disabled_so_there_is_no_paired_field(self, workspace): + """--no-renames is what removes the whole ambiguity class. + + Verified against real git: renaming `AD HOC.txt` to `new.py` gives + `R new.py\\0AD HOC.txt\\0` normally, but `D AD HOC.txt\\0A new.py\\0` + with --no-renames — two independent records and no field to consume. + """ + sbx = _sbx(" M ok.py", content="x") + _download(sbx, workspace) + command = sbx.commands.run.call_args[0][0] + assert "--no-renames" in command, command + + def test_every_entry_is_a_record_so_nothing_can_be_swallowed(self): + """A hostile `R` header can no longer eat the record after it.""" + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + paths, _ = E2BAgentAdapter._parse_porcelain("R fake.py\0 M real_change.py\0") + assert paths == ["fake.py", "real_change.py"], paths + + def test_an_odd_old_filename_can_no_longer_be_misparsed(self): + """`AD HOC.txt` and `v1 notes.txt` are just paths on their own records. + + Both used to be misparsed by the lookahead heuristic — the first + because `A`/`D` are real status characters, the second because index 2 + is a space. With no lookahead there is nothing left to guess. + """ + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + paths, rejected = E2BAgentAdapter._parse_porcelain( + "A AD HOC.txt\0?? v1 notes.txt\0" + ) + assert paths == ["AD HOC.txt", "v1 notes.txt"], paths + assert rejected == 0 + + @pytest.mark.parametrize("status", ["D ", " D", "AD", "MD"]) + def test_a_deleted_file_is_not_fetched(self, workspace, status): + """It is not in the sandbox to read — trying only logs a false failure.""" + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + paths, _ = E2BAgentAdapter._parse_porcelain(f"{status} gone.py\0 M kept.py\0") + assert paths == ["kept.py"], paths + + @pytest.mark.parametrize("entry", ["v1 notes.txt", "hello world", "ab cd"]) + def test_a_non_status_prefix_is_not_a_record(self, entry): + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + paths, rejected = E2BAgentAdapter._parse_porcelain(entry + "\0") + assert paths == [] + assert rejected == 1 + + @pytest.mark.parametrize("status", [" M", "M ", "??", "A ", " D", "R ", "!!", "UU"]) + def test_real_status_pairs_are_recognised(self, status): + """Tightening the alphabet must not start rejecting valid records. + + Recognition, not download: a `D` pair is a valid record that is then + deliberately skipped (the file is gone), which the deletion test + covers. + """ + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + paths, rejected = E2BAgentAdapter._parse_porcelain(f"{status} f.py\0") + assert rejected == 0 + assert paths == ([] if "D" in status else ["f.py"]) + + def test_porcelain_is_requested_nul_separated(self, workspace): + """-z is what removes the separator ambiguity above.""" + sbx = _sbx(" M ok.py", content="x") + _download(sbx, workspace) + command = sbx.commands.run.call_args[0][0] + assert "-z" in command, command + + +# ───────────────────────────────────────────────────────────────────────────── +# Outbound: one exclusion set (AC5) +# ───────────────────────────────────────────────────────────────────────────── + + +class TestUploaderAndScannerAgree: + def test_they_are_literally_the_same_constant(self): + from codeframe.adapters.e2b import adapter as adapter_mod + from codeframe.adapters.e2b.credential_scanner import EXCLUDED_DIRS + + assert adapter_mod.EXCLUDED_DIRS is EXCLUDED_DIRS + + @pytest.mark.parametrize("directory", [".tox", "dist", "build", ".eggs"]) + def test_build_output_is_scanned_not_skipped(self, directory): + """A skipped directory is an unscanned upload — the whole bug.""" + from codeframe.adapters.e2b.credential_scanner import EXCLUDED_DIRS + + assert directory not in EXCLUDED_DIRS + + def test_a_secret_under_dist_makes_the_scan_dirty(self, tmp_path): + from codeframe.adapters.e2b.credential_scanner import scan_path + + (tmp_path / "dist").mkdir() + (tmp_path / "dist" / ".env").write_text("SECRET=leaked") + + result = scan_path(tmp_path) + assert not result.is_clean + assert any("dist" in b for b in result.blocked_files), result.blocked_files + + def test_a_secret_under_dist_aborts_the_run(self, tmp_path): + """AC5, end to end: the adapter's abort-on-secrets contract holds.""" + import os + from unittest.mock import patch + + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + (tmp_path / "main.py").write_text("print(1)\n") + (tmp_path / "dist").mkdir() + (tmp_path / "dist" / "bundle.env").write_text("AKIA" + "IOSFODNN7EXAMPLE\n") # split: pre-commit secret scan + + adapter = E2BAgentAdapter(timeout_minutes=5) + with patch.dict(os.environ, {"E2B_API_KEY": "test-key"}): + with patch("e2b.Sandbox.create") as create: + result = adapter.run( + task_id="t-1", prompt="p", workspace_path=tmp_path + ) + assert create.call_count == 0, "created a sandbox despite a secret" + + assert result.status == "failed" + assert "credential" in (result.error or "").lower() + assert result.cloud_metadata["credential_scan_blocked"] == 1 + + def test_the_real_junk_dirs_are_still_skipped(self): + """Sharing the constant must not start uploading .git and node_modules.""" + from codeframe.adapters.e2b.credential_scanner import EXCLUDED_DIRS + + for directory in ("__pycache__", ".git", "node_modules", ".venv"): + assert directory in EXCLUDED_DIRS