Skip to content

fix(security): prevent profile export archives from reaching source and images - #92689

Closed
JoaoMarcos44 wants to merge 1 commit into
NousResearch:mainfrom
JoaoMarcos44:fix/security-webhook-archive-92457
Closed

fix(security): prevent profile export archives from reaching source and images#92689
JoaoMarcos44 wants to merge 1 commit into
NousResearch:mainfrom
JoaoMarcos44:fix/security-webhook-archive-92457

Conversation

@JoaoMarcos44

@JoaoMarcos44 JoaoMarcos44 commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Summary

fixes : #92457

This PR addresses the recurrence-control portion of #92457 without reopening the already-merged cleanup in #92394.

  • Automatic profile exports from the CLI and /export no longer default to <name>.tar.gz in the current working directory.

  • CLI, TUI, and profile-management API paths now share one managed destination helper.

  • A profile archive guard rejects .tar.gz and .tgz files anywhere in a checkout before CI can approve it or Docker can build/publish it.

  • The guard reports filenames only; it never reads or prints archive contents.

  • .gitignore, .dockerignore, user documentation, and regression tests now describe and enforce the same boundary.

This is intentionally not an incident-closure claim. Repository code cannot rotate the deployed webhook HMAC secret, invalidate already-pulled image layers, or delete Docker Hub manifests without the credential and registry owners. #92457 should remain open until those operational predicates are verified at their systems of record.

Is #92457 a real bug?

