Skip to content

fix(webhook): reject a non-ASCII signature header instead of crashing the endpoint - #65307

Closed
Drexuxux wants to merge 3 commits into
NousResearch:mainfrom
Drexuxux:fix/webhook-signature-nonascii-crash
Closed

fix(webhook): reject a non-ASCII signature header instead of crashing the endpoint#65307
Drexuxux wants to merge 3 commits into
NousResearch:mainfrom
Drexuxux:fix/webhook-signature-nonascii-crash

Conversation

@Drexuxux

Copy link
Copy Markdown
Contributor

What?

_validate_signature backs the public webhook receiver. It compared each attacker-supplied signature/token header (GitHub X-Hub-Signature-256, GitLab X-Gitlab-Token, generic X-Webhook-Signature/-V2, and the Svix v1 header) against a computed digest with hmac.compare_digest on two str values. compare_digest raises TypeError on a str containing non-ASCII characters, and the header is raw client input on an unauthenticated endpoint — so any internet client can POST a single non-ASCII byte in the signature header and raise out of the handler, returning a 500 instead of a clean 401. Fail-closed, but an on-demand crash of the request path.

Solution

Route all five comparisons through a small _hmac_str_equal() helper that encodes both sides to UTF-8 bytes before the constant-time compare (compare_digest has no ASCII restriction on bytes). Semantics are unchanged for valid signatures; a hostile non-ASCII header now fails closed with a rejection.

How to test

  1. pytest tests/gateway/test_webhook_adapter.py::TestValidateSignature -q
  2. New tests: non-ASCII GitHub/GitLab/generic/V2 signature headers return False (no raise); a non-ASCII configured secret still matches its exact token.
  3. Re-run against pre-fix code to confirm they raise TypeError there.

Test results

  • TestValidateSignature — 26 passed (23 existing + 3 new).
  • Pre-fix, the 3 new tests fail with TypeError: comparing strings with non-ASCII characters is not supported; post-fix they reject / validate correctly.
  • test_webhook_adapter.py + test_webhook_signature_rate_limit.py + test_msgraph_webhook.py — 120 passed, no regressions.

… the endpoint

_validate_signature backs the public webhook receiver. It compared each
attacker-supplied signature/token header (GitHub X-Hub-Signature-256,
GitLab X-Gitlab-Token, generic X-Webhook-Signature / -V2, and the Svix v1
header) against a computed hex/base64 digest with hmac.compare_digest on
two str values. compare_digest raises TypeError on a str containing
non-ASCII characters, and the header is raw client input on an
unauthenticated endpoint — so any internet client could POST a single
non-ASCII byte in the signature header and raise out of the handler,
returning a 500 instead of a clean 401. Fail-closed, but an on-demand
crash of the request path.

Route all five comparisons through a small _hmac_str_equal() helper that
encodes both sides to UTF-8 bytes before the constant-time compare
(compare_digest has no ASCII restriction on bytes). Semantics are
unchanged for valid signatures; a hostile non-ASCII header now fails
closed with a rejection instead of raising.

Adds regression tests: non-ASCII GitHub/GitLab/generic/V2 signature
headers return False (no raise), and a non-ASCII configured secret still
matches its exact token value.

Also maps drexux0@gmail.com in scripts/release.py AUTHOR_MAP.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/webhook Webhook / API server area/auth Authentication, OAuth, credential pools sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jul 16, 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 isolating the generic webhook fix and preserving constant-time comparison semantics. Current main still has the five direct str comparisons in gateway/platforms/webhook.py:930, :935, :978, :1002, and :1056; the PR replaces each one, and the handler's clean rejection path is at gateway/platforms/webhook.py:535-541.

Problems

  • The same public-header pattern remains in gateway/platforms/whatsapp_cloud.py: it reads X-Hub-Signature-256 at :1464 and directly compares the derived string at :1512. That leaves the reported failure class in a sibling webhook endpoint.

Suggested changes

  • Harden the WhatsApp Cloud path and add its non-ASCII rejection regression; audit the analogous authenticated string comparisons in gateway/platforms/msgraph_webhook.py:361 and gateway/platforms/api_server.py:1219.
  • Add a Svix non-ASCII regression: this PR changes the Svix comparison, but the new tests do not enter that branch.

