Skip to content

fix(collect): read only the agent's own regular files, capped in size and time - #607

Open
earakely-scale wants to merge 4 commits into
mainfrom
edgararakelyan/collect-artifacts-hardening
Open

earakely-scale wants to merge 4 commits into
mainfrom
edgararakelyan/collect-artifacts-hardening

Conversation

@earakely-scale

@earakely-scale earakely-scale commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator

Why

collect_artifacts copies what an agent leaves behind onto the operator's side. Three flaws let the agent choose more than its own deliverables:

  • A manifest could name any file. In manifest mode (manifest_step_id) the paths come from the agent's own structured output. Every absolute path was read, inside base_path or not.
    • Collecting from a local sandbox's host (sandbox_name alone) runs on the operator's machine. So a manifest naming ~/.ssh/id_rsa or ~/.aws/credentials copied that file into the run's object store.
    • A link the agent left under base_path did the same.
  • A manifest's names became paths. Manifest names were stored verbatim as the collected keys, and the explorer's "Download All" zip used each key as a member name. So a name like ../../.bashrc became an archive entry that climbs out of the folder it's extracted into, holding bytes the agent wrote.
  • A read had no size or time limit. A file was read with base64 < path until the sandbox closed the stream, and its size was checked only afterwards.
    • A link to /dev/zero or an 8 TiB sparse file streamed onto the operator's disk without end, and a FIFO hung the step.
    • The Modal adapter read the whole stream into memory.
    • The CUA path had no cap either.

What changes

  • Manifest paths:
    • A manifest entry is read only if it's under base_path once normalized. Any other entry is skipped with a warning.
    • An entry's name becomes a key with no separators and no . or .. segments, or the file's name if nothing is left. ../../.bashrc is stored as .bashrc, and logs/run.json as logs_run.json. Names that clean to the same key get key (2), key (3) and so on.
    • Entries are matched against base_path normalized.
  • Local sandbox host: collecting from a local sandbox's host reads a file only if it resolves (readlink -f) inside the sandbox's work dir, or inside base_path when the task names a folder outside /app. It reads the resolved path. Anything else, a link out included, is a hard failure for that file.
    • A sandbox now declares app_dir_on_this_machine: the folder on this machine its host commands see as /app. It's None by default, and the local sandbox's work dir. Collect asks the sandbox for it rather than checking its class. ON_THIS_MACHINE doesn't answer the question, because a smol VM sandbox sets it but runs its commands inside a VM.
    • A base_path outside /app stays readable, because a run_code script on a local VM sandbox writes to host folders the task chooses, and a task author can already run commands on that host. Under /app only the work dir counts, since the agent could swap a folder there for a link.
  • Regular files only, capped:
    • The size probe is stat -L -c %f:%s, and the raw mode bits it prints are checked with stat.S_ISREG; %F's type names are translated by locale. Anything but a regular file is refused before it's read: a device, FIFO, directory or socket, or a link to one.
    • A file over the step's new max_bytes is refused too. max_bytes defaults to 16 GiB, and to_dict/from_dict carry it.
  • Bounded reads:
    • The sandbox runs head -c SIZE < path | base64, so it sends no more than the size it reported, and the decoder refuses any byte past that size.
    • A read, the sandbox's exec included, is abandoned once it outlasts 300 s plus one second per MiB. Some providers run the whole command inside exec.
  • Modal streams: _ModalStreamAdapter is now async-iterable. Modal and Modal VM output is decoded to disk as it arrives instead of being read whole. Providers whose SDK returns the output whole are still bounded by the cap.
  • CUA path: the size probe refuses anything but a regular file ([ -f ], links followed). A file over max_bytes is refused before the first read.
  • Explorer zip: every "Download All" member name is a clean relative path:
    • backslashes count as separators;
    • drive prefixes, leading separators and . segments are dropped;
    • .. climbs at most to the archive root;
    • a name with nothing left becomes artifact.

Compatibility

  • Manifest entries outside base_path: they used to be collected, keyed by their file name; now they're skipped. A task whose agent reports deliverables in several folders needs a base_path that holds them all.
  • Manifest names with a /: logs/run.json is now keyed logs_run.json.
  • Links on a local sandbox's host: a link that leaves the work dir and base_path is no longer followed.
  • Large files: a file over 16 GiB needs max_bytes set on its step. The default sits well above the files collected in practice.
  • head in the sandbox: the read now also runs head, from the same coreutils that already provide base64.

