fix(sandbox): remove a symlink itself in UnixLocal rm, not its target

[coderdailyone]

Summary

This pull request fixes UnixLocalSandboxSession.rm() so that removing a symlink removes the link itself instead of its target.

Bug

UnixLocalSandboxSession.normalize_path() resolves every symlink (resolve_symlinks=True), and rm() operated on that resolved path. Against a real UnixLocalSandboxClient session on main (89c02c8), after the agent ran ln -s plain.txt linkfile; ln -s realdir linkdir; ln -s /etc/hostname outside_file; ln -s /nonexistent dangling in the workspace:

call	result on main
rm("linkfile")	deleted plain.txt; linkfile left dangling
rm("linkdir", recursive=True)	shutil.rmtree on realdir/ (all contents gone); linkdir left dangling
rm("dangling")	InvalidManifestPathError: manifest path must not escape root — cannot be removed
rm("outside_file")	same error — cannot be removed

So a model asking to delete a link destroys the real data, and a link the model itself created (pointing outside or dangling) can never be cleaned up through the file API.

This is UnixLocal-specific. The exec-backed session implementation in BaseSandboxSession.rm() runs rm -rf -- <path>, which removes the link, and the docstring of _validate_remote_path_access() already states the intended contract: "keeps safe leaf symlink operations working normally, such as removing a symlink instead of its target".

Fix

• UnixLocalSandboxSession._rm_target_path() validates the entry's parent directory with symlinks resolved (so containment and read-only-grant checks still apply), then keeps the leaf name unresolved. A leaf symlink is therefore unlinked as a symlink; regular files and directories behave exactly as before because their parent-resolved path equals the fully resolved path.
• BaseSandboxSession._check_rm_with_exec() is split so the sandbox-side access check (_check_rm_access_with_exec()) can run against an already-validated path. The user-scoped rm(..., user=...) path in UnixLocal now checks and removes the link entry rather than its target. _check_rm_with_exec() keeps its signature and behavior for existing callers.
• WorkspaceEditor (the apply_patch tool) now passes the workspace-relative path to rm() for delete_file and for the source of a move_to, instead of the symlink-resolved destination, so deleting or moving a link through apply_patch removes the link rather than its target on UnixLocal. The delete_file existence check reads the resolved target when it resolves inside an allowed root and otherwise confirms the named entry through ls() of its parent, so dangling and outward-pointing links can be deleted too. Exec-backed backends already removed the link and are unchanged.
• A configured extra path grant that is itself a symlink is addressed through its resolved form, exactly like a symlinked workspace root, so rm on the grant root alias stays authorized.
• Paths that reach an entry through an escaping symlinked parent (e.g. rm("escape_dir/victim.txt") where escape_dir -> /outside) are still rejected with InvalidManifestPathError.

Behavior change

rm on a symlink inside a UnixLocal workspace now removes only the link, matching POSIX rm, rm -rf, and every other sandbox backend. No public signatures change.

• The user-scoped probe (_RM_ACCESS_CHECK_SCRIPT, run as the requested user via sudo -u) now also enforces POSIX sticky-directory ownership: in a sticky directory the user does not own (e.g. /tmp), the entry itself must be owned by that user, judged with find -maxdepth 0 -user so a symlink is checked by the link's owner rather than its target's. This closes the gap where the SDK process unlinked an entry the requested user could not remove, which the newly accepted external-target and dangling symlinks would otherwise have widened.

Test plan

• tests/sandbox/test_session_utils.py::test_rm_access_check_enforces_sticky_directory_ownership runs the real probe script with sh against a sticky directory owned by another uid (as root): another user's file and symlink are refused, our own file and symlink are allowed; it fails on the previous revision. test_rm_access_check_allows_own_entries_in_a_writable_directory covers the ordinary directory, a dangling symlink entry, and the missing-path cases.

• New tests in tests/sandbox/test_unix_local.py::TestUnixLocalRmSymlinks cover file symlink, directory symlink (recursive and not), symlink pointing outside the workspace, dangling symlink, the escaping-parent rejection, and the user-scoped path. Two apply_patch tests (delete_file and move_to from a link) sit alongside them and fail without the apply_patch.py change. Six of the seven rm tests fail on main and pass with this change.

