diff --git a/packages/asana/webhooks/challenge.test.ts b/packages/asana/webhooks/challenge.test.ts index 23d503a8d..7dd7bd630 100644 --- a/packages/asana/webhooks/challenge.test.ts +++ b/packages/asana/webhooks/challenge.test.ts @@ -7,7 +7,9 @@ type ChallengeRequest = Parameters[1]; function createContext(storedSecret: string | null) { const keys = { get_webhook_signature: jest.fn().mockResolvedValue(storedSecret), - set_webhook_signature: jest.fn().mockResolvedValue(undefined), + set_webhook_signature_if_absent: jest + .fn() + .mockResolvedValue({ created: true }), }; return { ctx: { keys } as unknown as ChallengeContext, keys }; @@ -43,7 +45,9 @@ describe('asana challenge webhook', () => { createRequest({ 'x-hook-secret': 'asana-secret' }), ); - expect(keys.set_webhook_signature).toHaveBeenCalledWith('asana-secret'); + expect(keys.set_webhook_signature_if_absent).toHaveBeenCalledWith( + 'asana-secret', + ); expect(result.success).toBe(true); expect(result.responseHeaders).toEqual({ 'X-Hook-Secret': 'asana-secret', @@ -62,13 +66,11 @@ describe('asana challenge webhook', () => { createRequest({ 'x-hook-secret': 'attacker-secret' }), ); - expect(keys.set_webhook_signature).not.toHaveBeenCalled(); + expect(keys.set_webhook_signature_if_absent).not.toHaveBeenCalled(); expect(result.success).toBe(false); expect(result.statusCode).toBe(401); - // The sender's value must not be echoed back. expect(result.responseHeaders).toBeUndefined(); expect(result.data).toBeUndefined(); - // Operators need a signal; the rejection is otherwise invisible. expect(warn).toHaveBeenCalledTimes(1); warn.mockRestore(); @@ -83,7 +85,7 @@ describe('asana challenge webhook', () => { createRequest({ 'x-hook-secret': 'bbbbbbbb' }), ); - expect(keys.set_webhook_signature).not.toHaveBeenCalled(); + expect(keys.set_webhook_signature_if_absent).not.toHaveBeenCalled(); expect(result.success).toBe(false); expect(result.statusCode).toBe(401); @@ -98,7 +100,7 @@ describe('asana challenge webhook', () => { createRequest({ 'x-hook-secret': 'asana-secret' }), ); - expect(keys.set_webhook_signature).not.toHaveBeenCalled(); + expect(keys.set_webhook_signature_if_absent).not.toHaveBeenCalled(); expect(result.success).toBe(true); expect(result.responseHeaders).toEqual({ 'X-Hook-Secret': 'asana-secret', @@ -114,13 +116,15 @@ describe('asana challenge webhook', () => { expect(result.success).toBe(false); expect(result.statusCode).toBe(400); - expect(keys.set_webhook_signature).not.toHaveBeenCalled(); + expect(keys.set_webhook_signature_if_absent).not.toHaveBeenCalled(); expect(keys.get_webhook_signature).not.toHaveBeenCalled(); }); it('does not echo a secret it failed to persist', async () => { const { ctx, keys } = createContext(null); - keys.set_webhook_signature.mockRejectedValue(new Error('db down')); + keys.set_webhook_signature_if_absent.mockRejectedValue( + new Error('db down'), + ); const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); const result = await challenge.handler( @@ -134,5 +138,28 @@ describe('asana challenge webhook', () => { warn.mockRestore(); }); + + it('returns 401 when a concurrent writer already stored a different secret', async () => { + const { ctx, keys } = createContext(null); + keys.get_webhook_signature + .mockResolvedValueOnce(null) + .mockResolvedValueOnce('already-stored'); + keys.set_webhook_signature_if_absent.mockRejectedValue( + new Error('Webhook signature already configured'), + ); + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + + const result = await challenge.handler( + ctx, + createRequest({ 'x-hook-secret': 'asana-secret' }), + ); + + expect(result.success).toBe(false); + expect(result.statusCode).toBe(401); + expect(result.responseHeaders).toBeUndefined(); + expect(warn).toHaveBeenCalled(); + + warn.mockRestore(); + }); }); }); diff --git a/packages/asana/webhooks/challenge.ts b/packages/asana/webhooks/challenge.ts index 864ccbc7d..a5b32f2ac 100644 --- a/packages/asana/webhooks/challenge.ts +++ b/packages/asana/webhooks/challenge.ts @@ -48,6 +48,19 @@ export const challenge: AsanaWebhooks['challenge'] = { }; } + const rejectExistingSecret = () => { + // Logged so operators can tell an attack from a blocked + // re-registration; both land here. + console.warn( + '[corsair:asana] Rejected X-Hook-Secret for an account that already has one configured', + ); + return { + success: false as const, + statusCode: 401, + error: 'Webhook signing secret is already configured', + }; + }; + const storedSecret = await ctx.keys.get_webhook_signature(); if (storedSecret) { @@ -56,16 +69,7 @@ export const challenge: AsanaWebhooks['challenge'] = { // rewriting storage. A different value belongs to no handshake Corsair // started, so refuse it and do not echo the sender's value back. if (!secretsMatch(storedSecret, hookSecret)) { - // Logged so operators can tell an attack from a blocked - // re-registration; both land here. - console.warn( - '[corsair:asana] Rejected X-Hook-Secret for an account that already has one configured', - ); - return { - success: false, - statusCode: 401, - error: 'Webhook signing secret is already configured', - }; + return rejectExistingSecret(); } return { @@ -78,8 +82,12 @@ export const challenge: AsanaWebhooks['challenge'] = { } try { - await ctx.keys.set_webhook_signature(hookSecret); + await ctx.keys.set_webhook_signature_if_absent(hookSecret); } catch (error) { + const existing = await ctx.keys.get_webhook_signature(); + if (existing && !secretsMatch(existing, hookSecret)) { + return rejectExistingSecret(); + } // Echoing a secret we failed to store would leave Asana signing events // with a key this account cannot verify. console.warn( diff --git a/packages/corsair/core/auth/key-manager.ts b/packages/corsair/core/auth/key-manager.ts index cd43e1e74..3a3e10104 100644 --- a/packages/corsair/core/auth/key-manager.ts +++ b/packages/corsair/core/auth/key-manager.ts @@ -468,9 +468,9 @@ export function createAccountKeyManager( return decryptConfig(config, dek); }; - // Serialize config writes — same lost-update hazard as the integration - // manager above (this is what silently dropped Outlook's refreshed token - // when its keyBuilder persisted via Promise.all). + // Serialize config writes on this manager instance (same lost-update hazard + // that dropped Outlook's refreshed token under Promise.all). Cross-manager + // races use optimistic CAS on the opaque encrypted config blob below. let configWriteChain: Promise = Promise.resolve(); const updateConfig = ( updates: Record, @@ -480,62 +480,210 @@ export function createAccountKeyManager( return run; }; - const doUpdateConfig = async ( - updates: Record, - ): Promise => { - const dek = await getDecryptedDek(); + type AccountConfigRow = { + id: string; + config: unknown; + dek: string | null | undefined; + }; + + const loadFreshAccountConfig = async (): Promise<{ + row: AccountConfigRow; + dek: string; + currentConfig: Record; + }> => { + cachedAccount = null; + cachedDek = null; + + const account = await ctx.getAccount(); + const row = await database.db + .selectFrom('corsair_accounts') + .selectAll() + .where('id', '=', account.id) + .executeTakeFirstOrThrow(); + + if (!row.dek) { + throw new Error( + `No DEK found for account (tenant: "${tenantId}", integration: "${integrationName}"). Initialize the account first.`, + ); + } + + const dek = await decryptDEK(row.dek, kek); + const rawConfig = parseConfig(row.config) as Record; let currentConfig: Record; try { - currentConfig = await getDecryptedConfig(); + currentConfig = + !rawConfig || Object.keys(rawConfig).length === 0 + ? {} + : decryptConfig(rawConfig, dek); } catch (err) { console.error( - `[corsair] Failed to decrypt config for account (tenant: "${tenantId}", integration: "${integrationName}"), starting fresh:`, + `[corsair] Failed to decrypt config for account (tenant: "${tenantId}", integration: "${integrationName}"):`, err, ); - currentConfig = {}; + // Never CAS-write from a failed decrypt — that would wipe real secrets. + throw err; } - const newConfig = { ...currentConfig }; - for (const [key, value] of Object.entries(updates)) { - if (value === null) { - delete newConfig[key]; - } else { - newConfig[key] = value; + return { row, dek, currentConfig }; + }; + + const casWriteAccountConfig = async ( + row: AccountConfigRow, + encryptedConfig: Record, + encryptedDek?: string, + ): Promise => { + let query = database.db + .updateTable('corsair_accounts') + .set({ + config: encryptedConfig, + ...(encryptedDek !== undefined ? { dek: encryptedDek } : {}), + updated_at: new Date(), + }) + .where('id', '=', row.id) + .where('config', '=', row.config as any); + + // DEK rotation also locks on the prior dek so a concurrent config write + // (or another rotator) forces a clean re-read instead of a blind overwrite. + if (encryptedDek !== undefined) { + query = + row.dek == null + ? query.where('dek', 'is', null) + : query.where('dek', '=', row.dek); + } + + const result = await query.executeTakeFirst(); + + cachedAccount = null; + cachedDek = null; + + return result.numUpdatedRows !== undefined && result.numUpdatedRows > 0n; + }; + + const doUpdateConfig = async ( + updates: Record, + ): Promise => { + for (let attempt = 0; attempt < 5; attempt++) { + const { row, dek, currentConfig } = await loadFreshAccountConfig(); + + const newConfig = { ...currentConfig }; + for (const [key, value] of Object.entries(updates)) { + if (value === null) { + delete newConfig[key]; + } else { + newConfig[key] = value; + } + } + + const encryptedConfig = encryptConfig(newConfig, dek); + if (await casWriteAccountConfig(row, encryptedConfig)) { + return; } } - const encryptedConfig = encryptConfig(newConfig, dek); - await ctx.updateAccount({ config: encryptedConfig }); + throw new Error( + `Failed to update account config atomically (tenant: "${tenantId}", integration: "${integrationName}")`, + ); }; - // Build the key manager - const manager: Record = { - get_dek: getDecryptedDek, + const setWebhookSignatureIfAbsent = ( + value: string, + ): Promise<{ created: boolean }> => { + // Trim only for emptiness — store the exact value handlers echo/compare. + if (!value.trim()) { + return Promise.reject(new Error('Webhook signature cannot be empty')); + } + const normalized = value; + + // Share the write chain with set_* on this manager. Cross-manager races + // reuse the same optimistic config CAS as doUpdateConfig. + const run = configWriteChain.then(async () => { + for (let attempt = 0; attempt < 5; attempt++) { + const { row, dek, currentConfig } = await loadFreshAccountConfig(); + + const existing = currentConfig.webhook_signature; + if (existing) { + if (existing !== normalized) { + throw new Error('Webhook signature already configured'); + } + return { created: false }; + } - issue_new_dek: async () => { - const account = await ctx.getAccount(); - const newDek = generateDEK(); + const encryptedConfig = encryptConfig( + { ...currentConfig, webhook_signature: normalized }, + dek, + ); - // If there's an existing DEK, re-encrypt config; otherwise start fresh - let newConfig: Record = {}; - if (account.dek) { - const oldDek = await decryptDEK(account.dek, kek); - const config = account.config as Record; - if (config && Object.keys(config).length > 0) { - newConfig = reEncryptConfig(config, oldDek, newDek); + if (await casWriteAccountConfig(row, encryptedConfig)) { + return { created: true }; } } - const encryptedNewDek = await encryptDEK(newDek, kek); + throw new Error('Failed to set webhook signature atomically'); + }); - await ctx.updateAccount({ - config: newConfig, - dek: encryptedNewDek, - }); + configWriteChain = run.then( + () => undefined, + () => undefined, + ); + return run; + }; - cachedDek = newDek; - return newDek; - }, + const issueNewDek = (): Promise => { + const run = configWriteChain.then(async () => { + for (let attempt = 0; attempt < 5; attempt++) { + cachedAccount = null; + cachedDek = null; + + const account = await ctx.getAccount(); + const row = await database.db + .selectFrom('corsair_accounts') + .selectAll() + .where('id', '=', account.id) + .executeTakeFirstOrThrow(); + + const newDek = generateDEK(); + const config = parseConfig(row.config) as Record; + const hasConfig = !!config && Object.keys(config).length > 0; + let newConfig: Record = {}; + + if (row.dek) { + if (hasConfig) { + const oldDek = await decryptDEK(row.dek, kek); + newConfig = reEncryptConfig(config, oldDek, newDek); + } + } else if (hasConfig) { + // Config without a DEK is unreadable — refuse rather than wipe it. + throw new Error( + `Account has encrypted config but no DEK (tenant: "${tenantId}", integration: "${integrationName}"). Recover the DEK before rotating.`, + ); + } + + const encryptedNewDek = await encryptDEK(newDek, kek); + if (await casWriteAccountConfig(row, newConfig, encryptedNewDek)) { + cachedDek = newDek; + return newDek; + } + } + + throw new Error( + `Failed to rotate account DEK atomically (tenant: "${tenantId}", integration: "${integrationName}")`, + ); + }); + + configWriteChain = run.then( + () => undefined, + () => undefined, + ); + return run; + }; + + // Build the key manager + const manager: Record = { + get_dek: getDecryptedDek, + + issue_new_dek: issueNewDek, + + set_webhook_signature_if_absent: setWebhookSignatureIfAbsent, // Auto-generated field accessors ...createFieldAccessors(getDecryptedConfig, updateConfig, allFields), diff --git a/packages/corsair/core/auth/types.ts b/packages/corsair/core/auth/types.ts index 9b3cdc88f..551a35ad3 100644 --- a/packages/corsair/core/auth/types.ts +++ b/packages/corsair/core/auth/types.ts @@ -210,8 +210,16 @@ export type AccountKeyManagerFor< T extends AuthTypes, Config extends PluginAuthConfig | undefined = undefined, > = BaseKeyManager & - AllFieldAccessors> & - (T extends 'oauth_2' + AllFieldAccessors> & { + /** + * Persist webhook_signature only when unset. If another value is already + * stored, rejects. Use this for unauthenticated handshake registration + * (Notion url_verification, Asana X-Hook-Secret, etc.). + */ + set_webhook_signature_if_absent: ( + value: string, + ) => Promise<{ created: boolean }>; + } & (T extends 'oauth_2' ? { /** * Get the integration-level OAuth2 credentials (client_id, client_secret, redirect_url). diff --git a/packages/corsair/tests/process-webhook-status.test.ts b/packages/corsair/tests/process-webhook-status.test.ts new file mode 100644 index 000000000..08bac3759 --- /dev/null +++ b/packages/corsair/tests/process-webhook-status.test.ts @@ -0,0 +1,67 @@ +import { processWebhook } from '../webhooks/index'; + +describe('processWebhook status propagation', () => { + it('forwards success: false and statusCode from the handler', async () => { + const corsair = { + notion: { + webhooks: { + verification: { + match: () => true, + handler: async () => ({ + success: false, + statusCode: 401, + error: 'Invalid verification token', + }), + }, + }, + pluginWebhookMatcher: () => true, + }, + } as any; + + const result = await processWebhook( + corsair, + { 'content-type': 'application/json' }, + { type: 'url_verification', verification_token: 'x' }, + undefined, + { plugin: 'notion' }, + ); + + expect(result.plugin).toBe('notion'); + expect(result.response).toEqual({ + success: false, + statusCode: 401, + error: 'Invalid verification token', + data: undefined, + }); + }); + + it('maps thrown handler errors to success: false with statusCode 500', async () => { + const corsair = { + notion: { + webhooks: { + verification: { + match: () => true, + handler: async () => { + throw new Error('boom'); + }, + }, + }, + pluginWebhookMatcher: () => true, + }, + } as any; + + const result = await processWebhook( + corsair, + { 'content-type': 'application/json' }, + { type: 'url_verification' }, + undefined, + { plugin: 'notion' }, + ); + + expect(result.response).toEqual({ + success: false, + statusCode: 500, + error: 'Internal server error', + }); + }); +}); diff --git a/packages/corsair/tests/webhook-signature-if-absent.test.ts b/packages/corsair/tests/webhook-signature-if-absent.test.ts new file mode 100644 index 000000000..5c7fb460c --- /dev/null +++ b/packages/corsair/tests/webhook-signature-if-absent.test.ts @@ -0,0 +1,267 @@ +import { + encryptConfig, + encryptDEK, + generateDEK, +} from '../core/auth/encryption'; +import { createAccountKeyManager } from '../core/auth/key-manager'; +import { createTestDatabase } from './setup-db'; + +const KEK = 'test-kek-with-at-least-32-characters!!'; + +async function seedAccount( + database: ReturnType['database'], +) { + const now = new Date(); + const dek = generateDEK(); + const encryptedDek = await encryptDEK(dek, KEK); + + await database.db + .insertInto('corsair_integrations') + .values({ + id: 'integration-notion', + created_at: now, + updated_at: now, + name: 'notion', + config: encryptConfig({}, dek), + dek: encryptedDek, + }) + .execute(); + + await database.db + .insertInto('corsair_accounts') + .values({ + id: 'account-default', + created_at: now, + updated_at: now, + tenant_id: 'default', + integration_id: 'integration-notion', + config: encryptConfig({ access_token: 'tok' }, dek), + dek: encryptedDek, + }) + .execute(); +} + +function makeManager( + database: ReturnType['database'], +) { + return createAccountKeyManager({ + authType: 'oauth_2', + integrationName: 'notion', + tenantId: 'default', + kek: KEK, + database, + }); +} + +describe('set_webhook_signature_if_absent', () => { + it('creates the signature when none is stored', async () => { + const { database, cleanup } = createTestDatabase(); + try { + await seedAccount(database); + const km = makeManager(database); + + await expect( + km.set_webhook_signature_if_absent('secret-a'), + ).resolves.toEqual({ created: true }); + expect(await km.get_webhook_signature()).toBe('secret-a'); + } finally { + cleanup(); + } + }); + + it('is a no-op when the same secret is already stored', async () => { + const { database, cleanup } = createTestDatabase(); + try { + await seedAccount(database); + const km = makeManager(database); + await km.set_webhook_signature('secret-a'); + + await expect( + km.set_webhook_signature_if_absent('secret-a'), + ).resolves.toEqual({ created: false }); + expect(await km.get_webhook_signature()).toBe('secret-a'); + } finally { + cleanup(); + } + }); + + it('rejects a different secret when one is already stored', async () => { + const { database, cleanup } = createTestDatabase(); + try { + await seedAccount(database); + const km = makeManager(database); + await km.set_webhook_signature('secret-a'); + + await expect( + km.set_webhook_signature_if_absent('secret-b'), + ).rejects.toThrow('Webhook signature already configured'); + expect(await km.get_webhook_signature()).toBe('secret-a'); + } finally { + cleanup(); + } + }); + + it('rejects an empty or whitespace-only signature', async () => { + const { database, cleanup } = createTestDatabase(); + try { + await seedAccount(database); + const km = makeManager(database); + + await expect(km.set_webhook_signature_if_absent('')).rejects.toThrow( + 'Webhook signature cannot be empty', + ); + await expect(km.set_webhook_signature_if_absent(' ')).rejects.toThrow( + 'Webhook signature cannot be empty', + ); + } finally { + cleanup(); + } + }); + + it('lets only one of two concurrent managers create the first secret', async () => { + const { database, cleanup } = createTestDatabase(); + try { + await seedAccount(database); + const a = makeManager(database); + const b = makeManager(database); + + const results = await Promise.allSettled([ + a.set_webhook_signature_if_absent('secret-a'), + b.set_webhook_signature_if_absent('secret-b'), + ]); + + const fulfilled = results.filter((r) => r.status === 'fulfilled'); + const rejected = results.filter((r) => r.status === 'rejected'); + + expect(fulfilled).toHaveLength(1); + expect(rejected).toHaveLength(1); + expect( + (fulfilled[0] as PromiseFulfilledResult<{ created: boolean }>).value, + ).toEqual({ + created: true, + }); + expect((rejected[0] as PromiseRejectedResult).reason).toEqual( + expect.objectContaining({ + message: 'Webhook signature already configured', + }), + ); + + const stored = await a.get_webhook_signature(); + expect(stored === 'secret-a' || stored === 'secret-b').toBe(true); + } finally { + cleanup(); + } + }); + + it('does not let a concurrent set_* wipe a just-created signature', async () => { + const { database, cleanup } = createTestDatabase(); + try { + await seedAccount(database); + const registrar = makeManager(database); + const tokenWriter = makeManager(database); + + await Promise.all([ + registrar.set_webhook_signature_if_absent('secret-a'), + tokenWriter.set_access_token('tok-fresh'), + ]); + + expect(await registrar.get_webhook_signature()).toBe('secret-a'); + expect(await tokenWriter.get_access_token()).toBe('tok-fresh'); + } finally { + cleanup(); + } + }); + + it('does not let a concurrent issue_new_dek wipe a just-created signature', async () => { + const { database, cleanup } = createTestDatabase(); + try { + await seedAccount(database); + const registrar = makeManager(database); + const rotator = makeManager(database); + + await Promise.all([ + registrar.set_webhook_signature_if_absent('secret-a'), + rotator.issue_new_dek(), + ]); + + expect(await registrar.get_webhook_signature()).toBe('secret-a'); + expect(await registrar.get_access_token()).toBe('tok'); + } finally { + cleanup(); + } + }); + + it('refuses issue_new_dek when config exists without a DEK', async () => { + const { database, cleanup } = createTestDatabase(); + try { + await seedAccount(database); + const before = await database.db + .selectFrom('corsair_accounts') + .select(['config', 'dek']) + .where('id', '=', 'account-default') + .executeTakeFirstOrThrow(); + + await database.db + .updateTable('corsair_accounts') + .set({ dek: null }) + .where('id', '=', 'account-default') + .execute(); + + const km = makeManager(database); + await expect(km.issue_new_dek()).rejects.toThrow( + /encrypted config but no DEK/, + ); + + const after = await database.db + .selectFrom('corsair_accounts') + .select(['config', 'dek']) + .where('id', '=', 'account-default') + .executeTakeFirstOrThrow(); + + expect(after.config).toEqual(before.config); + expect(after.dek).toBeNull(); + } finally { + cleanup(); + } + }); + + it('does not write when the stored config cannot be decrypted', async () => { + const { database, cleanup } = createTestDatabase(); + try { + await seedAccount(database); + const wrongDek = generateDEK(); + await database.db + .updateTable('corsair_accounts') + .set({ + config: encryptConfig({ access_token: 'tok' }, wrongDek), + }) + .where('id', '=', 'account-default') + .execute(); + + const before = await database.db + .selectFrom('corsair_accounts') + .select('config') + .where('id', '=', 'account-default') + .executeTakeFirstOrThrow(); + + const km = makeManager(database); + const error = jest.spyOn(console, 'error').mockImplementation(() => {}); + + await expect( + km.set_webhook_signature_if_absent('secret-a'), + ).rejects.toThrow(); + + const after = await database.db + .selectFrom('corsair_accounts') + .select('config') + .where('id', '=', 'account-default') + .executeTakeFirstOrThrow(); + + expect(after.config).toEqual(before.config); + + error.mockRestore(); + } finally { + cleanup(); + } + }); +}); diff --git a/packages/corsair/tunnel/index.ts b/packages/corsair/tunnel/index.ts index eafc34fb2..43d1205b8 100644 --- a/packages/corsair/tunnel/index.ts +++ b/packages/corsair/tunnel/index.ts @@ -275,13 +275,25 @@ async function handleWebhookTunnel( } if (result.response && result.response.success === false) { + // Deliver the handler status to the original sender (e.g. 401 on a + // rejected handshake). Marking the tunnel envelope failed would show up + // as a transport error instead. return { - status: 'failed', - retryable: false, - error: - typeof result.response.error === 'string' - ? result.response.error - : 'Webhook handler failed', + status: 'ok', + webhookResponse: { + status: result.response.statusCode ?? 401, + body: { + success: false, + error: + typeof result.response.error === 'string' + ? result.response.error + : 'Webhook handler failed', + ...(result.response.data !== undefined && { + data: result.response.data, + }), + }, + headers: result.responseHeaders, + }, }; } diff --git a/packages/corsair/webhooks/index.ts b/packages/corsair/webhooks/index.ts index 33c78e7a1..3d4703388 100644 --- a/packages/corsair/webhooks/index.ts +++ b/packages/corsair/webhooks/index.ts @@ -260,6 +260,23 @@ export async function processWebhook( try { const response = await matched.webhook.handler(webhookRequest); + if (response.success === false) { + return { + plugin: pluginId, + action, + body: parsedBody, + response: { + success: false, + error: response.error, + statusCode: response.statusCode, + data: response.data, + }, + ...(response.responseHeaders && { + responseHeaders: response.responseHeaders, + }), + }; + } + const returnToSenderObjectExists = !!Object.keys( response.returnToSender || {}, )?.length; @@ -286,7 +303,9 @@ export async function processWebhook( body: parsedBody, response: { success: false, - error: error instanceof Error ? error.message : 'Unknown error', + statusCode: 500, + // Don't leak Error.message to webhook senders. + error: 'Internal server error', }, }; } diff --git a/packages/corsair/webhooks/tenant-links.ts b/packages/corsair/webhooks/tenant-links.ts index a30434bcc..c52f1ffcd 100644 --- a/packages/corsair/webhooks/tenant-links.ts +++ b/packages/corsair/webhooks/tenant-links.ts @@ -71,35 +71,48 @@ async function writeEncryptedAccountLinkField(options: { accountId: string; link: WebhookTenantLink; }): Promise { - const account = await options.database.db - .selectFrom('corsair_accounts') - .selectAll() - .where('id', '=', options.accountId) - .executeTakeFirst(); + // Optimistic CAS on the encrypted config blob — same race class as account + // key-manager set_* (e.g. concurrent webhook_signature registration). + for (let attempt = 0; attempt < 5; attempt++) { + const account = await options.database.db + .selectFrom('corsair_accounts') + .selectAll() + .where('id', '=', options.accountId) + .executeTakeFirst(); + + if (!account?.dek) { + throw new Error(`Account '${options.accountId}' has no DEK.`); + } - if (!account?.dek) { - throw new Error(`Account '${options.accountId}' has no DEK.`); - } + const dek = await decryptDEK(account.dek, options.kek); + const storedConfig = parseConfig(account.config) as Record; + let decryptedConfig: Record = {}; - const dek = await decryptDEK(account.dek, options.kek); - const storedConfig = parseConfig(account.config) as Record; - let decryptedConfig: Record = {}; + if (Object.keys(storedConfig).length > 0) { + decryptedConfig = decryptConfig(storedConfig, dek); + } - if (Object.keys(storedConfig).length > 0) { - decryptedConfig = decryptConfig(storedConfig, dek); + decryptedConfig[options.link.linkType] = options.link.externalId; + const encryptedConfig = encryptConfig(decryptedConfig, dek); + + const result = await options.database.db + .updateTable('corsair_accounts') + .set({ + config: encryptedConfig, + updated_at: new Date(), + }) + .where('id', '=', account.id) + .where('config', '=', account.config as any) + .executeTakeFirst(); + + if (result.numUpdatedRows !== undefined && result.numUpdatedRows > 0n) { + return; + } } - decryptedConfig[options.link.linkType] = options.link.externalId; - const encryptedConfig = encryptConfig(decryptedConfig, dek); - - await options.database.db - .updateTable('corsair_accounts') - .set({ - config: encryptedConfig, - updated_at: new Date(), - }) - .where('id', '=', account.id) - .execute(); + throw new Error( + `Failed to write webhook tenant link atomically for account '${options.accountId}'`, + ); } export async function setWebhookTenantLink(options: { diff --git a/packages/notion/webhooks.test.ts b/packages/notion/webhooks.test.ts new file mode 100644 index 000000000..188b8762c --- /dev/null +++ b/packages/notion/webhooks.test.ts @@ -0,0 +1,124 @@ +import { verification } from './webhooks/verification'; + +describe('Notion Webhook Verification', () => { + let webhookSignature: string | null = null; + let mockCtx: { + keys: { + get_webhook_signature: jest.Mock; + set_webhook_signature_if_absent: jest.Mock; + }; + }; + + beforeEach(() => { + webhookSignature = null; + mockCtx = { + keys: { + get_webhook_signature: jest + .fn() + .mockImplementation(async () => webhookSignature), + set_webhook_signature_if_absent: jest + .fn() + .mockImplementation(async (value: string) => { + if (webhookSignature && webhookSignature !== value) { + throw new Error('Webhook signature already configured'); + } + if (webhookSignature === value) { + return { created: false }; + } + webhookSignature = value; + return { created: true }; + }), + }, + }; + }); + + it('returns success: false if verification_token is missing', async () => { + const result = await verification.handler( + mockCtx as any, + { + payload: {}, + } as any, + ); + + expect(result).toEqual({ + success: false, + statusCode: 400, + error: 'Missing verification_token', + data: undefined, + }); + expect(mockCtx.keys.set_webhook_signature_if_absent).not.toHaveBeenCalled(); + }); + + it('persists verification_token on first handshake', async () => { + const result = await verification.handler( + mockCtx as any, + { + payload: { verification_token: 'new-token-123' }, + } as any, + ); + + expect(result).toMatchObject({ + success: true, + returnToSender: { verification_token: 'new-token-123' }, + }); + expect(webhookSignature).toBe('new-token-123'); + expect(mockCtx.keys.set_webhook_signature_if_absent).toHaveBeenCalledWith( + 'new-token-123', + ); + }); + + it('succeeds on a matching retry without rewriting', async () => { + webhookSignature = 'existing-secret-456'; + + const result = await verification.handler( + mockCtx as any, + { + payload: { verification_token: 'existing-secret-456' }, + } as any, + ); + + expect(result).toMatchObject({ success: true }); + expect(webhookSignature).toBe('existing-secret-456'); + }); + + it('returns 401 when the token does not match a stored secret', async () => { + webhookSignature = 'existing-secret-456'; + + const result = await verification.handler( + mockCtx as any, + { + payload: { verification_token: 'attacker-token-789' }, + } as any, + ); + + expect(result).toEqual({ + success: false, + statusCode: 401, + error: 'Invalid verification token', + }); + expect(webhookSignature).toBe('existing-secret-456'); + }); + + it('returns 500 when persistence fails and no secret is stored', async () => { + mockCtx.keys.set_webhook_signature_if_absent.mockRejectedValue( + new Error('db down'), + ); + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + + const result = await verification.handler( + mockCtx as any, + { + payload: { verification_token: 'new-token-123' }, + } as any, + ); + + expect(result).toEqual({ + success: false, + statusCode: 500, + error: 'Failed to persist verification token', + }); + expect(warn).toHaveBeenCalled(); + + warn.mockRestore(); + }); +}); diff --git a/packages/notion/webhooks/verification.ts b/packages/notion/webhooks/verification.ts index ca193c955..ccc505d06 100644 --- a/packages/notion/webhooks/verification.ts +++ b/packages/notion/webhooks/verification.ts @@ -1,6 +1,17 @@ +import crypto from 'crypto'; import type { NotionWebhooks } from '../index'; import { createNotionMatch } from './types'; +/** Constant-time compare. Length may short-circuit; length is not sensitive. */ +function secretsMatch(a: string, b: string): boolean { + const aBuf = Buffer.from(a); + const bBuf = Buffer.from(b); + if (aBuf.length !== bBuf.length) { + return false; + } + return crypto.timingSafeEqual(aBuf, bBuf); +} + export const verification: NotionWebhooks['verification'] = { match: createNotionMatch('url_verification'), handler: async (ctx, request) => { @@ -10,22 +21,47 @@ export const verification: NotionWebhooks['verification'] = { ) { return { success: false, + statusCode: 400, + error: 'Missing verification_token', data: undefined, }; } - ctx.keys.set_webhook_signature(request.payload.verification_token); - console.log( - `Enter this key in your Notion webhook verification modal: ${ctx.key}`, - ); + const token = request.payload.verification_token; + + try { + await ctx.keys.set_webhook_signature_if_absent(token); + } catch (error) { + const existing = await ctx.keys.get_webhook_signature(); + if (existing && !secretsMatch(existing, token)) { + return { + success: false, + statusCode: 401, + error: 'Invalid verification token', + }; + } + if (!existing) { + console.warn( + '[corsair:notion] Failed to persist verification token:', + error instanceof Error ? error.message : String(error), + ); + return { + success: false, + statusCode: 500, + error: 'Failed to persist verification token', + }; + } + } + + console.log('Notion webhook verification request received'); return { success: true, returnToSender: { - verification_token: request.payload.verification_token, + verification_token: token, }, data: { - verification_token: request.payload.verification_token, + verification_token: token, type: 'url_verification', }, }; diff --git a/www/src/app/api/webhooks/route.ts b/www/src/app/api/webhooks/route.ts index 2768a618c..36d263172 100644 --- a/www/src/app/api/webhooks/route.ts +++ b/www/src/app/api/webhooks/route.ts @@ -43,11 +43,15 @@ export async function POST(request: NextRequest) { ); } - if (result.response !== undefined) { - return NextResponse.json(result.response, { headers: nextHeaders }); - } - - return new NextResponse(null, { status: 200, headers: nextHeaders }); + const status = + result.response.success === false + ? (result.response.statusCode ?? 500) + : (result.response.statusCode ?? 200); + + return NextResponse.json(result.response, { + status, + headers: nextHeaders, + }); } export async function GET() {