From 329abf8168f6d00bce01d56df4ebb364d129688d Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Wed, 2 Sep 2026 16:53:21 +0000 Subject: [PATCH 1/6] 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 2/6] 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 3/6] 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 4/6] 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 5/6] 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 6/6] 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"]