• tests/sandbox (1453 passed), the full parallel suite (9261 passed; two tests/mcp/test_mcp_pagination_integration.py stdio tests failed under a memory-constrained 3-worker run and pass when rerun alone), serial tests, ruff format --check, ruff check, and mypy/pyright on the changed files all pass locally on Linux / Python 3.10.

Verification stack: make format/ruff check clean, mypy and pyright clean on the changed files, focused suites plus tests/sandbox pass on Linux/Python 3.10. The repository-wide mypy src reports pre-existing errors on Python 3.10 (archive_ops.py SpooledTemporaryFile typing, run_loop.py BaseExceptionGroup export) that are unrelated to this PR, so the full-stack checkbox is left unchecked.

Issue number

None (found while auditing the UnixLocal / Docker sandbox file operations).

Checks

• I've added new tests, if relevant
• I've run the verification steps from .agents/skills/code-change-verification individually (format, lint, typecheck on changed files, tests)
• I've confirmed all verification steps pass (see Test plan: pre-existing Python 3.10 mypy errors outside this change)
• If using Codex, I've run /review before submitting this PR

🤖 Generated with Claude Code

https://claude.ai/code/session_01BN4v25msJgrjNgac97g1Az

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6ff8a29e3d

ℹ️ 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: e505c43354

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

[sylvesterkaczmarek]

Keeping the leaf unresolved seems to reintroduce a path-based TOCTOU. After _rm_target_path() resolves and authorizes the parent, a background process can rename that parent and replace it with a symlink before unlink()/rmtree() runs; the host-side delete then follows the new parent outside the validated tree. Could this pin the validated parent with a dir fd and remove relative to that inode, rather than acting on the reconstructed pathname?

[coderdailyone]

Thanks for looking at this. I agree there is a rename race between authorizing the parent and the unlink()/rmtree() call, but I don't think this change introduces it: main already does normalized = normalize_path(...) (a Path.resolve()) and then shutil.rmtree(normalized) / normalized.unlink() by pathname, and read(), write(), mkdir() and ls() follow the same resolve-then-operate-by-path pattern. The window is the same size before and after; what changed is only which entry is removed (the link rather than its target).

Closing that class properly means pinning the validated directory (open with O_DIRECTORY, fstat against the inode seen at validation, then unlink(name, dir_fd=...) / rmdir(name, dir_fd=...)) across every UnixLocal file operation, and shutil.rmtree only grew dir_fd in Python 3.11 while this package supports 3.10. It is also worth keeping in mind that UnixLocal runs exec unconfined on Linux as the same user, so a process able to race the parent already has direct host access; the path policy is a guardrail for the file API rather than a boundary against a concurrent hostile process.

So I'd prefer to keep this PR to the symlink-target bug and treat fd-pinned removal as a separate, cross-operation change. Happy to write that follow-up if maintainers want it.

[chatgpt-codex-connector]

💡 Codex Review

openai-agents-python/src/agents/sandbox/workspace_paths.py

Line 458 in 2bf051c

grant = self._matching_grant(resolved, resolve_roots=True)

Allow removing a symlinked writable grant root

When a path-only writable extra_path_grant is itself a symlink, such as /tmp/shared-link -> /srv/shared, calling rm("/tmp/shared-link") now preserves the candidate as /tmp/shared-link but compares it only against the resolved grant root /srv/shared. _matching_grant() therefore returns None and raises InvalidManifestPathError, even though grants explicitly apply to SDK file APIs and the intended leaf-preserving behavior should unlink this writable grant entry rather than reject it; authorize the configured grant entry itself while retaining resolved-root matching for descendants.

AGENTS.md reference: AGENTS.md:L205-L205

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

[seratch]

The ordinary symlink cases are fixed. The existing sticky-directory gap is newly reachable for an external-target symlink that the base rejected: user= checks only parent write/execute access, then the SDK identity unlinks an entry the requested user may not own. Please enforce the requested user's removal permission on this newly accepted leaf path and add a controlled non-owner case.

[coderdailyone]

