Skip to content

Commit 6d00f10

Browse files
committed
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.
1 parent ab13e20 commit 6d00f10

16 files changed

Lines changed: 818 additions & 205 deletions

File tree

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

Lines changed: 167 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -462,6 +462,10 @@ vi.mock('postgres', () => vi.fn().mockReturnValue({}))
462462

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

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

467471
describe('Webhook Trigger API Route', () => {
@@ -683,14 +687,68 @@ 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+
686744
describe('GET deliveries', () => {
687745
it('dispatches a GET delivery to a generic webhook', async () => {
688746
testData.webhooks.push({
689747
id: 'generic-webhook-id',
690748
provider: 'generic',
691749
path: 'get-path',
692750
isActive: true,
693-
providerConfig: { requireAuth: false },
751+
providerConfig: { requireAuth: false, acceptOtherMethods: true },
694752
workflowId: 'test-workflow-id',
695753
})
696754

@@ -707,6 +765,61 @@ describe('Webhook Trigger API Route', () => {
707765
expect(dispatchResolvedWebhookTargetMock).toHaveBeenCalledOnce()
708766
})
709767

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+
710823
it('rejects a GET delivery to a provider that only accepts POST', async () => {
711824
testData.webhooks.push({
712825
id: 'stripe-webhook-id',
@@ -742,7 +855,7 @@ describe('Webhook Trigger API Route', () => {
742855
provider: 'generic',
743856
path: 'any-method-path',
744857
isActive: true,
745-
providerConfig: { requireAuth: false },
858+
providerConfig: { requireAuth: false, acceptOtherMethods: true },
746859
workflowId: 'test-workflow-id',
747860
})
748861

@@ -762,6 +875,29 @@ describe('Webhook Trigger API Route', () => {
762875
}
763876
)
764877

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+
765901
it('rejects a PUT delivery to a provider that only accepts POST', async () => {
766902
testData.webhooks.push({
767903
id: 'stripe-webhook-id',
@@ -785,6 +921,35 @@ describe('Webhook Trigger API Route', () => {
785921
expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled()
786922
})
787923

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+
788953
it('returns 405 for a DELETE to an unknown path', async () => {
789954
const req = createMockRequest(
790955
'DELETE',

0 commit comments

Comments
 (0)