fix(sandbox): prevent UnixLocal file API symlink races by seratch · Pull Request #4931 · openai/openai-agents-python

seratch · 2026-09-08T21:14:24Z

This pull request addresses a potential filesystem race in UnixLocal sandbox file APIs. If a workspace process replaces a validated path with a symlink between validation and the filesystem operation, the operation could reach a location outside the workspace or explicit grants.

For UnixLocal sessions, use descriptor-relative filesystem operations to avoid following replacement symlinks during traversal, file opening, and recursive removal. This also applies to apply_patch through its existing workspace file APIs.

Preserve supported symlinks, trusted manifest grant updates, stream ownership, user permission checks, error handling, and Python 3.10 compatibility. Other sandbox backends are unchanged.

chatgpt-codex-connector · 2026-09-08T21:14:38Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-09T00:10:45.587629Z	`20c32a3`	New commits
🔒 Security Review	✅ Completed 2026-09-09T00:23:39.737090Z	`20c32a3`	New commits

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c0d1d9e3b5

ℹ️ 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".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c2eaf0ab73

ℹ️ 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".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c2eaf0ab73

ℹ️ 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".

openai#4931 gave UnixLocal a dir_fd plus O_NOFOLLOW file-ops layer and routed the bound-user path through a Python worker running that same module. That is the primitive this change was approximating, so use it instead. write_new is the existing write with O_TRUNC replaced by O_EXCL, which claims the name in the same syscall that creates the file and fails with EEXIST when the name is taken by a file, a directory, or a dangling symlink. The worker exits with a distinct status for an existing target so the caller can report FileExistsError rather than a generic write error. This deletes the machinery the previous approach needed: the shell script, the staging file, the hard link, and the staging cleanup. With no hard link there is nothing to fail on writable object-storage mounts or across owners under fs.protected_hardlinks, and with no staging entry there is nothing for concurrent work to replace. The shared base now documents what it actually guarantees: it rejects an occupied target but is not atomic, and a backend that can claim a name should override it. test_parent_swap_after_validation_cannot_access_outside[patch] injected at session.normalize_path, which the create path no longer calls, so its swap stopped firing. Re-pointed the patch case at the file-ops authorize boundary the create path does traverse, keeping the coverage rather than letting it pass vacuously. Verified separately that a parent symlinked outside the workspace is still refused, with the outside file untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(sandbox): reject apply_patch create_file on an existing file Add File is documented to the model as creating a new file, and the delete and update operations both enforce their existing-file precondition. create_file enforced nothing, so an Add File operation aimed at a path that already existed overwrote it and reported "Created <path>", losing the previous contents with no error. Check that the destination is absent before writing, mirroring the existing _ensure_exists precondition used by delete_file. * fix(sandbox): keep the create precondition probe out of failed read spans The absence check reached SandboxSession.read(), which records a failed sandbox.read child span when the file is missing. Missing is the success case for a create, so every successful Add File looked like it contained a failed sandbox operation. Use the existing _read_with_expected_span_errors helper, the same path the skills capability already uses for an existence probe. * test(sandbox): skip the create span test on Windows The span assertion needs FilesystemTestSandboxSession, which is typed to UnixLocalSandboxSessionState, and importing agents.sandbox.sandboxes.unix_local raises ImportError on Windows by design. tests/conftest.py already collect-ignores the other files that depend on it, but test_apply_patch.py must stay collectible because its remaining tests are platform independent. The unix-only imports stay inside the test body so collection does not touch unix_local on Windows. * fix(sandbox): claim the create_file name at the backend write boundary The previous read-then-write check left a window: a concurrent sandbox command could create the file after the probe, and the unconditional write then destroyed that content. Reading also did not establish that a dangling symlink entry was absent, because the unix_local path policy resolves symlinks and the write landed on the link target. Add BaseSandboxSession.write_new_file(), which claims the target name before the payload is written and raises FileExistsError when the name is already taken. The shared implementation uses a shell noclobber redirection, so the redirect itself is the O_EXCL attempt; UnixLocal overrides it with os.open(O_CREAT|O_EXCL) for its direct path. Both validate the parent through the normal policy, preserving grants and the bound user, and leave the final component unresolved so a symlink at that name is rejected rather than followed. apply_patch create_file now uses it and drops the probe, so the tracing workaround for the probe read is no longer needed. Existing files still have to go through update_file. * fix(sandbox): link a completed payload into place for create_file Three problems with the previous commit. SandboxSession did not forward write_new_file, so a session built by SandboxClient fell back to the shared implementation and never reached the UnixLocal os.open override. Forward it. The shared implementation created an empty file and then wrote the payload in a separate step, so a concurrent writer could be overwritten and a failed upload left an empty file holding the name. Write the payload under a staging name first, then claim the target with ln, which fails when the name is taken. The content is complete before the name exists, and a failed create leaves only the staging entry, which is removed. The script used ':' for the noclobber redirection. ':' is a POSIX special builtin, so on dash a redirection failure ended the shell before the exit mapping ran and a collision surfaced as a generic write error instead of FileExistsError. ln is a regular command, and a test now runs the script through sh, dash and bash so this cannot regress silently. Also create local files with 0o666 so the process umask decides the final mode, matching Path.open("wb") on the ordinary write path. * fix(sandbox): claim the requested name, not its resolved target WorkspaceEditor normalizes the destination before dispatching, and UnixLocal resolves leaf symlinks, so create_file handed the primitive the link target. Add File on a dangling link.txt created missing.txt and reported success. Pass the unresolved path for create. Stage the payload locally too, then os.link it into place. os.link fails with EEXIST for a file, a directory or a dangling symlink, and a write that fails partway now leaves only the staging entry instead of a file holding the name. Reject a name held by a directory in the shared script. Bare ln treats an existing directory as a target directory and would have linked the staging file inside it while reporting the directory as created. Move the staging write inside the cleanup scope so a failed upload cannot leak the staging entry, and create the parent as the bound user so a fresh nested path is owned the way the previous write path owned it. The new tests drive session.apply_patch() rather than the primitive, which is the path that was actually broken. * fix(sandbox): drop the login shell and narrow the create collision Use sh -c instead of sh -lc for the exclusive create. This path runs for a filesystem-only capability set, so it must not source shell startup files that live in the workspace it is editing. A parent that is a regular file makes mkdir raise FileExistsError, and the broad handler reported that as a collision on the requested name, telling the model to use update_file for a target that does not exist. Only the os.link call can report a collision now; parent and staging failures are wrapped as write errors. * fix(sandbox): use a fixed-length staging basename The staging name was derived from the destination, so it was always longer than the destination itself. A filename that fits the filesystem's component limit, and that the ordinary write path accepts, could then fail to stage: a 254 character name raised WorkspaceArchiveWriteError where a plain write succeeded before this branch. The staging basename is now constant at 52 characters regardless of the destination. Reported by fscfede-beep in #4930. * fix(sandbox): classify a visible collision before staging the payload The staging write ran before the link script could classify the target, so a target inside an executable but non-writable parent failed on the staging write and the caller saw WorkspaceArchiveWriteError instead of the ApplyPatchDiffError that points it at update_file. It also meant an Add File onto an occupied name uploaded a payload that was then discarded. Probe the target first in both paths, then keep the atomic claim to decide real races. A creator that wins between the probe and the link still loses the name, and its staging entry is still cleaned up. Reported by Codex and by fscfede-beep in #4930. * fix(sandbox): build exclusive create on the descriptor-relative file ops #4931 gave UnixLocal a dir_fd plus O_NOFOLLOW file-ops layer and routed the bound-user path through a Python worker running that same module. That is the primitive this change was approximating, so use it instead. write_new is the existing write with O_TRUNC replaced by O_EXCL, which claims the name in the same syscall that creates the file and fails with EEXIST when the name is taken by a file, a directory, or a dangling symlink. The worker exits with a distinct status for an existing target so the caller can report FileExistsError rather than a generic write error. This deletes the machinery the previous approach needed: the shell script, the staging file, the hard link, and the staging cleanup. With no hard link there is nothing to fail on writable object-storage mounts or across owners under fs.protected_hardlinks, and with no staging entry there is nothing for concurrent work to replace. The shared base now documents what it actually guarantees: it rejects an occupied target but is not atomic, and a backend that can claim a name should override it. test_parent_swap_after_validation_cannot_access_outside[patch] injected at session.normalize_path, which the create path no longer calls, so its swap stopped firing. Re-pointed the patch case at the file-ops authorize boundary the create path does traverse, keeping the coverage rather than letting it pass vacuously. Verified separately that a parent symlinked outside the workspace is still refused, with the outside file untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sandbox): keep supported parent symlinks and stop reading the target Two problems with the previous commit. Passing the whole path into the file ops unresolved made a supported internal symlink parent fail. The ordinary write path resolves those safe aliases, and test_safe_symlinks_grants_and_listing_paths_remain_supported establishes the support, but a create through "internal -> real" opened "internal" with O_NOFOLLOW | O_DIRECTORY and failed. Resolve the parent the way write does and keep only the leaf name unresolved, so a dangling symlink at the target name is still rejected without its target being created. The shared default probed the target with read(), which eagerly fetches the whole payload on the remote backends that inherit it. An Add File aimed at an existing large file would download it before reporting the collision, and an existing file the bound user cannot read reported a read failure instead of the collision. List the parent instead, which needs no payload and no read permission on the target. Both regressions have a test that fails on the unfixed source, including one asserting the rejected create issues no read at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sandbox): probe the target directly instead of listing its parent Listing the parent to decide a collision failed when the parent did not exist. BaseSandboxSession.ls() raises ExecNonZeroError in that case, which the probe did not catch, so a nested create such as newdir/file.txt aborted before write() on every backend inheriting the default, while succeeding on UnixLocal. Those backends' write paths create missing parents. Use a bare existence test instead. It needs only execute permission on the parent, never reads the target, and reports absent when the parent is missing so the create still reaches write(). Only a positive result rejects; any other probe outcome falls through rather than blocking a valid create. The test is also more accurate than the listing: it sees a file inside an executable but unreadable parent, which listing could not, so an existing file there is no longer silently overwritten. Validated the exact shipped script under sh, bash and dash for an existing file, a dangling symlink, an absent name, a missing parent, and a file in a 0311 parent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sandbox): fail closed on a failed probe and keep scripted creates working Two problems with the previous commit. The probe treated any unexpected status as "absent" so a valid create would not be blocked. That was wrong: exit 0 already means absent, including when the parent does not exist, so the permissive fallback only covered the case where the probe itself did not run. On provider sessions whose write() uses a separate upload API, the write then succeeds and overwrites an existing target precisely when the precondition could not be checked. Only 0 may proceed now; 13 stays the collision and every other status is a write error. scripted_sandbox_session() broke. The inherited default reaches for exec, and a script that configures only file steps hides exec, so a previously valid mkdir-and-write script failed with AttributeError. Give the scripted session an implementation that consumes the same mkdir and write pair, and restore the explicit parent creation in the default so the observable call sequence matches what the create path did before this branch. Both have a test that fails on the unfixed source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sandbox): keep scripted create calls on their normalized paths The create path hands write_new_file an unresolved path on purpose, so a symlink at the target name is rejected rather than followed. The scripted session forwarded that path straight into its compatibility mkdir and write pair, so a script using the documented match callback saw "." and "notes.txt" where it previously saw the workspace paths, and failed with SandboxCallMatcherError. Normalize before emitting the pair. Verified against the previous commit: with a custom manifest root the calls were "sub" and "sub/notes.txt" and are now "/custom/root/sub" and "/custom/root/sub/notes.txt", matching what this surface recorded before this branch. The earlier test only asserted method names, which is why it could not catch this. It now asserts the recorded paths too and fails on the unfixed source. * fix(sandbox): narrow exclusive creates and clarify partial-write recovery --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Justin Beckwith <jbeckwith@openai.com>

