feat(webhook): configurable system header names, deprecate DISABLE_* flags#992
Conversation
|
As mentioned in #982 (comment), I do not believe that's the right approach, since we now have 2 configs that implement essentially the same thing. The approach should also be header-agnostic and cover every header value. We also need to account for backward compatibility of existing deployments, possibly by keeping the |
|
Thanks @alexbouchardd , this makes sense and I want to confirm I've got your point right. The signature header already has two configs that overlap: Concrete step I'd take now: keep the two configs that do distinct things: One scope question: Should this stay signature-only, or give the same treatment to event-id, timestamp, and topic too (deprecating their |
|
Hi @ambroziepaval, thanks for the PR and sorry for the miscommunication/confusion. It was caused by me misunderstanding what @alexbouchardd meant. We connected internally and agreed on the design here. Can you take a look and update the PR accordingly? We can chat more in the issue thread if there's anything you want to discuss further. |
ambroziepaval
force-pushed
the
feat/webhook-signature-header-name
branch
from
296bdf5 to
da870aa
Compare
ambroziepaval
changed the title
alexluong
left a comment
alexluong
left a comment
There was a problem hiding this comment.
Great PR! Overall looks great, just got a few small comments here, but otherwise everything makes sense!
| // Validate the four system headers: reject invalid/reserved pinned names, and | ||
| // reject collisions between the effective names (pinned name, or "<prefix>" + | ||
| // key) of two enabled headers. A collision would make emission order-dependent | ||
| // since Format writes headers from a map, so whichever iterated last would | ||
| // silently win. |
There was a problem hiding this comment.
I wonder if this blob is better suited at Config.Validate() instead? I know this already runs on startup but this feels like config validation.
That's my intuition but I'm not super super opinionated there. I get that this is destwebhook logic so I can be talked into this as well haha 😅
There was a problem hiding this comment.
The reason I kept it hereis that the two meaningful checks both depend on things in the destwebhook package: the invalid/reserved name check needs headerNameRegex and reservedHeaders, and the collision check needs resolveHeaderName to know what each header actually resolves to. Moving it to Config.Validate() would mean either duplicating those definitions or introducing a config → destwebhook dependency.
Happy to move it if you feel strongly though! 😅
…flags Replace the four DESTINATIONS_WEBHOOK_DISABLE_DEFAULT_*_HEADER flags with four DESTINATIONS_WEBHOOK_*_HEADER_NAME configs (event-id, signature, timestamp, topic). Each is three-state: unset uses the default, a value pins the exact header name, and an empty string disables the header. The deprecated flags keep working but log a warning at startup. Tracks hookdeck#982. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ambroziepaval
force-pushed
the
feat/webhook-signature-header-name
branch
from
da870aa to
fb7a149
Compare
|
Thanks @ambroziepaval for the contribution! LGTM and I'll kick off a new release with this soon! |
3 participants
Add the global DESTINATIONS_WEBHOOK_SIGNATURE_HEADER_NAME config option. When set, its exact value becomes the signature header name, bypassing the "signature" construction; when unset, behavior is unchanged, so the change is fully backwards compatible.
Scoped to default mode only: standard-webhooks mode fixes the header as "webhook-signature" per spec, and renaming it would break spec-compliant verifiers. This mirrors every other signature-customization field and the existing SignatureHeaderTemplate plumbing. The name is validated at construction against the header-name regex and reserved-header set.
Issue #982.