Yes. The issue's live evidence establishes exposure, not exploitation:

  1. Commit ee37f3d8976b1c7a79a0eacbb15e4e887133a59d added a populated default.tar.gz profile export.

  2. The archive contained a live-looking platforms.webhook.extra.secret and was still reachable through Git history.

  3. The release source for v2026.8.19 still contained the blob, and the published image was built with a repository-wide COPY . . boundary before the archive exclusion landed.

  4. chore: committed profile export (default.tar.gz) removed from repo root and Docker layers (salvage #91712) #92394 removed the current-tree artifact and added Docker exclusions, but it explicitly did not rotate the credential or settle historical image manifests.

The underlying code-level recurrence was also reproducible on the current pre-fix base: hermes profile export default with no -o wrote default.tar.gz into the caller's working directory. The fixed path writes to a managed profile-export store and leaves the checkout without that file.

Root cause

The incident had two separate causes, which must not be conflated:

  • Producer boundary: the CLI and interactive /export defaulted to a repository-visible filename. A normal export from a source checkout created a generated artifact at the exact location a contributor could accidentally stage.

  • Publication boundary: .gitignore and .dockerignore were passive filters. They reduced accidental staging and image inclusion, but did not stop git add -f, a differently named archive, or a generated archive present in a build context.

  • Operational boundary: revoking a webhook secret and retiring an already-published image are deployment/registry actions, not repository edits.

Implementation

Safer producer default

get_profile_export_path() now:

  • validates and normalizes the profile name;

  • writes automatic exports under <default Hermes home>/profile-exports/<name>-<timestamp>.tar.gz;

  • keeps that store outside named profiles;

  • detects the unusual custom-deployment case where HERMES_HOME is inside a Git checkout and falls back to a sibling/temp store instead of creating new source-tree input.

Explicit -o output remains supported for intentional exports.

The CLI command, interactive /export, and profile API all use this helper. This removes duplicated destination logic rather than adding another independent export implementation.

Executable defense at both gates

scripts/ci/check_profile_archive_boundary.py walks the checkout (excluding .git, virtual environments, dependency trees, and bytecode caches) and rejects any .tar.gz or .tgz path. It is wired into:

  • the unconditional CI orchestrator and the blocking All required checks pass aggregate;

  • the Docker build job before Buildx runs;

  • the trusted Docker publish job before registry credentials are used.

This is stronger than another ignore rule: a forced commit, a renamed root archive, a nested archive, or a generated file present after checkout cannot silently proceed to image publication.

Non-duplicate analysis

Security and compatibility

  • No credential value, archive content, private path, or registry token is added to source, tests, logs, or this PR.

  • Existing export redaction and credential-file exclusions remain unchanged.

  • Explicit output paths remain compatible; only the implicit destination changes.

  • CI failure is fail-closed and actionable: move the archive outside the checkout or choose an external explicit output path.

  • The checker does not inspect archive bytes, so it cannot leak a secret while reporting the violation.

GPT-5.6 Luna P1 hardening record

GitHub labels: P1, type/security, area/auth, area/docker, area/profiles, and sweeper:risk-security-boundary. Required intensity: three independent hardening passes.

Pass 1 — premise and root cause

Decision: this is a real recurrence gap, but #92457's credential rotation and registry retirement remain operational work.

Pass 2 — failure modes and bottlenecks

  • Forced staging: the executable CI guard does not trust .gitignore.

  • Renamed and nested archives: the guard matches .tar.gz/.tgz case-insensitively anywhere in the checkout.

  • Docker publication: the guard runs before both unprivileged Buildx builds and the credentialed publish job.

  • Custom HERMES_HOME inside a checkout: the automatic path selects a sibling/temp store.

  • Secret leakage in diagnostics: only archive filenames are emitted; fixture tests assert archive contents are never printed.

  • Valid path preservation: explicit -o output and existing archive scrubbing remain intact.

Pass 3 — simplification and regression

  • One shared destination resolver replaces three independent default-path implementations.

  • The source-tree fix and publication guard are separate enforcement points because either one can be bypassed in isolation.

  • Added behavioral coverage for CLI, slash command, API, custom checkout placement, root/nested/case-variant archives, and secret-free diagnostics.

Validation evidence

Passing

  • scripts/run_tests.sh tests/scripts/test_check_profile_archive_boundary.py tests/hermes_cli/test_profile_export_default_path.py -q — 8 passed.

  • scripts/run_tests.sh tests/hermes_cli/test_profile_export_credentials.py tests/hermes_cli/test_profiles.py -k 'export and not symlink' -q — 9 passed.

  • uv run ruff check scripts/ci/check_profile_archive_boundary.py tests/scripts/test_check_profile_archive_boundary.py tests/hermes_cli/test_profile_export_default_path.py hermes_cli/profiles.py hermes_cli/main.py hermes_cli/cli_commands_mixin.py hermes_cli/web_routers/profiles.py — passed.

  • uv run ruff format --check on the three new Python files — passed.

  • git diff --check — passed.

  • Direct checker smoke test on the checkout — no profile archives detected.

  • Red/green reproduction: pre-fix base reported baseline_archive True and managed_archive_count 0; this branch reported fixed_archive False and managed_archive_count 1.

Local limitations (not presented as green)

  • The canonical full scripts/run_tests.sh run was interrupted by the local 420-second tool ceiling at 20.1% (6,993 tests passed and 67 early failures were reported; the runner had no final summary). The first failures were in unrelated ACP/relay suites; they were not baseline-proven and are not claimed to be fixed by this PR.

  • npm run check could not start a valid workspace check in this clean worktree because JavaScript dependencies were not installed (@nanostores/react, React, Vitest, Node types, and other workspace packages were missing). The touched behavior is Python/CI/Docker/docs, and no JS source changed.

  • actionlint was not installed locally. GitHub Actions validation remains a remote-CI responsibility.

  • No local Docker Hub mutation or production webhook rotation was attempted.

Live CI follow-up

Remote run 32617194335 for head 13d6f392dacc50407836a2b5675bcadf85b18bc2 was inspected with the failed-job logs:

  • Python tests, Python lints, Windows/macOS tests, Docker builds, Docker scripts, Nix, OSV, docs, installer, Rust, supply-chain, and the new Profile Artifact Boundary all passed.

  • JS & TS checks failed only in apps/desktop :: check:test:ui because Vitest reported one unhandled ReferenceError: window is not defined from the existing user-edit-composer.tsx cooldown timer after user-message-edit.test.tsx teardown. The source and line are identical on origin/main; this PR changes no JavaScript or Desktop files, so I am not masking a baseline UI-test failure with unrelated changes.

  • Review label gate failed because the required ci-reviewed label is absent. The label exists, but this contributor token lacks permission to apply labels on NousResearch/hermes-agent (AddLabelsToLabelable returned 403). A maintainer must add it, then rerun the failed checks.

The aggregate All required checks pass failure is therefore the combination of the pre-existing/flaky UI test and the maintainer-only label gate, not a failure in this PR's Python, Docker, or profile-archive implementation.

Operational follow-up still required for #92457

The issue's remaining closure predicates are outside this repository PR:

  • identify the authoritative owner of the exposed inbound HMAC secret;

  • rotate it at Hermes and every configured sender, proving old signatures are rejected and new signatures accepted without publishing either value;

  • retire/replace v2026.8.19 and any affected immutable manifests, recording the replacement multi-platform index and child digests;

  • audit Docker Hub tag/digest retention for old main/latest manifests;

  • publish and independently read back the dated incident receipt.

Related issue

Related to #92457. This PR deliberately does not use a closing issue reference, because merging repository code must not falsely close an incident whose credential and registry predicates are still open.

Infographic (required)

PR infographic — prevent profile archives from reaching published images

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.
@JoaoMarcos44
JoaoMarcos44 requested a review from a team August 23, 2026 04:08
@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists area/profiles Multi-profile isolation, HERMES_HOME scoping area/docker Docker image, Compose, packaging sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 23, 2026

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking review on exact head 13d6f392dacc50407836a2b5675bcadf85b18bc2.

The release guard/default-path centralization is directionally correct, but the core source-checkout exclusion predicate is anchored to the process working directory rather than to the path whose safety it is supposed to prove.

_get_default_hermes_home() explicitly honors HERMES_HOME. _profile_export_directory() then constructs <default_home>/profile-exports, but discovers checkout_root only by walking Path.cwd() and its parents. That leaves a deterministic bypass:

  1. /tmp/repo/.git exists;
  2. HERMES_HOME=/tmp/repo;
  3. Hermes is launched from /tmp/outside (no .git ancestor);
  4. _profile_export_directory() gets checkout_root=None and returns /tmp/repo/profile-exports.

The default export archive is therefore written inside the source checkout again—the exact artifact-in-repository input this PR is intended to make impossible after #92457/#92394. The same bug appears when cwd is inside unrelated checkout A while HERMES_HOME is inside checkout B: the candidate is tested only against A, so B is not recognized as a checkout at all.

The new regression covers only the favorable topology where cwd, default home, and checkout are the same tree. It does not exercise the supported custom-home case that breaks the invariant.

Please make the authority path-relative rather than cwd-relative: resolve the candidate/default Hermes home, inspect its own ancestry for a Git worktree/repository marker, and if the candidate lies inside a checkout, choose and verify a fallback outside that checkout. Please add at least these two production-shape regressions:

  • HERMES_HOME inside checkout B + cwd outside any checkout -> export outside B;
  • HERMES_HOME inside checkout B + cwd inside unrelated checkout A -> export outside B (and not derive safety from A).

If worktree support is intended, remember .git may be a file rather than a directory; the invariant is “outside any source checkout,” not merely “outside a directory containing a .git/ folder.”

Everything around that boundary is useful defense-in-depth and should be preserved: the centralized default path, *.tar.gz / *.tgz Git and Docker exclusions, the repository archive scanner, and CI/Docker enforcement all reduce recurrence once the default path is actually guaranteed outside the source tree.

Topology/provenance:

  • #92457 is the open incident-response/settlement owner for the committed profile archive exposure and explicitly calls for a long-term recurrence invariant.
  • #92394 by @teknium1 is the closed/unmerged operational stopgap/provenance line; it removed published archive material and added immediate leak checks, preserving the earlier @EAbaracus #91712 hardening lineage. #92689 is the right place for the durable path invariant rather than another cleanup-only PR.
  • merged #83458 by @OutThisLife is complementary export-content minimization/redaction. It reduces credential content but cannot prevent an archive artifact from entering a checkout.
  • open/non-mergeable #35601 by @keiranhaax is broader export/import hardening (credentials/KDF/permissions/hardlinks), adjacent rather than a substitute for source-tree exclusion.

Exact object/repository state checked: PR base and merge-base fd760435c6688a2b6c6b7436dde30e267237baef; current main 8b86097a62ad9d35c74a6a62e2cf233c5ceb038d; branch 2 commits ahead / 13 behind. The main drift includes overlapping .gitignore, hermes_cli/main.py, and tests/hermes_cli/test_main.py, so this needs a current-main rebase and fresh exact-head evidence after the path fix.

Exact-head Docker and Nix are green. CI 32617194335 is red: the profile-archive release guard and relevant Python/test/lint/security lanes passed, while the JS/TS job and independent review-label gate failed. I am not attributing the JS/TS failure to this patch without a log-grounded cause, but it means the exact head is not repository-green today.

The blocker here is independent of that CI state: as submitted, the default-path proof can still place a profile export inside a source checkout under an explicit HERMES_HOME.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Addressed in the salvage PR #92771 (this PR's commit cherry-picked with authorship preserved, fixes layered on top):

  • cwd-anchoring bypass (your blocker): fixed. _inside_git_checkout() now proves safety on the export directory's OWN resolved ancestry — Path.cwd() is no longer consulted at all. .git-as-file (worktrees/submodules) is handled; resolve() failure conservatively treats the candidate as unsafe.
  • Both requested production-shape regressions added: HERMES_HOME inside checkout B + cwd outside any checkout, and HERMES_HOME inside B + cwd inside unrelated checkout A. Both were confirmed failing on this head via live repro before the fix, and both fail on the pre-fix helper under mutation testing.
  • Beyond the blocker: when every candidate destination resolves inside a checkout the helper now fails closed (clear error, no write) instead of proceeding; the temp-dir fallback uses a per-uid name and rejects pre-created symlinks/foreign-owned directories; and the get_profile_export_path() calls moved inside the CLI/TUI try blocks so an invalid profile name prints the clean Error: main gives today instead of a traceback.
  • Current-main rebase: the salvage stack is built on current main (your review noted the 13-commit drift), with fresh exact-head evidence on fix(security): profile exports never land in source checkouts; CI rejects archive artifacts (salvage #92689) #92771.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Thanks @JoaoMarcos44 — this was salvaged into #92771 with your commit cherry-picked and your authorship preserved. Your producer-boundary design (managed export destination shared across CLI/TUI/API) and the fail-closed CI/Docker guard survive intact.

On top of your commit, the salvage adds the fixes from @andrexibiza's blocking review plus final-diff review findings: checkout detection now anchors on the export path's own ancestry instead of Path.cwd() (with both requested topology regressions), the helper fails closed when no safe destination exists, the get_profile_export_path() calls moved inside the CLI/TUI try blocks so invalid names print clean errors again, and the temp fallback is hardened against predictable-path attacks.

Closing this PR in favor of #92771. Note: your Summary's "fixes : #92457" would have auto-closed the incident issue on merge, contradicting the PR's own "deliberately not an incident-closure claim" — the salvage PR uses a non-closing reference so #92457 stays open for the rotation/republish predicates, as you intended.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Salvaged via #92771 with your commit cherry-picked and authorship preserved — thank you for the thorough work here: the managed destination design, the guard script (filesystem-truth rationale included), the CI/Docker wiring, and the test coverage all survived intact.

What the salvage adds on top of your commit:

  • Fixed the blocking-review finding: checkout detection now proves safety on the export directory's own ancestry (_inside_git_checkout) instead of Path.cwd(), covering the HERMES_HOME-inside-checkout + cwd-elsewhere topology (cron/service-manager invocations). E2E-verified before and after.
  • When every candidate destination is inside a checkout, the export now fails closed with a clear error instead of proceeding.
  • get_profile_export_path() moved inside the CLI/TUI try blocks with OSError handling — bad profile names print a clean Error: again instead of a traceback.
  • Per-uid temp fallback + symlink/ownership checks on the managed directory (predictable-path hardening).
  • Changed fixes #92457Refs #92457 so the incident tracker (rotation/republish — operational work) stays open, matching your own note that the PR intentionally doesn't close those predicates.
  • Test file binds modules at call time so directory-sweep reload pollution can't detach the monkeypatches.

@andrexibiza — your exact-head blocker on 13d6f392 is addressed in #92771: the exclusion predicate is now anchored to the path being proven safe (both bypass topologies you described — HERMES_HOME-in-checkout with cwd outside, and cwd in unrelated checkout A while HERMES_HOME sits in checkout B — have dedicated regression tests, mutation-checked against the pre-fix helper).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docker Docker image, Compose, packaging area/profiles Multi-profile isolation, HERMES_HOME scoping comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security: rotate exposed webhook credential and republish affected images after committed profile archive

4 participants