* fix: keep the file when apply_patch does a case-only rename On a filesystem that folds case, `notes.txt` and `Notes.txt` are the same file. `_apply_update` wrote the new text to the destination and then removed the source, and because the path comparison is case-sensitive the removal deleted the file that had just been written. The user's edit was lost. For a case-only rename, remove the source before writing instead. That end state is correct on a folding filesystem and on a case-sensitive one, so the SDK does not have to know which kind it is talking to. The write is wrapped so a failure restores the original text at the source path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: commit the rename before removing the source The previous commit removed the source first on a case-only rename and restored the original text from memory if the write then failed. That trades one way to lose the file for another: if the write and the restore both fail the file is gone, and a restore that lands late overwrites whatever another writer put at the source path in the meantime. Ask the filesystem instead of comparing strings. `same_file` runs `[ "$1" -ef "$2" ]`, which compares device and inode, so it answers for a filesystem that folds case and for one that folds Unicode normalisation, which `str.casefold` cannot. `mv` renames a path and refuses a directory destination, because `mv` given a directory moves the source inside it and exits 0, which for a caller that then removes the source is a way to delete a file while believing it moved. The rename now writes a staging file, commits it onto the destination with one move, and removes the source only when the filesystem says it is a different file. Nothing is removed before the new content is on disk, and nothing is written back after a failure. Three of the new tests run on the macOS runner against a real case-folding volume, so the behaviour is no longer only modelled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: answer identity without following symlinks, and stage safely Acts on five of the six findings the Codex reviewer raised on 5b3650c. `test -ef` follows symlinks, so a source symlink pointing at the destination answered "same file" and the removal was skipped, leaving the old name pointing at the new one. `same_file` takes `follow_symlinks` now, and the editor asks with it off, because the answer decides whether removing one path destroys the other. The staging write moves inside the `try`, so a write that fails after creating the file no longer orphans it. The staging basename is a fixed length, because decorating a destination basename near the 255-byte limit overflowed it. A `move_to` naming the path the file already has short-circuits to an in-place write, which is what an update without `move_to` does and what this code did before the rename was staged. `docs/testing.md` is reverted: AGENTS.md keeps documentation for unreleased behaviour out of the pull request that introduces it. The directory check is still not atomic with the rename. Closing that needs a per-backend rename primitive, which is a question for the maintainer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: say what the removal does not guarantee The docstring said a file another writer creates at the source path is never overwritten. That is true and was standing in for a guarantee it does not make: the identity answer is read before the removal, and the removal names a path rather than the entry that answer was about, so such a file can still be removed. No behaviour change. Closing the window needs a removal that can be told which entry it may remove, which is the per-backend question already open with the maintainer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: complete case-only renames on folding filesystems * fix: skip redundant moves through path aliases * fix: ask the sandbox, not the host, whether two paths are one file `Path` equality folds case on a Windows host, so a case-only `move_to` took the same-path fast path there. The update was written in place, the operation reported success, and the requested name was never created in a case-sensitive sandbox. The host's filesystem semantics say nothing about the sandbox's. Comparing the canonical spellings keeps the fast path for a genuine no-op and sends a case-only rename down the staging-and-move path, where `same_file` asks the sandbox whether the two paths are one entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: say what committing a new inode costs an existing move target A staged commit replaces the destination inode, so a rename that lands on an existing distinct file replaces that file's mode, ownership and extended attributes, where writing into it in place kept them. The docstring covered only the case-folding entry. Preserving them would cost an existence probe and the single-`mv` commit for that case; the content is worth more than the bits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: rename and compare entries descriptor-relative on UnixLocal Since #4931, every UnixLocal file operation runs against a directory descriptor so the path that was validated is the entry acted on. The rename and identity check this branch added still went through `sh -lc`, which re-resolves the path and reopens the window #4931 closed. `_FileOps` gains `rename`, an `os.rename` with `src_dir_fd` and `dst_dir_fd`, and `same_file`, a descriptor-relative `stat` compared by device and inode. UnixLocal overrides `mv` and `same_file` with them, directly for the host user and through the file worker for a bound user. `os.rename` also refuses an existing directory as the destination, where `mv` would have moved the source inside it. The shell versions stay on `BaseSandboxSession` for backends that only offer `exec`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Restrict sandbox moves to files and symlinks * fix: preserve diverged source after alias patch commit * ci: allow Windows tests time to finish cleanup --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Justin Beckwith <jbeckwith@openai.com>

