diff --git a/apps/sim/app/api/webhooks/trigger/[path]/route.test.ts b/apps/sim/app/api/webhooks/trigger/[path]/route.test.ts index f54e18337fd..0aec54129cd 100644 --- a/apps/sim/app/api/webhooks/trigger/[path]/route.test.ts +++ b/apps/sim/app/api/webhooks/trigger/[path]/route.test.ts @@ -462,7 +462,11 @@ vi.mock('postgres', () => vi.fn().mockReturnValue({})) process.env.DATABASE_URL = 'postgresql://test:test@localhost:5432/test' -import { GET, POST } from '@/app/api/webhooks/trigger/[path]/route' +import { + handlePreLookupWebhookVerification, + handleProviderChallenges, +} from '@/lib/webhooks/processor' +import { DELETE, GET, PATCH, POST, PUT } from '@/app/api/webhooks/trigger/[path]/route' describe('Webhook Trigger API Route', () => { beforeEach(() => { @@ -683,6 +687,284 @@ describe('Webhook Trigger API Route', () => { }) }) + /** + * Both handshakes are answered from the request alone, before any webhook lookup, so their + * order relative to each other and to the load-shed gate is the behavior — and it is invisible + * to every other test here, which is how an earlier refactor inverted it unnoticed. + */ + describe('pre-lookup handshake ordering', () => { + /** + * Meta verifies a WhatsApp URL with a GET challenge. Answering it behind the load-shed gate + * means a busy instance returns 429 and the webhook silently fails to verify, at setup time + * only — so the challenge must be answered without taking a ticket at all. + */ + it('answers a provider challenge without taking an admission ticket', async () => { + vi.mocked(handleProviderChallenges).mockResolvedValueOnce( + new NextResponse('hub-challenge-123', { status: 200 }) + ) + + const req = createMockRequest( + 'GET', + undefined, + {}, + 'http://localhost:3000/api/webhooks/trigger/verify-path?hub.challenge=hub-challenge-123' + ) + + const response = await GET(req, { params: Promise.resolve({ path: 'verify-path' }) }) + + expect(response.status).toBe(200) + await expect(response.text()).resolves.toBe('hub-challenge-123') + expect(tryAdmitMock).not.toHaveBeenCalled() + }) + + /** + * A challenge is the more specific answer: the provider is echoing a token it chose, where a + * pending verification only claims the URL is reachable. Answering the generic 200 first + * fails the handshake that actually had a token to return. + */ + it('prefers a provider challenge over a pending setup verification', async () => { + vi.mocked(handleProviderChallenges).mockResolvedValueOnce( + new NextResponse('hub-challenge-123', { status: 200 }) + ) + + const req = createMockRequest( + 'GET', + undefined, + {}, + 'http://localhost:3000/api/webhooks/trigger/verify-path?hub.challenge=hub-challenge-123' + ) + + const response = await GET(req, { params: Promise.resolve({ path: 'verify-path' }) }) + + await expect(response.text()).resolves.toBe('hub-challenge-123') + expect(handlePreLookupWebhookVerification).not.toHaveBeenCalled() + }) + }) + + describe('GET deliveries', () => { + it('dispatches a GET delivery to a generic webhook', async () => { + testData.webhooks.push({ + id: 'generic-webhook-id', + provider: 'generic', + path: 'get-path', + isActive: true, + providerConfig: { requireAuth: false, acceptOtherMethods: true }, + workflowId: 'test-workflow-id', + }) + + const req = createMockRequest( + 'GET', + undefined, + {}, + 'http://localhost:3000/api/webhooks/trigger/get-path?srcId=123' + ) + + const response = await GET(req, { params: Promise.resolve({ path: 'get-path' }) }) + + expect(response.status).toBe(200) + expect(dispatchResolvedWebhookTargetMock).toHaveBeenCalledOnce() + }) + + /** + * The compatibility guarantee for the route: a generic webhook deployed before the flag + * existed has no flag, so it answers exactly as it did before — 405, no execution. + */ + it('rejects a GET delivery to a generic webhook that has not opted in', async () => { + testData.webhooks.push({ + id: 'generic-webhook-id', + provider: 'generic', + path: 'opt-out-path', + isActive: true, + providerConfig: { requireAuth: false }, + workflowId: 'test-workflow-id', + }) + + const req = createMockRequest( + 'GET', + undefined, + {}, + 'http://localhost:3000/api/webhooks/trigger/opt-out-path?srcId=123' + ) + + const response = await GET(req, { params: Promise.resolve({ path: 'opt-out-path' }) }) + + expect(response.status).toBe(405) + expect(response.headers.get('Allow')).toBe('POST') + expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled() + }) + + /** + * Next derives HEAD from the exported GET, so a HEAD probe reaches the same handler. It must + * not execute a workflow: scanners and prefetchers send HEAD unprompted. + */ + it('rejects a HEAD probe to a webhook that accepts every declared method', async () => { + testData.webhooks.push({ + id: 'generic-webhook-id', + provider: 'generic', + path: 'head-path', + isActive: true, + providerConfig: { requireAuth: false, acceptOtherMethods: true }, + workflowId: 'test-workflow-id', + }) + + const req = createMockRequest( + 'HEAD', + undefined, + {}, + 'http://localhost:3000/api/webhooks/trigger/head-path' + ) + + const response = await GET(req, { params: Promise.resolve({ path: 'head-path' }) }) + + expect(response.status).toBe(405) + expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled() + }) + + it('rejects a GET delivery to a provider that only accepts POST', async () => { + testData.webhooks.push({ + id: 'stripe-webhook-id', + provider: 'stripe', + path: 'post-only-path', + isActive: true, + providerConfig: {}, + workflowId: 'test-workflow-id', + }) + + const req = createMockRequest( + 'GET', + undefined, + {}, + 'http://localhost:3000/api/webhooks/trigger/post-only-path' + ) + + const response = await GET(req, { params: Promise.resolve({ path: 'post-only-path' }) }) + + expect(response.status).toBe(405) + expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled() + }) + }) + + describe('PUT, PATCH and DELETE deliveries', () => { + const handlers = { PUT, PATCH, DELETE } + + it.each(Object.keys(handlers) as Array)( + 'dispatches a %s delivery to a generic webhook', + async (method) => { + testData.webhooks.push({ + id: 'generic-webhook-id', + provider: 'generic', + path: 'any-method-path', + isActive: true, + providerConfig: { requireAuth: false, acceptOtherMethods: true }, + workflowId: 'test-workflow-id', + }) + + const req = createMockRequest( + method, + { event: 'test' }, + {}, + 'http://localhost:3000/api/webhooks/trigger/any-method-path?srcId=123' + ) + + const response = await handlers[method](req, { + params: Promise.resolve({ path: 'any-method-path' }), + }) + + expect(response.status).toBe(200) + expect(dispatchResolvedWebhookTargetMock).toHaveBeenCalledOnce() + } + ) + + it('rejects a PUT delivery to a generic webhook that has not opted in', async () => { + testData.webhooks.push({ + id: 'generic-webhook-id', + provider: 'generic', + path: 'opt-out-path', + isActive: true, + providerConfig: { requireAuth: false }, + workflowId: 'test-workflow-id', + }) + + const req = createMockRequest( + 'PUT', + { event: 'test' }, + {}, + 'http://localhost:3000/api/webhooks/trigger/opt-out-path' + ) + + const response = await PUT(req, { params: Promise.resolve({ path: 'opt-out-path' }) }) + + expect(response.status).toBe(405) + expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled() + }) + + it('rejects a PUT delivery to a provider that only accepts POST', async () => { + testData.webhooks.push({ + id: 'stripe-webhook-id', + provider: 'stripe', + path: 'post-only-path', + isActive: true, + providerConfig: {}, + workflowId: 'test-workflow-id', + }) + + const req = createMockRequest( + 'PUT', + { event: 'test' }, + {}, + 'http://localhost:3000/api/webhooks/trigger/post-only-path' + ) + + const response = await PUT(req, { params: Promise.resolve({ path: 'post-only-path' }) }) + + expect(response.status).toBe(405) + expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled() + }) + + /** + * Every non-POST rejection is the same 405, whether the path is unknown, holds only + * non-path triggers, or holds a trigger that has not opted in — so a probe cannot tell + * a configured path from an unused one. + */ + it('returns the same 405 for a DELETE to a non-path trigger as to an unknown path', async () => { + testData.webhooks.push({ + id: 'internal-webhook-id', + provider: 'sim', + path: 'internal-path', + isActive: true, + providerConfig: {}, + workflowId: 'test-workflow-id', + }) + + const req = createMockRequest( + 'DELETE', + undefined, + {}, + 'http://localhost:3000/api/webhooks/trigger/internal-path' + ) + + const response = await DELETE(req, { params: Promise.resolve({ path: 'internal-path' }) }) + + expect(response.status).toBe(405) + expect(response.headers.get('Allow')).toBe('POST') + expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled() + }) + + it('returns 405 for a DELETE to an unknown path', async () => { + const req = createMockRequest( + 'DELETE', + undefined, + {}, + 'http://localhost:3000/api/webhooks/trigger/unknown-path' + ) + + const response = await DELETE(req, { params: Promise.resolve({ path: 'unknown-path' }) }) + + expect(response.status).toBe(405) + expect(dispatchResolvedWebhookTargetMock).not.toHaveBeenCalled() + }) + }) + describe('Reservation-free filtering', () => { it('skips filtered webhook events before preprocessing reserves a slot', async () => { testData.webhooks.push({ diff --git a/apps/sim/app/api/webhooks/trigger/[path]/route.ts b/apps/sim/app/api/webhooks/trigger/[path]/route.ts index 31b37a6edbe..0a8a90ca774 100644 --- a/apps/sim/app/api/webhooks/trigger/[path]/route.ts +++ b/apps/sim/app/api/webhooks/trigger/[path]/route.ts @@ -1,6 +1,12 @@ import { createLogger } from '@sim/logger' import { type NextRequest, NextResponse } from 'next/server' -import { webhookTriggerGetContract, webhookTriggerPostContract } from '@/lib/api/contracts/webhooks' +import { + webhookTriggerDeleteContract, + webhookTriggerGetContract, + webhookTriggerPatchContract, + webhookTriggerPostContract, + webhookTriggerPutContract, +} from '@/lib/api/contracts/webhooks' import { parseRequest } from '@/lib/api/server' import { admissionRejectedResponse, tryAdmit } from '@/lib/core/admission/gate' import { generateRequestId } from '@/lib/core/utils/request' @@ -14,7 +20,7 @@ import { parseWebhookBody, verifyProviderAuth, } from '@/lib/webhooks/processor' -import { acceptsPathWebhookDelivery } from '@/lib/webhooks/providers' +import { acceptsPathWebhookDelivery, acceptsWebhookDeliveryMethod } from '@/lib/webhooks/providers' const logger = createLogger('WebhookTriggerAPI') @@ -22,44 +28,107 @@ export const dynamic = 'force-dynamic' export const runtime = 'nodejs' export const maxDuration = 60 -export const GET = withRouteHandler( - async (request: NextRequest, context: { params: Promise<{ path: string }> }) => { +type RouteContext = { params: Promise<{ path: string }> } + +type WebhookTriggerContract = + | typeof webhookTriggerGetContract + | typeof webhookTriggerPostContract + | typeof webhookTriggerPutContract + | typeof webhookTriggerPatchContract + | typeof webhookTriggerDeleteContract + +/** + * Shared delivery entry point, running the steps that need no body — and therefore no load-shed + * ticket — before admission, in the order the `GET` route has always run them: + * + * 1. resolve the path, + * 2. offer the request to the provider challenge handlers, + * 3. on `GET`, answer a setup-time verification probe for a path that has no webhook row yet, + * 4. take a ticket and hand off to the delivery path. + * + * Steps 2 and 3 keep their relative order because a challenge is the more specific answer: a + * provider echoing a token it chose beats a generic "reachable" 200 when a path somehow has both + * pending at once. + */ +function defineDeliveryRoute( + contract: WebhookTriggerContract, + options: { probeBeforeLookup?: boolean } = {} +) { + return withRouteHandler(async (request: NextRequest, context: RouteContext) => { const requestId = generateRequestId() - const parsed = await parseRequest(webhookTriggerGetContract, request, context) + const parsed = await parseRequest(contract, request, context) if (!parsed.success) return parsed.response const { path } = parsed.data.params - // Handle provider-specific GET verifications (Microsoft Graph, WhatsApp, etc.) - const challengeResponse = await handleProviderChallenges({}, request, requestId, path) - if (challengeResponse) { - return challengeResponse - } + const challenge = await handleProviderChallenges({}, request, requestId, path) + if (challenge) return challenge - return ( - (await handlePreLookupWebhookVerification(request.method, undefined, requestId, path)) || - new NextResponse('Method not allowed', { status: 405 }) - ) - } -) + if (options.probeBeforeLookup) { + const verification = await handlePreLookupWebhookVerification( + request.method, + undefined, + requestId, + path + ) + if (verification) return verification + } -export const POST = withRouteHandler( - async (request: NextRequest, context: { params: Promise<{ path: string }> }) => { const ticket = tryAdmit() if (!ticket) { return admissionRejectedResponse() } try { - return await handleWebhookPost(request, context) + return await handleWebhookDelivery(request, requestId, path) } finally { ticket.release() } - } -) + }) +} + +/** + * `GET` alone probes before the lookup, because a provider validating a URL it has not been given + * a webhook for can only be answered there. `handleWebhookDelivery` runs the same check for every + * method once the lookup comes back empty. + */ +export const GET = defineDeliveryRoute(webhookTriggerGetContract, { probeBeforeLookup: true }) + +export const POST = defineDeliveryRoute(webhookTriggerPostContract) + +/** + * Accepted only by a webhook whose provider declares the method AND whose owner has opted in. + * Everything else gets a 405 from `handleWebhookDelivery`. + */ +export const PUT = defineDeliveryRoute(webhookTriggerPutContract) +export const PATCH = defineDeliveryRoute(webhookTriggerPatchContract) +export const DELETE = defineDeliveryRoute(webhookTriggerDeleteContract) -async function handleWebhookPost( +/** + * A 405 response carries `Allow` per RFC 9110. Every rejection here allows exactly `POST`: a + * webhook + * that accepts more never reaches this branch, so the header cannot be used to tell an unknown + * path from a configured one. + */ +function methodNotAllowedResponse(): NextResponse { + return new NextResponse('Method not allowed', { status: 405, headers: { Allow: 'POST' } }) +} + +/** + * The answer for a path that will not accept this delivery. `POST` keeps its historical 404 so + * existing callers see no change; anything else answers 405 uniformly, whether the path is + * unknown, holds only non-path triggers, or holds a trigger that has not opted into the method — + * so a probe cannot tell those apart. + */ +function notDeliverableResponse(method: string): NextResponse { + return method === 'POST' + ? new NextResponse('Not Found', { status: 404 }) + : methodNotAllowedResponse() +} + +async function handleWebhookDelivery( request: NextRequest, - context: { params: Promise<{ path: string }> } + requestId: string, + path: string ): Promise { const receivedAt = Date.now() /** @@ -72,16 +141,6 @@ async function handleWebhookPost( ? Number(slackRequestTimestamp) * 1000 : undefined - const requestId = generateRequestId() - const parsed = await parseRequest(webhookTriggerPostContract, request, context) - if (!parsed.success) return parsed.response - const { path } = parsed.data.params - - const earlyChallenge = await handleProviderChallenges({}, request, requestId, path) - if (earlyChallenge) { - return earlyChallenge - } - const parseResult = await parseWebhookBody(request, requestId) // Check if parseWebhookBody returned an error response @@ -91,6 +150,10 @@ async function handleWebhookPost( const { body, rawBody } = parseResult + /** + * Offered a second time, now with the parsed body: the pre-admission pass answers only the + * handshakes readable from the URL, and the rest match on body shape. + */ const challengeResponse = await handleProviderChallenges(body, request, requestId, path, rawBody) if (challengeResponse) { return challengeResponse @@ -99,13 +162,24 @@ async function handleWebhookPost( // Find all webhooks for this path (multiple webhooks in one workflow may share a path) const allWebhooksForPath = await findAllWebhooksForPath({ requestId, path }) - const webhooksForPath = allWebhooksForPath.filter(({ webhook: foundWebhook }) => + const pathWebhooks = allWebhooksForPath.filter(({ webhook: foundWebhook }) => acceptsPathWebhookDelivery(foundWebhook.provider) ) - if (allWebhooksForPath.length > 0 && webhooksForPath.length === 0) { + if (allWebhooksForPath.length > 0 && pathWebhooks.length === 0) { logger.warn(`[${requestId}] Rejected HTTP delivery to non-path trigger: ${path}`) - return new NextResponse('Not Found', { status: 404 }) + return notDeliverableResponse(request.method) + } + + const webhooksForPath = pathWebhooks.filter(({ webhook: foundWebhook }) => + acceptsWebhookDeliveryMethod(foundWebhook.provider, request.method, foundWebhook.providerConfig) + ) + + if (pathWebhooks.length > 0 && webhooksForPath.length === 0) { + logger.warn( + `[${requestId}] Rejected ${request.method} delivery to path ${path}: no trigger on this path accepts that method` + ) + return methodNotAllowedResponse() } if (webhooksForPath.length === 0) { @@ -120,7 +194,7 @@ async function handleWebhookPost( } logger.warn(`[${requestId}] Webhook or workflow not found for path: ${path}`) - return new NextResponse('Not Found', { status: 404 }) + return notDeliverableResponse(request.method) } // Process each webhook matched on this path diff --git a/apps/sim/background/webhook-execution.ts b/apps/sim/background/webhook-execution.ts index e27d6edf3fd..5e6bb7d6617 100644 --- a/apps/sim/background/webhook-execution.ts +++ b/apps/sim/background/webhook-execution.ts @@ -269,6 +269,10 @@ export type WebhookExecutionPayload = { provider: string body: unknown headers: Record + /** Request URL query parameters; absent when the request had none or on legacy queued jobs. */ + query?: Record + /** HTTP method the delivery arrived with; absent on legacy queued jobs. */ + method?: string path: string blockId?: string /** Immutable deployment admitted by webhook ingress; absent on legacy queued jobs. */ @@ -622,6 +626,8 @@ async function executeWebhookJobInternal( workflow: { id: payload.workflowId, userId: payload.userId }, body: payload.body, headers: payload.headers, + query: payload.query ?? {}, + method: payload.method ?? '', requestId, }) input = result.input as Record | null diff --git a/apps/sim/lib/api/contracts/webhooks.ts b/apps/sim/lib/api/contracts/webhooks.ts index 5363c77f9c1..47bd6ea4c9e 100644 --- a/apps/sim/lib/api/contracts/webhooks.ts +++ b/apps/sim/lib/api/contracts/webhooks.ts @@ -273,6 +273,44 @@ export const webhookTriggerPostContract = defineRouteContract({ }, }) +/** + * `PUT`, `PATCH` and `DELETE` deliveries. Same shape as the `POST` contract — they exist as + * separate declarations rather than reusing it so each route method is described by a contract + * that states its own method, which is what the boundary audit and any future client read. + */ +export const webhookTriggerPutContract = defineRouteContract({ + method: 'PUT', + path: '/api/webhooks/trigger/[path]', + params: webhookTriggerParamsSchema, + response: { + mode: 'json', + // untyped-response: webhook trigger forwards arbitrary provider challenge or workflow execution payloads + schema: z.unknown(), + }, +}) + +export const webhookTriggerPatchContract = defineRouteContract({ + method: 'PATCH', + path: '/api/webhooks/trigger/[path]', + params: webhookTriggerParamsSchema, + response: { + mode: 'json', + // untyped-response: webhook trigger forwards arbitrary provider challenge or workflow execution payloads + schema: z.unknown(), + }, +}) + +export const webhookTriggerDeleteContract = defineRouteContract({ + method: 'DELETE', + path: '/api/webhooks/trigger/[path]', + params: webhookTriggerParamsSchema, + response: { + mode: 'json', + // untyped-response: webhook trigger forwards arbitrary provider challenge or workflow execution payloads + schema: z.unknown(), + }, +}) + /** * TikTok app-level webhook ingress. Signature is verified from the raw body * before this schema runs; `content` remains a JSON string per TikTok docs. diff --git a/apps/sim/lib/webhooks/processor.test.ts b/apps/sim/lib/webhooks/processor.test.ts index 3904473599b..a3257514ac1 100644 --- a/apps/sim/lib/webhooks/processor.test.ts +++ b/apps/sim/lib/webhooks/processor.test.ts @@ -14,7 +14,7 @@ import { workflowsPersistenceUtilsMock, workflowsPersistenceUtilsMockFns, } from '@sim/testing' -import { NextRequest } from 'next/server' +import { NextRequest, NextResponse } from 'next/server' import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' import { ADMISSION_ERROR_CODE, @@ -135,6 +135,7 @@ import { checkWebhookPreprocessing, dispatchResolvedWebhookTarget, findAllWebhooksForPath, + handleProviderChallenges, handleWebhookEventFilter, parseWebhookBody, processPolledWebhookEvent, @@ -477,6 +478,44 @@ describe('webhook processor execution identity', () => { expect(mockReleaseExecutionSlot).not.toHaveBeenCalled() }) + it('carries request query parameters into the queued payload', async () => { + await dispatchResolvedWebhookTarget( + makeWebhookRecord({ path: 'incoming/hook', provider: 'generic' }), + makeWorkflowRecord({}), + {}, + createMockRequest( + 'GET', + undefined, + {}, + 'http://localhost:3000/api/webhooks/trigger/incoming/hook?srcId=123&title=Hello%20World' + ) as NextRequest, + { requestId: 'request-1', path: 'incoming/hook' } + ) + + expect(mockEnqueue).toHaveBeenCalledWith( + 'webhook-execution', + expect.objectContaining({ query: { srcId: '123', title: 'Hello World' } }), + expect.anything() + ) + }) + + it('omits query from the queued payload when the request has none', async () => { + await dispatchResolvedWebhookTarget( + makeWebhookRecord({ path: 'incoming/hook', provider: 'generic' }), + makeWorkflowRecord({}), + { event: 'test' }, + createMockRequest( + 'POST', + { event: 'test' }, + {}, + 'http://localhost:3000/api/webhooks/trigger/incoming/hook' + ) as NextRequest, + { requestId: 'request-1', path: 'incoming/hook' } + ) + + expect(mockEnqueue.mock.calls[0]?.[1]).not.toHaveProperty('query') + }) + it('runs database-inline webhook jobs through the queue cancellation signal', async () => { mockShouldExecuteInline.mockReturnValue(true) const result = await dispatchResolvedWebhookTarget( @@ -735,3 +774,63 @@ describe('parseWebhookBody', () => { expect((response as Response).status).toBe(400) }) }) + +describe('handleProviderChallenges method gating', () => { + /** + * `getProviderHandler` is mocked module-wide here, so every provider in the challenge list + * resolves to the same stub. That is what this test wants: the behavior under test is the gate + * in `handleProviderChallenges`, not any one provider's matching logic. + */ + const challenge = (method: string, challengeMethods?: readonly string[]) => { + const handleChallenge = vi.fn(() => new NextResponse('answered', { status: 200 })) + mockProviderHandler.current = challengeMethods + ? { handleChallenge, challengeMethods } + : { handleChallenge } + + return { + handleChallenge, + response: handleProviderChallenges( + {}, + new NextRequest('http://localhost:3000/api/webhooks/trigger/abc', { method }), + 'req-1', + 'abc' + ), + } + } + + it('runs a handler that declares nothing on POST', async () => { + const { handleChallenge, response } = challenge('POST') + + expect((await response)?.status).toBe(200) + expect(handleChallenge).toHaveBeenCalledOnce() + }) + + /** + * The regression this gate exists for: a challenge handler matching on payload shape alone runs + * before the webhook lookup, so on a method its provider never uses it would answer a delivery + * addressed to whoever actually owns the path. + */ + it.each(['GET', 'PUT', 'PATCH', 'DELETE'])( + 'does not run a handler that declares nothing on a %s delivery', + async (method) => { + const { handleChallenge, response } = challenge(method) + + await expect(response).resolves.toBeNull() + expect(handleChallenge).not.toHaveBeenCalled() + } + ) + + it('runs a handler on a method it declares', async () => { + const { handleChallenge, response } = challenge('GET', ['GET', 'POST']) + + expect((await response)?.status).toBe(200) + expect(handleChallenge).toHaveBeenCalledOnce() + }) + + it('does not run a declaring handler on a method outside its list', async () => { + const { handleChallenge, response } = challenge('DELETE', ['GET', 'POST']) + + await expect(response).resolves.toBeNull() + expect(handleChallenge).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/webhooks/processor.ts b/apps/sim/lib/webhooks/processor.ts index 06933871349..ca8c6694404 100644 --- a/apps/sim/lib/webhooks/processor.ts +++ b/apps/sim/lib/webhooks/processor.ts @@ -166,6 +166,8 @@ export async function parseWebhookBody( /** Providers that implement challenge/verification handling, checked before webhook lookup. */ const CHALLENGE_PROVIDERS = ['monday', 'slack', 'microsoft-teams', 'whatsapp', 'zoom'] as const +const DEFAULT_CHALLENGE_METHODS = ['POST'] as const + export async function handleProviderChallenges( body: unknown, request: NextRequest, @@ -175,11 +177,19 @@ export async function handleProviderChallenges( ): Promise { for (const provider of CHALLENGE_PROVIDERS) { const handler = getProviderHandler(provider) - if (handler.handleChallenge) { - const response = await handler.handleChallenge(body, request, requestId, path, rawBody) - if (response) { - return response - } + if (!handler.handleChallenge) continue + + /** + * Challenge handlers run before the webhook lookup and match on payload shape alone, so one + * that answers on a method its provider never uses will intercept another provider's + * delivery to the same path. `POST` is the default because every handshake but Meta's is one. + */ + const allowedMethods = handler.challengeMethods ?? DEFAULT_CHALLENGE_METHODS + if (!allowedMethods.includes(request.method)) continue + + const response = await handler.handleChallenge(body, request, requestId, path, rawBody) + if (response) { + return response } } return null @@ -681,6 +691,7 @@ async function queueWebhookExecutionWithResult( } const credentialId = getCredentialId(providerConfig) + const query = Object.fromEntries(new URL(request.url).searchParams) const actorUserId = options.actorUserId const billingAttribution = options.billingAttribution @@ -722,6 +733,8 @@ async function queueWebhookExecutionWithResult( provider: foundWebhook.provider, body, headers, + method: request.method, + ...(Object.keys(query).length > 0 ? { query } : {}), path: options.path || foundWebhook.path || '', blockId: foundWebhook.blockId ?? undefined, ...(foundWebhook.deploymentVersionId diff --git a/apps/sim/lib/webhooks/providers/generic.test.ts b/apps/sim/lib/webhooks/providers/generic.test.ts new file mode 100644 index 00000000000..2dbc6d45750 --- /dev/null +++ b/apps/sim/lib/webhooks/providers/generic.test.ts @@ -0,0 +1,262 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { genericHandler } from '@/lib/webhooks/providers/generic' +import type { FormatInputContext } from '@/lib/webhooks/providers/types' + +interface ContextOptions { + headers?: Record + secretHeaderName?: string + token?: string + method?: string + /** `acceptOtherMethods` — gates the `method` key and the non-POST route methods. */ + acceptOtherMethods?: boolean + /** `exposeRequestHeaders` — gates the `headers` key. */ + exposeRequestHeaders?: boolean +} + +function context( + body: unknown, + query: Record, + options: ContextOptions = {} +): FormatInputContext { + return { + webhook: { + id: 'webhook-id', + provider: 'generic', + providerConfig: { + ...(options.secretHeaderName ? { secretHeaderName: options.secretHeaderName } : {}), + ...(options.token ? { token: options.token } : {}), + ...(options.acceptOtherMethods ? { acceptOtherMethods: true } : {}), + ...(options.exposeRequestHeaders ? { exposeRequestHeaders: true } : {}), + }, + }, + workflow: { id: 'workflow-id', userId: 'user-id' }, + body, + headers: options.headers ?? {}, + query, + method: options.method ?? 'POST', + requestId: 'req-1', + } +} + +const format = (...args: Parameters) => + genericHandler.formatInput!(context(...args)) + +describe('genericHandler.formatInput defaults', () => { + /** + * The compatibility guarantee for every webhook deployed before this feature existed: no flags + * in `providerConfig`, so a POST resolves to exactly the body it always did. + */ + it('passes a POST body through untouched when no flag is set', async () => { + const result = await format( + { event: 'test' }, + {}, + { headers: { 'x-event-name': 'created', authorization: 'Bearer secret' } } + ) + + expect(result.input).toEqual({ event: 'test' }) + }) + + it('withholds "method" until the webhook accepts more than POST', async () => { + const result = await format({ event: 'test' }, {}, { method: 'POST' }) + + expect(result.input).not.toHaveProperty('method') + }) + + it('withholds "headers" until the webhook opts in', async () => { + const result = await format({}, {}, { headers: { 'x-event-name': 'created' } }) + + expect(result.input).not.toHaveProperty('headers') + }) +}) + +describe('genericHandler.formatInput query parameters', () => { + /** + * Query parameters are the one key that is not gated: they are dropped today, they only appear + * when the caller's own URL carries them, and they add nothing to a request without them. + */ + it('exposes query parameters under "query" alongside body fields', async () => { + const result = await format({ event: 'test' }, { srcId: '123', title: 'Hello' }) + + expect(result.input).toEqual({ + event: 'test', + query: { srcId: '123', title: 'Hello' }, + }) + }) + + it('exposes query parameters when the request has no body', async () => { + const result = await format({}, { srcId: '123' }) + + expect(result.input).toEqual({ query: { srcId: '123' } }) + }) + + it('passes the body through unchanged when there are no query parameters', async () => { + const result = await format({ event: 'test' }, {}) + + expect(result.input).toEqual({ event: 'test' }) + }) +}) + +describe('genericHandler.formatInput body precedence', () => { + it.each([ + ['query', { query: 'user typed this' }, { srcId: '123' }, {}], + ['headers', { headers: 'user typed this' }, {}, { exposeRequestHeaders: true }], + ['method', { method: 'user typed this' }, {}, { acceptOtherMethods: true, method: 'PUT' }], + ])('keeps a body field named "%s" instead of overwriting it', async (_key, body, query, opts) => { + const result = await format(body, query, { + ...(opts as ContextOptions), + headers: { 'x-event-name': 'created' }, + }) + + expect(result.input).toEqual(body) + }) + + /** + * `Object.hasOwn`, not `in` — otherwise an inherited key would read as a collision and the + * metadata would silently vanish. + */ + it('does not treat an inherited property name as a body field', async () => { + const body = Object.create({ query: 'from the prototype' }) + body.event = 'test' + + const result = await format(body, { srcId: '123' }) + + expect(result.input).toMatchObject({ event: 'test', query: { srcId: '123' } }) + }) + + it('leaves non-object bodies untouched', async () => { + const body = [{ event: 'a' }] + const result = await format(body, { srcId: '123' }) + + expect(result.input).toEqual(body) + }) +}) + +describe('genericHandler.formatInput exposed headers', () => { + const withHeaders = (headers: Record, options: ContextOptions = {}) => + format({}, {}, { ...options, headers, exposeRequestHeaders: true }) + + it('exposes request headers under "headers" with lowercased names', async () => { + const result = await withHeaders({ 'X-Event-Name': 'created' }) + + expect(result.input).toEqual({ headers: { 'x-event-name': 'created' } }) + }) + + it.each([ + 'authorization', + 'authentication', + 'proxy-authorization', + 'cookie', + 'set-cookie', + 'api-key', + 'apikey', + 'x-api-key', + 'x-api-token', + 'x-auth-token', + 'x-auth-key', + 'x-access-token', + 'x-secret-key', + 'x-functions-key', + 'x-amz-security-token', + 'x-goog-api-key', + 'x-csrf-token', + 'x-sim-idempotency-key', + ])('withholds the credential header %s', async (name) => { + const result = await withHeaders({ [name]: 'secret', 'x-event-name': 'created' }) + + expect(result.input).toEqual({ headers: { 'x-event-name': 'created' } }) + }) + + it("withholds the webhook's own configured secret header", async () => { + const result = await withHeaders( + { 'X-Secret-Key': 'secret', 'x-event-name': 'created' }, + { secretHeaderName: 'X-Secret-Key' } + ) + + expect(result.input).toEqual({ headers: { 'x-event-name': 'created' } }) + }) + + /** + * The denylist is leaky by construction, so the token is withheld by value too: a sender that + * repeats it under a header name nobody anticipated is the case a name list cannot cover. + */ + it('withholds any header carrying the configured token, whatever it is named', async () => { + const result = await withHeaders( + { 'x-vendor-signature': 'not-a-real-token-fixture', 'x-event-name': 'created' }, + { token: 'not-a-real-token-fixture' } + ) + + expect(result.input).toEqual({ headers: { 'x-event-name': 'created' } }) + }) + + it('withholds a header that merely embeds the token', async () => { + const result = await withHeaders( + { 'x-vendor-auth': 'Token not-a-real-token-fixture' }, + { + token: 'not-a-real-token-fixture', + } + ) + + expect(result.input).not.toHaveProperty('headers') + }) + + /** + * A short token would match unrelated header values and quietly strip useful headers, so value + * matching only applies above a length where a collision stops being plausible. + */ + it('does not value-match a token too short to be distinctive', async () => { + const result = await withHeaders({ 'x-region': 'us' }, { token: 'us' }) + + expect(result.input).toEqual({ headers: { 'x-region': 'us' } }) + }) +}) + +describe('genericHandler delivery methods', () => { + it('declares the extra methods and the flag that unlocks them', () => { + expect(genericHandler.extraDeliveryMethods).toEqual({ + methods: ['GET', 'PUT', 'PATCH', 'DELETE'], + enabledBy: 'acceptOtherMethods', + }) + }) + + it('exposes the request method once the webhook accepts more than POST', async () => { + const result = await format( + { event: 'test' }, + {}, + { method: 'DELETE', acceptOtherMethods: true } + ) + + expect(result.input).toEqual({ event: 'test', method: 'DELETE' }) + }) + + it('omits "method" for legacy queued jobs that carry none', async () => { + const result = await format({ event: 'test' }, {}, { method: '', acceptOtherMethods: true }) + + expect(result.input).not.toHaveProperty('method') + }) + + /** + * The editor writes booleans, but a YAML- or Copilot-authored workflow can write the string + * `'false'`, which is truthy. Reading that as "on" would silently ship the opposite of the + * setting the user sees. + */ + it('treats a stringified "false" flag as off', async () => { + const ctx = context({ event: 'test' }, {}, { method: 'DELETE' }) + ;(ctx.webhook.providerConfig as Record).acceptOtherMethods = 'false' + + const result = await genericHandler.formatInput!(ctx) + + expect(result.input).not.toHaveProperty('method') + }) + + it('treats a stringified "true" flag as on', async () => { + const ctx = context({ event: 'test' }, {}, { method: 'DELETE' }) + ;(ctx.webhook.providerConfig as Record).acceptOtherMethods = 'true' + + const result = await genericHandler.formatInput!(ctx) + + expect(result.input).toEqual({ event: 'test', method: 'DELETE' }) + }) +}) diff --git a/apps/sim/lib/webhooks/providers/generic.ts b/apps/sim/lib/webhooks/providers/generic.ts index 71372bebad6..a97317d6fbe 100644 --- a/apps/sim/lib/webhooks/providers/generic.ts +++ b/apps/sim/lib/webhooks/providers/generic.ts @@ -1,4 +1,5 @@ import { createLogger } from '@sim/logger' +import { isRecordLike } from '@sim/utils/object' import { NextResponse } from 'next/server' import { getClientIp } from '@/lib/core/utils/request' import type { @@ -9,11 +10,139 @@ import type { ProcessFilesContext, WebhookProviderHandler, } from '@/lib/webhooks/providers/types' -import { verifyTokenAuth } from '@/lib/webhooks/providers/utils' +import { isProviderConfigFlagEnabled, verifyTokenAuth } from '@/lib/webhooks/providers/utils' const logger = createLogger('WebhookProvider:Generic') +/** + * `providerConfig` flags, set by the matching `switch` subBlocks on the generic webhook trigger. + * + * Both default to off, so every webhook deployed before these existed keeps its current behavior: + * `POST` only, and a workflow input that is exactly the request body. + */ +const ACCEPT_OTHER_METHODS_FLAG = 'acceptOtherMethods' +const EXPOSE_REQUEST_HEADERS_FLAG = 'exposeRequestHeaders' + +/** + * Headers withheld from the workflow input because they carry credentials. Exposing one would + * copy the secret into execution logs and trace spans, where it outlives the request. + * + * A denylist rather than an allowlist, because arbitrary custom headers being usable is the point + * of the feature. A denylist is leaky by construction, so it is not the only defense: the + * webhook's own token is withheld by value as well as by name, and the whole feature is off + * unless the webhook owner turns it on. + */ +const CREDENTIAL_HEADER_NAMES = new Set([ + 'authorization', + 'authentication', + 'proxy-authorization', + 'www-authenticate', + 'proxy-authenticate', + 'cookie', + 'set-cookie', + 'api-key', + 'apikey', + 'x-api-key', + 'x-apikey', + 'x-api-token', + 'x-auth-token', + 'x-auth-key', + 'x-access-token', + 'x-secret', + 'x-secret-key', + 'x-token', + 'x-functions-key', + 'x-amz-security-token', + 'x-goog-api-key', + 'x-csrf-token', + 'x-xsrf-token', + 'x-sim-idempotency-key', +]) + +/** Shortest token still worth matching header values against; below this, collisions dominate. */ +const MIN_TOKEN_MATCH_LENGTH = 8 + +/** + * Request headers for the workflow input, minus the ones that carry credentials. + * + * Names are matched against a fixed denylist plus the webhook's own `secretHeaderName`. Values + * are matched against the webhook's own token, which catches a sender that repeats the token in + * a header this list has never heard of — the failure mode a denylist cannot avoid on its own. + */ +function exposedHeaders( + headers: Record, + providerConfig: Record +): Record { + const secretHeaderName = providerConfig.secretHeaderName + const withheldName = + typeof secretHeaderName === 'string' ? secretHeaderName.toLowerCase() : undefined + + const token = providerConfig.token + const withheldValue = + typeof token === 'string' && token.length >= MIN_TOKEN_MATCH_LENGTH ? token : undefined + + const exposed: Record = {} + + for (const [name, value] of Object.entries(headers)) { + const lowerName = name.toLowerCase() + if (CREDENTIAL_HEADER_NAMES.has(lowerName) || lowerName === withheldName) continue + if (withheldValue !== undefined && value.includes(withheldValue)) continue + exposed[lowerName] = value + } + + return exposed +} + +/** + * Merge request metadata into the body under reserved keys. The body keeps precedence per key, + * so a payload that already carries a field of that name resolves exactly as it did before. + * + * Both drop paths log at debug: each fires once per delivery for a webhook whose shape simply is + * that way (an array body, a body with its own `headers` field), so a warning would be a + * per-request stream about a steady state rather than a signal. + */ +function mergeRequestData( + body: unknown, + requestData: Record>, + requestId: string +): unknown { + const entries = Object.entries(requestData).filter(([, value]) => + typeof value === 'string' ? value.length > 0 : Object.keys(value).length > 0 + ) + + if (entries.length === 0) { + return body + } + + if (!isRecordLike(body)) { + logger.debug( + `[${requestId}] Dropping webhook request metadata: the body is not an object, so there is no field to merge it into`, + { keys: entries.map(([key]) => key) } + ) + return body + } + + const merged: Record = { ...body } + + for (const [key, value] of entries) { + if (Object.hasOwn(body, key)) { + logger.debug( + `[${requestId}] Dropping webhook ${key}: the body already defines a "${key}" field` + ) + continue + } + merged[key] = value + } + + return merged +} + export const genericHandler: WebhookProviderHandler = { + extraDeliveryMethods: { + methods: ['GET', 'PUT', 'PATCH', 'DELETE'], + enabledBy: ACCEPT_OTHER_METHODS_FLAG, + }, + verifyAuth({ request, requestId, providerConfig }: AuthContext) { if (providerConfig.requireAuth) { const configToken = providerConfig.token as string | undefined @@ -84,8 +213,42 @@ export const genericHandler: WebhookProviderHandler = { return null }, - async formatInput({ body }: FormatInputContext): Promise { - return { input: body } + /** + * Expose request metadata under reserved `method`, `query` and `headers` keys alongside the + * body fields. Each key appears only when it carries information the webhook owner asked for: + * + * - `query` whenever the URL has parameters, which are otherwise silently dropped — the bug + * this exists to fix. It is not gated, because it is the caller's own URL and adds nothing + * to a request that has no query string. + * - `method` only once the webhook accepts more than `POST`; before that it is the constant + * `"POST"`, so emitting it would change every existing payload to say nothing. + * - `headers` only once the webhook opts in, because they land in execution logs and trace + * spans, where they outlive the request. + */ + async formatInput({ + body, + headers, + query, + method, + webhook, + requestId, + }: FormatInputContext): Promise { + const providerConfig = (webhook.providerConfig as Record | null) ?? {} + + const exposesMethod = isProviderConfigFlagEnabled(providerConfig[ACCEPT_OTHER_METHODS_FLAG]) + const exposesHeaders = isProviderConfigFlagEnabled(providerConfig[EXPOSE_REQUEST_HEADERS_FLAG]) + + return { + input: mergeRequestData( + body, + { + ...(exposesMethod ? { method } : {}), + query, + ...(exposesHeaders ? { headers: exposedHeaders(headers, providerConfig) } : {}), + }, + requestId + ), + } }, async processInputFiles({ diff --git a/apps/sim/lib/webhooks/providers/index.ts b/apps/sim/lib/webhooks/providers/index.ts index e3f4adfd48c..b39abc5e00e 100644 --- a/apps/sim/lib/webhooks/providers/index.ts +++ b/apps/sim/lib/webhooks/providers/index.ts @@ -1,6 +1,8 @@ export { getProviderHandler } from '@/lib/webhooks/providers/registry' +import { toRecord } from '@sim/utils/object' import { getProviderHandler } from '@/lib/webhooks/providers/registry' +import { isProviderConfigFlagEnabled } from '@/lib/webhooks/providers/utils' import { isInternalTriggerProvider, isPollingWebhookProvider } from '@/triggers/constants' /** @@ -28,3 +30,25 @@ export function acceptsPathWebhookDelivery(provider: string | null): boolean { if (isInternalTriggerProvider(provider) || isPollingWebhookProvider(provider)) return false return getProviderHandler(provider).ingressMode !== 'provider' } + +/** + * Whether this webhook accepts a delivery arriving with this HTTP method. + * + * Every webhook accepts `POST`. Anything else needs both a provider that declares the method in + * `extraDeliveryMethods` and the webhook itself opting in through the flag that declaration + * names. Requiring both is what keeps the capability from changing the behavior of a webhook + * deployed before it existed: such a row has no flag, so it keeps answering `405`. + */ +export function acceptsWebhookDeliveryMethod( + provider: string | null, + method: string, + providerConfig: unknown +): boolean { + if (method === 'POST') return true + if (!provider) return false + + const extra = getProviderHandler(provider).extraDeliveryMethods + if (!extra?.methods.includes(method)) return false + + return isProviderConfigFlagEnabled(toRecord(providerConfig)[extra.enabledBy]) +} diff --git a/apps/sim/lib/webhooks/providers/microsoft-teams.test.ts b/apps/sim/lib/webhooks/providers/microsoft-teams.test.ts index 93bf5642662..30dfcf2ba20 100644 --- a/apps/sim/lib/webhooks/providers/microsoft-teams.test.ts +++ b/apps/sim/lib/webhooks/providers/microsoft-teams.test.ts @@ -219,4 +219,34 @@ describe('microsoftTeamsHandler formatInput (outgoing webhook channelData)', () teamsChannelId: 'channel-1', }) }) + + describe('handleChallenge', () => { + function challengeRequest(method: string): NextRequest { + return new NextRequest( + 'https://app.example.com/api/webhooks/trigger/abc?validationToken=token-123', + { method } + ) + } + + it('echoes the validation token for the POST Microsoft Graph sends', async () => { + const response = microsoftTeamsHandler.handleChallenge!( + {}, + challengeRequest('POST'), + 'teams-challenge-post', + 'abc' + ) + + expect(response?.status).toBe(200) + await expect(response?.text()).resolves.toBe('token-123') + }) + + /** + * Non-POST deliveries never reach this handler: `handleProviderChallenges` gates it to the + * default `POST`, which is asserted in `lib/webhooks/processor.test.ts`. Declaring no + * `challengeMethods` is what buys that, so pin it here. + */ + it('claims no challenge method, so it is gated to POST by default', () => { + expect(microsoftTeamsHandler.challengeMethods).toBeUndefined() + }) + }) }) diff --git a/apps/sim/lib/webhooks/providers/types.ts b/apps/sim/lib/webhooks/providers/types.ts index 45a6dd5b03e..64da7506f3c 100644 --- a/apps/sim/lib/webhooks/providers/types.ts +++ b/apps/sim/lib/webhooks/providers/types.ts @@ -33,6 +33,10 @@ export interface FormatInputContext { workflow: { id: string; userId: string } body: unknown headers: Record + /** Request URL query parameters. Repeated keys collapse to the last value. */ + query: Record + /** HTTP method of the delivering request. Empty on legacy queued jobs. */ + method: string requestId: string } @@ -99,6 +103,35 @@ export interface WebhookProviderHandler { */ ingressMode?: 'path' | 'provider' + /** + * Methods this provider is *able* to accept in addition to `POST`, and the `providerConfig` + * flag each webhook must set to actually accept them. Declaring the capability is not enough: + * `acceptsWebhookDeliveryMethod` still requires the per-webhook flag, so a webhook deployed + * before the capability existed keeps answering `405` exactly as it did. + * + * Use for providers whose events can originate from a plain HTTP call (an email link, a + * REST-style client) rather than a signed callback. Such a delivery may carry no body at all, + * so the provider must be able to trigger on the query parameters alone, and with `GET` the + * caller must tolerate the request being replayed by link prefetchers and scanners — which is + * why it is the webhook owner's decision rather than a platform default. + */ + extraDeliveryMethods?: { + methods: readonly string[] + /** `providerConfig` key whose truthy value opts this webhook into {@link methods}. */ + enabledBy: string + } + + /** + * Methods on which {@link handleChallenge} may answer. Defaults to `POST` only, because a + * challenge is a provider handshake and every provider that sends one sends it as a `POST`. + * + * The default matters for correctness, not just tidiness: challenge handlers run before the + * webhook lookup and match on shape alone, so an unrestricted handler will answer a delivery + * addressed to a different provider on the same path. Widen this only for a provider that + * genuinely handshakes on another method (Meta sends the WhatsApp verification as a `GET`). + */ + challengeMethods?: readonly string[] + /** * Queue workflow execution through the configured durable backend instead of the low-latency * in-process path. Use for providers whose ingress is acknowledged before target processing. diff --git a/apps/sim/lib/webhooks/providers/utils.ts b/apps/sim/lib/webhooks/providers/utils.ts index 1afe2ce3196..5207a79ec36 100644 --- a/apps/sim/lib/webhooks/providers/utils.ts +++ b/apps/sim/lib/webhooks/providers/utils.ts @@ -96,6 +96,17 @@ export function createHmacVerifier({ } } +/** + * Read a boolean `providerConfig` flag written by a `switch` subBlock. + * + * The editor writes real booleans, but workflows authored through YAML or the Copilot can put + * the string `'false'` there, which is truthy. Anything other than an explicit on-value reads as + * off, so a flag that gates new behavior stays off for every webhook deployed before it existed. + */ +export function isProviderConfigFlagEnabled(value: unknown): boolean { + return value === true || value === 'true' +} + /** * Verify a bearer token or custom header token using timing-safe comparison. * Used by generic webhooks, Google Forms, and the default handler. diff --git a/apps/sim/lib/webhooks/providers/whatsapp.test.ts b/apps/sim/lib/webhooks/providers/whatsapp.test.ts index c26763dea02..67a50c98d9f 100644 --- a/apps/sim/lib/webhooks/providers/whatsapp.test.ts +++ b/apps/sim/lib/webhooks/providers/whatsapp.test.ts @@ -2,7 +2,7 @@ * @vitest-environment node */ import { createHmac } from 'node:crypto' -import { dbChainMock, schemaMock } from '@sim/testing' +import { dbChainMock, queueTableRows, schemaMock } from '@sim/testing' import { NextRequest } from 'next/server' import { describe, expect, it, vi } from 'vitest' @@ -265,6 +265,42 @@ describe('WhatsApp webhook provider', () => { expect(input.caption).toBeUndefined() }) + describe('handleChallenge', () => { + function verificationRequest(): NextRequest { + return new NextRequest( + 'http://localhost/api/webhooks/trigger/abc?hub.mode=subscribe&hub.verify_token=t&hub.challenge=c' + ) + } + + it('falls through when no WhatsApp webhook on the path expects a token', async () => { + queueTableRows(schemaMock.webhook, []) + + const response = await whatsappHandler.handleChallenge!( + {}, + verificationRequest(), + 'wa-challenge-no-webhook', + 'abc' + ) + + expect(response).toBeNull() + }) + + it('still fails verification when a WhatsApp webhook expects a different token', async () => { + queueTableRows(schemaMock.webhook, [ + { webhook: { id: 'wh_1', providerConfig: { verificationToken: 'other' } } }, + ]) + + const response = await whatsappHandler.handleChallenge!( + {}, + verificationRequest(), + 'wa-challenge-token-mismatch', + 'abc' + ) + + expect(response?.status).toBe(403) + }) + }) + it('ignores a media type whose payload object is missing', async () => { const input = await formatMediaMessage({ id: 'wamid.image.2', @@ -277,3 +313,14 @@ describe('WhatsApp webhook provider', () => { expect(input.mediaId).toBeUndefined() }) }) + +describe('whatsappHandler challenge methods', () => { + /** + * Meta sends the verification handshake as a GET, so WhatsApp is the one provider that must + * widen past the POST-only default in `handleProviderChallenges`. Losing this declaration + * would break verification silently, at setup time only. + */ + it('declares GET so Meta can verify the URL', () => { + expect(whatsappHandler.challengeMethods).toEqual(['GET', 'POST']) + }) +}) diff --git a/apps/sim/lib/webhooks/providers/whatsapp.ts b/apps/sim/lib/webhooks/providers/whatsapp.ts index a62fec720c4..5ab60964565 100644 --- a/apps/sim/lib/webhooks/providers/whatsapp.ts +++ b/apps/sim/lib/webhooks/providers/whatsapp.ts @@ -202,6 +202,8 @@ async function handleWhatsAppVerification( ) ) + let candidates = 0 + for (const row of webhooks) { const wh = row.webhook const providerConfig = (wh.providerConfig as Record) || {} @@ -211,6 +213,8 @@ async function handleWhatsAppVerification( continue } + candidates++ + if (safeCompare(token, verificationToken as string)) { logger.info(`[${requestId}] WhatsApp verification successful for webhook ${wh.id}`) return new NextResponse(challenge, { @@ -222,6 +226,15 @@ async function handleWhatsAppVerification( } } + /** + * A path with no WhatsApp webhook expecting a token is not a failed verification: the + * `hub.*` parameters belong to whoever owns that path. Fall through so the delivery is + * routed normally instead of answering 403 for someone else's query parameters. + */ + if (candidates === 0) { + return null + } + logger.warn(`[${requestId}] No matching WhatsApp verification token found`) return new NextResponse('Verification failed', { status: 403 }) } @@ -230,6 +243,12 @@ async function handleWhatsAppVerification( } export const whatsappHandler: WebhookProviderHandler = { + /** + * Meta sends the WhatsApp verification handshake as a `GET` with `hub.*` query parameters, so + * this is the one challenge handler that must answer outside `POST`. + */ + challengeMethods: ['GET', 'POST'], + verifyAuth({ request, rawBody, requestId, providerConfig }) { const appSecret = providerConfig.appSecret as string | undefined if (!appSecret) { diff --git a/apps/sim/triggers/generic/webhook.test.ts b/apps/sim/triggers/generic/webhook.test.ts new file mode 100644 index 00000000000..4bc93792e7f --- /dev/null +++ b/apps/sim/triggers/generic/webhook.test.ts @@ -0,0 +1,95 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { genericWebhookTrigger } from '@/triggers/generic/webhook' + +function subBlock(id: string) { + return genericWebhookTrigger.subBlocks.find((entry) => entry.id === id) +} + +function setupInstructions(): string { + return String(subBlock('triggerInstructions')?.defaultValue) +} + +describe('genericWebhookTrigger', () => { + it('declares the request metadata so it can be referenced from later blocks', () => { + expect(Object.keys(genericWebhookTrigger.outputs)).toEqual(['method', 'query', 'headers']) + expect(genericWebhookTrigger.outputs.method.type).toBe('string') + expect(genericWebhookTrigger.outputs.query.type).toBe('object') + expect(genericWebhookTrigger.outputs.headers.type).toBe('object') + }) + + /** + * The default is the compatibility contract: an existing webhook has neither key in its + * `providerConfig`, and a newly created one must start in the same state rather than silently + * opting every new webhook into replayable GET deliveries and headers in execution logs. + */ + it.each(['acceptOtherMethods', 'exposeRequestHeaders'])('ships %s off by default', (id) => { + const field = subBlock(id) + + expect(field?.type).toBe('switch') + expect(field?.defaultValue).toBe(false) + }) + + it('describes POST as the accepted method and names the switch that widens it', () => { + const instructions = setupInstructions() + + expect(instructions).toContain('The webhook accepts POST.') + expect(instructions).toContain('"Accept Other HTTP Methods"') + expect(instructions).toContain('GET, PUT, PATCH and DELETE') + }) + + it('names every reserved key the input can carry', () => { + const instructions = setupInstructions() + + for (const key of Object.keys(genericWebhookTrigger.outputs)) { + expect(instructions).toContain(`"${key}"`) + } + }) + + it('names the switch that exposes headers rather than promising them', () => { + expect(setupInstructions()).toContain('"Expose Request Headers"') + }) + + /** + * Two of the three outputs only exist once a switch is on, so they are conditioned on it: the + * reference dropdown must not offer a field the running webhook will not send. + */ + it.each([ + ['method', 'acceptOtherMethods'], + ['headers', 'exposeRequestHeaders'], + ])('gates the %s output on the switch that produces it', (key, field) => { + expect(genericWebhookTrigger.outputs[key].condition).toEqual({ + field, + value: [true, 'true'], + }) + }) + + /** + * Query parameters are the one key that is not opt-in, so offering them unconditionally is + * correct — gating them on a switch that does not exist would hide them entirely. + */ + it('offers query unconditionally', () => { + expect(genericWebhookTrigger.outputs.query.condition).toBeUndefined() + }) + + /** + * Auth is header-based, so a plain link cannot carry it. Saying so is the difference between a + * user disabling auth knowingly and discovering it after publishing an open trigger URL. + */ + it('warns that authentication cannot be used with a plain link', () => { + expect(setupInstructions()).toContain('cannot be used with a plain link') + }) + + /** + * The switch accepts four named methods, not every method — HEAD and OPTIONS still answer 405. + * A title claiming "all" would be the same kind of overstatement this trigger exists to remove. + */ + it('does not claim to accept methods it rejects', () => { + const field = subBlock('acceptOtherMethods') + + expect(field?.title).not.toContain('All') + expect(field?.description).toContain('GET, PUT, PATCH and DELETE') + }) +}) diff --git a/apps/sim/triggers/generic/webhook.ts b/apps/sim/triggers/generic/webhook.ts index c546c4d4f5c..8136e36bb2f 100644 --- a/apps/sim/triggers/generic/webhook.ts +++ b/apps/sim/triggers/generic/webhook.ts @@ -50,6 +50,24 @@ export const genericWebhookTrigger: TriggerConfig = { required: false, mode: 'trigger', }, + { + id: 'acceptOtherMethods', + title: 'Accept Other HTTP Methods', + type: 'switch', + description: + 'Also accept GET, PUT, PATCH and DELETE — no others — and expose the method under "method". Leave off unless you need it: a GET URL can be replayed by link prefetchers and scanners, and a request with no body cannot be deduplicated.', + defaultValue: false, + mode: 'trigger', + }, + { + id: 'exposeRequestHeaders', + title: 'Expose Request Headers', + type: 'switch', + description: + 'Make the request headers available under "headers". Headers that carry credentials are withheld. Leave off unless you need it: exposed headers are stored in execution logs and trace spans, where they outlive the request.', + defaultValue: false, + mode: 'trigger', + }, { id: 'idempotencyField', title: 'Deduplication Field (Optional)', @@ -118,9 +136,9 @@ export const genericWebhookTrigger: TriggerConfig = { defaultValue: [ 'Copy the webhook URL and use it in your external service or API.', 'Configure your service to send webhooks to this URL.', - 'The webhook will receive any HTTP method (GET, POST, PUT, DELETE, etc.).', - 'All request data (headers, body, query parameters) will be available in your workflow.', - 'If authentication is enabled, include the token in requests using either the custom header or "Authorization: Bearer TOKEN".', + 'The webhook accepts POST. Turn on "Accept Other HTTP Methods" to also accept GET, PUT, PATCH and DELETE — for example to trigger the workflow from a link in an email.', + 'Body fields are available in your workflow, and URL query parameters under "query" (for example "query.id"). Turn on "Expose Request Headers" to also get "headers" (for example "headers.x-event-name"), and "Accept Other HTTP Methods" to also get "method".', + 'Authentication is header-based, so it cannot be used with a plain link. If authentication is enabled, include the token in the Secret Header Name you configured, or in "Authorization: Bearer TOKEN" if you left it blank — only the configured one is accepted, not either.', 'To deduplicate incoming events, set the Deduplication Field to the dot-notation path of a unique identifier in the payload (e.g. "event.id"). Duplicate values within 7 days will be skipped.', 'Enable "Verify Test Events" only if the sending service needs a temporary 200 response while validating the webhook URL.', ] @@ -133,7 +151,34 @@ export const genericWebhookTrigger: TriggerConfig = { }, ], - outputs: {}, + /** + * Body fields stay undeclared because a generic webhook receives whatever JSON the caller + * sends. The request metadata below is known ahead of time, so it can be offered for reference. + * + * `method` and `headers` are conditioned on the switch that produces them, so the reference + * dropdown never offers a field the running webhook will not send. Both truthy forms are + * matched because a YAML- or Copilot-authored workflow can write the string rather than the + * boolean — the same tolerance `isProviderConfigFlagEnabled` applies at delivery time. + */ + outputs: { + method: { + type: 'string', + description: + 'HTTP method of the request. Yields to a body field of the same name if the caller sends one.', + condition: { field: 'acceptOtherMethods', value: [true, 'true'] }, + }, + query: { + type: 'object', + description: + 'Query parameters from the request URL, when it has any. Yields to a body field of the same name if the caller sends one.', + }, + headers: { + type: 'object', + description: + 'Request headers, excluding the ones that carry credentials. Yields to a body field of the same name if the caller sends one.', + condition: { field: 'exposeRequestHeaders', value: [true, 'true'] }, + }, + }, webhook: { method: 'POST',