diff --git a/gateway/platforms/webhook.py b/gateway/platforms/webhook.py index f6080c57dcd..50b59df8948 100644 --- a/gateway/platforms/webhook.py +++ b/gateway/platforms/webhook.py @@ -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( diff --git a/tests/gateway/test_webhook_adapter.py b/tests/gateway/test_webhook_adapter.py index ee5550ba041..a1e235f46e6 100644 --- a/tests/gateway/test_webhook_adapter.py +++ b/tests/gateway/test_webhook_adapter.py @@ -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,