diff --git a/WEBHOOK_VERIFICATION.md b/WEBHOOK_VERIFICATION.md index 3def73fd..5e436c0e 100644 --- a/WEBHOOK_VERIFICATION.md +++ b/WEBHOOK_VERIFICATION.md @@ -61,4 +61,9 @@ and default to the constants above when omitted. 7. Create secret rotation mechanism 8. Add comprehensive verification logging 9. Update webhook routes to use verification -10. Create admin interface for webhook management \ No newline at end of file +10. Create admin interface for webhook management + +## Replay window and signature comparison + +- A timestamp that does not parse as a date is rejected with `Timestamp is not a valid date`. An invalid date yields `NaN` in the tolerance check, and `NaN > tolerance` is false, so it previously passed the replay window. +- Digests are compared with `crypto.timingSafeEqual` over the decoded bytes. A non-hex or wrong-length signature fails closed without throwing. diff --git a/backend/src/index.ts b/backend/src/index.ts index e6f87f90..9e6e8efd 100644 --- a/backend/src/index.ts +++ b/backend/src/index.ts @@ -74,6 +74,7 @@ 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 { startPayoutRetryWorker } from './services/payouts/retry-worker.js'; import { getPrismaReplicaClient } from './db/PrismaReplicaClient.js'; import { cohortAnalyticsRouter } from './routes/cohort-analytics.js'; import { churnPredictionRouter } from './routes/churn-prediction.js'; @@ -508,6 +509,9 @@ server.listen(config.server.port, () => { // Load Sentry profiling integration now that the process is warm loadProfilingIntegration(); + // Payout retry worker: drives backed-off payout retries without a manual call + startPayoutRetryWorker(); + // Job scheduler if (config.jobs.enabled) { startJobs(); diff --git a/backend/src/services/batch.ts b/backend/src/services/batch.ts index 59b383d3..7fd2ddf9 100644 --- a/backend/src/services/batch.ts +++ b/backend/src/services/batch.ts @@ -3,6 +3,7 @@ import * as StellarSdk from '@stellar/stellar-sdk'; import { config } from '../config/env.js'; import { featureFlags } from '../config/featureFlags.js'; import { server, getNonceManager, getGasEstimator, UnitOfWorkError } from './stellar.js'; +import { CRYPTO_DECIMALS, sumAmounts } from './money.js'; const NETWORK = config().STELLAR_NETWORK; const networkPassphrase = @@ -151,17 +152,8 @@ export function getBatchReport(id: string): object | undefined { if (!record) return undefined; const successRate = record.total > 0 ? ((record.succeeded / record.total) * 100).toFixed(2) : '0.00'; - const totalAmount = record.results - .filter((r) => r.status === 'success') - .reduce((sum, r) => sum + parseFloat(r.amount), 0) - .toFixed(7); - - const byAsset = record.results - .filter((r) => r.status === 'success') - .reduce>((acc, r) => { - acc[r.asset] = (acc[r.asset] ?? 0) + parseFloat(r.amount); - return acc; - }, {}); + const byAsset = sumByAsset(record.results.filter((r) => r.status === 'success')); + const totalAmount = singleAssetTotal(byAsset); return { batchId: record.id, label: record.label, status: record.status, @@ -186,7 +178,8 @@ export function generateCSVTemplate(): string { export interface BatchEstimate { totalPayments: number; - totalAmount: string; + /** Null when the batch spans more than one asset: incomparable assets are never summed. */ + totalAmount: string | null; byAsset: Record; estimatedGasUnits: number; duplicateCount: number; @@ -194,31 +187,44 @@ export interface BatchEstimate { estimatedDurationMs: number; } +/** Sums amounts per asset in exact decimal arithmetic. */ +function sumByAsset(items: readonly Pick[]): Record { + const grouped: Record = {}; + for (const item of items) { + (grouped[item.asset] ??= []).push(item.amount); + } + const totals: Record = {}; + for (const [asset, amounts] of Object.entries(grouped)) { + totals[asset] = sumAmounts(amounts, CRYPTO_DECIMALS); + } + return totals; +} + +/** Returns the total only when there is exactly one asset to total. */ +function singleAssetTotal(byAsset: Record): string | null { + const assets = Object.keys(byAsset); + return assets.length === 1 ? byAsset[assets[0]] : null; +} + export function estimateBatch(payments: BatchPaymentItem[]): BatchEstimate { - const byAsset: Record = {}; - let totalAmount = 0; let invalidCount = 0; + const valid: BatchPaymentItem[] = []; for (const p of payments) { if (!/^G[A-Z2-7]{55}$/.test(p.recipient)) { invalidCount++; continue; } - const amount = parseFloat(p.amount) || 0; - totalAmount += amount; - byAsset[p.asset] = (byAsset[p.asset] ?? 0) + amount; + valid.push(p); } + const byAsset = sumByAsset(valid); const duplicateIndices = detectDuplicates(payments); - const byAssetStrings: Record = {}; - for (const [k, v] of Object.entries(byAsset)) { - byAssetStrings[k] = v.toFixed(7); - } return { totalPayments: payments.length, - totalAmount: totalAmount.toFixed(7), - byAsset: byAssetStrings, + totalAmount: singleAssetTotal(byAsset), + byAsset, estimatedGasUnits: payments.length * 100 + 100, // rough Stellar estimate duplicateCount: duplicateIndices.length, invalidAddressCount: invalidCount, diff --git a/backend/src/services/payouts/__tests__/retry-worker.test.ts b/backend/src/services/payouts/__tests__/retry-worker.test.ts new file mode 100644 index 00000000..5a983aa4 --- /dev/null +++ b/backend/src/services/payouts/__tests__/retry-worker.test.ts @@ -0,0 +1,65 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { startPayoutRetryWorker } from '../retry-worker.js'; + +describe('startPayoutRetryWorker', () => { + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + it('runs a retry pass on every interval', async () => { + vi.useFakeTimers(); + const retryDueBatches = vi.fn(async () => [{ id: 'b1' }]); + const worker = startPayoutRetryWorker({ retryDueBatches } as never, 1_000); + + await vi.advanceTimersByTimeAsync(3_000); + + expect(retryDueBatches).toHaveBeenCalledTimes(3); + worker.stop(); + }); + + it('stops scheduling passes once stopped', async () => { + vi.useFakeTimers(); + const retryDueBatches = vi.fn(async () => []); + const worker = startPayoutRetryWorker({ retryDueBatches } as never, 1_000); + + worker.stop(); + await vi.advanceTimersByTimeAsync(5_000); + + expect(retryDueBatches).not.toHaveBeenCalled(); + }); + + it('skips an interval while the previous pass is still running', async () => { + vi.useFakeTimers(); + let release!: () => void; + const retryDueBatches = vi.fn( + () => new Promise((resolve) => { + release = () => resolve([]); + }), + ); + const worker = startPayoutRetryWorker({ retryDueBatches } as never, 1_000); + + await vi.advanceTimersByTimeAsync(3_000); + expect(retryDueBatches).toHaveBeenCalledTimes(1); + + release(); + await vi.advanceTimersByTimeAsync(1_000); + expect(retryDueBatches).toHaveBeenCalledTimes(2); + worker.stop(); + }); + + it('reports a failed pass and keeps the worker alive for the next one', async () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const retryDueBatches = vi + .fn<() => Promise<{ id: string }[]>>() + .mockRejectedValueOnce(new Error('db down')) + .mockResolvedValueOnce([{ id: 'b1' }, { id: 'b2' }]); + const worker = startPayoutRetryWorker({ retryDueBatches } as never, 60_000); + + await expect(worker.tick()).resolves.toBe(0); + await expect(worker.tick()).resolves.toBe(2); + + expect(errorSpy).toHaveBeenCalledTimes(1); + worker.stop(); + }); +}); diff --git a/backend/src/services/payouts/retry-worker.ts b/backend/src/services/payouts/retry-worker.ts new file mode 100644 index 00000000..a65efeac --- /dev/null +++ b/backend/src/services/payouts/retry-worker.ts @@ -0,0 +1,49 @@ +import { payoutEngine, type PayoutEngine } from './payout-engine.js'; + +/** How often failed payout batches are checked for a due retry. */ +export const DEFAULT_PAYOUT_RETRY_INTERVAL_MS = 60_000; + +export interface PayoutRetryWorker { + /** Runs one retry pass now. Resolves to the number of batches retried; never rejects. */ + tick(): Promise; + stop(): void; +} + +/** + * Drives `retryDueBatches` on an interval. Without this, a failed batch's backoff + * timestamp is recorded but nothing acts on it until an operator calls the retries route. + * + * Passes never overlap: if a pass is still running when the next interval fires, that + * tick is skipped. A rejected pass is reported and retried on the next interval. + */ +export function startPayoutRetryWorker( + engine: Pick = payoutEngine, + intervalMs: number = DEFAULT_PAYOUT_RETRY_INTERVAL_MS, +): PayoutRetryWorker { + let running = false; + + const tick = async (): Promise => { + if (running) return 0; + running = true; + try { + const retried = await engine.retryDueBatches(); + return retried.length; + } catch (error) { + console.error('[payouts] Retry pass failed', error); + return 0; + } finally { + running = false; + } + }; + + const timer = setInterval(() => { + void tick(); + }, intervalMs); + // Do not keep the process alive solely for payout retries. + timer.unref(); + + return { + tick, + stop: () => clearInterval(timer), + }; +} diff --git a/backend/src/services/webhooks/__tests__/verification.test.ts b/backend/src/services/webhooks/__tests__/verification.test.ts index 09882d0e..7d7bf847 100644 --- a/backend/src/services/webhooks/__tests__/verification.test.ts +++ b/backend/src/services/webhooks/__tests__/verification.test.ts @@ -140,6 +140,47 @@ describe('Webhook Signature Verification', () => { }); }); + describe('Replay and signature hardening', () => { + it('rejects an unparseable timestamp instead of passing the tolerance window', () => { + createWebhookSecret(testProvider, testSecret); + const result = verifyWebhookSignature({ + signature: generateWebhookSignature(testPayload, testSecret, 'not-a-date'), + timestamp: 'not-a-date', + body: testPayload, + provider: testProvider, + }); + + expect(result.isValid).toBe(false); + expect(result.error).toBe('Timestamp is not a valid date'); + }); + + it('rejects a non-hex signature without throwing', () => { + createWebhookSecret(testProvider, testSecret); + const result = verifyWebhookSignature({ + signature: 'zz'.repeat(32), + timestamp: testTimestamp, + body: testPayload, + provider: testProvider, + }); + + expect(result.isValid).toBe(false); + }); + + it('rejects a signature that differs from the expected digest in one byte', () => { + createWebhookSecret(testProvider, testSecret); + const good = generateWebhookSignature(testPayload, testSecret, testTimestamp); + const flipped = (good[0] === '0' ? '1' : '0') + good.slice(1); + const result = verifyWebhookSignature({ + signature: flipped, + timestamp: testTimestamp, + body: testPayload, + provider: testProvider, + }); + + expect(result.isValid).toBe(false); + }); + }); + describe('Webhook Secret Management', () => { it('should create and retrieve webhook secrets', () => { const secret = createWebhookSecret(testProvider, testSecret); diff --git a/backend/src/services/webhooks/verification.ts b/backend/src/services/webhooks/verification.ts index 96f26446..d8887f51 100644 --- a/backend/src/services/webhooks/verification.ts +++ b/backend/src/services/webhooks/verification.ts @@ -1,4 +1,4 @@ -import { createHmac } from 'node:crypto'; +import { createHmac, timingSafeEqual } from 'node:crypto'; import { z } from 'zod'; import { AppError } from '../../middleware/errorHandler.js'; @@ -92,6 +92,16 @@ export function verifyWebhookSignature( const toleranceMs = parsed.toleranceSeconds * 1000; const timeDiff = Math.abs(now.getTime() - timestamp.getTime()); + // NaN fails every comparison, so an unparseable timestamp would otherwise pass the window. + if (Number.isNaN(timeDiff)) { + return { + isValid: false, + provider: parsed.provider, + timestamp, + error: 'Timestamp is not a valid date', + }; + } + if (timeDiff > toleranceMs) { return { isValid: false, @@ -125,19 +135,13 @@ export function verifyWebhookSignature( } /** - * Constant-time string comparison to prevent timing attacks + * Constant-time comparison of hex digests to prevent timing attacks. + * Compares the decoded bytes, so a non-hex or wrong-length signature fails closed. */ function constantTimeEquals(a: string, b: string): boolean { - if (a.length !== b.length) { - return false; - } - - let result = 0; - for (let i = 0; i < a.length; i++) { - result |= a.charCodeAt(i) ^ b.charCodeAt(i); - } - - return result === 0; + const aBuf = Buffer.from(a, 'hex'); + const bBuf = Buffer.from(b, 'hex'); + return aBuf.length > 0 && aBuf.length === bBuf.length && timingSafeEqual(aBuf, bBuf); } /** diff --git a/docs/PAYMENT_CORRECTNESS.md b/docs/PAYMENT_CORRECTNESS.md index 00c5d067..087bb11b 100644 --- a/docs/PAYMENT_CORRECTNESS.md +++ b/docs/PAYMENT_CORRECTNESS.md @@ -106,6 +106,15 @@ scheduled job to reclaim expired entries. Only responses below `500` are cached: a `5xx` may be transient. +## Batch totals are exact and never span assets + +`estimateBatch` and `getBatchReport` (`backend/src/services/batch.ts`) sum amounts with +`sumAmounts` at seven decimals (Stellar precision), not with `parseFloat`. Binary floating +point turned `0.1 + 0.2` into `0.30000000000000004` in reports shown to operators. + +Totals are per asset. A batch that pays both XLM and USDC reports `totalAmount: null` and +lists each asset under `byAsset`, because XLM and USDC are not comparable. + ## Unknown states are programming errors, not branches `PayoutBatch.status` is only ever changed through the engine's transition table. A