seratch added this to the 0.22.x milestone Sep 8, 2026

seratch added the feature:sandboxes label Sep 8, 2026

chatgpt-codex-connector Bot reviewed Sep 8, 2026

View reviewed changes

Comment thread src/agents/sandbox/sandboxes/unix_local.py

Comment thread src/agents/sandbox/sandboxes/_unix_local_files.py Outdated

seratch force-pushed the fix/unix-local-file-io-races branch from c0d1d9e to c2eaf0a Compare September 8, 2026 22:42

chatgpt-codex-connector Bot reviewed Sep 8, 2026

View reviewed changes

Comment thread src/agents/sandbox/sandboxes/unix_local.py

chatgpt-codex-connector Bot reviewed Sep 8, 2026

View reviewed changes

Comment thread src/agents/sandbox/sandboxes/unix_local.py Outdated

seratch force-pushed the fix/unix-local-file-io-races branch from c2eaf0a to 5f75bc6 Compare September 8, 2026 23:38

fix(sandbox): guard UnixLocal file APIs against path races

20c32a3

seratch force-pushed the fix/unix-local-file-io-races branch from 5f75bc6 to 20c32a3 Compare September 9, 2026 00:07

seratch enabled auto-merge (squash) September 9, 2026 01:34

seratch merged commit e3a03ed into main Sep 9, 2026
35 of 36 checks passed

seratch deleted the fix/unix-local-file-io-races branch September 9, 2026 01:34

seratch mentioned this pull request Sep 9, 2026

release: 0.22.2 #4935

Merged

ayaangazali mentioned this pull request Sep 9, 2026

fix(sandbox): scope exclusive Add File creation to UnixLocal #4893

Merged

ayaangazali mentioned this pull request Sep 12, 2026

fix: keep the file when apply_patch does a case-only rename #4890

Merged

This was referenced Sep 25, 2026

fix(sandbox): refuse FIFO, socket, and device nodes in UnixLocal read/write instead of blocking the event loop #4891

Closed

fix(sandbox): reject special files in shared UnixLocal file I/O #5177

Merged

Navigation Menu

Repository navigation

fix(sandbox): prevent UnixLocal file API symlink races - #4931

fix(sandbox): prevent UnixLocal file API symlink races#4931
seratch merged 1 commit into
mainfrom
fix/unix-local-file-io-races

seratch commented Sep 8, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 8, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

seratch commented Sep 8, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

chatgpt-codex-connector Bot commented Sep 8, 2026 •

edited

Loading