From 329abf8168f6d00bce01d56df4ebb364d129688d Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Wed, 2 Sep 2026 16:53:21 +0000 Subject: [PATCH 01/11] fix(sandbox): make Docker persist_workspace archives restorable Docker's persist_workspace() stages a copy of the workspace, has the daemon archive it, and rewrites the member prefix in Python with strip_tar_member_prefix(). That rewrite raised UnsafeTarMemberError ("hardlink member not allowed", "unsupported member type") as soon as the archive contained a hardlink member or a FIFO, so snapshotting a workspace where uv or pnpm had hardlinked installed packages, or a dev server had left a FIFO behind, failed outright. An absolute symlink target under the workspace root survived persist but was refused by the strict hydrate extractor. Rewrite those members while stripping the prefix: hardlink members are stored as regular files carrying the target's payload (the source is spooled to a temporary file so the earlier member can be re-read), FIFOs and device nodes are dropped, and, when the caller passes the workspace root, absolute symlink targets under it become relative to the link's directory. Absolute targets outside the workspace are left unchanged for hydrate's policy. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01BN4v25msJgrjNgac97g1Az --- src/agents/sandbox/sandboxes/docker.py | 6 +- src/agents/sandbox/util/tar_utils.py | 70 ++++++++++++++++++-- tests/sandbox/test_tar_utils.py | 90 +++++++++++++++++++++++++- 3 files changed, 157 insertions(+), 9 deletions(-) diff --git a/src/agents/sandbox/sandboxes/docker.py b/src/agents/sandbox/sandboxes/docker.py index 8ca4febe85..1aae2536a2 100644 --- a/src/agents/sandbox/sandboxes/docker.py +++ b/src/agents/sandbox/sandboxes/docker.py @@ -1371,7 +1371,11 @@ async def persist_workspace(self) -> io.IOBase: staging_workspace, cleanup_path=staging_parent, ) - return strip_tar_member_prefix(root_prefixed_archive, prefix=staging_workspace.name) + return strip_tar_member_prefix( + root_prefixed_archive, + prefix=staging_workspace.name, + relativize_symlinks_under=root, + ) except docker.errors.NotFound as e: raise WorkspaceArchiveReadError(path=error_root, cause=e, retryable=False) from e except docker.errors.APIError as e: diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index 55adbd77e4..cddbddab19 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -3,11 +3,12 @@ import copy import io import os +import posixpath import shutil import tarfile import tempfile from collections.abc import Iterable -from pathlib import Path, PurePosixPath, PureWindowsPath +from pathlib import Path, PurePath, PurePosixPath, PureWindowsPath from typing import cast @@ -100,24 +101,58 @@ def safe_tar_member_rel_path( return Path(*rel.parts) -def strip_tar_member_prefix(data: io.IOBase, *, prefix: str | Path) -> io.IOBase: +def strip_tar_member_prefix( + data: io.IOBase, + *, + prefix: str | Path, + relativize_symlinks_under: str | PurePath | None = None, +) -> io.IOBase: """Return a seekable tar stream after replacing a leading member prefix with `.`. For example, Docker archives a workspace copied to `/tmp/stage/workspace` as `workspace/...`; portable workspace snapshots should store the same files as `.` and `...`, independent of the source backend's root name. + + The rewritten archive only contains members that the strict hydrate extractor + accepts. Archivers such as Docker's represent a second hardlinked path as a + hardlink member and keep FIFOs and device nodes, and ordinary workspaces contain + them (``uv`` and ``pnpm`` hardlink installed packages, dev servers leave FIFOs + behind). Hardlink members are stored as regular files with the target's payload, + FIFOs and device nodes are dropped, and when `relativize_symlinks_under` names + the workspace root, an absolute symlink target under that root becomes relative + to the link's own directory so it restores under any root. """ prefix_rel = _normalize_rel(prefix) if prefix_rel == Path(): raise ValueError("tar member prefix must not be empty") + symlink_root: PurePosixPath | None = None + if relativize_symlinks_under is not None: + symlink_root = PurePosixPath( + relativize_symlinks_under.as_posix() + if isinstance(relativize_symlinks_under, PurePath) + else relativize_symlinks_under + ) out = tempfile.TemporaryFile() try: - with data: - with tarfile.open(fileobj=data, mode="r|*") as src: + # Spool the source so hardlink members can copy their target's payload; the + # incoming stream is not seekable and tar stores the payload once. + with data, tempfile.TemporaryFile() as spooled: + shutil.copyfileobj(data, spooled) + spooled.seek(0) + with tarfile.open(fileobj=spooled, mode="r:*") as src: with tarfile.open(fileobj=out, mode="w|") as dst: - for member in src: + for member in src.getmembers(): + if member.isfifo() or member.ischr() or member.isblk(): + continue + source = member + if member.islnk(): + source = src.getmember(member.linkname) + member = copy.copy(member) + member.type = tarfile.REGTYPE + member.linkname = "" + member.size = source.size rel_path = safe_tar_member_rel_path( member, allow_symlinks=True, @@ -141,8 +176,14 @@ def strip_tar_member_prefix(data: io.IOBase, *, prefix: str | Path) -> io.IOBase rewritten.name = stripped_name rewritten.pax_headers = dict(member.pax_headers) rewritten.pax_headers.pop("path", None) - if member.isreg(): - fileobj = src.extractfile(member) + if rewritten.issym() and symlink_root is not None: + rewritten.linkname = _relative_symlink_target( + rewritten.linkname, + link_name=stripped_name, + root=symlink_root, + ) + if rewritten.isreg(): + fileobj = src.extractfile(source) if fileobj is None: raise UnsafeTarMemberError( member=member.name, @@ -165,6 +206,21 @@ def strip_tar_member_prefix(data: io.IOBase, *, prefix: str | Path) -> io.IOBase raise +def _relative_symlink_target(linkname: str, *, link_name: str, root: PurePosixPath) -> str: + """Make an absolute symlink target under `root` relative to the link's directory.""" + + target = PurePosixPath(linkname) + if not target.is_absolute(): + return linkname + normalized = PurePosixPath(posixpath.normpath(linkname)) + try: + target_rel = normalized.relative_to(root) + except ValueError: + return linkname + link_dir = PurePosixPath(link_name).parent + return posixpath.relpath(target_rel.as_posix() or ".", start=link_dir.as_posix()) + + def _normalize_rel(prefix: str | Path) -> Path: rel = prefix if isinstance(prefix, Path) else Path(prefix) posix = rel.as_posix() diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index 50402557c6..2c1a3abd6c 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -6,7 +6,7 @@ import sys import tarfile from dataclasses import dataclass -from pathlib import Path +from pathlib import Path, PurePosixPath import pytest @@ -16,6 +16,7 @@ safe_tar_member_rel_path, strip_tar_member_prefix, validate_tar_bytes, + validate_tarfile, ) @@ -179,6 +180,93 @@ def test_strip_tar_member_prefix_returns_workspace_relative_archive() -> None: assert tar.getnames() == [".", "pkg", "pkg/main.py", "pkg/python"] +def _prefixed_workspace_archive(*, external_symlink: bool) -> io.BytesIO: + """A `workspace/...` archive shaped like Docker's, with members hydrate refuses as-is.""" + + buf = io.BytesIO() + with tarfile.open(fileobj=buf, mode="w") as tar: + root = tarfile.TarInfo("workspace") + root.type = tarfile.DIRTYPE + tar.addfile(root) + sub = tarfile.TarInfo("workspace/sub") + sub.type = tarfile.DIRTYPE + tar.addfile(sub) + payload = b"shared" + regular = tarfile.TarInfo("workspace/a.txt") + regular.size = len(payload) + tar.addfile(regular, io.BytesIO(payload)) + hardlink = tarfile.TarInfo("workspace/sub/hardlink.txt") + hardlink.type = tarfile.LNKTYPE + hardlink.linkname = "workspace/a.txt" + tar.addfile(hardlink) + fifo = tarfile.TarInfo("workspace/dev.fifo") + fifo.type = tarfile.FIFOTYPE + tar.addfile(fifo) + abs_inside = tarfile.TarInfo("workspace/sub/abs_up") + abs_inside.type = tarfile.SYMTYPE + abs_inside.linkname = "/workspace/a.txt" + tar.addfile(abs_inside) + rel = tarfile.TarInfo("workspace/rel") + rel.type = tarfile.SYMTYPE + rel.linkname = "a.txt" + tar.addfile(rel) + if external_symlink: + outside = tarfile.TarInfo("workspace/outside") + outside.type = tarfile.SYMTYPE + outside.linkname = "/usr/bin/python3" + tar.addfile(outside) + buf.seek(0) + return buf + + +def test_strip_tar_member_prefix_rewrites_members_hydrate_refuses() -> None: + stripped = strip_tar_member_prefix( + _prefixed_workspace_archive(external_symlink=True), + prefix="workspace", + relativize_symlinks_under="/workspace", + ) + + with tarfile.open(fileobj=stripped, mode="r:*") as tar: + members = {member.name: member for member in tar.getmembers()} + assert "dev.fifo" not in members + hardlink = members["sub/hardlink.txt"] + assert hardlink.isreg() and hardlink.size == len("shared") + extracted = tar.extractfile(hardlink) + assert extracted is not None and extracted.read() == b"shared" + assert members["sub/abs_up"].issym() + assert members["sub/abs_up"].linkname == "../a.txt" + assert members["rel"].linkname == "a.txt" + # External absolute targets are left for hydrate's policy to decide. + assert members["outside"].linkname == "/usr/bin/python3" + + +def test_strip_tar_member_prefix_output_passes_strict_hydrate_validation( + tmp_path: Path, +) -> None: + stripped = strip_tar_member_prefix( + _prefixed_workspace_archive(external_symlink=False), + prefix="workspace", + relativize_symlinks_under=PurePosixPath("/workspace"), + ) + + with tarfile.open(fileobj=stripped, mode="r:*") as tar: + validate_tarfile(tar, allow_external_symlink_targets=False) + safe_extract_tarfile(tar, root=tmp_path, allow_external_symlink_targets=False) + + assert (tmp_path / "sub" / "hardlink.txt").read_bytes() == b"shared" + assert (tmp_path / "sub" / "abs_up").read_bytes() == b"shared" + assert not (tmp_path / "dev.fifo").exists() + + +def test_strip_tar_member_prefix_keeps_absolute_symlinks_without_a_root() -> None: + stripped = strip_tar_member_prefix( + _prefixed_workspace_archive(external_symlink=False), prefix="workspace" + ) + + with tarfile.open(fileobj=stripped, mode="r:*") as tar: + assert tar.getmember("sub/abs_up").linkname == "/workspace/a.txt" + + def test_strip_tar_member_prefix_rewrites_pax_path_headers() -> None: long_name = "workspace/" + ("a" * 120) + ".txt" payload = b"payload" From a64ccf615ff940bf868c11176e45a3b67af00d4e Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Thu, 3 Sep 2026 09:31:44 +0000 Subject: [PATCH 02/11] fix(sandbox): stream the Docker archive while rewriting hardlink members Keep reading the source archive as a stream instead of spooling it to a temporary file first. A hardlink member's payload is read back from the rewritten archive being written (recorded by original member name), so peak temporary usage stays at one archive rather than the source plus the output. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HCNKceEs9sPdb6aHK3FqPf --- src/agents/sandbox/util/tar_utils.py | 159 +++++++++++++++++---------- 1 file changed, 102 insertions(+), 57 deletions(-) diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index cddbddab19..78868dd83b 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -9,7 +9,7 @@ import tempfile from collections.abc import Iterable from pathlib import Path, PurePath, PurePosixPath, PureWindowsPath -from typing import cast +from typing import IO, cast class UnsafeTarMemberError(ValueError): @@ -117,7 +117,8 @@ def strip_tar_member_prefix( accepts. Archivers such as Docker's represent a second hardlinked path as a hardlink member and keep FIFOs and device nodes, and ordinary workspaces contain them (``uv`` and ``pnpm`` hardlink installed packages, dev servers leave FIFOs - behind). Hardlink members are stored as regular files with the target's payload, + behind). Hardlink members are stored as regular files with the target's payload (read + back from the rewritten archive, so the source is still streamed once), FIFOs and device nodes are dropped, and when `relativize_symlinks_under` names the workspace root, an absolute symlink target under that root becomes relative to the link's own directory so it restores under any root. @@ -136,65 +137,73 @@ def strip_tar_member_prefix( out = tempfile.TemporaryFile() try: - # Spool the source so hardlink members can copy their target's payload; the - # incoming stream is not seekable and tar stores the payload once. - with data, tempfile.TemporaryFile() as spooled: - shutil.copyfileobj(data, spooled) - spooled.seek(0) - with tarfile.open(fileobj=spooled, mode="r:*") as src: - with tarfile.open(fileobj=out, mode="w|") as dst: - for member in src.getmembers(): - if member.isfifo() or member.ischr() or member.isblk(): - continue - source = member - if member.islnk(): - source = src.getmember(member.linkname) - member = copy.copy(member) - member.type = tarfile.REGTYPE - member.linkname = "" - member.size = source.size - rel_path = safe_tar_member_rel_path( - member, - allow_symlinks=True, + # Stream the source once. A hardlink member carries no payload of its own, so its + # target's bytes are read back from the rewritten archive being written (recorded by + # original member name), which keeps temp usage at one archive instead of two. + written_payloads: dict[str, tuple[int, int]] = {} + with data, tarfile.open(fileobj=data, mode="r|*") as src: + with tarfile.open(fileobj=out, mode="w") as dst: + for member in src: + if member.isfifo() or member.ischr() or member.isblk(): + continue + payload: tuple[int, int] | None = None + if member.islnk(): + payload = written_payloads.get(member.linkname) + if payload is None: + reason = ( + f"hardlink target is not a file in the archive: {member.linkname}" + ) + raise UnsafeTarMemberError(member=member.name, reason=reason) + member = copy.copy(member) + member.type = tarfile.REGTYPE + member.linkname = "" + member.size = payload[1] + rel_path = safe_tar_member_rel_path( + member, + allow_symlinks=True, + ) + if rel_path is None: + stripped_name = "." + elif rel_path == prefix_rel: + stripped_name = "." + elif rel_path.parts[: len(prefix_rel.parts)] == prefix_rel.parts: + stripped_name = Path(*rel_path.parts[len(prefix_rel.parts) :]).as_posix() + else: + reason = f"member does not start with prefix: {prefix_rel.as_posix()}" + raise UnsafeTarMemberError( + member=member.name, + reason=reason, + ) + + rewritten = copy.copy(member) + rewritten.name = stripped_name + rewritten.pax_headers = dict(member.pax_headers) + rewritten.pax_headers.pop("path", None) + if rewritten.issym() and symlink_root is not None: + rewritten.linkname = _relative_symlink_target( + rewritten.linkname, + link_name=stripped_name, + root=symlink_root, ) - if rel_path is None: - stripped_name = "." - elif rel_path == prefix_rel: - stripped_name = "." - elif rel_path.parts[: len(prefix_rel.parts)] == prefix_rel.parts: - stripped_name = Path( - *rel_path.parts[len(prefix_rel.parts) :] - ).as_posix() - else: - reason = f"member does not start with prefix: {prefix_rel.as_posix()}" + if not rewritten.isreg(): + dst.addfile(rewritten) + continue + if payload is not None: + fileobj: IO[bytes] = cast(IO[bytes], _ArchivePayloadReader(out, *payload)) + else: + extracted = src.extractfile(member) + if extracted is None: raise UnsafeTarMemberError( member=member.name, - reason=reason, - ) - - rewritten = copy.copy(member) - rewritten.name = stripped_name - rewritten.pax_headers = dict(member.pax_headers) - rewritten.pax_headers.pop("path", None) - if rewritten.issym() and symlink_root is not None: - rewritten.linkname = _relative_symlink_target( - rewritten.linkname, - link_name=stripped_name, - root=symlink_root, + reason="missing file payload", ) - if rewritten.isreg(): - fileobj = src.extractfile(source) - if fileobj is None: - raise UnsafeTarMemberError( - member=member.name, - reason="missing file payload", - ) - try: - dst.addfile(rewritten, fileobj) - finally: - fileobj.close() - else: - dst.addfile(rewritten) + fileobj = extracted + try: + dst.addfile(rewritten, fileobj) + finally: + fileobj.close() + padded = -(-rewritten.size // tarfile.BLOCKSIZE) * tarfile.BLOCKSIZE + written_payloads[member.name] = (dst.offset - padded, rewritten.size) out.seek(0) with tarfile.open(fileobj=out, mode="r:*") as tar: @@ -206,6 +215,42 @@ def strip_tar_member_prefix( raise +class _ArchivePayloadReader(io.RawIOBase): + """Read a member payload back from the archive file that is still being written. + + Every read seeks to the payload and then restores the writer's position, so the reader + can be interleaved with `TarFile.addfile()` writing to the same file object. + """ + + def __init__(self, archive: IO[bytes], start: int, size: int) -> None: + super().__init__() + self._archive = archive + self._position = start + self._end = start + size + + def readable(self) -> bool: + return True + + def read(self, size: int = -1) -> bytes: + remaining = self._end - self._position + if size is None or size < 0 or size > remaining: + size = remaining + if size <= 0: + return b"" + write_position = self._archive.tell() + try: + self._archive.seek(self._position) + data = self._archive.read(size) + finally: + self._archive.seek(write_position) + self._position += len(data) + return data + + def close(self) -> None: + # The archive stays open for the writer; only this view closes. + io.RawIOBase.close(self) + + def _relative_symlink_target(linkname: str, *, link_name: str, root: PurePosixPath) -> str: """Make an absolute symlink target under `root` relative to the link's directory.""" From dce44cfdb8d318b38fd42c14d109d46218d49710 Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Fri, 4 Sep 2026 14:56:38 +0000 Subject: [PATCH 03/11] fix(sandbox): drop the stale PAX linkpath when relativizing a symlink target A symlink target longer than the ustar field is carried in a PAX "linkpath" record. Rewriting only TarInfo.linkname left that record pointing at the original absolute target, and addfile() emitted it, so the rewritten archive still held the absolute link and strict hydrate refused it. Remove the record; tobuf() re-derives it from the new linkname when needed. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFSMrLZZ3oq1dKmJFAofz6 --- src/agents/sandbox/util/tar_utils.py | 3 +++ tests/sandbox/test_tar_utils.py | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index 78868dd83b..5bd819b387 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -185,6 +185,9 @@ def strip_tar_member_prefix( link_name=stripped_name, root=symlink_root, ) + # A long source target lives in a PAX "linkpath" record that would + # otherwise override the rewritten linkname; tobuf() re-derives it. + rewritten.pax_headers.pop("linkpath", None) if not rewritten.isreg(): dst.addfile(rewritten) continue diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index 2c1a3abd6c..42ce203386 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -210,6 +210,12 @@ def _prefixed_workspace_archive(*, external_symlink: bool) -> io.BytesIO: rel.type = tarfile.SYMTYPE rel.linkname = "a.txt" tar.addfile(rel) + # Longer than the 100-byte ustar field, so tarfile records it in a PAX linkpath. + long_target = "/workspace/" + "/".join(["deeply-nested-directory"] * 5) + "/target.txt" + long_link = tarfile.TarInfo("workspace/long_link") + long_link.type = tarfile.SYMTYPE + long_link.linkname = long_target + tar.addfile(long_link) if external_symlink: outside = tarfile.TarInfo("workspace/outside") outside.type = tarfile.SYMTYPE @@ -236,6 +242,11 @@ def test_strip_tar_member_prefix_rewrites_members_hydrate_refuses() -> None: assert members["sub/abs_up"].issym() assert members["sub/abs_up"].linkname == "../a.txt" assert members["rel"].linkname == "a.txt" + long_link = members["long_link"] + assert long_link.linkname == "/".join(["deeply-nested-directory"] * 5) + "/target.txt" + assert "linkpath" not in long_link.pax_headers or ( + long_link.pax_headers["linkpath"] == long_link.linkname + ) # External absolute targets are left for hydrate's policy to decide. assert members["outside"].linkname == "/usr/bin/python3" From 2d09f4d0e0eadb26d3f0bbca529df96c388f8579 Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Fri, 4 Sep 2026 16:12:04 +0000 Subject: [PATCH 04/11] fix(sandbox): relativize in-workspace symlink targets that start with a double slash posixpath.normpath keeps two leading slashes, so //workspace/a.txt was not recognized as under the workspace root and stayed absolute; Linux resolves // as /, so collapse it before the containment check. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFSMrLZZ3oq1dKmJFAofz6 --- src/agents/sandbox/util/tar_utils.py | 4 +++- tests/sandbox/test_tar_utils.py | 5 +++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index 5bd819b387..da742797b0 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -260,7 +260,9 @@ def _relative_symlink_target(linkname: str, *, link_name: str, root: PurePosixPa target = PurePosixPath(linkname) if not target.is_absolute(): return linkname - normalized = PurePosixPath(posixpath.normpath(linkname)) + # normpath keeps exactly two leading slashes (POSIX leaves "//" implementation-defined); + # Linux resolves them as "/", so collapse them before the containment check. + normalized = PurePosixPath("/" + posixpath.normpath(linkname).lstrip("/")) try: target_rel = normalized.relative_to(root) except ValueError: diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index 42ce203386..f0e1152810 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -210,6 +210,10 @@ def _prefixed_workspace_archive(*, external_symlink: bool) -> io.BytesIO: rel.type = tarfile.SYMTYPE rel.linkname = "a.txt" tar.addfile(rel) + double_slash = tarfile.TarInfo("workspace/double_slash") + double_slash.type = tarfile.SYMTYPE + double_slash.linkname = "//workspace/a.txt" + tar.addfile(double_slash) # Longer than the 100-byte ustar field, so tarfile records it in a PAX linkpath. long_target = "/workspace/" + "/".join(["deeply-nested-directory"] * 5) + "/target.txt" long_link = tarfile.TarInfo("workspace/long_link") @@ -242,6 +246,7 @@ def test_strip_tar_member_prefix_rewrites_members_hydrate_refuses() -> None: assert members["sub/abs_up"].issym() assert members["sub/abs_up"].linkname == "../a.txt" assert members["rel"].linkname == "a.txt" + assert members["double_slash"].linkname == "a.txt" long_link = members["long_link"] assert long_link.linkname == "/".join(["deeply-nested-directory"] * 5) + "/target.txt" assert "linkpath" not in long_link.pax_headers or ( From fa4a43a6e50a3603b1ca6846cbe1db3238d62e75 Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Sat, 5 Sep 2026 07:58:51 +0000 Subject: [PATCH 05/11] fix(sandbox): narrow Docker snapshot normalization to FIFOs and in-workspace symlinks Address review: Docker stages the workspace with `cp -R`, which already copies hardlinked files independently, so drop the hardlink expansion (payload read-back, spooling) and keep only what the staged copy really carries and the strict hydrate extractor refuses: FIFO/device members are dropped, and absolute symlink targets under the workspace root are rebased onto the link's directory with their components kept verbatim. Only the root prefix is replaced, never normalized: with `alias -> sub/deep`, `/workspace/alias/../data.txt` names `sub/data.txt` and must stay `alias/../data.txt`. Add a Docker-session persist/hydrate test that goes through staging and reads the restored links. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFSMrLZZ3oq1dKmJFAofz6 --- src/agents/sandbox/util/tar_utils.py | 200 ++++++++++----------------- tests/sandbox/test_docker.py | 54 +++++++- tests/sandbox/test_tar_utils.py | 86 ++++++------ 3 files changed, 173 insertions(+), 167 deletions(-) diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index da742797b0..ec734726ad 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -3,13 +3,12 @@ import copy import io import os -import posixpath import shutil import tarfile import tempfile from collections.abc import Iterable from pathlib import Path, PurePath, PurePosixPath, PureWindowsPath -from typing import IO, cast +from typing import cast class UnsafeTarMemberError(ValueError): @@ -113,23 +112,19 @@ def strip_tar_member_prefix( as `workspace/...`; portable workspace snapshots should store the same files as `.` and `...`, independent of the source backend's root name. - The rewritten archive only contains members that the strict hydrate extractor - accepts. Archivers such as Docker's represent a second hardlinked path as a - hardlink member and keep FIFOs and device nodes, and ordinary workspaces contain - them (``uv`` and ``pnpm`` hardlink installed packages, dev servers leave FIFOs - behind). Hardlink members are stored as regular files with the target's payload (read - back from the rewritten archive, so the source is still streamed once), - FIFOs and device nodes are dropped, and when `relativize_symlinks_under` names - the workspace root, an absolute symlink target under that root becomes relative - to the link's own directory so it restores under any root. + The strict hydrate extractor refuses FIFOs, device nodes, and absolute symlink + targets, and a staged workspace copy (`cp -R`) legitimately carries the first two + kinds and absolute links into the workspace. FIFOs and device nodes are dropped, and + when `relativize_symlinks_under` names the workspace root, an absolute symlink target + under it is rebased onto the link's own directory with its components kept verbatim. """ prefix_rel = _normalize_rel(prefix) if prefix_rel == Path(): raise ValueError("tar member prefix must not be empty") - symlink_root: PurePosixPath | None = None + symlink_root: str | None = None if relativize_symlinks_under is not None: - symlink_root = PurePosixPath( + symlink_root = ( relativize_symlinks_under.as_posix() if isinstance(relativize_symlinks_under, PurePath) else relativize_symlinks_under @@ -137,76 +132,55 @@ def strip_tar_member_prefix( out = tempfile.TemporaryFile() try: - # Stream the source once. A hardlink member carries no payload of its own, so its - # target's bytes are read back from the rewritten archive being written (recorded by - # original member name), which keeps temp usage at one archive instead of two. - written_payloads: dict[str, tuple[int, int]] = {} - with data, tarfile.open(fileobj=data, mode="r|*") as src: - with tarfile.open(fileobj=out, mode="w") as dst: - for member in src: - if member.isfifo() or member.ischr() or member.isblk(): - continue - payload: tuple[int, int] | None = None - if member.islnk(): - payload = written_payloads.get(member.linkname) - if payload is None: - reason = ( - f"hardlink target is not a file in the archive: {member.linkname}" - ) - raise UnsafeTarMemberError(member=member.name, reason=reason) - member = copy.copy(member) - member.type = tarfile.REGTYPE - member.linkname = "" - member.size = payload[1] - rel_path = safe_tar_member_rel_path( - member, - allow_symlinks=True, - ) - if rel_path is None: - stripped_name = "." - elif rel_path == prefix_rel: - stripped_name = "." - elif rel_path.parts[: len(prefix_rel.parts)] == prefix_rel.parts: - stripped_name = Path(*rel_path.parts[len(prefix_rel.parts) :]).as_posix() - else: - reason = f"member does not start with prefix: {prefix_rel.as_posix()}" - raise UnsafeTarMemberError( - member=member.name, - reason=reason, - ) - - rewritten = copy.copy(member) - rewritten.name = stripped_name - rewritten.pax_headers = dict(member.pax_headers) - rewritten.pax_headers.pop("path", None) - if rewritten.issym() and symlink_root is not None: - rewritten.linkname = _relative_symlink_target( - rewritten.linkname, - link_name=stripped_name, - root=symlink_root, + with data: + with tarfile.open(fileobj=data, mode="r|*") as src: + with tarfile.open(fileobj=out, mode="w|") as dst: + for member in src: + if member.isfifo() or member.ischr() or member.isblk(): + continue + rel_path = safe_tar_member_rel_path( + member, + allow_symlinks=True, ) - # A long source target lives in a PAX "linkpath" record that would - # otherwise override the rewritten linkname; tobuf() re-derives it. - rewritten.pax_headers.pop("linkpath", None) - if not rewritten.isreg(): - dst.addfile(rewritten) - continue - if payload is not None: - fileobj: IO[bytes] = cast(IO[bytes], _ArchivePayloadReader(out, *payload)) - else: - extracted = src.extractfile(member) - if extracted is None: + if rel_path is None: + stripped_name = "." + elif rel_path == prefix_rel: + stripped_name = "." + elif rel_path.parts[: len(prefix_rel.parts)] == prefix_rel.parts: + stripped_name = Path( + *rel_path.parts[len(prefix_rel.parts) :] + ).as_posix() + else: + reason = f"member does not start with prefix: {prefix_rel.as_posix()}" raise UnsafeTarMemberError( member=member.name, - reason="missing file payload", + reason=reason, + ) + + rewritten = copy.copy(member) + rewritten.name = stripped_name + rewritten.pax_headers = dict(member.pax_headers) + rewritten.pax_headers.pop("path", None) + if rewritten.issym() and symlink_root is not None: + rewritten.linkname = rebase_symlink_target( + rewritten.linkname, link_name=stripped_name, root=symlink_root ) - fileobj = extracted - try: - dst.addfile(rewritten, fileobj) - finally: - fileobj.close() - padded = -(-rewritten.size // tarfile.BLOCKSIZE) * tarfile.BLOCKSIZE - written_payloads[member.name] = (dst.offset - padded, rewritten.size) + # A long source target lives in a PAX "linkpath" record that + # would otherwise override the rewritten linkname. + rewritten.pax_headers.pop("linkpath", None) + if member.isreg(): + fileobj = src.extractfile(member) + if fileobj is None: + raise UnsafeTarMemberError( + member=member.name, + reason="missing file payload", + ) + try: + dst.addfile(rewritten, fileobj) + finally: + fileobj.close() + else: + dst.addfile(rewritten) out.seek(0) with tarfile.open(fileobj=out, mode="r:*") as tar: @@ -218,57 +192,33 @@ def strip_tar_member_prefix( raise -class _ArchivePayloadReader(io.RawIOBase): - """Read a member payload back from the archive file that is still being written. +def rebase_symlink_target(linkname: str, *, link_name: str, root: str) -> str: + """Rewrite an absolute symlink target under `root` as a target relative to the link. - Every read seeks to the payload and then restores the writer's position, so the reader - can be interleaved with `TarFile.addfile()` writing to the same file object. + Only the root prefix is replaced by the climb out of the link's own archive directory; + the remaining components are kept verbatim, because the kernel resolves ``..`` after a + symlink component against that link's target: with ``alias -> sub/deep``, + ``/workspace/alias/../data.txt`` names ``sub/data.txt`` and must stay + ``alias/../data.txt``. Absolute targets outside the root are returned unchanged. A + leading ``//`` is collapsed to ``/`` (Linux treats them alike). """ - def __init__(self, archive: IO[bytes], start: int, size: int) -> None: - super().__init__() - self._archive = archive - self._position = start - self._end = start + size - - def readable(self) -> bool: - return True - - def read(self, size: int = -1) -> bytes: - remaining = self._end - self._position - if size is None or size < 0 or size > remaining: - size = remaining - if size <= 0: - return b"" - write_position = self._archive.tell() - try: - self._archive.seek(self._position) - data = self._archive.read(size) - finally: - self._archive.seek(write_position) - self._position += len(data) - return data - - def close(self) -> None: - # The archive stays open for the writer; only this view closes. - io.RawIOBase.close(self) - - -def _relative_symlink_target(linkname: str, *, link_name: str, root: PurePosixPath) -> str: - """Make an absolute symlink target under `root` relative to the link's directory.""" - - target = PurePosixPath(linkname) - if not target.is_absolute(): + if not linkname.startswith("/"): return linkname - # normpath keeps exactly two leading slashes (POSIX leaves "//" implementation-defined); - # Linux resolves them as "/", so collapse them before the containment check. - normalized = PurePosixPath("/" + posixpath.normpath(linkname).lstrip("/")) - try: - target_rel = normalized.relative_to(root) - except ValueError: + target = "/" + linkname.lstrip("/") + prefix = "/" + root.strip("/") + if target == prefix: + rest = "" + elif target.startswith(prefix + "/"): + rest = target[len(prefix) + 1 :] + else: return linkname - link_dir = PurePosixPath(link_name).parent - return posixpath.relpath(target_rel.as_posix() or ".", start=link_dir.as_posix()) + # Members beneath a symlink are rejected by the archive validator, so the link's + # archive directory holds no symlink components and climbing it is exact. + climb = "/".join([".."] * len(PurePosixPath(link_name).parent.parts)) + if rest and climb: + return f"{climb}/{rest}" + return rest or climb or "." def _normalize_rel(prefix: str | Path) -> Path: diff --git a/tests/sandbox/test_docker.py b/tests/sandbox/test_docker.py index e4c7cc812f..a0626fd024 100644 --- a/tests/sandbox/test_docker.py +++ b/tests/sandbox/test_docker.py @@ -4,6 +4,7 @@ import builtins import errno import io +import os import queue import shutil import socket @@ -501,9 +502,10 @@ async def _exec_internal( src = self._host_path(cmd[3]) dst = self._host_path(cmd[4]) if src.is_dir(): - shutil.copytree(src, dst) + # Like `cp -R`, keep symlinks as symlinks instead of following them. + shutil.copytree(src, dst, symlinks=True) else: - shutil.copy2(src, dst) + shutil.copy2(src, dst, follow_symlinks=False) return ExecResult(stdout=b"", stderr=b"", exit_code=0) if cmd[:2] == ["cat", "--"]: src = self._host_path(cmd[2]) @@ -751,6 +753,54 @@ async def test_docker_persist_workspace_stages_copy_before_get_archive( assert not any(name == "workspace" or name.startswith("workspace/") for name in names) +@pytest.mark.asyncio +async def test_docker_persist_and_hydrate_keep_absolute_workspace_symlinks_resolving( + tmp_path: Path, +) -> None: + """Persist stages the workspace with `cp -R`, the daemon archives the copy, and the + archive is normalized for the strict hydrate extractor. An absolute in-workspace + symlink must come back relative *with its components intact*: `alias -> sub/deep` + makes `/workspace/alias/../data.txt` name `sub/data.txt`, not `data.txt`.""" + host_root = tmp_path / "container" + workspace = host_root / "workspace" + (workspace / "sub" / "deep").mkdir(parents=True) + (workspace / "data.txt").write_text("wrong", encoding="utf-8") + (workspace / "sub" / "data.txt").write_text("right", encoding="utf-8") + (workspace / "alias").symlink_to("sub/deep") + (workspace / "abs_alias").symlink_to("/workspace/alias/../data.txt") + (workspace / "sub" / "abs_up").symlink_to("/workspace/sub/data.txt") + session = _HostBackedDockerSession(host_root=host_root, manifest=Manifest(root="/workspace")) + + archive = await session.persist_workspace() + + restored_host_root = tmp_path / "restored-container" + (restored_host_root / "workspace").mkdir(parents=True) + restored = _HostBackedDockerSession( + host_root=restored_host_root, manifest=Manifest(root="/workspace") + ) + + async def _extract_like_tar( + *, + cmd: list[str], + stream: io.IOBase, + error_path: Path, + user: object = None, + ) -> None: + _ = (error_path, user) + assert cmd[:3] == ["tar", "-x", "-C"] + with tarfile.open(fileobj=stream, mode="r|*") as tar: + tar.extractall(restored._host_path(cmd[3])) + + restored._stream_into_exec = _extract_like_tar # type: ignore[method-assign] + await restored.hydrate_workspace(archive) + + restored_workspace = restored_host_root / "workspace" + assert os.readlink(restored_workspace / "abs_alias") == "alias/../data.txt" + assert os.readlink(restored_workspace / "sub" / "abs_up") == "../sub/data.txt" + assert (restored_workspace / "abs_alias").read_text(encoding="utf-8") == "right" + assert (restored_workspace / "sub" / "abs_up").read_text(encoding="utf-8") == "right" + + @pytest.mark.asyncio async def test_docker_persist_workspace_closes_archive_http_response_after_normalization( tmp_path: Path, diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index f0e1152810..e925be31e0 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -181,50 +181,47 @@ def test_strip_tar_member_prefix_returns_workspace_relative_archive() -> None: def _prefixed_workspace_archive(*, external_symlink: bool) -> io.BytesIO: - """A `workspace/...` archive shaped like Docker's, with members hydrate refuses as-is.""" + """A `workspace/...` archive shaped like Docker's staged copy, with members that the + strict hydrate extractor refuses as-is.""" + + def add_dir(tar: tarfile.TarFile, name: str) -> None: + info = tarfile.TarInfo(name) + info.type = tarfile.DIRTYPE + tar.addfile(info) + + def add_file(tar: tarfile.TarFile, name: str, payload: bytes) -> None: + info = tarfile.TarInfo(name) + info.size = len(payload) + tar.addfile(info, io.BytesIO(payload)) + + def add_symlink(tar: tarfile.TarFile, name: str, target: str) -> None: + info = tarfile.TarInfo(name) + info.type = tarfile.SYMTYPE + info.linkname = target + tar.addfile(info) buf = io.BytesIO() with tarfile.open(fileobj=buf, mode="w") as tar: - root = tarfile.TarInfo("workspace") - root.type = tarfile.DIRTYPE - tar.addfile(root) - sub = tarfile.TarInfo("workspace/sub") - sub.type = tarfile.DIRTYPE - tar.addfile(sub) - payload = b"shared" - regular = tarfile.TarInfo("workspace/a.txt") - regular.size = len(payload) - tar.addfile(regular, io.BytesIO(payload)) - hardlink = tarfile.TarInfo("workspace/sub/hardlink.txt") - hardlink.type = tarfile.LNKTYPE - hardlink.linkname = "workspace/a.txt" - tar.addfile(hardlink) + add_dir(tar, "workspace") + add_dir(tar, "workspace/sub") + add_dir(tar, "workspace/sub/deep") + add_file(tar, "workspace/a.txt", b"shared") + add_file(tar, "workspace/data.txt", b"wrong") + add_file(tar, "workspace/sub/data.txt", b"right") fifo = tarfile.TarInfo("workspace/dev.fifo") fifo.type = tarfile.FIFOTYPE tar.addfile(fifo) - abs_inside = tarfile.TarInfo("workspace/sub/abs_up") - abs_inside.type = tarfile.SYMTYPE - abs_inside.linkname = "/workspace/a.txt" - tar.addfile(abs_inside) - rel = tarfile.TarInfo("workspace/rel") - rel.type = tarfile.SYMTYPE - rel.linkname = "a.txt" - tar.addfile(rel) - double_slash = tarfile.TarInfo("workspace/double_slash") - double_slash.type = tarfile.SYMTYPE - double_slash.linkname = "//workspace/a.txt" - tar.addfile(double_slash) + add_symlink(tar, "workspace/sub/abs_up", "/workspace/a.txt") + add_symlink(tar, "workspace/rel", "a.txt") + add_symlink(tar, "workspace/double_slash", "//workspace/a.txt") + # `alias/..` resolves against the alias target (sub/deep), so this names sub/data.txt. + add_symlink(tar, "workspace/alias", "sub/deep") + add_symlink(tar, "workspace/abs_alias", "/workspace/alias/../data.txt") # Longer than the 100-byte ustar field, so tarfile records it in a PAX linkpath. long_target = "/workspace/" + "/".join(["deeply-nested-directory"] * 5) + "/target.txt" - long_link = tarfile.TarInfo("workspace/long_link") - long_link.type = tarfile.SYMTYPE - long_link.linkname = long_target - tar.addfile(long_link) + add_symlink(tar, "workspace/long_link", long_target) if external_symlink: - outside = tarfile.TarInfo("workspace/outside") - outside.type = tarfile.SYMTYPE - outside.linkname = "/usr/bin/python3" - tar.addfile(outside) + add_symlink(tar, "workspace/outside", "/usr/bin/python3") buf.seek(0) return buf @@ -239,14 +236,12 @@ def test_strip_tar_member_prefix_rewrites_members_hydrate_refuses() -> None: with tarfile.open(fileobj=stripped, mode="r:*") as tar: members = {member.name: member for member in tar.getmembers()} assert "dev.fifo" not in members - hardlink = members["sub/hardlink.txt"] - assert hardlink.isreg() and hardlink.size == len("shared") - extracted = tar.extractfile(hardlink) - assert extracted is not None and extracted.read() == b"shared" assert members["sub/abs_up"].issym() assert members["sub/abs_up"].linkname == "../a.txt" assert members["rel"].linkname == "a.txt" assert members["double_slash"].linkname == "a.txt" + # Components after the root prefix are kept verbatim; `..` is not collapsed. + assert members["abs_alias"].linkname == "alias/../data.txt" long_link = members["long_link"] assert long_link.linkname == "/".join(["deeply-nested-directory"] * 5) + "/target.txt" assert "linkpath" not in long_link.pax_headers or ( @@ -269,8 +264,8 @@ def test_strip_tar_member_prefix_output_passes_strict_hydrate_validation( validate_tarfile(tar, allow_external_symlink_targets=False) safe_extract_tarfile(tar, root=tmp_path, allow_external_symlink_targets=False) - assert (tmp_path / "sub" / "hardlink.txt").read_bytes() == b"shared" assert (tmp_path / "sub" / "abs_up").read_bytes() == b"shared" + assert (tmp_path / "abs_alias").read_bytes() == b"right" assert not (tmp_path / "dev.fifo").exists() @@ -283,6 +278,17 @@ def test_strip_tar_member_prefix_keeps_absolute_symlinks_without_a_root() -> Non assert tar.getmember("sub/abs_up").linkname == "/workspace/a.txt" +def test_strip_tar_member_prefix_still_rejects_hardlink_members() -> None: + raw = _tar_bytes( + _dir("workspace"), + _file("workspace/a.txt", b"x"), + _hardlink("workspace/b.txt", "workspace/a.txt"), + ) + + with pytest.raises(UnsafeTarMemberError, match="hardlink member not allowed"): + strip_tar_member_prefix(io.BytesIO(raw), prefix="workspace") + + def test_strip_tar_member_prefix_rewrites_pax_path_headers() -> None: long_name = "workspace/" + ("a" * 120) + ".txt" payload = b"payload" From 266eab83c199f3d60b3094b8e3476359a9977ef9 Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Sun, 6 Sep 2026 03:58:23 +0000 Subject: [PATCH 06/11] fix(sandbox): consume the whole separator run after the workspace root when rebasing symlinks Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFSMrLZZ3oq1dKmJFAofz6 --- src/agents/sandbox/util/tar_utils.py | 4 +++- tests/sandbox/test_tar_utils.py | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index ec734726ad..6239cc247c 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -210,7 +210,9 @@ def rebase_symlink_target(linkname: str, *, link_name: str, root: str) -> str: if target == prefix: rest = "" elif target.startswith(prefix + "/"): - rest = target[len(prefix) + 1 :] + # Consume the whole separator run at the boundary (`/workspace//a.txt`), keeping + # every later component, including `..`, untouched. + rest = target[len(prefix) :].lstrip("/") else: return linkname # Members beneath a symlink are rejected by the archive validator, so the link's diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index e925be31e0..36204013ab 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -214,6 +214,7 @@ def add_symlink(tar: tarfile.TarFile, name: str, target: str) -> None: add_symlink(tar, "workspace/sub/abs_up", "/workspace/a.txt") add_symlink(tar, "workspace/rel", "a.txt") add_symlink(tar, "workspace/double_slash", "//workspace/a.txt") + add_symlink(tar, "workspace/double_sep", "/workspace//a.txt") # `alias/..` resolves against the alias target (sub/deep), so this names sub/data.txt. add_symlink(tar, "workspace/alias", "sub/deep") add_symlink(tar, "workspace/abs_alias", "/workspace/alias/../data.txt") @@ -240,6 +241,7 @@ def test_strip_tar_member_prefix_rewrites_members_hydrate_refuses() -> None: assert members["sub/abs_up"].linkname == "../a.txt" assert members["rel"].linkname == "a.txt" assert members["double_slash"].linkname == "a.txt" + assert members["double_sep"].linkname == "a.txt" # Components after the root prefix are kept verbatim; `..` is not collapsed. assert members["abs_alias"].linkname == "alias/../data.txt" long_link = members["long_link"] From 5a7bee2c54360f025819acace2dcb9a6fcf803e6 Mon Sep 17 00:00:00 2001 From: root Date: Mon, 14 Sep 2026 03:08:59 +0000 Subject: [PATCH 07/11] fix(sandbox): prove rebased symlink targets stay under the root, test link metadata only The textual rebase keeps every component after the workspace root verbatim, so `a/link/../tmp` is only inside the workspace if `a/link` resolves there. With `a/link -> ..` it names `/tmp` after extraction while hydrate's lexical check accepts it, turning an absolute target the strict extractor would have refused into a relative one it lets through. strip_tar_member_prefix now resolves each rebased target through the archive's own symlink members, applying `..` to a link's target the way the kernel does, and rejects the archive when the walk leaves the root, passes through a link whose target stayed absolute, or exceeds the ELOOP budget. This is the same outcome as the hardlink case: the snapshot is refused at persist time with a named member instead of failing on restore. The regression tests no longer read through restored POSIX symlinks on the host, which raised EINVAL on the Windows jobs, and the Docker test extracts the archive member by member as GNU tar in the container does instead of relying on `extractall`, whose default filter rewrites `alias/../data.txt` to `data.txt` on Python 3.14. Assertions are on the archive and restored link metadata. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FRNVNFM6bqUNRQBnw5GBVC --- src/agents/sandbox/util/tar_utils.py | 75 +++++++++++++++++++++++++++- tests/sandbox/test_docker.py | 24 +++++++-- tests/sandbox/test_tar_utils.py | 74 +++++++++++++++++++++++++-- 3 files changed, 166 insertions(+), 7 deletions(-) diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index 6239cc247c..7ec7a19e19 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -117,6 +117,9 @@ def strip_tar_member_prefix( kinds and absolute links into the workspace. FIFOs and device nodes are dropped, and when `relativize_symlinks_under` names the workspace root, an absolute symlink target under it is rebased onto the link's own directory with its components kept verbatim. + A rebased target is only accepted when resolving it through the archive's own symlink + members provably stays under the root; otherwise the archive is rejected, because the + relative form would pass hydrate's lexical check while escaping on disk. """ prefix_rel = _normalize_rel(prefix) @@ -131,6 +134,7 @@ def strip_tar_member_prefix( ) out = tempfile.TemporaryFile() + rebased_symlinks: dict[str, str] = {} try: with data: with tarfile.open(fileobj=data, mode="r|*") as src: @@ -168,6 +172,8 @@ def strip_tar_member_prefix( # A long source target lives in a PAX "linkpath" record that # would otherwise override the rewritten linkname. rewritten.pax_headers.pop("linkpath", None) + if rewritten.linkname != member.linkname: + rebased_symlinks[stripped_name] = rewritten.linkname if member.isreg(): fileobj = src.extractfile(member) if fileobj is None: @@ -185,6 +191,7 @@ def strip_tar_member_prefix( out.seek(0) with tarfile.open(fileobj=out, mode="r:*") as tar: validate_tarfile(tar) + _validate_rebased_symlinks_contained(tar, rebased_symlinks) out.seek(0) return cast(io.IOBase, out) except Exception: @@ -192,6 +199,70 @@ def strip_tar_member_prefix( raise +# Symlink hops followed while proving that a rebased target stays under the root. Linux +# gives up after 40 (ELOOP); an archive that needs more is not worth restoring. +_MAX_SYMLINK_HOPS = 40 + + +def _resolve_through_archive_symlinks( + parts: tuple[str, ...], symlinks: dict[str, str] +) -> tuple[str, ...] | None: + """Resolve a root-relative path the way the kernel would after extraction. + + Every prefix that names a symlink member is replaced by that member's target, so + ``..`` is applied to the link's target rather than to the link's own directory. + Returns the resolved components, or ``None`` when the walk leaves the root, follows a + target that is not itself relative (external links are hydrate's decision, not a proof + of containment), or exceeds the hop budget. + """ + + pending = list(reversed(parts)) + resolved: list[str] = [] + hops = 0 + while pending: + part = pending.pop() + if part in ("", "."): + continue + if part == "..": + if not resolved: + return None + resolved.pop() + continue + target = symlinks.get("/".join([*resolved, part])) + if target is None: + resolved.append(part) + continue + hops += 1 + if hops > _MAX_SYMLINK_HOPS or target.startswith("/"): + return None + pending.extend(reversed(PurePosixPath(target).parts)) + return tuple(resolved) + + +def _validate_rebased_symlinks_contained( + tar: tarfile.TarFile, rebased_symlinks: dict[str, str] +) -> None: + """Reject rebased symlinks whose relative target does not provably stay under the root. + + ``rebase_symlink_target`` keeps the components after the root verbatim, so a target + such as ``a/link/../tmp`` is only inside the workspace if ``a/link`` resolves there. + With ``a/link -> ..`` it names ``/tmp`` after extraction, yet hydrate's lexical check + accepts it. Resolving through the archive's own symlink members settles the question + before the relative form is written out. + """ + + if not rebased_symlinks: + return + symlinks = {member.name: member.linkname for member in tar.getmembers() if member.issym()} + for link_name, target in rebased_symlinks.items(): + parts = (*PurePosixPath(link_name).parent.parts, *PurePosixPath(target).parts) + if _resolve_through_archive_symlinks(parts, symlinks) is None: + raise UnsafeTarMemberError( + member=link_name, + reason=f"rebased symlink target cannot be proven to stay under the root: {target}", + ) + + def rebase_symlink_target(linkname: str, *, link_name: str, root: str) -> str: """Rewrite an absolute symlink target under `root` as a target relative to the link. @@ -200,7 +271,9 @@ def rebase_symlink_target(linkname: str, *, link_name: str, root: str) -> str: symlink component against that link's target: with ``alias -> sub/deep``, ``/workspace/alias/../data.txt`` names ``sub/data.txt`` and must stay ``alias/../data.txt``. Absolute targets outside the root are returned unchanged. A - leading ``//`` is collapsed to ``/`` (Linux treats them alike). + leading ``//`` is collapsed to ``/`` (Linux treats them alike). The textual rewrite + does not prove containment on its own; `strip_tar_member_prefix` checks each rebased + target against the archive's symlink members afterwards. """ if not linkname.startswith("/"): diff --git a/tests/sandbox/test_docker.py b/tests/sandbox/test_docker.py index a0626fd024..1cd08d9d26 100644 --- a/tests/sandbox/test_docker.py +++ b/tests/sandbox/test_docker.py @@ -788,17 +788,35 @@ async def _extract_like_tar( ) -> None: _ = (error_path, user) assert cmd[:3] == ["tar", "-x", "-C"] + # The container runs GNU tar, which restores link targets verbatim. Spell that out + # instead of relying on `extractall`, whose default filter rewrites symlink targets + # on Python 3.14. + root = restored._host_path(cmd[3]) with tarfile.open(fileobj=stream, mode="r|*") as tar: - tar.extractall(restored._host_path(cmd[3])) + for member in tar: + dest = root / member.name + if member.isdir(): + dest.mkdir(parents=True, exist_ok=True) + elif member.issym(): + os.symlink(member.linkname, dest) + elif member.isreg(): + payload = tar.extractfile(member) + assert payload is not None + with payload: + dest.write_bytes(payload.read()) + else: + raise AssertionError(f"unexpected member type: {member.name}") restored._stream_into_exec = _extract_like_tar # type: ignore[method-assign] await restored.hydrate_workspace(archive) + # Assert on the restored link metadata: the targets are POSIX paths that only the + # sandbox's own filesystem resolves the way these assertions describe. restored_workspace = restored_host_root / "workspace" assert os.readlink(restored_workspace / "abs_alias") == "alias/../data.txt" assert os.readlink(restored_workspace / "sub" / "abs_up") == "../sub/data.txt" - assert (restored_workspace / "abs_alias").read_text(encoding="utf-8") == "right" - assert (restored_workspace / "sub" / "abs_up").read_text(encoding="utf-8") == "right" + assert os.readlink(restored_workspace / "alias") == "sub/deep" + assert (restored_workspace / "sub" / "data.txt").read_text(encoding="utf-8") == "right" @pytest.mark.asyncio diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index 36204013ab..79df7dce98 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -266,9 +266,77 @@ def test_strip_tar_member_prefix_output_passes_strict_hydrate_validation( validate_tarfile(tar, allow_external_symlink_targets=False) safe_extract_tarfile(tar, root=tmp_path, allow_external_symlink_targets=False) - assert (tmp_path / "sub" / "abs_up").read_bytes() == b"shared" - assert (tmp_path / "abs_alias").read_bytes() == b"right" - assert not (tmp_path / "dev.fifo").exists() + # Inspect the restored link metadata rather than reading through the links: the + # targets are POSIX paths that only a POSIX host resolves the way the sandbox does. + assert os.readlink(tmp_path / "sub" / "abs_up") == "../a.txt" + assert os.readlink(tmp_path / "abs_alias") == "alias/../data.txt" + assert (tmp_path / "sub" / "data.txt").read_bytes() == b"right" + assert not os.path.lexists(tmp_path / "dev.fifo") + + +def test_strip_tar_member_prefix_rejects_rebased_symlinks_that_escape_via_links() -> None: + """`a/link -> ..` resolves to the workspace root, so the rebased `a/link/../tmp` names + `/tmp` after extraction even though hydrate's lexical check would accept it.""" + raw = _tar_bytes( + _dir("workspace"), + _dir("workspace/a"), + _symlink("workspace/a/link", ".."), + _symlink("workspace/victim", "/workspace/a/link/../tmp"), + ) + + with pytest.raises(UnsafeTarMemberError, match="cannot be proven to stay under the root"): + strip_tar_member_prefix( + io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" + ) + + +def test_strip_tar_member_prefix_rejects_rebased_symlinks_through_external_links() -> None: + """A hop through a link that stays absolute (`outside -> /usr`) proves nothing about + where `outside/../x` ends up, so the rebased target is refused rather than guessed.""" + raw = _tar_bytes( + _dir("workspace"), + _symlink("workspace/outside", "/usr"), + _symlink("workspace/victim", "/workspace/outside/../x"), + ) + + with pytest.raises(UnsafeTarMemberError, match="cannot be proven to stay under the root"): + strip_tar_member_prefix( + io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" + ) + + +def test_strip_tar_member_prefix_rejects_rebased_symlinks_in_a_link_cycle() -> None: + raw = _tar_bytes( + _dir("workspace"), + _symlink("workspace/loop", "loop"), + _symlink("workspace/victim", "/workspace/loop/x"), + ) + + with pytest.raises(UnsafeTarMemberError, match="cannot be proven to stay under the root"): + strip_tar_member_prefix( + io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" + ) + + +def test_strip_tar_member_prefix_keeps_rebased_symlinks_that_resolve_inside() -> None: + """`..` after a link is applied to the link's target, so `b/../data.txt` with + `b -> a/link2` and `a/link2 -> ../sub` names `data.txt` at the root and is kept.""" + raw = _tar_bytes( + _dir("workspace"), + _dir("workspace/a"), + _dir("workspace/sub"), + _file("workspace/data.txt", b"root"), + _symlink("workspace/a/link2", "../sub"), + _symlink("workspace/b", "a/link2"), + _symlink("workspace/sub/victim", "/workspace/b/../data.txt"), + ) + + stripped = strip_tar_member_prefix( + io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" + ) + + with tarfile.open(fileobj=stripped, mode="r:*") as tar: + assert tar.getmember("sub/victim").linkname == "../b/../data.txt" def test_strip_tar_member_prefix_keeps_absolute_symlinks_without_a_root() -> None: From 37a5c4034dba69183b08f2c45936a71f0715fc39 Mon Sep 17 00:00:00 2001 From: root Date: Tue, 15 Sep 2026 16:25:14 +0000 Subject: [PATCH 08/11] fix(sandbox): only rebase symlinks whose every component the archive establishes hydrate_workspace() extracts into an existing root, so a component the archive does not create may already be a symlink in the destination: a persisted `victim -> /workspace/alias/../secret` rebased to `alias/../secret` resolves to `/tmp/secret` when the destination holds `alias -> /tmp/sub`. The containment walk now requires each intermediate component to be a directory member and the leaf to be any member of the archive itself; those are the paths the extractor protects with its own destination checks. Symlink members are still followed and applied to `..` as before; anything the archive does not establish, a dangling leaf included, fails the proof and the archive is rejected as with the other unprovable cases. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FRNVNFM6bqUNRQBnw5GBVC --- src/agents/sandbox/util/tar_utils.py | 44 +++++++++++++++---------- tests/sandbox/test_tar_utils.py | 48 ++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 16 deletions(-) diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index 7ec7a19e19..56c6115903 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -204,16 +204,22 @@ def strip_tar_member_prefix( _MAX_SYMLINK_HOPS = 40 -def _resolve_through_archive_symlinks( - parts: tuple[str, ...], symlinks: dict[str, str] +def _resolve_through_archive_members( + parts: tuple[str, ...], members: dict[str, tarfile.TarInfo] ) -> tuple[str, ...] | None: """Resolve a root-relative path the way the kernel would after extraction. Every prefix that names a symlink member is replaced by that member's target, so - ``..`` is applied to the link's target rather than to the link's own directory. - Returns the resolved components, or ``None`` when the walk leaves the root, follows a - target that is not itself relative (external links are hydrate's decision, not a proof - of containment), or exceeds the hop budget. + ``..`` is applied to the link's target rather than to the link's own directory. Every + other component must be established by the archive itself: an intermediate component + must be a directory member and the last one any member. Hydration extracts into an + existing root, so a component the archive does not create could already be a symlink + in the destination and send the restored link somewhere else; only archive-owned + components are protected by the extractor's own destination checks. Returns the + resolved components, or ``None`` when the walk leaves the root, follows a target that + is not itself relative (external links are hydrate's decision, not a proof of + containment), meets a component the archive does not establish, or exceeds the hop + budget. """ pending = list(reversed(parts)) @@ -228,14 +234,18 @@ def _resolve_through_archive_symlinks( return None resolved.pop() continue - target = symlinks.get("/".join([*resolved, part])) - if target is None: - resolved.append(part) + member = members.get("/".join([*resolved, part])) + if member is None: + return None + if member.issym(): + hops += 1 + if hops > _MAX_SYMLINK_HOPS or member.linkname.startswith("/"): + return None + pending.extend(reversed(PurePosixPath(member.linkname).parts)) continue - hops += 1 - if hops > _MAX_SYMLINK_HOPS or target.startswith("/"): + if pending and not member.isdir(): return None - pending.extend(reversed(PurePosixPath(target).parts)) + resolved.append(part) return tuple(resolved) @@ -247,16 +257,18 @@ def _validate_rebased_symlinks_contained( ``rebase_symlink_target`` keeps the components after the root verbatim, so a target such as ``a/link/../tmp`` is only inside the workspace if ``a/link`` resolves there. With ``a/link -> ..`` it names ``/tmp`` after extraction, yet hydrate's lexical check - accepts it. Resolving through the archive's own symlink members settles the question - before the relative form is written out. + accepts it. Resolving through the archive's own members settles the question before + the relative form is written out; components the archive does not create are not + trusted either, because hydration extracts into an existing root where such a + component may already be a symlink. """ if not rebased_symlinks: return - symlinks = {member.name: member.linkname for member in tar.getmembers() if member.issym()} + members = {member.name: member for member in tar.getmembers()} for link_name, target in rebased_symlinks.items(): parts = (*PurePosixPath(link_name).parent.parts, *PurePosixPath(target).parts) - if _resolve_through_archive_symlinks(parts, symlinks) is None: + if _resolve_through_archive_members(parts, members) is None: raise UnsafeTarMemberError( member=link_name, reason=f"rebased symlink target cannot be proven to stay under the root: {target}", diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index 79df7dce98..31da6b4e7b 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -219,6 +219,11 @@ def add_symlink(tar: tarfile.TarFile, name: str, target: str) -> None: add_symlink(tar, "workspace/alias", "sub/deep") add_symlink(tar, "workspace/abs_alias", "/workspace/alias/../data.txt") # Longer than the 100-byte ustar field, so tarfile records it in a PAX linkpath. + nested = "workspace" + for _ in range(5): + nested += "/deeply-nested-directory" + add_dir(tar, nested) + add_file(tar, nested + "/target.txt", b"deep") long_target = "/workspace/" + "/".join(["deeply-nested-directory"] * 5) + "/target.txt" add_symlink(tar, "workspace/long_link", long_target) if external_symlink: @@ -305,6 +310,49 @@ def test_strip_tar_member_prefix_rejects_rebased_symlinks_through_external_links ) +def test_strip_tar_member_prefix_rejects_rebased_symlinks_through_absent_components() -> None: + """Hydration extracts into an existing root. A component the archive does not create + (`alias` here) may already be a symlink in the destination, so `alias/../secret` proves + nothing; only archive-established components count.""" + raw = _tar_bytes( + _dir("workspace"), + _file("workspace/secret", b"s"), + _symlink("workspace/victim", "/workspace/alias/../secret"), + ) + + with pytest.raises(UnsafeTarMemberError, match="cannot be proven to stay under the root"): + strip_tar_member_prefix( + io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" + ) + + +def test_strip_tar_member_prefix_rejects_rebased_symlinks_to_absent_leaf() -> None: + """A dangling in-workspace target is left absolute: the leaf could be a pre-existing + destination symlink, and hydrate already refuses absolute targets as before.""" + raw = _tar_bytes( + _dir("workspace"), + _symlink("workspace/victim", "/workspace/missing.txt"), + ) + + with pytest.raises(UnsafeTarMemberError, match="cannot be proven to stay under the root"): + strip_tar_member_prefix( + io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" + ) + + +def test_strip_tar_member_prefix_rejects_rebased_symlinks_through_a_file_component() -> None: + raw = _tar_bytes( + _dir("workspace"), + _file("workspace/notes.txt", b"n"), + _symlink("workspace/victim", "/workspace/notes.txt/../secret"), + ) + + with pytest.raises(UnsafeTarMemberError, match="cannot be proven to stay under the root"): + strip_tar_member_prefix( + io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" + ) + + def test_strip_tar_member_prefix_rejects_rebased_symlinks_in_a_link_cycle() -> None: raw = _tar_bytes( _dir("workspace"), From 4e7a0c51b118aed1380d4479d88e2417e822aa99 Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Wed, 16 Sep 2026 06:52:34 +0000 Subject: [PATCH 09/11] fix(sandbox): rebase only simple archive-established symlink targets, drop the resolver Addresses the Codex P1 on 37a5c403 ("replace the symlink graph emulator with narrow rejection"). `_resolve_through_archive_members` and `_validate_rebased_symlinks_contained` are removed. `strip_tar_member_prefix` now works in two passes. The first strips the prefix and drops FIFOs and device nodes as before, keeping every symlink target verbatim. The second, which sees every member, rewrites an absolute target under the root only when it is *simple* and *established*: every component is a plain name (no `.` or `..`), every directory the relative form walks through (the link's own parents and the target's parents) is an explicit directory member of the archive, and the leaf is a regular file or directory member. Anything else (`/workspace/alias/../data.txt`, a leaf that is itself a symlink, a directory only implied by a file path, a missing leaf) is left absolute so the strict hydrate validation refuses it with its existing "absolute symlink target not allowed" error, instead of the rewrite modelling how another link resolves. `rebase_symlink_target` keeps its signature and returns the input unchanged for non-simple targets. Tests: the resolver cases become one parametrized `test_strip_tar_member_prefix_leaves_non_simple_targets_absolute` (nine shapes, each asserting the target stays absolute and strict validation refuses it); positive coverage in `..._rebases_simple_targets_established_by_the_archive`; the fixture's alias-relative link is asserted to stay absolute and to be refused by the strict check; the Docker round trip keeps only archive-established targets and a sibling test shows `hydrate_workspace` refusing the alias-relative one. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFSMrLZZ3oq1dKmJFAofz6 --- src/agents/sandbox/util/tar_utils.py | 232 ++++++++++++++------------- tests/sandbox/test_docker.py | 42 ++++- tests/sandbox/test_tar_utils.py | 208 ++++++++++++++---------- 3 files changed, 277 insertions(+), 205 deletions(-) diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index 56c6115903..d130523db8 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -117,9 +117,12 @@ def strip_tar_member_prefix( kinds and absolute links into the workspace. FIFOs and device nodes are dropped, and when `relativize_symlinks_under` names the workspace root, an absolute symlink target under it is rebased onto the link's own directory with its components kept verbatim. - A rebased target is only accepted when resolving it through the archive's own symlink - members provably stays under the root; otherwise the archive is rejected, because the - relative form would pass hydrate's lexical check while escaping on disk. + Only *simple* targets are rebased: every component is a plain name and every directory + the relative target walks through (the link's own parents and the target's parents) is + a directory member of the archive, with a regular-file or directory leaf. A target + whose meaning depends on how another symlink resolves (``alias/../x``), or that walks + through a directory the archive does not create, is left absolute so the strict + hydrate validation refuses it rather than this rewrite guessing where it lands. """ prefix_rel = _normalize_rel(prefix) @@ -133,12 +136,12 @@ def strip_tar_member_prefix( else relativize_symlinks_under ) - out = tempfile.TemporaryFile() - rebased_symlinks: dict[str, str] = {} + staged = tempfile.TemporaryFile() + candidates: dict[str, str] = {} try: with data: with tarfile.open(fileobj=data, mode="r|*") as src: - with tarfile.open(fileobj=out, mode="w|") as dst: + with tarfile.open(fileobj=staged, mode="w|") as dst: for member in src: if member.isfifo() or member.ischr() or member.isblk(): continue @@ -166,14 +169,11 @@ def strip_tar_member_prefix( rewritten.pax_headers = dict(member.pax_headers) rewritten.pax_headers.pop("path", None) if rewritten.issym() and symlink_root is not None: - rewritten.linkname = rebase_symlink_target( + rebased = rebase_symlink_target( rewritten.linkname, link_name=stripped_name, root=symlink_root ) - # A long source target lives in a PAX "linkpath" record that - # would otherwise override the rewritten linkname. - rewritten.pax_headers.pop("linkpath", None) - if rewritten.linkname != member.linkname: - rebased_symlinks[stripped_name] = rewritten.linkname + if rebased != rewritten.linkname: + candidates[stripped_name] = rebased if member.isreg(): fileobj = src.extractfile(member) if fileobj is None: @@ -188,126 +188,128 @@ def strip_tar_member_prefix( else: dst.addfile(rewritten) - out.seek(0) - with tarfile.open(fileobj=out, mode="r:*") as tar: - validate_tarfile(tar) - _validate_rebased_symlinks_contained(tar, rebased_symlinks) - out.seek(0) - return cast(io.IOBase, out) - except Exception: - out.close() - raise - - -# Symlink hops followed while proving that a rebased target stays under the root. Linux -# gives up after 40 (ELOOP); an archive that needs more is not worth restoring. -_MAX_SYMLINK_HOPS = 40 - - -def _resolve_through_archive_members( - parts: tuple[str, ...], members: dict[str, tarfile.TarInfo] -) -> tuple[str, ...] | None: - """Resolve a root-relative path the way the kernel would after extraction. - - Every prefix that names a symlink member is replaced by that member's target, so - ``..`` is applied to the link's target rather than to the link's own directory. Every - other component must be established by the archive itself: an intermediate component - must be a directory member and the last one any member. Hydration extracts into an - existing root, so a component the archive does not create could already be a symlink - in the destination and send the restored link somewhere else; only archive-owned - components are protected by the extractor's own destination checks. Returns the - resolved components, or ``None`` when the walk leaves the root, follows a target that - is not itself relative (external links are hydrate's decision, not a proof of - containment), meets a component the archive does not establish, or exceeds the hop - budget. - """ - - pending = list(reversed(parts)) - resolved: list[str] = [] - hops = 0 - while pending: - part = pending.pop() - if part in ("", "."): - continue - if part == "..": - if not resolved: - return None - resolved.pop() - continue - member = members.get("/".join([*resolved, part])) - if member is None: - return None - if member.issym(): - hops += 1 - if hops > _MAX_SYMLINK_HOPS or member.linkname.startswith("/"): - return None - pending.extend(reversed(PurePosixPath(member.linkname).parts)) - continue - if pending and not member.isdir(): - return None - resolved.append(part) - return tuple(resolved) - - -def _validate_rebased_symlinks_contained( - tar: tarfile.TarFile, rebased_symlinks: dict[str, str] -) -> None: - """Reject rebased symlinks whose relative target does not provably stay under the root. - - ``rebase_symlink_target`` keeps the components after the root verbatim, so a target - such as ``a/link/../tmp`` is only inside the workspace if ``a/link`` resolves there. - With ``a/link -> ..`` it names ``/tmp`` after extraction, yet hydrate's lexical check - accepts it. Resolving through the archive's own members settles the question before - the relative form is written out; components the archive does not create are not - trusted either, because hydration extracts into an existing root where such a - component may already be a symlink. - """ - - if not rebased_symlinks: - return - members = {member.name: member for member in tar.getmembers()} - for link_name, target in rebased_symlinks.items(): - parts = (*PurePosixPath(link_name).parent.parts, *PurePosixPath(target).parts) - if _resolve_through_archive_members(parts, members) is None: - raise UnsafeTarMemberError( - member=link_name, - reason=f"rebased symlink target cannot be proven to stay under the root: {target}", - ) - - -def rebase_symlink_target(linkname: str, *, link_name: str, root: str) -> str: - """Rewrite an absolute symlink target under `root` as a target relative to the link. - - Only the root prefix is replaced by the climb out of the link's own archive directory; - the remaining components are kept verbatim, because the kernel resolves ``..`` after a - symlink component against that link's target: with ``alias -> sub/deep``, - ``/workspace/alias/../data.txt`` names ``sub/data.txt`` and must stay - ``alias/../data.txt``. Absolute targets outside the root are returned unchanged. A - leading ``//`` is collapsed to ``/`` (Linux treats them alike). The textual rewrite - does not prove containment on its own; `strip_tar_member_prefix` checks each rebased - target against the archive's symlink members afterwards. + # Second pass: now that every member is known, rewrite only the simple targets + # whose every component the archive itself establishes; the rest stay absolute. + staged.seek(0) + out = tempfile.TemporaryFile() + try: + with tarfile.open(fileobj=staged, mode="r:*") as src: + members = {member.name: member for member in src.getmembers()} + with tarfile.open(fileobj=out, mode="w|") as dst: + for member in src.getmembers(): + rewritten = member + candidate = candidates.get(member.name) + if candidate is not None and symlink_root is not None: + parts = _simple_rebase_components(member.linkname, root=symlink_root) + if parts is not None and _archive_establishes( + parts, link_name=member.name, members=members + ): + rewritten = copy.copy(member) + rewritten.linkname = candidate + # A long source target lives in a PAX "linkpath" record + # that would otherwise override the rewritten linkname. + rewritten.pax_headers = dict(member.pax_headers) + rewritten.pax_headers.pop("linkpath", None) + if member.isreg(): + fileobj = src.extractfile(member) + if fileobj is None: + raise UnsafeTarMemberError( + member=member.name, reason="missing file payload" + ) + try: + dst.addfile(rewritten, fileobj) + finally: + fileobj.close() + else: + dst.addfile(rewritten) + out.seek(0) + with tarfile.open(fileobj=out, mode="r:*") as tar: + validate_tarfile(tar) + out.seek(0) + return cast(io.IOBase, out) + except Exception: + out.close() + raise + finally: + staged.close() + + +def _simple_rebase_components(linkname: str, *, root: str) -> tuple[str, ...] | None: + """Return the root-relative components of a *simple* absolute target under `root`. + + Simple means every component after the root is a plain name: no ``.``, ``..``, or + empty segment. A leading ``//`` is collapsed to ``/`` and the whole separator run at + the root boundary is consumed (``/workspace//a.txt``). Targets outside the root, and + targets whose meaning depends on how a symlink component resolves (``alias/../x``), + return ``None`` and are left absolute for the strict hydrate check to refuse. """ if not linkname.startswith("/"): - return linkname + return None target = "/" + linkname.lstrip("/") prefix = "/" + root.strip("/") if target == prefix: rest = "" elif target.startswith(prefix + "/"): - # Consume the whole separator run at the boundary (`/workspace//a.txt`), keeping - # every later component, including `..`, untouched. rest = target[len(prefix) :].lstrip("/") else: + return None + parts = tuple(part for part in rest.split("/") if part) + if any(part in (".", "..") for part in parts): + return None + return parts + + +def rebase_symlink_target(linkname: str, *, link_name: str, root: str) -> str: + """Rewrite a simple absolute symlink target under `root` as a target relative to the link. + + Only targets whose components are all plain names are rewritten (see + `_simple_rebase_components`): ``/workspace/sub/data.txt`` from ``sub/abs_up`` becomes + ``../sub/data.txt``. Anything that would need a symlink component resolved to know + where it lands (``/workspace/alias/../data.txt``) is returned unchanged, so the strict + hydrate validation refuses it as an absolute target instead of this function guessing. + Whether the components are established by the archive itself is checked by + `strip_tar_member_prefix`, which sees every member. + """ + + parts = _simple_rebase_components(linkname, root=root) + if parts is None: return linkname - # Members beneath a symlink are rejected by the archive validator, so the link's - # archive directory holds no symlink components and climbing it is exact. climb = "/".join([".."] * len(PurePosixPath(link_name).parent.parts)) + rest = "/".join(parts) if rest and climb: return f"{climb}/{rest}" return rest or climb or "." +def _archive_establishes( + parts: tuple[str, ...], *, link_name: str, members: dict[str, tarfile.TarInfo] +) -> bool: + """Whether every component a rebased target walks through is an ordinary archive path. + + Hydration extracts into an existing root, so a directory the archive does not create + could already be a symlink in the destination and redirect the restored link. Every + directory the relative target climbs out of (the link's own parents) and every + directory it descends into must therefore be a directory member of the archive, and + the leaf must be a regular file or directory member: a symlink leaf would make the + result depend on another link, which is exactly what this rewrite refuses to model. + """ + + if not parts: + return False + link_parents = PurePosixPath(link_name).parent.parts + for depth in range(1, len(link_parents) + 1): + parent = members.get("/".join(link_parents[:depth])) + if parent is None or not parent.isdir(): + return False + for depth in range(1, len(parts)): + intermediate = members.get("/".join(parts[:depth])) + if intermediate is None or not intermediate.isdir(): + return False + leaf = members.get("/".join(parts)) + return leaf is not None and (leaf.isreg() or leaf.isdir()) + + def _normalize_rel(prefix: str | Path) -> Path: rel = prefix if isinstance(prefix, Path) else Path(prefix) posix = rel.as_posix() diff --git a/tests/sandbox/test_docker.py b/tests/sandbox/test_docker.py index 1cd08d9d26..dbf5cf09fd 100644 --- a/tests/sandbox/test_docker.py +++ b/tests/sandbox/test_docker.py @@ -759,16 +759,18 @@ async def test_docker_persist_and_hydrate_keep_absolute_workspace_symlinks_resol ) -> None: """Persist stages the workspace with `cp -R`, the daemon archives the copy, and the archive is normalized for the strict hydrate extractor. An absolute in-workspace - symlink must come back relative *with its components intact*: `alias -> sub/deep` - makes `/workspace/alias/../data.txt` name `sub/data.txt`, not `data.txt`.""" + symlink whose target is a plain path the archive establishes comes back relative + (`/workspace/sub/data.txt` from `sub/abs_up` as `../sub/data.txt`); a target whose + destination depends on another link (`/workspace/alias/../data.txt`) is left absolute + and refused by strict hydration, see the sibling test.""" host_root = tmp_path / "container" workspace = host_root / "workspace" (workspace / "sub" / "deep").mkdir(parents=True) (workspace / "data.txt").write_text("wrong", encoding="utf-8") (workspace / "sub" / "data.txt").write_text("right", encoding="utf-8") (workspace / "alias").symlink_to("sub/deep") - (workspace / "abs_alias").symlink_to("/workspace/alias/../data.txt") (workspace / "sub" / "abs_up").symlink_to("/workspace/sub/data.txt") + (workspace / "to_deep").symlink_to("/workspace/sub/deep") session = _HostBackedDockerSession(host_root=host_root, manifest=Manifest(root="/workspace")) archive = await session.persist_workspace() @@ -813,12 +815,44 @@ async def _extract_like_tar( # Assert on the restored link metadata: the targets are POSIX paths that only the # sandbox's own filesystem resolves the way these assertions describe. restored_workspace = restored_host_root / "workspace" - assert os.readlink(restored_workspace / "abs_alias") == "alias/../data.txt" assert os.readlink(restored_workspace / "sub" / "abs_up") == "../sub/data.txt" + assert os.readlink(restored_workspace / "to_deep") == "sub/deep" assert os.readlink(restored_workspace / "alias") == "sub/deep" assert (restored_workspace / "sub" / "data.txt").read_text(encoding="utf-8") == "right" +@pytest.mark.asyncio +async def test_docker_persist_keeps_link_dependent_symlink_targets_absolute_for_hydrate( + tmp_path: Path, +) -> None: + """`alias -> sub/deep` makes `/workspace/alias/../data.txt` name `sub/data.txt` only + through another symlink. The archive keeps such a target absolute rather than + guessing, and the strict hydrate check refuses it.""" + host_root = tmp_path / "container" + workspace = host_root / "workspace" + (workspace / "sub" / "deep").mkdir(parents=True) + (workspace / "data.txt").write_text("wrong", encoding="utf-8") + (workspace / "sub" / "data.txt").write_text("right", encoding="utf-8") + (workspace / "alias").symlink_to("sub/deep") + (workspace / "abs_alias").symlink_to("/workspace/alias/../data.txt") + session = _HostBackedDockerSession(host_root=host_root, manifest=Manifest(root="/workspace")) + + archive = await session.persist_workspace() + + with tarfile.open(fileobj=archive, mode="r:*") as tar: + assert tar.getmember("abs_alias").linkname == "/workspace/alias/../data.txt" + assert tar.getmember("alias").linkname == "sub/deep" + archive.seek(0) + + restored_host_root = tmp_path / "restored-container" + (restored_host_root / "workspace").mkdir(parents=True) + restored = _HostBackedDockerSession( + host_root=restored_host_root, manifest=Manifest(root="/workspace") + ) + with pytest.raises(WorkspaceArchiveWriteError): + await restored.hydrate_workspace(archive) + + @pytest.mark.asyncio async def test_docker_persist_workspace_closes_archive_http_response_after_normalization( tmp_path: Path, diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index 31da6b4e7b..470401cbae 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -180,7 +180,9 @@ def test_strip_tar_member_prefix_returns_workspace_relative_archive() -> None: assert tar.getnames() == [".", "pkg", "pkg/main.py", "pkg/python"] -def _prefixed_workspace_archive(*, external_symlink: bool) -> io.BytesIO: +def _prefixed_workspace_archive( + *, external_symlink: bool, link_through_alias: bool = True +) -> io.BytesIO: """A `workspace/...` archive shaped like Docker's staged copy, with members that the strict hydrate extractor refuses as-is.""" @@ -215,9 +217,11 @@ def add_symlink(tar: tarfile.TarFile, name: str, target: str) -> None: add_symlink(tar, "workspace/rel", "a.txt") add_symlink(tar, "workspace/double_slash", "//workspace/a.txt") add_symlink(tar, "workspace/double_sep", "/workspace//a.txt") - # `alias/..` resolves against the alias target (sub/deep), so this names sub/data.txt. add_symlink(tar, "workspace/alias", "sub/deep") - add_symlink(tar, "workspace/abs_alias", "/workspace/alias/../data.txt") + if link_through_alias: + # `alias/..` resolves against the alias target (sub/deep): where this lands + # depends on another symlink, so the rewrite must leave it absolute. + add_symlink(tar, "workspace/abs_alias", "/workspace/alias/../data.txt") # Longer than the 100-byte ustar field, so tarfile records it in a PAX linkpath. nested = "workspace" for _ in range(5): @@ -248,7 +252,8 @@ def test_strip_tar_member_prefix_rewrites_members_hydrate_refuses() -> None: assert members["double_slash"].linkname == "a.txt" assert members["double_sep"].linkname == "a.txt" # Components after the root prefix are kept verbatim; `..` is not collapsed. - assert members["abs_alias"].linkname == "alias/../data.txt" + # Depends on how `alias` resolves: left absolute for strict hydration to refuse. + assert members["abs_alias"].linkname == "/workspace/alias/../data.txt" long_link = members["long_link"] assert long_link.linkname == "/".join(["deeply-nested-directory"] * 5) + "/target.txt" assert "linkpath" not in long_link.pax_headers or ( @@ -262,7 +267,7 @@ def test_strip_tar_member_prefix_output_passes_strict_hydrate_validation( tmp_path: Path, ) -> None: stripped = strip_tar_member_prefix( - _prefixed_workspace_archive(external_symlink=False), + _prefixed_workspace_archive(external_symlink=False, link_through_alias=False), prefix="workspace", relativize_symlinks_under=PurePosixPath("/workspace"), ) @@ -274,109 +279,140 @@ def test_strip_tar_member_prefix_output_passes_strict_hydrate_validation( # Inspect the restored link metadata rather than reading through the links: the # targets are POSIX paths that only a POSIX host resolves the way the sandbox does. assert os.readlink(tmp_path / "sub" / "abs_up") == "../a.txt" - assert os.readlink(tmp_path / "abs_alias") == "alias/../data.txt" + assert os.readlink(tmp_path / "alias") == "sub/deep" assert (tmp_path / "sub" / "data.txt").read_bytes() == b"right" assert not os.path.lexists(tmp_path / "dev.fifo") -def test_strip_tar_member_prefix_rejects_rebased_symlinks_that_escape_via_links() -> None: - """`a/link -> ..` resolves to the workspace root, so the rebased `a/link/../tmp` names - `/tmp` after extraction even though hydrate's lexical check would accept it.""" - raw = _tar_bytes( - _dir("workspace"), - _dir("workspace/a"), - _symlink("workspace/a/link", ".."), - _symlink("workspace/victim", "/workspace/a/link/../tmp"), - ) - - with pytest.raises(UnsafeTarMemberError, match="cannot be proven to stay under the root"): - strip_tar_member_prefix( - io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" - ) - - -def test_strip_tar_member_prefix_rejects_rebased_symlinks_through_external_links() -> None: - """A hop through a link that stays absolute (`outside -> /usr`) proves nothing about - where `outside/../x` ends up, so the rebased target is refused rather than guessed.""" - raw = _tar_bytes( - _dir("workspace"), - _symlink("workspace/outside", "/usr"), - _symlink("workspace/victim", "/workspace/outside/../x"), +def test_strip_tar_member_prefix_output_with_alias_link_is_refused_by_strict_hydrate() -> None: + """The archive keeps `/workspace/alias/../data.txt` absolute, and the strict hydrate + validation is what refuses it: this rewrite never guesses through another link.""" + stripped = strip_tar_member_prefix( + _prefixed_workspace_archive(external_symlink=False), + prefix="workspace", + relativize_symlinks_under=PurePosixPath("/workspace"), ) - with pytest.raises(UnsafeTarMemberError, match="cannot be proven to stay under the root"): - strip_tar_member_prefix( - io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" - ) - - -def test_strip_tar_member_prefix_rejects_rebased_symlinks_through_absent_components() -> None: - """Hydration extracts into an existing root. A component the archive does not create - (`alias` here) may already be a symlink in the destination, so `alias/../secret` proves - nothing; only archive-established components count.""" - raw = _tar_bytes( - _dir("workspace"), - _file("workspace/secret", b"s"), - _symlink("workspace/victim", "/workspace/alias/../secret"), - ) + with tarfile.open(fileobj=stripped, mode="r:*") as tar: + assert tar.getmember("abs_alias").linkname == "/workspace/alias/../data.txt" + with pytest.raises(UnsafeTarMemberError, match="absolute symlink target not allowed"): + validate_tarfile(tar, allow_external_symlink_targets=False) - with pytest.raises(UnsafeTarMemberError, match="cannot be proven to stay under the root"): - strip_tar_member_prefix( - io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" - ) +@pytest.mark.parametrize( + ("members", "victim", "target"), + [ + pytest.param( + (_dir("workspace/a"), _symlink("workspace/a/link", "..")), + "workspace/victim", + "/workspace/a/link/../tmp", + id="dot-dot after a link that climbs out", + ), + pytest.param( + (_symlink("workspace/outside", "/usr"),), + "workspace/victim", + "/workspace/outside/../x", + id="dot-dot after an external link", + ), + pytest.param( + (_file("workspace/secret", b"s"),), + "workspace/victim", + "/workspace/alias/../secret", + id="dot-dot through a component the archive does not create", + ), + pytest.param( + (), + "workspace/victim", + "/workspace/missing.txt", + id="leaf the archive does not create", + ), + pytest.param( + (_file("workspace/notes.txt", b"n"),), + "workspace/victim", + "/workspace/notes.txt/secret", + id="descends through a regular file", + ), + pytest.param( + (_symlink("workspace/loop", "loop"),), + "workspace/victim", + "/workspace/loop/x", + id="descends through a symlink member", + ), + pytest.param( + (_symlink("workspace/alias", "sub"), _dir("workspace/sub")), + "workspace/victim", + "/workspace/alias", + id="leaf is a symlink member", + ), + pytest.param( + (_file("workspace/implied/deep/data.txt", b"d"),), + "workspace/victim", + "/workspace/implied/deep/data.txt", + id="directories only implied by a file path", + ), + pytest.param( + ( + _dir("workspace/a"), + _dir("workspace/sub"), + _file("workspace/data.txt", b"root"), + _symlink("workspace/a/link2", "../sub"), + _symlink("workspace/b", "a/link2"), + ), + "workspace/sub/victim", + "/workspace/b/../data.txt", + id="would resolve inside, but only by interpreting two links", + ), + ], +) +def test_strip_tar_member_prefix_leaves_non_simple_targets_absolute( + members: tuple[bytes, ...], victim: str, target: str +) -> None: + """Targets whose destination depends on another symlink, or that walk through a path + the archive does not establish as an ordinary directory, are not rewritten. They stay + absolute so the strict hydrate validation refuses them instead of a rewrite guessing.""" + raw = _tar_bytes(_dir("workspace"), *members, _symlink(victim, target)) -def test_strip_tar_member_prefix_rejects_rebased_symlinks_to_absent_leaf() -> None: - """A dangling in-workspace target is left absolute: the leaf could be a pre-existing - destination symlink, and hydrate already refuses absolute targets as before.""" - raw = _tar_bytes( - _dir("workspace"), - _symlink("workspace/victim", "/workspace/missing.txt"), + stripped = strip_tar_member_prefix( + io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" ) - with pytest.raises(UnsafeTarMemberError, match="cannot be proven to stay under the root"): - strip_tar_member_prefix( - io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" - ) + with tarfile.open(fileobj=stripped, mode="r:*") as tar: + name = victim.removeprefix("workspace/") + assert tar.getmember(name).linkname == target + with pytest.raises(UnsafeTarMemberError, match="absolute symlink target not allowed"): + validate_tarfile(tar, allow_external_symlink_targets=False) -def test_strip_tar_member_prefix_rejects_rebased_symlinks_through_a_file_component() -> None: +def test_strip_tar_member_prefix_rebases_simple_targets_established_by_the_archive() -> None: + """Every directory the relative target walks through is an explicit directory member + and the leaf is a regular file, so the rewrite is exact.""" raw = _tar_bytes( _dir("workspace"), - _file("workspace/notes.txt", b"n"), - _symlink("workspace/victim", "/workspace/notes.txt/../secret"), + _dir("workspace/sub"), + _dir("workspace/sub/deep"), + _file("workspace/sub/deep/data.txt", b"d"), + _dir("workspace/other"), + _symlink("workspace/other/victim", "/workspace/sub/deep/data.txt"), + _symlink("workspace/to_dir", "/workspace/sub"), ) - with pytest.raises(UnsafeTarMemberError, match="cannot be proven to stay under the root"): - strip_tar_member_prefix( - io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" - ) - - -def test_strip_tar_member_prefix_rejects_rebased_symlinks_in_a_link_cycle() -> None: - raw = _tar_bytes( - _dir("workspace"), - _symlink("workspace/loop", "loop"), - _symlink("workspace/victim", "/workspace/loop/x"), + stripped = strip_tar_member_prefix( + io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" ) - with pytest.raises(UnsafeTarMemberError, match="cannot be proven to stay under the root"): - strip_tar_member_prefix( - io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" - ) + with tarfile.open(fileobj=stripped, mode="r:*") as tar: + assert tar.getmember("other/victim").linkname == "../sub/deep/data.txt" + assert tar.getmember("to_dir").linkname == "sub" + validate_tarfile(tar, allow_external_symlink_targets=False) -def test_strip_tar_member_prefix_keeps_rebased_symlinks_that_resolve_inside() -> None: - """`..` after a link is applied to the link's target, so `b/../data.txt` with - `b -> a/link2` and `a/link2 -> ../sub` names `data.txt` at the root and is kept.""" +def test_strip_tar_member_prefix_leaves_link_under_unestablished_parent_absolute() -> None: + """The link's own parent directory is only implied, so the `..` climb cannot be trusted + either: hydration could find a symlink there in the destination.""" raw = _tar_bytes( _dir("workspace"), - _dir("workspace/a"), - _dir("workspace/sub"), - _file("workspace/data.txt", b"root"), - _symlink("workspace/a/link2", "../sub"), - _symlink("workspace/b", "a/link2"), - _symlink("workspace/sub/victim", "/workspace/b/../data.txt"), + _file("workspace/data.txt", b"d"), + _symlink("workspace/implied/victim", "/workspace/data.txt"), ) stripped = strip_tar_member_prefix( @@ -384,7 +420,7 @@ def test_strip_tar_member_prefix_keeps_rebased_symlinks_that_resolve_inside() -> ) with tarfile.open(fileobj=stripped, mode="r:*") as tar: - assert tar.getmember("sub/victim").linkname == "../b/../data.txt" + assert tar.getmember("implied/victim").linkname == "/workspace/data.txt" def test_strip_tar_member_prefix_keeps_absolute_symlinks_without_a_root() -> None: From 0877a9423bb58c3f463d9d23f1c2fc238ad6a8fb Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Sun, 20 Sep 2026 08:01:13 +0000 Subject: [PATCH 10/11] fix(sandbox): rebase links to the workspace root, leave trailing-separator targets absolute Two restoration edge cases in the narrowed symlink rebase: - `link -> /workspace` (or `/workspace/`) has no components after the root, and `_archive_establishes` refused the empty tuple, so the target stayed absolute and hydrate rejected the archive. The root is established by hydration itself; only the climb out of the link's own parents has to be proven, so such links now become `.` / `../..`. - `link -> /workspace/a.txt/` lost its trailing separator when empty segments were filtered, turning a link that fails with ENOTDIR on the source into a working file link. A trailing separator after a component is no longer a simple target; it is left absolute for strict hydration to refuse. Co-Authored-By: Claude Fable 5.1 --- src/agents/sandbox/util/tar_utils.py | 16 +++++++++--- tests/sandbox/test_tar_utils.py | 37 ++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index d130523db8..a3e8db0548 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -238,8 +238,10 @@ def _simple_rebase_components(linkname: str, *, root: str) -> tuple[str, ...] | """Return the root-relative components of a *simple* absolute target under `root`. Simple means every component after the root is a plain name: no ``.``, ``..``, or - empty segment. A leading ``//`` is collapsed to ``/`` and the whole separator run at - the root boundary is consumed (``/workspace//a.txt``). Targets outside the root, and + empty segment, and no trailing separator. A leading ``//`` is collapsed to ``/`` and + the whole separator run at the root boundary is consumed (``/workspace//a.txt``). The + root itself (``/workspace`` or ``/workspace/``) is the empty tuple. Targets outside + the root, and targets whose meaning depends on how a symlink component resolves (``alias/../x``), return ``None`` and are left absolute for the strict hydrate check to refuse. """ @@ -254,6 +256,10 @@ def _simple_rebase_components(linkname: str, *, root: str) -> tuple[str, ...] | rest = target[len(prefix) :].lstrip("/") else: return None + if rest.endswith("/"): + # `/workspace/a.txt/` fails with ENOTDIR when `a.txt` is a file; dropping the + # separator would turn it into a working link, so it is not a simple target. + return None parts = tuple(part for part in rest.split("/") if part) if any(part in (".", "..") for part in parts): return None @@ -295,13 +301,15 @@ def _archive_establishes( result depend on another link, which is exactly what this rewrite refuses to model. """ - if not parts: - return False link_parents = PurePosixPath(link_name).parent.parts for depth in range(1, len(link_parents) + 1): parent = members.get("/".join(link_parents[:depth])) if parent is None or not parent.isdir(): return False + if not parts: + # The target is the workspace root, which hydration itself establishes; only the + # climb out of the link's own parents had to be proven. + return True for depth in range(1, len(parts)): intermediate = members.get("/".join(parts[:depth])) if intermediate is None or not intermediate.isdir(): diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index 470401cbae..61a7f7f72f 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -344,6 +344,18 @@ def test_strip_tar_member_prefix_output_with_alias_link_is_refused_by_strict_hyd "/workspace/alias", id="leaf is a symlink member", ), + pytest.param( + (_file("workspace/a.txt", b"a"),), + "workspace/victim", + "/workspace/a.txt/", + id="trailing separator after a regular file (ENOTDIR on the source)", + ), + pytest.param( + (_dir("workspace/sub"),), + "workspace/victim", + "/workspace/sub/", + id="trailing separator after a directory", + ), pytest.param( (_file("workspace/implied/deep/data.txt", b"d"),), "workspace/victim", @@ -406,6 +418,31 @@ def test_strip_tar_member_prefix_rebases_simple_targets_established_by_the_archi validate_tarfile(tar, allow_external_symlink_targets=False) +def test_strip_tar_member_prefix_rebases_links_to_the_workspace_root() -> None: + """The root needs no archive member to be established: hydration creates it. Only the + link's own parents have to be directory members for the climb to be exact.""" + raw = _tar_bytes( + _dir("workspace"), + _dir("workspace/sub"), + _dir("workspace/sub/deep"), + _symlink("workspace/top", "/workspace"), + _symlink("workspace/top_slash", "/workspace/"), + _symlink("workspace/sub/deep/up", "/workspace"), + _symlink("workspace/implied/up", "/workspace"), + ) + + stripped = strip_tar_member_prefix( + io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" + ) + + with tarfile.open(fileobj=stripped, mode="r:*") as tar: + assert tar.getmember("top").linkname == "." + assert tar.getmember("top_slash").linkname == "." + assert tar.getmember("sub/deep/up").linkname == "../.." + # `implied/` is not a directory member, so the climb out of it proves nothing. + assert tar.getmember("implied/up").linkname == "/workspace" + + def test_strip_tar_member_prefix_leaves_link_under_unestablished_parent_absolute() -> None: """The link's own parent directory is only implied, so the `..` climb cannot be trusted either: hydration could find a symlink there in the destination.""" From 2b3596d1ec36014203ad57568918fde99ec41d6e Mon Sep 17 00:00:00 2001 From: Justin Beckwith Date: Sun, 27 Sep 2026 12:10:21 -0700 Subject: [PATCH 11/11] fix(sandbox): stream Docker snapshots once and narrow link rebasing --- src/agents/sandbox/util/tar_utils.py | 91 ++++++++------------- tests/sandbox/test_docker.py | 16 ++-- tests/sandbox/test_tar_utils.py | 118 +++++++++++++++++++++------ 3 files changed, 131 insertions(+), 94 deletions(-) diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index a3e8db0548..c558262a70 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -119,10 +119,9 @@ def strip_tar_member_prefix( under it is rebased onto the link's own directory with its components kept verbatim. Only *simple* targets are rebased: every component is a plain name and every directory the relative target walks through (the link's own parents and the target's parents) is - a directory member of the archive, with a regular-file or directory leaf. A target - whose meaning depends on how another symlink resolves (``alias/../x``), or that walks - through a directory the archive does not create, is left absolute so the strict - hydrate validation refuses it rather than this rewrite guessing where it lands. + a directory member of the archive, with a regular-file leaf. Targets that depend on + another symlink (``alias/../x``), walk through directories absent from the archive, or + name directories stay absolute for strict hydration to refuse. """ prefix_rel = _normalize_rel(prefix) @@ -136,12 +135,13 @@ def strip_tar_member_prefix( else relativize_symlinks_under ) - staged = tempfile.TemporaryFile() - candidates: dict[str, str] = {} + out = tempfile.TemporaryFile() + members: dict[str, tarfile.TarInfo] = {} + candidates: list[tuple[tarfile.TarInfo, str]] = [] try: with data: with tarfile.open(fileobj=data, mode="r|*") as src: - with tarfile.open(fileobj=staged, mode="w|") as dst: + with tarfile.open(fileobj=out, mode="w|") as dst: for member in src: if member.isfifo() or member.ischr() or member.isblk(): continue @@ -168,12 +168,14 @@ def strip_tar_member_prefix( rewritten.name = stripped_name rewritten.pax_headers = dict(member.pax_headers) rewritten.pax_headers.pop("path", None) + members[stripped_name] = rewritten if rewritten.issym() and symlink_root is not None: rebased = rebase_symlink_target( rewritten.linkname, link_name=stripped_name, root=symlink_root ) if rebased != rewritten.linkname: - candidates[stripped_name] = rebased + candidates.append((rewritten, rebased)) + continue if member.isreg(): fileobj = src.extractfile(member) if fileobj is None: @@ -188,50 +190,27 @@ def strip_tar_member_prefix( else: dst.addfile(rewritten) - # Second pass: now that every member is known, rewrite only the simple targets - # whose every component the archive itself establishes; the rest stay absolute. - staged.seek(0) - out = tempfile.TemporaryFile() - try: - with tarfile.open(fileobj=staged, mode="r:*") as src: - members = {member.name: member for member in src.getmembers()} - with tarfile.open(fileobj=out, mode="w|") as dst: - for member in src.getmembers(): - rewritten = member - candidate = candidates.get(member.name) - if candidate is not None and symlink_root is not None: + # Only symlink headers need the complete member inventory. Keep file + # payloads in one output archive instead of copying them a second time. + for member, candidate in candidates: + if symlink_root is not None: parts = _simple_rebase_components(member.linkname, root=symlink_root) if parts is not None and _archive_establishes( parts, link_name=member.name, members=members ): - rewritten = copy.copy(member) - rewritten.linkname = candidate + member.linkname = candidate # A long source target lives in a PAX "linkpath" record # that would otherwise override the rewritten linkname. - rewritten.pax_headers = dict(member.pax_headers) - rewritten.pax_headers.pop("linkpath", None) - if member.isreg(): - fileobj = src.extractfile(member) - if fileobj is None: - raise UnsafeTarMemberError( - member=member.name, reason="missing file payload" - ) - try: - dst.addfile(rewritten, fileobj) - finally: - fileobj.close() - else: - dst.addfile(rewritten) - out.seek(0) - with tarfile.open(fileobj=out, mode="r:*") as tar: - validate_tarfile(tar) - out.seek(0) - return cast(io.IOBase, out) - except Exception: - out.close() - raise - finally: - staged.close() + cast(dict[str, str], member.pax_headers).pop("linkpath", None) + dst.addfile(member) + out.seek(0) + with tarfile.open(fileobj=out, mode="r:*") as tar: + validate_tarfile(tar) + out.seek(0) + return cast(io.IOBase, out) + except BaseException: + out.close() + raise def _simple_rebase_components(linkname: str, *, root: str) -> tuple[str, ...] | None: @@ -240,8 +219,7 @@ def _simple_rebase_components(linkname: str, *, root: str) -> tuple[str, ...] | Simple means every component after the root is a plain name: no ``.``, ``..``, or empty segment, and no trailing separator. A leading ``//`` is collapsed to ``/`` and the whole separator run at the root boundary is consumed (``/workspace//a.txt``). The - root itself (``/workspace`` or ``/workspace/``) is the empty tuple. Targets outside - the root, and + root itself (``/workspace`` or ``/workspace/``), targets outside the root, and targets whose meaning depends on how a symlink component resolves (``alias/../x``), return ``None`` and are left absolute for the strict hydrate check to refuse. """ @@ -256,7 +234,7 @@ def _simple_rebase_components(linkname: str, *, root: str) -> tuple[str, ...] | rest = target[len(prefix) :].lstrip("/") else: return None - if rest.endswith("/"): + if not rest or rest.endswith("/"): # `/workspace/a.txt/` fails with ENOTDIR when `a.txt` is a file; dropping the # separator would turn it into a working link, so it is not a simple target. return None @@ -283,9 +261,7 @@ def rebase_symlink_target(linkname: str, *, link_name: str, root: str) -> str: return linkname climb = "/".join([".."] * len(PurePosixPath(link_name).parent.parts)) rest = "/".join(parts) - if rest and climb: - return f"{climb}/{rest}" - return rest or climb or "." + return f"{climb}/{rest}" if climb else rest def _archive_establishes( @@ -297,8 +273,9 @@ def _archive_establishes( could already be a symlink in the destination and redirect the restored link. Every directory the relative target climbs out of (the link's own parents) and every directory it descends into must therefore be a directory member of the archive, and - the leaf must be a regular file or directory member: a symlink leaf would make the - result depend on another link, which is exactly what this rewrite refuses to model. + the leaf must be a regular file. Directory targets stay absolute: another relative + link can traverse a directory link followed by ``..``, so proving the rewritten link + alone is insufficient to preserve strict hydration's boundary. """ link_parents = PurePosixPath(link_name).parent.parts @@ -306,16 +283,12 @@ def _archive_establishes( parent = members.get("/".join(link_parents[:depth])) if parent is None or not parent.isdir(): return False - if not parts: - # The target is the workspace root, which hydration itself establishes; only the - # climb out of the link's own parents had to be proven. - return True for depth in range(1, len(parts)): intermediate = members.get("/".join(parts[:depth])) if intermediate is None or not intermediate.isdir(): return False leaf = members.get("/".join(parts)) - return leaf is not None and (leaf.isreg() or leaf.isdir()) + return leaf is not None and leaf.isreg() def _normalize_rel(prefix: str | Path) -> Path: diff --git a/tests/sandbox/test_docker.py b/tests/sandbox/test_docker.py index dbf5cf09fd..5c91520e52 100644 --- a/tests/sandbox/test_docker.py +++ b/tests/sandbox/test_docker.py @@ -770,7 +770,6 @@ async def test_docker_persist_and_hydrate_keep_absolute_workspace_symlinks_resol (workspace / "sub" / "data.txt").write_text("right", encoding="utf-8") (workspace / "alias").symlink_to("sub/deep") (workspace / "sub" / "abs_up").symlink_to("/workspace/sub/data.txt") - (workspace / "to_deep").symlink_to("/workspace/sub/deep") session = _HostBackedDockerSession(host_root=host_root, manifest=Manifest(root="/workspace")) archive = await session.persist_workspace() @@ -816,31 +815,32 @@ async def _extract_like_tar( # sandbox's own filesystem resolves the way these assertions describe. restored_workspace = restored_host_root / "workspace" assert os.readlink(restored_workspace / "sub" / "abs_up") == "../sub/data.txt" - assert os.readlink(restored_workspace / "to_deep") == "sub/deep" assert os.readlink(restored_workspace / "alias") == "sub/deep" assert (restored_workspace / "sub" / "data.txt").read_text(encoding="utf-8") == "right" @pytest.mark.asyncio -async def test_docker_persist_keeps_link_dependent_symlink_targets_absolute_for_hydrate( +@pytest.mark.parametrize( + "target", ["/workspace/alias/../data.txt", "/workspace/sub/deep", "/workspace"] +) +async def test_docker_persist_keeps_unsupported_symlink_targets_absolute_for_hydrate( tmp_path: Path, + target: str, ) -> None: - """`alias -> sub/deep` makes `/workspace/alias/../data.txt` name `sub/data.txt` only - through another symlink. The archive keeps such a target absolute rather than - guessing, and the strict hydrate check refuses it.""" + """Directory and link-dependent targets stay absolute for strict hydration to refuse.""" host_root = tmp_path / "container" workspace = host_root / "workspace" (workspace / "sub" / "deep").mkdir(parents=True) (workspace / "data.txt").write_text("wrong", encoding="utf-8") (workspace / "sub" / "data.txt").write_text("right", encoding="utf-8") (workspace / "alias").symlink_to("sub/deep") - (workspace / "abs_alias").symlink_to("/workspace/alias/../data.txt") + (workspace / "abs_alias").symlink_to(target) session = _HostBackedDockerSession(host_root=host_root, manifest=Manifest(root="/workspace")) archive = await session.persist_workspace() with tarfile.open(fileobj=archive, mode="r:*") as tar: - assert tar.getmember("abs_alias").linkname == "/workspace/alias/../data.txt" + assert tar.getmember("abs_alias").linkname == target assert tar.getmember("alias").linkname == "sub/deep" archive.seek(0) diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index 61a7f7f72f..b3cdd5c10e 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -1,5 +1,6 @@ from __future__ import annotations +import errno import io import os import stat @@ -9,7 +10,9 @@ from pathlib import Path, PurePosixPath import pytest +from typing_extensions import Buffer +from agents.sandbox.util import tar_utils from agents.sandbox.util.tar_utils import ( UnsafeTarMemberError, safe_extract_tarfile, @@ -180,6 +183,86 @@ def test_strip_tar_member_prefix_returns_workspace_relative_archive() -> None: assert tar.getnames() == [".", "pkg", "pkg/main.py", "pkg/python"] +@pytest.mark.parametrize("absolute_link", [False, True]) +def test_strip_tar_member_prefix_retains_only_one_archive_payload( + monkeypatch: pytest.MonkeyPatch, absolute_link: bool +) -> None: + payload = b"workspace content\n" * 65536 + entries = [_dir("workspace")] + if absolute_link: + # The target follows the link in the input stream. + entries.append(_symlink("workspace/link", "/workspace/data.txt")) + entries.append(_file("workspace/data.txt", payload)) + raw = _tar_bytes(*entries) + storage: list[io.BytesIO] = [] + + class BudgetedArchive(io.BytesIO): + def write(self, data: Buffer) -> int: + written = super().write(data) + retained = sum(len(stream.getbuffer()) for stream in storage if not stream.closed) + if retained > len(raw) + tarfile.RECORDSIZE: + raise OSError(errno.ENOSPC, "archive storage budget exceeded") + return written + + def temporary_file() -> io.BytesIO: + stream = BudgetedArchive() + storage.append(stream) + return stream + + monkeypatch.setattr(tar_utils.tempfile, "TemporaryFile", temporary_file) + source = io.BytesIO(raw) + with strip_tar_member_prefix( + source, prefix="workspace", relativize_symlinks_under="/workspace" + ) as normalized: + assert source.closed + with tarfile.open(fileobj=normalized, mode="r:*") as archive: + validate_tarfile(archive, allow_external_symlink_targets=False) + restored = archive.extractfile("data.txt") + assert restored is not None + with restored: + assert restored.read() == payload + if absolute_link: + assert archive.getmember("link").linkname == "data.txt" + assert all(stream.closed for stream in storage) + + +@pytest.mark.parametrize("failure", ["read", "write", "validation"]) +def test_strip_tar_member_prefix_closes_streams_on_failure( + monkeypatch: pytest.MonkeyPatch, failure: str +) -> None: + entries = [_dir("workspace"), _file("workspace/data.txt")] + if failure == "validation": + entries.append(_file("workspace/data.txt", b"duplicate")) + + class Source(io.BytesIO): + def read(self, size: int | None = -1) -> bytes: + if failure == "read": + raise OSError("source read failed") + return super().read(size) + + class Output(io.BytesIO): + def write(self, data: Buffer) -> int: + if failure == "write": + raise OSError("archive write failed") + return super().write(data) + + source = Source(_tar_bytes(*entries)) + outputs: list[Output] = [] + + def temporary_file() -> Output: + output = Output() + outputs.append(output) + return output + + monkeypatch.setattr(tar_utils.tempfile, "TemporaryFile", temporary_file) + error = UnsafeTarMemberError if failure == "validation" else OSError + message = "duplicate archive path" if failure == "validation" else f"{failure} failed" + with pytest.raises(error, match=message): + strip_tar_member_prefix(source, prefix="workspace", relativize_symlinks_under="/workspace") + assert source.closed + assert outputs and all(output.closed for output in outputs) + + def _prefixed_workspace_archive( *, external_symlink: bool, link_through_alias: bool = True ) -> io.BytesIO: @@ -356,6 +439,14 @@ def test_strip_tar_member_prefix_output_with_alias_link_is_refused_by_strict_hyd "/workspace/sub/", id="trailing separator after a directory", ), + pytest.param( + (_dir("workspace/sub"),), + "workspace/victim", + "/workspace/sub", + id="directory target", + ), + pytest.param((), "workspace/victim", "/workspace", id="workspace root"), + pytest.param((), "workspace/victim", "/workspace/", id="workspace root with separator"), pytest.param( (_file("workspace/implied/deep/data.txt", b"d"),), "workspace/victim", @@ -405,7 +496,6 @@ def test_strip_tar_member_prefix_rebases_simple_targets_established_by_the_archi _file("workspace/sub/deep/data.txt", b"d"), _dir("workspace/other"), _symlink("workspace/other/victim", "/workspace/sub/deep/data.txt"), - _symlink("workspace/to_dir", "/workspace/sub"), ) stripped = strip_tar_member_prefix( @@ -414,35 +504,9 @@ def test_strip_tar_member_prefix_rebases_simple_targets_established_by_the_archi with tarfile.open(fileobj=stripped, mode="r:*") as tar: assert tar.getmember("other/victim").linkname == "../sub/deep/data.txt" - assert tar.getmember("to_dir").linkname == "sub" validate_tarfile(tar, allow_external_symlink_targets=False) -def test_strip_tar_member_prefix_rebases_links_to_the_workspace_root() -> None: - """The root needs no archive member to be established: hydration creates it. Only the - link's own parents have to be directory members for the climb to be exact.""" - raw = _tar_bytes( - _dir("workspace"), - _dir("workspace/sub"), - _dir("workspace/sub/deep"), - _symlink("workspace/top", "/workspace"), - _symlink("workspace/top_slash", "/workspace/"), - _symlink("workspace/sub/deep/up", "/workspace"), - _symlink("workspace/implied/up", "/workspace"), - ) - - stripped = strip_tar_member_prefix( - io.BytesIO(raw), prefix="workspace", relativize_symlinks_under="/workspace" - ) - - with tarfile.open(fileobj=stripped, mode="r:*") as tar: - assert tar.getmember("top").linkname == "." - assert tar.getmember("top_slash").linkname == "." - assert tar.getmember("sub/deep/up").linkname == "../.." - # `implied/` is not a directory member, so the climb out of it proves nothing. - assert tar.getmember("implied/up").linkname == "/workspace" - - def test_strip_tar_member_prefix_leaves_link_under_unestablished_parent_absolute() -> None: """The link's own parent directory is only implied, so the `..` climb cannot be trusted either: hydration could find a symlink there in the destination."""