fix: rate-lock reuse gap, overlapping webhook retries, key-rotation docs

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

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);
 }));
 

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');
+  });
+});

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<RateLock> {
+export async function lockRate(userId: string, fromCurrency: string, toAsset: string, amount: number): Promise<RateLock> {
   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<ConversionRecord> {
+export async function confirmConversion(rateLockId: string, userId: string, stripePaymentIntentId: string): Promise<ConversionRecord> {
   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,

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');
+  });
+});

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<string, WebhookDeliveryLog>();
 const idempotencyIndex = new Map<string, string>();
 const deadLetterQueue: WebhookDeliveryLog[] = [];
 
-const worker: WorkerState = { running: false };
+const worker: WorkerState = { running: false, processing: false };
 
 function nowIso(): string {
   return new Date().toISOString();
@@ -254,14 +256,24 @@ 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);
 }
 
 export function stopWebhookWorker(): void {
   if (worker.timer) clearInterval(worker.timer);
   worker.timer = undefined;
   worker.running = false;
+  worker.processing = false;
 }
 
 export function listWebhookDeliveries(): WebhookDeliveryLog[] {