From 3ac376f96fd0a5a90d22ee25a548d25d0286bcfd Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 12 Sep 2026 20:07:52 -0700 Subject: [PATCH 1/4] fix(webhooks): authorize credential references on webhook upsert POST /api/webhooks persisted client-supplied providerConfig verbatim, while subscription handlers and pollers resolve providerConfig.credentialId by id alone and mint tokens as the credential's owner. Authorize credentialId for the acting user within the workflow's workspace before subscribing or saving, require it to be a literal id, and never accept a client-supplied userId, which the polling token resolver falls back to. --- apps/sim/app/api/webhooks/route.test.ts | 218 ++++++++++++++++++------ apps/sim/app/api/webhooks/route.ts | 43 ++++- 2 files changed, 204 insertions(+), 57 deletions(-) diff --git a/apps/sim/app/api/webhooks/route.test.ts b/apps/sim/app/api/webhooks/route.test.ts index 85c318c1ca2..c22221417a3 100644 --- a/apps/sim/app/api/webhooks/route.test.ts +++ b/apps/sim/app/api/webhooks/route.test.ts @@ -19,6 +19,7 @@ import { import { beforeEach, describe, expect, it, vi } from 'vitest' const mocks = vi.hoisted(() => ({ + authorizeCredentialUseForAuth: vi.fn(), configurePolling: vi.fn(), createExternalWebhookSubscription: vi.fn(), findConflictingWebhookPathOwner: vi.fn(), @@ -31,6 +32,9 @@ vi.mock('@sim/audit', () => auditMock) vi.mock('@/lib/permission-groups/config-scope.server', () => permissionGroupScopeMock) vi.mock('@/lib/core/telemetry', () => telemetryMock) vi.mock('@/lib/posthog/server', () => posthogServerMock) +vi.mock('@/lib/auth/credential-access', () => ({ + authorizeCredentialUseForAuth: mocks.authorizeCredentialUseForAuth, +})) vi.mock('@/lib/webhooks/env-resolver', () => ({ resolveEnvVarsInObject: mocks.resolveEnvVarsInObject, })) @@ -353,46 +357,72 @@ describe('POST /api/webhooks polling configuration', () => { }) }) -describe('POST /api/webhooks triggers.webhook gate', () => { - beforeEach(() => { - vi.clearAllMocks() - resetDbChainMock() - authMockFns.mockGetSession.mockResolvedValue({ - user: { id: 'actor-1', name: 'Actor', email: 'actor@example.com' }, - session: { id: 'session-1' }, - }) - workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({ - allowed: true, - status: 200, - workflow: { id: 'workflow-1' }, - workspacePermission: 'write', - }) - workflowAuthzMockFns.mockAssertWorkflowMutable.mockResolvedValue(undefined) - permissionGroupScopeMockFns.mockResolvePermissionGroupConfig.mockResolvedValue(null) - mocks.findConflictingWebhookPathOwner.mockResolvedValue(null) - mocks.resolveEnvVarsInObject.mockImplementation(async (config) => config) - mocks.shouldRecreateExternalWebhookSubscription.mockReturnValue(false) - mocks.getProviderHandler.mockReturnValue({}) - mocks.createExternalWebhookSubscription.mockResolvedValue({ - updatedProviderConfig: {}, - externalSubscriptionCreated: false, - }) +/** Mocks an actor with write access to `workflow-1` and no provider side effects. */ +function setupUpsertMocks(): void { + vi.clearAllMocks() + resetDbChainMock() + authMockFns.mockGetSession.mockResolvedValue({ + user: { id: 'actor-1', name: 'Actor', email: 'actor@example.com' }, + session: { id: 'session-1' }, }) - - function upsertRequest() { - return createMockRequest('POST', { + workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({ + allowed: true, + status: 200, + workflow: { id: 'workflow-1' }, + workspacePermission: 'write', + }) + workflowAuthzMockFns.mockAssertWorkflowMutable.mockResolvedValue(undefined) + permissionGroupScopeMockFns.mockResolvePermissionGroupConfig.mockResolvedValue(null) + mocks.findConflictingWebhookPathOwner.mockResolvedValue(null) + mocks.resolveEnvVarsInObject.mockImplementation(async (config) => config) + mocks.shouldRecreateExternalWebhookSubscription.mockReturnValue(false) + mocks.getProviderHandler.mockReturnValue({}) + mocks.createExternalWebhookSubscription.mockResolvedValue({ + updatedProviderConfig: {}, + externalSubscriptionCreated: false, + }) +} + +function upsertRequest(providerConfig: Record = {}) { + return createMockRequest('POST', { + workflowId: 'workflow-1', + path: 'inbound-orders', + provider: 'generic', + providerConfig, + }) +} + +/** The reads the create path makes, in the order the handler issues them. */ +function queueCreatePathRows(): void { + queueTableRows(workflow, [{ id: 'workflow-1', userId: 'actor-1', workspaceId: 'workspace-1' }]) + queueTableRows(webhook, []) +} + +/** The reads the update path makes: the path claim, then the existing row. */ +function queueUpdatePathRows( + isActive: boolean, + providerConfig: Record = {} +): void { + queueTableRows(workflow, [{ id: 'workflow-1', userId: 'actor-1', workspaceId: 'workspace-1' }]) + queueTableRows(webhook, [{ id: 'webhook-1' }]) + queueTableRows(webhook, [ + { + id: 'webhook-1', workflowId: 'workflow-1', + blockId: 'block-1', path: 'inbound-orders', provider: 'generic', - providerConfig: {}, - }) - } + providerConfig, + isActive, + }, + ]) + dbChainMockFns.returning.mockImplementationOnce(async () => [ + { id: 'webhook-1', workflowId: 'workflow-1', path: 'inbound-orders', isActive: true }, + ]) +} - /** The reads the create path makes, in the order the handler issues them. */ - function queueCreatePathRows(): void { - queueTableRows(workflow, [{ id: 'workflow-1', userId: 'actor-1', workspaceId: 'workspace-1' }]) - queueTableRows(webhook, []) - } +describe('POST /api/webhooks triggers.webhook gate', () => { + beforeEach(setupUpsertMocks) /** * Making a workflow reachable from an inbound webhook is the only external @@ -421,26 +451,6 @@ describe('POST /api/webhooks triggers.webhook gate', () => { expect(mocks.createExternalWebhookSubscription).toHaveBeenCalledTimes(1) }) - /** The reads the update path makes: the path claim, then the existing row. */ - function queueUpdatePathRows(isActive: boolean): void { - queueTableRows(workflow, [{ id: 'workflow-1', userId: 'actor-1', workspaceId: 'workspace-1' }]) - queueTableRows(webhook, [{ id: 'webhook-1' }]) - queueTableRows(webhook, [ - { - id: 'webhook-1', - workflowId: 'workflow-1', - blockId: 'block-1', - path: 'inbound-orders', - provider: 'generic', - providerConfig: {}, - isActive, - }, - ]) - dbChainMockFns.returning.mockImplementationOnce(async () => [ - { id: 'webhook-1', workflowId: 'workflow-1', path: 'inbound-orders', isActive: true }, - ]) - } - /** * The upsert always writes `isActive: true`, so re-saving a dormant webhook is * the same transition `PATCH /api/webhooks/[id]` gates — a workflow becoming @@ -488,3 +498,101 @@ describe('POST /api/webhooks triggers.webhook gate', () => { expect(dbChainMockFns.set).toHaveBeenCalledWith(expect.objectContaining({ isActive: true })) }) }) + +describe('POST /api/webhooks credential references', () => { + beforeEach(setupUpsertMocks) + + /** + * Subscription setup and polling mint tokens as the credential's owner, so a + * reference the actor cannot use must be refused before either runs. + */ + it('refuses a credential the actor cannot use', async () => { + mocks.authorizeCredentialUseForAuth.mockResolvedValue({ + ok: false, + error: 'Credential is not accessible from this workflow workspace', + }) + queueCreatePathRows() + + const response = await POST(upsertRequest({ credentialId: 'victim-credential' })) + + expect(response.status).toBe(403) + expect(mocks.authorizeCredentialUseForAuth).toHaveBeenCalledWith( + expect.objectContaining({ success: true, userId: 'actor-1' }), + { credentialId: 'victim-credential', workflowId: 'workflow-1' } + ) + expect(mocks.createExternalWebhookSubscription).not.toHaveBeenCalled() + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + it('refuses a credential id supplied through an env-var reference', async () => { + mocks.resolveEnvVarsInObject.mockImplementation(async (config) => ({ + ...config, + credentialId: 'victim-credential', + })) + queueCreatePathRows() + + const response = await POST(upsertRequest({ credentialId: '{{CREDENTIAL}}' })) + + expect(response.status).toBe(400) + expect(mocks.authorizeCredentialUseForAuth).not.toHaveBeenCalled() + expect(mocks.createExternalWebhookSubscription).not.toHaveBeenCalled() + }) + + it('saves a credential the actor can use in the workflow workspace', async () => { + mocks.authorizeCredentialUseForAuth.mockResolvedValue({ ok: true, workspaceId: 'workspace-1' }) + queueCreatePathRows() + + const response = await POST(upsertRequest({ credentialId: 'own-credential' })) + + expect(response.status).toBe(201) + expect(mocks.createExternalWebhookSubscription).toHaveBeenCalledTimes(1) + expect(dbChainMockFns.values).toHaveBeenCalledWith( + expect.objectContaining({ providerConfig: { credentialId: 'own-credential' } }) + ) + }) + + /** The polling token resolver mints `providerConfig.userId`'s token when no credential is set. */ + it('drops a client-supplied userId before subscribing or saving', async () => { + queueCreatePathRows() + + const response = await POST( + upsertRequest({ userId: 'victim-user', eventType: 'record.created' }) + ) + + expect(response.status).toBe(201) + expect(mocks.createExternalWebhookSubscription).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ providerConfig: { eventType: 'record.created' } }), + expect.anything(), + 'actor-1', + expect.anything() + ) + expect(dbChainMockFns.values).toHaveBeenCalledWith( + expect.objectContaining({ providerConfig: { eventType: 'record.created' } }) + ) + }) + + /** + * A re-save that omits the identity fields keeps the ones the server stored, + * so an existing integration is not broken by the client never sending them. + */ + it('keeps the stored credential and server-set userId on a re-save that omits them', async () => { + queueUpdatePathRows(true, { credentialId: 'stored-credential', userId: 'credential-owner' }) + + const response = await POST( + upsertRequest({ userId: 'victim-user', eventType: 'record.created' }) + ) + + expect(response.status).toBe(200) + expect(mocks.authorizeCredentialUseForAuth).not.toHaveBeenCalled() + expect(dbChainMockFns.set).toHaveBeenCalledWith( + expect.objectContaining({ + providerConfig: { + eventType: 'record.created', + credentialId: 'stored-credential', + userId: 'credential-owner', + }, + }) + ) + }) +}) diff --git a/apps/sim/app/api/webhooks/route.ts b/apps/sim/app/api/webhooks/route.ts index e96cb78b084..be098817f91 100644 --- a/apps/sim/app/api/webhooks/route.ts +++ b/apps/sim/app/api/webhooks/route.ts @@ -9,11 +9,14 @@ import { } from '@sim/platform-authz/workflow' import { getErrorMessage } from '@sim/utils/errors' import { generateId, generateShortId } from '@sim/utils/id' +import { omit } from '@sim/utils/object' import { and, desc, eq, inArray, isNull, or } from 'drizzle-orm' import { type NextRequest, NextResponse } from 'next/server' import { listWebhooksContract, upsertWebhookContract } from '@/lib/api/contracts/webhooks' import { parseRequest } from '@/lib/api/server' import { getSession } from '@/lib/auth' +import { authorizeCredentialUseForAuth } from '@/lib/auth/credential-access' +import { AuthType } from '@/lib/auth/hybrid' import { PlatformEvents } from '@/lib/core/telemetry' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' @@ -374,13 +377,49 @@ export const POST = withRouteHandler(async (request: NextRequest) => { let savedWebhook: any = null let existingWebhook: any = null - const originalProviderConfig = providerConfig || {} + /** + * `userId` is server-owned: the polling token resolver falls back to that + * user's own OAuth account when no credential is set. + */ + const originalProviderConfig: Record = omit(providerConfig || {}, ['userId']) let resolvedProviderConfig = await resolveEnvVarsInObject( originalProviderConfig, userId, workflowRecord.workspaceId || undefined ) + /** + * Subscription handlers and pollers look `credentialId` up by id alone and + * mint tokens as its owner, so the actor must be able to use it in the + * workflow's workspace before anything is subscribed or saved. + */ + const requestedCredentialId = originalProviderConfig.credentialId + if (requestedCredentialId != null && requestedCredentialId !== '') { + /** The row stores the unresolved text, so only a literal id is what gets authorized. */ + if ( + typeof requestedCredentialId !== 'string' || + resolvedProviderConfig.credentialId !== requestedCredentialId + ) { + return NextResponse.json( + { error: 'providerConfig.credentialId must be a literal credential id' }, + { status: 400 } + ) + } + const credentialAccess = await authorizeCredentialUseForAuth( + { success: true, userId, authType: AuthType.SESSION }, + { credentialId: requestedCredentialId, workflowId } + ) + if (!credentialAccess.ok) { + logger.warn(`[${requestId}] Webhook credential reference denied`, { + userId, + workflowId, + credentialId: requestedCredentialId, + reason: credentialAccess.error, + }) + return NextResponse.json({ error: credentialAccess.error }, { status: 403 }) + } + } + let externalSubscriptionCreated = false const createTempWebhookData = (providerConfigOverride = resolvedProviderConfig) => ({ id: targetWebhookId || generateShortId(), @@ -389,7 +428,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => { providerConfig: providerConfigOverride, }) - const userProvided = originalProviderConfig as Record + const userProvided = originalProviderConfig const configToSave: Record = { ...userProvided } if (targetWebhookId) { From e631110bbcb8b665e69f40cf30ade6760b9ae677 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 12 Sep 2026 20:15:50 -0700 Subject: [PATCH 2/4] fix(webhooks): authorize the stored credential and drop stored userId on re-save --- apps/sim/app/api/webhooks/route.test.ts | 38 ++++++++----- apps/sim/app/api/webhooks/route.ts | 72 ++++++++++++++----------- 2 files changed, 67 insertions(+), 43 deletions(-) diff --git a/apps/sim/app/api/webhooks/route.test.ts b/apps/sim/app/api/webhooks/route.test.ts index c22221417a3..dce67600c5c 100644 --- a/apps/sim/app/api/webhooks/route.test.ts +++ b/apps/sim/app/api/webhooks/route.test.ts @@ -573,26 +573,38 @@ describe('POST /api/webhooks credential references', () => { }) /** - * A re-save that omits the identity fields keeps the ones the server stored, - * so an existing integration is not broken by the client never sending them. + * A re-save that omits `credentialId` still acts with the stored credential + * (polling setup and subscription cleanup read it), so that credential is + * authorized and kept, while a stored `userId` is never carried forward. */ - it('keeps the stored credential and server-set userId on a re-save that omits them', async () => { - queueUpdatePathRows(true, { credentialId: 'stored-credential', userId: 'credential-owner' }) + it('authorizes and keeps the stored credential on a re-save that omits it', async () => { + mocks.authorizeCredentialUseForAuth.mockResolvedValue({ ok: true, workspaceId: 'workspace-1' }) + queueUpdatePathRows(true, { credentialId: 'stored-credential', userId: 'stored-user' }) - const response = await POST( - upsertRequest({ userId: 'victim-user', eventType: 'record.created' }) - ) + const response = await POST(upsertRequest({ eventType: 'record.created' })) expect(response.status).toBe(200) - expect(mocks.authorizeCredentialUseForAuth).not.toHaveBeenCalled() + expect(mocks.authorizeCredentialUseForAuth).toHaveBeenCalledWith(expect.anything(), { + credentialId: 'stored-credential', + workflowId: 'workflow-1', + }) expect(dbChainMockFns.set).toHaveBeenCalledWith( expect.objectContaining({ - providerConfig: { - eventType: 'record.created', - credentialId: 'stored-credential', - userId: 'credential-owner', - }, + providerConfig: { eventType: 'record.created', credentialId: 'stored-credential' }, }) ) }) + + it('refuses a re-save whose stored credential the actor cannot use', async () => { + mocks.authorizeCredentialUseForAuth.mockResolvedValue({ + ok: false, + error: 'You do not have access to this credential.', + }) + queueUpdatePathRows(true, { credentialId: 'stored-credential' }) + + const response = await POST(upsertRequest({ eventType: 'record.created' })) + + expect(response.status).toBe(403) + expect(dbChainMockFns.set).not.toHaveBeenCalled() + }) }) diff --git a/apps/sim/app/api/webhooks/route.ts b/apps/sim/app/api/webhooks/route.ts index be098817f91..eb453c5b5c9 100644 --- a/apps/sim/app/api/webhooks/route.ts +++ b/apps/sim/app/api/webhooks/route.ts @@ -379,7 +379,9 @@ export const POST = withRouteHandler(async (request: NextRequest) => { let existingWebhook: any = null /** * `userId` is server-owned: the polling token resolver falls back to that - * user's own OAuth account when no credential is set. + * user's own OAuth account when no credential is set. It is neither accepted + * from the client nor carried forward from a stored row; Gmail and Outlook + * polling setup derive it again from the credential after the save. */ const originalProviderConfig: Record = omit(providerConfig || {}, ['userId']) let resolvedProviderConfig = await resolveEnvVarsInObject( @@ -388,36 +390,12 @@ export const POST = withRouteHandler(async (request: NextRequest) => { workflowRecord.workspaceId || undefined ) - /** - * Subscription handlers and pollers look `credentialId` up by id alone and - * mint tokens as its owner, so the actor must be able to use it in the - * workflow's workspace before anything is subscribed or saved. - */ - const requestedCredentialId = originalProviderConfig.credentialId - if (requestedCredentialId != null && requestedCredentialId !== '') { - /** The row stores the unresolved text, so only a literal id is what gets authorized. */ - if ( - typeof requestedCredentialId !== 'string' || - resolvedProviderConfig.credentialId !== requestedCredentialId - ) { - return NextResponse.json( - { error: 'providerConfig.credentialId must be a literal credential id' }, - { status: 400 } - ) - } - const credentialAccess = await authorizeCredentialUseForAuth( - { success: true, userId, authType: AuthType.SESSION }, - { credentialId: requestedCredentialId, workflowId } + /** The row stores the unresolved text, so only a literal credential id can be authorized. */ + if (resolvedProviderConfig.credentialId !== originalProviderConfig.credentialId) { + return NextResponse.json( + { error: 'providerConfig.credentialId must be a literal credential id' }, + { status: 400 } ) - if (!credentialAccess.ok) { - logger.warn(`[${requestId}] Webhook credential reference denied`, { - userId, - workflowId, - credentialId: requestedCredentialId, - reason: credentialAccess.error, - }) - return NextResponse.json({ error: credentialAccess.error }, { status: 403 }) - } } let externalSubscriptionCreated = false @@ -440,6 +418,39 @@ export const POST = withRouteHandler(async (request: NextRequest) => { existingWebhook = existingRows[0] || null } + /** + * Subscription handlers, pollers, and subscription cleanup look `credentialId` + * up by id alone and mint tokens as its owner. A save acts with the requested + * credential or, when the request omits it, the stored one, so that credential + * must be usable by the actor in the workflow's workspace before anything is + * subscribed, cleaned up, or saved. + */ + const effectiveCredentialId = + 'credentialId' in originalProviderConfig + ? originalProviderConfig.credentialId + : existingWebhook?.providerConfig?.credentialId + if (effectiveCredentialId != null && effectiveCredentialId !== '') { + if (typeof effectiveCredentialId !== 'string') { + return NextResponse.json( + { error: 'providerConfig.credentialId must be a literal credential id' }, + { status: 400 } + ) + } + const credentialAccess = await authorizeCredentialUseForAuth( + { success: true, userId, authType: AuthType.SESSION }, + { credentialId: effectiveCredentialId, workflowId } + ) + if (!credentialAccess.ok) { + logger.warn(`[${requestId}] Webhook credential reference denied`, { + userId, + workflowId, + credentialId: effectiveCredentialId, + reason: credentialAccess.error, + }) + return NextResponse.json({ error: credentialAccess.error }, { status: 403 }) + } + } + /** * permission-group-enforced: triggers.webhook — a raw upsert handler with no * application operation to declare the capability on, so it is asserted @@ -519,6 +530,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => { userProvided ) } + configToSave.userId = undefined try { if (targetWebhookId) { From c77dd6d5d0e683dbc05a4b6e70e9a2195c38ea56 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 12 Sep 2026 20:22:42 -0700 Subject: [PATCH 3/4] fix(webhooks): authorize both requested and stored credentials on upsert --- apps/sim/app/api/webhooks/route.test.ts | 39 +++++++++++++++++++++++++ apps/sim/app/api/webhooks/route.ts | 26 +++++++++-------- 2 files changed, 53 insertions(+), 12 deletions(-) diff --git a/apps/sim/app/api/webhooks/route.test.ts b/apps/sim/app/api/webhooks/route.test.ts index dce67600c5c..d688721f2c4 100644 --- a/apps/sim/app/api/webhooks/route.test.ts +++ b/apps/sim/app/api/webhooks/route.test.ts @@ -607,4 +607,43 @@ describe('POST /api/webhooks credential references', () => { expect(response.status).toBe(403) expect(dbChainMockFns.set).not.toHaveBeenCalled() }) + + /** + * Clearing `credentialId` does not stop the save from acting with the stored + * credential: a recreate still cleans up the previous subscription with it. + */ + it.each([null, ''])( + 'still authorizes the stored credential when a re-save sends credentialId %j', + async (credentialId) => { + mocks.authorizeCredentialUseForAuth.mockResolvedValue({ + ok: false, + error: 'You do not have access to this credential.', + }) + mocks.shouldRecreateExternalWebhookSubscription.mockReturnValue(true) + queueUpdatePathRows(true, { credentialId: 'stored-credential' }) + + const response = await POST(upsertRequest({ credentialId })) + + expect(response.status).toBe(403) + expect(mocks.authorizeCredentialUseForAuth).toHaveBeenCalledWith(expect.anything(), { + credentialId: 'stored-credential', + workflowId: 'workflow-1', + }) + expect(mocks.createExternalWebhookSubscription).not.toHaveBeenCalled() + expect(dbChainMockFns.set).not.toHaveBeenCalled() + } + ) + + it('authorizes both credentials when a re-save replaces the stored one', async () => { + mocks.authorizeCredentialUseForAuth.mockResolvedValue({ ok: true, workspaceId: 'workspace-1' }) + queueUpdatePathRows(true, { credentialId: 'stored-credential' }) + + const response = await POST(upsertRequest({ credentialId: 'new-credential' })) + + expect(response.status).toBe(200) + expect(mocks.authorizeCredentialUseForAuth.mock.calls.map(([, params]) => params)).toEqual([ + { credentialId: 'new-credential', workflowId: 'workflow-1' }, + { credentialId: 'stored-credential', workflowId: 'workflow-1' }, + ]) + }) }) diff --git a/apps/sim/app/api/webhooks/route.ts b/apps/sim/app/api/webhooks/route.ts index eb453c5b5c9..645ba6f941a 100644 --- a/apps/sim/app/api/webhooks/route.ts +++ b/apps/sim/app/api/webhooks/route.ts @@ -420,17 +420,19 @@ export const POST = withRouteHandler(async (request: NextRequest) => { /** * Subscription handlers, pollers, and subscription cleanup look `credentialId` - * up by id alone and mint tokens as its owner. A save acts with the requested - * credential or, when the request omits it, the stored one, so that credential - * must be usable by the actor in the workflow's workspace before anything is - * subscribed, cleaned up, or saved. + * up by id alone and mint tokens as its owner. A save can act with both the + * requested credential and the stored one — the stored one is merged back when + * the request omits it, or used to clean up the previous subscription — so + * each must be usable by the actor in the workflow's workspace before anything + * is subscribed, cleaned up, or saved. */ - const effectiveCredentialId = - 'credentialId' in originalProviderConfig - ? originalProviderConfig.credentialId - : existingWebhook?.providerConfig?.credentialId - if (effectiveCredentialId != null && effectiveCredentialId !== '') { - if (typeof effectiveCredentialId !== 'string') { + const credentialIds = new Set( + [originalProviderConfig.credentialId, existingWebhook?.providerConfig?.credentialId].filter( + (id) => id != null && id !== '' + ) + ) + for (const credentialId of credentialIds) { + if (typeof credentialId !== 'string') { return NextResponse.json( { error: 'providerConfig.credentialId must be a literal credential id' }, { status: 400 } @@ -438,13 +440,13 @@ export const POST = withRouteHandler(async (request: NextRequest) => { } const credentialAccess = await authorizeCredentialUseForAuth( { success: true, userId, authType: AuthType.SESSION }, - { credentialId: effectiveCredentialId, workflowId } + { credentialId, workflowId } ) if (!credentialAccess.ok) { logger.warn(`[${requestId}] Webhook credential reference denied`, { userId, workflowId, - credentialId: effectiveCredentialId, + credentialId, reason: credentialAccess.error, }) return NextResponse.json({ error: credentialAccess.error }, { status: 403 }) From ad22bd12a42becaa75513d3f6b1be4fb11278906 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 12 Sep 2026 20:28:51 -0700 Subject: [PATCH 4/4] fix(webhooks): authorize the stored credential only when the save uses it --- apps/sim/app/api/webhooks/route.test.ts | 24 ++++++++++++++- apps/sim/app/api/webhooks/route.ts | 40 ++++++++++++++----------- 2 files changed, 45 insertions(+), 19 deletions(-) diff --git a/apps/sim/app/api/webhooks/route.test.ts b/apps/sim/app/api/webhooks/route.test.ts index d688721f2c4..332c7dd88b5 100644 --- a/apps/sim/app/api/webhooks/route.test.ts +++ b/apps/sim/app/api/webhooks/route.test.ts @@ -634,8 +634,30 @@ describe('POST /api/webhooks credential references', () => { } ) - it('authorizes both credentials when a re-save replaces the stored one', async () => { + /** Rotation without recreation never touches the old credential, so it needs no access to it. */ + it('rotates the credential without access to the stored one when nothing is recreated', async () => { + mocks.authorizeCredentialUseForAuth.mockImplementation(async (_auth, { credentialId }) => + credentialId === 'new-credential' + ? { ok: true, workspaceId: 'workspace-1' } + : { ok: false, error: 'You do not have access to this credential.' } + ) + queueUpdatePathRows(true, { credentialId: 'stored-credential' }) + + const response = await POST(upsertRequest({ credentialId: 'new-credential' })) + + expect(response.status).toBe(200) + expect(mocks.authorizeCredentialUseForAuth.mock.calls.map(([, params]) => params)).toEqual([ + { credentialId: 'new-credential', workflowId: 'workflow-1' }, + ]) + expect(dbChainMockFns.set).toHaveBeenCalledWith( + expect.objectContaining({ providerConfig: { credentialId: 'new-credential' } }) + ) + }) + + /** Recreation cleans up the previous subscription with the stored credential. */ + it('authorizes both credentials when a rotation recreates the subscription', async () => { mocks.authorizeCredentialUseForAuth.mockResolvedValue({ ok: true, workspaceId: 'workspace-1' }) + mocks.shouldRecreateExternalWebhookSubscription.mockReturnValue(true) queueUpdatePathRows(true, { credentialId: 'stored-credential' }) const response = await POST(upsertRequest({ credentialId: 'new-credential' })) diff --git a/apps/sim/app/api/webhooks/route.ts b/apps/sim/app/api/webhooks/route.ts index 645ba6f941a..62dd3ec6962 100644 --- a/apps/sim/app/api/webhooks/route.ts +++ b/apps/sim/app/api/webhooks/route.ts @@ -418,18 +418,32 @@ export const POST = withRouteHandler(async (request: NextRequest) => { existingWebhook = existingRows[0] || null } + const shouldRecreateSubscription = + existingWebhook && + shouldRecreateExternalWebhookSubscription({ + previousProvider: existingWebhook.provider as string, + nextProvider: provider, + previousConfig: ((existingWebhook.providerConfig as Record) || + {}) as Record, + nextConfig: resolvedProviderConfig, + }) + /** * Subscription handlers, pollers, and subscription cleanup look `credentialId` - * up by id alone and mint tokens as its owner. A save can act with both the - * requested credential and the stored one — the stored one is merged back when - * the request omits it, or used to clean up the previous subscription — so - * each must be usable by the actor in the workflow's workspace before anything - * is subscribed, cleaned up, or saved. + * up by id alone and mint tokens as its owner, so every credential this save + * acts with must be usable by the actor in the workflow's workspace before + * anything is subscribed, cleaned up, or saved. That is the requested + * credential, plus the stored one when the save uses it: merged back because + * the request omits `credentialId`, or used to clean up the previous + * subscription on recreation. */ + const usesStoredCredential = + existingWebhook && (shouldRecreateSubscription || !('credentialId' in originalProviderConfig)) const credentialIds = new Set( - [originalProviderConfig.credentialId, existingWebhook?.providerConfig?.credentialId].filter( - (id) => id != null && id !== '' - ) + [ + originalProviderConfig.credentialId, + usesStoredCredential ? existingWebhook.providerConfig?.credentialId : undefined, + ].filter((id) => id != null && id !== '') ) for (const credentialId of credentialIds) { if (typeof credentialId !== 'string') { @@ -492,16 +506,6 @@ export const POST = withRouteHandler(async (request: NextRequest) => { } } - const shouldRecreateSubscription = - existingWebhook && - shouldRecreateExternalWebhookSubscription({ - previousProvider: existingWebhook.provider as string, - nextProvider: provider, - previousConfig: ((existingWebhook.providerConfig as Record) || - {}) as Record, - nextConfig: resolvedProviderConfig, - }) - if (!existingWebhook || shouldRecreateSubscription) { try { const result = await createExternalWebhookSubscription(