fix(skills,pairing): traversal guard + hash symmetry + list_pending lock (#6656)#30715
Merged
Conversation
…ding, hash file paths - skills_hub: validate that uninstall_skill's install_path resolves inside SKILLS_DIR before calling shutil.rmtree, preventing recursive deletion of arbitrary directories via poisoned lock.json entries - skills_hub: include file paths (not just contents) in bundle_content_hash so swapping filenames between files changes the hash, strengthening update-detection integrity - pairing: wrap list_pending() in self._lock so _cleanup_expired() file writes don't race with concurrent generate_code()/approve_code() calls Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
… bundle_content_hash) PR #6656 added rel_path + \x00 prefixing to ``bundle_content_hash`` so a filename swap between two files in a bundle changes the digest. But it only patched the in-memory side — ``content_hash`` in ``tools/skills_guard.py`` (the on-disk equivalent) still hashed file contents only. These two functions need to stay symmetric: ``check_for_skill_updates`` compares the disk hash of an installed skill against the bundle hash of the upstream copy. With the asymmetric fix, every clean install showed as drifted because the digests no longer matched (2 existing tests in ``test_skills_hub.py`` started failing as soon as the contributor's change landed). Apply the same ``rel_path + \x00 + content`` shape to the disk-side function. Both functions now produce the same digest for the same skill content laid out two ways. Documented the symmetry invariant in the docstring so a future change to either function knows to touch both. Also adds tests/tools/test_pr_6656_regressions.py with 10 regression tests covering all three fixes salvaged in PR #6656: - uninstall_skill path traversal (4 cases: parent segments, absolute paths, symlink escape, legitimate skill) - bundle_content_hash filename swap detection (4 cases: in-memory swap, identity, disk-side swap, bundle↔disk symmetry) - list_pending lock contract (2 cases: source-grep contract, smoke) Also fixes AUTHOR_MAP entry for @aaronlab — their commit email (1115117931@qq.com) maps to "aaronagent" which isn't a real GitHub login, so changelog @mentions would 404.
Contributor
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-import |
1 |
First entries
tests/tools/test_pr_6656_regressions.py:27: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
✅ Fixed issues: none
Unchanged: 4770 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
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
3 participants
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
Salvages @aaronlab's PR #6656 — three independent fixes bundled together. All three are real, well-targeted, and small. Salvaged onto current
mainwith the cherry-pick + a follow-up commit that completes a half-finished symmetry fix and adds the regression tests the original PR was missing.What it fixes
Fix 1 — Skill uninstall path traversal (highest impact)
entry["install_path"]comes from~/.hermes/skills/.hub/lock.json. If a malicious skill ever gets installed (orlock.jsonis otherwise corrupted), an entry likeinstall_path: "../../"makesSKILLS_DIR / "../../"resolve to a parent dir, andshutil.rmtreerecursively obliterates it. Now guarded withPath.resolve().is_relative_to(SKILLS_DIR.resolve())— catches../, absolute paths, and symlink-escape via realpath normalisation.Fix 2 — Bundle / disk content hash filename inclusion
bundle_content_hash()previously mixed file CONTENTS only — swappingSKILL.md↔scripts/run.shcontents produced an identical digest. The contributor patched this in the bundle hash but missed the on-disk equivalentcontent_hash()intools/skills_guard.py. The two functions need to stay symmetric (one operates on a bundle, one on the disk layout, andcheck_for_skill_updatescompares them). Without the symmetric fix, 2 existing tests intest_skills_hub.pystarted failing because every clean install reported as drifted. Follow-up commit fixes the disk side and documents the symmetry invariant.Fix 3 —
PairingStore.list_pendingTOCTOU locklist_pending()called_cleanup_expired()(which writes the JSON file) without holdingself._lock, racing with concurrentgenerate_code/approve_codewhich DO hold it. PR was filed before our hash-pairing-codes work; conflict resolved by keeping main's hashed-entry schema and applying the contributor's lock wrap.Changes
tools/skills_hub.py(+9 -2, @aaronlab): realpath guard inuninstall_skill, filename prefix inbundle_content_hash.gateway/pairing.py(+1 indented, @aaronlab):with self._lock:wrap onlist_pending.tools/skills_guard.py(+13 -1, follow-up): makecontent_hashfilename-sensitive to stay symmetric withbundle_content_hash. Symmetry invariant documented in docstring.tests/tools/test_pr_6656_regressions.py(+287, follow-up): 10 new regression tests covering all three fixes (parent-segment traversal, absolute paths, symlink escape, legitimate uninstall, in-memory filename swap, identity, disk-side filename swap, bundle↔disk symmetry, list_pending lock source-grep contract, smoke).scripts/release.py(+1 -1, follow-up): AUTHOR_MAP correction —1115117931@qq.commapped toaaronagent(not a real GitHub login) → fixed toaaronlab.Validation
uninstall_skillon lock entryinstall_path="../../"list_pendingracing withapprove_codeself._locktests/tools/test_pr_6656_regressions.py— 10/10 new tests pass.Full skill + pairing surface (8 test files, 287 tests) — all green, no regressions.
Sibling-site audit
Checked other
shutil.rmtreecall sites inskills_hub.py. The two not patched by this PR (quarantine_bundleandinstall_from_quarantine) construct paths from_validate_skill_name/_validate_bundle_rel_pathoutputs which are already safe. The lock.json-derivedinstall_pathinuninstall_skillwas the only unguarded site.Attribution
@aaronlab did all three fixes in one commit. Cherry-picked with original authorship (date 2026-04-09 preserved). The follow-up commit (completing the hash symmetry + tests + AUTHOR_MAP fix) is attributed to Teknium. Rebase-merge keeps both visible in
git log.Infographic