Conversation
…e distributed files from user additions Follow-up per review of NousResearch#74409. Two real gaps in the original allowlist enforcement: 1. The root-name comparison (checking staged.iterdir() entry names against owned_paths()) could only ever match root-level entries, so documented nested forms like skills/research/ and cron/digest.json were silently skipped -- their source content never reached the target at all. Rewrote to resolve each owned_paths() entry directly as a manifest-relative path against staged/target, supporting any depth. 2. dirs_exist_ok=True (needed to fix a separate bug this rewrite introduced along the way -- see below) retains files from an older distribution revision that were removed from a newer one, which contradicts the documented "replaced from the new clone" contract and doesn't distinguish that from a genuine target-only user file. Added a new manifest field, installed_files, tracking every file path this function writes; on the next update it diffs against the PREVIOUS installation's own tracked list and surgically removes only files that were tracked as distributed before and aren't part of this revision -- a file never tracked (a user addition placed inside an owned directory) is never touched, regardless of whether it lives under the default whole-directory "skills" or a narrow nested "skills/research" entry. Three additional bugs surfaced and fixed during implementation and testing (documented directly in code comments at each site): - An upfront rmtree(dest) for a whole-directory owned entry (e.g. the default "skills") would wipe out a user addition living inside that directory before the installed_files diff could even run -- switched to a non-destructive dirs_exist_ok=True merge-copy everywhere, relying entirely on the surgical stale-file prune (rather than directory deletion) to handle real removals. - distribution.yaml (MANIFEST_FILENAME) is itself one of the default owned paths, so the owned-path copy loop was overwriting target's manifest with the raw staged source's version (no installed_files) BEFORE the stale-file diff read it -- moved the previous-installed read to the very top of the function, before any copying. - Walking dest.rglob() after a merge-copy to determine "what did this payload just install" incorrectly counted pre-existing files (including genuinely stale ones) that were already physically present in the destination -- switched to walking src.rglob() (the staged source itself) instead. - Skipping config.yaml's copy due to preserve_config caused the stale-file prune to see it as removed-from-this-revision and delete the user's own config -- now explicitly counted as installed even when its copy is skipped. Added 6 new tests (nested directory/file owned entries, narrow-entry sibling isolation, stale-file removal on update, target-only file survival inside an owned directory, and installed_files tracking), and updated two pre-existing tests that assumed unrestricted copying (from before any allowlist enforcement existed) to explicitly opt their directories into distribution_owned, matching the now-correctly- enforced allowlist. 49/49 pass in the full tests/hermes_cli/test_profile_distribution.py file (0 pre-existing failures once updated for the new allowlist semantics).
alt-glitch
added
type/bug
Related: #74414 covers the profile-distribution allowlist path. This PR additionally implements nested owned paths and per-revision installed-file tracking to preserve user additions while pruning only stale distributed files. |
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for closing the allowlist gap and preserving target-only additions. The premise is verified: current main's hermes_cli/profile_distribution.py:563-590 copies every staged root entry without consulting distribution_owned, despite the selective-update contract in website/docs/reference/profile-commands.md:431-433.
Problems
- Blocking:
hermes_cli/profile_distribution.py:626only strips slashes before joining an owned path tostagedandtargetat lines 633 and 644.skills/../../auth.jsonbypasses the top-level exclusion check and escapes both roots. - Blocking:
installed_filessimilarly preserves..at lines 222-226, and stale cleanup joins and unlinks it at lines 674-678. A persisted traversal entry can delete outside the profile.
Suggested changes
- Reject non-normalized, non-relative manifest paths (including
.and..) for both owned and tracked file lists before any filesystem operation, and add traversal regression tests.
This is an automated hermes-sweeper review.
| shutil.copy2(staged / ENV_TEMPLATE_FILENAME, target / ENV_EXAMPLE_FILENAME) | ||
|
|
||
| for owned_path in owned: | ||
| owned_path = owned_path.strip().strip("/") |
There was a problem hiding this comment.
Please validate this as a normalized manifest-relative path before joining it to either root. skills/../../auth.json passes the top-level skills check, but makes src and dest escape staged and target; validate persisted installed_files similarly before stale pruning.
teknium1
added
sweeper:risk-security-boundary
|
Both blocking security issues confirmed and fixed in #75494. Added _is_safe_manifest_relative_path() rejecting empty strings, absolute paths, backslashes, Windows drive forms, and any "."/".." component anywhere in the path -- applied at the owned_path loop, the stale-file prune loop, and DistributionManifest.from_dict() itself (defense-in-depth, filtering at parse time). Verified this was genuinely exploitable: temporarily reverted the fix and confirmed skills/../../auth.json actually writes into the shared profiles/ directory, one level above every profiles own target_dir. Restored the fix and confirmed its blocked, with the rejection explicitly logged. Added the requested traversal regression tests (crafted owned_path, corrupted persisted installed_files entry, and parse-time filtering). 53/53 pass in the full file. Closing this in favor of #75494. |
3 participants
Context
Supersedes #74409 per @teknium1's review -- two real gaps, plus three additional bugs surfaced during implementation.
Fixes
owned_paths()entry directly as a manifest-relative path (e.g.skills/research,cron/digest.json) against staged/target, instead of a root-name comparison that could only ever match top-level entries.installed_filestracking to the manifest; on update, diffs against the previous installation's own tracked list and surgically removes only files that were tracked as distributed before and aren't part of this revision.Bugs found and fixed along the way
rmtreefor a whole-directory owned entry would wipe out user additions before the diff could run -- switched to non-destructive merge-copy everywhere.distribution.yamlis itself a default owned path, so the copy loop was clobbering the tracking info before it could be read -- moved the read to the top of the function.config.yaml's copy forpreserve_configcaused it to look stale and get deleted -- now explicitly counted as installed.Tests
6 new tests plus 2 pre-existing tests updated to opt their directories into
distribution_owned(they predated any allowlist enforcement).49/49 pass in the full
tests/hermes_cli/test_profile_distribution.pyfile.