Skip to content

Commit d374ebc

Browse files
fix(webhooks): accept the methods and expose the request metadata the generic webhook advertises (#6893)
* feat(webhooks): support query parameters and GET deliveries on generic webhooks The generic webhook Setup Instructions promised that query parameters would be available in the workflow and that any HTTP method would be accepted, but neither was true: query parameters were never carried past the route, and every GET that was not a provider challenge got a 405. Carry the request query string into the execution payload and expose it to providers through FormatInputContext. The generic provider merges it into the workflow input under a reserved `query` key, leaving the body's own fields untouched so existing payloads resolve exactly as before. Add an opt-in `acceptsGetDelivery` provider capability and enable it for the generic provider, so a workflow can be triggered by a plain URL fetch such as a link in an email. Providers that have not opted in still answer 405, and unknown paths keep answering 405 on GET so probes cannot distinguish them. Update the Setup Instructions to describe what the endpoint actually accepts. Signed-off-by: mini.jeong <mini.jeong@navercorp.com> * feat(webhooks): expose generic webhook request headers The generic webhook's Setup Instructions promised that request headers would be available in the workflow, but formatInput returned only the body: headers were used solely for the idempotency key and provider signature checks. Expose them under a reserved `headers` key, withholding the ones that carry credentials. Exposing a credential would copy it into execution logs and trace spans, where it outlives the request, so a fixed denylist (authorization, cookie, x-api-key, ...) is combined with the webhook's own configured secretHeaderName. A denylist rather than an allowlist keeps arbitrary custom headers usable, which is the point of the feature. Generalize the query-parameter merge so query and headers share the same key-wise body-precedence rule. Also correct the authentication instruction: only the configured method is accepted, not either one. Refs #6888 Signed-off-by: mini.jeong <mini.jeong@navercorp.com> * feat(webhooks): accept PUT, PATCH and DELETE deliveries and expose the request method The generic webhook's Setup Instructions promised any HTTP method, and the /api CORS policy already advertises PUT, PATCH and DELETE, yet the route answered 405 for everything except POST and GET. Open the remaining methods for providers that opt in, which today is only the generic webhook. Expose the method on the trigger input as well. Without it a workflow behind one URL cannot tell a create from a delete, which makes multi-method delivery half a feature. The payload field is optional so jobs already queued at deploy time keep executing. Turn the GET-only opt-in into a per-provider method set, and let the request metadata merge carry scalar values so `method` follows the same key-wise body-precedence rule as query and headers. Refs #6888 Signed-off-by: mini.jeong <mini.jeong@navercorp.com> * feat(webhooks): declare the generic webhook trigger outputs The trigger declared no outputs, so the reference dropdown in the editor offered no completions for it and users had to type paths like `query.id` by hand after reading the setup instructions. Declare the request metadata that is known ahead of time. Body fields stay undeclared because a generic webhook receives whatever JSON the caller sends. Refs #6888 Signed-off-by: mini.jeong <mini.jeong@navercorp.com> * fix(webhooks): stop provider challenges from intercepting other providers' deliveries The challenge handlers run before webhook lookup and are provider-blind, so two query parameter names are effectively reserved across every path. Now that a generic webhook can be triggered by a URL fetch, a link carrying either name answers the challenge instead of running the workflow: - `?validationToken=x` is echoed back as a Microsoft Graph subscription validation. Graph sends that validation as a POST, so ignore the parameter on every other method. - `hub.mode`, `hub.verify_token` and `hub.challenge` answer 403 when no WhatsApp webhook on the path expects a token. A path with no such webhook is not a failed verification - the parameters belong to whoever owns that path - so fall through and let the delivery route normally. A token mismatch against a WhatsApp webhook still fails with 403. Refs #6888 Signed-off-by: mini.jeong <mini.jeong@navercorp.com> * fix(webhooks): make the request metadata opt-in per webhook The four commits below make the generic webhook do what its Setup Instructions promise. They do it through a provider-level capability, which applies to every generic webhook row the moment it deploys: each one begins accepting GET, PUT, PATCH and DELETE, and each one's workflow input gains `method` and `headers`, on POST deliveries too. No webhook owner chose either. Gate both behind `providerConfig` flags written by two switches, off by default. A webhook deployed before these existed has neither flag, so it answers POST only and its input is exactly the body, as before. `query` stays ungated: it is dropped today, only appears when the caller's own URL carries it, and yields to a body field of the same name. Generalize the Microsoft Teams challenge fix. Every challenge handler runs before the webhook lookup and matches on payload shape alone, so any of them will answer a delivery addressed to another provider on the same path. Gate them centrally to POST via `challengeMethods`, which WhatsApp widens to GET for Meta's handshake, rather than guarding one handler inline. Also: - Widen the credential header denylist to 24 names and withhold the webhook's own token by value as well as by name, since a denylist is leaky by construction. - Condition the `method` and `headers` trigger outputs on their switches, so the reference dropdown cannot offer a field the webhook will not send. - Give PUT, PATCH and DELETE their own contracts instead of reusing the POST one, whose `method: 'POST'` had become untrue. - Parse, challenge and generate a request ID once per delivery rather than twice on GET, which was logging one request under two IDs. - Offer the challenge handlers the request before admission, so Meta's GET handshake cannot be answered with a 429 by a busy instance. - Answer every non-POST rejection with the same 405 plus `Allow`, whether the path is unknown, holds only non-path triggers, or holds a trigger that has not opted in. - Read flags through a helper treating only `true`/`'true'` as on: the editor writes booleans, but a YAML- or Copilot-authored workflow can write the string `'false'`, which is truthy. - Name the methods switch "Accept Other HTTP Methods": HEAD and OPTIONS still answer 405, so claiming "all" would reintroduce the overstatement this whole change set exists to remove. - Drop the per-delivery metadata warn logs to debug. --------- Signed-off-by: mini.jeong <mini.jeong@navercorp.com> Co-authored-by: mini.jeong <mini.jeong@navercorp.com>
1 parent d9cfd7c commit d374ebc

16 files changed

Lines changed: 1293 additions & 52 deletions

File tree

apps/sim/app/api/webhooks/trigger/[path]/route.test.ts

Lines changed: 283 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -462,7 +462,11 @@ vi.mock('postgres', () => vi.fn().mockReturnValue({}))
462462

463463
process.env.DATABASE_URL = 'postgresql://test:test@localhost:5432/test'
464464

465-
import { GET, POST } from '@/app/api/webhooks/trigger/[path]/route'
465+
import {
466+
handlePreLookupWebhookVerification,
467+
handleProviderChallenges,
468+
} from '@/lib/webhooks/processor'
469+
import { DELETE, GET, PATCH, POST, PUT } from '@/app/api/webhooks/trigger/[path]/route'
466470

467471
describe('Webhook Trigger API Route', () => {
468472
beforeEach(() => {
@@ -683,6 +687,284 @@ describe('Webhook Trigger API Route', () => {
683687
})
684688
})
685689

690+
/**
691+
* Both handshakes are answered from the request alone, before any webhook lookup, so their
692+
* order relative to each other and to the load-shed gate is the behavior — and it is invisible
693+
* to every other test here, which is how an earlier refactor inverted it unnoticed.
694+
*/
695+
describe('pre-lookup handshake ordering', () => {
696+
/**
697+
* Meta verifies a WhatsApp URL with a GET challenge. Answering it behind the load-shed gate
698+
* means a busy instance returns 429 and the webhook silently fails to verify, at setup time
699+
* only — so the challenge must be answered without taking a ticket at all.
700+
*/
701+
it('answers a provider challenge without taking an admission ticket', async () => {
702+
vi.mocked(handleProviderChallenges).mockResolvedValueOnce(
703+
new NextResponse('hub-challenge-123', { status: 200 })
704+
)
705+
706+
const req = createMockRequest(
707+
'GET',
708+
undefined,
709+
{},
710+
'http://localhost:3000/api/webhooks/trigger/verify-path?hub.challenge=hub-challenge-123'
711+
)
712+
713+
const response = await GET(req, { params: Promise.resolve({ path: 'verify-path' }) })
714+
715+
expect(response.status).toBe(200)
716+
await expect(response.text()).resolves.toBe('hub-challenge-123')
717+
expect(tryAdmitMock).not.toHaveBeenCalled()
718+
})
719+
720+
/**
721+
* A challenge is the more specific answer: the provider is echoing a token it chose, where a
722+
* pending verification only claims the URL is reachable. Answering the generic 200 first
723+
* fails the handshake that actually had a token to return.
724+
*/
725+
it('prefers a provider challenge over a pending setup verification', async () => {
726+
vi.mocked(handleProviderChallenges).mockResolvedValueOnce(
727+
new NextResponse('hub-challenge-123', { status: 200 })
728+
)
729+
730+
const req = createMockRequest(
731+
'GET',
732+
undefined,
733+
{},
734+
'http://localhost:3000/api/webhooks/trigger/verify-path?hub.challenge=hub-challenge-123'
735+
)
736+
737+
const response = await GET(req, { params: Promise.resolve({ path: 'verify-path' }) })
738+
739+
await expect(response.text()).resolves.toBe('hub-challenge-123')
740+
expect(handlePreLookupWebhookVerification).not.toHaveBeenCalled()
741+
})
742+
})
743+
744+
describe('GET deliveries', () => {
745+
it('dispatches a GET delivery to a generic webhook', async () => {
746+
testData.webhooks.push({
747+
id: 'generic-webhook-id',
748+
provider: 'generic',
749+
path: 'get-path',
750+
isActive: true,
751+
providerConfig: { requireAuth: false, acceptOtherMethods: true },
752+
workflowId: 'test-workflow-id',
753+
})
754+
755+
const req = createMockRequest(
756+
'GET',
757+
undefined,
758+
{},
759+
'http://localhost:3000/api/webhooks/trigger/get-path?srcId=123'
760+
)
761+
762+
const response = await GET(req, { params: Promise.resolve({ path: 'get-path' }) })
763+
764+
expect(response.status).toBe(200)
765+
expect(dispatchResolvedWebhookTargetMock).toHaveBeenCalledOnce()
766+
})
767+
768+
/**
769+
* The compatibility guarantee for the route: a generic webhook deployed before the flag
770+
* existed has no flag, so it answers exactly as it did before — 405, no execution.
771+
*/
772+
it('rejects a GET delivery to a generic webhook that has not opted in', async () => {
773+
testData.webhooks.push({
774+
id: 'generic-webhook-id',
775+
provider: 'generic',
776+
path: 'opt-out-path',
777+
isActive: true,
778+
providerConfig: { requireAuth: false },
779+
workflowId: 'test-workflow-id',
780+
})
781+
782+
const req = createMockRequest(
783+
'GET',
784+
undefined,
785+
{},
786+
'http://localhost:3000/api/webhooks/trigger/opt-out-path?srcId=123'
787+
)
788+
789+
const response = await GET(req, { params: Promise.resolve({ path: 'opt-out-path' }) })
790+
791+
expect(response.status).toBe(405)
792+
expect(response.headers.get('Allow')).toBe('POST')
793+
expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled()
794+
})
795+
796+
/**
797+
* Next derives HEAD from the exported GET, so a HEAD probe reaches the same handler. It must
798+
* not execute a workflow: scanners and prefetchers send HEAD unprompted.
799+
*/
800+
it('rejects a HEAD probe to a webhook that accepts every declared method', async () => {
801+
testData.webhooks.push({
802+
id: 'generic-webhook-id',
803+
provider: 'generic',
804+
path: 'head-path',
805+
isActive: true,
806+
providerConfig: { requireAuth: false, acceptOtherMethods: true },
807+
workflowId: 'test-workflow-id',
808+
})
809+
810+
const req = createMockRequest(
811+
'HEAD',
812+
undefined,
813+
{},
814+
'http://localhost:3000/api/webhooks/trigger/head-path'
815+
)
816+
817+
const response = await GET(req, { params: Promise.resolve({ path: 'head-path' }) })
818+
819+
expect(response.status).toBe(405)
820+
expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled()
821+
})
822+
823+
it('rejects a GET delivery to a provider that only accepts POST', async () => {
824+
testData.webhooks.push({
825+
id: 'stripe-webhook-id',
826+
provider: 'stripe',
827+
path: 'post-only-path',
828+
isActive: true,
829+
providerConfig: {},
830+
workflowId: 'test-workflow-id',
831+
})
832+
833+
const req = createMockRequest(
834+
'GET',
835+
undefined,
836+
{},
837+
'http://localhost:3000/api/webhooks/trigger/post-only-path'
838+
)
839+
840+
const response = await GET(req, { params: Promise.resolve({ path: 'post-only-path' }) })
841+
842+
expect(response.status).toBe(405)
843+
expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled()
844+
})
845+
})
846+
847+
describe('PUT, PATCH and DELETE deliveries', () => {
848+
const handlers = { PUT, PATCH, DELETE }
849+
850+
it.each(Object.keys(handlers) as Array<keyof typeof handlers>)(
851+
'dispatches a %s delivery to a generic webhook',
852+
async (method) => {
853+
testData.webhooks.push({
854+
id: 'generic-webhook-id',
855+
provider: 'generic',
856+
path: 'any-method-path',
857+
isActive: true,
858+
providerConfig: { requireAuth: false, acceptOtherMethods: true },
859+
workflowId: 'test-workflow-id',
860+
})
861+
862+
const req = createMockRequest(
863+
method,
864+
{ event: 'test' },
865+
{},
866+
'http://localhost:3000/api/webhooks/trigger/any-method-path?srcId=123'
867+
)
868+
869+
const response = await handlers[method](req, {
870+
params: Promise.resolve({ path: 'any-method-path' }),
871+
})
872+
873+
expect(response.status).toBe(200)
874+
expect(dispatchResolvedWebhookTargetMock).toHaveBeenCalledOnce()
875+
}
876+
)
877+
878+
it('rejects a PUT delivery to a generic webhook that has not opted in', async () => {
879+
testData.webhooks.push({
880+
id: 'generic-webhook-id',
881+
provider: 'generic',
882+
path: 'opt-out-path',
883+
isActive: true,
884+
providerConfig: { requireAuth: false },
885+
workflowId: 'test-workflow-id',
886+
})
887+
888+
const req = createMockRequest(
889+
'PUT',
890+
{ event: 'test' },
891+
{},
892+
'http://localhost:3000/api/webhooks/trigger/opt-out-path'
893+
)
894+
895+
const response = await PUT(req, { params: Promise.resolve({ path: 'opt-out-path' }) })
896+
897+
expect(response.status).toBe(405)
898+
expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled()
899+
})
900+
901+
it('rejects a PUT delivery to a provider that only accepts POST', async () => {
902+
testData.webhooks.push({
903+
id: 'stripe-webhook-id',
904+
provider: 'stripe',
905+
path: 'post-only-path',
906+
isActive: true,
907+
providerConfig: {},
908+
workflowId: 'test-workflow-id',
909+
})
910+
911+
const req = createMockRequest(
912+
'PUT',
913+
{ event: 'test' },
914+
{},
915+
'http://localhost:3000/api/webhooks/trigger/post-only-path'
916+
)
917+
918+
const response = await PUT(req, { params: Promise.resolve({ path: 'post-only-path' }) })
919+
920+
expect(response.status).toBe(405)
921+
expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled()
922+
})
923+
924+
/**
925+
* Every non-POST rejection is the same 405, whether the path is unknown, holds only
926+
* non-path triggers, or holds a trigger that has not opted in — so a probe cannot tell
927+
* a configured path from an unused one.
928+
*/
929+
it('returns the same 405 for a DELETE to a non-path trigger as to an unknown path', async () => {
930+
testData.webhooks.push({
931+
id: 'internal-webhook-id',
932+
provider: 'sim',
933+
path: 'internal-path',
934+
isActive: true,
935+
providerConfig: {},
936+
workflowId: 'test-workflow-id',
937+
})
938+
939+
const req = createMockRequest(
940+
'DELETE',
941+
undefined,
942+
{},
943+
'http://localhost:3000/api/webhooks/trigger/internal-path'
944+
)
945+
946+
const response = await DELETE(req, { params: Promise.resolve({ path: 'internal-path' }) })
947+
948+
expect(response.status).toBe(405)
949+
expect(response.headers.get('Allow')).toBe('POST')
950+
expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled()
951+
})
952+
953+
it('returns 405 for a DELETE to an unknown path', async () => {
954+
const req = createMockRequest(
955+
'DELETE',
956+
undefined,
957+
{},
958+
'http://localhost:3000/api/webhooks/trigger/unknown-path'
959+
)
960+
961+
const response = await DELETE(req, { params: Promise.resolve({ path: 'unknown-path' }) })
962+
963+
expect(response.status).toBe(405)
964+
expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled()
965+
})
966+
})
967+
686968
describe('Reservation-free filtering', () => {
687969
it('skips filtered webhook events before preprocessing reserves a slot', async () => {
688970
testData.webhooks.push({

0 commit comments

Comments
 (0)