Skip to content

Commit 9119b6a

Browse files
waleedlatif1claude
andcommitted
refactor(security): consolidate webhook path-collision check into one helper
Extract findConflictingWebhookPathOwner to lib/webhooks/utils.server.ts as the single source of truth for cross-tenant path-collision detection, used by both webhook creation paths (deploy sync and the manual /api/webhooks route). This also repairs two latent issues in the manual route's previous inline check, which queried with limit(1) and only webhook.archivedAt: - limit(1) inspected one arbitrary row, so a same-workflow row could mask a foreign collision (false negative). The shared helper scans all matching rows. - It omitted isActive/workflow.archivedAt, so inactive or archived-workflow webhooks (which never receive deliveries) permanently blocked path reuse. The helper mirrors the runtime dispatcher's filter. Same-workflow webhook reuse for upsert is now a separate, explicit lookup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 7a18723 commit 9119b6a

3 files changed

Lines changed: 69 additions & 53 deletions

File tree

apps/sim/app/api/webhooks/route.ts

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,10 @@ import {
2727
} from '@/lib/webhooks/provider-subscriptions'
2828
import { getProviderHandler } from '@/lib/webhooks/providers'
2929
import { mergeNonUserFields } from '@/lib/webhooks/utils'
30-
import { syncWebhooksForCredentialSet } from '@/lib/webhooks/utils.server'
30+
import {
31+
findConflictingWebhookPathOwner,
32+
syncWebhooksForCredentialSet,
33+
} from '@/lib/webhooks/utils.server'
3134
import { extractCredentialSetId, isCredentialSetValue } from '@/executor/constants'
3235

