fix(security): profile exports never land in source checkouts; CI rejects archive artifacts (salvage #92689) - #92771
Conversation
Route automatic profile exports to a managed store instead of the current checkout, and enforce a CI/Docker boundary that rejects archive files before they can be published.
Follow-up to the salvaged NousResearch#92689: - _profile_export_directory() now proves safety on the export dir's OWN ancestry (_inside_git_checkout) instead of walking Path.cwd(). The old heuristic missed the checkout whenever HERMES_HOME sat inside one but the process ran from elsewhere (cron, service manager) — the export landed back inside the source tree, the exact incident class. - When every candidate is inside a checkout, warn instead of silently violating the invariant. - CLI/TUI export callers: move get_profile_export_path() inside the try and catch OSError too — a bad profile name or read-only home printed a raw traceback instead of the clean error main previously gave. - Tests: bind module objects at call time (importlib) so sibling reload pollution in the tests/hermes_cli sweep can't divorce monkeypatches from the code under test; add regression tests for the cwd-independent topology and the clean-error path. - Docs: mention the ~/.hermes-profile-exports fallback store.
…ge edges - _profile_export_directory(): when the managed store, the home-sibling store, AND the temp dir all resolve inside Git checkouts, raise a clear ValueError instead of warning and proceeding — a stderr warning would not stop a scripted export from staging a secret-bearing archive in a source tree, which is the exact NousResearch#92457 incident class. All three callers already surface ValueError cleanly (CLI/TUI print Error: + exit, API returns 400). - .dockerignore: drop the /default.tar.gz line made redundant by the global *.tar.gz pattern this PR adds. - hermes profile export -o help text: stop advertising the old <name>.tar.gz cwd default. - Tests: cwd-in-unrelated-checkout topology (the second production shape from the blocking review) and the fail-closed path. Mutation-checked: both fail on the pre-fix helper.
…rror text; docs Final-diff review findings (/simplify-code on the full 3-commit stack): - Temp-dir fallback uses a per-uid name (hermes-profile-exports-<uid>) and get_profile_export_path refuses a pre-existing symlink or a directory owned by another user — a fixed /tmp/hermes-profile-exports is a predictable shared path a local attacker could pre-create to receive the secret-bearing archive. Regression test mutation-checked. - Fail-closed message reworded interface-neutrally (the web API surfaces it as HTTP 400 detail where '-o' alone made no sense). - Docs now cover the temp fallback and the fail-closed refusal.
andrexibiza
left a comment
There was a problem hiding this comment.
Blocking review (GitHub will not permit this identity to set REQUEST_CHANGES without explicit repository review access) on exact head e33e27b3e8fd7b47e5f2ac27f20e4d0c68c842a7.
The salvage fixes the blocker from #92689 correctly: checkout detection is now candidate-relative rather than cwd-relative, .git files cover worktrees/submodules, the all-candidates-in-checkouts case fails closed, CLI/TUI/API surface that failure cleanly, and the per-uid temp fallback plus symlink/owner checks close the predictable-path precreation shape. The CI/Docker archive guard is also wired into the required aggregate and both build/publish paths. Exact-head CI 32627005328, Docker 32627004922, and Nix 32627004878 are all green, including the new Profile Artifact Boundary job, Python/e2e, Windows/macOS, lint/type, supply-chain, attribution, and review-label lanes.
There is one remaining confidentiality boundary before I think this is safe to merge: the managed export destination proves ownership, but never makes the directory or archive private.
get_profile_export_path() currently does:
export_dir.mkdir(parents=True, exist_ok=True)
...
if export_dir.stat().st_uid != os.getuid():
raise ValueError(...)
return export_dir / f"{canon}-{stamp}.tar.gz"and export_profile() ultimately creates the returned path through _make_profile_archive() / tarfile.open(..., "w:gz"). Neither path sets a restrictive mode. Under the ordinary POSIX umask 022, this exact primitive produces a 0755 export directory and a 0644 archive. I reproduced that directly with Path.mkdir() + the same tarfile.open(..., format=GNU_FORMAT) sequence.
That matters specifically because this PR has expanded the threat model to other local users: the temp fallback was hardened so another user cannot pre-create/symlink the predictable directory and receive a "secret-bearing archive". But once the current user creates that same directory successfully, the default permissions can let local peers read the archive anyway. The fallback cases make this concrete: ~/.hermes-profile-exports may sit under a traversable home, and the per-uid temp directory sits under a shared temp root. Ownership is not confidentiality.
The archive is also not safe to treat as public after #83458. Merged #83458 / @OutThisLife force-redacts recognized secret-shaped strings and excludes exact auth.json / .env, but its own contract explicitly says this is not general PII. Open #35601 / @keiranhaax is adjacent structural hardening because current exports can still carry renamed credential stores, opaque auth state, private-key formats, binary databases, and sensitive extras. #92771 must therefore protect the artifact it creates even if #35601 has not landed yet; the two concerns are complementary, not prerequisite/substitute.
Required fix:
- make every automatically managed export directory private (
0700, or fail closed if an existing owned directory cannot be made/verified private); - create the archive as
0600at creation time, not only with a post-create chmod that leaves a disclosure window. Avoid a process-globalumaskflip in this multi-threaded application; a private pre-opened fd / equivalent atomic creation path is the safer shape; - add regressions under a normal
022umask proving the managed directory has no group/other bits and the resulting archive is owner-only; - cover a pre-existing owned-but-broad-readable managed directory (tighten safely or refuse), plus the temp-fallback path.
I would also document the Windows disposition explicitly. The POSIX finding above is sufficient to block; on Windows os.getuid is absent, so this branch relies on the platform temp/home ACL model instead of the ownership/mode checks it applies on POSIX. That can be a separately justified platform contract if the ACLs are already per-user.
Topology/provenance is otherwise clean:
- #92457 remains the incident-response owner for credential rotation, affected images, and history settlement; this PR correctly uses
Refs, notFixes. - merged #92394 is the current-tree/Docker cleanup owner and preserves @EAbaracus's #91712 removal provenance.
- #92689 by @JoaoMarcos44 is superseded as a delivery object by this salvage; the original implementation is genuinely preserved here as commit
95e5612b63c7b2945e9b86aeacda4012e438d5a5with Joao's authorship intact. - merged #83458 is complementary content redaction; open #35601 is broader structural export filtering. Neither replaces destination confidentiality.
Repository drift is non-overlapping but real: current main is f377140e3ddb4c98a9e0b42c4497b8bb46ca697c; actual merge base is 1684877868807aad695dd714b9109a216d5741ae; this branch is 5 commits ahead / 15 behind. Those 15 main-side commits do not touch this PR's files, so they do not explain the finding, but rebase and fresh exact-head receipts should follow the mode fix.
The core recurrence architecture is now right; close this last local-read authority hole and I think the producer + repository + Docker boundary becomes coherent.
|
Thanks @kshitijk4poor — the #92689 protection finally landed via #100101, which resurrected @JoaoMarcos44's work onto current main composed with #97835's atomic make_targz and re-fixed the cwd-anchoring review point this salvage carried. Closing as superseded by the merged resurrection. |
Pull request was closed
Summary
Default profile exports can no longer land inside a source checkout, and CI/Docker publication now reject any archive in the build context. Salvages #92689 by @JoaoMarcos44 (cherry-picked, authorship preserved) with the blocking-review finding fixed on top.
Refs #92457 (recurrence-control predicate only — rotation/republish/history stay with the incident issue; intentionally NOT
Fixes, so the incident tracker stays open).Changes
From #92689 (@JoaoMarcos44, authorship preserved):
hermes profile export), TUI (/export), and the profile API share one managed default destination:HERMES_HOME/profile-exports/<name>-<timestamp>.tar.gz— never./<name>.tar.gzin the cwd (howdefault.tar.gzgot committed)scripts/ci/check_profile_archive_boundary.py: blocking guard rejecting*.tar.gz/*.tgzanywhere in the checkout; wired intoci.yaml(aggregate-gated, infographic-check precedent) and bothdocker.ymlbuild jobs before buildx; reports filenames only, never contents.gitignore/.dockerignoreun-anchored to global*.tar.gz+*.tgz(coherent now that the guard rejects nested archives loudly at CI; localgit addof a deep fixture still no-ops silently — future archive fixtures must be generated at test runtime)Follow-up fixes (this PR, addressing the blocking review on the original head):
_inside_git_checkout), notPath.cwd(). Before:HERMES_HOMEinside a checkout + invocation from elsewhere (cron, service manager) put the export back inside the source tree — the exact incident class. Reproduced live pre-fix, E2E-verified fixed.~/.hermes-profile-exports, temp dir) is inside a checkout, fail closed with a clear ValueError telling the user to pass-ooutside the repo — a stderr warning would not stop a scripted export from staging a secret-bearing archive in a source tree (all three callers already surface ValueError cleanly: CLI/TUI printError: …, API returns 400)get_profile_export_path()moved inside thetry,OSErroradded — a bad profile name or read-only home now prints the same cleanError: …main gives today instead of a raw tracebacktests/hermes_clisweep reload-pollution can't divorce monkeypatches from the code under test; new regressions for the cwd-independent topology and the clean-error path~/.hermes-profile-exportsfallback store/default.tar.gzline dropped from.dockerignore(redundant under the global pattern);hermes profile export -ohelp text no longer advertises the old cwd defaultget_profile_export_path()refuses a pre-existing symlink or a directory owned by another user (a fixed/tmp/hermes-profile-exportscould be pre-created by another local user to receive the secret-bearing archive); fail-closed message reworded interface-neutrally for the HTTP-400 API surface; docs cover the temp fallback and the refusalBehavior notes for review
get_hermes_home()(profile-aware) → now the shared pre-profile root'sprofile-exports/. Matches the "outside every named profile" intent; name+timestamp keeps collisions unlikely.*.tar.gzignore reverses the root-anchoring fromdb7dda468f(review feedback on chore/docs: cleanup committed root artifacts + minor docs/diagnostics fixes #91712). The new CI guard makes the policy "no archives anywhere, enforced loudly" — flagging for explicit sign-off.Validation
hermes profile exportdefault destination./<name>.tar.gz(cwd — committable)HERMES_HOME/profile-exports/<name>-<ts>.tar.gz~/.hermes-profile-exports/(E2E-verified)hermes profile export 'bad//name'git add -f/ generated pre-buildTargeted tests: 13/13 new tests pass;
tests/hermes_cli -k profilesweep failure set byte-identical to origin/main (pre-existing local pollution class, CI green). Guard script E2E: exit 0 clean / exit 1 on root, nested,.TGZ, dir-named offenders; contents never printed.Credit
Based on #92689 by @JoaoMarcos44 — commit cherry-picked with authorship preserved. Blocking-review finding (cwd anchoring) reported by @andrexibiza on the original PR.