mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-15 14:22:43 +00:00
fix(webhook): reject generic V2 signature missing timestamp instead of falling back to V1
This commit is contained in:
parent
ebfc49c4d9
commit
d577408f3f
2 changed files with 51 additions and 2 deletions
|
|
@ -888,9 +888,28 @@ class WebhookAdapter(BasePlatformAdapter):
|
|||
# Checked independently of (and before) legacy V1 below — a sender
|
||||
# that only ever sends V2 headers must still validate here; nesting
|
||||
# this inside `if generic_sig:` would silently skip V2-only senders.
|
||||
#
|
||||
# The presence of X-Webhook-Signature-V2 alone selects V2 mode and
|
||||
# commits to it — it must NOT fall through to the V1 branch just
|
||||
# because the timestamp is missing/malformed/expired. A sender
|
||||
# migrating to V2 typically sends both V1 and V2 headers together
|
||||
# for compatibility; if incomplete V2 fell through to V1, an
|
||||
# attacker who captured one such mixed request could strip the
|
||||
# X-Webhook-Timestamp header from a replay and have it validate
|
||||
# against the still-present, still-unprotected V1 signature instead
|
||||
# — silently downgrading a V2-protected request back to the replay
|
||||
# hole V2 exists to close.
|
||||
v2_sig = request.headers.get("X-Webhook-Signature-V2", "")
|
||||
v2_timestamp = request.headers.get("X-Webhook-Timestamp", "")
|
||||
if v2_sig and v2_timestamp:
|
||||
if v2_sig:
|
||||
v2_timestamp = request.headers.get("X-Webhook-Timestamp", "")
|
||||
if not v2_timestamp:
|
||||
logger.warning(
|
||||
"[webhook] Route '%s' sent X-Webhook-Signature-V2 with "
|
||||
"no X-Webhook-Timestamp — rejecting rather than "
|
||||
"falling back to legacy V1",
|
||||
request.match_info.get("route_name", ""),
|
||||
)
|
||||
return False
|
||||
try:
|
||||
ts = int(v2_timestamp)
|
||||
except (TypeError, ValueError):
|
||||
|
|
@ -911,6 +930,8 @@ class WebhookAdapter(BasePlatformAdapter):
|
|||
# (deprecated — no replay protection, since the signature only
|
||||
# covers the body: a captured (body, signature) pair replays
|
||||
# indefinitely with no timestamp binding it to a specific delivery.)
|
||||
# Only reachable when X-Webhook-Signature-V2 was not sent at all —
|
||||
# see the guard above.
|
||||
generic_sig = request.headers.get("X-Webhook-Signature", "")
|
||||
if generic_sig:
|
||||
expected = hmac.new(
|
||||
|
|
|
|||
|
|
@ -277,6 +277,34 @@ class TestValidateSignature:
|
|||
})
|
||||
assert adapter._validate_signature(req, body, secret) is True
|
||||
|
||||
def test_validate_generic_v2_stripped_timestamp_does_not_downgrade_to_v1(self):
|
||||
"""Regression test for a downgrade attack found in review: a sender
|
||||
migrating to V2 typically sends BOTH the V1 and V2 signatures
|
||||
together (for compatibility while both ends update). If an
|
||||
attacker captures one such mixed request and replays it with the
|
||||
X-Webhook-Timestamp header stripped, the presence of
|
||||
X-Webhook-Signature-V2 must still commit to V2 validation and
|
||||
reject — it must NOT silently fall through to validating the
|
||||
still-present, still-unprotected V1 signature instead. Falling
|
||||
through would let an attacker downgrade a V2-protected request
|
||||
back into the exact replay hole V2 exists to close, just by
|
||||
deleting one header from a captured request."""
|
||||
adapter = _make_adapter()
|
||||
body = b'{"event": "push"}'
|
||||
secret = "generic-secret"
|
||||
timestamp = str(int(time.time()))
|
||||
v2_sig = _generic_v2_signature(body, secret, timestamp)
|
||||
v1_sig = _generic_signature(body, secret)
|
||||
# Simulates a captured mixed V1+V2 request replayed with the
|
||||
# timestamp header stripped — V1 signature is still valid on its
|
||||
# own, but must not be reachable via this path.
|
||||
req = _mock_request(headers={
|
||||
"X-Webhook-Signature-V2": v2_sig,
|
||||
"X-Webhook-Signature": v1_sig,
|
||||
# X-Webhook-Timestamp deliberately omitted.
|
||||
})
|
||||
assert adapter._validate_signature(req, body, secret) is False
|
||||
|
||||
def test_v1_replay_attack_succeeds_demonstrating_the_hole_v2_closes(self):
|
||||
"""Regression/documentation test: a captured (body, signature) V1
|
||||
pair replays successfully no matter how much time has passed,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue