fix(e2b): contain sandbox downloads and close the unscanned-upload leak (#967) - #1100
Conversation
…ak (#967) The E2B sandbox is a trust boundary in both directions, and neither was enforced. The coding agent has arbitrary command execution in the sandbox before we read `git status --porcelain`, so it can shadow the git binary — every path in that output is attacker-controlled input. Inbound: `local = workspace_path / rel_path` had no containment check, then `mkdir(parents=True)` and a write. Two ways out, both confirmed working: `../` segments, and an absolute path — `Path("/ws") / "/etc/passwd"` is `/etc/passwd`, because pathlib discards the left side. The issue's worked example (~/.ssh/authorized_keys) reproduced. - `_safe_local_path` resolves both sides and requires the candidate to be under the workspace root. Resolving both closes the symlink route (a symlink inside the tree pointing out is rejected) without falsely rejecting a workspace reached *through* a symlink. - Absolute paths are rejected outright, including ones that happen to resolve inside the workspace: porcelain paths are always relative, and the remote read would be built as `/workspace//abs/path` anyway. - Rejections are logged as warnings, counted, and emitted — never silently dropped. The check runs before the read, so a rejected path costs no round trip and creates no directory. Parsing uses `--porcelain -z`, which emits each path as raw bytes. Verified against real git: `--porcelain` alone renders `café.txt` as `"caf\303\251.txt"` — quotes and all — and splits renames on `" -> "`, which a filename containing that string would break. `-z` has neither problem, and paths are taken verbatim: a file genuinely named `"a.py"` keeps its quotes rather than being rewritten to `a.py` over the top of a different file. Containment, not parsing, is the security boundary. Outbound: the scanner and the uploader kept separate exclusion sets, and the scanner's was the *wider* one — it skipped dist/build/.tox/.eggs while the uploader shipped them, so a .env baked into a build artifact reached the third-party sandbox unscanned, defeating the abort-on-secrets contract. There is now one `EXCLUDED_DIRS`, and build output is not in it. Existing porcelain fixtures updated to the -z wire format.
Third-party review —
|
|
Claude finished @frankbria's task in 7m 25s —— View job GLM precision review — E2B trust boundary (#967)
Done. The inbound containment (
|
ReviewThis is a solid, well-scoped security fix. The threat model (shadowed One correctness nit, not security-relevant:
No other issues found — parsing of renames, quoted/UTF-8/tab filenames, and the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
GLM precision review — #967
In scope and verified clean:
No critical or major defects found. |
Both reviewers caught the same leftover: `_parse_porcelain`'s `rejected` counter was never incremented. Deleting the C-unquoter removed the only thing that incremented it, and I left the variable and its docstring claim behind — so a malformed record was silently dropped, contradicting this PR's own "never silently dropped" framing for AC2. Real git cannot emit a record that isn't "XY PATH", so a malformed one means the sandbox's git is not git — precisely the case worth surfacing. Now warned, counted, and folded into the count emitted to the user alongside containment rejections.
|
Claude finished @frankbria's task in 3m 39s —— View job GLM precision review — E2B trust boundary (#967, synchronize)
Reviewed the synchronize commit ✅ GLM review: no defects found. The fix is minimal and purely additive —
The broader inbound containment ( |
ReviewThis is a well-executed fix for a real trust-boundary gap, and the PR body/tests do the hard part of proving it (real-git verification of -z semantics, mutation-checked guards, end-to-end secret-under-dist/ abort). I focused on what has not already been covered by the codex/GLM passes and the earlier claude review in this thread. Already resolved, confirming it is fixed: the earlier review round (both claude and GLM) flagged that _parse_porcelain's New finding - a crafted rename record can silently swallow the next legitimate entry (adapter.py:406-415). The rename/copy consumption logic trusts the entry's own self-reported XY status to decide whether to skip the next NUL-delimited field: if status[0] in ("R", "C") or status[1] in ("R", "C"):
index += 1This is correct for genuine git output (verified: -z renames really are Tracing _parse_porcelain: the parser accepts "fake.py" as a rename target, then unconditionally consumes " M real_change.py" as its "old path" - so real_change.py never reaches Impact is limited - nothing escapes the workspace, a swallowed entry just means a legitimate change does not get downloaded - so I would not block on it, but it seems worth either a follow-up test + fix (e.g. validate that the consumed "old path" field does not itself look like a well-formed "XY " record before trusting it as a pair, or at minimum count/log a rename consumption as a rejection when the input is otherwise flagged suspicious) or a documented known-limitation alongside the ones already listed in the PR body. Everything else checks out:
No other security or correctness issues found. |
…#967) claude-review: under this PR's own threat model, a shadowed git can emit `R fake.py\0 M real_change.py\0` — the parser accepted the rename header and unconditionally ate the following well-formed entry as its "old path", so a real change disappeared with no log and no count. Not an escape (nothing is written outside the workspace, and an attacker who controls the output could simply omit the entry instead), but it is a silent drop under the invariant this PR just established for AC2. Honest git always follows a rename header with a bare path, so a field that itself looks like `XY PATH` was never half of a rename pair. The consumption is now conditional on that and warns when it declines. The honest case is pinned separately so the guard cannot start treating every rename's old name as a download.
|
Fixed in the latest push — verified first, and it is a genuine silent drop under this PR's own threat model.
Agreed on the impact framing — nothing escapes the workspace, and an attacker who controls git's output could simply omit the entry rather than go to the trouble of faking a rename to hide it, so no capability is gained. What made it worth fixing rather than documenting is the silent part: this PR had just gone back and fixed the same class of thing for malformed records, and leaving a second silent-drop path would make that invariant decorative. The fix uses the property that makes the attack detectable: honest git always follows a rename header with a bare path, so a field that itself looks like Three tests, and the honest case is pinned separately ( |
|
Claude finished @frankbria's task in 4m 54s —— View job GLM precision review — E2B trust boundary (#967, synchronize)
Reviewed the synchronize commit ✅ GLM review: no defects found.The fix is sound, and its correctness rests on a structural guarantee of the logic rather than on trusting the shape check as a parser. Tracing
The previously-reviewed areas are untouched by this commit and stay sound: inbound containment ( |
ReviewThis PR is now heavily reviewed (codex x2, GLM x3, claude x2 in this thread), and each round has caught and fixed a real issue: the -z/unquoter contradiction, the dead Confirming the latest fix is correct. I traced One new edge case, not security-relevant, low severity. The Practical impact is small: the renamed file's new name is still captured correctly from the rename entry itself, so no data is lost — it's a spurious warning + a harmless failed-download attempt for a wrong path, not a silent drop. Given how deliberately this PR pins every other parsing edge case (quotes, UTF-8, tabs, the Everything else — |
claude-review edge case: `_looks_like_record` tested only "space at index 2", so an honest rename whose OLD name happens to fit that shape — e.g. `v1 notes.txt` — was refused as the rename's old path and then re-parsed as a record with status `v1`, producing a spurious warning and a failed download of `notes.txt`. No data loss (the new name was still captured), but it read as an oversight next to how deliberately every other parsing edge case here is pinned. Real XY status characters come from a small alphabet, which disambiguates the two cases outright rather than documenting the collision. Checking it also tightens the top-level malformed-record test: `v1 notes.txt` standing alone is now correctly rejected instead of parsed as status `v1`. The adversarial case is unaffected — ` M real_change.py` still reads as a record, so a faked rename header still cannot swallow it.
|
Fixed in dcc86f1 rather than documented — the collision turned out to be avoidable. Confirmed the misfire exactly as described: The disambiguator is that real XY status characters come from a small alphabet ( The adversarial case is unaffected —
Mutation-checked: dropping the alphabet condition fails three tests. Thanks for the four rounds on this one — every pass found something real (the -z/unquoter contradiction, the dead counter, the record-swallowing, and now this), and the parser is meaningfully better for it. |
|
Claude finished @frankbria's task in 4m 39s —— View job GLM precision review — E2B trust boundary (#967, synchronize)
Reviewed the synchronize commit ✅ GLM review: no defects found.The fix is correct, and — importantly — complete on its stated axis. Tracing
One residual edge, deliberately not flagged. A rename whose old filename happens to start with two status-alphabet characters and a space at index 2 (e.g. a file literally named The areas untouched by |
ReviewI re-traced the final diff (HEAD Confirmed sound on independent trace:
One low-confidence, non-blocking observation, not raised in the thread yet: in Minor style nit: the hunk adding Nothing here changes the bottom line from the last two review rounds: the containment fix, the |
…oad (#967) claude-review flagged this as speculative; it is real, and worse than it looked. `Path.resolve()` is not total — a symlink loop raises RuntimeError on Python 3.11 and 3.12, both inside this project's `requires-python`. Since `_safe_local_path` was called outside the per-file try/except, one loop anywhere under the workspace aborted the entire download loop instead of rejecting one path. It did not reproduce locally because 3.13 resolves loops quietly, so a real-symlink test passes on a 3.13 dev box while the crash stays live for most users. The test forces the raise instead, pinning the handling on every supported version. An unresolvable path is precisely one we must not write to, so rejecting (warned, like every other rejection here) is the right answer rather than propagating. Also collapses the stray blank lines around _PORCELAIN_STATUS_CHARS.
|
Both handled in the latest push. The "low-confidence, speculative" one turned out to be real — and the reason it looked speculative is worth recording.
My first attempt to pin it was a test that builds a real symlink loop — and it passed against the unfixed code, which is what nearly made me file this as "not reproducible". The reason: CPython 3.13 resolves loops quietly, and my local env is 3.13. So a real-symlink test would have gone green on a 3.13 dev box while the crash stayed live for most users. The committed test forces the raise via monkeypatch instead, so the handling is pinned on every supported version rather than on whichever interpreter happens to be installed. Fixed by catching Blank lines. Right, unintentional — four instead of two, and only one before Thanks for flagging it despite the low confidence; the version split is exactly the kind of thing a local test run hides. |
|
Claude finished @frankbria's task in 4m 0s —— View job GLM precision review — E2B trust boundary (#967, synchronize)
Reviewed the synchronize commit ✅ GLM review: no defects found.The fix is correct and minimal. I traced
The areas untouched by this commit were confirmed sound by the prior eight passes and re-traced here:
The one messaging nuance (parse-rejects folded into the user-facing "outside the workspace" count) was raised by an earlier GLM pass and is a documented, deliberate fold — not a defect, and not repeated. |
ReviewThis PR has already been through nine rounds of review (codex ×2, GLM ×4, claude ×2) in this thread, and every round found and fixed a real issue: the One residual finding, low severity, not security-relevant — the status-alphabet fix narrows but does not close the rename-old-path misparse class it was meant to fix.
So this doesn't escape containment and isn't silently dropped in the strict AC2 sense (a warning is still logged) — but it's a misleading one ("failed to download" rather than "malformed/unconsumed old path"), it's not counted in Given how narrow the trigger is (old filename must start with two of No other new findings. The overall design — containment as the actual security boundary, |
) claude-review was right that the status-alphabet fix narrowed the rename-old-path misparse without closing it, and right that my last comment overstated it. `AD HOC.txt` is a plausible filename whose first two characters are both real status characters followed by a space, so the lookahead heuristic still misread it as a record. Rather than narrow the heuristic again, remove the thing it was guessing about. `git status --porcelain -z --no-renames` reports a rename as an independent delete + add — verified against real git: -z R new.py\0AD HOC.txt\0 -z --no-renames D AD HOC.txt\0A new.py\0 There is no paired field, so there is no lookahead, so both failure directions disappear at once: a hostile `R` header cannot swallow the record after it, and no old filename can be misread as a record. Every entry is a record. The rename branch and its heuristic are deleted. The delete half of that pair would otherwise be fetched and fail, so deleted paths are now skipped — they are not in the sandbox to read, and trying only produced a misleading "Failed to download" for a file that is meant to be gone. Applying deletions locally remains the parked #966 defect; this only stops us fetching a path we know is absent.
|
Right on both counts —
No paired field ⇒ no lookahead ⇒ both failure directions disappear at once: a hostile That the third attempt at a heuristic was still incomplete was the signal I should have taken earlier: the ambiguity was never in the parsing, it was in asking git for a format that has a context-dependent field. One consequence handled in the same commit: the delete half of that pair would otherwise be fetched and fail. Deleted paths are now skipped — they are not in the sandbox to read, and trying only produced a misleading "Failed to download" for a file that is meant to be gone. Applying deletions locally stays the parked #966 defect; this only stops us fetching a path we know is absent. Tests:
|
GLM precision review — E2B trust boundary (#967, synchronize)
|
Spotted while re-running the demo: the emitted line said "Rejected N path(s) outside the workspace — the sandbox tried to write somewhere it may not", but that count folds in unparseable records too, which are a different thing and not necessarily an attempted escape. Reworded to name both causes and point at the per-path log lines.
|
Claude finished @frankbria's task in 5m 6s —— View job GLM precision review — E2B trust boundary (#967, synchronize)Reviewed the two newest synchronize commits — ✅ GLM review: no defects found.No new inline findings; the table below is empty. Each item is a confirmation against the checked-out code.
Re-confirmed clean (untouched by these two commits, validated in prior passes and re-traced here):
One residual I deliberately did not flag, consistent with the prior GLM/claude passes: if copy detection were forced on in the sandbox repo (e.g.
|
ReviewI independently re-traced the full diff (all 7 commits, final HEAD Containment ( Parsing ( Outbound ( Two non-blocking observations, both low severity:
No security-relevant defects found. Test coverage ( |
ReviewThis thread already has ten review rounds (codex ×2, GLM ×5, claude ×3), and every round found and fixed something real: the The Containment ( Outbound ( No new correctness or security issues found. One genuinely trivial, non-blocking observation not raised before: Note there's an automated GLM review still in progress against these last two commits ( |
1 participant
Closes #967.
The E2B sandbox is a trust boundary in both directions and neither was enforced. #966 gated the engine as experimental, but anyone who sets
CODEFRAME_ENABLE_CLOUD_ENGINE=1was exposed to both of these — which is exactly why the issue was carved out of #966's parked list.Threat model
The coding agent has arbitrary command execution inside the sandbox before
_download_changed_filesruns. It can shadow thegitbinary. So the output ofgit status --porcelainis attacker-controlled input, and nothing git normally guarantees can be relied on.Inbound: paths escaped the workspace
local = workspace_path / rel_pathhad no containment check, followed bymkdir(parents=True, exist_ok=True)and a write. Two ways out, both confirmed:Reproduced end to end against a real filesystem, then re-run after the fix:
_safe_local_pathresolves both sides and requires the candidate under the root:startswithcheck gets wrong/workspace//abs/path. This is the case that distinguishes the absolute check from the containment check — without it the check is untested redundancy (see below)files.readand before anymkdir, so a rejected path costs no round trip and creates no directorylogger.warning-ed, counted, and emitted — never silently dropped (AC2)Parsing:
-z, verbatim--porcelain -zemits each path as raw bytes. Verified against real git rather than from memory:So
-zremoves both problems at once: no C-quoting to decode, and no" -> "rename separator to be ambiguous with a filename containing that string (which used to split such a path in half). Paths are then taken verbatim — a file genuinely named"a.py"keeps its quotes instead of being rewritten toa.pyover the top of a different file.Containment, not parsing, is the security boundary. That is what lets the parser stay simple: hostile input does not need to be understood, only kept inside the tree.
Outbound: build artifacts shipped unscanned
The scanner and the uploader kept separate exclusion sets, and the scanner's was the wider one — it skipped
dist/build/.tox/.eggswhile the uploader happily shipped them. A.envor key baked into a build artifact reached the third-party sandbox unscanned, defeating the adapter's abort-on-secrets contract.One
EXCLUDED_DIRSnow, exported fromcredential_scannerand imported by the uploader, with build output removed from it. A test asserts they are literally the same object, so they cannot drift apart again.End to end: a secret under
dist/now makesadapter.run()returnstatus='failed'withcredential_scan_blocked == 1and zero sandboxes created.Verification
tests/adapters/test_e2b_trust_boundary_967.py— traversal, absolute, deep.., symlink-out, symlink-in (false-reject guard), no-mkdir-outside, warned/counted/not-read, verbatim quoted + UTF-8 + tab names, rename handling, the" -> "filename, one-shared-constant,dist/abort.pytest tests/ --ignore=tests/e2e -m "not lifecycle"→ 6117 passed, 49 skipped, 0 failed (16m04s), run underenv -u DATABASE_PATH -u CODEFRAME_AUTH_REQUIRED -u ANTHROPIC_API_KEY -u AUTH_SECRET -u OPENAI_API_KEYto mirror CI.ruff checkclean.codex review --base main, twice. The first pass caught a real bug I introduced — see below. The second is clean.The review caught a bug worth naming
I switched to
-zand wrote a C-unquoter for git's\303\251octal escapes in the same change, both from memory of the format. codex [P2] pointed out they contradict:-zis precisely what removes the quoting, so unquoting its output would rewrite a legitimately-quoted filename. I verified with real git (output above) and deleted the unquoter rather than keeping it as defense in depth — it was not dead code, it was a correctness bug added while hardening a security boundary.Known limitations
dist/can trip theapi_key = "..."content pattern and block a run. That is the secure default and what AC5 requires — a directory the scanner will not read must not be a directory we upload — but it is a real behaviour change for anyone with a large built tree.completedwith the safe files downloaded and the reject count emitted. Failing the whole run on a rejection is arguably better, but it belongs with the#966PARKED item "any file that fails to sync back makes the run report non-completed", not here.--engine cloud/--isolation cloudbehind an experimental flag and remove it from the advertised surface #966 PARKED E2B defects (codeframe-aipackage name,CommandExitException, sync-back semantics, upload batching, autospec mocks) are untouched by design.