Enforce symlink containment for reads and internal links - #90
Merged
Conversation
Fixes #86, fixes #88. SafeReadFile now takes the skill root and, in addition to the existing leaf Lstat check, verifies that the fully resolved path stays inside the resolved root. This closes the bypass where a symlinked directory (for example references/ pointing outside the skill tree) exposed regular files that passed the leaf-only check and were read and shipped to the LLM judge. The signature change is safe: SafeReadFile was introduced after v1.5.5 and has not appeared in a release. Internal link validation previously checked containment syntactically and then called os.Stat, which follows symlinks, so a link target inside the skill directory could resolve outside it. CheckInternalLinks now resolves the target and reports "internal link escapes skill directory" when the resolved path leaves the skill package. Symlinks that resolve within the package still pass link validation.
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
1 participant
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.
What this PR does
Fixes #86 and #88 with a shared mechanism: resolution-based containment. The skill root and each candidate path are resolved with
filepath.EvalSymlinks, and the resolved path must remain inside the resolved root.util.SafeReadFilenow takes the skill root and adds a resolved-path containment check on top of the existing leafLstatcheck. A symlinked directory (e.g.references/pointing outside the skill tree) no longer exposes out-of-tree regular files to reads, so that content cannot be shipped to the LLM judge. The signature change is safe:SafeReadFilewas introduced after v1.5.5 and has not appeared in any release.CheckInternalLinkspreviously verified containment syntactically and then calledos.Stat, which follows symlinks. It now resolves the target and reportsinternal link escapes skill directory: <link> (resolves outside the skill package)as an error. Symlinks that resolve within the package still pass link validation.File symlinks remain unread everywhere (the #78 rule is unchanged); this PR only tightens.
How to test
Verified end-to-end with a hostile fixture (a skill whose
references/is a symlink to an outside directory and whose SKILL.md links to a file within it): the link is reported as escaping and the read is refused. Also:go test -race ./... -count=1util), escaping and in-tree symlink link targets (structure), symlinkedreferences/never reaching judge inputs (skillcheck)Checklist
go test -race ./... -count=1)golangci-lint run)