Skip to content

Commit e631110

Browse files
committed
fix(webhooks): authorize the stored credential and drop stored userId on re-save
1 parent 3ac376f commit e631110

2 files changed

Lines changed: 67 additions & 43 deletions

File tree

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

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -573,26 +573,38 @@ describe('POST /api/webhooks credential references', () => {
573573
})
574574

575575
/**
576-
* A re-save that omits the identity fields keeps the ones the server stored,
577-
* so an existing integration is not broken by the client never sending them.
576+
* A re-save that omits `credentialId` still acts with the stored credential
577+
* (polling setup and subscription cleanup read it), so that credential is
578+
* authorized and kept, while a stored `userId` is never carried forward.
578579
*/
579-
it('keeps the stored credential and server-set userId on a re-save that omits them', async () => {
580-
queueUpdatePathRows(true, { credentialId: 'stored-credential', userId: 'credential-owner' })
580+
it('authorizes and keeps the stored credential on a re-save that omits it', async () => {
581+
mocks.authorizeCredentialUseForAuth.mockResolvedValue({ ok: true, workspaceId: 'workspace-1' })
582+
queueUpdatePathRows(true, { credentialId: 'stored-credential', userId: 'stored-user' })
581583

582-
const response = await POST(
583-
upsertRequest({ userId: 'victim-user', eventType: 'record.created' })
584-
)
584+
const response = await POST(upsertRequest({ eventType: 'record.created' }))
585585

586586
expect(response.status).toBe(200)
587-
expect(mocks.authorizeCredentialUseForAuth).not.toHaveBeenCalled()
587+
expect(mocks.authorizeCredentialUseForAuth).toHaveBeenCalledWith(expect.anything(), {
588+
credentialId: 'stored-credential',
589+
workflowId: 'workflow-1',
590+
})
588591
expect(dbChainMockFns.set).toHaveBeenCalledWith(
589592
expect.objectContaining({
590-
providerConfig: {
591-
eventType: 'record.created',
592-
credentialId: 'stored-credential',
593-
userId: 'credential-owner',
594-
},
593+
providerConfig: { eventType: 'record.created', credentialId: 'stored-credential' },
595594
})
596595
)
597596
})
597+
598+
it('refuses a re-save whose stored credential the actor cannot use', async () => {
599+
mocks.authorizeCredentialUseForAuth.mockResolvedValue({
600+
ok: false,
601+
error: 'You do not have access to this credential.',
602+
})
603+
queueUpdatePathRows(true, { credentialId: 'stored-credential' })
604+
605+
const response = await POST(upsertRequest({ eventType: 'record.created' }))
606+
607+
expect(response.status).toBe(403)
608+
expect(dbChainMockFns.set).not.toHaveBeenCalled()
609+
})
598610
})

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

Lines changed: 42 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,9 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
379379
let existingWebhook: any = null
380380
/**
381381
* `userId` is server-owned: the polling token resolver falls back to that
382-
* user's own OAuth account when no credential is set.
382+
* user's own OAuth account when no credential is set. It is neither accepted
383+
* from the client nor carried forward from a stored row; Gmail and Outlook
384+
* polling setup derive it again from the credential after the save.
383385
*/
384386
const originalProviderConfig: Record<string, unknown> = omit(providerConfig || {}, ['userId'])
385387
let resolvedProviderConfig = await resolveEnvVarsInObject(
@@ -388,36 +390,12 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
388390
workflowRecord.workspaceId || undefined
389391
)
390392

391-
/**
392-
* Subscription handlers and pollers look `credentialId` up by id alone and
393-
* mint tokens as its owner, so the actor must be able to use it in the
394-
* workflow's workspace before anything is subscribed or saved.
395-
*/
396-
const requestedCredentialId = originalProviderConfig.credentialId
397-
if (requestedCredentialId != null && requestedCredentialId !== '') {
398-
/** The row stores the unresolved text, so only a literal id is what gets authorized. */
399-
if (
400-
typeof requestedCredentialId !== 'string' ||
401-
resolvedProviderConfig.credentialId !== requestedCredentialId
402-
) {
403-
return NextResponse.json(
404-
{ error: 'providerConfig.credentialId must be a literal credential id' },
405-
{ status: 400 }
406-
)
407-
}
408-
const credentialAccess = await authorizeCredentialUseForAuth(
409-
{ success: true, userId, authType: AuthType.SESSION },
410-
{ credentialId: requestedCredentialId, workflowId }
393+
/** The row stores the unresolved text, so only a literal credential id can be authorized. */
394+
if (resolvedProviderConfig.credentialId !== originalProviderConfig.credentialId) {
395+
return NextResponse.json(
396+
{ error: 'providerConfig.credentialId must be a literal credential id' },
397+
{ status: 400 }
411398
)
412-
if (!credentialAccess.ok) {
413-
logger.warn(`[${requestId}] Webhook credential reference denied`, {
414-
userId,
415-
workflowId,
416-
credentialId: requestedCredentialId,
417-
reason: credentialAccess.error,
418-
})
419-
return NextResponse.json({ error: credentialAccess.error }, { status: 403 })
420-
}
421399
}
422400

423401
let externalSubscriptionCreated = false
@@ -440,6 +418,39 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
440418
existingWebhook = existingRows[0] || null
441419
}
442420

421+
/**
422+
* Subscription handlers, pollers, and subscription cleanup look `credentialId`
423+
* up by id alone and mint tokens as its owner. A save acts with the requested
424+
* credential or, when the request omits it, the stored one, so that credential
425+
* must be usable by the actor in the workflow's workspace before anything is
426+
* subscribed, cleaned up, or saved.
427+
*/
428+
const effectiveCredentialId =
429+
'credentialId' in originalProviderConfig
430+
? originalProviderConfig.credentialId
431+
: existingWebhook?.providerConfig?.credentialId
432+
if (effectiveCredentialId != null && effectiveCredentialId !== '') {
433+
if (typeof effectiveCredentialId !== 'string') {
434+
return NextResponse.json(
435+
{ error: 'providerConfig.credentialId must be a literal credential id' },
436+
{ status: 400 }
437+
)
438+
}
439+
const credentialAccess = await authorizeCredentialUseForAuth(
440+
{ success: true, userId, authType: AuthType.SESSION },
441+
{ credentialId: effectiveCredentialId, workflowId }
442+
)
443+
if (!credentialAccess.ok) {
444+
logger.warn(`[${requestId}] Webhook credential reference denied`, {
445+
userId,
446+
workflowId,
447+
credentialId: effectiveCredentialId,
448+
reason: credentialAccess.error,
449+
})
450+
return NextResponse.json({ error: credentialAccess.error }, { status: 403 })
451+
}
452+
}
453+
443454
/**
444455
* permission-group-enforced: triggers.webhook — a raw upsert handler with no
445456
* application operation to declare the capability on, so it is asserted
@@ -519,6 +530,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
519530
userProvided
520531
)
521532
}
533+
configToSave.userId = undefined
522534

523535
try {
524536
if (targetWebhookId) {

0 commit comments

Comments
 (0)