Thanks for the review. Addressed in 4b3a17d0: the user-scoped probe now enforces the requested user's removal permission on the leaf entry itself. In a sticky directory the user does not own, the entry must be owned by that user (find -maxdepth 0 -user, so a symlink is judged by the link's owner, not its target's); ordinary directories keep the write/search check. test_rm_access_check_enforces_sticky_directory_ownership runs the real probe with sh against a sticky directory owned by another uid and checks a non-owner file and symlink are refused while the user's own are allowed (root-only, since it needs to stage another owner; it fails on the previous revision), and test_rm_access_check_allows_own_entries_in_a_writable_directory covers the ordinary, dangling-symlink, and missing-path cases.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b3a17d057

ℹ️ 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]

Allow privileged users through the sticky-directory probe

When rm(..., user="root") targets another user's entry in a sticky directory that root does not own, this branch rejects the operation solely because neither ownership test passes. On Linux, the exact probe returns 1 in that scenario while rm succeeds because root has CAP_FOWNER, so UnixLocalSandboxSession.rm() raises WorkspaceArchiveWriteError for a deletion that the requested identity is authorized to perform. Account for privileged-user semantics rather than treating ownership as exhaustive.

Useful? React with 👍 / 👎.

[coderdailyone]

Fixed in 001a24c6: the ownership rule is skipped when the probe runs as uid 0, since root (CAP_FOWNER) may unlink any entry it can reach. The sticky test now runs the real probe as an unprivileged user (subprocess.run(..., user=nobody)) for the non-owner refusals, and asserts that root is allowed to remove another user's file inside a sticky directory root does not own; that last assertion fails on the previous revision.

[chatgpt-codex-connector]

Canonicalize the configured root before comparing it

When Manifest.root is accepted as a noncanonical symlink spelling such as /tmp/dummy/../ws-link, passing that same root value to rm() misses this resolving branch because it is not lexically equal to the normalized /tmp/ws-link; the unresolved leaf is then outside the resolved /tmp/ws root and is rejected, although the previous full-resolution path and rm(".") accept it. Fresh evidence beyond the prior root-alias fix is that test_rm_validates_the_symlinked_root_alias_itself constructs exactly this noncanonical root but tests only "." and the canonical alias, not the configured string itself. Compare canonical root identities through the existing path-policy source of truth rather than lexical Path equality.

AGENTS.md reference: AGENTS.md:L95-L95

Useful? React with 👍 / 👎.

[coderdailyone]

Fixed in 001a24c6: both sides of the root comparison now go through the policy's normalized root (posixpath.normpath of the absolute input against _normalized_root()), so the configured noncanonical spelling itself (/tmp/dummy/../ws-link) resolves as the root. test_rm_validates_the_symlinked_root_alias_itself now also passes that exact configured string and fails on the previous revision.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 001a24c6f5

ℹ️ 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]

Use a portable ownership check on macOS

On the supported macOS UnixLocal backend (including the native-macos-sandbox workflow), /usr/bin/find does not support the GNU-only -maxdepth primary; it is absent from Apple's find(1) expression reference. Consequently, when rm(..., user=...) checks an entry owned by that non-root user in a sticky directory owned by someone else, find fails, its stderr is discarded, the substitution is empty, and the SDK raises WorkspaceArchiveWriteError even though the user may unlink the entry. Use a portable lstat/ownership probe or platform-specific syntax.

Useful? React with 👍 / 👎.

[coderdailyone]

Fixed in e25457b2: the ownership probe now uses stat -c %u with a stat -f %u fallback (GNU and BSD/macOS spellings), both of which report the entry itself without -L, so a symlink is still judged by the link's owner and macOS find is no longer involved. The sticky tests (run as an unprivileged user and as root) pass unchanged.

[coderdailyone]

Added one more call site in 4736a7a6: the model-facing apply_patch tool reached rm() through WorkspaceEditor, which passed the symlink-resolved destination rather than the entry the model named. On UnixLocal that meant {"type": "delete_file", "path": "alias"} with alias -> plain.txt deleted plain.txt and left alias dangling, and update_file with move_to from a link did the same to its target. WorkspaceEditor now hands the workspace-relative path to rm() so the backend's leaf semantics apply; the exec-backed backends already removed the link and are unchanged. Two tests in TestUnixLocalRmSymlinks cover both operations and fail without the apply_patch.py change.

The PR description's Fix section is updated accordingly.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4736a7a618

ℹ️ 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]

