Skip to content

security: force redaction on in gateway mode, protect config.yaml writes - #8738

Closed
Dylanwooo wants to merge 1 commit into
NousResearch:mainfrom
Dylanwooo:fix/redact-secrets-gateway-hardening
Closed

security: force redaction on in gateway mode, protect config.yaml writes#8738
Dylanwooo wants to merge 1 commit into
NousResearch:mainfrom
Dylanwooo:fix/redact-secrets-gateway-hardening

Conversation

@Dylanwooo

Copy link
Copy Markdown

Summary

The security.redact_secrets: false config option disables all secret redaction globally — logs, tool output, LLM context, and outbound messages — with a single boolean flip. This creates two concrete risks:

  1. Gateway multi-user exposure: In gateway mode (Telegram/Discord/Slack), disabling redaction leaks API keys, bot tokens, and credentials into LLM context that can be reflected to any connected user.
  2. Prompt injection → persistent config write: ~/.hermes/config.yaml is not protected by approval.py, so an LLM manipulated via prompt injection can write security: { redact_secrets: false } to config.yaml. The change takes effect on next startup, silently disabling all 11 redaction call sites.

What this PR does

1. Force redaction ON in gateway mode (agent/redact.py)

Gateway serves multiple users over messaging platforms — secrets in LLM context risk being reflected to any connected user. The config option is now ignored in gateway mode:

_IS_GATEWAY = os.getenv("HERMES_GATEWAY_SESSION", "").lower() in ("1", "true", "yes")
_USER_DISABLED = os.getenv("HERMES_REDACT_SECRETS", "").lower() in ("0", "false", "no", "off")
_REDACT_ENABLED = True if _IS_GATEWAY else not _USER_DISABLED

When redaction is disabled (CLI mode) or overridden (gateway mode), a WARNING is now logged so administrators know the state.

2. Protect config.yaml from unreviewed writes (tools/approval.py)

approval.py already protects ~/.hermes/.env from shell writes (tee, redirection). This PR extends the same protection to ~/.hermes/config.yaml, which contains security settings (redact_secrets, tirith_enabled, etc.) that control the agent's own safety mechanisms.

3. Clarify gateway comment (gateway/run.py)

Documents why the config value is still read even though gateway forces redaction — so redact.py can log the override warning.

Why this doesn't affect agent capability

Redaction operates at the output text layer, not the permission layer:

  • The agent can still read any file, run any command, make any API call — unchanged
  • Environment variables used by subprocesses ($OPENAI_API_KEY in curl, etc.) are the real values from the process env, never touched by redaction
  • The partial mask (sk-pro...l012 — first 6 + last 4 chars) is sufficient for the LLM to confirm a key exists, distinguish between different keys, and diagnose format/provider errors
  • CLI mode still respects redact_secrets: false for local debugging

Design notes

The existing import-time snapshot (redact.py:16-18) correctly prevents runtime export mutations from disabling redaction mid-session — this shows the threat was anticipated. This PR closes two remaining gaps:

  • The config.yaml persistence path (write config → restart → redaction off)
  • Gateway mode not being treated as a higher trust boundary

Tests

12 new tests across 2 files:

  • tests/agent/test_redact.pyTestGatewayForcesRedaction: 4 tests verifying gateway override, env var variations, CLI fallback, and default behavior
  • tests/tools/test_approval.pyTestTeePattern + TestSensitiveRedirectConfigYaml + TestSensitiveRedirectPattern: 8 tests verifying config.yaml write detection via tee, redirect, append, and that unrelated config.yaml paths are not flagged

All existing tests pass unchanged (47 redact + 126 approval = 173 total).

The `security.redact_secrets: false` config option disables all secret
redaction globally — logs, tool output, LLM context, and outbound
messages — with a single boolean flip. This creates two risks:

1. Gateway multi-user exposure: in gateway mode, disabling redaction
   leaks API keys and credentials into LLM context that can be
   reflected to any connected user.

2. Prompt injection persistence: ~/.hermes/config.yaml was not
   protected by the approval system, so an LLM manipulated via
   prompt injection could write `security: { redact_secrets: false }`
   and silently disable all 11 redaction call sites on next startup.

Changes:
- agent/redact.py: detect HERMES_GATEWAY_SESSION and force
  _REDACT_ENABLED=True regardless of config; log warnings when
  redaction is disabled or overridden
- tools/approval.py: add ~/.hermes/config.yaml to _SENSITIVE_WRITE_TARGET
  so shell writes require the same approval as .env
- gateway/run.py: clarify comment on why config value is still read
- tests: 12 new tests covering gateway override logic and config.yaml
  approval detection
@alt-glitch alt-glitch added type/security Security vulnerability or hardening P1 High — major feature broken, no workaround comp/gateway Gateway runner, session dispatch, delivery area/config Config system, migrations, profiles labels Apr 28, 2026

@egilewski egilewski 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.

Recommendation: request changes.

I reviewed this against current GitHub main 2c174bce2408f5d6810b0f16dbd55cb65cd1c6e3, PR base 15b1a3aa69da339124f6fbbfd08c2cc27c00bc2e, and PR head 84cb59ad04e09fb63abb1faef2c8ef8496fefd3f.

Validation:

  • gh pr checks 8738 --repo NousResearch/hermes-agent: no checks were reported, so there was no completed failing PR-specific check to gate on.
  • git diff --check refs/remotes/upstream/main...refs/remotes/upstream/pr/8738: passed.
  • git merge-tree --write-tree refs/remotes/upstream/main refs/remotes/upstream/pr/8738: failed with content conflicts in agent/redact.py, tools/approval.py, and tests/tools/test_approval.py.

Finding:
The PR currently cannot be reviewed or merged safely against current main because the security-sensitive files it changes conflict with the live branch. I stopped at this current-main mergeability gate, so the redaction and config-write behavior still need security validation after the branch is updated.

Signed: GPT-5.5-xhigh in Codex

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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 Jun 29, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for this, @Dylanwooo — the security thinking here is sound, and the threat model (gateway multi-user exposure + config.yaml as a prompt-injection persistence path) is exactly right. Closing it, though, because each half doesn't land the way it needs to:

1. Force redaction in gateway mode — gates on the wrong env var.
The patch keys off HERMES_GATEWAY_SESSION, but the real messaging gateway (Telegram/Discord/Slack — the multi-user surface this protects) sets _HERMES_GATEWAY=1 and never sets HERMES_GATEWAY_SESSION. That variable is only set by the local single-user TUI backend (tui_gateway/server.py), which approval.py itself documents as the "legacy" integration. Verified empirically: in the actual messaging-gateway env (_HERMES_GATEWAY=1, HERMES_REDACT_SECRETS=false), _REDACT_ENABLED evaluates to False — redaction stays off. So the protection is a no-op in the exact environment it's meant for, and only activates in the single-user TUI where the leak threat doesn't apply.

2. Protect config.yaml from unreviewed writes — already on main, more thoroughly.

  • write_file / patch already refuse writes to ~/.hermes/config.yaml via _check_sensitive_path() in tools/file_tools.py.
  • The shell-redirection path you added (tee / > / >> to config.yaml in approval.py) also already exists on current main as _HERMES_CONFIG_PATH, wired into _SENSITIVE_WRITE_TARGET. Current coverage is broader than this PR's — it also catches sed -i, cp/mv/install, and project-local config.yaml writes. All of the test cases this PR adds pass against main as-is.

Given the env-var mismatch in part 1 and part 2 being fully superseded, there's nothing left to merge. Appreciate the careful write-up and the defense-in-depth instinct — please keep them coming.

@teknium1 teknium1 closed this Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/gateway Gateway runner, session dispatch, delivery P1 High — major feature broken, no workaround 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.

4 participants