fix(sandbox): restore archived file modes when extracting a workspace tar

[hsusul]

Summary

Component: src/agents/sandbox/util/tar_utils.py — safe_extract_tarfile(), the workspace-tar extractor used by UnixLocalSandbox.hydrate_workspace().

Problem: safe_extract_tarfile creates every regular file with a hardcoded 0o600 and never applies the archived member mode. A persist_workspace() → hydrate_workspace() round trip therefore rewrites every file in the restored workspace to owner-read/write only. Executable scripts in the workspace — a checked-in ./bin/start, a .venv/bin/* wrapper, node_modules/.bin/* — come back non-executable, so shell commands that worked before the snapshot fail after the restore with a permission error.

Repro (no key, no live service):

import io, os, stat, tarfile, tempfile
from pathlib import Path
from agents.sandbox.util.tar_utils import safe_extract_tarfile

with tempfile.TemporaryDirectory() as td:
    src = Path(td) / "src"
    src.mkdir()
    script = src / "run.sh"
    script.write_text("#!/bin/sh\necho hi\n")
    script.chmod(0o755)

    buf = io.BytesIO()
    with tarfile.open(fileobj=buf, mode="w") as tar:
        tar.add(src, arcname=".")          # member mode is 0o755
    buf.seek(0)

    dest = Path(td) / "dest"
    with tarfile.open(fileobj=buf, mode="r:*") as tar:
        safe_extract_tarfile(tar, root=dest, allow_external_symlink_targets=False)

    print(oct(stat.S_IMODE((dest / "run.sh").stat().st_mode)))  # 0o600
    print(os.access(dest / "run.sh", os.X_OK))                  # False

Before: 0o600, os.access(..., os.X_OK) is False.
After: 0o755, os.access(..., os.X_OK) is True.

Root cause: _write_file() passes 0o600 as the creation mode to os.open() and stops there. That value is only meant to keep the file private while it is being written; nothing ever restores member.mode. The custom extractor exists to enforce a stricter containment policy than TarFile.extractall(), and when it replaced the stdlib path it also dropped the stdlib's mode handling.

Fix: apply the archived mode with os.fchmod() on the still-open descriptor, after shutil.copyfileobj() completes and the buffer is flushed. The file therefore keeps its private 0o600 creation mode for as long as it may hold partial data: a concurrent reader never sees a half-written file through its final readable/executable mode, and a copy that fails partway (interrupted input, full disk) leaves the partial file at 0o600 rather than at those permissions. The mode is sanitized with the same policy the standard library's tarfile data filter applies to regular files, which is the policy this extractor replaced:

• mode & 0o755 — setuid, setgid, sticky and group/other write bits are dropped, so an untrusted archive cannot plant a setuid binary or a world-writable file.
• execute bits are cleared unless the owner had 0o100.
• | 0o600 — the owner always keeps read and write access.

Why minimal:

• One helper plus one call, in the one place regular files are written. No change to validation, traversal, symlink or hardlink policy, or to any public signature.
• Directory and symlink modes are deliberately left alone. That matches the same stdlib data filter ("Ignore mode for directories & symlinks"), and it avoids the ordering hazard of applying a restrictive archived directory mode before that directory's children are extracted.
• os.fchmod is applied to the open descriptor rather than the path, so it cannot follow a path that changed after os.open(..., O_EXCL | O_NOFOLLOW) succeeded, and no extra stat/chmod round trip on the path is introduced.
• Guarded by hasattr(os, "fchmod"), mirroring the existing hasattr(os, "O_NOFOLLOW") guard two lines above, because os.fchmod is Unix-only and this suite runs in the Windows CI job.

Non-goals: the zip/source-archive materialization path in sandbox/session/archive_extraction.py, directory and symlink modes, and ownership (uid/gid) restoration — the stdlib data filter drops ownership too, and doing otherwise would need privileges the extractor does not have.

Test plan

New tests in tests/sandbox/test_tar_utils.py, all skipped on Windows via sys.platform == "win32" because they assert POSIX permission bits (the existing suite already runs unskipped on Windows, so the new cases had to opt out rather than the file):

• test_safe_extract_tarfile_restores_regular_file_modes — 11 parametrized cases pinning the sanitizing policy: 0o755→0o755, 0o644→0o644, 0o600→0o600, 0o700→0o700, 0o444→0o644 (owner stays writable), 0o000→0o600 (owner stays readable), 0o777→0o755 (group/other write dropped), 0o655→0o644 (execute without owner-execute dropped), and 0o4755/0o2755/0o1755 → 0o755 (setuid/setgid/sticky dropped).
• test_safe_extract_tarfile_keeps_workspace_scripts_executable — the real-world symptom: a nested ./bin/start stays executable and a sibling README.md does not become executable.
• test_safe_extract_tarfile_restores_mode_when_replacing_an_existing_file — the replace path (_prepare_replaceable_leaf unlinks, then re-creates), extracting 0o644 and then 0o755 over it.
• test_safe_extract_tarfile_keeps_a_partially_written_file_private — the failure path. tar.extractfile is patched to return a payload that yields one chunk and then raises, so the copy fails mid-write; the test asserts the partial bytes did land, that the destination is still 0o600, and that it is not executable.

_file() in that test module gained an optional mode argument; every existing call site is unchanged.

Red/green was verified twice, by toggling only src/agents/sandbox/util/tar_utils.py with the tests already in place:

• against upstream/main: 11 failed, 37 passed → 49 passed. The three mode cases that pass in both directions (0o600, and the two clamped-to-0o600 boundaries) are kept as boundary coverage.
• against the earlier ordering in this PR (fchmod before the copy): test_safe_extract_tarfile_keeps_a_partially_written_file_private fails with assert 0o755 == 0o600 on the partially written file, and passes once the fchmod moved after the copy and flush.

Validation, all from a clean worktree branched at upstream/main (237716cf), Python 3.12.13, macOS:

• uv run pytest tests/sandbox/test_tar_utils.py -q → 49 passed (run 3× consecutively, identical)
• uv run pytest tests/sandbox -q → 1005 passed, 2 skipped
• make format → 864 files left unchanged; ruff check --fix all checks passed
• make lint → all checks passed
• make typecheck → mypy: no issues in 851 source files; pyright: 0 errors, 0 warnings
• make tests → 7078 passed, 28 skipped; serial suite 77 passed, 11 skipped, 55 deselected
• bash .agents/skills/code-change-verification/scripts/run.sh → all commands passed
• git diff --check → clean

Not run: make coverage and make build-docs (no docs touched); the Windows CI job — the new tests skip there by construction and the runtime change is behind hasattr(os, "fchmod"), but the Windows behavior itself is unverified locally.

Issue number

None — found by reading safe_extract_tarfile against the stdlib tarfile filter policy it replaced.

Checks

• I've added new tests, if relevant
• I've run .agents/skills/code-change-verification/scripts/run.sh
• I've confirmed all verification steps pass
• If using Codex, I've run /review before submitting this PR

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4cd2d509f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[seratch]

The mode-restoration fix is needed and the sanitization policy is appropriate. Before merge, please keep the new file at its initial 0o600 mode until the payload has been copied successfully and flushed, then apply fchmod() on the still-open descriptor. As written, the final readable/executable mode becomes visible while the file may still contain partial data, and a copy failure leaves that partial file with final permissions.

Please also add a failure-path regression test that makes the payload copy fail after writing some bytes and asserts the destination remains 0o600. With that change, the PR should be ready for another review.

[hsusul]

Good catch — fixed in 9b06b8e0.

os.fchmod() now runs after shutil.copyfileobj() returns and out.flush() has pushed the buffer out, still on the same open descriptor. The file keeps its 0o600 creation mode for the whole window in which it may hold partial data, so a concurrent reader never sees a half-written payload through the final readable/executable mode, and a copy that fails partway leaves the partial file at 0o600 instead of at those permissions.

Added test_safe_extract_tarfile_keeps_a_partially_written_file_private for the failure path: tar.extractfile is patched to return a payload that yields one chunk and then raises OSError, so the copy fails after some bytes are written. It asserts the partial bytes did land, that the destination is still 0o600, and that it is not executable.

That test is a real regression test for this exact ordering — with the previous commit's placement it fails with assert 0o755 == 0o600 on the partially written file, and passes with the new placement.

Verification re-run in full on the updated branch: uv run pytest tests/sandbox/test_tar_utils.py -q 49 passed (3 consecutive identical runs), uv run pytest tests/sandbox -q 1005 passed / 2 skipped, make tests 7078 passed / 28 skipped (serial 77 passed), make format / make lint / make typecheck clean, bash .agents/skills/code-change-verification/scripts/run.sh all commands passed, git diff --check clean. PR description updated to match.