fix(sandbox): normalize UnixLocal snapshot special files and symlinks

src/agents/sandbox/sandboxes/unix_local.py

@@ -7,6 +7,7 @@
     )
 
 import asyncio
+import copy
 import errno
 import fcntl
 import inspect
@@ -28,7 +29,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
@@ -120,6 +121,97 @@ def _close_fd_quietly(fd: int) -> None:
         os.close(fd)
 
 
+# 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(
+    members: Mapping[str, tarfile.TarInfo], *, 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 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 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
+    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
+        rel_name = "/".join([*resolved, part])
+        candidate = members.get(rel_name)
+        if candidate is None:
+            return False
+        if candidate.issym():
+            hops += 1
+            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.isdir():
+                return False
+        elif not (candidate.isdir() or candidate.isreg()):
+            return False
+        resolved.append(part)
+    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.
+
+    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 ``<root>/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 + "/"):
+            # Consume the whole separator run at the boundary (`<root>//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
+        # 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)
@@ -1148,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:
@@ -1157,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
+                    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)

tests/sandbox/test_unix_local.py

@@ -719,6 +719,205 @@ 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 eligible local links and omit special files without relaxing hydration."""
+
+    @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.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 / "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
+
+    @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
+            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["double_sep"].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:
+        """`<root>/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_rebased_symlink_that_escapes_through_a_link_stays_absolute(
+        self,
+        tmp_path: Path,
+    ) -> None:
+        """`a/link -> ..` resolves to the workspace root, so `<root>/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_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
+    @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)
+        (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 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,