mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-20 15:33:54 +00:00
fix(webhook): reject a non-ASCII signature header instead of crashing 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.
This commit is contained in:
parent
efb6c21498
commit
1b69c47e97
2 changed files with 53 additions and 5 deletions
|
|
@ -125,6 +125,20 @@ def _is_loopback_host(host: Optional[str]) -> bool:
|
|||
return host.strip().lower() in _LOOPBACK_HOSTS
|
||||
|
||||
|
||||
def _hmac_str_equal(provided: str, expected: str) -> bool:
|
||||
"""Timing-safe equality for two ``str`` values, tolerant of non-ASCII input.
|
||||
|
||||
``hmac.compare_digest`` raises ``TypeError`` when given a ``str`` that
|
||||
contains non-ASCII characters. The ``provided`` value here is an
|
||||
attacker-controlled signature/token header on a public, unauthenticated
|
||||
webhook endpoint, so a single non-ASCII byte would otherwise raise out of
|
||||
the request handler and return a 500 instead of rejecting the request.
|
||||
Comparing as UTF-8 bytes keeps the constant-time guarantee while making a
|
||||
hostile header fail closed with a clean rejection.
|
||||
"""
|
||||
return hmac.compare_digest(provided.encode(), expected.encode())
|
||||
|
||||
|
||||
def check_webhook_requirements() -> bool:
|
||||
"""Check if webhook adapter dependencies are available."""
|
||||
return AIOHTTP_AVAILABLE
|
||||
|
|
@ -969,12 +983,12 @@ class WebhookAdapter(BasePlatformAdapter):
|
|||
expected = "sha256=" + hmac.new(
|
||||
secret.encode(), body, hashlib.sha256
|
||||
).hexdigest()
|
||||
return hmac.compare_digest(gh_sig, expected)
|
||||
return _hmac_str_equal(gh_sig, expected)
|
||||
|
||||
# GitLab: X-Gitlab-Token = <plain secret>
|
||||
gl_token = request.headers.get("X-Gitlab-Token", "")
|
||||
if gl_token:
|
||||
return hmac.compare_digest(gl_token, secret)
|
||||
return _hmac_str_equal(gl_token, secret)
|
||||
|
||||
# Generic V2: X-Webhook-Signature-V2 = <hex HMAC-SHA256 of "<timestamp>.<body>">
|
||||
# X-Webhook-Timestamp = <unix seconds> (required for V2)
|
||||
|
|
@ -1017,7 +1031,7 @@ class WebhookAdapter(BasePlatformAdapter):
|
|||
expected_v2 = hmac.new(
|
||||
secret.encode(), signed_content, hashlib.sha256
|
||||
).hexdigest()
|
||||
return hmac.compare_digest(v2_sig, expected_v2)
|
||||
return _hmac_str_equal(v2_sig, expected_v2)
|
||||
|
||||
# Generic V1 (legacy): X-Webhook-Signature = <hex HMAC-SHA256 of body>
|
||||
# (deprecated — no replay protection, since the signature only
|
||||
|
|
@ -1041,7 +1055,7 @@ class WebhookAdapter(BasePlatformAdapter):
|
|||
"'<timestamp>.<body>').",
|
||||
route_name,
|
||||
)
|
||||
return hmac.compare_digest(generic_sig, expected)
|
||||
return _hmac_str_equal(generic_sig, expected)
|
||||
|
||||
# No recognised signature header but secret is configured → reject
|
||||
logger.debug(
|
||||
|
|
@ -1095,7 +1109,7 @@ class WebhookAdapter(BasePlatformAdapter):
|
|||
version, signature = part.split(",", 1)
|
||||
except ValueError:
|
||||
continue
|
||||
if version == "v1" and hmac.compare_digest(signature, expected):
|
||||
if version == "v1" and _hmac_str_equal(signature, expected):
|
||||
return True
|
||||
return False
|
||||
|
||||
|
|
|
|||
|
|
@ -166,6 +166,40 @@ class TestValidateSignature:
|
|||
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):
|
||||
"""The signature headers are attacker-controlled on a public, unauth
|
||||
endpoint. A non-ASCII byte in one must be rejected (False), not crash
|
||||
the handler: hmac.compare_digest raises TypeError on a non-ASCII str."""
|
||||
adapter = _make_adapter()
|
||||
body = b'{"action": "opened"}'
|
||||
secret = "webhook-secret-42"
|
||||
hostile = "ské-not-a-valid-signature"
|
||||
for header in (
|
||||
"X-Hub-Signature-256",
|
||||
"X-Gitlab-Token",
|
||||
"X-Webhook-Signature",
|
||||
):
|
||||
req = _mock_request(headers={header: hostile})
|
||||
# Must return False, never raise.
|
||||
assert adapter._validate_signature(req, body, secret) is False
|
||||
|
||||
def test_non_ascii_generic_v2_signature_rejected(self):
|
||||
"""V2 branch (timestamp-bound) also rejects a non-ASCII signature."""
|
||||
adapter = _make_adapter()
|
||||
req = _mock_request(headers={
|
||||
"X-Webhook-Signature-V2": "ské-bad",
|
||||
"X-Webhook-Timestamp": str(int(time.time())),
|
||||
})
|
||||
assert adapter._validate_signature(req, b"{}", "secret") is False
|
||||
|
||||
def test_non_ascii_secret_still_validates_a_matching_token(self):
|
||||
"""A non-ASCII configured secret must still match its exact GitLab
|
||||
token value byte for byte (bytes comparison keeps this working)."""
|
||||
adapter = _make_adapter()
|
||||
secret = "gl-tökén-välue"
|
||||
req = _mock_request(headers={"X-Gitlab-Token": secret})
|
||||
assert adapter._validate_signature(req, b"{}", secret) is True
|
||||
|
||||
def test_validate_no_secret_allows_all(self):
|
||||
"""When the secret is empty/falsy, the validator is never even called
|
||||
by the handler (secret check is 'if secret and secret != _INSECURE...').
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue