diff --git a/prisma/migrations/20260901000000_add_idempotency_expires_index/migration.sql b/prisma/migrations/20260901000000_add_idempotency_expires_index/migration.sql new file mode 100644 index 0000000..e21e908 --- /dev/null +++ b/prisma/migrations/20260901000000_add_idempotency_expires_index/migration.sql @@ -0,0 +1,5 @@ +-- CreateIndex +CREATE INDEX "Payment_idempotencyKeyExpiresAt_idx" ON "Payment"("idempotencyKeyExpiresAt"); + +-- CreateIndex +CREATE INDEX "Settlement_idempotencyKeyExpiresAt_idx" ON "Settlement"("idempotencyKeyExpiresAt"); diff --git a/prisma/schema.prisma b/prisma/schema.prisma index d72fe04..323a812 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -76,6 +76,8 @@ model Payment { fxQuoteExpiresAt DateTime? @@index([merchantId, status, createdAt(sort: Desc)]) + // #622 — supports the idempotency-key cleanup job's scan for expired keys. + @@index([idempotencyKeyExpiresAt]) } model Settlement { @@ -112,6 +114,8 @@ model Settlement { @@index([merchantId, initiatedAt(sort: Desc)]) @@index([merchantId, status, initiatedAt(sort: Desc)]) @@index([supersededById]) + // #622 — supports the idempotency-key cleanup job's scan for expired keys. + @@index([idempotencyKeyExpiresAt]) } model IndexedEvent { diff --git a/services/api-gateway/src/idempotency-key-cleanup-cron.test.ts b/services/api-gateway/src/idempotency-key-cleanup-cron.test.ts new file mode 100644 index 0000000..87fd14b --- /dev/null +++ b/services/api-gateway/src/idempotency-key-cleanup-cron.test.ts @@ -0,0 +1,146 @@ +import test from 'tape'; +import { reclaimExpiredIdempotencyKeys, getCronIntervalMs } from './idempotency-key-cleanup-cron.js'; + +interface MockPrisma { + payment: { + updateMany: (args: { where: any; data: any }) => Promise<{ count: number }>; + }; + settlement: { + updateMany: (args: { where: any; data: any }) => Promise<{ count: number }>; + }; +} + +interface MockLogger { + info: (obj: any, msg?: string) => void; + error: (obj: any, msg?: string) => void; + warn: (obj: any, msg?: string) => void; +} + +let lastPaymentArgs: { where: any; data: any } | null = null; +let lastSettlementArgs: { where: any; data: any } | null = null; + +function createMockPrisma(paymentCount: number, settlementCount: number): MockPrisma { + return { + payment: { + updateMany: async (args) => { + lastPaymentArgs = args; + return { count: paymentCount }; + }, + }, + settlement: { + updateMany: async (args) => { + lastSettlementArgs = args; + return { count: settlementCount }; + }, + }, + }; +} + +function createMockLogger(): MockLogger { + return { info: () => {}, error: () => {}, warn: () => {} }; +} + +test('reclaimExpiredIdempotencyKeys - nulls out expired keys on both Payment and Settlement', async (t) => { + lastPaymentArgs = null; + lastSettlementArgs = null; + const mockPrisma = createMockPrisma(2, 1); + const mockLogger = createMockLogger(); + + const total = await reclaimExpiredIdempotencyKeys(mockPrisma as any, mockLogger as any); + + t.equal(total, 3, 'returns the combined reclaimed count across both tables'); + + t.ok(lastPaymentArgs, 'Payment.updateMany should have been called'); + t.deepEqual(lastPaymentArgs?.data, { idempotencyKey: null, idempotencyKeyExpiresAt: null }, 'clears the key and its expiry on Payment'); + t.ok(lastPaymentArgs?.where.idempotencyKeyExpiresAt.lt instanceof Date, 'Payment filter compares expiresAt against now'); + t.equal(lastPaymentArgs?.where.idempotencyKey.not, null, 'Payment filter only targets rows that still have a key set'); + + t.ok(lastSettlementArgs, 'Settlement.updateMany should have been called'); + t.deepEqual(lastSettlementArgs?.data, { idempotencyKey: null, idempotencyKeyExpiresAt: null }, 'clears the key and its expiry on Settlement'); + t.ok(lastSettlementArgs?.where.idempotencyKeyExpiresAt.lt instanceof Date, 'Settlement filter compares expiresAt against now'); + + t.end(); +}); + +test('reclaimExpiredIdempotencyKeys - a row with expiresAt in the future is not targeted (time-mocked)', async (t) => { + lastPaymentArgs = null; + lastSettlementArgs = null; + const mockPrisma = createMockPrisma(0, 0); + const mockLogger = createMockLogger(); + + const now = Date.now(); + const futureExpiry = new Date(now + 60 * 60 * 1000); // expires in 1 hour — not yet reclaimable + + const total = await reclaimExpiredIdempotencyKeys(mockPrisma as any, mockLogger as any); + + t.equal(total, 0, 'returns 0 when nothing is expired'); + const cutoff = (lastPaymentArgs?.where.idempotencyKeyExpiresAt.lt as Date).getTime(); + t.ok(cutoff <= now + 1000, 'cutoff is "now", so a row expiring an hour from now would not match `lt` cutoff'); + t.ok(futureExpiry.getTime() > cutoff, 'sanity check: the future-expiring row is after the cutoff used by the query'); + + t.end(); +}); + +test('reclaimExpiredIdempotencyKeys - returns 0 and does not call Prisma when a concurrent run is in progress', async (t) => { + lastPaymentArgs = null; + lastSettlementArgs = null; + const mockPrisma = createMockPrisma(5, 5); + const mockLogger = createMockLogger(); + + // Kick off a slow first run without awaiting it, then immediately try a + // second run — it must see isCronRunning and bail out. + const slowPrisma: MockPrisma = { + payment: { + updateMany: async (args) => { + lastPaymentArgs = args; + await new Promise((resolve) => setTimeout(resolve, 50)); + return { count: 1 }; + }, + }, + settlement: { + updateMany: async (args) => { + lastSettlementArgs = args; + return { count: 0 }; + }, + }, + }; + + const firstRun = reclaimExpiredIdempotencyKeys(slowPrisma as any, mockLogger as any); + const secondRunCount = await reclaimExpiredIdempotencyKeys(mockPrisma as any, mockLogger as any); + + t.equal(secondRunCount, 0, 'concurrent second run returns 0 immediately'); + + const firstRunCount = await firstRun; + t.equal(firstRunCount, 1, 'first run completes normally'); + + t.end(); +}); + +test('reclaimExpiredIdempotencyKeys - skips execution when Redis lock cannot be acquired', async (t) => { + lastPaymentArgs = null; + const mockPrisma = createMockPrisma(3, 3); + const mockLogger = createMockLogger(); + + const mockRedisLocked = { set: async () => null }; + + const total = await reclaimExpiredIdempotencyKeys(mockPrisma as any, mockLogger as any, mockRedisLocked); + + t.equal(total, 0, 'returns 0 when lock fails'); + t.equal(lastPaymentArgs, null, 'Payment.updateMany is not called when lock is unavailable'); + + t.end(); +}); + +test('getCronIntervalMs - reads options or environment variable', (t) => { + t.equal(getCronIntervalMs({ intervalMs: 30000 }), 30000, 'reads intervalMs option'); + + const originalEnv = process.env.IDEMPOTENCY_KEY_CLEANUP_CRON_INTERVAL_MS; + process.env.IDEMPOTENCY_KEY_CLEANUP_CRON_INTERVAL_MS = '120000'; + t.equal(getCronIntervalMs(), 120000, 'reads env variable'); + + delete process.env.IDEMPOTENCY_KEY_CLEANUP_CRON_INTERVAL_MS; + t.equal(getCronIntervalMs(), 3600000, 'defaults to 1 hour (3600000ms)'); + + process.env.IDEMPOTENCY_KEY_CLEANUP_CRON_INTERVAL_MS = originalEnv; + t.end(); +}); diff --git a/services/api-gateway/src/idempotency-key-cleanup-cron.ts b/services/api-gateway/src/idempotency-key-cleanup-cron.ts new file mode 100644 index 0000000..7eda09b --- /dev/null +++ b/services/api-gateway/src/idempotency-key-cleanup-cron.ts @@ -0,0 +1,141 @@ +/** + * idempotency-key-cleanup-cron.ts (issue #622) + * + * Payment.idempotencyKey and Settlement.idempotencyKey are @unique so a + * stale row's key value permanently blocks a client from reusing that same + * key once its 24h idempotencyKeyExpiresAt window has passed — the DB + * uniqueness constraint doesn't know the key expired, only the application's + * `idempotencyKeyExpiresAt: { gt: now }` lookup does. This job periodically + * reclaims expired keys by nulling them out, freeing the value for reuse. + */ + +import type { PrismaClient } from '@prisma/client'; +import type { FastifyLoggerInstance } from 'fastify'; + +let cronInterval: NodeJS.Timeout | null = null; +let isCronRunning = false; + +export interface CronOptions { + intervalMs?: number; + jitterMs?: number; + redis?: any; +} + +export function getCronIntervalMs(opts?: CronOptions): number { + if (opts?.intervalMs && opts.intervalMs > 0) return opts.intervalMs; + const envVal = process.env.IDEMPOTENCY_KEY_CLEANUP_CRON_INTERVAL_MS; + if (envVal) { + const parsed = parseInt(envVal, 10); + if (!isNaN(parsed) && parsed > 0) return parsed; + } + return 60 * 60 * 1000; +} + +/** + * Lua script for atomic lock release — only deletes the lock if the stored + * value matches what we wrote, preventing accidental deletion of a lock + * acquired by another gateway instance. + */ +const RELEASE_LOCK_SCRIPT = ` + if redis.call("get", KEYS[1]) == ARGV[1] then + return redis.call("del", KEYS[1]) + else + return 0 + end +`; + +/** + * Nulls out idempotencyKey (and its expiry) on Payment and Settlement rows + * whose idempotencyKeyExpiresAt has passed, so the @unique constraint no + * longer blocks a client from reusing that key value. + * + * @returns the total number of rows reclaimed across both tables. + */ +export async function reclaimExpiredIdempotencyKeys( + prisma: PrismaClient, + logger: FastifyLoggerInstance, + redis?: any, +): Promise { + if (isCronRunning) { + logger.info('Idempotency key cleanup cron job already in progress; skipping execution.'); + return 0; + } + + let lockAcquired = false; + const lockKey = 'lock:idempotency-key-cleanup-cron'; + const lockVal = Math.random().toString(36).substring(2); + const lockTtlMs = 5 * 60 * 1000; + + if (redis && typeof redis.set === 'function') { + try { + const res = await redis.set(lockKey, lockVal, 'PX', lockTtlMs, 'NX'); + if (res !== 'OK' && res !== true && res !== '1') { + logger.info('Idempotency key cleanup cron lock held by another instance; skipping run.'); + return 0; + } + lockAcquired = true; + } catch (err) { + logger.warn({ err }, 'Failed to acquire Redis lock for idempotency key cleanup cron'); + } + } + + isCronRunning = true; + try { + const now = new Date(); + const where = { + idempotencyKey: { not: null }, + idempotencyKeyExpiresAt: { lt: now }, + }; + const data = { idempotencyKey: null, idempotencyKeyExpiresAt: null }; + + const [payments, settlements] = await Promise.all([ + prisma.payment.updateMany({ where, data }), + prisma.settlement.updateMany({ where, data }), + ]); + + const total = payments.count + settlements.count; + if (total > 0) { + logger.info( + { paymentsReclaimed: payments.count, settlementsReclaimed: settlements.count }, + 'Reclaimed expired idempotency keys.', + ); + } + return total; + } catch (error) { + logger.error({ err: error }, 'Error during idempotency key cleanup cron job.'); + return 0; + } finally { + isCronRunning = false; + if (lockAcquired && redis && typeof redis.eval === 'function') { + await redis.eval(RELEASE_LOCK_SCRIPT, 1, lockKey, lockVal).catch(() => {}); + } + } +} + +export function startIdempotencyKeyCleanupCron( + prisma: PrismaClient, + logger: FastifyLoggerInstance, + opts?: CronOptions, +) { + if (cronInterval) return; + + const intervalMs = getCronIntervalMs(opts); + + const runJob = async () => { + const jitter = Math.floor(Math.random() * (opts?.jitterMs ?? 1000)); + if (jitter > 0) { + await new Promise((resolve) => setTimeout(resolve, jitter)); + } + await reclaimExpiredIdempotencyKeys(prisma, logger, opts?.redis).catch((err) => + logger.error({ err }, 'Idempotency key cleanup cron job failed unexpectedly.'), + ); + }; + + cronInterval = setInterval(runJob, intervalMs); +} + +export function stopIdempotencyKeyCleanupCron() { + if (cronInterval) clearInterval(cronInterval); + cronInterval = null; + isCronRunning = false; +} diff --git a/services/api-gateway/src/index.ts b/services/api-gateway/src/index.ts index 1792578..e50c841 100644 --- a/services/api-gateway/src/index.ts +++ b/services/api-gateway/src/index.ts @@ -115,6 +115,10 @@ import { startAbandonedPaymentsCron, stopAbandonedPaymentsCron, } from "./abandoned-payments-cron.js"; +import { + startIdempotencyKeyCleanupCron, + stopIdempotencyKeyCleanupCron, +} from "./idempotency-key-cleanup-cron.js"; import { createWebhookQueue, type WebhookJobData, @@ -3185,6 +3189,7 @@ async function shutdown(signal: string) { } await getDefaultPrisma().$disconnect(); stopAbandonedPaymentsCron(); + stopIdempotencyKeyCleanupCron(); process.exit(0); } catch (err) { app.log.error(err, "Error during shutdown"); @@ -3221,6 +3226,7 @@ const start = async () => { (env as any).PAYMENT_ABANDONMENT_HOURS ?? 24, webhookQueue, ); + startIdempotencyKeyCleanupCron(prisma, app.log, { redis }); } await app.listen({ port: PORT, host: "0.0.0.0" }); } catch (err) { diff --git a/services/fx-engine/src/index.ts b/services/fx-engine/src/index.ts index 7bbf1bc..46180d6 100644 --- a/services/fx-engine/src/index.ts +++ b/services/fx-engine/src/index.ts @@ -1250,6 +1250,14 @@ const QuoteQuerySchema = z.object({ slippageBps: z .string() .regex(/^\d+$/, "slippageBps must be a non-negative integer") + // Issue #620: without an upper bound, a merchant could submit an + // arbitrarily large slippageBps (e.g. 10000 = 100%) and get silently + // clamped to env.MAX_SLIPPAGE_BPS below with no error signal — the + // request looked accepted while their actual tolerance was ignored. + // Reject out-of-range values outright instead. + .refine((val) => parseInt(val, 10) <= 1000, { + message: "slippageBps must be between 0 and 1000", + }) .optional(), }); diff --git a/services/fx-engine/src/quote-slippage-bounds.test.ts b/services/fx-engine/src/quote-slippage-bounds.test.ts new file mode 100644 index 0000000..ce39724 --- /dev/null +++ b/services/fx-engine/src/quote-slippage-bounds.test.ts @@ -0,0 +1,41 @@ +process.env.NODE_ENV = 'test'; +process.env.RATES_API_URL = 'http://localhost:1234'; +process.env.REDIS_URL = 'redis://localhost:6379'; +process.env.INTER_SERVICE_SECRET = 'test-secret-that-is-at-least-16-chars'; +process.env.MAX_STALE_SECONDS = '300'; +process.env.LOG_LEVEL = 'silent'; + +import test from 'tape'; + +const { fastify } = await import('./index.js'); + +// Issue #620 — a merchant submitting an out-of-range slippageBps used to be +// silently clamped to env.MAX_SLIPPAGE_BPS with no error signal. It must now +// be rejected with 400 instead. + +test('GET /api/quote rejects slippageBps=10000 (100%) with 400', async (t) => { + await fastify.ready(); + + const res = await fastify.inject({ + method: 'GET', + url: '/api/quote?from=USDC&to=NGN&amount=100&slippageBps=10000', + }); + + t.equal(res.statusCode, 400, 'rejects slippageBps above the allowed 0-1000 range'); + t.end(); +}); + +test('GET /api/quote accepts slippageBps within the allowed range and echoes slippageLimit', async (t) => { + const res = await fastify.inject({ + method: 'GET', + url: '/api/quote?from=USDC&to=NGN&amount=100&slippageBps=250', + }); + + t.equal(res.statusCode, 200, 'accepts an in-range slippageBps'); + const body = JSON.parse(res.body); + t.equal(body.slippageBps, 250, 'echoes the validated slippageBps'); + t.equal(body.slippageLimit, (250 / 10_000).toFixed(4), 'slippageLimit matches slippageBps/10000'); + + t.end(); + process.exit(0); +}); diff --git a/shared/validation/audit.test.ts b/shared/validation/audit.test.ts new file mode 100644 index 0000000..96ba5e7 --- /dev/null +++ b/shared/validation/audit.test.ts @@ -0,0 +1,81 @@ +import test from 'node:test'; +import assert from 'node:assert'; +import { getClientIp } from './audit.js'; + +// Issue #621 — AuditLog.ipAddress used to trust X-Forwarded-For unconditionally, +// letting a direct (non-proxied) attacker spoof it and poison the audit trail. + +test('getClientIp: trustedProxyCount=0 (default) ignores X-Forwarded-For and uses request.ip', () => { + const request = { + headers: { 'x-forwarded-for': '1.1.1.1' }, + ip: '203.0.113.9', + }; + assert.strictEqual(getClientIp(request, 0), '203.0.113.9'); + assert.strictEqual(getClientIp(request), '203.0.113.9', 'defaults to trustedProxyCount=0'); +}); + +test('getClientIp: spoofed X-Forwarded-For from an untrusted direct client is ignored', () => { + // No real proxy in front — request.ip is the attacker's own address, and + // they set X-Forwarded-For themselves trying to impersonate someone else. + const request = { + headers: { 'x-forwarded-for': '1.1.1.1' }, + ip: '198.51.100.7', // attacker's real address + }; + assert.strictEqual(getClientIp(request, 0), '198.51.100.7'); +}); + +test('getClientIp: 1-proxy chain extracts the original client through a single trusted proxy', () => { + // real client 203.0.113.9 -> trusted LB (appends its view of the source, + // then forwards; our socket sees the LB itself as the direct peer) + const request = { + headers: { 'x-forwarded-for': '203.0.113.9' }, + ip: '10.0.0.1', // the trusted LB's address + }; + assert.strictEqual(getClientIp(request, 1), '203.0.113.9'); +}); + +test('getClientIp: 2-proxy chain strips both trusted hops to find the original client', () => { + // real client 203.0.113.9 -> CDN edge -> internal LB -> us. + // X-Forwarded-For accumulates left-to-right as it passes through each hop. + const request = { + headers: { 'x-forwarded-for': '203.0.113.9, 10.0.0.2' }, + ip: '10.0.0.1', // the internal LB's address (direct socket peer) + }; + assert.strictEqual(getClientIp(request, 2), '203.0.113.9'); +}); + +test('getClientIp: 1-proxy trust only strips the single trusted hop, not entries further upstream', () => { + // With trustedProxyCount=1 we trust that exactly one proxy appended the + // entry immediately before our socket peer — "203.0.113.9" here. Anything + // further left ("9.9.9.9") arrived already in the header before it ever + // reached that trusted proxy, so it is not the resolved client, same as + // Express/Fastify's trust proxy: N semantics — this is why the default + // stays 0 unless the deployment topology genuinely has a proxy there. + const request = { + headers: { 'x-forwarded-for': '9.9.9.9, 203.0.113.9' }, + ip: '10.0.0.1', + }; + assert.strictEqual(getClientIp(request, 1), '203.0.113.9'); +}); + +test('getClientIp: array-valued X-Forwarded-For header is handled', () => { + const request = { + headers: { 'x-forwarded-for': ['203.0.113.9, 10.0.0.2'] }, + ip: '10.0.0.1', + }; + assert.strictEqual(getClientIp(request, 2), '203.0.113.9'); +}); + +test('getClientIp: falls back to request.ip when X-Forwarded-For is absent even with trust configured', () => { + const request = { headers: {}, ip: '203.0.113.9' }; + assert.strictEqual(getClientIp(request, 1), '203.0.113.9'); +}); + +test('getClientIp: X-Real-IP is only trusted when trustedProxyCount > 0', () => { + const request = { + headers: { 'x-real-ip': '1.1.1.1' }, + ip: '198.51.100.7', + }; + assert.strictEqual(getClientIp(request, 0), '198.51.100.7', 'ignored when untrusted'); + assert.strictEqual(getClientIp(request, 1), '1.1.1.1', 'trusted when a proxy is configured'); +}); diff --git a/shared/validation/audit.ts b/shared/validation/audit.ts index 71f2131..57be13d 100644 --- a/shared/validation/audit.ts +++ b/shared/validation/audit.ts @@ -17,24 +17,58 @@ export interface AuditLogRequestLike { user?: unknown; } -function getRequestIp(request?: AuditLogRequestLike | null): string | null { - if (!request) return null; +function parseTrustedProxyCount(raw: string | undefined): number { + const parsed = raw !== undefined ? parseInt(raw, 10) : 0; + return Number.isFinite(parsed) && parsed > 0 ? parsed : 0; +} + +/** + * Extract the real client IP from a request, honoring a configured number of + * trusted reverse-proxy hops (#621). + * + * X-Forwarded-For (and X-Real-IP) are entirely attacker-controlled unless + * something in front of this process is known to overwrite/append to them + * honestly. With trustedProxyCount = 0 (the default), neither header is + * consulted — the request's own transport-layer address (request.ip) is + * used, which the client cannot spoof. With trustedProxyCount = N, the + * X-Forwarded-For chain is combined with request.ip into one hop list + * [xff-entries..., request.ip] and the client is taken to be the entry N + * positions in from the right — i.e. the entries a chain of N trusted + * proxies would have appended are stripped off, mirroring Express's/ + * Fastify's `trust proxy: N` semantics. + */ +export function getClientIp( + request: AuditLogRequestLike, + trustedProxyCount: number = parseTrustedProxyCount(process.env.TRUSTED_PROXY_COUNT), +): string { + const fallback = request.ip ?? ''; + if (trustedProxyCount <= 0) return fallback; const forwardedFor = request.headers?.['x-forwarded-for']; - if (typeof forwardedFor === 'string') { - const [first] = forwardedFor.split(','); - return first?.trim() || null; - } - if (Array.isArray(forwardedFor) && forwardedFor.length > 0) { - return String(forwardedFor[0]).trim() || null; + const rawForwardedFor = Array.isArray(forwardedFor) ? forwardedFor.join(',') : forwardedFor; + + if (typeof rawForwardedFor === 'string' && rawForwardedFor.trim()) { + const hops = rawForwardedFor + .split(',') + .map((hop) => hop.trim()) + .filter(Boolean); + const chain = [...hops, fallback]; + const clientIndex = Math.max(0, chain.length - 1 - trustedProxyCount); + return chain[clientIndex] || fallback; } const realIp = request.headers?.['x-real-ip']; - if (typeof realIp === 'string' && realIp.trim()) { - return realIp.trim(); + const rawRealIp = Array.isArray(realIp) ? realIp[0] : realIp; + if (typeof rawRealIp === 'string' && rawRealIp.trim()) { + return rawRealIp.trim(); } - return request.ip ?? null; + return fallback; +} + +function getRequestIp(request?: AuditLogRequestLike | null): string | null { + if (!request) return null; + return getClientIp(request) || null; } function getActorFromRequest(request?: AuditLogRequestLike | null): { diff --git a/shared/validation/index.ts b/shared/validation/index.ts index 1285e05..1edea1d 100644 --- a/shared/validation/index.ts +++ b/shared/validation/index.ts @@ -273,6 +273,16 @@ export const EnvSchema = z SETTLEMENT_ENGINE_URL: z.string().url().default("http://localhost:3001"), INDEXER_URL: z.string().url().default("http://localhost:3003"), + // Number of trusted reverse-proxy hops in front of the gateway (#621). + // X-Forwarded-For / X-Real-IP are only consulted when this is > 0 — the + // header is otherwise attacker-controlled and unconditionally trusting it + // (the prior behavior) let a spoofed X-Forwarded-For poison AuditLog.ipAddress. + // Default 0 means: never trust the header, always use the raw socket address. + TRUSTED_PROXY_COUNT: z + .string() + .transform((s) => parseInt(s, 10)) + .default("0"), + // FX Engine — live rate fetching and caching RATES_API_URL: z .string()