From 460c2d15d5bb71d1032a5d01d19f727ef1da5612 Mon Sep 17 00:00:00 2001 From: Anwanga-Abasi Frriday Date: Wed, 29 Jul 2026 22:12:07 +0100 Subject: [PATCH] feat(webhooks): add webhook signing secret rotation endpoint --- docs/SECRETS.md | 11 +- src/app.ts | 12 ++ src/db/repositories/webhookRepository.test.ts | 107 ++++++++++++++++++ ..._add_webhook_previous_secret_expires_at.ts | 24 ++++ src/routes/webhooks.ts | 22 +++- .../webhooks/__tests__/rotation.test.ts | 19 ++-- src/services/webhooks/rotationService.ts | 7 +- src/services/webhooks/service.test.ts | 50 ++++++++ src/services/webhooks/service.ts | 22 ++-- tests/routes/webhooks.test.ts | 6 + 10 files changed, 253 insertions(+), 27 deletions(-) create mode 100644 src/db/repositories/webhookRepository.test.ts create mode 100644 src/migrations/032_add_webhook_previous_secret_expires_at.ts diff --git a/docs/SECRETS.md b/docs/SECRETS.md index f70d493c..6f26c548 100644 --- a/docs/SECRETS.md +++ b/docs/SECRETS.md @@ -197,9 +197,12 @@ Content-Type: application/json *Response (200 OK):* ```json { - "webhookId": "wh_9876543210abcdef", - "newSecret": "d9f8e7d6c5b4a392817263544536271809a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4", - "rotatedAt": "2026-07-24T00:30:00.000Z", - "previousSecretExpiresAt": "2026-07-25T00:30:00.000Z" + "success": true, + "data": { + "webhookId": "wh_9876543210abcdef", + "newSecret": "d9f8e7d6c5b4a392817263544536271809a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4", + "rotatedAt": "2026-07-24T00:30:00.000Z", + "previousSecretExpiresAt": "2026-07-25T00:30:00.000Z" + } } ``` diff --git a/src/app.ts b/src/app.ts index bc62e74e..6e4c92d0 100644 --- a/src/app.ts +++ b/src/app.ts @@ -61,6 +61,9 @@ import { sunsetHeaderMiddleware } from "./middleware/sunsetHeader.js"; import { createOutboxAdminRouter } from "./routes/admin/outbox.js"; import { structuredLoggingMiddleware } from "./middleware/structuredLogging.js"; import { createWebhookReplayRouter } from "./routes/webhookReplay.js"; +import { createWebhookRouter } from "./routes/webhooks.js"; +import { PostgresWebhookRepository } from "./db/repositories/webhookRepository.js"; +import { auditLogService } from "./services/audit/index.js"; import { createExportRouter } from "./routes/export/index.js"; const app = express(); @@ -257,6 +260,15 @@ app.use("/api/admin/outbox", createOutboxAdminRouter()); // Webhook DLQ replay — GET /api/webhooks/dlq, POST /api/webhooks/dlq/:id/replay app.use("/api/webhooks/dlq", createWebhookReplayRouter(pool)); +// Webhook signing-secret rotation — POST /api/webhooks/:webhookId/rotate-secret +// (audited, safe-rollout dual-secret grace period — see docs/SECRETS.md and +// docs/WEBHOOK_SIGNING.md). Mounted after /api/webhooks/dlq so it never +// intercepts that router's requests. +app.use( + "/api/webhooks", + createWebhookRouter(new PostgresWebhookRepository(pool), auditLogService), +); + app.use("/api/orgs/:orgId/policies", createPolicyRouter()); const analyticsThresholdSeconds = Number( diff --git a/src/db/repositories/webhookRepository.test.ts b/src/db/repositories/webhookRepository.test.ts new file mode 100644 index 00000000..45279bae --- /dev/null +++ b/src/db/repositories/webhookRepository.test.ts @@ -0,0 +1,107 @@ +import { describe, it, expect, beforeEach } from 'vitest' +import { newDb } from 'pg-mem' +import type { Pool } from 'pg' +import { PostgresWebhookRepository } from './webhookRepository.js' +import type { WebhookConfig } from '../../services/webhooks/types.js' + +/** + * Exercises PostgresWebhookRepository against a real (in-memory) Postgres so + * that schema drift between this repository's SQL and the migrations that + * actually create `webhook_configs` gets caught here instead of only at + * runtime against a real database. This is the schema exactly as it exists + * after migrations 005, 007, 009, and 032 have all run. + */ +async function buildTestDb(): Promise { + const db = newDb() + + db.public.registerFunction({ + name: 'gen_random_uuid', + returns: 'uuid', + implementation: () => crypto.randomUUID(), + } as Parameters[0]) + + const adapter = db.adapters.createPg() + const pool = new adapter.Pool() as unknown as Pool + + await pool.query(` + CREATE TABLE IF NOT EXISTS webhook_configs ( + id UUID PRIMARY KEY DEFAULT gen_random_uuid(), + url TEXT NOT NULL, + secret TEXT NOT NULL, + previous_secret VARCHAR(255), + previous_secret_expires_at TIMESTAMPTZ, + secret_updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + active BOOLEAN NOT NULL DEFAULT TRUE, + events TEXT[] NOT NULL, + timeout_ms INTEGER NOT NULL DEFAULT 5000, + max_attempts INTEGER NOT NULL DEFAULT 3, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() + ); + `) + + return pool +} + +const SEED: WebhookConfig = { + id: '', + url: 'https://example.com/hook', + events: ['bond.created'], + secret: 'initial-secret', + secretUpdatedAt: new Date(), + active: true, +} + +describe('PostgresWebhookRepository', () => { + let pool: Pool + let repo: PostgresWebhookRepository + let webhookId: string + + beforeEach(async () => { + pool = await buildTestDb() + repo = new PostgresWebhookRepository(pool) + webhookId = crypto.randomUUID() + await repo.set({ ...SEED, id: webhookId }) + }) + + it('rotateSecret() atomically swaps the secret and persists previousSecretExpiresAt', async () => { + const expiresAt = new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString() + + const updated = await repo.rotateSecret(webhookId, 'brand-new-secret', 'initial-secret', expiresAt) + + expect(updated.secret).toBe('brand-new-secret') + expect(updated.previousSecret).toBe('initial-secret') + expect(updated.previousSecretExpiresAt).toBeTruthy() + expect(new Date(updated.previousSecretExpiresAt!).toISOString()).toBe(new Date(expiresAt).toISOString()) + }) + + it('rotateSecret() throws when the webhook does not exist', async () => { + const nonExistentId = crypto.randomUUID() + await expect( + repo.rotateSecret(nonExistentId, 'new-secret', 'old-secret', new Date().toISOString()), + ).rejects.toThrow('Webhook not found') + }) + + it('get() reflects the rotated secret and expiry after rotation', async () => { + const expiresAt = new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString() + await repo.rotateSecret(webhookId, 'rotated-secret', 'initial-secret', expiresAt) + + const fetched = await repo.get(webhookId) + + expect(fetched).not.toBeNull() + expect(fetched!.secret).toBe('rotated-secret') + expect(fetched!.previousSecret).toBe('initial-secret') + expect(fetched!.previousSecretExpiresAt).toBeTruthy() + }) + + it('a second rotation replaces previousSecret rather than accumulating history', async () => { + const firstExpiry = new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString() + await repo.rotateSecret(webhookId, 'second-secret', 'initial-secret', firstExpiry) + + const secondExpiry = new Date(Date.now() + 48 * 60 * 60 * 1000).toISOString() + const updated = await repo.rotateSecret(webhookId, 'third-secret', 'second-secret', secondExpiry) + + expect(updated.secret).toBe('third-secret') + expect(updated.previousSecret).toBe('second-secret') + }) +}) diff --git a/src/migrations/032_add_webhook_previous_secret_expires_at.ts b/src/migrations/032_add_webhook_previous_secret_expires_at.ts new file mode 100644 index 00000000..682c5ce6 --- /dev/null +++ b/src/migrations/032_add_webhook_previous_secret_expires_at.ts @@ -0,0 +1,24 @@ +import { MigrationBuilder } from 'node-pg-migrate' + +/** + * Migration: add webhook_configs.previous_secret_expires_at + * + * `PostgresWebhookRepository.rotateSecret()` (src/db/repositories/webhookRepository.ts) + * and `WebhookRotationService` (src/services/webhooks/rotationService.ts) have relied + * on this column since the safe-rollout secret rotation endpoint was written, but no + * migration ever created it — rotation against a real Postgres-backed store would fail + * with "column previous_secret_expires_at does not exist". This column stores the + * explicit ISO timestamp (set at rotation time, 24h TTL) after which `previous_secret` + * should no longer be accepted for signature verification. + * + * Nullable, no default: existing rows have no previous secret to expire. + */ +export async function up(pgm: MigrationBuilder): Promise { + pgm.addColumns('webhook_configs', { + previous_secret_expires_at: { type: 'timestamptz', notNull: false }, + }) +} + +export async function down(pgm: MigrationBuilder): Promise { + pgm.dropColumns('webhook_configs', ['previous_secret_expires_at']) +} diff --git a/src/routes/webhooks.ts b/src/routes/webhooks.ts index 99ddff4c..2a8c2af9 100644 --- a/src/routes/webhooks.ts +++ b/src/routes/webhooks.ts @@ -78,6 +78,7 @@ export function createWebhookRouter(store: WebhookStore, audit: AuditLogService) webhookId, actor.id, actor.email, + actor.tenantId, ipAddress, ) @@ -87,11 +88,28 @@ export function createWebhookRouter(store: WebhookStore, audit: AuditLogService) }) } catch (err) { if (err instanceof WebhookNotFoundError) { - sendError(res, ErrorCode.NOT_FOUND, err.message) + res.status(404).json({ + error: 'NotFound', + code: 'WebhookNotFound', + message: err.message, + }) return } - sendError(res, ErrorCode.INTERNAL_SERVER_ERROR, err instanceof Error ? err.message : 'Unknown error') + const message = err instanceof Error ? err.message : 'Unknown error' + void audit.logAction({ + tenantId: (req as AuthenticatedRequest).user?.tenantId ?? tenantId, + actorId: (req as AuthenticatedRequest).user?.id ?? 'unknown', + actorEmail: (req as AuthenticatedRequest).user?.email ?? 'unknown', + action: AuditAction.ROTATE_WEBHOOK_SECRET, + resourceType: 'webhook', + resourceId: req.params.webhookId, + details: { webhookId: req.params.webhookId, reason: 'unexpected_error' }, + status: 'failure', + errorMessage: message, + ipAddress: req.ip ?? req.socket.remoteAddress, + }) + sendError(res, ErrorCode.INTERNAL_SERVER_ERROR, message) } finally { // Restore original tenant context if (!originalTenant) { diff --git a/src/services/webhooks/__tests__/rotation.test.ts b/src/services/webhooks/__tests__/rotation.test.ts index c9d697f5..906455db 100644 --- a/src/services/webhooks/__tests__/rotation.test.ts +++ b/src/services/webhooks/__tests__/rotation.test.ts @@ -11,15 +11,16 @@ async function buildTestDb(): Promise<{ db: IMemoryDb; pool: Pool }> { const db = newDb(); db.public.none(` CREATE TABLE webhook_configs ( - id UUID PRIMARY KEY, - url TEXT NOT NULL, - secret TEXT NOT NULL, - previous_secret TEXT, - secret_updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), - active BOOLEAN NOT NULL DEFAULT TRUE, - events TEXT[] NOT NULL, - created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), - updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() + id UUID PRIMARY KEY, + url TEXT NOT NULL, + secret TEXT NOT NULL, + previous_secret TEXT, + previous_secret_expires_at TIMESTAMPTZ, + secret_updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + active BOOLEAN NOT NULL DEFAULT TRUE, + events TEXT[] NOT NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() ); `); const adapter = db.adapters.createPg(); diff --git a/src/services/webhooks/rotationService.ts b/src/services/webhooks/rotationService.ts index b24e36cd..f73ff1e3 100644 --- a/src/services/webhooks/rotationService.ts +++ b/src/services/webhooks/rotationService.ts @@ -4,7 +4,7 @@ import type { AuditLogService } from '../audit/index.js' import { AuditAction } from '../audit/index.js' /** Grace period during which the previous secret remains valid for client verification. */ -const PREVIOUS_SECRET_TTL_MS = 24 * 60 * 60 * 1000 // 24 hours +export const PREVIOUS_SECRET_TTL_MS = 24 * 60 * 60 * 1000 // 24 hours export class WebhookNotFoundError extends Error { constructor(webhookId: string) { @@ -30,13 +30,14 @@ export class WebhookRotationService { webhookId: string, actorId: string, actorEmail: string, + tenantId: string, ipAddress?: string, ): Promise { const webhook = await this.store.get(webhookId) if (!webhook) { void this.audit.logAction({ - tenantId: 'tenant-unknown', + tenantId, actorId, actorEmail, action: AuditAction.ROTATE_WEBHOOK_SECRET, @@ -58,7 +59,7 @@ export class WebhookRotationService { await this.store.rotateSecret(webhookId, newSecret, webhook.secret, previousSecretExpiresAt) void this.audit.logAction({ - tenantId: 'tenant-unknown', + tenantId, actorId, actorEmail, action: AuditAction.ROTATE_WEBHOOK_SECRET, diff --git a/src/services/webhooks/service.test.ts b/src/services/webhooks/service.test.ts index 6ce2fc28..286e0c7a 100644 --- a/src/services/webhooks/service.test.ts +++ b/src/services/webhooks/service.test.ts @@ -290,6 +290,56 @@ describe('WebhookService', () => { expect(duration).toBeLessThan(200) }) + describe('rotateSecret', () => { + it('rotates via the atomic store.rotateSecret(), not read-modify-write via store.set()', async () => { + const service = new WebhookService(mockStore) + + const updated = await service.rotateSecret('wh_1') + + // The whole point of this method: a concurrent rotation of the same + // webhook elsewhere can't be silently clobbered by a stale + // read-then-set(), because there is no read-then-set at all. + expect(mockStore.rotateSecret).toHaveBeenCalledTimes(1) + expect(mockStore.set).not.toHaveBeenCalled() + expect(updated.secret).not.toBe('secret1') + expect(updated.previousSecret).toBe('secret1') + expect(updated.previousSecretExpiresAt).toBeTruthy() + }) + + it('computes previousSecretExpiresAt roughly 24h in the future', async () => { + const service = new WebhookService(mockStore) + const before = Date.now() + + const updated = await service.rotateSecret('wh_2') + + const expiresMs = new Date(updated.previousSecretExpiresAt!).getTime() + expect(expiresMs).toBeGreaterThan(before + 23 * 60 * 60 * 1000) + expect(expiresMs).toBeLessThan(before + 25 * 60 * 60 * 1000) + }) + + it('throws when the webhook does not exist', async () => { + const service = new WebhookService(mockStore) + await expect(service.rotateSecret('wh_nonexistent')).rejects.toThrow('Webhook not found') + }) + + it('audit-logs success with the actor tenant and the computed previousSecretExpiresAt', async () => { + const mockAudit = { logAction: vi.fn().mockResolvedValue(undefined) } + const actor = { id: 'admin_1', email: 'admin@example.com', tenantId: 'tenant_1' } + const service = new WebhookService(mockStore, undefined, undefined, mockAudit as any) + + await service.rotateSecret('wh_1', actor, 'req-123') + + expect(mockAudit.logAction).toHaveBeenCalledTimes(1) + const [tenantId, actorId, actorEmail, action, resourceId, , details] = mockAudit.logAction.mock.calls[0] + expect(tenantId).toBe('tenant_1') + expect(actorId).toBe('admin_1') + expect(actorEmail).toBe('admin@example.com') + expect(action).toBe('ROTATE_WEBHOOK_SECRET') + expect(resourceId).toBe('wh_1') + expect(details).toMatchObject({ previousSecretExpiresAt: expect.any(String) }) + }) + }) + describe('replayWebhook', () => { it('throws if DLQ is not configured', async () => { const service = new WebhookService(mockStore) diff --git a/src/services/webhooks/service.ts b/src/services/webhooks/service.ts index 5d22e52b..217535bb 100644 --- a/src/services/webhooks/service.ts +++ b/src/services/webhooks/service.ts @@ -2,6 +2,7 @@ import { randomBytes } from 'crypto' import type { WebhookStore, WebhookEventType, WebhookPayload, WebhookDeliveryResult, WebhookConfig, DlqStore, WebhookEmitOptions } from './types.js' import { deliverWebhook, type DeliveryOptions } from './delivery.js' import { type AuditLogService, AuditAction } from '../audit/index.js' +import { PREVIOUS_SECRET_TTL_MS } from './rotationService.js' import { buildDlqEntry } from './dlq.js' import { recordJobDeadLetter, @@ -25,7 +26,12 @@ export class WebhookService { /** * Rotate a webhook's signing secret. - * Moves current secret to previousSecret and generates a new one. + * + * Uses the store's atomic `rotateSecret()` (a single UPDATE ... RETURNING) + * rather than read-modify-write via `get()` + `set()`, so a concurrent + * rotation of the same webhook (e.g. via the other rotation entry point, + * POST /api/webhooks/:webhookId/rotate-secret) can't silently clobber the + * secret written by this call with a stale in-memory copy. */ async rotateSecret(id: string, admin?: { id: string, email: string, tenantId: string }, requestId?: string): Promise { const webhook = await this.store.get(id) @@ -33,12 +39,10 @@ export class WebhookService { throw new Error('Webhook not found') } - // Move current to previous and generate new - webhook.previousSecret = webhook.secret - webhook.secret = randomBytes(32).toString('hex') - webhook.secretUpdatedAt = new Date() + const newSecret = randomBytes(32).toString('hex') + const previousSecretExpiresAt = new Date(Date.now() + PREVIOUS_SECRET_TTL_MS).toISOString() - await this.store.set(webhook) + const updated = await this.store.rotateSecret(id, newSecret, webhook.secret, previousSecretExpiresAt) if (this.auditLog && admin) { this.auditLog.logAction( @@ -47,8 +51,8 @@ export class WebhookService { admin.email, AuditAction.ROTATE_WEBHOOK_SECRET, id, - webhook.url, - { rotatedAt: webhook.secretUpdatedAt }, + updated.url, + { rotatedAt: updated.secretUpdatedAt, previousSecretExpiresAt }, undefined, undefined, undefined, @@ -56,7 +60,7 @@ export class WebhookService { ) } - return webhook + return updated } /** diff --git a/tests/routes/webhooks.test.ts b/tests/routes/webhooks.test.ts index faf87f6f..f71d1966 100644 --- a/tests/routes/webhooks.test.ts +++ b/tests/routes/webhooks.test.ts @@ -203,6 +203,10 @@ describe('Webhook Routes', () => { expect(mockAuditLogs[0].action).toBe('ROTATE_WEBHOOK_SECRET') expect(mockAuditLogs[0].status).toBe('success') expect(mockAuditLogs[0].resourceId).toBe(SEED_WEBHOOK.id) + // Regression check: the audit entry must carry the authenticated + // actor's real tenant, not a hardcoded placeholder. + expect(mockAuditLogs[0].tenantId).toBe('tenant-admin-1') + expect(mockAuditLogs[0].tenantId).not.toBe('tenant-unknown') }) it('two consecutive rotations produce different secrets', async () => { @@ -248,6 +252,8 @@ describe('Webhook Routes', () => { expect(mockAuditLogs[0].action).toBe('ROTATE_WEBHOOK_SECRET') expect(mockAuditLogs[0].status).toBe('failure') expect(mockAuditLogs[0].resourceId).toBe('nonexistent-webhook') + expect(mockAuditLogs[0].tenantId).toBe('tenant-admin-1') + expect(mockAuditLogs[0].tenantId).not.toBe('tenant-unknown') }) it('returns 401 when no Authorization header is provided', async () => {