fix(sandbox): remove a symlink itself in UnixLocal rm, not its target - #4830
fix(sandbox): remove a symlink itself in UnixLocal rm, not its target#4830coderdailyone wants to merge 3 commits into
Conversation
UnixLocalSandboxSession.rm() resolved every symlink before removing, so `rm` on a symlink deleted the link target and left the link dangling: a file symlink unlinked the real file, and a directory symlink with recursive=True rmtree'd the real directory. Symlinks that pointed outside the workspace or nowhere could not be removed at all, because the resolved target failed the workspace check. POSIX `rm`, the exec-backed session implementations (`rm -rf -- path`), and the documented intent of _validate_remote_path_access all remove the link itself. Validate the entry's parent directory (following symlinks) and keep the leaf name unresolved, for both the direct and the user-scoped rm paths. Entries reached through an escaping symlinked parent are still rejected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BN4v25msJgrjNgac97g1Az
![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: 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".
| POSIX ``rm`` and the exec-backed sessions remove the link itself, so validate the entry's | ||
| parent directory (following symlinks) and keep the leaf name unresolved. | ||
| """ | ||
| raw_path = Path(path) |
There was a problem hiding this comment.
On Unix, when path is a raw drive-absolute Windows string such as C:\\outside\\file, Path(path) treats the backslashes as ordinary filename characters, so only . is normalized and the call deletes <workspace>/C:\\outside\\file. Previously, normalize_path() recognized this as an absolute Windows path and raised InvalidManifestPathError; validate the original input with the canonical workspace path helpers before separating the unresolved leaf.
AGENTS.md reference: AGENTS.md:L205-L205
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in e505c43. _rm_target_path() now runs the raw input through WorkspacePathPolicy.normalize_path(path, for_write=True) (no symlink following) before splitting off the leaf, so a Windows drive-absolute string such as C:\outside\file raises InvalidManifestPathError exactly as before instead of being treated as a literal entry name under the workspace root. Added test_rm_rejects_absolute_and_escaping_raw_paths_before_splitting_the_leaf, which fails on the previous revision for the C:\... case and passes now.
| if user is not None: | ||
| normalized = await self._check_rm_with_exec(path, recursive=recursive, user=user) | ||
| else: | ||
| normalized = self.normalize_path(path, for_write=True) | ||
| await self._check_rm_access_with_exec(normalized, recursive=recursive, user=user) |
There was a problem hiding this comment.
When user= names a non-owner removing a symlink from a sticky world-writable directory, _RM_ACCESS_CHECK_SCRIPT succeeds because it checks only parent write/execute permission, even though an actual rm as that user is rejected by the sticky bit. The subsequent normalized.unlink() runs as the SDK process and therefore deletes an entry the requested user cannot remove; execute the removal under the requested identity or extend the check to enforce sticky-directory ownership.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is pre-existing behavior of the user-scoped path rather than something this change introduces: before this PR, rm("file", user=...) ran the same _RM_ACCESS_CHECK_SCRIPT against the same parent directory and then unlinked as the SDK process, so a non-owner in a sticky world-writable directory was already able to remove another user's regular file. The only difference now is that the check and the unlink target the symlink entry instead of its resolved target, which is the intended rm semantics.
I'd like to keep this PR scoped to the symlink-target bug. Enforcing sticky-directory ownership in the access check (or performing the removal under the requested identity) affects every user-scoped rm, not just symlinks, and is hard to exercise in the test suite because CI does not run as a second uid. Happy to send it as a follow-up PR if maintainers want it.
Run the raw input through the workspace path policy (without following symlinks) before separating the unresolved leaf name, so a Windows drive-absolute string such as `C:\outside\file` is still rejected with InvalidManifestPathError on Unix instead of being treated as a literal entry name under the workspace root. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BN4v25msJgrjNgac97g1Az
There was a problem hiding this comment.
💡 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".
| parent = self.normalize_path(raw_path.parent, for_write=True) | ||
| return parent / raw_path.name |
There was a problem hiding this comment.
When a workspace symlink points into a writable extra grant and the leaf is itself a more-specific read-only grant—for example, tmp-link -> /tmp/shared with /tmp/shared/protected read-only—rm("tmp-link/protected", recursive=True) validates only the writable parent and then appends protected without applying the longest matching grant. The host-side rmtree can therefore delete the read-only grant root, whereas the previous full-path normalization rejected it; validate the reconstructed entry against the canonical grant policy without following the leaf symlink.
AGENTS.md reference: AGENTS.md:L95-L95
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and thanks — this and the root-alias comment below have the same root cause, so I replaced the hand-built parent/leaf split. WorkspacePathPolicy.normalize_path() gained follow_leaf_symlink=False (with resolve_symlinks=True it resolves the parents, keeps the final component, and then applies the workspace root, the longest matching extra grant, and the read-only check to that location). _rm_target_path() is now a single call into that. Regression test test_rm_applies_the_most_specific_grant_to_the_leaf covers exactly this shape (tmp-link -> <writable grant> with a nested read-only grant root) and fails on the previous revision.
| # Validate the raw input with the workspace path policy first (without following | ||
| # symlinks) so Windows-absolute strings and lexical escapes are rejected exactly as | ||
| # before, instead of being split into a leaf name under the workspace root. | ||
| self._workspace_path_policy().normalize_path(path, for_write=True) |
There was a problem hiding this comment.
When Manifest.root is a symlink such as /tmp/ws-link -> /tmp/ws, an absolute resolved path such as /tmp/ws/file is accepted by UnixLocalSandboxSession.normalize_path() and can be returned as FileEntry.path by ls(). This new non-resolving precheck instead compares it with the lexical /tmp/ws-link root and raises InvalidManifestPathError, so callers can no longer pass that listed path back to rm() even though the old implementation accepted it. Preserve the canonical resolved-root behavior while separately rejecting invalid raw syntax.
AGENTS.md reference: AGENTS.md:L95-L95
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed together with the comment above: the lexical precheck is gone, and validation now goes through the policy's resolving branch, which compares against the resolved root. test_rm_accepts_paths_resolved_through_a_symlinked_root sets Manifest.root to ws-link -> ws and removes entries both by their resolved path and by their root-alias path; it fails on the previous revision and passes now.
Add `follow_leaf_symlink` to WorkspacePathPolicy.normalize_path(): with resolve_symlinks=True it resolves the parent directories and keeps the final component as the entry itself, then applies the workspace root, the longest matching extra grant and the read-only check to that location. UnixLocalSandboxSession._rm_target_path() now uses it instead of a lexical precheck plus a hand-built parent/leaf split, which missed a more specific read-only grant under a writable one and rejected absolute paths resolved through a symlinked Manifest.root. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BN4v25msJgrjNgac97g1Az
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
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?
|
Thanks for looking at this. I agree there is a rename race between authorizing the parent and the Closing that class properly means pinning the validated directory (open with 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. |
2 participants
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), andrm()operated on that resolved path. Against a realUnixLocalSandboxClientsession onmain(89c02c8), after the agent ranln -s plain.txt linkfile; ln -s realdir linkdir; ln -s /etc/hostname outside_file; ln -s /nonexistent danglingin the workspace:mainrm("linkfile")plain.txt;linkfileleft danglingrm("linkdir", recursive=True)shutil.rmtreeonrealdir/(all contents gone);linkdirleft danglingrm("dangling")InvalidManifestPathError: manifest path must not escape root— cannot be removedrm("outside_file")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()runsrm -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-scopedrm(..., 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.rm("escape_dir/victim.txt")whereescape_dir -> /outside) are still rejected withInvalidManifestPathError.Behavior change
rmon a symlink inside a UnixLocal workspace now removes only the link, matching POSIXrm,rm -rf, and every other sandbox backend. No public signatures change.Evidence
tests/sandbox/test_unix_local.py::TestUnixLocalRmSymlinkscover file symlink, directory symlink (recursive and not), symlink pointing outside the workspace, dangling symlink, the escaping-parent rejection, and the user-scoped path. Six of the seven fail onmainand pass with this change.tests/sandbox(1453 passed), the full parallel suite (9261 passed; twotests/mcp/test_mcp_pagination_integration.pystdio tests failed under a memory-constrained 3-worker run and pass when rerun alone), serial tests,ruff format --check,ruff check, andmypy/pyrighton the changed files all pass locally on Linux / Python 3.10.🤖 Generated with Claude Code
https://claude.ai/code/session_01BN4v25msJgrjNgac97g1Az