Automated hermes-sweeper review.

req = _mock_request(headers={}) # no sig headers at all
assert adapter._validate_signature(req, b"{}", "my-secret") is False

def test_non_ascii_signature_headers_reject_without_raising(self):

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.

This covers GitHub, GitLab, generic V1, and generic V2, but the patch also changes the Svix v1 comparison. Please add a non-ASCII svix-signature case with valid svix-id and timestamp so that branch is regression-tested too.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 16, 2026
Drexuxux added 2 commits July 16, 2026 15:14
…gression

The fix routes the Svix v1 comparison through _hmac_str_equal too, but the
existing non-ASCII tests only exercised the GitHub/GitLab/generic V1/V2
branches. Add a Svix case (valid svix-id + fresh svix-timestamp so it
reaches the v1,<sig> compare) with a non-ASCII signature, which raised
TypeError before the fix and now rejects cleanly.
…-nonascii-crash

# Conflicts:
#	scripts/release.py
teknium1 added a commit that referenced this pull request Jul 16, 2026
…g sites

Same bug class as the salvaged #65305/#65307: hmac.compare_digest (and
secrets.compare_digest) raise TypeError when given a str containing
non-ASCII characters, and these call sites feed it raw request input.
Compare as UTF-8 bytes everywhere:

- gateway/platforms/msgraph_webhook.py: clientState from request body
- gateway/platforms/whatsapp_cloud.py: hub.verify_token query param +
  X-Hub-Signature-256 header (comment claimed 'works on str' — it
  doesn't for non-ASCII)
- plugins/platforms/feishu: verification token + x-lark-signature
- plugins/platforms/raft: bridge token header
- plugins/platforms/line: X-Line-Signature
- plugins/platforms/sms: X-Twilio-Signature
- tools/code_execution_tool.py: sandbox RPC token (both loops)

Regression tests for the two gateway-core sites (msgraph, whatsapp).
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #65697 (together with #65305) — your commits were cherry-picked onto current main with your authorship preserved in git log (rebase-merge), including the Svix v1 test coverage commit. We widened the fix to the remaining sibling compare_digest sites in a follow-up commit. Thanks!

@teknium1 teknium1 closed this Jul 16, 2026
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
…g sites

Same bug class as the salvaged NousResearch#65305/NousResearch#65307: hmac.compare_digest (and
secrets.compare_digest) raise TypeError when given a str containing
non-ASCII characters, and these call sites feed it raw request input.
Compare as UTF-8 bytes everywhere:

- gateway/platforms/msgraph_webhook.py: clientState from request body
- gateway/platforms/whatsapp_cloud.py: hub.verify_token query param +
  X-Hub-Signature-256 header (comment claimed 'works on str' — it
  doesn't for non-ASCII)
- plugins/platforms/feishu: verification token + x-lark-signature
- plugins/platforms/raft: bridge token header
- plugins/platforms/line: X-Line-Signature
- plugins/platforms/sms: X-Twilio-Signature
- tools/code_execution_tool.py: sandbox RPC token (both loops)

Regression tests for the two gateway-core sites (msgraph, whatsapp).
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
…g sites

Same bug class as the salvaged NousResearch#65305/NousResearch#65307: hmac.compare_digest (and
secrets.compare_digest) raise TypeError when given a str containing
non-ASCII characters, and these call sites feed it raw request input.
Compare as UTF-8 bytes everywhere:

- gateway/platforms/msgraph_webhook.py: clientState from request body
- gateway/platforms/whatsapp_cloud.py: hub.verify_token query param +
  X-Hub-Signature-256 header (comment claimed 'works on str' — it
  doesn't for non-ASCII)
- plugins/platforms/feishu: verification token + x-lark-signature
- plugins/platforms/raft: bridge token header
- plugins/platforms/line: X-Line-Signature
- plugins/platforms/sms: X-Twilio-Signature
- tools/code_execution_tool.py: sandbox RPC token (both loops)

Regression tests for the two gateway-core sites (msgraph, whatsapp).
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/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/webhook Webhook / API server sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants