Skip to content

fix(collect): read only the agent's own regular files, within a deadline - #607

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

earakely-scale wants to merge 5 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 wasn't bounded by the file. 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 streamed onto the operator's disk without end, and a FIFO hung the step.
    • The Modal adapter read the whole stream into memory.

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:
    • 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.
  • 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.
  • CUA path: the size probe refuses anything but a regular file ([ -f ], links followed).
  • 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.
  • 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;
    • 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;
    • 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;
    • 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
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 and a FIFO were each refused in 0.4 s.

Not covered here

  • No size cap: a regular file is read whatever its size, as before. A large or sparse file the agent leaves is read in full, bounded by its own size and the deadline. On providers whose SDK returns the output whole, it is also held in memory.
  • 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: 3/5

Restore the file-size limit before merging so agent-created files cannot exhaust the collector's disk.

Fix All in CursorFindings

  1. P1 Security Agent files fill operator disk ▶
Fix with agent prompt
### Issue 1
src/agent_env/task_step/task_steps/collect_artifacts.py:932-935
Removing the `max_bytes` checks lets an agent exhaust the collector's disk with a huge sparse regular file under `base_path`. `_collect_items` now passes the full reported size into `_collect_file`, which writes it to a local temporary file. The decoder only rejects bytes beyond that source size, and the deadline grows with it: an 8 TiB file gets roughly 97 days. `_controller_get_file` likewise no longer rejects oversized files.

Restore a configurable limit before either read, with a bounded default.

**How this was verified:** Agent-created regular files reach local temporary-file writes without a maximum-size check, and the remaining byte and time limits grow with the source file's reported size.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

This PR restricts manifest paths, checks local-host file locations, refuses non-regular files, streams Modal output, and cleans archive names.

  • Manifest entries stay under the chosen folder and get safe names.
  • Local sandbox collection stays inside its work folder or chosen base folder.
  • Artifact reads accept regular files and stop at a size and time limit.
  • Download All keeps artifact names inside the extracted folder.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Agent file] --> B[Check path and regular-file type]
    B --> C[Read reported size without maximum]
    C --> D[Write local temporary file]
    D --> E[Upload artifact]
Loading

Reviews (8) · Last reviewed commit: "revert(collect): drop the per-file size ..." · 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 3 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>
Collect reads a regular file of any size again, as before this branch. Reads
stay bounded to the size stat reports and to the deadline.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@earakely-scale earakely-scale changed the title fix(collect): read only the agent's own regular files, capped in size and time fix(collect): read only the agent's own regular files, within a deadline Nov 12, 2026
Comment on lines +932 to 935
file_size = -1 if read_path is None else await self._get_file_size(sandbox, container, read_path)
if file_size < 0:
logger.warning(f"File not found: {source_path}")
continue

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security Agent files fill operator disk

Removing the max_bytes checks lets an agent exhaust the collector's disk with a huge sparse regular file under base_path. _collect_items now passes the full reported size into _collect_file, which writes it to a local temporary file. The decoder only rejects bytes beyond that source size, and the deadline grows with it: an 8 TiB file gets roughly 97 days. _controller_get_file likewise no longer rejects oversized files.

Restore a configurable limit before either read, with a bounded default.

How this was verified: Agent-created regular files reach local temporary-file writes without a maximum-size check, and the remaining byte and time limits grow with the source file's reported size.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agent_env/task_step/task_steps/collect_artifacts.py
Line: 932-935

Comment:
**Agent files fill operator disk**

Removing the `max_bytes` checks lets an agent exhaust the collector's disk with a huge sparse regular file under `base_path`. `_collect_items` now passes the full reported size into `_collect_file`, which writes it to a local temporary file. The decoder only rejects bytes beyond that source size, and the deadline grows with it: an 8 TiB file gets roughly 97 days. `_controller_get_file` likewise no longer rejects oversized files.

Restore a configurable limit before either read, with a bounded default.

**How this was verified:** Agent-created regular files reach local temporary-file writes without a maximum-size check, and the remaining byte and time limits grow with the source file's reported size.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Cursor Fix in Claude Code Fix in Codex

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deliberate: the maintainers took the size cap out of this PR, so a regular file is read whatever its size, as before this change. The PR body lists it under "Not covered here", and the remaining gap is recorded on the tracking issue. A cap can come in a follow-up if it's needed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. Since the size cap was deliberately removed, the PR explicitly records the limitation, and the follow-up is tracked, this isn’t a defect to fix in this PR. I’m withdrawing the finding; the disk-exhaustion risk remains a documented follow-up.

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