Skip to content

fix(built-in-tools): resolve symlink dir containment before probing it#387

Open
herikwebb wants to merge 1 commit into
plmbr:mainfrom
herikwebb:promote/security/fix-search-files-symlink-workspace-20260704140058
Open

fix(built-in-tools): resolve symlink dir containment before probing it#387
herikwebb wants to merge 1 commit into
plmbr:mainfrom
herikwebb:promote/security/fix-search-files-symlink-workspace-20260704140058

Conversation

@herikwebb

Copy link
Copy Markdown
Contributor

_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).

_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 pjdoland added the bug Something isn't working label Jul 9, 2026

@pjdoland pjdoland left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants