Skip to content

fix(peer): route hermes peer dm through the credential-redirect guard - #88819

Closed
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/peer-dm-credential-redirect-leak
Closed

pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/peer-dm-credential-redirect-leak

Conversation

@pierrenode

Copy link
Copy Markdown
Contributor

What

hermes_cli/subcommands/peer.py's _request() (the shared HTTP helper for hermes peer add/list/dm) sends the peer's Authorization: Bearer <API_SERVER_KEY> via a raw urllib.request.urlopen() call. Python's default HTTPRedirectHandler preserves all request headers — including Authorization — across a 3xx redirect, even when the redirect target is a different origin than the URL registered with hermes peer add.

Why it matters

A compromised peer gateway, or a LAN MITM answering the peer's registered URL, can respond with a redirect to an attacker-controlled host and harvest the peer's API key. This isn't a theoretical/manual-only path either: bots on a Bot-Mode-managed install are told (via tools/bot_mode_probe.py's injected "Messaging other agents" protocol section) to use hermes peer dm <peer>/<agent> "..." autonomously to message teammates on other machines, so the leak can be triggered without a human in the loop.

This is the exact credential-redirect-leak class this repo has closed repeatedly in other outbound HTTP call sites (providers/base.py, hermes_cli/models.py, hermes_cli/azure_detect.py, the Anthropic adapter, plugins/model-providers/actual) via hermes_cli/urllib_security.py::open_credentialed_url(), which strips non-safelisted headers whenever a redirect crosses origin. peer.py — added in this same window as the new hermes peer/Bot Mode cross-machine messaging feature — never adopted it.

Fix

Route _request() through open_credentialed_url() instead of urlopen() directly. It's a drop-in swap: both accept the same pre-built Request object and timeout kwarg; only the redirect-handling opener changes.

Testing

  • Added test_request_strips_bearer_key_across_redirect_origin to tests/hermes_cli/test_peer_cmd.py, using two real loopback HTTPServer instances (the "peer," which 302-redirects every request, and a stand-in attacker origin) to prove the Authorization header does not reach the redirect target.
  • Mutation-verified: reverting the one-line fix makes the new test fail with the key present at the attacker origin (peer's Bearer key leaked to the redirect target: ['Bearer top-secret-peer-key']).
  • Full tests/hermes_cli/test_peer_cmd.py (11 tests) and tests/hermes_cli/test_urllib_security.py (16 tests) pass together.
  • ruff check clean on both changed files.

Checklist

  • Tests added/updated
  • Mutation-verified (fix reverted → new test fails)
  • No behavior change for the non-redirect (common) case — same headers, same body, same timeout semantics

hermes_cli/subcommands/peer.py's _request() sends the peer's
Authorization: Bearer <API_SERVER_KEY> via a raw urllib.request.urlopen()
call. Python's default HTTPRedirectHandler preserves all request headers
across a 3xx redirect, including Authorization, even when the redirect
crosses origins. A compromised peer gateway (or a LAN MITM answering the
URL registered with `hermes peer add`) can redirect to an attacker-
controlled host and harvest the peer key -- which bots also send
autonomously via `hermes peer dm`, per the Bot Mode messaging protocol
injected by tools/bot_mode_probe.py.

This is the same credential-redirect-leak class this repo has closed
repeatedly elsewhere (providers/base.py, hermes_cli/models.py,
azure_detect.py, the anthropic adapter, plugins/model-providers/actual)
via hermes_cli/urllib_security.py::open_credentialed_url(), which strips
non-safelisted headers whenever a redirect crosses origin. peer.py never
adopted it.

Route _request() through open_credentialed_url() -- a drop-in swap for
urlopen() since it accepts the same pre-built Request object.

Added a regression test using two real loopback HTTP servers (the peer
and a stand-in attacker origin): a peer that 302-redirects every request
must not leak the Bearer key to the redirect target. Mutation-verified:
reverting the fix makes the new test fail with the key present at the
attacker origin.
@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/cli CLI entry point, hermes_cli/, setup wizard 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 needs-repro Bug needs reproduction steps labels Aug 18, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

Correct and well-proven: urlopen follows redirects with the Authorization header attached, so a compromised/MITM'd peer could harvest the Bearer key via one 302; routing through open_credentialed_url closes it, and the test is the gold standard — two real HTTP servers, a successful redirect to an attacker origin, and an assertion that the key never arrived.

— reviewer-b (automated review)

No blocking issues found.

@dokterdok

Copy link
Copy Markdown
Contributor

@pierrenode I am composing this credential-redirect fix into draft #95965 because RoomLink depends on the same peer HTTP boundary. Your commit and authorship are preserved unchanged. If this lands first, I will drop the duplicate patch from the composition.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks @pierrenode — this landed on main in e743391: hermes_cli/subcommands/peer.py::_request now opens every peer request through hermes_cli.urllib_security.open_credentialed_url, which strips the Bearer header across a cross-origin redirect — exactly what this PR proposed. Closing as superseded, with credit.

@teknium1 teknium1 closed this Sep 20, 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/cli CLI entry point, hermes_cli/, setup wizard needs-repro Bug needs reproduction steps P2 Medium — degraded but workaround exists 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