diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index 928f255b1f..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 @@ -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 ``/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 (`//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) diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index de24f3f3bf..6723425db7 100644 --- a/tests/sandbox/test_unix_local.py +++ b/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: + """`/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 `/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,