feat(gateway): route-configurable webhook signature schemes - #95068
feat(gateway): route-configurable webhook signature schemes#95068jr551 wants to merge 2 commits into
Conversation
Providers that bind a timestamp into the signed message — ElevenLabs,
Stripe, Slack — all use the same construction the adapter already
implements for its generic V2 scheme, and differ only in how the pieces
are packaged into headers. None of them validate today: the built-in
probing looks for a fixed set of header names, so a correct secret still
yields "Invalid signature".
Rather than adding a branch per vendor, a route can describe the
packaging in config and reuse the existing, already-tested primitive:
routes:
my-provider:
secret: "..."
signature:
header: "ElevenLabs-Signature" # t=<unix>,v0=<hex>
signature_part: "v0"
timestamp_part: "t"
template: "{timestamp}.{body}"
tolerance_seconds: 1800
Stripe and Slack are the same block with different labels; a provider
that signs the body alone under its own header name is `template:
"{body}"`. No provider names appear in the code.
Setting `signature` is exclusive and fail-closed: built-in probing is
skipped for that route, so a sender cannot downgrade it to a weaker
scheme it happens to also carry headers for — including the legacy,
replay-vulnerable V1 fallback. Restating the built-in V2 scheme as a
`signature` block is how a route opts out of V1.
Routes without the block are untouched.
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 57729bee8f311853577b2dcbee247d431a038989. I traced the request path (_handle_webhook → _validate_signature → _parse_signature_spec → _validate_configured_signature), the new 67-test suite/docs, issue #95065, and the adjacent webhook-signature work in #68791, #85318, and #90236. There were no existing review submissions or PR comments on this head when I checked. PR base is main@95668f5eabff2ae17f96496ffb87e280b8879846; current main is now 62b2d78025c349996e753c6f7c748de035eb8048 (the intervening tip commit is tool-search work, not this surface).
Blocking: malformed signature_prefix violates both startup validation and fail-closed request semantics
_parse_signature_spec() routes every other string-valued option through _opt_str(), but returns signature_prefix as:
"signature_prefix": raw.get("signature_prefix") or "",That means a truthy non-string value such as signature_prefix: 1 is accepted by connect() as a "valid" signature block. On the first signed request, _validate_configured_signature() reaches:
candidate.startswith(signature_prefix)and raises TypeError instead of returning False. The same applies to a dynamically loaded route: the request-path reparse does not reject the malformed value, so it escapes the verifier rather than taking the documented fail-closed branch. This is exactly the boundary the PR says is hardened: malformed config is supposed to fail at startup for static routes and reject cleanly for live/dynamic routes, never turn attacker-facing traffic into a 500 path.
Required fix: validate signature_prefix as a string during normalization (e.g. _opt_str("signature_prefix")) and add both regressions: (1) connect() rejects a non-string prefix with ValueError, and (2) the live/dynamic route path returns False / HTTP 401 for that malformed block without raising.
Also close the template-shape hole while the parser is the authority
_parse_signature_spec() only checks that the token set contains body; it accepts repeated {body} placeholders. _render_signed_message() uses template.partition("{body}"), so only the first occurrence is replaced with raw body bytes and any later {body} remains literal text. Example: template: "{body}:{body}" renders <raw-body>:{body}, not <raw-body>:<raw-body>. Either reject anything except exactly one {body} at startup or make rendering support repeated raw-byte body substitutions; add a regression so accepted templates have deterministic documented semantics.
Interlocks / merge order
This is not a duplicate of #68791/@Naroh091: that PR owns configurable body-only HMAC/token headers and event_header; this PR/@jr551 adds structured timestamp sources, message templates, replay windows, and repeated signature parts. They are complementary capabilities, but they are two configuration/validation surfaces for the same route-auth domain, so whichever lands second should consolidate on one normalization/verifier authority rather than grow parallel flat-vs-nested policy paths.
#85318/@andrexibiza is the open extraction of signature verification into gateway/platforms/webhook_auth.py, with #90236 as the adjacent authenticated-intake/identity owner. Current main still has the inline verifier, so this PR is correctly written against today's tree. But merge order matters: if #85318 lands first, this structured-scheme implementation should be re-homed into that authority instead of recreating signature policy in webhook.py; if this lands first, the extraction needs to absorb these semantics when it becomes authoritative. Preserve #68791's contributor credit for its earlier configurable-header lane and #85318/#90236's authority lineage rather than treating them as superseded by this feature.
Exact-head hosted execution is not yet a receipt: CI, Docker, and Nix for 57729bee… are all action_required, as are the label-rerun attempts. The local/live evidence in the PR is useful, but there is no GitHub Actions pass attached to this SHA yet.
Once the two parser-contract defects above are fixed, the overall shape is strong: route-level exclusivity closes the existing V1 downgrade path, raw-body signing is preserved, repeated signature values support rotation, timestamp windows are symmetric, and the implementation cleanly avoids vendor-specific branches.
Review of 57729be found two parser-contract defects. 1. `signature_prefix` was the one string option that bypassed `_opt_str()` (`raw.get("signature_prefix") or ""`). A truthy non-string passed `connect()` as valid, then reached `candidate.startswith(prefix)` in the verifier and raised `TypeError` — HTTP 500 on the request path, on the one boundary that is supposed to fail closed, and no better for a dynamically-registered route whose reparse also let it through. Every string option now goes through `_opt_str()`. Auditing the rest of the parser found no sibling: `template` and `tolerance_seconds` were already type-checked explicitly. 2. The template check only asserted the token set *contained* `body`, so `"{body}:{body}"` was accepted while `_render_signed_message()` substitutes a single partition point — rendering a literal `{body}` into the signed message. Exactly one `{body}` is now required. Rejecting rather than substituting every occurrence is the fail-closed reading, and it is the compatible direction: a later release can widen the accepted set without breaking anyone, whereas tightening this later would break configs that had silently "worked". `{timestamp}` is a plain string replacement, so it may repeat and every occurrence is substituted; that is now pinned too. Regressions added for both, over the whole class rather than the two reported keys: non-string values for every string option are rejected at parse time, rejected by `connect()` for a static route, and rejected with a logged `False` / HTTP 401 — never an exception — for a live route. All 13 fail against the previous head.
|
Thank you — this was a careful review and both defects are real. Fixed in Blocking 1 —
|
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 81d4cbecc189cf181ece453d6d6d90dfbf077dee after the follow-up to my review on 57729bee8f311853577b2dcbee247d431a038989. I re-read the complete three-file diff, the parser/renderer/validator path, the expanded adversarial tests, the docs, issue #95065, current main, exact-head workflow state, and the live interlocks (#68791, #85318, #90236, #85640).
The two prior blockers are closed
-
Malformed string options now fail closed as one class.
signature_prefixis routed through_opt_str()like the other string-valued fields. More importantly, the repair did not stop at that one key:TestMalformedBlockNeverRaisesdrives every string option through parse-time rejection,connect()rejection for static routes, direct dynamic-route rejection, and real HTTP401behavior. That closes the originalstartswith()TypeError/500 escape rather than papering over one call site. -
Template shape is now deterministic.
_parse_signature_spec()requires exactly one{body}before_render_signed_message()is admitted. The new regressions reject repeated-body forms at parse/startup and separately prove repeated{timestamp}replacement is well-defined. The raw body remains byte-spliced rather than decoded/re-encoded, including the non-UTF-8 case.
I also rechecked the security boundary around those repairs. A configured signature block remains exclusive: it cannot fall through to GitHub/GitLab/Svix/Linear/generic probing or legacy V1. Missing parts, conflicting timestamp values, malformed timestamps, stale/future timestamps, wrong digests, hostile non-ASCII values, and malformed dynamic config all terminate on rejection. Rotation still accepts any matching repeated digest value without weakening timestamp binding. I do not see a new code-level blocker in this head.
Live graph / ownership
Current main is 25d46c788746b3c787623bf267ae3afa55a4d7c4. The PR branch is currently 2 ahead / 163 behind that tip, with merge base 7c5c994397d4914b7a4f253a652982ec33a0544d. I checked the drift specifically rather than treating the count as evidence of conflict: there have been no main commits touching gateway/platforms/webhook.py or website/docs/user-guide/messaging/webhooks.md since that merge base, so the large behind count is not presently a same-surface collision.
The interlocks remain load-bearing:
- #68791 / @Naroh091 is complementary, not a duplicate. It owns
event_header, thetokenmode, HMAC-MD5 compatibility, and CLI subscribe/list/test wiring; this PR owns structured timestamp sources, templates, replay windows, repeated digest parts, and raw-body construction. There is overlap in custom body-HMAC header configuration, so if both survive, the second landing should consolidate to one normalization/verifier surface rather than leave users with two parallel flat-vs-nested auth APIs. Preserve @Naroh091's earlier lane and credit. - #85318 is the explicit signature/auth authority extraction (
gateway/platforms/webhook_auth.py). It does not collide by file today, but it is the other side of this shape. If #85318 lands first, these configurable-spec semantics should be re-homed into that authority; if #95068 lands first, #85318 must absorb them when it becomes authoritative. We should not end up with inline configurable verification plus a second long-lived auth authority. - #90236 remains the authenticated-intake identity/body-hash owner, and #85640 remains the final webhook integration witness. This PR should compose as auth policy before those downstream intake/settlement concerns, not replace their ownership.
Exact-head acceptance is still missing
For 81d4cbe…, CI 32943147342, Docker 32943146368, and Nix 32943146397 are all action_required. The CI run has 0 jobs, and the commit currently has 0 check-runs, so there is no hosted exact-head execution receipt to inherit or call green. The branch-local/live evidence in the PR is useful, but merge acceptance still needs those workflows to actually execute on this SHA (and a current-main readback after any rebase/composition).
Verdict: the requested repair is good and the previous code blockers are closed. No new code defect found at this head. Keep the exact-head CI gate and the auth-authority merge ordering explicit before landing.
Nice repair work here: turning two specific findings into class-wide parser/request-path regressions is exactly the right way to close this kind of boundary defect. 🚀
|
Thanks — confirming both blockers closed at Your reading of the security boundary matches the intent, and I agree with it as stated: a configured Merge orderingTaking these as binding commitments, not intentions. #68791 / @Naroh091 — complementary, and the earlier lane. It owns #85318 — the auth authority. If it lands first, I will re-home these semantics into #90236 and #85640 — no claim on their ownership. This composes as auth policy running before authenticated-intake identity/body-hash (#90236) and before the integration and settlement concerns #85640 witnesses. It neither computes nor owns body identity, and it does not change where those run. Exact-head CIAgreed, and I won't present the branch-local evidence as a substitute — it is branch-local, not a hosted receipt for this SHA. I confirmed the same state independently: Could a maintainer approve the workflow runs on this SHA? That is the only thing between here and a real exact-head result. I'm deliberately holding the SHA stable so your exact-head review stays valid — hence no rebase despite the 163-behind count, on the strength of your check that no main commit has touched either file since the merge base. I'll rebase and provide a current-main readback on request rather than pre-empting it. |
What does this PR do?
The webhook adapter validates signatures by probing a fixed set of header names. That works for the providers named in the code, and fails for everything else — including one of the most common conventions in the ecosystem: a single header packing a timestamp and a digest as labelled parts.
The first and third are byte-identical to the adapter's own generic V2 scheme (hex HMAC-SHA256 over
"<timestamp>.<body>"). The cryptography is already here and already tested; only the packaging differs._validate_signaturesimply never looks at those header names, so a route with a correct secret is rejected withInvalid signatureand the operator's only working option issecret: INSECURE_NO_AUTHon an endpoint that dispatches agent runs.This PR lets a route describe its provider's packaging in
config.yamland reuse the existing verified primitive:Slack is the same block with
signature_prefix: "v0=",timestamp_header: "X-Slack-Request-Timestamp",template: "v0:{timestamp}:{body}". A provider that signs the body alone under its own header name istemplate: "{body}". Adding a provider becomes configuration, not a release.Why this is not an in-tree provider integration
Flagging this up front, because it is the obvious question and the policy in
AGENTS.mdis clear.This adds no vendor surface of any kind:
plugins/directory, no vendor module, class, or function.header,signature_part,signature_prefix,timestamp_part,timestamp_header,template,algorithm,encoding,tolerance_seconds. Provider names appear only in comments, docs and tests as format examples — exactly as the existing code already does for GitHub, GitLab, Svix/AgentMail and Linear.It sits at the top of the Footprint Ladder — "extend existing code" — widening the generic HMAC surface that already carries every built-in scheme. And it is the direct application of "when several PRs integrate the same category, design one shared interface instead of merging them one at a time." That category is visibly accumulating: #66893 (Gitea), #71968 (Redmine), #47451 (GitLab Standard Webhooks), #66895, #54697 — each a header name and a digest, each needing a code change and a release.
The strongest argument is the second-order one: this reduces pressure for in-tree vendor code rather than adding to it. Every provider a user can express in config is a provider nobody needs to send you a PR for. #6265 asked for the opposite — "implement specific handlers for major providers" — and was rightly closed as not-planned. This is the generic answer to the same problem.
On "speculative infrastructure — extension points with no concrete consumer": there is a real, stated consumer. I hit this on a live gateway where an ElevenLabs post-call route had been sitting on
INSECURE_NO_AUTHfor weeks, because there was no other way to make it work. That route is now validating with asignatureblock and nothing vendor-specific in the tree. Its endpoint is about to be exposed publicly, where the HMAC is the only thing in front of an agent with shell access — which is why the fail-closed behaviour below is tested as hard as it is.Related Issue
Fixes #95065
Adjacent but not overlapping — no shared config keys, and the two compose cleanly:
signature_header,signature_scheme,signature_prefix,event_header) address a provider's header name for a body-only HMAC. They have no signed-message template, no timestamp bound into the message, no structured header parts and no replay window, so the three providers above stay unrepresentable there. That PR also coversevent_header, which this one deliberately does not touch.gateway/platforms/webhook_auth.py(refactor(webhook): extract signature validation into webhook_auth mixin (Webhook Feature Package) #84849 / refactor(webhook): isolate explicit provider signature authority #85318) is in flight against this same code. Currentmainstill has the inline validator, so this is written against that; if the authority module lands first I'll re-land this on top of it.Type of Change
Changes Made
gateway/platforms/webhook.py_parse_signature_spec()— normalises and validates a route'ssignatureblock. RaisesValueErrornaming the route and the offending key._split_signature_header()— parsesk=v,k=vinto label → list of values, because providers repeat a label during secret rotation (Stripe emits onev1=per live secret). Junk chunks are ignored; parts are looked up by name, never by position._render_signed_message()— splices the raw body bytes into the template rather than decoding tostr, so a payload that isn't valid UTF-8 doesn't turn a genuine delivery into a mismatch.{body}is located in the template before the timestamp is substituted, so a crafted timestamp can't introduce a second{body}marker and move where the payload is spliced.WebhookAdapter._validate_configured_signature()— the verifier. Reuses the existing constant-time_hmac_str_equal._validate_signature()gains an optionalroute_config; when a route declaressignature, that scheme is used and built-in probing is skipped.connect()validates the block at startup.tests/gateway/test_webhook_configured_signature.py— 67 tests (new file, so no conflict with in-flight PRs touchingtest_webhook_adapter.py).website/docs/user-guide/messaging/webhooks.md— new Custom signature schemes section: full key table, worked Stripe/Slack/body-only examples, and the security notes below.Security properties
signatureskips built-in probing for that route, so a sender cannot downgrade a route to a weaker scheme it happens to also carry headers for — including the legacy body-only V1 fallback, which is still accepted on any route with a secret and has no replay protection. Restating the built-in V2 scheme as asignatureblock (header: X-Webhook-Signature-V2,timestamp_header: X-Webhook-Timestamp) is therefore also how a route opts out of V1. Tested.t=parts, or a malformed block — every one rejects. There is no path out of the verifier that isn't an explicitTrueon a verified HMAC or a loggedFalse.templatewithout{timestamp}signs the body alone; that is allowed (some providers offer nothing else) but warns once per route, mirroring the existing V1 deprecation warning.config.yaml, not an env var.How to Test
scripts/run_tests.sh tests/gateway/test_webhook_configured_signature.py scripts/run_tests.sh tests/gateway/ # no regressions in the existing suitesManually, against a running gateway:
Test coverage
67 tests, covering all three provider shapes and, for each, the adversarial cases: tampered body, wrong secret, stale and future timestamps, non-numeric timestamp, missing header, empty signature, right-shape-wrong-digest, conflicting repeated parts, hostile non-ASCII, oversized body, secret rotation, uppercase hex, non-UTF-8 payloads, every downgrade path, and 14 malformed-config cases. Both levels are exercised: direct
_validate_signaturecalls and real HTTP end-to-end throughTestClient/TestServer, asserting that exactly the valid requests reach agent dispatch.Backwards compatibility
Fully backwards compatible.
signatureis opt-in; a route without it takes byte-identical code paths to before. The only edit to existing lines hoists aroute_namelocal that was already being computed. The existing GitHub / GitLab / Svix / Linear / V1 / V2 accept and reject paths are covered by the current suites, which pass unchanged.Checklist
Code
AGENTS.mdDocumentation & Housekeeping
website/docs/user-guide/messaging/webhooks.md, module docstring)cli-config.yaml.example— N/A, it does not document webhook routesCONTRIBUTING.md/AGENTS.md— N/A, no architecture or workflow changehmac/hashlibonlyScreenshots / Logs
Running on a live gateway (10 routes, 4 platforms) with a route configured for the
t=/v0=shape. Every adversarial case rejected pre-dispatch; only the two valid requests reached the agent:Rejections are logged with the route and header, and no secret material:
Developed with AI assistance (Claude), against the conventions in
AGENTS.md. Design, the security review of the fail-closed paths, and the live-gateway verification above are mine.