fix(sandbox): let UnixLocal ls list a regular file like the other backends - #4833
fix(sandbox): let UnixLocal ls list a regular file like the other backends#4833coderdailyone wants to merge 5 commits into
Conversation
…kends
BaseSandboxSession.ls() runs `ls -la -- <path>`, so listing a file
returns that file's single entry. UnixLocalSandboxSession.ls() called
os.scandir() on the path unconditionally and failed with
ExecNonZeroError("[Errno 20] Not a directory") for the same call.
Return the entry itself when the validated path is not a directory,
and build FileEntry values from stat results in one helper for both
the single-entry and the directory case. Missing paths still fail.
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: addf8a475e
ℹ️ 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".
| if not normalized.is_dir(): | ||
| # `ls -la <file>` lists the file itself; the exec-backed sessions return that | ||
| # single entry, so do the same instead of failing with ENOTDIR. | ||
| return [_unix_file_entry(str(normalized), normalized.lstat())] |
There was a problem hiding this comment.
When path is a Unix-domain socket or block device, this new branch also returns a single entry because it accepts every non-directory, but _unix_file_entry() passes the mode to Permissions.from_mode(), whose bitmask check marks those file types as permissions.directory=True even though their kind is OTHER. This produces contradictory caller-visible metadata and differs from the exec-backed parser; restrict this branch to regular files or determine the directory flag with stat.S_ISDIR().
AGENTS.md reference: AGENTS.md:L163-L165
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — the root cause is in Permissions.from_mode() itself: bool(mode & stat.S_IFDIR) is also true for S_IFSOCK (0o140000) and S_IFBLK (0o060000), so the existing directory-listing branch on main already reported sockets and block devices as directory=True. Switched it to stat.S_ISDIR(mode). Added a parametrized test_permissions_from_mode_uses_the_file_type_not_a_bit_mask (socket and block cases fail on the previous revision) and test_ls_of_a_socket_is_not_reported_as_a_directory, which binds a real Unix socket in the workspace and asserts kind=OTHER, directory=False.
| if not normalized.is_dir(): | ||
| # `ls -la <file>` lists the file itself; the exec-backed sessions return that | ||
| # single entry, so do the same instead of failing with ENOTDIR. | ||
| return [_unix_file_entry(str(normalized), normalized.lstat())] |
There was a problem hiding this comment.
When a workspace path is a safe symlink to a regular file, normalize_path() resolves the leaf before this branch runs, so the new successful result reports the target's absolute path and EntryKind.FILE rather than the requested link and EntryKind.SYMLINK. Exec-backed sessions deliberately preserve safe leaf symlinks after validating their resolved targets, so callers now receive backend-dependent path identity and kind; retain the requested leaf for lstat() and result construction while still validating its resolved target.
AGENTS.md reference: AGENTS.md:L147-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done. The path is still validated through normalize_path() (resolving the leaf, so an escaping link is rejected as before), but the returned entry is now built from the requested leaf: parents resolved, final component kept, lstat() on that. ls("link") for a link to a regular file returns EntryKind.SYMLINK with the link's own path, matching what ls -la link prints on the exec-backed sessions. A link whose target is a directory keeps the existing behavior of listing the target's entries, since I did not want to change that surface in this PR. Regression test: test_ls_of_a_file_symlink_returns_the_link_itself.
…x from_mode `ls -la <link>` prints the link, not its target, so return the requested leaf entry (validated through its resolved target as before) instead of the resolved file. Permissions.from_mode() decided `directory` with `mode & S_IFDIR`, which is also set for sockets and block devices; use stat.S_ISDIR() so special files are not reported as directories. 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: 52ab1c3d3a
ℹ️ 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".
| requested = self._requested_leaf_path(path, normalized) | ||
| return [_unix_file_entry(str(requested), requested.lstat())] |
There was a problem hiding this comment.
When ls() receives a safe file symlink together with user, the early super().ls() path still calls the overridden normalize_path(), resolves the leaf, and reports the target as EntryKind.FILE; without user, these new lines report the requested EntryKind.SYMLINK. Fresh evidence beyond the earlier symlink comment is this remaining user-scoped path, which makes the caller-visible result depend on whether user is supplied; preserve the requested leaf for that supported branch as well.
AGENTS.md reference: AGENTS.md:L147-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. BaseSandboxSession.ls() now takes the listed path from a small _validate_listing_path() hook (default: _validate_path_access()), and UnixLocalSandboxSession overrides it to return the requested leaf after validating through the resolved target. So ls("link", user=...) runs ls -la -- <workspace>/link and parse_ls_la reports the symlink, the same as the direct branch. test_ls_as_user_lists_the_link_itself asserts the exec'd command targets the link path.
| raw_path = Path(path) | ||
| if raw_path.name in ("", ".", ".."): | ||
| return normalized | ||
| return self.normalize_path(raw_path.parent) / raw_path.name |
There was a problem hiding this comment.
When path is a regular file covered by an exact SandboxPathGrant outside the workspace, the initial normalize_path(path) succeeds, but this second normalization checks the file's parent, which is not covered by that exact grant, and raises InvalidManifestPathError. Consequently the new regular-file listing still fails for a supported SDK file API path; reconstruct the requested leaf without requiring authority over its parent after the full path has already been validated.
AGENTS.md reference: AGENTS.md:L147-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right — validating the parent was more authority than the leaf needs. _requested_leaf_path() no longer calls normalize_path() on the parent; after the full path has been validated it only rebuilds where the entry lives (absolute.parent.resolve() / name) for lstat() and the reported path. test_ls_of_a_file_covered_by_an_exact_grant lists a regular file that is covered only by an exact SandboxPathGrant outside the workspace; it fails on the previous revision and passes now.
…le grants Route the path that `ls` lists through a `_validate_listing_path()` hook so UnixLocal's user-scoped branch runs `ls -la` on the requested leaf (the link, not its target) like the direct branch does. Rebuild that leaf from the resolved parent without re-validating the parent, so a regular file covered by an exact SandboxPathGrant outside the workspace can still be listed after the full path was validated. 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: a136e76ede
ℹ️ 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".
| absolute = raw_path if raw_path.is_absolute() else Path(self.state.manifest.root) / raw_path | ||
| return absolute.parent.resolve(strict=False) / absolute.name |
There was a problem hiding this comment.
When a relative input contains a symlinked parent followed by .. (for example, link/../secret), normalize_path() lexically collapses it to the safe workspace path before validation, but this new absolute.parent.resolve() follows link first and can rebuild an ungranted /outside/secret path. The no-user branch then calls lstat() on that host path, while the user branch passes it to ls, allowing metadata or directory entries outside the workspace and configured grants to be exposed; rebuild the leaf from the already canonical lexical path instead of resolving the original parent again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed — thank you, that was a real hole. _requested_leaf_path() now starts from the policy's lexical canonical path (WorkspacePathPolicy.normalize_path() without symlink resolution, which collapses link/../secret to <root>/secret exactly as the validating call did) and only then resolves the parents; link is never re-followed. If the lexical form is not under the configured root (a path already resolved through a symlinked Manifest.root), it falls back to the validated resolved path. test_ls_never_rebuilds_the_leaf_through_a_symlinked_parent sets up link -> /outside with /outside/secret, asserts ls("link/../secret") fails while <root>/secret is absent and reports <root>/secret once it exists; it fails on the previous revision.
| normalized = self.normalize_path(path) | ||
| command = ("ls", "-la", "--", str(normalized)) | ||
| try: | ||
| if not normalized.is_dir(): |
There was a problem hiding this comment.
When path is a symlink to a directory and user is omitted, testing the resolved normalized path here enters scandir() and returns the target directory's children, whereas the newly changed user-scoped path sends the preserved leaf to ls -la and returns one SYMLINK entry. GNU ls --help identifies -H as the option that follows command-line symlinks, and this command uses only -la, so normal results now depend on whether user is supplied; inspect the requested leaf with lstat() before deciding to scan.
AGENTS.md reference: AGENTS.md:L148-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed; with the user-scoped branch now preserving the leaf, the two branches had drifted. The direct branch now lstat()s the requested leaf first and only scans when that entry is itself a directory, so a symlink to a directory is reported as the single SYMLINK entry in both branches, matching ls -la <path> without -H. This is a behavior change from main, where the direct branch listed the target's contents; I've noted it in the PR description. test_ls_of_a_directory_symlink_returns_the_link_itself covers it.
…t first Rebuild the requested leaf from the policy's lexical canonical path so `link/../secret` names `<root>/secret`, as validation saw it, instead of re-following `link` and stepping outside the workspace. Classify the leaf with lstat() before scanning: a directory lists its entries and anything else, including a symlink to a directory, is the single entry itself, matching `ls -la <path>` on the exec-backed sessions for both the direct and the user-scoped branch. 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: be3bb30f56
ℹ️ 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".
| return normalized | ||
| if not canonical.name: | ||
| return normalized | ||
| return canonical.parent.resolve(strict=False) / canonical.name |
There was a problem hiding this comment.
When a workspace parent symlink points to an ungranted directory whose leaf is itself a symlink back into the workspace (for example, jump/back -> /workspace/inside), the initial normalize_path() succeeds because the fully resolved target is confined, but this line reconstructs /outside/back; the subsequent lstat() or user-scoped ls therefore reads metadata from a host path not covered by the workspace or grants. Fresh evidence beyond the prior link/../secret report is that this bypass requires neither .. nor a race, and the current revision still resolves the parent after validation. Revalidate the reconstructed leaf location without following the final symlink, or derive it through a confined parent before accessing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — rebuilding the leaf by hand kept leaving gaps, so I replaced it with a policy-level check. WorkspacePathPolicy.normalize_path() gained follow_leaf_symlink=False (with resolve_symlinks=True it resolves the parents, keeps the final component, and applies the workspace root / longest grant / read-only rules to that location; the same option #4830 uses for rm). ls() now validates the resolved target as before and then validates the leaf's own location through that option, so jump/back (entry living in an ungranted directory, target inside the workspace) raises InvalidManifestPathError instead of lstat()-ing /outside/back. Regression test: test_ls_rejects_a_leaf_that_lives_outside_the_workspace.
| not demand authority over the parent, which an exact-file grant does not confer. | ||
| """ | ||
| try: | ||
| canonical = self._workspace_path_policy().normalize_path(path) |
There was a problem hiding this comment.
When path is a raw backslash string such as r"link\secret" or a relative PureWindowsPath, the initial self.normalize_path(path) uses native Path semantics, so Unix validates /workspace/link\secret as one literal component, while this second normalization converts the backslash to a POSIX separator and produces /workspace/link/secret. If link is a symlink to an ungranted directory, the subsequent parent resolution accesses /outside/secret, which was never validated; even without a symlink, ls() can return a different file from the one initially checked. Canonicalize the input to one POSIX identity before both symlink validation and leaf reconstruction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the input is coerced once with coerce_posix_path() and that single POSIX identity feeds both the target validation and the leaf-location validation, so sub\file.txt names sub/file.txt in both steps and link\secret (link to an ungranted directory) is rejected rather than resolved through the link. Regression test: test_ls_treats_backslashes_as_separators_for_both_checks.
| except InvalidManifestPathError: | ||
| # The lexical form is not under the configured root (e.g. a path already resolved | ||
| # through a symlinked Manifest.root); the resolved path is the best we have. | ||
| return normalized |
There was a problem hiding this comment.
When Manifest.root is a symlink alias and the caller uses the supported absolute resolved-root path to a safe leaf symlink, the initial normalization resolves the leaf to its target, while the lexical normalization rejects the path because it is not beneath the literal configured alias. This fallback then returns the resolved target, so both direct and user-scoped ls() report the target as FILE instead of returning the requested SYMLINK. Fresh evidence beyond the earlier leaf-symlink comments is the explicitly supported resolved-root addressing case; reconstruct the leaf relative to the resolved workspace root rather than discarding its identity here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed as part of the same change: the leaf location is now validated through the policy's resolving branch (which compares against the resolved root) rather than the lexical branch, so the fallback that discarded the leaf's identity is gone. ls(<resolved-root>/link) under a symlinked Manifest.root returns EntryKind.SYMLINK with the requested path in both the direct and the user-scoped branch. Regression test: test_ls_keeps_a_leaf_symlink_addressed_through_the_resolved_root.
…h policy Give WorkspacePathPolicy.normalize_path() a follow_leaf_symlink option: with resolve_symlinks=True it resolves the parent directories, keeps the final component, and applies the workspace root, the longest matching grant and the read-only check to that location. UnixLocal ls() now coerces the input to one POSIX identity, validates the resolved target as before, and then validates the leaf's own location with that option instead of rebuilding it by hand. A leaf that lives in an ungranted directory (jump/back -> workspace) is rejected, backslashes mean the same thing in both checks, and a leaf symlink addressed through the resolved root keeps its symlink identity. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HCNKceEs9sPdb6aHK3FqPf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14b81582dd
ℹ️ 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".
| if follow_leaf_symlink or absolute_path.name in ("", ".", ".."): | ||
| resolved = absolute_path.resolve(strict=False) | ||
| else: | ||
| resolved = absolute_path.parent.resolve(strict=False) / absolute_path.name |
There was a problem hiding this comment.
When Manifest.root is a symlink and the caller lists the normal root path "." (or the configured root alias itself), absolute_path becomes that alias and this line preserves it while only resolving its parent. The subsequent containment check compares the alias against the resolved workspace target and raises InvalidManifestPathError, so the workspace root cannot be listed. Fresh evidence beyond the earlier resolved-root report is the root path itself—the added test covers only a child addressed through the resolved root. Treat the configured root alias as authorized when the requested leaf is the root, while preserving leaf symlinks below it.
Useful? React with 👍 / 👎.
1 participant
This pull request fixes
UnixLocalSandboxSession.ls()so that listing a regular file returns that file's entry, matching the exec-backed session implementation.Bug
BaseSandboxSession.ls()(every exec-backed backend) runsls -la -- <path>and parses the output, sols("plain.txt")returns a singleFileEntryfor the file.UnixLocalSandboxSession.ls()calledos.scandir()on the validated path unconditionally, so the same call failed:Reproduced on
mainwith a realUnixLocalSandboxClientsession; the same request against the base implementation (verified by feeding realls -la <file>output throughparse_ls_la) yields[('file', '/…/plain.txt')].Fix
lstat()result instead of callingos.scandir().FileEntryvalues fromstat_resultin a small helper (_unix_file_entry) used by both the single-entry and the directory branch, deriving the kind viastat.S_ISLNK/S_ISDIR/S_ISREG; directory listings are unchanged.ExecNonZeroErrorfrom theOSErrorpath as before.Behavior changes
ls("<regular file>")returns that file's entry instead of raisingExecNonZeroError(the fix).ls("<symlink>")reports the symlink itself (EntryKind.SYMLINK, the link's own path), including a symlink to a directory, which previously listed the target directory's contents in the direct branch. This matchesls -la <path>on the exec-backed sessions and is now identical with and withoutuser=.Permissions.from_mode()reportsdirectory=Falsefor sockets and block devices (it usedmode & S_IFDIR, which those file types also set).Evidence
tests/sandbox/test_unix_local.py::TestUnixLocalLscovers the regular-file case (fails onmain), a directory listing with file / directory / symlink kinds, and a missing path.tests/sandbox/test_unix_local.py,ruff,mypy, andpyrighton the changed files pass locally (Linux, Python 3.10).🤖 Generated with Claude Code
https://claude.ai/code/session_01BN4v25msJgrjNgac97g1Az