Skip to content

fix(security): mask secret values in secret-source status lines - #77012

Open
andrexibiza wants to merge 2 commits into
NousResearch:mainfrom
andrexibiza:fix/security-secret-source-status-formatter
Open

fix(security): mask secret values in secret-source status lines#77012
andrexibiza wants to merge 2 commits into
NousResearch:mainfrom
andrexibiza:fix/security-secret-source-status-formatter

Conversation

@andrexibiza

@andrexibiza andrexibiza commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Related #60295 #69054

What changed and why

The startup status block in hermes_cli/env_loader.py prints secret-source error, remediation-hint, warning, and conflict lines straight to stderr. The merged secret-name suppression (#60295 / #69054) removed the applied-name list from the status line — but a source error or warning that echoes a secret value (e.g. a backend quoting the key back) would still print that value.

This PR routes all four status-line types through a value masker:

  • agent/redact.py::mask_known_secret_values() — masks the exact values of credential-named env vars (*_API_KEY, *_TOKEN, *_SECRET, *_KEY, *_PASSWORD), catching opaque tokens that carry no recognizable vendor prefix and would slip past the shape-based regex passes.
  • hermes_cli/env_loader.py::_mask_secret_text() — additionally masks every value applied from external secret sources this process (Bitwarden / 1Password / command sources), which is the authoritative set for what the status line is about.

Why this matters to you as a user

Secret names already stopped being printed at startup; now secret values cannot leak through error or warning text either. If a backend echoes your key back inside an error message, you still see the diagnostic — just without the key in it. This closes the value half of the same disclosure class the name fix addressed.

Reproduction steps (current behavior on main)

  1. Configure BWS per the docs and export BWS_ACCESS_TOKEN.
  2. Point secrets.bitwarden.access_token_env at a token whose value contains a recognizable prefix, or force any source error/warning that quotes a credential value.
  3. Observe stderr — the value appears verbatim in the status line.

Current: Bitwarden Secrets Manager: <error text containing the raw secret value>
Expected: same diagnostic with the value replaced by ***.

How to test

  • scripts/run_tests.sh tests/test_env_loader_secret_sources.py → 20 passed (2 new regression tests: value-in-warning masked, value-in-error masked while the diagnostic is preserved).
  • scripts/run_tests.sh tests/test_env_loader.py tests/test_env_loader_applied_homes.py tests/secret_sources/ tests/agent/test_redact.py tests/hermes_cli/test_redact_config_bridge.py → 143 passed, 0 failed.

Platforms tested

  • Windows 11 (git-bash), Python 3.11, canonical scripts/run_tests.sh runner.
  • git diff --check clean; check-windows-footguns.py clean on changed files.

Related

Part of #77162
Part of #77165

The startup status block in env_loader prints error, remediation-hint,
warning, and conflict lines straight to stderr. The merged secret-name
suppression (NousResearch#60295/NousResearch#69054) removed the applied-name list, but a source
error or warning that echoes a secret VALUE would still print it.

This change routes all four status-line types through a value masker:
- agent.redact.mask_known_secret_values() masks the exact values of
  credential-named env vars (opaque tokens with no vendor prefix)
- env_loader._mask_secret_text() additionally masks every value applied
  from external secret sources this process (Bitwarden/1Password), which
  is the authoritative set for what the status line is about

Why this matters to users: secret names already stopped being printed;
now secret values cannot leak through error or warning text either. If a
backend echoes a key back in an error message, you see the diagnostic
without the key.

Tests: 67 passed across env_loader + secret-sources suites (2 new
regression tests: value in warning masked, value in error masked) plus
the existing 76-test redact suite.
@teknium1

teknium1 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Thanks for covering the direct stderr status path; the premise holds on current main: hermes_cli/env_loader.py:563-571 prints source errors, hints, warnings, and conflicts without redaction.

Problems

  • In ef46cf0, _mask_secret_text() scans every _SECRET_SOURCE_VALUES_BY_HOME snapshot. Those snapshots are intentionally per-home (hermes_cli/env_loader.py:39-41, 159-164); status output for one profile should not depend on another profile's values.
  • ef46cf0 skips exact values shorter than six characters. A short external-source *_PASSWORD or *_TOKEN echoed by a backend would still leak through the status line.

Suggested changes

  • Thread home_path into _mask_secret_text() and inspect only that resolved home's snapshot.
  • Mask every non-empty value from that authoritative source snapshot, and add short-value plus cross-home regression tests.

Automated hermes-sweeper review.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 2, 2026
…lues

Addresses hermes-sweeper review on NousResearch#77012:

- _mask_secret_text() now takes home_path and reads only that resolved
  home's snapshot from _SECRET_SOURCE_VALUES_BY_HOME. Status output for
  one profile no longer depends on another profile's secret values.
- Drop the 6-character minimum-length filter on snapshot values: a short
  external-source *_PASSWORD / *_TOKEN echoed by a backend would have
  leaked through the status line.

Adds two regression tests: a short-value warning echo through the real
apply path, and a cross-home isolation test asserting each home's status
line masks only its own snapshot values.
@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Aug 2, 2026
@andrexibiza

Copy link
Copy Markdown
Contributor Author

Fixed in 35c592a77f. Both points addressed:

  • Home-scoped masking_mask_secret_text(text, home_path) now resolves home_path and reads only that home's snapshot (_SECRET_SOURCE_VALUES_BY_HOME[home_key]). Status output for one profile no longer depends on — or masks against — another profile's values.
  • No minimum-length filter — every non-empty value from the home's authoritative snapshot is masked. A short external-source *_PASSWORD / *_TOKEN echoed by a backend can no longer leak through a status line (the generic agent.redact env scan skips values under 6 chars, so the snapshot pass is the only thing that can catch these).

Regression tests added to tests/test_env_loader_secret_sources.py:

  • test_status_warning_with_short_secret_value_is_masked — a 2-char applied value echoed in a warning is masked through the real apply path.
  • test_mask_secret_text_scoped_to_own_home_snapshot — cross-home isolation: each home's status line masks only its own snapshot values, leaving the other home's value untouched.

Validation: scripts/run_tests.sh tests/test_env_loader_secret_sources.py → 22 passed, 0 failed (2 new tests included); git diff --check clean; scripts/check-windows-footguns.py → no footguns.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed both sweeper items. Ready for another look.

andrexibiza added a commit to andrexibiza/hermes-agent that referenced this pull request Aug 3, 2026
…cription, skill test

Addresses teknium1's review on NousResearch#77097:

1. 'Not true on main' — the security contract is now explicitly scoped
   as implemented by the secrets-exfiltration hardening series
   (NousResearch#77008/NousResearch#77012/NousResearch#77020/NousResearch#77027/NousResearch#77031/NousResearch#77039). The docs state current
   main behavior plainly (plaintext bws_cache.json read/written when
   encryption disabled, default false) and keep the rotation instruction
   mandatory today, since that exposure already exists on main. The
   posture framing stays — this eliminates an entire vulnerability
   class — but the claim is now sequenced truthfully.
2. Skill description shortened to 53 chars, one sentence, ends with a
   period (AGENTS.md hardline).
3. tests/skills/test_bitwarden_secrets_skill.py added: validates
   frontmatter, description length, required sections, user-only
   rotation + clipboard discipline, honest series scoping (no claim the
   gate test is on main), and docs-page metadata consistency.
4. Clipboard discipline added to rotation instructions (docs + skill):
   create token, copy to clipboard, paste into terminal, save nowhere
   in between.
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Three PRs address related secret-value disclosure paths: #77012 masks external-secret-source status errors, hints, warnings, and conflicts with home-scoped snapshots; #77014 combined an earlier status-path implementation with formatter-wide masking; and #77020 independently adds formatter-wide masking for opaque credential environment values while preserving the global opt-out.

Related pull requests

Duplicates

#77014 substantially overlaps #77012 on status-line masking and #77020 on formatter masking, but #77012 and #77020 cover distinct output paths and are not duplicates of each other.

Suggested consolidation

Keep #77012 open with the salvage path of its home-scoped status-output masking and short-value/cross-home tests, and keep #77020 open with the salvage path of its opt-out-aware formatter masking and disabled-setting test. Retain already-closed #77014 as the superseded combined precursor; its two substantive parts are covered separately by #77012 and #77020.

Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 28 kB of PR diffs, 7 kB of issue/PR text, 4 kB of discussion (6 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@andrexibiza

Copy link
Copy Markdown
Contributor Author

Both sweeper findings were addressed in the current head (35c592a): (1) home-scoped masking — _mask_secret_text(text, home_path) resolves and reads only that homes snapshot, so one profiles status output no longer depends on another profiles values; (2) short values — every non-empty value from the authoritative source snapshot is masked, with short-value and cross-home regression tests added. The GottZ delta confirms the diff now addresses both cited problems. Ready for another look.

@alt-glitch alt-glitch added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data and removed sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

4 participants