feat(webhook): configurable system header names, deprecate DISABLE_* flags#992
feat(webhook): configurable system header names, deprecate DISABLE_* flags#992ambroziepaval wants to merge 1 commit into
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. |
…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
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!
| // - name set to a value -> {Name: value} (pin exact name) | ||
| // - name set to "" -> {Disabled: true} (disable) |
There was a problem hiding this comment.
I think we have a scenario of value being a whitespace of some sort " " that we should treat as disabled as well.
For now, as I'm tracing it, it will be treated as default headers instead.
| } | ||
| } | ||
|
|
||
| func TestDestinationWebhookHeaderNames(t *testing.T) { |
There was a problem hiding this comment.
let's add the " " case as well as mentioned above
| // 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 😅
| | `DESTINATIONS_WEBHOOK_EVENT_ID_HEADER_NAME` | — | Complete name of the event ID header. Unset uses the default `<prefix>event-id`; an explicit value pins that exact name; an empty string disables the header. | | ||
| | `DESTINATIONS_WEBHOOK_TIMESTAMP_HEADER_NAME` | — | Complete name of the timestamp header. Unset uses the default `<prefix>timestamp`; an explicit value pins that exact name; an empty string disables the header. | | ||
| | `DESTINATIONS_WEBHOOK_TOPIC_HEADER_NAME` | — | Complete name of the topic header. Unset uses the default `<prefix>topic`; an explicit value pins that exact name; an empty string disables the header. | | ||
| | `DESTINATIONS_WEBHOOK_SIGNATURE_HEADER_NAME` | — | Complete name of the signature header. Unset uses the default `<prefix>signature`; an explicit value pins that exact name; an empty string disables the header. | |
There was a problem hiding this comment.
In our OpenAPI schema file, there's ManagedConfig that we should update to include these values as well (and probably remove the DISABLED configs)
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.