From dcd86f503af4631b52eb5f349b311de3740c8425 Mon Sep 17 00:00:00 2001 From: maikNeta Date: Fri, 16 Oct 2026 13:50:17 +0100 Subject: [PATCH] fix: close rate-lock reuse gap, guard overlapping webhook retries, document key rotation windows - fiat-crypto: bind a rate lock to the user who created it and consume it on first use, so the same lock can no longer be confirmed twice (duplicate conversions) or confirmed by a different user than the one who locked it. - webhooks: add an in-flight guard around the delivery worker's 1s ticker so a slow or backlogged pass can never overlap with the next tick and deliver the same webhook twice. - WEBHOOK_VERIFICATION.md: document the overlap/retention/replay-tolerance windows WebhookKeyRegistry actually enforces during key rotation, which were implemented but never written down. Closes #1278, Closes #1279, Closes #1280, Closes #1281. --- WEBHOOK_VERIFICATION.md | 31 ++++++++++ backend/src/routes/stripe.ts | 12 ++-- .../services/__tests__/fiat-crypto.test.ts | 30 ++++++++++ backend/src/services/fiat-crypto.ts | 18 +++++- backend/src/services/webhooks.test.ts | 58 +++++++++++++++++++ backend/src/services/webhooks.ts | 16 ++++- 6 files changed, 156 insertions(+), 9 deletions(-) create mode 100644 backend/src/services/__tests__/fiat-crypto.test.ts create mode 100644 backend/src/services/webhooks.test.ts diff --git a/WEBHOOK_VERIFICATION.md b/WEBHOOK_VERIFICATION.md index 64523a7e..3def73fd 100644 --- a/WEBHOOK_VERIFICATION.md +++ b/WEBHOOK_VERIFICATION.md @@ -20,6 +20,37 @@ Implement webhook signature verification for all incoming webhooks to prevent sp ## Complexity Estimate 200 points - Verification, secure storage. +## Key Rotation: Overlap, Retention, and Replay Tolerance + +`WebhookKeyRegistry` (`backend/src/services/webhookKeys.ts`) governs how long a +rotated secret keeps working and how it is retired. Get these windows wrong +and either genuine events get rejected mid-rotation, or revoked secrets keep +verifying signatures long after they should not. + +- **`overlapSeconds`** (default `DEFAULT_OVERLAP_SECONDS` = 72h) — passed to + `registry.rotate({ overlapSeconds })`. For this long after a secret is + rotated, the *previous* secret still verifies signatures alongside the new + one. Set this to at least as long as the slowest provider takes to pick up + a newly configured secret (e.g. Stripe can take a while to fully cut over + after a dashboard change) — too short, and in-flight webhooks signed with + the old secret start failing verification before the provider has switched. +- **`retentionSeconds`** (default `DEFAULT_RETENTION_SECONDS` = 7 days) — how + long an expired or revoked key is kept around (rather than purged) so that + verification attempts against it can still be logged and classified by + reason, instead of just vanishing. This is operational/audit retention, not + a security control — a key past its `overlapSeconds` window no longer + verifies anything regardless of retention. +- **`toleranceSeconds`** (default `DEFAULT_TOLERANCE_SECONDS` = 300) — the + replay-protection clock-skew window: a webhook's timestamp must be within + this many seconds of `registry` time, in either direction, or verification + rejects it as expired/replayed. Widen this only to accommodate real clock + drift between you and the provider, since every second added is a second a + captured signature + body could be replayed. + +All three are constructor options on `WebhookKeyRegistry` (`new +WebhookKeyRegistry({ overlapSeconds, retentionSeconds, toleranceSeconds })`) +and default to the constants above when omitted. + ## Implementation Plan 1. Create webhook verification service with HMAC-SHA256 2. Implement per-provider secret management diff --git a/backend/src/routes/stripe.ts b/backend/src/routes/stripe.ts index 5b54d415..ad48f6ca 100644 --- a/backend/src/routes/stripe.ts +++ b/backend/src/routes/stripe.ts @@ -296,17 +296,21 @@ stripeRouter.get('/rates', asyncHandler(async (req, res) => { res.json({ from, to, rate, updatedAt: new Date().toISOString() }); })); -stripeRouter.post('/convert/lock-rate', asyncHandler(async (req, res) => { +stripeRouter.post('/convert/lock-rate', asyncHandler(async (req: any, res) => { const { fromCurrency = 'USD', toAsset = 'USDC', amount } = req.body; if (!amount) return res.status(400).json({ error: 'amount required' }); - const lock = await lockRate(fromCurrency, toAsset, amount); + const userId = req.user?.id ?? req.headers['x-actor-id']?.toString(); + if (!userId) return res.status(401).json({ error: 'authentication required' }); + const lock = await lockRate(userId, fromCurrency, toAsset, amount); res.json(lock); })); -stripeRouter.post('/convert/confirm', asyncHandler(async (req, res) => { +stripeRouter.post('/convert/confirm', asyncHandler(async (req: any, res) => { const { rateLockId, stripePaymentIntentId } = req.body; if (!rateLockId || !stripePaymentIntentId) return res.status(400).json({ error: 'rateLockId and stripePaymentIntentId required' }); - const conversion = await confirmConversion(rateLockId, stripePaymentIntentId); + const userId = req.user?.id ?? req.headers['x-actor-id']?.toString(); + if (!userId) return res.status(401).json({ error: 'authentication required' }); + const conversion = await confirmConversion(rateLockId, userId, stripePaymentIntentId); res.json(conversion); })); diff --git a/backend/src/services/__tests__/fiat-crypto.test.ts b/backend/src/services/__tests__/fiat-crypto.test.ts new file mode 100644 index 00000000..a1d73589 --- /dev/null +++ b/backend/src/services/__tests__/fiat-crypto.test.ts @@ -0,0 +1,30 @@ +import { describe, it, expect } from 'vitest'; +import { lockRate, confirmConversion, getConversion } from '../fiat-crypto.js'; + +const USER_A = 'user_a'; +const USER_B = 'user_b'; + +describe('fiat-crypto conversions', () => { + it('ties a confirmed conversion to the user who locked the rate', async () => { + const lock = await lockRate(USER_A, 'USD', 'USDC', 100); + const conversion = await confirmConversion(lock.id, USER_A, 'pi_123'); + expect(conversion.userId).toBe(USER_A); + }); + + it('rejects confirming a rate lock that belongs to a different user', async () => { + const lock = await lockRate(USER_A, 'USD', 'USDC', 100); + await expect(confirmConversion(lock.id, USER_B, 'pi_123')).rejects.toThrow(); + }); + + it('rejects confirming the same rate lock twice, even for its own owner', async () => { + const lock = await lockRate(USER_A, 'USD', 'USDC', 100); + const first = await confirmConversion(lock.id, USER_A, 'pi_first'); + await expect(confirmConversion(lock.id, USER_A, 'pi_second')).rejects.toThrow( + 'Rate lock has already been used', + ); + + // Only the first confirmation produced a conversion. + const stored = await getConversion(first.id); + expect(stored?.stripePaymentIntentId).toBe('pi_first'); + }); +}); diff --git a/backend/src/services/fiat-crypto.ts b/backend/src/services/fiat-crypto.ts index c0b1d402..38346f34 100644 --- a/backend/src/services/fiat-crypto.ts +++ b/backend/src/services/fiat-crypto.ts @@ -6,12 +6,15 @@ export type ConversionStatus = 'pending' | 'rate_locked' | 'completed' | 'failed export interface RateLock { id: string; + userId: string; fromCurrency: string; toAsset: string; rate: number; amount: number; expiresAt: number; locked: boolean; + /** Set once this lock has been consumed by `confirmConversion`, so it cannot be reused. */ + consumedAt?: number; } export interface ConversionRecord { @@ -44,10 +47,11 @@ function getCurrentRate(fromCurrency: string, toAsset: string): number { return 1.0; } -export async function lockRate(fromCurrency: string, toAsset: string, amount: number): Promise { +export async function lockRate(userId: string, fromCurrency: string, toAsset: string, amount: number): Promise { const rate = getCurrentRate(fromCurrency, toAsset); const lock: RateLock = { id: randomUUID(), + userId, fromCurrency, toAsset, rate, @@ -59,15 +63,23 @@ export async function lockRate(fromCurrency: string, toAsset: string, amount: nu return lock; } -export async function confirmConversion(rateLockId: string, stripePaymentIntentId: string): Promise { +export async function confirmConversion(rateLockId: string, userId: string, stripePaymentIntentId: string): Promise { const lock = rateLocks.get(rateLockId); if (!lock) throw new Error('Rate lock not found'); + // A lock belongs to whoever created it, and can be spent exactly once -- otherwise + // one locked rate could be confirmed repeatedly (duplicate conversions from a single + // amount) or confirmed by a user who never created it. + if (lock.userId !== userId) throw new Error('Rate lock not found'); + if (lock.consumedAt !== undefined) throw new Error('Rate lock has already been used'); if (Date.now() > lock.expiresAt) throw new Error('Rate lock has expired'); + lock.consumedAt = Date.now(); + rateLocks.set(lock.id, lock); + const cryptoAmount = (lock.amount * lock.rate).toFixed(7); const conversion: ConversionRecord = { id: randomUUID(), - userId: 'user_default', + userId: lock.userId, fromCurrency: lock.fromCurrency, toAsset: lock.toAsset, fiatAmount: lock.amount, diff --git a/backend/src/services/webhooks.test.ts b/backend/src/services/webhooks.test.ts new file mode 100644 index 00000000..3edc8c5b --- /dev/null +++ b/backend/src/services/webhooks.test.ts @@ -0,0 +1,58 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { + upsertWebhookConfig, + enqueueWebhookEvent, + startWebhookWorker, + stopWebhookWorker, + getWebhookDelivery, +} from './webhooks.js'; + +describe('webhook worker', () => { + afterEach(() => { + stopWebhookWorker(); + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + it('never runs two overlapping passes while a delivery is still in flight', async () => { + vi.useFakeTimers(); + + let resolveFetch: (() => void) | undefined; + const fetchMock = vi.fn( + () => + new Promise((resolve) => { + resolveFetch = () => resolve({ ok: true, status: 200, text: async () => '{}' } as Response); + }), + ); + vi.stubGlobal('fetch', fetchMock); + + const config = upsertWebhookConfig({ + merchantId: 'merchant_1', + url: 'https://merchant.example/webhook', + secret: 'a_very_secret_value', + }); + const { delivery } = enqueueWebhookEvent({ + merchantId: config.merchantId, + type: 'payment.completed', + payload: { amount: 100 }, + }); + expect(delivery).toBeDefined(); + + startWebhookWorker(); + + // First tick: picks up the delivery and starts the (slow, unresolved) fetch. + await vi.advanceTimersByTimeAsync(1_000); + expect(fetchMock).toHaveBeenCalledTimes(1); + + // Several more ticks elapse while the first attempt is still in flight. + // Without the in-flight guard, each tick would start another pass and + // call fetch again for the same still-"processing" delivery. + await vi.advanceTimersByTimeAsync(5_000); + expect(fetchMock).toHaveBeenCalledTimes(1); + + resolveFetch?.(); + await vi.advanceTimersByTimeAsync(0); + + expect(getWebhookDelivery(delivery!.id)?.status).toBe('delivered'); + }); +}); diff --git a/backend/src/services/webhooks.ts b/backend/src/services/webhooks.ts index b283996f..fd9b697c 100644 --- a/backend/src/services/webhooks.ts +++ b/backend/src/services/webhooks.ts @@ -48,6 +48,8 @@ export interface WebhookDeliveryLog { type WorkerState = { timer?: NodeJS.Timeout; running: boolean; + /** True while a pass over due deliveries is in flight, so the 1s ticker never overlaps itself. */ + processing: boolean; }; const MAX_ATTEMPTS = 5; @@ -60,7 +62,7 @@ const deliveries = new Map(); const idempotencyIndex = new Map(); const deadLetterQueue: WebhookDeliveryLog[] = []; -const worker: WorkerState = { running: false }; +const worker: WorkerState = { running: false, processing: false }; function nowIso(): string { return new Date().toISOString(); @@ -254,7 +256,16 @@ export function startWebhookWorker(): void { if (worker.running) return; worker.running = true; worker.timer = setInterval(() => { - void processDueDeliveries(); + // A pass that is still in flight (e.g. a slow endpoint, or a backlog bigger + // than one tick can clear) must finish before another one starts. Without + // this guard, the next 1s tick starts a second, overlapping pass that can + // pick up and deliver the same still-"pending"/"retrying" delivery a + // second time before the first pass has marked it "processing". + if (worker.processing) return; + worker.processing = true; + void processDueDeliveries().finally(() => { + worker.processing = false; + }); }, 1_000); } @@ -262,6 +273,7 @@ export function stopWebhookWorker(): void { if (worker.timer) clearInterval(worker.timer); worker.timer = undefined; worker.running = false; + worker.processing = false; } export function listWebhookDeliveries(): WebhookDeliveryLog[] {