Skip to content

fix: redact known Hermes secret env vars regardless of code_file mode - #61352

Closed
ShaoRou459 wants to merge 3 commits into
NousResearch:mainfrom
ShaoRou459:fix/redact-known-secret-env-vars
Closed

fix: redact known Hermes secret env vars regardless of code_file mode#61352
ShaoRou459 wants to merge 3 commits into
NousResearch:mainfrom
ShaoRou459:fix/redact-known-secret-env-vars

Conversation

@ShaoRou459

Copy link
Copy Markdown
Contributor

Problem

Terminal output from non-env-dump commands (cat, type) uses code_file=True which skips the generic KEY=VALUE redaction pass. This causes opaque API keys without recognized vendor prefixes to leak when reading .env files through the terminal.

Leaked keys: Gemini (AQ.*), Mistral (no prefix), Tavily dev keys (tvly-dev-), BrowserUse (bu_), Spotify client IDs (hex).

Root cause

redact_terminal_output sets code_file = not is_env_dump_command(command). For cat, this is True, skipping all _ENV_ASSIGN_RE / _JSON_FIELD_RE / _YAML_ASSIGN_RE passes. Only prefix patterns run — but most of these keys have no recognized prefix.

Fix

  1. Manual set of 95 known Hermes secret env var names (LLM providers, tools, messaging platforms, etc.)
  2. AST-based auto-scanner that parses tools/*.py for requires_env lists in registry.register() calls — so new tools are automatically covered
  3. New redaction pass in redact_sensitive_text that runs before the code_file gate, matching only these known names to avoid false positives on source code like MAX_TOKENS=100

All values are redacted to ***, consistent with the prefix-matcher output.

Verification

  • All 149 existing redact tests pass
  • Manual verification: cat .env now redacts all 6 previously-leaked keys

Terminal output from non-env-dump commands (cat, type) uses
code_file=True which skips the generic KEY=VALUE redaction pass.
This causes opaque API keys without recognized vendor prefixes
(Gemini AQ.*, Mistral, Tavily dev keys, BrowserUse bu_*, Spotify
client IDs) to leak when cat'ing .env files.

Add a curated set of known Hermes secret env var names plus an
AST-based auto-scanner for tool requires_env lists. A new redaction
pass runs before the code_file gate, matching only these known names
to avoid false positives on source code like MAX_TOKENS=100.

All values are redacted to ***, consistent with the prefix-matcher
output for recognized key formats.
@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 area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P2 Medium — degraded but workaround exists labels Jul 9, 2026
@egilewski

Copy link
Copy Markdown
Contributor

suggesting changes

The new known-env redaction still misses a common .env shape: an inline comment after the value. In agent/redact.py, _HERMES_KNOWN_ENV_RE requires the value to run to end-of-line with (?=\s*$), so terminal/file-read output for a line such as MISTRAL_API_KEY=mistralOpaqueSecret12345 # used for tests still returns the secret value unchanged under the code_file=True path used for cat .env. The same happens for export MISTRAL_API_KEY=... # ....

Security evidence:

  • trust boundary: terminal output from file-reading commands is untrusted content before it is returned to the agent/logging surfaces.
  • source/sink/invariant: known Hermes secret env var assignments in .env-style output should redact the value under code_file=True, including the optional export prefix and trailing inline comments.
  • current-main reproduction: running the redactor from the current-main worktree confirmed MISTRAL_API_KEY=mistralOpaqueSecret12345 and the inline-comment variant both leaked, establishing the pre-existing gap.
  • PR-head or patch-replay validation: running the redactor from the PR worktree confirmed the plain assignment now becomes MISTRAL_API_KEY=***, but MISTRAL_API_KEY=mistralOpaqueSecret12345 # used for tests and export MISTRAL_API_KEY=mistralOpaqueSecret12345 # used for tests still leak the value.
  • positive/negative cases: the plain known-secret assignment now redacts, MAX_TOKENS=100 remains unchanged under code_file=True, and tests/agent/test_redact.py passes with 149 tests, but there is no coverage for the inline-comment form.
  • residual bypass search: the bypass is caused by the new line-end-bound regex in agent/redact.py, before the generic env-assignment redactor is skipped for code/file-read output.
  • reviewer validation: the blocker is in-scope because the PR claims to redact known Hermes secret env vars regardless of code_file mode, and the missed examples use the same known key family and file-read path targeted by the patch.

Signed: GPT-5.5-xhigh in Codex

@egilewski

Copy link
Copy Markdown
Contributor

suggesting changes

The latest head fixes the inline-comment redaction gap from my earlier comment, but it also rewrites tests/agent/test_redact.py with CRLF line endings. git diff --check 73b611ad19720d70308dad6b0fb64648aaadc216 3905781ac31f714acd41119959ae749bd64e1da4 -- agent/redact.py tests/agent/test_redact.py now reports trailing whitespace on the rewritten test file starting at line 1, and git show 3905781ac31f714acd41119959ae749bd64e1da4:tests/agent/test_redact.py | sed -n '1,8l' shows \r on every sampled line. This turns a small redaction/test update into a full-file whitespace rewrite and leaves the patch failing the repository whitespace check, so it should be normalized before merge.

Security evidence:

  • trust boundary: terminal/file-read output redaction protects secrets before they reach agent-visible output and logs.
  • source/sink/invariant: this follow-up review checked whether the PR's redaction fix remained reviewable and merge-hygienic after addressing the prior inline-comment bypass.
  • current-main reproduction: the same file on current main uses normal LF endings in the sampled lines, so the CRLF/trailing-whitespace churn is introduced by this PR head.
  • PR-head or patch-replay validation: the current PR head 3905781ac31f714acd41119959ae749bd64e1da4 fails git diff --check with trailing-whitespace reports across tests/agent/test_redact.py.
  • positive/negative cases: agent/redact.py is not the whitespace problem, but the test file diff is dominated by line-ending churn (1133 additions and 1048 deletions for the same test file).
  • residual bypass search: not continued past this first decisive blocker in till-first-blocker mode; additional redaction issues may remain unchecked.
  • reviewer validation: this is in scope because the PR changes security-sensitive redaction tests, and the introduced line-ending/whitespace churn makes the submitted patch fail a standard repository hygiene check before deeper security validation.

Signed: GPT-5.5-xhigh in Codex

The _HERMES_KNOWN_ENV_RE required value to reach end-of-line
((?=\s*$)), so KEY=VALUE # comment forms leaked the value.
Change the lookahead to (?=\s*(?:#.*)?$) so optional inline
# comments after the value are allowed — value still redacted,
comment preserved.

Adds 8 tests covering: plain, export, inline comment, quoted
with comment, extra spaces before comment, non-secret passthrough,
and multiline blocks with mixed comments.
@ShaoRou459
ShaoRou459 force-pushed the fix/redact-known-secret-env-vars branch from 3905781 to 8348396 Compare July 9, 2026 13:22
@ShaoRou459

Copy link
Copy Markdown
Contributor Author

Apologies about the whitespace issue, will watch out going forward.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for addressing the code_file=True terminal-output gap; current main confirms the premise at agent/redact.py:741 and agent/redact.py:549.

Problems

  • The new scanner is limited to tools/*.py, but provider credentials are declared elsewhere. For example, current main declares NOUS_API_KEY in plugins/model-providers/nous/__init__.py:42; it is absent from the PR's manual list. An opaque NOUS_API_KEY=value therefore still bypasses generic assignment redaction on the cat .env path.
  • The added tests call redact_sensitive_text with MISTRAL_API_KEY but do not cover this terminal entry point with a provider credential outside tools/*.py.

Suggested changes

  • Include authoritative provider credential declarations in the secret-name source, or add audited coverage for all supported provider env vars.
  • Add an end-to-end redact_terminal_output(..., "cat .env") regression using opaque NOUS_API_KEY content.

Automated hermes-sweeper review.

@alt-glitch alt-glitch added P3 Low — cosmetic, nice to have and removed P2 Medium — degraded but workaround exists labels Jul 10, 2026
…ar list

Following review feedback from @teknium1, replaced the manual list + AST
scanner approach with a simpler and more robust solution.

Instead of maintaining a list of ~95 known secret env var names + scanning
tools/*.py for requires_env, the fix now detects when a terminal command
reads a .env file (cat .env, head .env.local, etc.) and sets code_file=False
so the existing _ENV_ASSIGN_RE regex handles redaction. This regex already
matches any KEY=value where the key contains API_KEY, TOKEN, SECRET,
PASSWORD, CREDENTIAL, or AUTH — covering all current and future env vars
with zero list maintenance.

This also resolves the gap @teknium1 identified: NOUS_API_KEY and any other
provider credential declared outside tools/*.py is now covered automatically,
since we're no longer relying on enumerating declaration sites.

Per AGENTS.md, .env is for secrets only, so running the generic ENV redactor
on .env content is the correct behavior to prevent secret leaks. Template
files (.env.example, .env.sample, etc.) are explicitly excluded so the agent
can still read those freely.

Trade-off for this approach is agent autonomy: Because .env values are now
redacted when read through the terminal (the read_file tool already blocks
.env entirely), the agent cannot inspect or compare raw secret values in
.env files without human intervention. An agent debugging a misconfigured
API key would see OPENAI_API_KEY=*** instead of the actual key, and would
need to ask the user to verify the value. File writes (write_file, patch)
are unaffected — the agent can still create and modify .env files.
@ShaoRou459

ShaoRou459 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Following review feedback from @teknium1, replaced the manual list + AST scanner approach with a simpler and more robust solution.

Instead of maintaining a list of ~95 known secret env var names + scanning tools/*.py for requires_env, the fix now detects when a terminal command reads a .env file (cat .env, head .env.local, etc.) and sets code_file=False so the existing _ENV_ASSIGN_RE regex handles redaction. This regex already matches any KEY=value where the key contains API_KEY, TOKEN, SECRET, PASSWORD, CREDENTIAL, or AUTH — covering all current and future env vars with zero list maintenance.

This also resolves the gap @teknium1 identified: NOUS_API_KEY and any other provider credential declared outside tools/*.py is now covered automatically, since we're no longer relying on enumerating declaration sites.

Per AGENTS.md, .env is for secrets only, so running the generic ENV redactor on .env content is the correct behavior to prevent secret leaks. Template files (.env.example, .env.sample, etc.) are explicitly excluded so the agent can still read those freely.

Trade-off for this approach is agent autonomy: Because .env values are now redacted when read through the terminal (the read_file tool already blocks .env entirely), the agent cannot inspect or compare raw secret values in .env files without human intervention. An agent debugging a misconfigured API key would see OPENAI_API_KEY=*** instead of the actual key, and would need to ask the user to verify the value. File writes (write_file, patch) are unaffected — the agent can still create and modify .env files.

  • Shaorou459 & GLM5.2 & DSV4Pro

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 11, 2026
@alt-glitch alt-glitch removed the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 11, 2026
@teknium1 teknium1 added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 11, 2026
@alt-glitch alt-glitch removed the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 11, 2026
@teknium1 teknium1 added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 11, 2026
@alt-glitch alt-glitch removed the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 11, 2026
@teknium1 teknium1 added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 11, 2026
@alt-glitch alt-glitch removed the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 12, 2026
@ShaoRou459

Copy link
Copy Markdown
Contributor Author

eh? Conflict of agents
image

@teknium1 teknium1 added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 12, 2026
@alt-glitch alt-glitch removed the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 12, 2026
@teknium1 teknium1 added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 12, 2026
@alt-glitch alt-glitch removed the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 12, 2026
@teknium1 teknium1 added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 12, 2026
kshitijk4poor added a commit that referenced this pull request Aug 7, 2026
- Import file_safety._BLOCKED_PROJECT_ENV_BASENAMES instead of copying
  it (comment-enforced parallel lists drift); lookup is now
  case-insensitive to match file_safety's .lower() semantics (cat .ENV
  on macOS/Windows case-insensitive filesystems reads the same secrets).
- Strip shell quotes plain split() leaves attached (cat ".env").
- Drop the dead _ENV_FILE_EXCLUDE_SUFFIXES logic (exact-basename
  membership already excludes templates) and the stray blank-line noise.
- Document the defense-in-depth limits (sudo/full-path/substitution
  readers) mirroring is_env_dump_command's precedent, and correct the
  docstring overclaim about name-independence.
- Annotate command as str | None (tests pass None).
kshitijk4poor pushed a commit to kshitijk4poor/hermes-agent that referenced this pull request Aug 7, 2026
…ar list

Terminal output from file-read commands (cat, head, tail, ...) uses
code_file=True, which skips the generic ENV-assignment redaction pass.
Reading a .env file through the terminal therefore leaked any key whose
value has no recognized vendor prefix (Mistral, Gemini AQ.*, tvly-dev-,
bu_, Spotify client secrets).

Detect file-read commands targeting .env-style basenames (mirroring
agent/file_safety's blocked list) and route them to code_file=False so
the existing ENV pass masks opaque values. Templates (.env.example,
.env.sample, ...) are excluded.

Salvaged from NousResearch#61352 (145 commits of drift; conflict with the test-prune
wave resolved by NOT resurrecting pruned tests). Authored by @ShaoRou459.

Closes NousResearch#61352
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #80964 — your commit was cherry-picked onto current main with your authorship preserved (git log shows you as author on main).

The branch had drifted ~6000 commits; the only conflict was with the test-prune wave, resolved by not resurrecting the pruned tests (two of the carried-over fixtures had also been corrupted by the display-redaction layer into sentinel strings, so dropping them fixed that too). A small follow-up commit on top hardened the detection per review: the basename list is now imported from agent/file_safety instead of copied, the lookup is case-insensitive (cat .ENV), and shell quotes are stripped (cat ".env").

Thanks for the fix — and for iterating through the three design rounds to the routing approach.

ma1138569845 pushed a commit to ma1138569845/dechnicAuditor-agent that referenced this pull request Aug 10, 2026
…ar list

Terminal output from file-read commands (cat, head, tail, ...) uses
code_file=True, which skips the generic ENV-assignment redaction pass.
Reading a .env file through the terminal therefore leaked any key whose
value has no recognized vendor prefix (Mistral, Gemini AQ.*, tvly-dev-,
bu_, Spotify client secrets).

Detect file-read commands targeting .env-style basenames (mirroring
agent/file_safety's blocked list) and route them to code_file=False so
the existing ENV pass masks opaque values. Templates (.env.example,
.env.sample, ...) are excluded.

Salvaged from NousResearch#61352 (145 commits of drift; conflict with the test-prune
wave resolved by NOT resurrecting pruned tests). Authored by @ShaoRou459.

Closes NousResearch#61352
ma1138569845 pushed a commit to ma1138569845/dechnicAuditor-agent that referenced this pull request Aug 10, 2026
…61352

- Import file_safety._BLOCKED_PROJECT_ENV_BASENAMES instead of copying
  it (comment-enforced parallel lists drift); lookup is now
  case-insensitive to match file_safety's .lower() semantics (cat .ENV
  on macOS/Windows case-insensitive filesystems reads the same secrets).
- Strip shell quotes plain split() leaves attached (cat ".env").
- Drop the dead _ENV_FILE_EXCLUDE_SUFFIXES logic (exact-basename
  membership already excludes templates) and the stray blank-line noise.
- Document the defense-in-depth limits (sudo/full-path/substitution
  readers) mirroring is_env_dump_command's precedent, and correct the
  docstring overclaim about name-independence.
- Annotate command as str | None (tests pass None).
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
…ar list

Terminal output from file-read commands (cat, head, tail, ...) uses
code_file=True, which skips the generic ENV-assignment redaction pass.
Reading a .env file through the terminal therefore leaked any key whose
value has no recognized vendor prefix (Mistral, Gemini AQ.*, tvly-dev-,
bu_, Spotify client secrets).

Detect file-read commands targeting .env-style basenames (mirroring
agent/file_safety's blocked list) and route them to code_file=False so
the existing ENV pass masks opaque values. Templates (.env.example,
.env.sample, ...) are excluded.

Salvaged from NousResearch#61352 (145 commits of drift; conflict with the test-prune
wave resolved by NOT resurrecting pruned tests). Authored by @ShaoRou459.

Closes NousResearch#61352
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
…61352

- Import file_safety._BLOCKED_PROJECT_ENV_BASENAMES instead of copying
  it (comment-enforced parallel lists drift); lookup is now
  case-insensitive to match file_safety's .lower() semantics (cat .ENV
  on macOS/Windows case-insensitive filesystems reads the same secrets).
- Strip shell quotes plain split() leaves attached (cat ".env").
- Drop the dead _ENV_FILE_EXCLUDE_SUFFIXES logic (exact-basename
  membership already excludes templates) and the stray blank-line noise.
- Document the defense-in-depth limits (sudo/full-path/substitution
  readers) mirroring is_env_dump_command's precedent, and correct the
  docstring overclaim about name-independence.
- Annotate command as str | None (tests pass None).
sanshi2018 pushed a commit to sanshi2018/hermes-agent that referenced this pull request Aug 18, 2026
…ar list

Terminal output from file-read commands (cat, head, tail, ...) uses
code_file=True, which skips the generic ENV-assignment redaction pass.
Reading a .env file through the terminal therefore leaked any key whose
value has no recognized vendor prefix (Mistral, Gemini AQ.*, tvly-dev-,
bu_, Spotify client secrets).

Detect file-read commands targeting .env-style basenames (mirroring
agent/file_safety's blocked list) and route them to code_file=False so
the existing ENV pass masks opaque values. Templates (.env.example,
.env.sample, ...) are excluded.

Salvaged from NousResearch#61352 (145 commits of drift; conflict with the test-prune
wave resolved by NOT resurrecting pruned tests). Authored by @ShaoRou459.

Closes NousResearch#61352

(cherry picked from commit cf755f5)
sanshi2018 pushed a commit to sanshi2018/hermes-agent that referenced this pull request Aug 18, 2026
…61352

- Import file_safety._BLOCKED_PROJECT_ENV_BASENAMES instead of copying
  it (comment-enforced parallel lists drift); lookup is now
  case-insensitive to match file_safety's .lower() semantics (cat .ENV
  on macOS/Windows case-insensitive filesystems reads the same secrets).
- Strip shell quotes plain split() leaves attached (cat ".env").
- Drop the dead _ENV_FILE_EXCLUDE_SUFFIXES logic (exact-basename
  membership already excludes templates) and the stray blank-line noise.
- Document the defense-in-depth limits (sudo/full-path/substitution
  readers) mirroring is_env_dump_command's precedent, and correct the
  docstring overclaim about name-independence.
- Annotate command as str | None (tests pass None).

(cherry picked from commit 15d7103)
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 P3 Low — cosmetic, nice to have sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit 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.

5 participants