feat(webhook): Standard Webhooks senders now authenticate (fixes #47451, salvage #47849) - #92024
feat(webhook): Standard Webhooks senders now authenticate (fixes #47451, salvage #47849)#92024teknium1 wants to merge 1 commit into
Conversation
૮ >ﻌ< ა ci reviewran on 3a63116 — feat(webhook): accept standard webhook signatures
|
andrexibiza
left a comment
There was a problem hiding this comment.
Blocking review (GitHub will not permit this identity to set REQUEST_CHANGES without explicit repository review access).
1. GitLab's documented dual-token migration becomes a 401 regression
_validate_signature() now treats the presence of any svix-* / webhook-* field as authoritative and returns immediately through _validate_svix_signature() before the existing X-Gitlab-Token branch. That is not backward-compatible with the migration state GitLab explicitly documents.
GitLab 19.x supports a legacy Secret token (X-Gitlab-Token) and a separate Standard Webhooks Signing token (whsec_<base64>). Their docs recommend configuring both during migration, then updating the receiver, then removing the old token: https://docs.gitlab.com/user/project/integrations/webhooks/#signing-tokens
Hermes currently has only one route secret. A deterministic existing-install sequence is therefore:
- Hermes route is configured with the existing legacy secret
old-secret. - GitLab sends
X-Gitlab-Token: old-secret; currentmainaccepts it. - Operator follows GitLab's zero-downtime migration guidance and additionally enables a different
whsec_...signing token. GitLab now sends both header families. - On this PR,
webhook-id/webhook-timestamp/webhook-signatureselect the Standard/Svix branch first. - Hermes computes that HMAC using the route's
old-secret, not GitLab's distinct signing token, so validation fails and returns 401. The still-validX-Gitlab-Token: old-secretis never consulted.
That means enabling GitLab's stronger auth mechanism can break an already-working Hermes endpoint before the operator has any way to supply the second credential. The PR's tests only cover Standard-only headers with a synthetic raw secret; they do not cover the exact mixed-header transition GitLab ships and recommends.
This should not be repaired by blindly falling back from a failed Standard signature to a weaker scheme: that recreates downgrade ambiguity. The route needs an explicit auth decision/credential model. At minimum, preserve the existing GitLab-token route when that is the configured credential and add a regression for X-Gitlab-Token + webhook-* with distinct secrets. Preferably, compose this on the explicit provider/auth authority already being built below.
2. This reintroduces header-inferred auth in the exact area the repository is already consolidating
There is already an open, exact-head-green authority PR for this defect class: #85318 owns gateway/platforms/webhook_auth.py, including an explicit gitlab_standard mode, and deliberately does not infer the verifier from attacker-controlled headers. #90995 is the open convergence spine that binds provider + verifier once and explicitly says the next adapter composition must consume #85318 rather than copy verifier behavior. #90236 owns provider-native delivery identity / idempotency, and #85640 remains terminal assembly.
This PR instead adds another Standard Webhooks implementation directly to the legacy inline validator and extends the legacy global delivery-ID precedence chain. It also allows mixed families field-by-field (svix-id + webhook-timestamp + webhook-signature, etc.) because each component is selected independently with or. That is not an auth bypass without the key, but it is exactly the provider/verifier identity ambiguity #85318/#90995 were created to eliminate.
So I would not merge this as a parallel authority. Preserve @HwangJohn's #47849 implementation/provenance, but fold its wire-header support and tests into the canonical composition path: #90236 intake identity → #85318 explicit verifier → #90995 bound provider/envelope → #85640 assembly. Once the superseding candidate lands, #47849 can be closed as superseded rather than losing credit.
Verification / exact object
Reviewed head: 3a63116aa5781ed44e661a3b05f03d6e153f4c2c.
The PR records base/merge-base 7d6db4efb885856078e4d19f804035226df81e0d; current main is now fc7523ca31eeb6eff9114afe384c2cf6380359df (head is 1 ahead / 3 behind current main). I re-read the relevant current-main webhook.py hunk and those intervening commits have not already solved this selection problem.
Exact-head hosted workflows are green: CI 32548369568, Docker 32548369211, Nix 32548369190. There were no submitted reviews or inline review threads before this review. Green CI does not cover the dual-token migration case above, and the current-main merge candidate still needs fresh verification after the auth/topology repair.
Nice fit for the existing Svix validator — Standard Webhooks is wire-compatible (same Correctness
Tests
Docs
|
Summary
The generic webhook gateway now authenticates Standard Webhooks senders (
webhook-id/webhook-timestamp/webhook-signature) — providers using that spec (GitLab and a growing set of SaaS webhooks) previously had every delivery to a secret-configured route rejected as an unrecognized signature.Salvages #47849 by @HwangJohn (fixes #47451), rebased onto current main with the stale test-file snapshot dropped and hardening tests added on top.
Changes
gateway/platforms/webhook.py: acceptwebhook-*headers as aliases into the existing Svix-compatible validator — same signed content{id}.{timestamp}.{raw_body}andv1,<base64-hmac-sha256>format, same ±300s replay window, fail-closed when any field is missing;webhook-idalso feeds the delivery-ID dedupe cache; module docstring documents the scheme.tests/gateway/test_webhook_adapter.py: valid-signature accept, tampered-body reject, expired-timestamp reject, incomplete-headers fail-closed, andwebhook-iddedupe (hardening tests for replay-window and fail-closed added during salvage).website/docs/user-guide/messaging/webhooks.md: Standard Webhooks listed under signature validation and idempotency.Validation
tests/gateway/test_webhook_adapter.pytests/gateway/test_webhook_signature_rate_limit.pywebhook-id→ duplicatescripts/audit_pr_attribution.py --fixPorted context: paradigmxyz/centaur#1380 shipped the same protocol support this week (Rust,
standardwebhookscrate) with the identical design points — five-minute tolerance,v1space-separated signatures for key rotation, fail closed on missing secret. Ours reuses the existing Svix validator, which already implements all of that, so the diff stays at ~30 LOC of adapter code.Infographic