security: confine skills-sync destinations to the cloned repo root - #116
Merged
Conversation
sync-skills.sh resolved a consumer repo's .agents symlink and each managed-skill destination with realpath -m but never checked the result stayed inside the temporary clone. An untrusted consumer repo could plant a .agents symlink (or a pre-existing .agents/skills/<skill> symlink) pointing outside the clone and turn the sync's mkdir, rsync --delete, or marker write into an escape from the clone root. Add skills_sync_require_contained/skills_sync_sync_repo in scripts/lib/skills-sync-containment.sh, shared by sync-skills.sh and its regression tests, which fails closed before any mutation when the resolved skills root or a skill's destination is not strictly contained in the clone. In-repo layouts such as .agents -> docs/agents still resolve and sync normally. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Precompute and validate every selected managed-skill destination (and the skills root) in a validation-only pass before entering the mkdir/rsync/marker-write loop, so an escaping destination later in the skill list can no longer be caught after an earlier valid destination was already mutated. Addresses review blocker defect:scripts/lib/skills-sync-containment.sh:validate-all-before-mutation
skills_sync_require_contained accepted path_real == root_real, so an in-clone `.agents/skills -> ..` symlink could resolve exactly to the clone root and slip past the containment check, letting sync write skill contents and markers directly into the clone root instead of the managed .agents/skills subtree. Require strict containment (root_real/* only) for both the .agents/skills root and each skill destination, and add a regression covering the root-equality symlink case.
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.
Summary
scripts/sync-skills.shresolved a freshly cloned consumer repo's.agents/skillsroot and each managed-skill destination withrealpath -mbut never checked the result stayed inside the temporary clone. A consumer repo (untrusted input to the sync) that planted a.agentssymlink — or a pre-existing.agents/skills/<skill>symlink — pointing outside the clone could turn the sync'smkdir,rsync --delete, or marker write into an escape from the clone root.skills_sync_require_contained/skills_sync_sync_repoinscripts/lib/skills-sync-containment.sh, shared bysync-skills.shand its regression tests.skills_sync_sync_repofails closed — before anymkdir,rsync --delete, or marker write — when the resolved.agents/skillsroot or a skill's destination is not strictly contained in the clone root.sync-skills.shnow sources the library and delegates the per-skill mkdir/rsync/marker work toskills_sync_sync_repo, aborting (and markingfail=1) that repo's sync on a containment violation instead of mutating outside the clone..agents -> docs/agentsstill resolve and sync normally, since the resolved path stays under the clone root.Test plan
test/sync-skills-containment.test.mjs(new): proves an escaping.agentssymlink and an escaping existing managed-skill destination symlink are rejected without creating/deleting anything under an outside sentinel directory, while a normal.agentsdirectory and an in-repo.agents -> docs/agentssymlink still sync and write the marker.npm run check(root) passes —check:docs,check:organization, andnpm test(15 tests, including the 4 new containment tests) all green.🤖 Generated with Claude Code