Recognize symlink aliases for exact grant roots

When a writable extra_path_grants entry is itself a symlink, rm(str(grant.path), recursive=True) now preserves that leaf, but _matching_grant(..., resolve_roots=True) compares it only with the resolved grant target and raises InvalidManifestPathError; the old full-resolution path accepted the configured grant root, and paths below the alias remain accepted. Fresh evidence beyond the earlier workspace-root-alias fix is that this condition special-cases only _normalized_root(), with no equivalent canonical identity check for configured grants. Recognize an exact configured grant alias through the canonical grant policy before preserving its leaf.

AGENTS.md reference: AGENTS.md:L94-L95

Useful? React with 👍 / 👎.

[coderdailyone]

Fixed in 4e3f519f: _is_configured_root_alias() now treats the configured spelling of every extra path grant the way it already treated the workspace root, so rm(str(grant.path), ...) on a grant that is itself a symlink resolves fully and is authorized against the grant's resolved target, while entries below it keep their leaf. test_rm_accepts_a_symlinked_grant_root_alias covers both a file below the alias and the alias itself.

[chatgpt-codex-connector]

Reach rm before resolving delete-file symlinks

When delete_file names a dangling symlink or a symlink whose target is outside the workspace, this new rm(relative_path) call is never reached: the earlier normalize_path() follows the leaf and either rejects the outside target, while _ensure_exists() reads the resolved target and reports a dangling link as missing. Consequently, apply_patch still cannot delete those named workspace entries even though UnixLocalSandboxSession.rm() now explicitly supports both cases; validate existence without following the leaf for the delete branch.

Useful? React with 👍 / 👎.

[coderdailyone]

Fixed in 4e3f519f: the delete_file branch no longer goes through the leaf-following normalize_path() first. _ensure_entry_exists() reads the resolved target when the leaf resolves inside an allowed root, and otherwise (dangling link, or a link whose target is outside) confirms the named entry through ls() of its parent, then rm() receives the workspace-relative path. test_apply_patch_delete_file_removes_dangling_and_outward_symlinks (parametrized over missing.txt and /etc/hostname) and test_apply_patch_delete_file_reports_a_missing_entry cover it; the fake apply_patch session gained an ls() for the new existence check.

[sylvesterkaczmarek]

Thanks for the extra symlink and apply_patch coverage. One earlier blocker is still present on the current head: _rm_target_path() resolves and validates the parent, but rm() later calls shutil.rmtree(normalized), normalized.rmdir(), or normalized.unlink() by pathname. A concurrent rename and symlink replacement of the parent between those steps can still redirect the delete outside the validated tree. I would still pin the parent inode and remove relative to it, or otherwise make validation and deletion atomic.

[coderdailyone]

Thanks. My position from the earlier thread is unchanged: the resolve-then-operate-by-pathname window exists identically on main for rm(), read(), write(), mkdir() and ls(), and this PR narrows what that window can reach (the link instead of its target) without widening it. Pinning the parent inode (O_DIRECTORY + fstat + unlink/rmdir with dir_fd, plus an fd-walking recursive delete since shutil.rmtree(dir_fd=...) is 3.11+) is a cross-operation hardening for UnixLocal that I am happy to do as a follow-up if the maintainers want it, but I would rather keep this change to the symlink-target bug so it can be reviewed on its own.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4e3f519fe1

ℹ️ 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]

Inspect the link entry before opening its target

When delete_file names a symlink to an in-workspace directory, normalization follows the leaf and this call attempts to open the target directory; UnixLocalSandboxSession.read() wraps the resulting IsADirectoryError as WorkspaceArchiveReadError, so the listing fallback never runs and the link remains. Check the named directory entry before reading its target, so apply_patch supports the same directory-symlink removal that the updated rm() path now supports.

AGENTS.md reference: AGENTS.md:L205-L205

Useful? React with 👍 / 👎.

[sylvesterkaczmarek]

That scope distinction makes sense. The resolve-then-operate pathname race is broader UnixLocal hardening that already exists across multiple operations, so I agree it should not remain a blocker specific to this symlink-leaf fix. I would track the inode-pinned hardening separately. The current directory-symlink delete case is a separate issue already raised inline on the latest head, so I will leave that thread to resolve rather than duplicate it here.