Tests

  • Unit:
    • a manifest entry outside base_path, or climbing out with .., is skipped;
    • manifest names lose their separators and dot segments;
    • a device, FIFO, directory or socket is refused by its mode bits, and an empty regular file is sized 0;
    • a file over max_bytes is a hard failure and is never read;
    • the read command is bounded by the reported size;
    • a longer stream is refused and its temp file removed;
    • a read past its deadline is abandoned;
    • max_bytes round-trips;
    • the CUA size probe, run by a real shell, sizes a regular file and refuses a link to /dev/zero, a FIFO or a directory, and a file over the cap is refused before any read;
    • on a real local sandbox, a link out of the work dir is refused and an author-named base_path is read;
    • only the local sandbox declares a folder on this machine;
    • Modal output streams;
    • a base_path under /app that the agent swapped for a link to a home folder reaches nothing outside the work dir;
    • names that clean to one key keep a key each;
    • base_path is matched normalized;
    • an exec that never returns is held to the deadline;
    • the explorer smoke checks ../../x, /abs/../../y, backslashes and drive prefixes.
  • On an agent-environments-admin EC2 box, before the review fixes in the third commit, which CI covers:
    • Unit suite: 14,422 passed and 6 failed.
      • Five of the failures also fail on main on that box, with a missing command.
      • The sixth was the rule that code outside the providers must not check a sandbox's class. That's fixed by the declaration above, and the affected tests were rerun: 425 passed.
    • Explorer: the smoke suite passed.
    • Local-provider integration: the collect, verify, run_code named-sandbox and bundle-run tests, 25 passed, no skips.
    • Before and after, on real containers and a real local sandbox, each case cut at 20 s:
Case main this branch
report.pdf is a link to /dev/zero 551 MB streamed to the temp dir when cut refused in 0.2 s, not a regular file
an 8 TiB sparse file, base_path enumerated 556 MB streamed when cut refused, over max_bytes; the rest collected
a FIFO hung until cut refused in 0.2 s
a manifest naming an operator file, directly and via /app/artifact/../.. collected, canary uploaded both skipped with a warning
a link in the work dir to an operator file refused only because stat without -L sized the link itself refused, resolves outside
manifest names ../../.bashrc, ..\..\evil.sh stored as given stored as .bashrc, evil.sh
the explorer's JSZip archive of those keys plus ../../x, /abs/../../y members ../../.bashrc, ..\..\evil.sh, ../../x, abs/../../y members .bashrc, evil.sh, x, y
  • Live Modal, one python:3.12-slim sandbox, collected through ModalSandbox's exec adapter:
    • a 256 MiB random file arrived with its sha256 intact. The collecting process's peak memory grew 16 MiB, against 946 MiB on main, which read the whole stream;
    • a link to /dev/zero, a FIFO and an 8 TiB sparse file were each refused in 0.4 s.

Not covered here

  • Swapping the file mid-read: the agent can still swap the file between the size probe and the read. The read then gets at most the reported size, and is refused unless it's exactly that size. On the local host, the read uses the path resolved at the check.

🤖 Generated with Claude Code

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge, with no new actionable issues found.

Summary

collect_artifacts limits which files agents can deliver and bounds their reads.

  • Manifest entries stay under the base folder and get safe, distinct keys.
  • Artifact reads accept regular files and stop at a size and time limit.
  • Local sandbox collection stays inside approved host folders.
  • Collected artifact names stay inside the explorer's download zip.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Choose files] --> B[Check paths and names]
  B --> C[Check regular file and size]
  C --> D[Read within size and time limits]
  D --> E[Upload and register artifacts]
  E --> F[Download with safe archive names]
Loading

Reviews (7) · Last reviewed commit: "fix(collect): tell a regular file by its..." · Reviewed by Greptile

earakely-scale and others added 2 commits November 10, 2026 16:18
… and time

A manifest entry is read only under base_path, and its name is stored as a
key with no separators or dot segments. On a local sandbox's host, which is
this machine, a file is read only where it resolves inside the sandbox's work
dir or base_path; sandboxes declare that folder as app_dir_on_this_machine.

Only regular files are read, links followed, up to the step's max_bytes
(16 GiB by default). The sandbox sends at most the size it reported, the
decoder refuses more, and a read past its deadline is abandoned. Modal exec
output streams instead of being read whole, and the CUA path applies the same
checks.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each collected key becomes a clean relative member name: backslashes
separate, drive prefixes, leading separators and '.' segments are dropped,
and '..' climbs no higher than the archive root.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
earakely-scale and others added 2 commits November 10, 2026 16:25
…n, time the whole read

Under /app only the sandbox's work dir counts as a root: an agent could swap a
folder there for a link out. A base_path outside /app still counts, resolved.
Two manifest names that clean to one key get "key (2)" and on. The deadline
covers exec too, which some providers run to completion. Manifest paths are
matched against base_path normalized.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ranslates

stat's %F names the file type in the system's language, so on a host with a
non-English locale every file would have been refused. %f prints the raw mode,
checked with stat.S_ISREG.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant