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..6239cc247c 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -7,7 +7,7 @@ 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,17 +100,35 @@ 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 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: str | None = None + if relativize_symlinks_under is not None: + symlink_root = ( + relativize_symlinks_under.as_posix() + if isinstance(relativize_symlinks_under, PurePath) + else relativize_symlinks_under + ) out = tempfile.TemporaryFile() try: @@ -118,6 +136,8 @@ def strip_tar_member_prefix(data: io.IOBase, *, prefix: str | Path) -> io.IOBase 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, @@ -141,6 +161,13 @@ 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 rewritten.issym() and symlink_root is not None: + rewritten.linkname = 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 member.isreg(): fileobj = src.extractfile(member) if fileobj is None: @@ -165,6 +192,37 @@ def strip_tar_member_prefix(data: io.IOBase, *, prefix: str | Path) -> io.IOBase raise +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). + """ + + if not linkname.startswith("/"): + return linkname + 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 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)) + if rest and climb: + return f"{climb}/{rest}" + return rest or climb or "." + + 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 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 50402557c6..36204013ab 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,117 @@ 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 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: + 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) + 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") + # 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" + add_symlink(tar, "workspace/long_link", long_target) + if external_symlink: + add_symlink(tar, "workspace/outside", "/usr/bin/python3") + 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 + 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" + 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"] + 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" + + +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" / "abs_up").read_bytes() == b"shared" + assert (tmp_path / "abs_alias").read_bytes() == b"right" + 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_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"