Repository navigation
fix(skills): skip symlinked resources when loading a skill from disk - #7601
behnam1990adel-hash wants to merge 2 commits into
Conversation
_load_dir walked a skill's references/, assets/, and scripts/
directories with pathlib.Path.rglob("*") and, for every entry where
is_file() was true, read its content with read_text()/read_bytes().
Both calls follow symbolic links transparently, so a symlink
committed inside one of those directories, pointing at a file
anywhere else on disk, was read into the skill's resources under the
symlink's own name, indistinguishable from an ordinary file once
loaded. LoadSkillResourceTool then served that content back verbatim,
since it is a plain dict lookup against what _load_dir already
populated, with no further check at request time.
Skip a path that is itself a symlink before is_file() or either read
call runs. rglob("*") does not recurse into a symlinked directory (a
pathlib default), so this closes the only way a symlink inside these
directories could otherwise leak content.
Added test__load_skill_from_dir_skips_symlinked_resources, which
reproduces the previous behavior on a real on-disk filesystem and
confirms an ordinary (non-symlink) resource is still loaded normally.
Somay-kousis
left a comment
Somay-kousis
left a comment
There was a problem hiding this comment.
test__load_skill_from_dir_skips_symlinked_resources fails on the base (assert 'top-secret' is None) and passes on the head. tests/unittests/skills: 105 passed on the head.
The check covers symlinks inside references/, assets/ and scripts/, but not those directories being symlinks themselves. _load_dir(skill_dir / "assets") gets the symlink as directory, rglob("*") walks the target, and none of the yielded entries is a symlink, so everything outside the skill is loaded. That goes against the new comment ("keeps a skill from reading a file that lives outside its own directory").
Probe on this branch, Python 3.12.13, with outside/secret.txt next to the skill:
file symlink in assets/ assets={} references={} scripts=[]
dir symlink inside assets/ assets={} references={} scripts=[]
assets/ itself is a symlink assets={'secret.txt': 'top-secret'} references={} scripts=[]
references/ itself is a symlink assets={} references={'secret.txt': 'top-secret'} scripts=[]
scripts/ itself is a symlink assets={} references={} scripts=['run.py', 'secret.txt']
Returning early when directory.is_symlink() at the top of _load_dir closes these and keeps a skill whose own directory is a symlink loading normally (tried on your branch, then reverted):
assets/ itself is a symlink assets={} references={} scripts=[]
references/ itself is a symlink assets={} references={} scripts=[]
scripts/ itself is a symlink assets={} references={} scripts=[]
symlinked skill dir assets= {'plain.txt': 'plain'} references= {'ref.md': 'ref'}
tests/unittests/skills still 105 passed with that. Parametrizing the new test over the file-in-dir and dir-itself shapes would cover both.
Probe script: probe_7601.py (outputs probe_7601_head.out, probe_7601_base.out, probe_7601_suggested.out).
Somay-kousis pointed out the first fix only covered a symlink found
while walking inside references/, assets/, or scripts/ -- not one of
those directories being a symlink itself. _load_dir calls
directory.rglob("*") directly on the path it's given, and rglob only
skips recursing into a symlinked directory it *encounters during the
walk*; it has no such guard for the root it's walking from. So a
skill shipping e.g. assets/ as a symlink to an external directory had
every file under that external directory read in, recursively --
worse than the single-file leak the first fix closed.
Return early, before is_dir() is even checked, when the directory
passed to _load_dir is itself a symlink. Covered by two new tests, one
for a symlinked directory nested inside assets/ and one for assets/
itself being a symlink; both pass with this change and fail without
it.
|
Good catch — thanks for digging into this and testing the alternative. Pushed a second commit (453ef92) that returns early in |
3 participants
_load_dirwalks a skill'sreferences/,assets/, andscripts/directories withpathlib.Path.rglob("*")and, for every entry whereis_file()is true, reads its content withread_text()/read_bytes(). Both calls follow symbolic links transparently, so a symlink committed inside one of those directories, pointing at a file anywhere else on disk, is read into the skill's resources under the symlink's own name — indistinguishable from an ordinary file once loaded.LoadSkillResourceToolthen serves that content back verbatim, since it is a plain dict lookup against what_load_diralready populated, with no further check at request time — confirmed end-to-end through the realload_skill_resourcetool.This skips a path that is itself a symlink before
is_file()or either read call runs.rglob("*")does not recurse into a symlinked directory (a pathlib default), so this closes the only way a symlink inside these directories could otherwise leak content.Added
test__load_skill_from_dir_skips_symlinked_resources, which reproduces the previous behavior on a real on-disk filesystem and confirms an ordinary (non-symlink) resource is still loaded normally. The existing test suite undertests/unittests/skills/passes unchanged.