fix(built-in-tools): resolve symlink dir containment before probing it#387
Conversation
_explicit_descendable stat'd an outbound symlink's target via is_dir(follow_symlinks=True) before the workspace-containment check. Resolve the path first, reject anything outside the workspace, and only os.path.isdir() an in-workspace target -- so an outbound symlink is never stat-followed past the boundary during enumeration. Behavior is unchanged (verified: 13 sandbox tests pass, 1300+ fuzz trials match Path.glob).
pjdoland
left a comment
There was a problem hiding this comment.
Thanks for this. The core change reads cleanly, and I really like that the safe walker enforces confinement during enumeration rather than only at the per-match gate. The docstrings on _glob_pattern_escapes and _explicit_descendable make the intent easy to follow.
One thing I would like to raise before approving. The fix hardens search_files, but the sibling list_files tool in the same file still enumerates through list_dir.glob(...) with only the root sandboxed via _get_safe_path. Its matches are never routed back through the workspace boundary, so the same enumeration leak this PR closes for search_files still applies there. For example, list_files(pattern="../*"), or a link -> /outside symlink with list_files(pattern="link/*"), will list [file] ../secret.txt and outside-directory contents, disclosing the names and existence of host paths outside the workspace (contents are not exposed, since list_files only lists).
I checked the rest of this stack (388 through 391) and none of them touch this file, so it does not look like list_files gets picked up later. Could we extend the same _iter_pattern_matches / boundary treatment to list_files in a revision? If you would rather keep this PR tightly scoped, splitting it into a fast follow-up works just as well. Happy to help however is easiest.
Everything else looks good to me. Thanks again!
_explicit_descendable stat'd an outbound symlink's target via
is_dir(follow_symlinks=True) before the workspace-containment check.
Resolve the path first, reject anything outside the workspace, and only
os.path.isdir() an in-workspace target -- so an outbound symlink is never
stat-followed past the boundary during enumeration. Behavior is unchanged
(verified: 13 sandbox tests pass, 1300+ fuzz trials match Path.glob).