feat(webhook): configurable system header names, deprecate DISABLE_* flags - #992
Conversation
Sorry, something went wrong.
|
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 DISABLE_* config but marking it as deprecated and printing a warning. |
Sorry, something went wrong.
|
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: DISABLE_DEFAULT_SIGNATURE_HEADER (emit or not) and SIGNATURE_HEADER_TEMPLATE (the value). "Disable" and "empty value" are the same outcome, and my new SIGNATURE_HEADER_NAME piles a third knob onto the same header. Concrete step I'd take now: keep the two configs that do distinct things: SIGNATURE_HEADER_NAME (the header name) and SIGNATURE_HEADER_TEMPLATE (the value), and treat an empty template (SIGNATURE_HEADER_TEMPLATE='') as "no signature emitted". So we go from three signature knobs down to two, with no overlap. DISABLE_DEFAULT_SIGNATURE_HEADER stays working but deprecated with a warning for existing deployments. That directly removes the config creep you flagged. One scope question: Should this stay signature-only, or give the same treatment to event-id, timestamp, and topic too (deprecating their DISABLE_DEFAULT_*_HEADER flags the same way)? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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!
Sorry, something went wrong.
| // 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 😅
Sorry, something went wrong.
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! 😅
Sorry, something went wrong.
…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>
|
Thanks @ambroziepaval for the contribution! LGTM and I'll kick off a new release with this soon! |
Sorry, something went wrong.
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.