diff --git a/apps/sim/app/api/webhooks/route.test.ts b/apps/sim/app/api/webhooks/route.test.ts index 85c318c1ca2..9399aa0415f 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,75 @@ 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, - }) - }) +const CREDENTIAL_ALLOWED = { ok: true, workspaceId: 'workspace-1' } +const CREDENTIAL_DENIED = { ok: false, error: 'You do not have access to this credential.' } - function upsertRequest() { - return createMockRequest('POST', { +/** 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' }, + }) + 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 +454,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 +501,205 @@ 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(CREDENTIAL_ALLOWED) + 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 `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('authorizes and keeps the stored credential on a re-save that omits it', async () => { + mocks.authorizeCredentialUseForAuth.mockResolvedValue(CREDENTIAL_ALLOWED) + queueUpdatePathRows(true, { credentialId: 'stored-credential', userId: 'stored-user' }) + + const response = await POST(upsertRequest({ eventType: 'record.created' })) + + expect(response.status).toBe(200) + 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' }, + }) + ) + }) + + it('refuses a re-save whose stored credential the actor cannot use', async () => { + mocks.authorizeCredentialUseForAuth.mockResolvedValue(CREDENTIAL_DENIED) + queueUpdatePathRows(true, { credentialId: 'stored-credential' }) + + const response = await POST(upsertRequest({ eventType: 'record.created' })) + + expect(response.status).toBe(403) + expect(mocks.createExternalWebhookSubscription).not.toHaveBeenCalled() + 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(CREDENTIAL_DENIED) + 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() + } + ) + + /** 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' ? CREDENTIAL_ALLOWED : CREDENTIAL_DENIED + ) + 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(CREDENTIAL_ALLOWED) + mocks.shouldRecreateExternalWebhookSubscription.mockReturnValue(true) + 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' }, + ]) + }) + + /** + * Recreation cleans up the previous subscription with the stored credential even + * when the request omits it, and a `userId` echoed back by the provider is not saved. + */ + it('authorizes the stored credential and drops userId when an omitting re-save recreates', async () => { + mocks.authorizeCredentialUseForAuth.mockResolvedValue(CREDENTIAL_ALLOWED) + mocks.shouldRecreateExternalWebhookSubscription.mockReturnValue(true) + mocks.createExternalWebhookSubscription.mockResolvedValue({ + updatedProviderConfig: { externalId: 'subscription-2', userId: 'stored-user' }, + externalSubscriptionCreated: true, + }) + queueUpdatePathRows(true, { credentialId: 'stored-credential', userId: 'stored-user' }) + + const response = await POST(upsertRequest({ eventType: 'record.created' })) + + expect(response.status).toBe(200) + expect(mocks.authorizeCredentialUseForAuth).toHaveBeenCalledWith(expect.anything(), { + credentialId: 'stored-credential', + workflowId: 'workflow-1', + }) + const savedConfig = dbChainMockFns.set.mock.calls.at(-1)?.[0].providerConfig + expect(savedConfig).toEqual({ eventType: 'record.created', externalId: 'subscription-2' }) + }) + + /** The permission-group refusal keeps answering first, before any credential lookup. */ + it('refuses a withheld creation before authorizing its credential', async () => { + permissionGroupScopeMockFns.mockResolvePermissionGroupConfig.mockResolvedValue({ + ...DEFAULT_PERMISSION_GROUP_CONFIG, + disableWebhookTriggers: true, + }) + queueCreatePathRows() + + const response = await POST(upsertRequest({ credentialId: 'victim-credential' })) + + expect(response.status).toBe(403) + expect(mocks.authorizeCredentialUseForAuth).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/app/api/webhooks/route.ts b/apps/sim/app/api/webhooks/route.ts index e96cb78b084..eb3942c7262 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,7 +377,13 @@ 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. 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( originalProviderConfig, userId, @@ -389,7 +398,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => { providerConfig: providerConfigOverride, }) - const userProvided = originalProviderConfig as Record + const userProvided = originalProviderConfig const configToSave: Record = { ...userProvided } if (targetWebhookId) { @@ -450,6 +459,48 @@ export const POST = withRouteHandler(async (request: NextRequest) => { nextConfig: resolvedProviderConfig, }) + /** + * Subscription handlers, pollers, and subscription cleanup look `credentialId` + * 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: the requested one, and the + * stored one when it is merged back (the request omits `credentialId`) or + * cleans up the previous subscription (recreation). + */ + const usesStoredCredential = + existingWebhook && (shouldRecreateSubscription || !('credentialId' in originalProviderConfig)) + const credentialIds = [ + originalProviderConfig.credentialId, + usesStoredCredential ? existingWebhook.providerConfig?.credentialId : undefined, + ].filter((id) => id != null && id !== '') + + /** The row stores the unresolved text, so only a literal credential id can be authorized. */ + if ( + resolvedProviderConfig.credentialId !== originalProviderConfig.credentialId || + !credentialIds.every((id): id is string => typeof id === 'string') + ) { + return NextResponse.json( + { error: 'providerConfig.credentialId must be a literal credential id' }, + { status: 400 } + ) + } + + for (const credentialId of new Set(credentialIds)) { + const credentialAccess = await authorizeCredentialUseForAuth( + { success: true, userId, authType: AuthType.SESSION }, + { credentialId, workflowId } + ) + if (!credentialAccess.ok) { + logger.warn(`[${requestId}] Webhook credential reference denied`, { + userId, + workflowId, + credentialId, + reason: credentialAccess.error, + }) + return NextResponse.json({ error: credentialAccess.error }, { status: 403 }) + } + } + if (!existingWebhook || shouldRecreateSubscription) { try { const result = await createExternalWebhookSubscription( @@ -480,6 +531,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => { userProvided ) } + configToSave.userId = undefined try { if (targetWebhookId) {