3336
const logger = createLogger('WebhooksAPI')
@@ -330,21 +333,31 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
330333
}
331334
}
332335
if (!targetWebhookId) {
333-
const existingByPath = await db
334-
.select({ id: webhook.id, workflowId: webhook.workflowId })
336+
const conflictingOwner = await findConflictingWebhookPathOwner({
337+
path: finalPath,
338+
workflowId,
339+
})
340+
if (conflictingOwner) {
341+
logger.warn(`[${requestId}] Webhook path conflict: ${finalPath}`)
342+
return NextResponse.json(
343+
{ error: 'Webhook path already exists.', code: 'PATH_EXISTS' },
344+
{ status: 409 }
345+
)
346+
}
347+
348+
const ownExisting = await db
349+
.select({ id: webhook.id })
335350
.from(webhook)
336-
.where(and(eq(webhook.path, finalPath), isNull(webhook.archivedAt)))
337-
.limit(1)
338-
if (existingByPath.length > 0) {
339-
// If a webhook with the same path exists but belongs to a different workflow, return an error
340-
if (existingByPath[0].workflowId !== workflowId) {
341-
logger.warn(`[${requestId}] Webhook path conflict: ${finalPath}`)
342-
return NextResponse.json(
343-
{ error: 'Webhook path already exists.', code: 'PATH_EXISTS' },
344-
{ status: 409 }
351+
.where(
352+
and(
353+
eq(webhook.path, finalPath),
354+
eq(webhook.workflowId, workflowId),
355+
isNull(webhook.archivedAt)
345356
)
346-
}
347-
targetWebhookId = existingByPath[0].id
357+
)
358+
.limit(1)
359+
if (ownExisting.length > 0) {
360+
targetWebhookId = ownExisting[0].id
348361
}
349362
}
350363

apps/sim/lib/webhooks/deploy.ts

Lines changed: 5 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,5 @@
11
import { db } from '@sim/db'
2-
import {
3-
account,
4-
credentialSetMember,
5-
webhook,
6-
workflow,
7-
workflowDeploymentVersion,
8-
} from '@sim/db/schema'
2+
import { account, credentialSetMember, webhook, workflowDeploymentVersion } from '@sim/db/schema'
93
import { createLogger } from '@sim/logger'
104
import { generateShortId } from '@sim/utils/id'
115
import { and, eq, inArray, isNotNull, isNull, or } from 'drizzle-orm'
@@ -18,7 +12,10 @@ import {
1812
shouldRecreateExternalWebhookSubscription,
1913
} from '@/lib/webhooks/provider-subscriptions'
2014
import { getProviderHandler } from '@/lib/webhooks/providers'
21-
import { syncWebhooksForCredentialSet } from '@/lib/webhooks/utils.server'
15+
import {
16+
findConflictingWebhookPathOwner,
17+
syncWebhooksForCredentialSet,
18+
} from '@/lib/webhooks/utils.server'
2219
import { buildCanonicalIndex } from '@/lib/workflows/subblocks/visibility'
2320
import { getBlock } from '@/blocks'
2421
import type { SubBlockConfig } from '@/blocks/types'
@@ -29,37 +26,6 @@ import { SYSTEM_SUBBLOCK_IDS } from '@/triggers/constants'
2926
const logger = createLogger('DeployWebhookSync')
3027
const CREDENTIAL_SET_PREFIX = 'credentialSet:'
3128

32-
/**
33-
* Returns the id of a different workflow that already owns an active webhook on
34-
* the given path, or `null` if the path is free or owned by this workflow.
35-
* Guards against cross-tenant path collisions, since paths are user-controlled
36-
* and only unique per deployment version. Mirrors the runtime dispatcher's
37-
* filter (active, non-archived webhook on a non-archived workflow) so webhooks
38-
* that can never receive deliveries don't permanently reserve a path.
39-
*/
40-
async function findConflictingWebhookPathOwner(params: {
41-
path: string
42-
workflowId: string
43-
}): Promise<string | null> {
44-
const { path, workflowId } = params
45-
46-
const existing = await db
47-
.select({ workflowId: webhook.workflowId })
48-
.from(webhook)
49-
.innerJoin(workflow, eq(webhook.workflowId, workflow.id))
50-
.where(
51-
and(
52-
eq(webhook.path, path),
53-
eq(webhook.isActive, true),
54-
isNull(webhook.archivedAt),
55-
isNull(workflow.archivedAt)
56-
)
57-
)
58-
59-
const conflict = existing.find((row) => row.workflowId !== workflowId)
60-
return conflict ? conflict.workflowId : null
61-
}
62-
6329
interface TriggerSaveError {
6430
message: string
6531
status: number

apps/sim/lib/webhooks/utils.server.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,43 @@ import { cleanupExternalWebhook } from '@/lib/webhooks/provider-subscriptions'
1010
import { getCredentialsForCredentialSet } from '@/app/api/auth/oauth/utils'
1111
import { isPollingWebhookProvider } from '@/triggers/constants'
1212

13+
/**
14+
* Returns the id of a different workflow that already owns an active webhook on
15+
* the given path, or `null` if the path is free or owned by `workflowId`.
16+
*
17+
* Webhook paths are user-controlled and the database only enforces uniqueness
18+
* per deployment version, so this is the single guard against cross-tenant path
19+
* collisions for every webhook creation path. The filter mirrors the runtime
20+
* dispatcher (`findAllWebhooksForPath`): an active, non-archived webhook on a
21+
* non-archived workflow — inactive or archived webhooks never receive
22+
* deliveries, so they must not reserve a path. All matching rows are scanned so
23+
* a same-workflow row can never mask a foreign collision.
24+
*/
25+
export async function findConflictingWebhookPathOwner(params: {
26+
path: string
27+
workflowId: string
28+
tx?: DbOrTx
29+
}): Promise<string | null> {
30+
const { path, workflowId, tx } = params
31+
const dbCtx = tx ?? db
32+
33+
const existing = await dbCtx
34+
.select({ workflowId: webhook.workflowId })
35+
.from(webhook)
36+
.innerJoin(workflow, eq(webhook.workflowId, workflow.id))
37+
.where(
38+
and(
39+
eq(webhook.path, path),
40+
eq(webhook.isActive, true),
41+
isNull(webhook.archivedAt),
42+
isNull(workflow.archivedAt)
43+
)
44+
)
45+
46+
const conflict = existing.find((row) => row.workflowId !== workflowId)
47+
return conflict ? conflict.workflowId : null
48+
}
49+
1350
/**
1451
* Result of syncing webhooks for a credential set
1552
*/

0 commit comments

Comments
 (0)