From 607f6d19e9c823389e6e6816a9acd7b671f9d9a0 Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Wed, 2 Sep 2026 16:39:49 +0000 Subject: [PATCH 1/8] fix(sandbox): make UnixLocal persist_workspace archives restorable UnixLocalSandboxSession.persist_workspace() archived the workspace with tarfile.add() unchanged, while hydrate_workspace() extracts with the strict policy that refuses hardlink members, FIFOs/device nodes and absolute symlink targets. Ordinary workspaces hit all three: uv and pnpm hardlink installed packages, dev servers leave FIFOs behind, and `ln -s "$PWD/file" link` writes an absolute target. The snapshot was taken successfully and then could never be restored ("hardlink member not allowed", "unsupported member type", "absolute symlink target not allowed: /tmp/sandbox-local-.../file"). Rewrite members while archiving: store hardlinks as regular files, drop FIFOs and device nodes, and turn an absolute symlink target that stays under the workspace root into a relative one so it also survives the root moving between sessions. Absolute targets outside the workspace are left unchanged; hydrate keeps rejecting them by design. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01BN4v25msJgrjNgac97g1Az --- src/agents/sandbox/sandboxes/unix_local.py | 38 +++++++++++++++- tests/sandbox/test_unix_local.py | 53 ++++++++++++++++++++++ 2 files changed, 89 insertions(+), 2 deletions(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index ea7f83e9d8..e819ede410 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -25,7 +25,7 @@ from contextlib import suppress from dataclasses import dataclass, field from functools import partial -from pathlib import Path +from pathlib import Path, PurePosixPath from typing import Literal, cast from ...logger import log_tool_action_warning @@ -112,6 +112,40 @@ def _close_fd_quietly(fd: int) -> None: os.close(fd) +def _restorable_tar_member(ti: tarfile.TarInfo, *, root: Path) -> tarfile.TarInfo | None: + """Rewrite one ``persist_workspace`` member so ``hydrate_workspace`` can restore it. + + The strict extractor used for hydrate refuses hardlink members, special files, and + absolute symlink targets. A local workspace legitimately contains all three (``uv`` and + ``pnpm`` hardlink installed packages, dev servers leave FIFOs behind, ``ln -s "$PWD/x"`` + makes an absolute link), and archiving them as-is produced a snapshot that could never be + restored. Store hardlinks as regular files, drop FIFOs and device nodes, and make an + absolute symlink target that stays under the workspace root relative so it survives the + root moving between sessions. Absolute targets outside the workspace are kept unchanged. + """ + + if ti.isfifo() or ti.ischr() or ti.isblk(): + return None + if ti.islnk(): + # tarfile turns the second occurrence of an inode into a hardlink member with no + # payload; ``TarFile.add`` reads the file contents for a regular member instead. + ti.type = tarfile.REGTYPE + ti.linkname = "" + ti.size = os.stat(root / ti.name).st_size + return ti + if ti.issym() and PurePosixPath(ti.linkname).is_absolute(): + normalized_target = Path(os.path.normpath(ti.linkname)) + link_dir = PurePosixPath(ti.name).parent + for candidate_root in (root, root.resolve(strict=False)): + try: + target_rel = normalized_target.relative_to(candidate_root) + except ValueError: + continue + ti.linkname = os.path.relpath(target_rel.as_posix() or ".", start=link_dir.as_posix()) + break + return ti + + def _restore_pty_child_signal_defaults() -> None: for signum in _PTY_CHILD_SIGNAL_DEFAULTS: signal.signal(signum, signal.SIG_DFL) @@ -1097,7 +1131,7 @@ def _archive_workspace() -> None: skip_rel_paths=skip, root_name=None, ) - else ti + else _restorable_tar_member(ti, root=root) ), ) diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index fb13be3c51..e4b18effbd 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -2,6 +2,7 @@ import asyncio import io +import os import signal import tarfile import threading @@ -470,6 +471,58 @@ async def test_rm_as_user_checks_permissions_then_uses_local_fs( assert not any(part.startswith("rm ") for part in session.exec_commands[0]) +class TestUnixLocalPersistWorkspaceRestorable: + """persist_workspace must only emit members that the strict hydrate extractor accepts.""" + + @staticmethod + def _workspace(tmp_path: Path) -> Path: + workspace = tmp_path / "workspace" + (workspace / "sub").mkdir(parents=True) + (workspace / "a.txt").write_text("shared", encoding="utf-8") + os.link(workspace / "a.txt", workspace / "sub" / "hardlink.txt") + os.mkfifo(workspace / "dev.fifo") + (workspace / "abs_inside").symlink_to(workspace / "a.txt") + (workspace / "sub" / "abs_up").symlink_to(workspace / "a.txt") + (workspace / "rel").symlink_to("a.txt") + (workspace / "outside").symlink_to(tmp_path / "elsewhere.txt") + return workspace + + @pytest.mark.asyncio + async def test_persist_emits_restorable_members(self, tmp_path: Path) -> None: + workspace = self._workspace(tmp_path) + session = _RecordingUnixLocalSession(workspace) + + blob = await session.persist_workspace() + + with tarfile.open(fileobj=cast(io.BytesIO, blob), mode="r:*") as tar: + members = {member.name.removeprefix("./"): 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["abs_inside"].linkname == "a.txt" + assert members["sub/abs_up"].linkname == "../a.txt" + assert members["rel"].linkname == "a.txt" + assert members["outside"].linkname == str(tmp_path / "elsewhere.txt") + + @pytest.mark.asyncio + async def test_persisted_workspace_hydrates_into_a_new_root(self, tmp_path: Path) -> None: + workspace = self._workspace(tmp_path) + (workspace / "outside").unlink() # hydrate rejects external targets by design + blob = await _RecordingUnixLocalSession(workspace).persist_workspace() + + restored_root = tmp_path / "restored" + restored = _RecordingUnixLocalSession(restored_root) + await restored.hydrate_workspace(blob) + + assert (restored_root / "sub" / "hardlink.txt").read_text(encoding="utf-8") == "shared" + assert not (restored_root / "dev.fifo").exists() + assert os.readlink(restored_root / "abs_inside") == "a.txt" + assert (restored_root / "abs_inside").read_text(encoding="utf-8") == "shared" + assert (restored_root / "sub" / "abs_up").read_text(encoding="utf-8") == "shared" + + @pytest.mark.asyncio async def test_hydrate_workspace_cancellation_waits_for_the_extracting_worker( tmp_path: Path, From 570b70e2064986a99244b0abc1f835c884cfe6a1 Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Fri, 4 Sep 2026 16:12:56 +0000 Subject: [PATCH 2/8] fix(sandbox): relativize in-workspace symlink targets that start with a double slash os.path.normpath keeps two leading slashes, so ///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/sandboxes/unix_local.py | 3 ++- tests/sandbox/test_unix_local.py | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index e819ede410..5f8eecedee 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -134,7 +134,8 @@ def _restorable_tar_member(ti: tarfile.TarInfo, *, root: Path) -> tarfile.TarInf ti.size = os.stat(root / ti.name).st_size return ti if ti.issym() and PurePosixPath(ti.linkname).is_absolute(): - normalized_target = Path(os.path.normpath(ti.linkname)) + # normpath keeps a leading "//"; Linux resolves it as "/", so collapse it first. + normalized_target = Path("/" + os.path.normpath(ti.linkname).lstrip("/")) link_dir = PurePosixPath(ti.name).parent for candidate_root in (root, root.resolve(strict=False)): try: diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index e4b18effbd..eea0933f28 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -484,6 +484,7 @@ def _workspace(tmp_path: Path) -> Path: (workspace / "abs_inside").symlink_to(workspace / "a.txt") (workspace / "sub" / "abs_up").symlink_to(workspace / "a.txt") (workspace / "rel").symlink_to("a.txt") + (workspace / "double_slash").symlink_to("/" + str(workspace / "a.txt")) (workspace / "outside").symlink_to(tmp_path / "elsewhere.txt") return workspace @@ -504,6 +505,7 @@ async def test_persist_emits_restorable_members(self, tmp_path: Path) -> None: assert members["abs_inside"].linkname == "a.txt" assert members["sub/abs_up"].linkname == "../a.txt" assert members["rel"].linkname == "a.txt" + assert members["double_slash"].linkname == "a.txt" assert members["outside"].linkname == str(tmp_path / "elsewhere.txt") @pytest.mark.asyncio From 1e1806d47cabd6d5fbd9bd1669ebdc3a2e1781ab Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Sat, 5 Sep 2026 07:55:12 +0000 Subject: [PATCH 3/8] fix(sandbox): rebase absolute symlink targets without collapsing their components Replacing the workspace-root prefix of an absolute symlink target went through normpath(), which collapses `..` lexically. The kernel resolves `..` after a symlink component against the link target, so `/current/../config` with `current -> releases/v1` names `releases/config`, and the normalized `config` silently retargeted the restored link. Keep the target's components verbatim and only climb out of the link's own archive directory. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFSMrLZZ3oq1dKmJFAofz6 --- src/agents/sandbox/sandboxes/unix_local.py | 44 ++++++++++++++++------ tests/sandbox/test_unix_local.py | 29 ++++++++++++++ 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index 5f8eecedee..a44bddfce3 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -133,20 +133,42 @@ def _restorable_tar_member(ti: tarfile.TarInfo, *, root: Path) -> tarfile.TarInf ti.linkname = "" ti.size = os.stat(root / ti.name).st_size return ti - if ti.issym() and PurePosixPath(ti.linkname).is_absolute(): - # normpath keeps a leading "//"; Linux resolves it as "/", so collapse it first. - normalized_target = Path("/" + os.path.normpath(ti.linkname).lstrip("/")) - link_dir = PurePosixPath(ti.name).parent - for candidate_root in (root, root.resolve(strict=False)): - try: - target_rel = normalized_target.relative_to(candidate_root) - except ValueError: - continue - ti.linkname = os.path.relpath(target_rel.as_posix() or ".", start=link_dir.as_posix()) - break + if ti.issym() and ti.linkname.startswith("/"): + ti.linkname = _rebase_symlink_target( + ti.linkname, link_name=ti.name, roots=(root, root.resolve(strict=False)) + ) return ti +def _rebase_symlink_target(linkname: str, *, link_name: str, roots: tuple[Path, ...]) -> str: + """Rewrite an absolute symlink target under the workspace root as a link-relative one. + + Only the root prefix is replaced; the remaining components are kept verbatim (no + normalization), because ``..`` after a symlink component is resolved by the kernel + against the link target, so ``/current/../config`` with ``current -> releases/v1`` + names ``releases/config`` and must stay ``current/../config``. Absolute targets outside + the workspace are returned unchanged. A leading ``//`` is collapsed to ``/`` (Linux + treats them alike). + """ + + target = "/" + linkname.lstrip("/") + for candidate_root in roots: + prefix = candidate_root.as_posix().rstrip("/") + if target == prefix: + rest = "" + elif target.startswith(prefix + "/"): + rest = target[len(prefix) + 1 :] + else: + continue + # The link's own directory inside the archive holds no symlink components (the + # archive validator rejects members beneath a symlink), so 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 "." + return linkname + + def _restore_pty_child_signal_defaults() -> None: for signum in _PTY_CHILD_SIGNAL_DEFAULTS: signal.signal(signum, signal.SIG_DFL) diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index eea0933f28..ea731e0a0f 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -508,6 +508,35 @@ async def test_persist_emits_restorable_members(self, tmp_path: Path) -> None: assert members["double_slash"].linkname == "a.txt" assert members["outside"].linkname == str(tmp_path / "elsewhere.txt") + @pytest.mark.asyncio + async def test_rebased_symlink_keeps_parent_steps_after_symlink_components( + self, + tmp_path: Path, + ) -> None: + """`/current/../config` with `current -> releases/v1` names `releases/config`; + collapsing the `..` lexically would silently retarget the restored link.""" + workspace = tmp_path / "workspace" + (workspace / "releases" / "v1").mkdir(parents=True) + (workspace / "releases" / "config").write_text("right", encoding="utf-8") + (workspace / "config").write_text("wrong", encoding="utf-8") + (workspace / "current").symlink_to("releases/v1") + (workspace / "abs_config").symlink_to(workspace / "current" / ".." / "config") + (workspace / "releases" / "v1" / "abs_up").symlink_to( + workspace / "current" / ".." / "config" + ) + assert (workspace / "abs_config").read_text(encoding="utf-8") == "right" + + blob = await _RecordingUnixLocalSession(workspace).persist_workspace() + restored_root = tmp_path / "restored" + await _RecordingUnixLocalSession(restored_root).hydrate_workspace(blob) + + assert os.readlink(restored_root / "abs_config") == "current/../config" + assert ( + os.readlink(restored_root / "releases" / "v1" / "abs_up") == "../../current/../config" + ) + assert (restored_root / "abs_config").read_text(encoding="utf-8") == "right" + assert (restored_root / "releases" / "v1" / "abs_up").read_text(encoding="utf-8") == "right" + @pytest.mark.asyncio async def test_persisted_workspace_hydrates_into_a_new_root(self, tmp_path: Path) -> None: workspace = self._workspace(tmp_path) From 7133097a3671917f0121e233e0fdaeff145604c0 Mon Sep 17 00:00:00 2001 From: coderdailyone Date: Sun, 6 Sep 2026 03:57:36 +0000 Subject: [PATCH 4/8] 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/sandboxes/unix_local.py | 4 +++- tests/sandbox/test_unix_local.py | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index a44bddfce3..cb79d58184 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -157,7 +157,9 @@ def _rebase_symlink_target(linkname: str, *, link_name: str, roots: tuple[Path, if target == prefix: rest = "" elif target.startswith(prefix + "/"): - rest = target[len(prefix) + 1 :] + # Consume the whole separator run at the boundary (`//a.txt`), keeping + # every later component, including `..`, untouched. + rest = target[len(prefix) :].lstrip("/") else: continue # The link's own directory inside the archive holds no symlink components (the diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index ea731e0a0f..6741ff5191 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -485,6 +485,7 @@ def _workspace(tmp_path: Path) -> Path: (workspace / "sub" / "abs_up").symlink_to(workspace / "a.txt") (workspace / "rel").symlink_to("a.txt") (workspace / "double_slash").symlink_to("/" + str(workspace / "a.txt")) + (workspace / "double_sep").symlink_to(str(workspace) + "//a.txt") (workspace / "outside").symlink_to(tmp_path / "elsewhere.txt") return workspace @@ -506,6 +507,7 @@ async def test_persist_emits_restorable_members(self, tmp_path: Path) -> 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" assert members["outside"].linkname == str(tmp_path / "elsewhere.txt") @pytest.mark.asyncio From 06365a87e5a4f235602d1fc97ae1346d8d252465 Mon Sep 17 00:00:00 2001 From: root Date: Mon, 14 Sep 2026 03:12:19 +0000 Subject: [PATCH 5/8] fix(sandbox): only rebase absolute symlinks that provably stay under the workspace The rebase keeps the components after the root verbatim, so `a/link/../tmp` is only inside the workspace if `a/link` resolves there. With `a/link -> ..` it names `/tmp` once restored, while the strict extractor's lexical check accepts the relative form: an absolute target hydrate would have refused became one it lets through. Before rewriting, walk the target through the workspace's own relative links, applying `..` to a link's target the way the kernel does. Leaving the root, a hop through a link whose target is absolute (which proves nothing about the restored tree even when the live tree leads back inside), or exceeding the ELOOP budget keeps the target absolute, so hydrate refuses it exactly as before this change. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FRNVNFM6bqUNRQBnw5GBVC --- src/agents/sandbox/sandboxes/unix_local.py | 51 +++++++++++++++++++++- tests/sandbox/test_unix_local.py | 35 +++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index 753aeb66b1..6414dad328 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -141,12 +141,61 @@ def _restorable_tar_member(ti: tarfile.TarInfo, *, root: Path) -> tarfile.TarInf ti.size = os.stat(root / ti.name).st_size return ti if ti.issym() and ti.linkname.startswith("/"): - ti.linkname = _rebase_symlink_target( + rebased = _rebase_symlink_target( ti.linkname, link_name=ti.name, roots=(root, root.resolve(strict=False)) ) + if rebased != ti.linkname and _symlink_target_stays_under( + root, link_name=ti.name, target=rebased + ): + ti.linkname = rebased return ti +# Symlink hops followed while proving that a rebased target stays under the root. Linux +# gives up after 40 (ELOOP); a workspace that needs more is not worth restoring. +_MAX_SYMLINK_HOPS = 40 + + +def _symlink_target_stays_under(root: Path, *, link_name: str, target: str) -> bool: + """Whether a rebased, link-relative target provably resolves under the workspace root. + + The rebase keeps the components after the root verbatim, so ``a/link/../tmp`` is only + inside the workspace if ``a/link`` resolves inside it: with ``a/link -> ..`` it names + ``/tmp`` once restored, while the strict extractor's lexical check accepts the relative + form. The walk applies ``..`` to a link's target the way the kernel does and only + follows the workspace's own relative links, which restore verbatim; a hop through a + link whose target is absolute proves nothing about the restored tree (on the live tree + it may happen to lead back inside), so it fails the proof, as do leaving the root and + exceeding the hop budget. A target that cannot be proven contained keeps its absolute + form, which hydrate refuses as it always has. + """ + + pending = list( + reversed((*PurePosixPath(link_name).parent.parts, *PurePosixPath(target).parts)) + ) + resolved: list[str] = [] + hops = 0 + while pending: + part = pending.pop() + if part in ("", "."): + continue + if part == "..": + if not resolved: + return False + resolved.pop() + continue + candidate = root.joinpath(*resolved, part) + if not candidate.is_symlink(): + resolved.append(part) + continue + hops += 1 + link_target = os.readlink(candidate) + if hops > _MAX_SYMLINK_HOPS or link_target.startswith("/"): + return False + pending.extend(reversed(PurePosixPath(link_target).parts)) + return True + + def _rebase_symlink_target(linkname: str, *, link_name: str, roots: tuple[Path, ...]) -> str: """Rewrite an absolute symlink target under the workspace root as a link-relative one. diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index bbcb05c453..5c465378de 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -636,6 +636,41 @@ async def test_rebased_symlink_keeps_parent_steps_after_symlink_components( assert (restored_root / "abs_config").read_text(encoding="utf-8") == "right" assert (restored_root / "releases" / "v1" / "abs_up").read_text(encoding="utf-8") == "right" + @pytest.mark.asyncio + async def test_rebased_symlink_that_escapes_through_a_link_stays_absolute( + self, + tmp_path: Path, + ) -> None: + """`a/link -> ..` resolves to the workspace root, so `/a/link/../tmp` names + `/tmp`; the relative `a/link/../tmp` would pass hydrate's lexical check and escape, + so the target is left absolute for hydrate to refuse as before. A hop through an + absolute link (`outside`) or a loop proves nothing either, even when the live tree + happens to lead back inside.""" + workspace = tmp_path / "workspace" + (workspace / "a").mkdir(parents=True) + (workspace / "a" / "link").symlink_to("..") + (workspace / "victim").symlink_to(workspace / "a" / "link" / ".." / "tmp") + (workspace / "outside").symlink_to(tmp_path) + (workspace / "via_outside").symlink_to(workspace / "outside" / "workspace" / "a") + (workspace / "loop").symlink_to("loop") + (workspace / "via_loop").symlink_to(workspace / "loop" / ".." / ".." / "etc") + (workspace / "b").symlink_to("a/link") + (workspace / "a" / "fine").symlink_to(workspace / "b" / "a") + + blob = await _RecordingUnixLocalSession(workspace).persist_workspace() + + with tarfile.open(fileobj=cast(io.BytesIO, blob), mode="r:*") as tar: + members = {member.name.removeprefix("./"): member for member in tar.getmembers()} + assert members["victim"].linkname == str(workspace / "a" / "link" / ".." / "tmp") + assert members["via_outside"].linkname == str( + workspace / "outside" / "workspace" / "a" + ) + assert members["via_loop"].linkname == str( + workspace / "loop" / ".." / ".." / "etc" + ) + # `..` after `b -> a/link -> ..` lands on the root, so `b/a` is provably inside. + assert members["a/fine"].linkname == "../b/a" + @pytest.mark.asyncio async def test_persisted_workspace_hydrates_into_a_new_root(self, tmp_path: Path) -> None: workspace = self._workspace(tmp_path) From 53638a7651642bd59dfa19dda86ffd8343c0ded3 Mon Sep 17 00:00:00 2001 From: root Date: Mon, 14 Sep 2026 03:13:03 +0000 Subject: [PATCH 6/8] style(sandbox): ruff format the containment walk and its test Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FRNVNFM6bqUNRQBnw5GBVC --- src/agents/sandbox/sandboxes/unix_local.py | 4 +--- tests/sandbox/test_unix_local.py | 8 ++------ 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index 6414dad328..68a827c0f8 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -170,9 +170,7 @@ def _symlink_target_stays_under(root: Path, *, link_name: str, target: str) -> b form, which hydrate refuses as it always has. """ - pending = list( - reversed((*PurePosixPath(link_name).parent.parts, *PurePosixPath(target).parts)) - ) + pending = list(reversed((*PurePosixPath(link_name).parent.parts, *PurePosixPath(target).parts))) resolved: list[str] = [] hops = 0 while pending: diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index 5c465378de..ecaafc3678 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -662,12 +662,8 @@ async def test_rebased_symlink_that_escapes_through_a_link_stays_absolute( with tarfile.open(fileobj=cast(io.BytesIO, blob), mode="r:*") as tar: members = {member.name.removeprefix("./"): member for member in tar.getmembers()} assert members["victim"].linkname == str(workspace / "a" / "link" / ".." / "tmp") - assert members["via_outside"].linkname == str( - workspace / "outside" / "workspace" / "a" - ) - assert members["via_loop"].linkname == str( - workspace / "loop" / ".." / ".." / "etc" - ) + assert members["via_outside"].linkname == str(workspace / "outside" / "workspace" / "a") + assert members["via_loop"].linkname == str(workspace / "loop" / ".." / ".." / "etc") # `..` after `b -> a/link -> ..` lands on the root, so `b/a` is provably inside. assert members["a/fine"].linkname == "../b/a" From 04c7d51388f2a03b20ecb59ff74a3ac924351fc5 Mon Sep 17 00:00:00 2001 From: root Date: Tue, 15 Sep 2026 16:27:41 +0000 Subject: [PATCH 7/8] fix(sandbox): only rebase symlinks whose every component the snapshot establishes hydrate_workspace() extracts into an existing root, so a component the snapshot does not create may already be a symlink in the destination: a persisted `victim -> /alias/../secret` rebased to `alias/../secret` resolves elsewhere when the destination holds `alias -> /tmp/sub`. The containment walk now requires each component to be established by the snapshot itself: present in the workspace, not excluded by the persist skip list, a directory unless it is the leaf, and the leaf a regular file or directory. Those are the paths the extractor guards against pre-existing destination symlinks. Relative links are still followed and `..` applied to their targets; anything else keeps its absolute form for hydrate to refuse as before. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FRNVNFM6bqUNRQBnw5GBVC --- src/agents/sandbox/sandboxes/unix_local.py | 47 +++++++++++++++------- tests/sandbox/test_unix_local.py | 31 ++++++++++++++ 2 files changed, 64 insertions(+), 14 deletions(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index 68a827c0f8..2b455b8265 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -24,7 +24,7 @@ import time import uuid from collections import deque -from collections.abc import Collection, Mapping, Sequence +from collections.abc import Collection, Iterable, Mapping, Sequence from contextlib import suppress from dataclasses import dataclass, field from functools import partial @@ -119,7 +119,9 @@ def _close_fd_quietly(fd: int) -> None: os.close(fd) -def _restorable_tar_member(ti: tarfile.TarInfo, *, root: Path) -> tarfile.TarInfo | None: +def _restorable_tar_member( + ti: tarfile.TarInfo, *, root: Path, skip_rel_paths: Iterable[str | Path] = () +) -> tarfile.TarInfo | None: """Rewrite one ``persist_workspace`` member so ``hydrate_workspace`` can restore it. The strict extractor used for hydrate refuses hardlink members, special files, and @@ -145,7 +147,7 @@ def _restorable_tar_member(ti: tarfile.TarInfo, *, root: Path) -> tarfile.TarInf ti.linkname, link_name=ti.name, roots=(root, root.resolve(strict=False)) ) if rebased != ti.linkname and _symlink_target_stays_under( - root, link_name=ti.name, target=rebased + root, link_name=ti.name, target=rebased, skip_rel_paths=skip_rel_paths ): ti.linkname = rebased return ti @@ -156,7 +158,9 @@ def _restorable_tar_member(ti: tarfile.TarInfo, *, root: Path) -> tarfile.TarInf _MAX_SYMLINK_HOPS = 40 -def _symlink_target_stays_under(root: Path, *, link_name: str, target: str) -> bool: +def _symlink_target_stays_under( + root: Path, *, link_name: str, target: str, skip_rel_paths: Iterable[str | Path] = () +) -> bool: """Whether a rebased, link-relative target provably resolves under the workspace root. The rebase keeps the components after the root verbatim, so ``a/link/../tmp`` is only @@ -166,8 +170,15 @@ def _symlink_target_stays_under(root: Path, *, link_name: str, target: str) -> b follows the workspace's own relative links, which restore verbatim; a hop through a link whose target is absolute proves nothing about the restored tree (on the live tree it may happen to lead back inside), so it fails the proof, as do leaving the root and - exceeding the hop budget. A target that cannot be proven contained keeps its absolute - form, which hydrate refuses as it always has. + exceeding the hop budget. + + Every other component must be established by the snapshot itself: it has to exist in + the workspace, not be excluded by ``skip_rel_paths``, and be a directory unless it is + the last one, which must be a regular file or directory. ``hydrate_workspace`` extracts + into an existing root, so a component the snapshot does not create may already be a + symlink in the destination and send the restored link elsewhere; only snapshot-owned + components are protected by the extractor's destination checks. A target that cannot + be proven contained keeps its absolute form, which hydrate refuses as it always has. """ pending = list(reversed((*PurePosixPath(link_name).parent.parts, *PurePosixPath(target).parts))) @@ -182,15 +193,23 @@ def _symlink_target_stays_under(root: Path, *, link_name: str, target: str) -> b return False resolved.pop() continue - candidate = root.joinpath(*resolved, part) - if not candidate.is_symlink(): - resolved.append(part) + rel_name = "/".join([*resolved, part]) + if should_skip_tar_member(f"./{rel_name}", skip_rel_paths=skip_rel_paths, root_name=None): + return False + candidate = root / rel_name + if candidate.is_symlink(): + hops += 1 + link_target = os.readlink(candidate) + if hops > _MAX_SYMLINK_HOPS or link_target.startswith("/"): + return False + pending.extend(reversed(PurePosixPath(link_target).parts)) continue - hops += 1 - link_target = os.readlink(candidate) - if hops > _MAX_SYMLINK_HOPS or link_target.startswith("/"): + if pending: + if not candidate.is_dir(): + return False + elif not (candidate.is_dir() or candidate.is_file()): return False - pending.extend(reversed(PurePosixPath(link_target).parts)) + resolved.append(part) return True @@ -1254,7 +1273,7 @@ def _archive_workspace() -> None: skip_rel_paths=skip, root_name=None, ) - else _restorable_tar_member(ti, root=root) + else _restorable_tar_member(ti, root=root, skip_rel_paths=skip) ), ) diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index ecaafc3678..7caff3f3f5 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -667,6 +667,37 @@ async def test_rebased_symlink_that_escapes_through_a_link_stays_absolute( # `..` after `b -> a/link -> ..` lands on the root, so `b/a` is provably inside. assert members["a/fine"].linkname == "../b/a" + @pytest.mark.asyncio + async def test_rebased_symlink_through_components_the_snapshot_does_not_create_stays_absolute( + self, + tmp_path: Path, + ) -> None: + """Hydration extracts into an existing root, so a component the snapshot does not + create may already be a symlink there. Only components the snapshot establishes + (present, not skipped, directories on the way) count towards the proof.""" + workspace = tmp_path / "workspace" + (workspace / "skipped").mkdir(parents=True) + (workspace / "secret").write_text("s", encoding="utf-8") + (workspace / "notes.txt").write_text("n", encoding="utf-8") + (workspace / "via_missing").symlink_to(workspace / "alias" / ".." / "secret") + (workspace / "dangling").symlink_to(workspace / "missing.txt") + (workspace / "via_file").symlink_to(workspace / "notes.txt" / ".." / "secret") + (workspace / "via_skipped").symlink_to(workspace / "skipped" / ".." / "secret") + (workspace / "fine").symlink_to(workspace / "secret") + + session = _RecordingUnixLocalSession(workspace) + session._runtime_persist_workspace_skip_relpaths = {Path("skipped")} + blob = await session.persist_workspace() + + with tarfile.open(fileobj=cast(io.BytesIO, blob), mode="r:*") as tar: + members = {member.name.removeprefix("./"): member for member in tar.getmembers()} + assert "skipped" not in members + assert members["via_missing"].linkname == str(workspace / "alias" / ".." / "secret") + assert members["dangling"].linkname == str(workspace / "missing.txt") + assert members["via_file"].linkname == str(workspace / "notes.txt" / ".." / "secret") + assert members["via_skipped"].linkname == str(workspace / "skipped" / ".." / "secret") + assert members["fine"].linkname == "secret" + @pytest.mark.asyncio async def test_persisted_workspace_hydrates_into_a_new_root(self, tmp_path: Path) -> None: workspace = self._workspace(tmp_path) From 08b98fdf92a75f98104df11371421714f7de2f1f Mon Sep 17 00:00:00 2001 From: Justin Beckwith Date: Sun, 27 Sep 2026 13:29:06 -0700 Subject: [PATCH 8/8] fix(sandbox): prove symlink rebases against captured archive metadata --- src/agents/sandbox/sandboxes/unix_local.py | 77 +++++++++++----------- tests/sandbox/test_unix_local.py | 58 ++++++++++++++++ 2 files changed, 97 insertions(+), 38 deletions(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index a5a0775d23..6d304e2243 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -7,6 +7,7 @@ ) import asyncio +import copy import errno import fcntl import inspect @@ -24,7 +25,7 @@ import time import uuid from collections import deque -from collections.abc import Collection, Iterable, Mapping, Sequence +from collections.abc import Collection, Mapping, Sequence from contextlib import suppress from dataclasses import dataclass, field from functools import partial @@ -120,39 +121,13 @@ def _close_fd_quietly(fd: int) -> None: os.close(fd) -def _restorable_tar_member( - ti: tarfile.TarInfo, *, root: Path, skip_rel_paths: Iterable[str | Path] = () -) -> tarfile.TarInfo | None: - """Rewrite one ``persist_workspace`` member so ``hydrate_workspace`` can restore it. - - The strict extractor used for hydrate refuses special files and absolute symlink - targets. Local dev servers can leave FIFOs behind, and ``ln -s "$PWD/x"`` makes an - absolute link. Archiving these as-is produces a snapshot that cannot be restored. - Drop FIFOs and device nodes, and make an - absolute symlink target that stays under the workspace root relative so it survives the - root moving between sessions. Absolute targets outside the workspace are kept unchanged. - """ - - if ti.isfifo() or ti.ischr() or ti.isblk(): - return None - if ti.issym() and ti.linkname.startswith("/"): - rebased = _rebase_symlink_target( - ti.linkname, link_name=ti.name, roots=(root, root.resolve(strict=False)) - ) - if rebased != ti.linkname and _symlink_target_stays_under( - root, link_name=ti.name, target=rebased, skip_rel_paths=skip_rel_paths - ): - ti.linkname = rebased - return ti - - # Symlink hops followed while proving that a rebased target stays under the root. Linux # gives up after 40 (ELOOP); a workspace that needs more is not worth restoring. _MAX_SYMLINK_HOPS = 40 def _symlink_target_stays_under( - root: Path, *, link_name: str, target: str, skip_rel_paths: Iterable[str | Path] = () + members: Mapping[str, tarfile.TarInfo], *, link_name: str, target: str ) -> bool: """Whether a rebased, link-relative target provably resolves under the workspace root. @@ -160,14 +135,14 @@ def _symlink_target_stays_under( inside the workspace if ``a/link`` resolves inside it: with ``a/link -> ..`` it names ``/tmp`` once restored, while the strict extractor's lexical check accepts the relative form. The walk applies ``..`` to a link's target the way the kernel does and only - follows the workspace's own relative links, which restore verbatim; a hop through a + follows the archive's own relative links, which restore verbatim; a hop through a link whose target is absolute proves nothing about the restored tree (on the live tree it may happen to lead back inside), so it fails the proof, as do leaving the root and exceeding the hop budget. Every other component must be established by the snapshot itself: it has to exist in - the workspace, not be excluded by ``skip_rel_paths``, and be a directory unless it is - the last one, which must be a regular file or directory. ``hydrate_workspace`` extracts + the archive and be a directory unless it is the last one, which must be a regular file + or directory. ``hydrate_workspace`` extracts into an existing root, so a component the snapshot does not create may already be a symlink in the destination and send the restored link elsewhere; only snapshot-owned components are protected by the extractor's destination checks. A target that cannot @@ -187,20 +162,20 @@ def _symlink_target_stays_under( resolved.pop() continue rel_name = "/".join([*resolved, part]) - if should_skip_tar_member(f"./{rel_name}", skip_rel_paths=skip_rel_paths, root_name=None): + candidate = members.get(rel_name) + if candidate is None: return False - candidate = root / rel_name - if candidate.is_symlink(): + if candidate.issym(): hops += 1 - link_target = os.readlink(candidate) + link_target = candidate.linkname if hops > _MAX_SYMLINK_HOPS or link_target.startswith("/"): return False pending.extend(reversed(PurePosixPath(link_target).parts)) continue if pending: - if not candidate.is_dir(): + if not candidate.isdir(): return False - elif not (candidate.is_dir() or candidate.is_file()): + elif not (candidate.isdir() or candidate.isreg()): return False resolved.append(part) return True @@ -1265,6 +1240,8 @@ async def persist_workspace(self) -> io.IOBase: buf = io.BytesIO() def _archive_workspace() -> None: + roots = (root, root.resolve(strict=False)) + symlinks: list[tarfile.TarInfo] = [] with tarfile.open(fileobj=buf, mode="w") as tar: def filter_member(member: tarfile.TarInfo) -> tarfile.TarInfo | None: @@ -1274,9 +1251,33 @@ def filter_member(member: tarfile.TarInfo) -> tarfile.TarInfo | None: getattr(tar, "inodes").clear() # noqa: B009 - Not exposed by typeshed. if should_skip_tar_member(member.name, skip_rel_paths=skip, root_name=None): return None - return _restorable_tar_member(member, root=root, skip_rel_paths=skip) + if member.isfifo() or member.ischr() or member.isblk(): + return None + if member.issym(): + symlinks.append(member) + return None + return member tar.add(root, arcname=".", filter=filter_member) + # Defer symlink headers until capture is complete. The live tree can change + # during tar.add, so only the captured topology can prove containment. + members = { + PurePosixPath(member.name).as_posix(): member + for member in [*tar.getmembers(), *symlinks] + } + for member in symlinks: + # Keep the proof graph unchanged: a hop through an originally absolute + # target must stay unprovable regardless of symlink emission order. + archived_member = copy.copy(member) + if member.linkname.startswith("/"): + rebased = _rebase_symlink_target( + member.linkname, link_name=member.name, roots=roots + ) + if rebased != member.linkname and _symlink_target_stays_under( + members, link_name=member.name, target=rebased + ): + archived_member.linkname = rebased + tar.addfile(archived_member) try: await run_blocking_workspace_io(_archive_workspace) diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index 0729795c00..6723425db7 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -844,6 +844,64 @@ async def test_rebased_symlink_through_components_the_snapshot_does_not_create_s assert members["via_skipped"].linkname == str(workspace / "skipped" / ".." / "secret") assert members["fine"].linkname == "secret" + @pytest.mark.asyncio + @pytest.mark.parametrize("mutation_order", ["before_absolute_link", "after_absolute_link"]) + async def test_rebase_uses_archived_topology_when_workspace_changes( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, mutation_order: str + ) -> None: + workspace = tmp_path / "workspace" + workspace.mkdir() + (workspace / "m-trigger").write_text("capture boundary", encoding="utf-8") + if mutation_order == "before_absolute_link": + (workspace / "dir").mkdir() + (workspace / "outside").write_text("inside", encoding="utf-8") + changed_path = workspace / "a-hop" + changed_path.symlink_to(".") + absolute_link = workspace / "z-link" + original_target = str(workspace / "a-hop" / ".." / "outside") + replacement_target = "dir" + else: + (workspace / "q").mkdir() + (workspace / "q" / "hop").symlink_to("..") + changed_path = workspace / "z-target" + changed_path.write_text("inside", encoding="utf-8") + absolute_link = workspace / "a-link" + original_target = str(changed_path) + replacement_target = "q/hop/../outside" + absolute_link.symlink_to(original_target) + + original_addfile = tarfile.TarFile.addfile + mutated = False + + def addfile_with_workspace_mutation( + archive: tarfile.TarFile, + member: tarfile.TarInfo, + fileobj: io.BufferedReader | None = None, + ) -> None: + nonlocal mutated + original_addfile(archive, member, fileobj) + # Change the live tree at a deterministic boundary in archive capture. + if member.name == "./m-trigger" and not mutated: + changed_path.unlink() + changed_path.symlink_to(replacement_target) + mutated = True + + monkeypatch.setattr(tarfile.TarFile, "addfile", addfile_with_workspace_mutation) + blob = await _RecordingUnixLocalSession(workspace).persist_workspace() + assert mutated + with tarfile.open(fileobj=cast(io.BytesIO, blob), mode="r:*") as archive: + assert archive.getmember(f"./{absolute_link.name}").linkname == original_target + + restored_root = tmp_path / "restored" + restored_root.mkdir() + sentinel = restored_root / "keep.txt" + sentinel.write_text("unchanged", encoding="utf-8") + blob.seek(0) + with pytest.raises(WorkspaceArchiveWriteError): + await _RecordingUnixLocalSession(restored_root).hydrate_workspace(blob) + assert sentinel.read_text(encoding="utf-8") == "unchanged" + assert list(restored_root.iterdir()) == [sentinel] + @pytest.mark.asyncio async def test_persisted_workspace_hydrates_into_a_new_root(self, tmp_path: Path) -> None: workspace = self._workspace(tmp_path)