Skip to content

Commit ad663a9

Browse files
committed
refactor(webhooks): tighten the delivery route and metadata gating
Audit pass over the diff. No behavior change; the tests and mutation checks are unchanged. - `defineDeliveryRoute` takes a named option rather than a bare positional boolean, so the GET call site says what the flag means. - A gated-off `method` is omitted from the merge input instead of being passed as an empty string for the filter to drop. The sentinel conflated "the owner did not enable this" with "a legacy job carried no method"; the filter still handles the second, which is what it is for. - The accept-all-methods flag constant is no longer exported. Nothing outside the module read it, and its counterpart was already module-private. - Say why the challenge handlers are offered the request a second time: the pre-admission pass answers only what is readable from the URL.
1 parent 89c48cd commit ad663a9

2 files changed

Lines changed: 18 additions & 12 deletions

File tree

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

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,10 @@ type WebhookTriggerContract =
5050
* provider echoing a token it chose beats a generic "reachable" 200 when a path somehow has both
5151
* pending at once.
5252
*/
53-
function defineDeliveryRoute(contract: WebhookTriggerContract, probeBeforeLookup = false) {
53+
function defineDeliveryRoute(
54+
contract: WebhookTriggerContract,
55+
options: { probeBeforeLookup?: boolean } = {}
56+
) {
5457
return withRouteHandler(async (request: NextRequest, context: RouteContext) => {
5558
const requestId = generateRequestId()
5659
const parsed = await parseRequest(contract, request, context)
@@ -60,7 +63,7 @@ function defineDeliveryRoute(contract: WebhookTriggerContract, probeBeforeLookup
6063
const challenge = await handleProviderChallenges({}, request, requestId, path)
6164
if (challenge) return challenge
6265

63-
if (probeBeforeLookup) {
66+
if (options.probeBeforeLookup) {
6467
const verification = await handlePreLookupWebhookVerification(
6568
request.method,
6669
undefined,
@@ -88,7 +91,7 @@ function defineDeliveryRoute(contract: WebhookTriggerContract, probeBeforeLookup
8891
* a webhook for can only be answered there. `handleWebhookDelivery` runs the same check for every
8992
* method once the lookup comes back empty.
9093
*/
91-
export const GET = defineDeliveryRoute(webhookTriggerGetContract, true)
94+
export const GET = defineDeliveryRoute(webhookTriggerGetContract, { probeBeforeLookup: true })
9295

9396
export const POST = defineDeliveryRoute(webhookTriggerPostContract)
9497

@@ -101,8 +104,9 @@ export const PATCH = defineDeliveryRoute(webhookTriggerPatchContract)
101104
export const DELETE = defineDeliveryRoute(webhookTriggerDeleteContract)
102105

103106
/**
104-
* A 405 body carries `Allow` per RFC 9110. Every rejection here allows exactly `POST`: a webhook
105-
* that accepts more never reaches this path, so the header cannot be used to tell an unknown
107+
* A 405 response carries `Allow` per RFC 9110. Every rejection here allows exactly `POST`: a
108+
* webhook
109+
* that accepts more never reaches this branch, so the header cannot be used to tell an unknown
106110
* path from a configured one.
107111
*/
108112
function methodNotAllowedResponse(): NextResponse {
@@ -146,6 +150,10 @@ async function handleWebhookDelivery(
146150

147151
const { body, rawBody } = parseResult
148152

153+
/**
154+
* Offered a second time, now with the parsed body: the pre-admission pass answers only the
155+
* handshakes readable from the URL, and the rest match on body shape.
156+
*/
149157
const challengeResponse = await handleProviderChallenges(body, request, requestId, path, rawBody)
150158
if (challengeResponse) {
151159
return challengeResponse

apps/sim/lib/webhooks/providers/generic.ts

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ const logger = createLogger('WebhookProvider:Generic')
2020
* Both default to off, so every webhook deployed before these existed keeps its current behavior:
2121
* `POST` only, and a workflow input that is exactly the request body.
2222
*/
23-
export const GENERIC_ACCEPT_ALL_METHODS_FLAG = 'acceptAllMethods'
23+
const ACCEPT_ALL_METHODS_FLAG = 'acceptAllMethods'
2424
const EXPOSE_REQUEST_HEADERS_FLAG = 'exposeRequestHeaders'
2525

2626
/**
@@ -140,7 +140,7 @@ function mergeRequestData(
140140
export const genericHandler: WebhookProviderHandler = {
141141
extraDeliveryMethods: {
142142
methods: ['GET', 'PUT', 'PATCH', 'DELETE'],
143-
enabledBy: GENERIC_ACCEPT_ALL_METHODS_FLAG,
143+
enabledBy: ACCEPT_ALL_METHODS_FLAG,
144144
},
145145

146146
verifyAuth({ request, requestId, providerConfig }: AuthContext) {
@@ -235,18 +235,16 @@ export const genericHandler: WebhookProviderHandler = {
235235
}: FormatInputContext): Promise<FormatInputResult> {
236236
const providerConfig = (webhook.providerConfig as Record<string, unknown> | null) ?? {}
237237

238-
const exposesMethod = isProviderConfigFlagEnabled(
239-
providerConfig[GENERIC_ACCEPT_ALL_METHODS_FLAG]
240-
)
238+
const exposesMethod = isProviderConfigFlagEnabled(providerConfig[ACCEPT_ALL_METHODS_FLAG])
241239
const exposesHeaders = isProviderConfigFlagEnabled(providerConfig[EXPOSE_REQUEST_HEADERS_FLAG])
242240

243241
return {
244242
input: mergeRequestData(
245243
body,
246244
{
247-
method: exposesMethod ? method : '',
245+
...(exposesMethod ? { method } : {}),
248246
query,
249-
headers: exposesHeaders ? exposedHeaders(headers, providerConfig) : {},
247+
...(exposesHeaders ? { headers: exposedHeaders(headers, providerConfig) } : {}),
250248
},
251249
requestId
252250
),

0 commit comments

Comments
 (0)