fix(sandbox): reject external symlink targets by default - #4798
fix(sandbox): reject external symlink targets by default#4798HuzaifaAbdulRehman wants to merge 4 commits into
Conversation
openai#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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5729c6d3a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| skip_rel_paths: Iterable[str | Path] = (), | ||
| root_name: str | None = None, | ||
| allow_external_symlink_targets: bool = True, | ||
| allow_external_symlink_targets: bool = False, |
There was a problem hiding this comment.
With this default flipped, test_safe_extract_tarfile_preserves_venv_style_symlinks still calls validate_tar_bytes(raw) and _safe_extract(raw) on an archive containing ./uv-project/.venv/bin/python3 -> /usr/local/bin/python3; both now raise UnsafeTarMemberError, and the later replacement tests that seed absolute symlinks through _safe_extract fail for the same reason. Please update the persistence/round-trip callers or fixtures to pass allow_external_symlink_targets=True if preserving those pre-existing workspace links remains expected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and correct. Reproduced it directly: validate_tar_bytes on that venv fixture rejects /usr/local/bin/python3 with absolute symlink target not allowed.
My Windows run masked it. Those tests already fail at os.symlink with WinError 1314, so they stayed in the failing set for a different reason and comparing failing test IDs showed no change.
Fixed in dc3cc58. The venv round-trip and the three tests that seed an absolute link before replacing it now pass allow_external_symlink_targets=True, since preserving pre-existing workspace links is the behaviour under test. _safe_extract takes the same keyword, defaulting to the safe value so a new test cannot inherit the permissive one silently.
Full suite is back to the pristine baseline: 19 failures, identical set, identical causes (17 WinError 1314, 2 RecursionError), and no UnsafeTarMemberError raised anywhere.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc3cc58f59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| root_name: str | None = None, | ||
| allow_symlinks: bool = True, | ||
| allow_external_symlink_targets: bool = True, | ||
| allow_external_symlink_targets: bool = False, |
There was a problem hiding this comment.
When allow_external_symlink_targets now defaults to False, callers can rely on validate_tarfile()/safe_extract_tarfile() to reject external symlink targets, but _validate_symlink_target() only treats POSIX absolute paths and POSIX .. escapes as external. A tar containing link -> C:/Windows/System32 or link -> ..\..\Windows still passes this default and safe_extract_tarfile() will create that host-absolute/traversing link on Windows, so an untrusted archive can escape the extraction root despite the new strict default; reject Windows-drive and backslash target syntax before extraction. .agents/references/sandbox-runtime-boundary.mdL35-L39
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 9451dde. Under the strict default, C:/Windows/System32, C:\Windows\System32, ..\..\Windows and \\server\share were all accepted, because the target is parsed as a PurePosixPath while _raise_if_windows_member_path only covers member names. _validate_symlink_target now rejects a Windows drive or a backslash in the target, mirroring that helper, with a parametrized test over those four shapes.
One correction on the impact. safe_extract_tarfile has a single caller, unix_local.py:1118, and unix_local.py raises ImportError on win32 at module import, so it cannot run on a Windows host. Every other caller validates before handing the archive to a remote or containerised extractor, per the validate_tarfile docstring. So this was an incomplete guarantee in strict mode rather than a reachable escape.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9451ddecaa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "C:\Windows\System32", | ||
| "..\..\Windows", | ||
| "\\server\share", |
There was a problem hiding this comment.
When the lint path checks this test, these newly added string literals contain invalid escape sequences (\W, \., and \s); ruff check reports W605 on these lines, so make lint fails before the sandbox regression can merge. Make these entries raw strings or double-escape the backslashes so the Windows-target cases are valid Python literals.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, fixed in 14c06a9. ruff check was reporting five W605 on those lines and I had missed it. Repo-wide ruff check and ruff format --check now pass, exit 0 over 924 files.
Worth recording that the same escaping also broke one case semantically, not just stylistically. "\\server\share" evaluates to \server\share, a single leading backslash, so the UNC entry was not exercising a UNC path. It still passed, because any backslash in a target is rejected, so it was green for the wrong reason. The four entries are raw literals now and the values are C:/Windows/System32, C:\Windows\System32, ..\..\Windows and \\server\share.
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.
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Checked the current head after the follow-up fixes. The strict default is now consistent across validate_tarfile, validate_tar_bytes, and safe_extract_tarfile; snapshot persistence opts in explicitly, while hydration stays strict. The Windows drive/backslash cases are also rejected before POSIX target normalization, and the old venv/existing-link tests opt in only where that behavior is intentional. I don't see a blocker here.
2 participants
Summary
#3094 made every hydrate path reject external symlink targets, walking nine backend files to pass
allow_external_symlink_targets=False. The parameter still defaults toTrueonvalidate_tarfile,validate_tar_bytesandsafe_extract_tarfile, so the safe behaviour is opt-in and a new backend inherits the unsafe one by staying silent.This flips those three defaults. No production call site changes behaviour, since all ten call sites across those nine files already pass the flag explicitly.
strip_tar_member_prefixwas the only production caller left on the old default. It persists 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 were also sitting on the permissive default while asserting unrelated checks such as
descends through symlink, which is the same silent inheritance this change stops. No assertion was modified; each opts out explicitly.Test plan
Full suite on Windows, this branch against a pristine
89c02c82:The failing set is identical and those 19 fail the same way on a pristine checkout. The four extra passes are the new tests.
Four new tests pin the strict default on all three entry points, and one covers persisting an external target.
Issue number
N/A
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PRI ran
pytest,ruff format --check,ruff checkandmypy srcdirectly rather than the verification script, so I have left that box unticked.