From 5729c6d3a60e1c7ffa56286c132e0799deb5c662 Mon Sep 17 00:00:00 2001 From: Huzaifa Abdul Rehman Date: Mon, 31 Aug 2026 18:14:29 +0500 Subject: [PATCH 1/4] fix(sandbox): reject external symlink targets by default #3094 walked nine call sites to pass allow_external_symlink_targets=False on every hydrate path. The parameter still defaults to True, so the safe behaviour is opt-in and a new backend inherits the unsafe one by staying silent. Flip the default on validate_tarfile, validate_tar_bytes and safe_extract_tarfile. No production call site changes behaviour, because all nine already pass the flag explicitly. strip_tar_member_prefix is the one caller that relied on the old default. It runs while persisting a snapshot, where an external symlink is something the workspace already contained rather than something an archive is introducing, so it now opts in explicitly. Hydration is where such a link would take effect and every hydrate path rejects it. Five test call sites also relied on the permissive default while asserting unrelated checks. Their assertions are unchanged; each opts out explicitly. --- src/agents/sandbox/util/tar_utils.py | 12 ++++-- tests/extensions/sandbox/test_blaxel.py | 6 +-- tests/sandbox/test_tar_utils.py | 54 +++++++++++++++++++++++-- 3 files changed, 62 insertions(+), 10 deletions(-) diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index 55adbd77e4..42671a33ca 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -157,7 +157,11 @@ def strip_tar_member_prefix(data: io.IOBase, *, prefix: str | Path) -> io.IOBase out.seek(0) with tarfile.open(fileobj=out, mode="r:*") as tar: - validate_tarfile(tar) + # Persisting a snapshot reads a workspace that already exists, so an external + # symlink here is something the workspace already contained rather than + # something an archive is introducing. Hydration is where that link would + # take effect, and every hydrate path rejects it. + validate_tarfile(tar, allow_external_symlink_targets=True) out.seek(0) return cast(io.IOBase, out) except Exception: @@ -259,7 +263,7 @@ def validate_tarfile( skip_rel_paths: Iterable[str | Path] = (), root_name: str | None = None, allow_symlinks: bool = True, - allow_external_symlink_targets: bool = True, + allow_external_symlink_targets: bool = False, ) -> None: """Validate a workspace tar before handing it to a local or remote extractor. @@ -345,7 +349,7 @@ def validate_tar_bytes( reject_symlink_rel_paths: Iterable[str | Path] = (), skip_rel_paths: Iterable[str | Path] = (), root_name: str | None = None, - allow_external_symlink_targets: bool = True, + allow_external_symlink_targets: bool = False, ) -> None: """Validate raw workspace tar bytes with the shared safe tar policy.""" @@ -369,7 +373,7 @@ def safe_extract_tarfile( tar: tarfile.TarFile, *, root: Path, - allow_external_symlink_targets: bool = True, + allow_external_symlink_targets: bool = False, ) -> None: """ Safely extract a tar archive into `root`. diff --git a/tests/extensions/sandbox/test_blaxel.py b/tests/extensions/sandbox/test_blaxel.py index 3fe1d0d93a..8b98629ef1 100644 --- a/tests/extensions/sandbox/test_blaxel.py +++ b/tests/extensions/sandbox/test_blaxel.py @@ -1178,8 +1178,8 @@ def _make_tar_with_symlink_and_file(*, symlink_name: str, target: str, file_name class TestValidateTarBytes: - def _validate(self, raw: bytes) -> None: - validate_tar_bytes(raw) + def _validate(self, raw: bytes, *, allow_external_symlink_targets: bool = False) -> None: + validate_tar_bytes(raw, allow_external_symlink_targets=allow_external_symlink_targets) def test_valid_tar(self) -> None: raw = _make_tar({"hello.txt": b"content", "subdir/": None}) @@ -1210,7 +1210,7 @@ def test_tar_member_under_archive_symlink_rejected(self) -> None: file_name="link.txt/nested.txt", ) with pytest.raises(ValueError, match="descends through symlink"): - self._validate(raw) + self._validate(raw, allow_external_symlink_targets=True) def test_corrupt_tar_rejected(self) -> None: with pytest.raises(ValueError, match="invalid tar"): diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index 50402557c6..be7a39efe4 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -16,6 +16,7 @@ safe_tar_member_rel_path, strip_tar_member_prefix, validate_tar_bytes, + validate_tarfile, ) @@ -165,6 +166,44 @@ def test_validate_tar_bytes_allows_internal_symlink_target_in_strict_mode() -> N validate_tar_bytes(raw, allow_external_symlink_targets=False) +def test_validate_tar_bytes_rejects_external_symlink_target_by_default() -> None: + raw = _tar_bytes(_symlink("leak", "/etc/passwd")) + + with pytest.raises(UnsafeTarMemberError, match="absolute symlink target not allowed"): + validate_tar_bytes(raw) + + +def test_validate_tarfile_rejects_external_symlink_target_by_default() -> None: + raw = _tar_bytes(_symlink("leak", "/etc/passwd")) + + with tarfile.open(fileobj=io.BytesIO(raw), mode="r:*") as tar: + with pytest.raises(UnsafeTarMemberError, match="absolute symlink target not allowed"): + validate_tarfile(tar) + + +def test_safe_extract_tarfile_rejects_external_symlink_target_by_default(tmp_path: Path) -> None: + raw = _tar_bytes(_symlink("leak", "/etc/passwd")) + root = tmp_path / "root" + root.mkdir() + + with tarfile.open(fileobj=io.BytesIO(raw), mode="r:*") as tar: + with pytest.raises(UnsafeTarMemberError, match="absolute symlink target not allowed"): + safe_extract_tarfile(tar, root=root) + + assert not (root / "leak").exists() + + +def test_strip_tar_member_prefix_still_persists_external_symlink_targets() -> None: + # Persisting reads a workspace that already contains the link. Hydration is where it + # would take effect, and every hydrate path rejects it. + raw = _tar_bytes(_dir("workspace"), _symlink("workspace/python", "/usr/bin/python3")) + + normalized = strip_tar_member_prefix(io.BytesIO(raw), prefix="workspace") + + with tarfile.open(fileobj=normalized, mode="r:*") as tar: + assert tar.getnames() == [".", "python"] + + def test_strip_tar_member_prefix_returns_workspace_relative_archive() -> None: raw = _tar_bytes( _dir("workspace"), @@ -292,27 +331,36 @@ def test_validate_tar_bytes_rejects_members_under_archive_symlink() -> None: ) with pytest.raises(UnsafeTarMemberError, match="descends through symlink"): - validate_tar_bytes(raw) + validate_tar_bytes(raw, allow_external_symlink_targets=True) def test_validate_tar_bytes_can_reject_specific_symlink_path() -> None: raw = _tar_bytes(_symlink("workspace", "/tmp/outside")) with pytest.raises(UnsafeTarMemberError, match="symlink member not allowed: workspace"): - validate_tar_bytes(raw, reject_symlink_rel_paths={Path("workspace")}) + validate_tar_bytes( + raw, + reject_symlink_rel_paths={Path("workspace")}, + allow_external_symlink_targets=True, + ) def test_validate_tar_bytes_specific_symlink_rejection_normalizes_dot_prefix() -> None: raw = _tar_bytes(_symlink("./workspace", "/tmp/outside")) with pytest.raises(UnsafeTarMemberError, match="symlink member not allowed: workspace"): - validate_tar_bytes(raw, reject_symlink_rel_paths={"workspace"}) + validate_tar_bytes( + raw, + reject_symlink_rel_paths={"workspace"}, + allow_external_symlink_targets=True, + ) def test_validate_tar_bytes_specific_symlink_rejection_does_not_reject_children() -> None: validate_tar_bytes( _tar_bytes(_dir("workspace"), _symlink("workspace/link", "/tmp/outside")), reject_symlink_rel_paths={"workspace"}, + allow_external_symlink_targets=True, ) From dc3cc58f599b2f77b6061a106ecd065f5e9deaba Mon Sep 17 00:00:00 2001 From: Huzaifa Abdul Rehman Date: Mon, 31 Aug 2026 19:14:06 +0500 Subject: [PATCH 2/4] test(sandbox): opt pre-existing workspace links into the permissive path Four tests seed an absolute symlink such as /usr/local/bin/python3 and then assert it round-trips or gets replaced. Preserving those pre-existing workspace links is the behaviour under test, so they now pass allow_external_symlink_targets=True explicitly rather than relying on a default. _safe_extract gains the same keyword, defaulting to the safe value so a new test cannot inherit the permissive one by staying silent. --- tests/sandbox/test_tar_utils.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index be7a39efe4..ae4fa5127f 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -71,9 +71,11 @@ def _fifo(name: str) -> _Member: return _Member(member) -def _safe_extract(raw: bytes, root: Path) -> None: +def _safe_extract(raw: bytes, root: Path, *, allow_external_symlink_targets: bool = False) -> None: with tarfile.open(fileobj=io.BytesIO(raw), mode="r:*") as tar: - safe_extract_tarfile(tar, root=root) + safe_extract_tarfile( + tar, root=root, allow_external_symlink_targets=allow_external_symlink_targets + ) def test_safe_extract_tarfile_preserves_venv_style_symlinks(tmp_path: Path) -> None: @@ -89,8 +91,8 @@ def test_safe_extract_tarfile_preserves_venv_style_symlinks(tmp_path: Path) -> N _symlink("./uv-project/.venv/bin/python", "python3"), ) - validate_tar_bytes(raw) - _safe_extract(raw, tmp_path) + validate_tar_bytes(raw, allow_external_symlink_targets=True) + _safe_extract(raw, tmp_path, allow_external_symlink_targets=True) assert (tmp_path / "uv-project" / "main.py").read_text() == 'print("snapshot smoke")\n' assert os.readlink(tmp_path / "uv-project" / ".venv" / "lib64") == "lib" @@ -239,7 +241,7 @@ def test_strip_tar_member_prefix_rewrites_pax_path_headers() -> None: def test_safe_extract_tarfile_can_rehydrate_existing_leaf_symlink(tmp_path: Path) -> None: raw = _tar_bytes(_symlink("link.txt", "/usr/local/bin/python3")) - _safe_extract(raw, tmp_path) + _safe_extract(raw, tmp_path, allow_external_symlink_targets=True) assert os.readlink(tmp_path / "link.txt") == "/usr/local/bin/python3" raw = _tar_bytes(_symlink("link.txt", "target-v2.txt")) @@ -278,7 +280,7 @@ def test_safe_extract_tarfile_can_replace_existing_leaf_symlink_with_file( tmp_path: Path, ) -> None: raw = _tar_bytes(_symlink("python", "/usr/local/bin/python3")) - _safe_extract(raw, tmp_path) + _safe_extract(raw, tmp_path, allow_external_symlink_targets=True) raw = _tar_bytes(_file("python", b"real file")) @@ -291,7 +293,7 @@ def test_safe_extract_tarfile_can_replace_existing_leaf_symlink_with_directory( tmp_path: Path, ) -> None: raw = _tar_bytes(_symlink("bin", "/usr/local/bin")) - _safe_extract(raw, tmp_path) + _safe_extract(raw, tmp_path, allow_external_symlink_targets=True) raw = _tar_bytes(_dir("bin"), _file("bin/python", b"real file")) From 9451ddecaaf7f1166ffcd3feedb98208052c35ab Mon Sep 17 00:00:00 2001 From: Huzaifa Abdul Rehman Date: Mon, 31 Aug 2026 19:31:00 +0500 Subject: [PATCH 3/4] fix(sandbox): reject Windows-style symlink targets in strict mode Symlink targets are parsed as PurePosixPath, so C:/Windows/System32, C:\Windows\System32, ..\..\Windows and \server\share all read as ordinary relative components and passed the strict checks. Member names already reject that syntax through _raise_if_windows_member_path; targets did not. Reject a Windows drive or a backslash in the target before the POSIX checks, so the strict default means what callers now rely on it meaning. --- src/agents/sandbox/util/tar_utils.py | 14 ++++++++++++++ tests/sandbox/test_tar_utils.py | 24 ++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index 42671a33ca..15746e2b62 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -59,6 +59,20 @@ def _validate_symlink_target( if not member.issym() or allow_external_symlink_targets: return + # Member names already reject Windows drive and separator syntax. Targets are parsed + # as POSIX, so `C:/Windows` and `..\\..\\Windows` would otherwise read as ordinary + # relative components and pass the checks below. + if PureWindowsPath(member.linkname).drive: + raise UnsafeTarMemberError( + member=member.name, + reason=f"windows drive symlink target not allowed: {member.linkname}", + ) + if "\\" in member.linkname: + raise UnsafeTarMemberError( + member=member.name, + reason=f"windows path separator in symlink target: {member.linkname}", + ) + target = PurePosixPath(member.linkname) if target.is_absolute(): raise UnsafeTarMemberError( diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index ae4fa5127f..0eaa0987cb 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -195,6 +195,30 @@ def test_safe_extract_tarfile_rejects_external_symlink_target_by_default(tmp_pat assert not (root / "leak").exists() +@pytest.mark.parametrize( + "target", + [ + "C:/Windows/System32", + "C:\Windows\System32", + "..\..\Windows", + "\\server\share", + ], +) +def test_validate_tar_bytes_rejects_windows_symlink_targets_by_default(target: str) -> None: + # Member names already reject this syntax. Targets are parsed as POSIX, so these + # would otherwise read as ordinary relative components. + raw = _tar_bytes(_symlink("link", target)) + + with pytest.raises(UnsafeTarMemberError, match="windows (drive symlink target|path separator)"): + validate_tar_bytes(raw) + + +def test_validate_tar_bytes_allows_windows_symlink_targets_when_opted_in() -> None: + raw = _tar_bytes(_symlink("link", "C:/Windows/System32")) + + validate_tar_bytes(raw, allow_external_symlink_targets=True) + + def test_strip_tar_member_prefix_still_persists_external_symlink_targets() -> None: # Persisting reads a workspace that already contains the link. Hydration is where it # would take effect, and every hydrate path rejects it. From 14c06a9052f5406a443e58adf05050815c1b9094 Mon Sep 17 00:00:00 2001 From: Huzaifa Abdul Rehman Date: Mon, 31 Aug 2026 19:51:34 +0500 Subject: [PATCH 4/4] fix(tests): use raw literals for Windows symlink targets The parametrized targets were written as plain strings, so `\W`, `\S`, `\.` and `\s` were invalid escape sequences. ruff reported W605 on five of them, which fails lint. The UNC case was also wrong rather than merely untidy: "\server\share" evaluates to \server\share, a single leading backslash, so it was not testing a UNC path at all. Raw literals fix both. --- tests/sandbox/test_tar_utils.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index 0eaa0987cb..5475ce4417 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -199,9 +199,9 @@ def test_safe_extract_tarfile_rejects_external_symlink_target_by_default(tmp_pat "target", [ "C:/Windows/System32", - "C:\Windows\System32", - "..\..\Windows", - "\\server\share", + r"C:\Windows\System32", + r"..\..\Windows", + r"\\server\share", ], ) def test_validate_tar_bytes_rejects_windows_symlink_targets_by_default(target: str) -> None: