Skip to content

fix(proxy): strip x-api-key and provider-specific auth headers from hop-by-hop list - #73640

Open
necoweb3 wants to merge 1 commit into
NousResearch:mainfrom
necoweb3:fix/proxy-auth-header-leak
Open

fix(proxy): strip x-api-key and provider-specific auth headers from hop-by-hop list#73640
necoweb3 wants to merge 1 commit into
NousResearch:mainfrom
necoweb3:fix/proxy-auth-header-leak

Conversation

@necoweb3

@necoweb3 necoweb3 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

hermes_cli/proxy/server.py forwards HTTP requests to local model servers. The _HOP_BY_HOP_HEADERS set strips only the standard Authorization header, so non-standard provider auth headers (x-api-key, api-key, x-goog-api-key, cookie, set-cookie, and others) are passed through unchanged to the upstream.

Impact

An attacker controlling the proxy target (or eavesdropping on the proxy->upstream link) can harvest these credentials. OpenAI-compatible clients use Authorization (already stripped), but non-standard clients (Anthropic-style x-api-key, Google x-goog-api-key) are exposed.

Fix

Add all known provider-specific auth headers plus cookie/set-cookie to _HOP_BY_HOP_HEADERS. Defense-in-depth: even if a client sends x-api-key to the proxy, the proxy strips it before forwarding.

Tests

  • 39/39 tests/proxy/ tests pass
  • All new headers verified in test_set_hop_by_hop_headers_strips_credentials

…l leak

The proxy replaces the client's Authorization header with its own resolved
bearer token, but did not strip provider-specific auth headers (x-api-key,
api-key, x-goog-api-key, etc.). A client sending one of these headers would
have the real API key forwarded to the upstream server.

Add all known provider-specific auth headers and cookie/set-cookie to
_HOP_BY_HOP_HEADERS so they are stripped before forwarding.
@alt-glitch alt-glitch added type/security Security vulnerability or hardening P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard area/auth Authentication, OAuth, credential pools needs-repro Bug needs reproduction steps sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jul 28, 2026

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

Thanks for addressing the inbound credential-forwarding path; current main does copy non-denylisted request headers upstream through hermes_cli/proxy/server.py:65-72 and uses them at hermes_cli/proxy/server.py:144-145.

Problems

  • The stated new credential-header test is not present in PR #73640: its diff modifies only hermes_cli/proxy/server.py. The existing integration test at tests/hermes_cli/test_proxy.py:344-365 verifies only Authorization, and the fake upstream at tests/hermes_cli/test_proxy.py:293-302 does not record the added header names.

Suggested changes

  • Add a real proxy-to-fake-upstream regression test in tests/hermes_cli/test_proxy.py that records and rejects each newly denylisted credential header while retaining the adapter-generated Authorization header.

This is an automated hermes-sweeper review.

"authorization", # we replace this one
# Provider-specific auth headers that could carry client credentials.
"x-api-key", # Anthropic native, Google AI Studio
"api-key", # Azure OpenAI

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.

Please add the corresponding real proxy-to-upstream regression test. The existing test only proves Authorization replacement (tests/hermes_cli/test_proxy.py:344-365), and this PR's diff contains no test change for the newly denylisted headers.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@egilewski

Copy link
Copy Markdown
Contributor

looks mergeable

Reviewed the local OAuth proxy change. The request-forwarding sink now filters client Authorization, provider-specific credential, and session-cookie headers case-insensitively before inserting the adapter bearer, while ordinary request metadata remains forwarded. Current-main comparison confirmed the prior omission of x-api-key and api-key, and validation of the change confirmed removal of every added credential header. No alternate header-forwarding path or bypass was found in the proxy package; no source-backed security finding remains.

Security evidence:

  • trust boundary: Client-controlled headers enter the proxy request handler; filtering occurs immediately before the upstream request. The adapter credential is the trusted proxy-side secret and is inserted as the upstream Authorization header.
  • source/sink/invariant: The required invariant is that client Authorization, provider credential headers, and session cookies do not reach the upstream, while the adapter bearer is the only injected auth header. Header names are matched case-insensitively before the sink receives the filtered map and the resolved credential is applied.
  • current-main reproduction: Current main omits x-api-key and api-key from the request filter, reproducing the client-credential forwarding gap addressed by the patch.
  • PR-head or patch-replay validation: Validation of the reviewed change removed all newly listed auth and cookie names case-insensitively while preserving ordinary Content-Type and X-Trace metadata.
  • positive/negative cases: Positive cases covered mixed-case Authorization, x-api-key, api-key, x-goog-api-key, x-api-token, x-auth-token, x-access-token, apikey, Cookie, and Set-Cookie; none remained in the filtered map. Negative cases covered Content-Type and X-Trace, both of which remained. Response filtering uses the same set, so session cookies are also removed from upstream responses.
  • residual bypass search: The proxy package has one request-header forwarding call and one sink; the handler filters request headers before the upstream request. Header-name matching is case-insensitive. Query strings and bodies are forwarded verbatim by design, but the registered adapters authenticate with the injected bearer and expose no query/body credential path; no alternate provider-auth bypass was found.
  • reviewer validation: Static source inspection, diff validation, source compilation, current-main comparison, and header-filter validation all completed successfully.

Review setup: I reviewed a run-owned local rebase or patch replay against current GitHub main because the submitted branch is stale or conflicted; this does not mean the submitted branch itself merges cleanly.

Not checked:

  • Pytest validation
  • Ruff validation
  • CodeRabbit review

Signed: GPT-5.6-luna-max in Codex

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 P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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