Repository navigation
fix(skills): make the read confinement real — a symlink could leave the directory - #81
Merged
Merged
Conversation
…he directory The read tool's confinement was a string check: `resolve()` is lexical and never consults the filesystem. A symlink sitting in a skill directory is inside by name and points wherever it likes, and `readFileSync` follows it — so `…/skills/<anything>/leak -> /etc/passwd` read straight through the guarantee the docs make. `listSkillFiles` had the same hole twice over: `statSync` follows links, so a symlinked *directory* let the listing walk out of the tree and advertise files it had no business seeing. A skill you wrote yourself never hits this. A skill you copied — a vendored pack, a submodule, the untrusted case the catalogue is about — is exactly where links appear, which is why this comes before the trust work. Now: - `resolveInside(root, path)` realpaths the target, requires it to stay inside the *real* skill directory, and requires a regular file (a directory, socket or device is refused rather than read or hung on). - `Skill.dir` is the realpath of the skill directory, so the checks and the values they protect are compared against the same resolved root. - discovery refuses a `SKILL.md` that is not a regular file inside the skill, and *reports* it: a symlinked `SKILL.md` is not an accident. - `listSkillFiles` uses `lstat` and skips symlinks, so a link is neither listed nor walked to. Tests cover the escapes — link out, symlinked `SKILL.md`, symlinked directory, a directory passed as a file — and pin the one case that stays legal (a link that resolves inside the skill). Four of the five fail against the previous loader.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
The read tool's confinement was a string check —
resolve()is lexical and never touches the filesystem:A symlink inside the skill directory is inside by name and points wherever it likes, and
readFileSyncfollows it. Soskills/<name>/leak -> /etc/passwdread straight through a guarantee the README,AGENTS.mdand the archetype catalogue all state.listSkillFileshad the same hole twice over:statSyncfollows links, so a symlinked directory let the listing walk out of the tree and advertise files it should never have seen.A skill you wrote never hits this. A skill you copied — a vendored pack, a submodule, exactly the untrusted case the catalogue discusses — is where links actually appear, which is why this goes first.
The fix
resolveInside(root, path)realpaths the target, requires it inside the real skill directory, and requires a regular file (a directory, socket or device is refused rather than read or hung on).Skill.diris now the realpath of the skill directory, so the checks and the values they protect compare against the same resolved root.SKILL.mdthat is not a regular file inside the skill, and reports it — a symlinkedSKILL.mdis not an accident.listSkillFilesuseslstatand skips symlinks: neither listed nor recursed through.Docs that made the claim now say how it is enforced.
Tests
Five new cases: link out of the skill, symlinked
SKILL.md, symlinked directory, a directory passed asfile, and the one case that stays legal (a link resolving inside the skill). Four of the five fail against the previous loader — verified by stashing onlysrc/skills.tsand re-running.pnpm build/typecheck/lintgreen;pnpm test346 passed / 5 skipped; coverage 90.5 / 84.1 / 89.6 / 90.5.