feat: payout execution, money correctness and payout API security (#1183 #1184 #1185 #1186)

PR_DESCRIPTION.md

@@ -1,44 +1,114 @@
-# Docs, payment correctness, payout reliability, and withdrawal security (#1258–#1261)
+# Payout reliability, payment correctness and payment security (iterations 10-11)
 
-Closes #1258
-Closes #1259
-Closes #1260
-Closes #1261
+Closes #1183, #1184, #1185, #1186.
+
+These four tickets are templated "deliver the next increment" issues with no fixed scope.
+Following the precedent of #1286 and #1287, this PR takes one concrete, real problem per
+theme instead of a broad sweep, and keeps each change independently reviewable.
 
 ## Scope
 
-This PR delivers one focused increment for each issue, in issue order:
+### #1185 — Improve payout reliability
+
+`PayoutEngine` had **no execution path**. Auto-approved and manually approved batches were
+stamped `completed` without anything being sent, `executing` was unreachable dead state,
+there was no failure or retry path, and calendar schedules fired on every evaluation pass.
 
-### #1258 — Documentation discovery
+- `pending -> executing -> completed | failed` is now a guarded transition table.
+  `completed` is only reached when the rail returns a transaction hash.
+- An injected `executor` actually settles the batch and receives a stable
+  `idempotencyKey` on every attempt.
+- Failures schedule an exponential backoff with jitter, capped by `maxAttempts`.
+- `daily` / `weekly` / `monthly` respect their period and key their batches by period
+  bucket, so a replayed pass cannot pay the same period twice.
+- `thresholdAmount` of `0` no longer falls through to the "pay anything" branch.
+- `payout.created` / `pending_review` / `executing` / `completed` / `failed` / `delayed`
+  events via a pluggable sink. A throwing subscriber cannot abort a payout decision.
+- Reservations are released on rejection, and on exhaustion only after an approver
+  explicitly calls `releaseReservedFunds` — returning them automatically would mint a
+  fresh batch and spin against a rail that is already down.
+- One merchant's misconfiguration is isolated from the rest of the evaluation pass.
+- New REST surface at `/api/v1/payouts`, mounted in `backend/src/index.ts`.
 
-- Adds `docs/README.md` as a curated entry point for contributor, payment, SDK, operations, and security guides.
-- Links the root README to the index.
-- Adds a dependency-free Node test that verifies every local link in the index exists, and runs it from the root `test` command.
+### #1184 — Fix payment correctness bugs
 
-### #1259 — Payment amount correctness
+- **Unsupported FX pairs were priced at `1.0`.** `getCurrentRate` returned par for any
+  pair it did not know, so `GBP -> BTC` would quote one Bitcoin per pound and
+  `confirmConversion` would record it. Unquoted pairs are now an error.
+- **Money was derived with binary floating point.** `33.33 * 1.08` is
+  `35.99640000000001`, so the crypto amount stopped agreeing with the fiat amount charged.
+  Added `backend/src/services/money.ts` (bigint minor units, half-away-from-zero
+  rounding) and routed conversions through it.
+- **`lockRate` accepted `NaN`, negative and infinite amounts**, producing locks whose
+  conversion amounts were `"NaN"`.
+- **`processFiatRefund` recorded `'failed'`** — indistinguishable from a real failure —
+  returned `true` without refunding anything when no payment intent was present, and was
+  not idempotent, so a retry issued a second Stripe refund.
+- **Revenue pool recipients were validated individually but not in aggregate**, so a pool
+  could be configured to distribute more than 100% of what it received.
+- **`recordDistribution` had no idempotency on `txHash`**, so a reorg or retried indexer
+  double-counted revenue.
+- **`getConversion` / `listConversions` returned live internal references**, letting a
+  caller mutate a financial record.
 
-- Rejects non-finite, zero, and negative amounts at the shared payment-provider router boundary.
-- Returns `INVALID_PAYMENT_AMOUNT` with status `400` before invoking or recording metrics for any provider.
-- Documents amount validation and provider selection/fallback behavior.
+### #1186 — Strengthen payment security
 
-### #1260 — Payout state reliability
+- Every `/api/v1/payouts` route, **including reads**, requires an authenticated approver
+  principal. The check runs before any lookup, so an unauthenticated caller cannot probe
+  batch existence from the status code.
+- `Idempotency-Key` on every money-moving route. A key reused with a **different** body
+  is rejected with `409 idempotency_key_reuse_mismatch` instead of silently replaying the
+  earlier response — previously a caller could believe it created a new resource while
+  holding the old one.
+- Strict allowlists (asset, schedule cadence) and bounded amounts and identifiers.
+- The asset allowlist and amount ceiling are enforced in the **engine** as well as the
+  route, so a caller reaching the engine directly is not less protected.
+- Route-scoped rate limits: 120/min for the router, 30/min for approval decisions.
+- Engine errors are mapped to sanitised HTTP responses; rail text never reaches clients.
 
-- Rejects negative and non-finite pending payout balances.
-- Returns defensive copies from payout evaluation, history, and audit APIs, including cloned `Date` values.
-- Prevents callers from corrupting authoritative payout status, amount, or audit state by mutating returned objects.
-- Updates the payout approval guide with the new state-integrity guarantees.
+### #1183 — Improve documentation
 
-### #1261 — Withdrawal decision security
+- `docs/PAYOUT_RELIABILITY.md` — lifecycle, retry policy, reservations, events, API, ops runbook.
+- `docs/PAYMENT_CORRECTNESS.md` — the money, FX, idempotency and allocation invariants.
+- `docs/security/payment-security.md` — authentication, authorization, allowlists, rate limits, audit.
+- `docs/README.md` index entries, and `PAYOUT_APPROVALS.md` cross-links.
 
-- Applies the configured approver allowlist to rejection as well as approval.
-- Fails closed when a non-allowlisted withdrawal has no configured approvers.
-- Documents approver configuration, the API identity trust boundary, and velocity controls.
+## Files
+
+| File | Change |
+| ---- | ------ |
+| `backend/src/services/money.ts` | new — decimal-safe money arithmetic |
+| `backend/src/services/payouts/payout-engine.ts` | execution, retry, cadence, events, limits |
+| `backend/src/services/fiat-crypto.ts` | pair validation, amount validation, exact conversion, refund semantics |
+| `backend/src/services/revenue-pool/index.ts` | allocation ceiling, distribution idempotency |
+| `backend/src/middleware/idempotency.ts` | payload fingerprinting, bounded cache |
+| `backend/src/routes/payouts.ts` | new — `/api/v1/payouts` |
+| `backend/src/routes/stripe.ts` | unsupported-pair and settlement guards |
+| `backend/src/index.ts` | mount the payouts router |
+| `backend/src/**/__tests__/*` | new and updated tests |
+| `docs/PAYOUT_RELIABILITY.md`, `docs/PAYMENT_CORRECTNESS.md`, `docs/security/payment-security.md` | new |
 
 ## Tests
 
-- `npm run test:docs`
-- `npm --workspace backend test -- src/services/payments/__tests__/payment-router.test.ts src/services/payouts/__tests__/payout-engine.test.ts src/services/__tests__/issues-456-459.test.ts src/services/__tests__/withdrawal-allowlist.test.ts`
-- `npm run lint`
-- `npm run build`
+- `services/__tests__/money.test.ts` (new) — parsing, rounding, exact rate application.
+- `services/payouts/__tests__/payout-engine.test.ts` — execution, backoff growth,
+  attempt exhaustion, reservation release, cadence, cycle keys, event emission,
+  per-merchant isolation, concurrency collapse.
+- `services/__tests__/fiat-crypto.test.ts` — unquoted pairs, amount validation, exact
+  conversion, refund state and idempotency, deterministic settlement references, snapshot
+  isolation.
+- `services/revenue-pool/__tests__/revenue-pool.test.ts` (new) — allocation ceiling on
+  add and update, distribution replay.
+- `middleware/__tests__/idempotency.test.ts` — fingerprint mismatch, key ordering,
+  nested payloads, cache eviction.
+- `routes/__tests__/payouts.test.ts` (new) — auth on every route, validation, idempotency
+  replay and mismatch, approve/reject lifecycle, rate limiting.
+
+## Verification
 
-New regression coverage includes invalid payment amounts, invalid payout balances, mutation isolation for payout/audit snapshots, missing withdrawal approvers, unauthorized withdrawal rejection, and documentation link integrity.
+`npm run test --workspace backend`, `npm run lint --workspace backend` and
+`npm run test:docs` are the relevant commands. **Note:** this change set was authored in an
+environment without a Node runtime, so the suites were not executed locally — CI is the
+first run. The changes were reviewed by hand against the existing suites, and the two
+pre-existing payout tests that assert the old "no execution" behaviour were updated to the
+new contract.

backend/src/index.ts

@@ -73,6 +73,7 @@ import { searchRouter } from './routes/search.js';
 import { zapierRouter } from './routes/zapier.js';
 import { intercomRouter } from './routes/intercom.js';
 import { allowancesRouter } from './routes/allowances.js';
+import { payoutsRouter } from './routes/payouts.js';
 import { getPrismaReplicaClient } from './db/PrismaReplicaClient.js';
 import { cohortAnalyticsRouter } from './routes/cohort-analytics.js';
 import { churnPredictionRouter } from './routes/churn-prediction.js';
@@ -288,6 +289,7 @@ apiV1Router.use('/queue/payments', paymentQueueRouter);
 apiV1Router.use('/splits', splitsRouter);
 apiV1Router.use('/refunds', refundsRouter);
 apiV1Router.use('/allowances', allowancesRouter);
+apiV1Router.use('/payouts', payoutsRouter);
 apiV1Router.use('/forms', formsRouter);
 // Webhook management and verification
 apiV1Router.use('/webhooks', webhooksRouter);

backend/src/middleware/__tests__/idempotency.test.ts

@@ -1,6 +1,6 @@
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 import type { Request, Response } from 'express';
-import { idempotency, clearIdempotencyCache } from '../idempotency.js';
+import { idempotency, clearIdempotencyCache, getIdempotencyCacheStats } from '../idempotency.js';
 
 function makeReq(overrides: Partial<Request> = {}): Request {
   return {
@@ -152,3 +152,89 @@ describe('idempotency middleware', () => {
     expect(next2).toHaveBeenCalledOnce();
   });
 });
+
+describe('idempotency middleware payload fingerprinting', () => {
+  beforeEach(() => {
+    clearIdempotencyCache();
+  });
+
+  /** Runs one request through the middleware and returns the mocked response plus `next`. */
+  function send(key: string, body: unknown) {
+    const req = makeReq({ method: 'POST', headers: { 'idempotency-key': key }, body });
+    const { res } = makeRes();
+    const next = vi.fn();
+    idempotency()(req, res, next);
+    return { res, next };
+  }
+
+  function respond(res: Response, statusCode: number, body: unknown) {
+    res.statusCode = statusCode;
+    (res.json as ReturnType<typeof vi.fn>)(body);
+  }
+
+  it('rejects reuse of a key with a different payload', () => {
+    respond(send('key-x', { amount: 100 }).res, 201, { id: 'payment_1' });
+
+    const second = send('key-x', { amount: 999 });
+
+    expect(second.res.status).toHaveBeenCalledWith(409);
+    expect(second.res.json).toHaveBeenCalledWith(
+      expect.objectContaining({ error: 'idempotency_key_reuse_mismatch' }),
+    );
+    expect(second.next).not.toHaveBeenCalled();
+  });
+
+  it('replays the cached response when the payload matches', () => {
+    respond(send('key-y', { amount: 100 }).res, 201, { id: 'payment_1' });
+
+    const second = send('key-y', { amount: 100 });
+
+    expect(second.res.status).toHaveBeenCalledWith(201);
+    expect(second.res.json).toHaveBeenCalledWith({ id: 'payment_1' });
+    expect(second.next).not.toHaveBeenCalled();
+  });
+
+  it('treats a differently ordered payload as the same request', () => {
+    respond(send('key-z', { amount: 100, currency: 'USD' }).res, 200, { id: 'payment_1' });
+
+    const second = send('key-z', { currency: 'USD', amount: 100 });
+
+    expect(second.res.status).toHaveBeenCalledWith(200);
+    expect(second.next).not.toHaveBeenCalled();
+  });
+
+  it('detects a changed nested field', () => {
+    respond(send('key-n', { meta: { tags: ['a'] } }).res, 200, { id: 'payment_1' });
+
+    const second = send('key-n', { meta: { tags: ['b'] } });
+
+    expect(second.res.status).toHaveBeenCalledWith(409);
+  });
+});
+
+describe('idempotency middleware cache bounds', () => {
+  beforeEach(() => {
+    clearIdempotencyCache();
+  });
+
+  it('evicts the oldest entries once the limit is reached', () => {
+    const mw = idempotency(60_000, 2);
+
+    for (const key of ['a', 'b', 'c']) {
+      const req = makeReq({ headers: { 'idempotency-key': key } });
+      const { res } = makeRes();
+      mw(req, res, vi.fn());
+      res.statusCode = 200;
+      (res.json as ReturnType<typeof vi.fn>)({ key });
+    }
+
+    expect(getIdempotencyCacheStats().cachedKeys).toBe(2);
+
+    // The oldest entry was evicted, so it executes again instead of replaying.
+    const req = makeReq({ headers: { 'idempotency-key': 'a' } });
+    const { res } = makeRes();
+    const next = vi.fn();
+    mw(req, res, next);
+    expect(next).toHaveBeenCalledOnce();
+  });
+});

backend/src/middleware/idempotency.ts

@@ -1,10 +1,13 @@
+import { createHash } from 'node:crypto';
 import { Request, Response, NextFunction } from 'express';
 
 interface CacheEntry {
   response: any;
   statusCode: number;
   expiresAt: number;
   completedAt: number;
+  /** SHA-256 of the request that produced this entry; detects key reuse with a different payload. */
+  fingerprint: string;
 }
 
 /** In-flight request tracker — prevents two concurrent calls with same key racing each other. */
@@ -16,6 +19,9 @@ const idempotencyCache = new Map<string, CacheEntry>();
 const DEFAULT_TTL = 24 * 60 * 60 * 1000; // 24 hours in ms
 const MAX_KEY_LENGTH = 255;
 
+/** Hard cap on cached responses so a flood of unique keys cannot exhaust memory. */
+export const DEFAULT_MAX_CACHE_ENTRIES = 10_000;
+
 /** Accepts both the standard `Idempotency-Key` and legacy `X-Idempotency-Key` headers. */
 function extractIdempotencyKey(req: Request): string | undefined {
   return (
@@ -31,7 +37,41 @@ function validateKey(key: string): string | null {
   return null;
 }
 
-export const idempotency = (ttl: number = DEFAULT_TTL) => {
+/**
+ * Serialises a value with object keys sorted, so `{a:1,b:2}` and `{b:2,a:1}`
+ * produce the same string. Without this, a client that re-serialises its payload
+ * with different key ordering would look like a different request.
+ */
+function stableStringify(value: unknown): string {
+  if (value === undefined) return 'undefined';
+  if (value === null || typeof value !== 'object') return JSON.stringify(value) ?? 'null';
+  if (Array.isArray(value)) return `[${value.map(stableStringify).join(',')}]`;
+
+  const entries = Object.entries(value as Record<string, unknown>)
+    .filter(([, entryValue]) => entryValue !== undefined)
+    .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0));
+
+  return `{${entries.map(([key, entryValue]) => `${JSON.stringify(key)}:${stableStringify(entryValue)}`).join(',')}}`;
+}
+
+/**
+ * Fingerprints the parts of a request that must not change when an idempotency key
+ * is reused. The route is already part of the cache key, so only the body matters.
+ */
+export function fingerprintRequest(req: Pick<Request, 'body'>): string {
+  return createHash('sha256').update(stableStringify(req.body)).digest('hex');
+}
+
+/** Evicts the oldest entries once the cache exceeds `maxEntries`. */
+function enforceCacheLimit(maxEntries: number): void {
+  while (idempotencyCache.size > maxEntries) {
+    const oldest = idempotencyCache.keys().next();
+    if (oldest.done) return;
+    idempotencyCache.delete(oldest.value);
+  }
+}
+
+export const idempotency = (ttl: number = DEFAULT_TTL, maxEntries: number = DEFAULT_MAX_CACHE_ENTRIES) => {
   return (req: Request, res: Response, next: NextFunction) => {
     const key = extractIdempotencyKey(req);
 
@@ -46,11 +86,21 @@ export const idempotency = (ttl: number = DEFAULT_TTL) => {
 
     // Scope the cache key to method + route so the same UUID can't be reused across endpoints
     const cacheKey = `${req.method}:${req.originalUrl}:${key}`;
+    const fingerprint = fingerprintRequest(req);
 
     // --- Check completed-response cache ---
     const cached = idempotencyCache.get(cacheKey);
     if (cached) {
       if (Date.now() < cached.expiresAt) {
+        // Reusing a key with a different payload would silently replay the earlier
+        // response — the caller believes it created a new resource but got the old one.
+        if (cached.fingerprint !== fingerprint) {
+          return res.status(409).json({
+            error: 'idempotency_key_reuse_mismatch',
+            message:
+              'This Idempotency-Key was already used with a different request body. Use a new key for a different request.',
+          });
+        }
         return res.status(cached.statusCode).json(cached.response);
       }
       idempotencyCache.delete(cacheKey);
@@ -79,7 +129,9 @@ export const idempotency = (ttl: number = DEFAULT_TTL) => {
           statusCode: res.statusCode,
           expiresAt: Date.now() + ttl,
           completedAt: Date.now(),
+          fingerprint,
         });
+        enforceCacheLimit(maxEntries);
       }
 
       return originalJson(body);
@@ -122,4 +174,4 @@ export function getIdempotencyCacheStats() {
 export const clearIdempotencyCache = () => {
   idempotencyCache.clear();
   inFlight.clear();
-};
+};