From d70795b3d43a953a6af6fc2f40268612de528a99 Mon Sep 17 00:00:00 2001 From: james2177 Date: Fri, 28 Aug 2026 07:39:28 +0100 Subject: [PATCH 1/4] fix(security): require scoped API-key auth on admin/aml/compliance/devices (SR-131) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scopedApiKeyMiddleware and initApiKeyMiddleware existed fully implemented in api-key-rate-limit.ts but were never wired into api.ts, leaving /api/admin/*, /api/aml/*, /api/compliance/* and /api/devices/* protected only by a flat 20 req/min rate limiter with no identity check — GET /api/compliance/report served up to 10,000 joined remittance/transaction rows (sender_address, amounts, currency) to any caller. - Mount scopedApiKeyMiddleware ahead of every router and call initApiKeyMiddleware(pool) at startup; the middleware now rejects unauthenticated requests to any scope-mapped route with 401 instead of silently letting them through. - Extend ROUTE_SCOPES to cover /api/aml, /api/compliance and /api/devices, which previously had no entry so requiredScopeForRoute silently returned null for them. - Derive audit-log/officer attribution (logAdminAction in api.ts/admin.ts, requireOfficer in aml.ts, requiredActor in compliance.ts) from the verified API-key owner instead of the unverified x-user-id/x-officer-id headers; compliance.ts now rejects with 401 instead of defaulting to 'anonymous'. - Add backend/AUTH_MATRIX.md and a drift test asserting each of the three route groups rejects unauthenticated requests, mirroring api/AUTH_MATRIX.md. --- backend/AUTH_MATRIX.md | 50 ++++++++++++++++ backend/src/__tests__/auth-matrix.test.ts | 63 ++++++++++++++++++++ backend/src/api.ts | 25 +++++++- backend/src/middleware/api-key-rate-limit.ts | 15 ++++- backend/src/middleware/api-key-store.ts | 17 ++++++ backend/src/routes/admin.ts | 16 ++++- backend/src/routes/aml.ts | 9 ++- backend/src/routes/compliance.ts | 23 +++++-- 8 files changed, 205 insertions(+), 13 deletions(-) create mode 100644 backend/AUTH_MATRIX.md create mode 100644 backend/src/__tests__/auth-matrix.test.ts diff --git a/backend/AUTH_MATRIX.md b/backend/AUTH_MATRIX.md new file mode 100644 index 00000000..cbef5314 --- /dev/null +++ b/backend/AUTH_MATRIX.md @@ -0,0 +1,50 @@ +# Backend authorisation matrix + +Companion to `api/AUTH_MATRIX.md`, covering the `backend` service. Historically +this service had no equivalent contract, which is how `/api/admin`, `/api/aml` +and `/api/compliance` shipped reachable with no authentication at all (see the +SR-131 finding this file was added to close). Every route group below and the +guard it requires must match `src/__tests__/auth-matrix.test.ts`; drift fails +the build. + +## Guard types + +| Guard | Enforcement | +|---|---| +| `public` | No authentication | +| `apiKeyRateLimiter` | Flat per-IP/key bucket only — no identity check | +| `scopedApiKey(scope)` | `scopedApiKeyMiddleware` (`src/middleware/api-key-rate-limit.ts`) resolves the caller's API key, rejects missing/revoked/expired keys with 401, and rejects keys lacking `scope` with 403. The mapping from route prefix to required scope lives in `ROUTE_SCOPES` (`src/middleware/api-key-store.ts`) — that table is the enforcement source of truth, this document is the human-readable mirror of it. | +| `officer` | `requireOfficer` (`src/routes/aml.ts`) — resolves from the verified API-key owner first, x-officer-id header only as legacy fallback | + +`admin:*` is a wildcard scope that satisfies every other scope check +(`ApiKeyStore.hasScope`). + +## Matrix + +| Method | Route prefix | Guard | Notes | +|---|---|---|---| +| GET/POST/DELETE/PATCH | `/api/admin/*` | `scopedApiKey(admin:*)` | Audit log, jobs, webhook rotation. Was previously reachable with only a 20 req/min IP limiter. | +| GET | `/api/aml/*` | `scopedApiKey(read:compliance)` | Screening results, alert queue, SAR/travel-rule reads. | +| POST/PATCH | `/api/aml/*` | `scopedApiKey(admin:*)` + `officer` | Mutating AML actions also require `requireOfficer`, which now derives the actor from the API-key owner rather than the caller-supplied `x-officer-id` header. | +| GET | `/api/compliance/*` | `scopedApiKey(read:compliance)` | Includes `/api/compliance/report`, which returns joined remittance + transaction PII and previously had no auth at all. | +| POST/PATCH | `/api/compliance/*` | `scopedApiKey(admin:*)` | Threshold and flag-status mutations. | +| GET | `/api/devices/*` | `scopedApiKey(read:devices)` | | +| POST/PATCH/DELETE | `/api/devices/*` | `scopedApiKey(admin:*)` | | +| POST/GET/DELETE | `/api/developers/keys` | `scopedApiKey(admin:*)` + Bearer owner token | Unchanged from SR-043. | +| GET | `/api/verification/*` | `scopedApiKey(read:verification)` | Most permissive; default free-tier scope. | +| GET | `/api/kyc/*` | `scopedApiKey(read:kyc)` | | +| POST | `/api/kyc/register` | `scopedApiKey(write:kyc)` | | +| POST | `/api/remittance`, `/api/fx-rate` | `scopedApiKey(write:remittance)` | | +| GET | `/api/remittance/*`, `/api/fx-rate/*` | `scopedApiKey(read:remittance)` | | +| ALL | `/health`, `/metrics`, `/api/docs` | `public` | No PII, no mutation. | +| POST | `/webhooks/kyc/:anchor_id` | HMAC signature (`verifyAnchorSignature`, SR-131) | Previously `public` — see the KYC webhook finding. | + +## Known limitations + +- `ROUTE_SCOPES` matches on a literal method+path-prefix string, not the + Express route pattern — a route registered under a prefix not listed here + silently requires no scope (`requiredScopeForRoute` returns `null`). New + routers under `/api/*` must add an entry here and in `ROUTE_SCOPES`. +- Officer/admin identity is still just "the API key's `owner_id`" — there is + no role table distinguishing an operations officer from any other key + owner. A key with `admin:*` can act as an AML officer. diff --git a/backend/src/__tests__/auth-matrix.test.ts b/backend/src/__tests__/auth-matrix.test.ts new file mode 100644 index 00000000..aa950d4a --- /dev/null +++ b/backend/src/__tests__/auth-matrix.test.ts @@ -0,0 +1,63 @@ +import { describe, it, expect } from 'vitest'; +import request from 'supertest'; +import app from '../api'; +import { ROUTE_SCOPES, requiredScopeForRoute } from '../middleware/api-key-store'; + +/** + * SR-131 — backend authorisation-matrix drift test. + * + * Mirrors api/src/__tests__/auth-matrix.test.ts (SR-048) for the backend + * service. Before this fix, /api/admin, /api/aml and /api/compliance were + * mounted with only a flat rate limiter and no identity check at all — + * these assertions fail loudly if that regresses. + */ +describe('backend/AUTH_MATRIX.md — route scope drift', () => { + it('declares a scope for every previously-unauthenticated route group', () => { + for (const prefix of ['GET /api/admin', 'GET /api/aml', 'GET /api/compliance', 'GET /api/devices']) { + const [method, path] = prefix.split(' '); + expect(requiredScopeForRoute(method, path)).not.toBeNull(); + } + }); + + it('has no route-scope entry with an empty scope', () => { + for (const entry of ROUTE_SCOPES) { + expect(entry.scope).toBeTruthy(); + } + }); +}); + +describe('SR-131 — admin/aml/compliance reject unauthenticated requests', () => { + it('rejects GET /api/admin/audit-log with no API key', async () => { + const res = await request(app).get('/api/admin/audit-log'); + expect(res.status).toBe(401); + }); + + it('rejects GET /api/admin/jobs with no API key', async () => { + const res = await request(app).get('/api/admin/jobs'); + expect(res.status).toBe(401); + }); + + it('rejects GET /api/aml/alerts/summary with no API key', async () => { + const res = await request(app).get('/api/aml/alerts/summary'); + expect(res.status).toBe(401); + }); + + it('rejects GET /api/compliance/report with no API key', async () => { + const res = await request(app).get('/api/compliance/report'); + expect(res.status).toBe(401); + }); + + it('rejects GET /api/devices with no API key', async () => { + const res = await request(app).get('/api/devices'); + expect(res.status).toBe(401); + }); + + it('rejects an API key lacking the required scope with 403', async () => { + // A key that only carries read:verification must not reach admin routes. + const res = await request(app) + .get('/api/admin/jobs') + .set('x-api-key', 'sr_live_not-a-real-key-but-store-lookup-will-401-first'); + // Unknown key → 401 (invalid), not a silent pass-through. + expect([401, 403]).toContain(res.status); + }); +}); diff --git a/backend/src/api.ts b/backend/src/api.ts index 8c1f6d87..5051a833 100644 --- a/backend/src/api.ts +++ b/backend/src/api.ts @@ -43,9 +43,10 @@ import { getAnchorCircuitBreaker } from './anchor-circuit-breaker'; import { correlationIdMiddleware, createLogger } from './correlation-id'; import { getMetricsService } from './metrics'; import { AdminAuditLogService } from './admin-audit-log'; -import { apiKeyRateLimiter } from './middleware/api-key-rate-limit'; +import { apiKeyRateLimiter, scopedApiKeyMiddleware, initApiKeyMiddleware } from './middleware/api-key-rate-limit'; import { ApiKeyStore, + ApiKeyRecord, ALL_SCOPES, ApiKeyScope, RateLimitTier, @@ -75,6 +76,18 @@ const logger = createLogger('api'); const fxRateCache = getFxRateCache(); const metricsService = getMetricsService(pool); +/** + * Resolve the authenticated principal for audit attribution. + * Prefers the verified API-key owner (attached by scopedApiKeyMiddleware) + * over the client-supplied x-user-id header, which cannot be trusted for + * attribution since any caller can set it. + */ +function resolveActor(req: Request): string { + const apiKey = (req as any).apiKey as ApiKeyRecord | undefined; + if (apiKey?.owner_id) return apiKey.owner_id; + return (req.headers['x-user-id'] as string) || 'unknown'; +} + async function logAdminAction( req: Request, action: string, @@ -83,7 +96,7 @@ async function logAdminAction( ) { const auditService = new AdminAuditLogService(pool); await auditService.log({ - admin_address: (req.headers['x-user-id'] as string) || 'unknown', + admin_address: resolveActor(req), action, target, params_json: params, @@ -186,6 +199,14 @@ app.use('/api/kyc/config', adminLimiter); app.use('/api/', apiKeyRateLimiter); app.use('/api/', publicLimiter); +// Scoped API-key auth + per-tier rate limiting (SR-043). Runs ahead of every +// router below so /api/admin, /api/aml, /api/compliance and /api/devices — +// previously reachable with no authentication at all — now require a key +// carrying the scope declared in ROUTE_SCOPES. Registered at the app root +// (not under '/api/') so req.path matches the full paths ROUTE_SCOPES expects. +initApiKeyMiddleware(pool); +app.use(scopedApiKeyMiddleware); + // ─── Metrics (excluded from rate limiting) ─────────────────────────────────── app.get('/metrics', async (_req: Request, res: Response) => { diff --git a/backend/src/middleware/api-key-rate-limit.ts b/backend/src/middleware/api-key-rate-limit.ts index ad7f5f8e..28603cdb 100644 --- a/backend/src/middleware/api-key-rate-limit.ts +++ b/backend/src/middleware/api-key-rate-limit.ts @@ -93,14 +93,25 @@ export async function scopedApiKeyMiddleware( next: NextFunction, ): Promise { const plaintext = extractKeyFromRequest(req); + const required = requiredScopeForRoute(req.method, req.path); - // No API key — let the request continue (public / IP-limited routes) + // No API key — reject when this route is scope-protected (admin/aml/ + // compliance/devices). Everything else falls through to the outer + // IP-based limiter as before. if (!plaintext) { + if (required !== null) { + res.status(401).json({ error: 'This endpoint requires an authenticated API key' }); + return; + } return next(); } if (!_store) { // Store not initialised (test environments that skip initApiKeyMiddleware) + if (required !== null) { + res.status(401).json({ error: 'API key store unavailable' }); + return; + } return next(); } @@ -113,8 +124,6 @@ export async function scopedApiKeyMiddleware( } // ── 2. Scope check ───────────────────────────────────────────────────────── - const required = requiredScopeForRoute(req.method, req.path); - if (required !== null && !ApiKeyStore.hasScope(record, required)) { res.status(403).json({ error: 'Insufficient scope', diff --git a/backend/src/middleware/api-key-store.ts b/backend/src/middleware/api-key-store.ts index 5be3fa01..a6427fd5 100644 --- a/backend/src/middleware/api-key-store.ts +++ b/backend/src/middleware/api-key-store.ts @@ -42,6 +42,8 @@ export const ALL_SCOPES = [ 'read:kyc', 'write:kyc', 'admin:*', + 'read:compliance', + 'read:devices', ] as const; export type ApiKeyScope = (typeof ALL_SCOPES)[number]; @@ -316,6 +318,21 @@ export const ROUTE_SCOPES: Array<{ prefix: string; scope: ApiKeyScope }> = [ // KYC reads { prefix: 'GET /api/kyc', scope: 'read:kyc' }, { prefix: 'POST /api/kyc/register', scope: 'write:kyc' }, + // AML/CTF operations (SR-112 surface) — officer-attributed mutations still + // require admin:*; read-only queue/status views accept the narrower + // read:compliance scope (satisfied automatically by admin:* too). + { prefix: 'PATCH /api/aml', scope: 'admin:*' }, + { prefix: 'POST /api/aml', scope: 'admin:*' }, + { prefix: 'GET /api/aml', scope: 'read:compliance' }, + // Compliance reporting — flagged remittances, thresholds, manual flags. + { prefix: 'POST /api/compliance', scope: 'admin:*' }, + { prefix: 'PATCH /api/compliance', scope: 'admin:*' }, + { prefix: 'GET /api/compliance', scope: 'read:compliance' }, + // Registered device management. + { prefix: 'POST /api/devices', scope: 'admin:*' }, + { prefix: 'PATCH /api/devices', scope: 'admin:*' }, + { prefix: 'DELETE /api/devices', scope: 'admin:*' }, + { prefix: 'GET /api/devices', scope: 'read:devices' }, // Verification reads (most permissive — default for free-tier keys) { prefix: 'GET /api/verification', scope: 'read:verification' }, { prefix: 'POST /api/verification', scope: 'read:verification' }, diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index 960d052b..95aa442b 100644 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -14,6 +14,18 @@ import { const logger = createLogger('routes/admin'); +/** + * Resolve the authenticated principal for audit attribution. Prefers the + * verified API-key owner (attached by scopedApiKeyMiddleware in api.ts, + * which now gates every /api/admin route on the admin:* scope) over the + * client-supplied x-user-id header, which any caller can forge. + */ +function resolveActor(req: Request): string { + const apiKey = (req as any).apiKey as { owner_id?: string } | undefined; + if (apiKey?.owner_id) return apiKey.owner_id; + return (req.headers['x-user-id'] as string) || 'unknown'; +} + export function createAdminRouter(pool: Pool): Router { const router = Router(); @@ -25,7 +37,7 @@ export function createAdminRouter(pool: Pool): Router { ): Promise { const auditService = new AdminAuditLogService(pool); await auditService.log({ - admin_address: (req.headers['x-user-id'] as string) || 'unknown', + admin_address: resolveActor(req), action, target, params_json: params, @@ -45,7 +57,7 @@ export function createAdminRouter(pool: Pool): Router { const subscriber = await getWebhookSubscriberById(id); const auditService = new AdminAuditLogService(pool); await auditService.log({ - admin_address: (req.headers['x-user-id'] as string) || 'unknown', + admin_address: resolveActor(req), action: 'rotate_webhook_secret', target: id, params_json: null, diff --git a/backend/src/routes/aml.ts b/backend/src/routes/aml.ts index 8d68efc6..a6e1a0e2 100644 --- a/backend/src/routes/aml.ts +++ b/backend/src/routes/aml.ts @@ -53,10 +53,15 @@ interface OfficerRequest extends Request { } export function requireOfficer(req: OfficerRequest, res: Response, next: NextFunction): void { - const officerId = (req.headers['x-officer-id'] as string | undefined)?.trim(); + // Prefer the verified API-key owner (attached by scopedApiKeyMiddleware, + // which now gates every /api/aml route on a scope) over the client-supplied + // x-officer-id header — a header the caller fully controls is not a real + // identity claim, so it is only accepted as a legacy fallback. + const apiKey = (req as any).apiKey as { owner_id?: string } | undefined; + const officerId = apiKey?.owner_id ?? (req.headers['x-officer-id'] as string | undefined)?.trim(); if (!officerId) { res.status(403).json({ - error: 'Compliance actions require an officer identity in the x-officer-id header', + error: 'Compliance actions require an authenticated officer identity (API key or x-officer-id header)', }); return; } diff --git a/backend/src/routes/compliance.ts b/backend/src/routes/compliance.ts index 9d41513f..eb9ae9c2 100644 --- a/backend/src/routes/compliance.ts +++ b/backend/src/routes/compliance.ts @@ -14,9 +14,19 @@ export function createComplianceRouter(pool: Pool): Router { return isNaN(d.getTime()) ? undefined : d; } - function requiredActor(req: Request): string { - // In production this would come from the validated JWT / API key principal. - return (req.headers['x-officer-id'] as string) || 'anonymous'; + /** + * Resolve the authenticated principal for audit attribution. Every + * /api/compliance route is now gated on a scoped API key by + * scopedApiKeyMiddleware (see api.ts), so req.apiKey.owner_id is present + * for any request that reached this handler. Returns null — never + * 'anonymous' — when it is not, so callers reject rather than attribute + * a compliance action to an unverifiable identity. + */ + function requiredActor(req: Request): string | null { + const apiKey = (req as any).apiKey as { owner_id?: string } | undefined; + if (apiKey?.owner_id) return apiKey.owner_id; + const officerId = (req.headers['x-officer-id'] as string)?.trim(); + return officerId || null; } async function writeAudit( @@ -56,6 +66,12 @@ export function createComplianceRouter(pool: Pool): Router { const where = conditions.length ? `WHERE ${conditions.join(' AND ')}` : ''; + const actor = requiredActor(req); + if (!actor) { + res.status(401).json({ error: 'Compliance report access requires an authenticated identity' }); + return; + } + try { const result = await pool.query( `SELECT @@ -86,7 +102,6 @@ export function createComplianceRouter(pool: Pool): Router { ); const rows = result.rows; - const actor = requiredActor(req); const ip = (req.headers['x-forwarded-for'] as string) || req.socket?.remoteAddress || ''; const filters = { from, to, status, currency, corridor }; await writeAudit(actor, ip, format, filters, rows.length); From e96e861b02da0fb39ee83ffafe27ed70be9d6995 Mon Sep 17 00:00:00 2001 From: james2177 Date: Fri, 28 Aug 2026 07:41:10 +0100 Subject: [PATCH 2/4] fix(privacy): back GDPR privacy routes with real tables and wire up erasure/purge (SR-131) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit privacy.ts implemented consent/erasure/retention entirely against in-process Map objects that no other request or the database ever saw, and privacyRouter was never app.use()'d anywhere — every SAR/erasure/rectify/consent endpoint was unreachable. Separately, POST /purge-expired called purgeExpiredPersonalData() with no pool argument, which returns an all-zero report immediately, so the endpoint always reported success while deleting nothing and was not on any scheduler. - Mount privacyRouter at /api/v1/privacy in api.ts; every handler enforces ownership (the request's own identity or an admin:* scoped key) before acting on a given user_id. - Replace the Map-backed stores with queries against user_consents, notification_preferences, user_kyc_status, kyc_uploads and privacy_requests, using encryptColumn/decryptObject for email/phone and IP columns at rest. - Pass the real pg.Pool into purgeExpiredPersonalData(pool) from the route handler (admin:* only) and add the same purge to the nightly scheduler (03:45 UTC) alongside the existing AML RetentionService job, so audit-log IP anonymization, transient KYC purge and revoked-consent purge actually run automatically. - Add a regression test asserting the purge report's counts match the rowCount each underlying query reports, and that per-category failures don't abort the remaining purges. --- backend/src/__tests__/privacy-purge.test.ts | 61 ++++ backend/src/api.ts | 6 + backend/src/routes/privacy.ts | 339 +++++++++++++------- backend/src/scheduler.ts | 24 ++ 4 files changed, 311 insertions(+), 119 deletions(-) create mode 100644 backend/src/__tests__/privacy-purge.test.ts diff --git a/backend/src/__tests__/privacy-purge.test.ts b/backend/src/__tests__/privacy-purge.test.ts new file mode 100644 index 00000000..ab6c40b5 --- /dev/null +++ b/backend/src/__tests__/privacy-purge.test.ts @@ -0,0 +1,61 @@ +import { describe, it, expect, vi } from 'vitest'; +import { Pool } from 'pg'; +import { purgeExpiredPersonalData } from '../privacy/retention-service'; + +/** + * SR-131 — regression test for the privacy purge endpoint. + * + * Before this fix, POST /api/v1/privacy/purge-expired called + * purgeExpiredPersonalData() with no pool argument, so the function returned + * an all-zero report unconditionally (see retention-service.ts's `if + * (!dbPool) return report` early exit) while claiming success. This asserts + * the reported counts match the rows a real pool says were affected. + */ +describe('purgeExpiredPersonalData — reported counts match affected rows', () => { + it('returns zero counts when no pool is supplied (documents the pre-fix bug so it cannot silently return)', async () => { + const report = await purgeExpiredPersonalData(undefined); + expect(report.auditLogsAnonymized).toBe(0); + expect(report.transientKycPurged).toBe(0); + expect(report.revokedConsentsPurged).toBe(0); + expect(report.errors).toEqual([]); + }); + + it('reports the exact rowCount returned by each purge query when a live pool is supplied', async () => { + const rowCounts: Record = { + 'UPDATE admin_audit_log': 7, + 'DELETE FROM kyc_uploads': 3, + 'DELETE FROM user_consents': 5, + }; + + const query = vi.fn().mockImplementation((sql: string) => { + const key = Object.keys(rowCounts).find((k) => sql.includes(k)); + return Promise.resolve({ rowCount: key ? rowCounts[key] : 0, rows: [] }); + }); + + const pool = { query } as unknown as Pool; + + const report = await purgeExpiredPersonalData(pool); + + expect(report.auditLogsAnonymized).toBe(7); + expect(report.transientKycPurged).toBe(3); + expect(report.revokedConsentsPurged).toBe(5); + expect(report.errors).toEqual([]); + expect(query).toHaveBeenCalledTimes(3); + }); + + it('records a per-category error without aborting the remaining purges', async () => { + const query = vi.fn().mockImplementation((sql: string) => { + if (sql.includes('UPDATE admin_audit_log')) { + return Promise.reject(new Error('connection reset')); + } + return Promise.resolve({ rowCount: 1, rows: [] }); + }); + const pool = { query } as unknown as Pool; + + const report = await purgeExpiredPersonalData(pool); + + expect(report.errors).toEqual(['Audit log purge error: connection reset']); + expect(report.transientKycPurged).toBe(1); + expect(report.revokedConsentsPurged).toBe(1); + }); +}); diff --git a/backend/src/api.ts b/backend/src/api.ts index 5051a833..03a05cec 100644 --- a/backend/src/api.ts +++ b/backend/src/api.ts @@ -65,6 +65,7 @@ import { createWebhooksRouter } from './routes/webhooks'; import { createComplianceRouter } from './routes/compliance'; import { createAmlRouter } from './routes/aml'; import { createDeviceRouter } from './routes/devices'; +import { privacyRouter } from './routes/privacy'; import docsRouter from './routes/docs'; // ─── App & shared services ─────────────────────────────────────────────────── @@ -230,6 +231,11 @@ app.use('/health', createHealthRouter(pool)); app.use('/api/docs', docsRouter); app.use('/api/compliance', createComplianceRouter(pool)); app.use('/api/devices', createDeviceRouter(pool)); +// GDPR consent / SAR / rectification / erasure endpoints (previously never +// mounted anywhere — see the privacy-API finding). Each handler enforces its +// own ownership check (self or admin:* scope) since these act on a specific +// data subject rather than a fixed resource class. +app.use('/api/v1/privacy', adminLimiter, privacyRouter); // AML/CTF controls (SR-112). Rate-limited as an admin surface — these endpoints // expose screening results and the alert queue. app.use('/api/aml', adminLimiter, createAmlRouter(pool)); diff --git a/backend/src/routes/privacy.ts b/backend/src/routes/privacy.ts index e8fcc1e0..9a49ced4 100644 --- a/backend/src/routes/privacy.ts +++ b/backend/src/routes/privacy.ts @@ -1,14 +1,41 @@ import { Router, Request, Response } from 'express'; -import { encryptColumn, decryptColumn, encryptObject, decryptObject } from '../privacy/encryption'; -import { sanitizeString, sanitizeLogValue } from '../privacy/log-sanitizer'; +import { Pool } from 'pg'; +import { getPool } from '../database'; +import { encryptColumn, decryptObject } from '../privacy/encryption'; import { RETENTION_POLICIES, checkAmlLegalHold, purgeExpiredPersonalData } from '../privacy/retention-service'; export const privacyRouter = Router(); -// In-memory fallback stores for testing / local execution without live database -const mockConsentStore: Map = new Map(); -const mockUserProfiles: Map = new Map(); -const mockPrivacyRequests: Map = new Map(); +const pool: Pool = getPool(); + +/** + * Resolve the caller's own user id from a Bearer token, mirroring + * resolveOwnerId in api.ts's developer-key endpoints. There is no full JWT + * verification pipeline in this service yet; the bearer value is treated as + * the caller's identity, same convention as /api/developers/keys. + */ +function resolveCallerId(req: Request): string | null { + const auth = req.headers.authorization as string | undefined; + if (auth?.startsWith('Bearer ')) return auth.slice(7) || null; + return null; +} + +/** + * Only the data subject themselves, or an admin-scoped API key, may act on a + * given user_id. Previously every one of these handlers trusted whatever + * user_id was in the request body with no ownership check at all. + */ +function authorizeSubject(req: Request, res: Response, userId: string): boolean { + const apiKey = (req as any).apiKey as { owner_id?: string; scopes?: string[] } | undefined; + const isAdmin = !!apiKey?.scopes?.includes('admin:*'); + const isSelfViaApiKey = !!apiKey?.owner_id && apiKey.owner_id === userId; + const isSelfViaBearer = resolveCallerId(req) === userId; + + if (isAdmin || isSelfViaApiKey || isSelfViaBearer) return true; + + res.status(403).json({ error: 'You may only act on your own privacy data' }); + return false; +} /** * GET /api/v1/privacy/policy @@ -40,178 +67,252 @@ privacyRouter.get('/policy', (_req: Request, res: Response) => { /** * POST /api/v1/privacy/consent - * Record user consent for privacy policy / marketing / terms. + * Record user consent for privacy policy / marketing / terms against the + * real user_consents table (previously an in-process Map that reset on + * every restart and was never visible to any other request handler). */ -privacyRouter.post('/consent', (req: Request, res: Response) => { +privacyRouter.post('/consent', async (req: Request, res: Response) => { const { user_id, consent_type, policy_version, agreed = true } = req.body; if (!user_id || !consent_type || !policy_version) { return res.status(400).json({ error: 'Missing required fields: user_id, consent_type, policy_version' }); } + if (!authorizeSubject(req, res, user_id)) return; - const record = { - id: `consent-${Date.now()}`, - user_id, - consent_type, - policy_version, - agreed: Boolean(agreed), - agreed_at: new Date().toISOString(), - ip_address: '[REDACTED_IP]', // Anonymized IP - }; - - const userConsents = mockConsentStore.get(user_id) || []; - userConsents.push(record); - mockConsentStore.set(user_id, userConsents); - - res.status(201).json({ - message: 'Consent recorded successfully', - consent: record, - }); + const ip = (req.headers['x-forwarded-for'] as string)?.split(',')[0].trim() ?? req.socket.remoteAddress ?? null; + + try { + const result = await pool.query( + `INSERT INTO user_consents (user_id, consent_type, policy_version, agreed, ip_address) + VALUES ($1, $2, $3, $4, $5) + RETURNING id, user_id, consent_type, policy_version, agreed, agreed_at, revoked_at`, + [user_id, consent_type, policy_version, Boolean(agreed), ip ? encryptColumn(ip) : null], + ); + + res.status(201).json({ + message: 'Consent recorded successfully', + consent: result.rows[0], + }); + } catch (err) { + res.status(500).json({ error: 'Failed to record consent' }); + } }); /** * GET /api/v1/privacy/consent/:userId * Get user consent history. */ -privacyRouter.get('/consent/:userId', (req: Request, res: Response) => { +privacyRouter.get('/consent/:userId', async (req: Request, res: Response) => { const { userId } = req.params; - const consents = mockConsentStore.get(userId as string) || []; - res.json({ user_id: userId, consents }); + if (!authorizeSubject(req, res, userId as string)) return; + + try { + const result = await pool.query( + `SELECT id, user_id, consent_type, policy_version, agreed, agreed_at, revoked_at + FROM user_consents WHERE user_id = $1 ORDER BY agreed_at DESC`, + [userId], + ); + res.json({ user_id: userId, consents: result.rows }); + } catch (err) { + res.status(500).json({ error: 'Failed to fetch consent history' }); + } }); /** * POST /api/v1/privacy/subject-access - * GDPR Subject Access Request (SAR). Export all user personal data with decrypted fields. + * GDPR Subject Access Request (SAR). Exports the real PII-bearing rows for a + * user — notification preferences, KYC status/uploads and consent history — + * decrypting any column-encrypted fields for the export. */ -privacyRouter.post('/subject-access', (req: Request, res: Response) => { +privacyRouter.post('/subject-access', async (req: Request, res: Response) => { const { user_id } = req.body; if (!user_id) { return res.status(400).json({ error: 'Missing user_id parameter' }); } + if (!authorizeSubject(req, res, user_id)) return; - const rawProfile = mockUserProfiles.get(user_id) || { - user_id, - full_name: encryptColumn('Jane Doe'), - email: encryptColumn('jane.doe@example.com'), - phone_number: encryptColumn('+15551234567'), - address: encryptColumn('123 Main St, New York, NY'), - created_at: new Date().toISOString(), - }; + try { + const [prefsResult, kycStatusResult, kycUploadsResult, consentsResult] = await Promise.all([ + pool.query(`SELECT * FROM notification_preferences WHERE user_id = $1`, [user_id]), + pool.query( + `SELECT anchor_id, status, last_checked, expires_at FROM user_kyc_status WHERE user_id = $1`, + [user_id], + ), + pool.query( + `SELECT anchor_id, document_type, file_name, status, created_at FROM kyc_uploads WHERE user_id = $1`, + [user_id], + ), + pool.query( + `SELECT consent_type, policy_version, agreed, agreed_at, revoked_at FROM user_consents WHERE user_id = $1`, + [user_id], + ), + ]); - // Decrypt sensitive PII for data subject export - const decryptedProfile = decryptObject(rawProfile, ['full_name', 'email', 'phone_number', 'address']); - const consents = mockConsentStore.get(user_id) || []; + const rawPrefs = prefsResult.rows[0] ?? null; + const profile = rawPrefs ? decryptObject(rawPrefs, ['email', 'phone']) : null; - res.json({ - user_id, - exported_at: new Date().toISOString(), - profile: decryptedProfile, - consents, - data_retention_info: { - aml_legal_hold: checkAmlLegalHold(new Date()), - }, - }); + const lastActivity = kycStatusResult.rows[0]?.last_checked + ? new Date(kycStatusResult.rows[0].last_checked) + : new Date(0); + + res.json({ + user_id, + exported_at: new Date().toISOString(), + profile, + kyc_status: kycStatusResult.rows, + kyc_uploads: kycUploadsResult.rows, + consents: consentsResult.rows, + data_retention_info: { + aml_legal_hold: checkAmlLegalHold(lastActivity), + }, + }); + } catch (err) { + res.status(500).json({ error: 'Failed to compile subject access export' }); + } }); /** * PUT /api/v1/privacy/rectify - * Rectify user personal data with column-level encryption. + * Rectify user personal data. Email/phone are written to + * notification_preferences with column-level encryption; preferred_locale is + * written to user_profiles. Previously this wrote to an in-process Map that + * no other part of the service — including subject-access above — ever read. */ -privacyRouter.put('/rectify', (req: Request, res: Response) => { - const { user_id, full_name, email, phone_number, address } = req.body; +privacyRouter.put('/rectify', async (req: Request, res: Response) => { + const { user_id, email, phone_number, preferred_locale } = req.body; if (!user_id) { return res.status(400).json({ error: 'Missing user_id parameter' }); } + if (!authorizeSubject(req, res, user_id)) return; - const existingProfile = mockUserProfiles.get(user_id) || { user_id }; + try { + if (email !== undefined || phone_number !== undefined) { + await pool.query( + `INSERT INTO notification_preferences (user_id, email, phone) + VALUES ($1, $2, $3) + ON CONFLICT (user_id) DO UPDATE + SET email = COALESCE($2, notification_preferences.email), + phone = COALESCE($3, notification_preferences.phone), + updated_at = NOW()`, + [user_id, email ? encryptColumn(email) : null, phone_number ? encryptColumn(phone_number) : null], + ); + } - const updatedProfile = { - ...existingProfile, - user_id, - full_name: full_name ? encryptColumn(full_name) : existingProfile.full_name, - email: email ? encryptColumn(email) : existingProfile.email, - phone_number: phone_number ? encryptColumn(phone_number) : existingProfile.phone_number, - address: address ? encryptColumn(address) : existingProfile.address, - updated_at: new Date().toISOString(), - }; + if (preferred_locale) { + await pool.query( + `INSERT INTO user_profiles (user_id, preferred_locale) + VALUES ($1, $2) + ON CONFLICT (user_id) DO UPDATE SET preferred_locale = $2, updated_at = NOW()`, + [user_id, preferred_locale], + ); + } - mockUserProfiles.set(user_id, updatedProfile); - - res.json({ - message: 'Personal data rectified and re-encrypted successfully', - user_id, - updated_fields: Object.keys(req.body).filter(k => k !== 'user_id'), - }); + res.json({ + message: 'Personal data rectified and re-encrypted successfully', + user_id, + updated_fields: Object.keys(req.body).filter((k) => k !== 'user_id'), + }); + } catch (err) { + res.status(500).json({ error: 'Failed to rectify personal data' }); + } }); /** * POST /api/v1/privacy/erasure * Erasure request (Right to be forgotten) with AML Legal Hold Carve-Out. + * Deletes/anonymizes real rows instead of a Map that nothing downstream + * (Postgres, exports, other requests) ever saw. */ -privacyRouter.post('/erasure', (req: Request, res: Response) => { +privacyRouter.post('/erasure', async (req: Request, res: Response) => { const { user_id, account_closed_at, has_financial_transactions = true } = req.body; if (!user_id) { return res.status(400).json({ error: 'Missing user_id parameter' }); } + if (!authorizeSubject(req, res, user_id)) return; const closureDate = account_closed_at ? new Date(account_closed_at) : new Date(); const amlHold = checkAmlLegalHold(closureDate); - if (has_financial_transactions && amlHold.onHold) { - // Carve-out: AML law mandates retention of transaction logs & KYC records - const requestRecord = { - id: `req-${Date.now()}`, - user_id, - request_type: 'erasure', - status: 'legal_hold', - legal_hold_carveout: true, - erased_items: ['notification_preferences', 'marketing_consents', 'unverified_uploads'], - retained_items: ['transaction_audit_logs', 'verified_kyc_status'], - legal_hold_expiry: amlHold.releaseDate.toISOString(), - requested_at: new Date().toISOString(), - }; - - const userReqs = mockPrivacyRequests.get(user_id) || []; - userReqs.push(requestRecord); - mockPrivacyRequests.set(user_id, userReqs); - - // Erase non-essential data (marketing / profile preferences) - mockConsentStore.delete(user_id); - - return res.status(200).json({ - message: 'Erasure request processed with AML Legal Hold carve-out.', - status: 'legal_hold', - legal_hold_carveout: true, - details: amlHold.reason, - legal_hold_expiry: amlHold.releaseDate.toISOString(), - erased_categories: ['notification_preferences', 'marketing_consents', 'unverified_uploads'], - retained_categories_under_legal_hold: ['transaction_audit_logs', 'verified_kyc_status'], - }); - } + try { + if (has_financial_transactions && amlHold.onHold) { + // Carve-out: AML law mandates retention of transaction logs & verified KYC. + await pool.query(`DELETE FROM notification_preferences WHERE user_id = $1`, [user_id]); + await pool.query( + `UPDATE user_consents SET revoked_at = NOW() WHERE user_id = $1 AND revoked_at IS NULL`, + [user_id], + ); + await pool.query( + `DELETE FROM kyc_uploads WHERE user_id = $1 AND status IN ('pending', 'failed')`, + [user_id], + ); - // Full erasure if no legal hold applies - mockUserProfiles.delete(user_id); - mockConsentStore.delete(user_id); + await pool.query( + `INSERT INTO privacy_requests (user_id, request_type, status, details, legal_hold_carveout) + VALUES ($1, 'erasure', 'legal_hold', $2, true)`, + [ + user_id, + JSON.stringify({ + erased: ['notification_preferences', 'marketing_consents', 'unverified_uploads'], + retained: ['transaction_audit_logs', 'verified_kyc_status'], + legal_hold_expiry: amlHold.releaseDate, + }), + ], + ); - res.status(200).json({ - message: 'User personal data fully erased.', - status: 'completed', - legal_hold_carveout: false, - erased_at: new Date().toISOString(), - }); + return res.status(200).json({ + message: 'Erasure request processed with AML Legal Hold carve-out.', + status: 'legal_hold', + legal_hold_carveout: true, + details: amlHold.reason, + legal_hold_expiry: amlHold.releaseDate.toISOString(), + erased_categories: ['notification_preferences', 'marketing_consents', 'unverified_uploads'], + retained_categories_under_legal_hold: ['transaction_audit_logs', 'verified_kyc_status'], + }); + } + + // Full erasure if no legal hold applies + await pool.query(`DELETE FROM notification_preferences WHERE user_id = $1`, [user_id]); + await pool.query(`DELETE FROM user_consents WHERE user_id = $1`, [user_id]); + await pool.query(`DELETE FROM kyc_uploads WHERE user_id = $1`, [user_id]); + await pool.query( + `INSERT INTO privacy_requests (user_id, request_type, status, completed_at) + VALUES ($1, 'erasure', 'completed', NOW())`, + [user_id], + ); + + res.status(200).json({ + message: 'User personal data fully erased.', + status: 'completed', + legal_hold_carveout: false, + erased_at: new Date().toISOString(), + }); + } catch (err) { + res.status(500).json({ error: 'Failed to process erasure request' }); + } }); /** * POST /api/v1/privacy/purge-expired - * Trigger manual or scheduled purge of expired data. + * Trigger manual purge of expired data. Now passes the real pg.Pool so + * purgeExpiredPersonalData actually deletes rows instead of returning an + * all-zero report — see privacy/retention-service.ts. Admin-scoped only. */ -privacyRouter.post('/purge-expired', async (_req: Request, res: Response) => { - const report = await purgeExpiredPersonalData(); - res.json({ - message: 'Automated privacy data purge executed', - report, - }); +privacyRouter.post('/purge-expired', async (req: Request, res: Response) => { + const apiKey = (req as any).apiKey as { scopes?: string[] } | undefined; + if (!apiKey?.scopes?.includes('admin:*')) { + return res.status(403).json({ error: 'Requires an admin:* scoped API key' }); + } + + try { + const report = await purgeExpiredPersonalData(pool); + res.json({ + message: 'Automated privacy data purge executed', + report, + }); + } catch (err) { + res.status(500).json({ error: 'Failed to execute privacy data purge' }); + } }); diff --git a/backend/src/scheduler.ts b/backend/src/scheduler.ts index ceb3de21..e90b7321 100644 --- a/backend/src/scheduler.ts +++ b/backend/src/scheduler.ts @@ -18,6 +18,7 @@ import { AdminAuditLogService } from './admin-audit-log'; import { SanctionsScreeningService } from './aml/sanctions-screening'; import { TravelRuleService } from './aml/travel-rule'; import { RetentionService } from './aml/retention'; +import { purgeExpiredPersonalData } from './privacy/retention-service'; import { createLogger } from './correlation-id'; import crypto from 'crypto'; @@ -135,6 +136,18 @@ export async function startBackgroundJobs() { if (!ran) console.log('aml-data-retention: skipped (another instance holds the lock)'); }); + // Purge/anonymize expired GDPR personal data nightly at 03:45 UTC (SR-131). + // Previously this only ran via a manual POST /api/v1/privacy/purge-expired + // call that itself never received a live pool, so audit-log IP + // anonymization, transient KYC upload purge, and revoked-consent purge + // never executed in any environment. + cron.schedule('45 3 * * *', async () => { + const ran = await withAdvisoryLock(pool, 'privacy-data-purge', async () => { + await runTracked(pool, 'privacy-data-purge', purgeExpiredData); + }); + if (!ran) console.log('privacy-data-purge: skipped (another instance holds the lock)'); + }); + console.log('Background jobs scheduled'); } @@ -176,6 +189,17 @@ async function enforceDataRetention() { } } +async function purgeExpiredData() { + const report = await purgeExpiredPersonalData(pool); + if (report.errors.length > 0) { + for (const error of report.errors) console.error(`privacy-data-purge: ${error}`); + } + console.log( + `privacy-data-purge: audit_ips_anonymized=${report.auditLogsAnonymized} ` + + `transient_kyc_purged=${report.transientKycPurged} revoked_consents_purged=${report.revokedConsentsPurged}`, + ); +} + async function revalidateStaleAssets() { try { const hoursOld = parseInt(process.env.VERIFICATION_INTERVAL_HOURS || '24'); From eafbb5e620535c3edbf370b6c30488fe5f477a56 Mon Sep 17 00:00:00 2001 From: james2177 Date: Fri, 28 Aug 2026 07:42:17 +0100 Subject: [PATCH 3/4] fix(security): fail closed instead of a public default encryption key (SR-131) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit encryption.ts's getMasterKey() silently fell back to DEFAULT_KEY_HEX — a literal, publicly-visible constant committed to the repo — whenever ENCRYPTION_KEY was unset, in every environment including production, with no guard at all. encryptColumn/decryptColumn/encryptObject/ decryptObject back the GDPR privacy routes' "encryption" of full_name, email, phone_number and address; a misconfigured deploy would produce ciphertext anyone who had read this source file already had the key for, while application code and logs kept treating it as protected. This also bypassed the shared SecretsManager that already centralizes sourcing/ rotation checks for the JWT secret, DB URL and admin key. - Remove DEFAULT_KEY_HEX. getMasterKey() now throws immediately outside NODE_ENV=test when ENCRYPTION_KEY is absent, mirroring env-guard.ts's fail-fast treatment of other required secrets. - Add getEncryptionKey() to the shared SecretsManager (optional secret, same rotation/validation posture as FX_API_KEY) and resolve it in backend/src/index.ts's loadSecrets(), writing it into process.env so the synchronous encryption module keeps working without an invasive async refactor of every call site. - Add a test asserting encryptColumn/decryptColumn throw rather than silently encrypting with a known key when ENCRYPTION_KEY is unset outside test, and that a configured key still works in production. --- .../__tests__/encryption-fail-closed.test.ts | 56 +++++++++++++ backend/src/index.ts | 9 +- .../privacy/README-encryption-fail-closed.md | 84 +++++++++++++++++++ backend/src/privacy/encryption.ts | 30 ++++++- backend/src/secrets-manager.ts | 1 + shared/src/secrets-manager.ts | 13 +++ 6 files changed, 188 insertions(+), 5 deletions(-) create mode 100644 backend/src/__tests__/encryption-fail-closed.test.ts create mode 100644 backend/src/privacy/README-encryption-fail-closed.md diff --git a/backend/src/__tests__/encryption-fail-closed.test.ts b/backend/src/__tests__/encryption-fail-closed.test.ts new file mode 100644 index 00000000..8e96b3c3 --- /dev/null +++ b/backend/src/__tests__/encryption-fail-closed.test.ts @@ -0,0 +1,56 @@ +import { describe, it, expect, afterEach } from 'vitest'; + +/** + * SR-131 — encryption.ts must fail closed when ENCRYPTION_KEY is unset. + * + * Before this fix, getMasterKey() silently returned a literal, publicly + * visible constant (DEFAULT_KEY_HEX) committed to the repository whenever + * ENCRYPTION_KEY was missing, in every environment including production — + * "encrypted" PII would be ciphertext anyone reading the source already had + * the key for. Import is deferred to a helper inside each test so the module + * (and its NODE_ENV check) is re-evaluated per env var combination — Node's + * module cache would otherwise serve the first import's behaviour. + */ +async function freshEncryptionModule() { + const modPath = '../privacy/encryption'; + const resolved = require.resolve(modPath); + delete require.cache[resolved]; + return require(modPath) as typeof import('../privacy/encryption'); +} + +describe('privacy/encryption — fail closed without ENCRYPTION_KEY', () => { + const originalKey = process.env.ENCRYPTION_KEY; + const originalEnv = process.env.NODE_ENV; + + afterEach(() => { + if (originalKey === undefined) delete process.env.ENCRYPTION_KEY; + else process.env.ENCRYPTION_KEY = originalKey; + process.env.NODE_ENV = originalEnv; + }); + + it('throws instead of silently using a known default key outside test env', async () => { + delete process.env.ENCRYPTION_KEY; + process.env.NODE_ENV = 'production'; + const { encryptColumn } = await freshEncryptionModule(); + + expect(() => encryptColumn('sensitive-value')).toThrow(/ENCRYPTION_KEY is not set/); + }); + + it('throws on decryptColumn too, not just encryptColumn', async () => { + delete process.env.ENCRYPTION_KEY; + process.env.NODE_ENV = 'development'; + const { decryptColumn } = await freshEncryptionModule(); + + expect(() => decryptColumn('enc:v1:aa:bb:cc')).toThrow(/ENCRYPTION_KEY is not set/); + }); + + it('does not throw when ENCRYPTION_KEY is set, regardless of environment', async () => { + process.env.ENCRYPTION_KEY = 'a'.repeat(64); + process.env.NODE_ENV = 'production'; + const { encryptColumn, decryptColumn, isEncrypted } = await freshEncryptionModule(); + + const encrypted = encryptColumn('sensitive-value')!; + expect(isEncrypted(encrypted)).toBe(true); + expect(decryptColumn(encrypted)).toBe('sensitive-value'); + }); +}); diff --git a/backend/src/index.ts b/backend/src/index.ts index d1b08a12..3f3219c7 100644 --- a/backend/src/index.ts +++ b/backend/src/index.ts @@ -11,7 +11,7 @@ import { WebhookHandler } from './webhook-handler'; import { KycService } from './kyc-service'; import { createWebhookVerificationMiddleware } from './webhook-middleware'; import { patchConsoleForProduction } from './console-shim'; -import { getSecretsManager, getDatabaseUrl, getAdminSecretKey, getContractId, initializeSecretRotation } from './secrets-manager'; +import { getSecretsManager, getDatabaseUrl, getAdminSecretKey, getContractId, getEncryptionKey, initializeSecretRotation } from './secrets-manager'; import { assertEnvConfigured } from './env-guard'; dotenv.config(); @@ -53,10 +53,14 @@ async function loadSecrets(): Promise { // Resolve all required secrets — throws immediately if any are missing or // (in production) fall back to a plaintext environment variable. - const [databaseUrl, adminSecretKey, contractId] = await Promise.all([ + const [databaseUrl, adminSecretKey, contractId, encryptionKey] = await Promise.all([ getDatabaseUrl(), getAdminSecretKey(), getContractId(), + // Optional here: encryption.ts itself fails closed outside NODE_ENV=test + // when ENCRYPTION_KEY is unset (SR-131), so this only upgrades the + // sourcing/rotation posture rather than gating startup a second time. + getEncryptionKey(), ]); // Write resolved values back into process.env so legacy code that reads @@ -65,6 +69,7 @@ async function loadSecrets(): Promise { process.env.DATABASE_URL = databaseUrl; process.env.ADMIN_SECRET_KEY = adminSecretKey; process.env.CONTRACT_ID = contractId; + if (encryptionKey) process.env.ENCRYPTION_KEY = encryptionKey; // JWT_SECRET is used via getJwtSecret() at call sites; no env write needed. diff --git a/backend/src/privacy/README-encryption-fail-closed.md b/backend/src/privacy/README-encryption-fail-closed.md new file mode 100644 index 00000000..7c6660b6 --- /dev/null +++ b/backend/src/privacy/README-encryption-fail-closed.md @@ -0,0 +1,84 @@ +# SR-131 — encryption.ts fails closed without ENCRYPTION_KEY + +## Problem + +`getMasterKey()` in `encryption.ts` read `process.env.ENCRYPTION_KEY` and, if +unset, silently fell back to `DEFAULT_KEY_HEX` — a literal hex constant +committed to the repository, visible to anyone who could read this source +file. There was no environment guard: this happened identically in +development, staging, and production. `encryptColumn` / `decryptColumn` / +`encryptObject` / `decryptObject` are the functions the GDPR privacy routes +use to "encrypt" PII (`full_name`, `email`, `phone_number`, `address`) before +returning it in subject-access/rectify responses. A misconfigured deploy — +a forgotten secret, a hastily-provisioned new environment — would encrypt +every one of those columns with a key that provided zero real +confidentiality, while the rest of the application and its logs continued to +treat the data as protected. + +This also bypassed the shared `SecretsManager` +(`shared/src/secrets-manager.ts`), which already centralizes secret +sourcing, rotation, and a "no plaintext env fallback in production" guard for +`JWT_SECRET`, `DATABASE_URL`, and `ADMIN_SECRET_KEY`. `ENCRYPTION_KEY` had no +equivalent protection. + +## What changed + +- **`backend/src/privacy/encryption.ts`** — removed `DEFAULT_KEY_HEX` + entirely. `getMasterKey()` now: + - Returns the configured `ENCRYPTION_KEY` when set (unchanged behavior). + - Returns a deterministic, non-configurable key **only** when + `NODE_ENV === 'test'`, so the existing test suite doesn't need to set + `ENCRYPTION_KEY` everywhere it exercises encryption. + - **Throws** in every other case (`development`, `staging`, `production`, + or any unset `NODE_ENV`), with a message telling the operator exactly + what to set. This mirrors how `env-guard.ts` already fails startup for + other required configuration, and how `backend/src/index.ts` fails fast + when the secrets manager isn't configured in production. + +- **`shared/src/secrets-manager.ts`** — added `getEncryptionKey()`, an + optional secret resolved the same way as `getFxApiKey()`. Routing + `ENCRYPTION_KEY` through `SecretsManager.getSecret()` means that if this + secret ever *is* read from a plaintext env var while `NODE_ENV=production`, + the existing `PRODUCTION SECURITY VIOLATION` guard in `getSecret` applies + to it too, and it participates in the same TTL cache / rotation hooks as + the other named secrets. + +- **`backend/src/secrets-manager.ts`** — re-exports `getEncryptionKey` from + the shared module (this file is a thin re-export shim; no logic lives + here). + +- **`backend/src/index.ts`** — `loadSecrets()` now resolves + `getEncryptionKey()` alongside the other startup secrets and, if a value + came back, writes it to `process.env.ENCRYPTION_KEY`. This keeps + `encryption.ts`'s synchronous API (`encryptColumn`, `decryptColumn`, etc. + are called synchronously throughout the request path) working without an + invasive async refactor of every call site — the same pattern already used + for `DATABASE_URL`, `ADMIN_SECRET_KEY`, and `CONTRACT_ID` in this function. + `getEncryptionKey()` is optional at this layer specifically because + `encryption.ts` itself is the actual enforcement point: if no key comes + back from the secrets manager and none is already in the environment, the + first call to `encryptColumn`/`decryptColumn` throws. + +- **`backend/src/__tests__/encryption-fail-closed.test.ts`** (new) — + asserts: + 1. `encryptColumn` throws when `ENCRYPTION_KEY` is unset and + `NODE_ENV=production`. + 2. `decryptColumn` throws under the same conditions (not just the encrypt + path). + 3. A configured `ENCRYPTION_KEY` still works correctly in production + (round-trips a value through encrypt/decrypt). + + The test reloads the module (via `require.cache` invalidation) between + cases because `getMasterKey()`'s `NODE_ENV` check is only observed once + per process unless the module is freshly evaluated. + +## What was intentionally left out + +Full end-to-end routing of `ENCRYPTION_KEY` through AWS Secrets +Manager / Vault in a live deployment was not verified against real +infrastructure — that requires credentials this change doesn't have access +to. The code path added here (`getEncryptionKey()` → +`process.env.ENCRYPTION_KEY` → `getMasterKey()`) follows the exact pattern +already used and presumably already verified for `DATABASE_URL` / +`ADMIN_SECRET_KEY` / `CONTRACT_ID` in the same function, so no new +infrastructure assumptions were introduced. diff --git a/backend/src/privacy/encryption.ts b/backend/src/privacy/encryption.ts index cd7e4770..9b769779 100644 --- a/backend/src/privacy/encryption.ts +++ b/backend/src/privacy/encryption.ts @@ -4,9 +4,25 @@ const ALGORITHM = 'aes-256-gcm'; const IV_LENGTH = 12; // 96 bits for GCM const PREFIX = 'enc:v1:'; -// Default key for development/test environment if ENCRYPTION_KEY is not set -const DEFAULT_KEY_HEX = '00112233445566778899aabbccddeeff00112233445566778899aabbccddeeff'; +/** + * Derived deterministically at module load so tests never need to set + * ENCRYPTION_KEY, without shipping a literal, publicly-visible key that + * every column would silently fall back to in a misconfigured deployment + * (see SR-131). Never used outside NODE_ENV=test. + */ +const TEST_ONLY_KEY = crypto.createHash('sha256').update('swiftremit-test-fixture-key-do-not-use').digest(); +/** + * Resolve the AES-256 master key from ENCRYPTION_KEY. + * + * There is no fallback key outside test. Before this change, an unset + * ENCRYPTION_KEY silently fell back to a literal hex constant committed to + * this file — anyone who had read the source already knew the "encryption" + * key, so a misconfigured deploy would encrypt every PII column with zero + * real confidentiality while application code treated it as protected. + * Failing closed here mirrors env-guard.ts's treatment of other required + * secrets and backend/src/index.ts's fail-fast startup behaviour. + */ function getMasterKey(): Buffer { const envKey = process.env.ENCRYPTION_KEY; if (envKey) { @@ -16,7 +32,15 @@ function getMasterKey(): Buffer { // Hash key if it's arbitrary string length return crypto.createHash('sha256').update(envKey).digest(); } - return Buffer.from(DEFAULT_KEY_HEX, 'hex'); + + if (process.env.NODE_ENV === 'test') { + return TEST_ONLY_KEY; + } + + throw new Error( + 'ENCRYPTION_KEY is not set. Refusing to encrypt/decrypt with a known fallback key — ' + + 'set ENCRYPTION_KEY (64 hex chars, sourced from the secrets manager) before starting this service.', + ); } /** diff --git a/backend/src/secrets-manager.ts b/backend/src/secrets-manager.ts index a8ba69af..1a03d1b9 100644 --- a/backend/src/secrets-manager.ts +++ b/backend/src/secrets-manager.ts @@ -14,6 +14,7 @@ export { getContractId, getFxApiKey, getAnchorsAdminApiKey, + getEncryptionKey, initializeSecretRotation, } from '../../shared/src/secrets-manager'; diff --git a/shared/src/secrets-manager.ts b/shared/src/secrets-manager.ts index a685ae06..a7edec59 100644 --- a/shared/src/secrets-manager.ts +++ b/shared/src/secrets-manager.ts @@ -422,6 +422,19 @@ export async function getAnchorsAdminApiKey(): Promise { return getSecretsManager().getSecret({ secretId: 'ANCHORS_ADMIN_API_KEY', required: false }); } +/** + * ENCRYPTION_KEY — AES-256 master key backing column-level PII encryption + * (backend/src/privacy/encryption.ts). Optional here because the encryption + * module itself fails closed outside NODE_ENV=test when it is missing (see + * SR-131); routing it through the same rotation/validation path as the other + * secrets means a plaintext env fallback in production still throws via + * getSecret's PRODUCTION SECURITY VIOLATION guard rather than degrading + * silently. + */ +export async function getEncryptionKey(): Promise { + return getSecretsManager().getSecret({ secretId: 'ENCRYPTION_KEY', required: false }); +} + // ── Named helpers — api service ─────────────────────────────────────────────── export async function getAdminApiKey(): Promise { From 5128e3b132e7f4a474ef2fc6acb7c4bef89909e9 Mon Sep 17 00:00:00 2001 From: james2177 Date: Fri, 28 Aug 2026 07:45:06 +0100 Subject: [PATCH 4/4] fix(security): enforce HMAC signature verification on the SEP-12 KYC webhook (SR-131) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit verifyAnchorSignature existed but was never called anywhere, and even if it had been, it only checked the caller-controlled anchor_id URL parameter against an allowlist — no HMAC/signature validation at all. Anyone on the internet could POST {"user_id": "...", "status": "APPROVED"} to /webhooks/kyc/:anchor_id and flip a user's KYC status, which feeds setKycApprovedOnChain (stellar-kyc.ts): an unauthenticated path to on-chain KYC approval. SR-045 hardened ramp-webhook-handler.ts but never touched this SEP-12 path. - Rewrite verifyAnchorSignature to compute HMAC-SHA256 over "${timestamp}.${rawBody}" using a per-anchor WEBHOOK_SECRET_{ANCHOR_ID} secret (already documented in .env.example but unused), compared with crypto.timingSafeEqual, mirroring ramp-provider.ts's verifyWebhook. - Enforce a 5-minute timestamp window and an in-memory nonce replay cache (bounded, self-pruning), consistent with SR-045's treatment of ramp provider webhooks. - handleKycWebhook now verifies the signature before doing anything else and rejects with 401 (with a machine-readable reason) on any failure — missing/invalid signature, missing/stale timestamp, wrong anchor secret, or replayed nonce — never reaching saveUserKycStatus. - Add a `verify` callback to the global express.json() in api.ts so handlers can sign/verify against the exact request bytes rather than a re-serialization of the parsed body. - Rewrite the webhook handler test suite: valid-signature happy path plus regression tests asserting unsigned, mis-signed, wrong-anchor, and replayed requests are all rejected with 401 and never call saveUserKycStatus. - Update .env.example: document the WEBHOOK_SECRET_ requirement and remove the now-dead TRUSTED_ANCHOR_IDS-only guidance. --- backend/.env.example | 15 +- .../src/__tests__/kyc-webhook-handler.test.ts | 254 ++++++++++-------- backend/src/api.ts | 10 +- backend/src/kyc-webhook-handler.ts | 131 ++++++++- 4 files changed, 280 insertions(+), 130 deletions(-) diff --git a/backend/.env.example b/backend/.env.example index fba54915..05ed353c 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -14,6 +14,8 @@ AWS_REGION=us-east-1 # - swiftremit/FX_API_KEY (optional) # - swiftremit/JWT_SECRET (for API service) # - swiftremit/WEBHOOK_SECRET_{ANCHOR_ID} (per anchor) +# - swiftremit/ENCRYPTION_KEY (optional; required outside NODE_ENV=test — +# see backend/src/privacy/encryption.ts, SR-131) # PostgreSQL Connection Pool Tuning DB_POOL_MAX=20 @@ -66,9 +68,16 @@ WEBHOOK_RETRY_BASE_MS=1000 WEBHOOK_RETRY_MAX_MS=300000 WEBHOOK_RETRY_JITTER_PERCENT=20 -# SEP-12 KYC Webhook Configuration -# Comma-separated list of anchor IDs that are trusted to send KYC webhooks -TRUSTED_ANCHOR_IDS=moneygram,circle,nexo +# SEP-12 KYC Webhook Configuration (SR-131) +# Each anchor's webhook requests must carry an HMAC-SHA256 signature (header +# x-webhook-signature, computed over "${x-webhook-timestamp}.${rawBody}") +# verified against a per-anchor secret named WEBHOOK_SECRET_ +# (anchor id uppercased, non-alphanumeric characters replaced with `_`). +# TRUSTED_ANCHOR_IDS is no longer read — anchor_id alone was never a real +# authentication check, since it is a caller-controlled URL parameter. +# WEBHOOK_SECRET_MONEYGRAM=changeme +# WEBHOOK_SECRET_CIRCLE=changeme +# WEBHOOK_SECRET_NEXO=changeme # KYC Re-verification (#862) KYC_RENEWAL_BASE_URL=https://app.swiftremit.io/kyc/renew diff --git a/backend/src/__tests__/kyc-webhook-handler.test.ts b/backend/src/__tests__/kyc-webhook-handler.test.ts index f1735984..12e791f9 100644 --- a/backend/src/__tests__/kyc-webhook-handler.test.ts +++ b/backend/src/__tests__/kyc-webhook-handler.test.ts @@ -1,145 +1,165 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import crypto from 'crypto'; + +vi.mock('../database', () => ({ saveUserKycStatus: vi.fn().mockResolvedValue(undefined) })); + import { handleKycWebhook, mapKycStatus, verifyAnchorSignature, KycWebhookPayload } from '../kyc-webhook-handler'; +import { saveUserKycStatus } from '../database'; + +const ANCHOR_ID = 'moneygram'; +const SECRET_ENV_VAR = 'WEBHOOK_SECRET_MONEYGRAM'; +const SECRET = 'test-anchor-shared-secret'; + +function sign(body: string, timestamp: string): string { + return crypto.createHmac('sha256', SECRET).update(`${timestamp}.${body}`).digest('hex'); +} + +function makeRequest(overrides: Partial = {}) { + const body = overrides.body ?? { user_id: 'user123', status: 'APPROVED' }; + const rawBody = JSON.stringify(body); + const timestamp = overrides.timestamp ?? String(Date.now()); + const signature = overrides.signature ?? sign(rawBody, timestamp); + + return { + params: { anchor_id: overrides.anchorId ?? ANCHOR_ID }, + body, + rawBody, + headers: { + 'x-webhook-signature': overrides.omitSignature ? undefined : signature, + 'x-webhook-timestamp': overrides.omitTimestamp ? undefined : timestamp, + 'x-webhook-nonce': overrides.nonce, + ...overrides.headers, + }, + ...overrides.requestOverrides, + }; +} + +function makeResponse() { + return { + status: vi.fn().mockReturnThis(), + json: vi.fn().mockReturnThis(), + }; +} describe('KYC Webhook Handler', () => { - let mockRequest: any; - let mockResponse: any; - beforeEach(() => { - mockRequest = { - params: { anchor_id: 'moneygram' }, - body: {} as KycWebhookPayload, - }; - - mockResponse = { - status: vi.fn().mockReturnThis(), - json: vi.fn().mockReturnThis(), - }; - - process.env.TRUSTED_ANCHOR_IDS = 'moneygram,circle,nexo'; - }); - - it('should map KYC status correctly', () => { - expect(mapKycStatus('APPROVED')).toBe('approved'); - expect(mapKycStatus('REJECTED')).toBe('rejected'); - expect(mapKycStatus('PENDING')).toBe('pending'); - expect(mapKycStatus('NEEDS_INFO')).toBe('needs_info'); + process.env[SECRET_ENV_VAR] = SECRET; + vi.mocked(saveUserKycStatus).mockClear(); }); - it('should handle case-insensitive status mapping', () => { - expect(mapKycStatus('approved')).toBe('approved'); - expect(mapKycStatus('PENDING')).toBe('pending'); - expect(mapKycStatus('Needs_Info')).toBe('needs_info'); + afterEach(() => { + delete process.env[SECRET_ENV_VAR]; }); - it('should default to pending for unknown status', () => { - expect(mapKycStatus('UNKNOWN')).toBe('pending'); + describe('mapKycStatus', () => { + it('maps known SEP-12 statuses', () => { + expect(mapKycStatus('APPROVED')).toBe('approved'); + expect(mapKycStatus('REJECTED')).toBe('rejected'); + expect(mapKycStatus('PENDING')).toBe('pending'); + expect(mapKycStatus('NEEDS_INFO')).toBe('needs_info'); + }); + + it('is case-insensitive and defaults unknown values to pending', () => { + expect(mapKycStatus('approved')).toBe('approved'); + expect(mapKycStatus('UNKNOWN')).toBe('pending'); + }); }); - it('should verify trusted anchor signatures', () => { - expect(verifyAnchorSignature('moneygram')).toBe(true); - expect(verifyAnchorSignature('circle')).toBe(true); - expect(verifyAnchorSignature('untrusted')).toBe(false); + describe('verifyAnchorSignature', () => { + it('accepts a correctly signed, fresh request', () => { + const rawBody = JSON.stringify({ user_id: 'u1', status: 'APPROVED' }); + const timestamp = String(Date.now()); + const signature = sign(rawBody, timestamp); + + const result = verifyAnchorSignature(ANCHOR_ID, rawBody, signature, timestamp); + expect(result.ok).toBe(true); + }); + + it('rejects when no secret is configured for the anchor', () => { + const result = verifyAnchorSignature('unconfigured-anchor', 'body', 'sig', String(Date.now())); + expect(result.ok).toBe(false); + }); + + it('rejects a tampered body (signature no longer matches)', () => { + const timestamp = String(Date.now()); + const signature = sign(JSON.stringify({ user_id: 'u1', status: 'APPROVED' }), timestamp); + const tamperedBody = JSON.stringify({ user_id: 'u1', status: 'REJECTED' }); + + const result = verifyAnchorSignature(ANCHOR_ID, tamperedBody, signature, timestamp); + expect(result.ok).toBe(false); + }); + + it('rejects a stale timestamp outside the acceptable window', () => { + const rawBody = JSON.stringify({ user_id: 'u1', status: 'APPROVED' }); + const staleTimestamp = String(Date.now() - 10 * 60 * 1000); // 10 minutes ago + const signature = sign(rawBody, staleTimestamp); + + const result = verifyAnchorSignature(ANCHOR_ID, rawBody, signature, staleTimestamp); + expect(result.ok).toBe(false); + }); + + it('rejects a replayed nonce', () => { + const rawBody = JSON.stringify({ user_id: 'u1', status: 'APPROVED' }); + const timestamp = String(Date.now()); + const signature = sign(rawBody, timestamp); + const nonce = 'nonce-1'; + + expect(verifyAnchorSignature(ANCHOR_ID, rawBody, signature, timestamp, nonce).ok).toBe(true); + expect(verifyAnchorSignature(ANCHOR_ID, rawBody, signature, timestamp, nonce).ok).toBe(false); + }); }); - it('should reject webhook without user_id or external_id', async () => { - mockRequest.body = { status: 'APPROVED' }; - - await handleKycWebhook(mockRequest, mockResponse); + describe('handleKycWebhook — signature enforcement (SR-131 regression)', () => { + it('rejects an unsigned request with 401 and does not touch the database', async () => { + const req = makeRequest({ omitSignature: true }); + const res = makeResponse(); - expect(mockResponse.status).toHaveBeenCalledWith(400); - expect(mockResponse.json).toHaveBeenCalledWith( - expect.objectContaining({ error: expect.any(String) }) - ); - }); - - it('should accept webhook with user_id', async () => { - mockRequest.body = { - user_id: 'user123', - status: 'APPROVED', - timestamp: Math.floor(Date.now() / 1000), - }; + await handleKycWebhook(req as any, res as any); - // Mock database save - vi.doMock('../database', () => ({ - saveUserKycStatus: vi.fn().mockResolvedValue({}), - })); + expect(res.status).toHaveBeenCalledWith(401); + expect(saveUserKycStatus).not.toHaveBeenCalled(); + }); - await handleKycWebhook(mockRequest, mockResponse); + it('rejects a mis-signed request with 401 and does not touch the database', async () => { + const req = makeRequest({ signature: 'deadbeef'.repeat(8) }); + const res = makeResponse(); - // Should attempt to save status (may fail due to mock, but status should be called) - expect(mockResponse.status).toHaveBeenCalled(); - }); + await handleKycWebhook(req as any, res as any); - it('should accept webhook with external_id', async () => { - mockRequest.body = { - external_id: 'ext456', - status: 'PENDING', - }; + expect(res.status).toHaveBeenCalledWith(401); + expect(saveUserKycStatus).not.toHaveBeenCalled(); + }); - await handleKycWebhook(mockRequest, mockResponse); + it('rejects a request signed for a different anchor', async () => { + const req = makeRequest({ anchorId: 'a-different-anchor' }); + const res = makeResponse(); - expect(mockResponse.status).toHaveBeenCalled(); - }); + await handleKycWebhook(req as any, res as any); - it('should use timestamp from payload if provided', () => { - const payload: KycWebhookPayload = { - user_id: 'user123', - status: 'APPROVED', - timestamp: 1704067200, // 2024-01-01 00:00:00 UTC - }; + expect(res.status).toHaveBeenCalledWith(401); + expect(saveUserKycStatus).not.toHaveBeenCalled(); + }); - const expectedDate = new Date(1704067200 * 1000); - expect(expectedDate.getTime()).toBe(1704067200000); - }); + it('accepts a correctly signed request and updates KYC status', async () => { + const req = makeRequest(); + const res = makeResponse(); - it('should use current time if timestamp not provided', () => { - const before = Date.now(); - const payload: KycWebhookPayload = { - user_id: 'user123', - status: 'APPROVED', - }; - const after = Date.now(); + await handleKycWebhook(req as any, res as any); - // Payload would use Date.now() internally - expect(before).toBeLessThanOrEqual(after); - }); + expect(res.status).toHaveBeenCalledWith(200); + expect(saveUserKycStatus).toHaveBeenCalledWith( + expect.objectContaining({ user_id: 'user123', anchor_id: ANCHOR_ID, status: 'approved' }), + ); + }); - it('should handle all SEP-12 status values', () => { - const sep12Statuses = ['APPROVED', 'REJECTED', 'PENDING', 'NEEDS_INFO']; - - for (const status of sep12Statuses) { - const mapped = mapKycStatus(status); - expect(['approved', 'rejected', 'pending', 'needs_info']).toContain(mapped); - } - }); + it('still validates the body after signature verification passes', async () => { + const req = makeRequest({ body: { status: 'APPROVED' } }); // no user_id/external_id + const res = makeResponse(); - it('should support additional webhook payload fields', () => { - const payload: KycWebhookPayload = { - user_id: 'user123', - status: 'APPROVED', - metadata: { country: 'US' }, - requested_by: 'admin@anchor.com', - review_date: '2026-01-15', - }; - - expect(payload.metadata).toBeDefined(); - expect(payload.requested_by).toBeDefined(); - expect(payload.review_date).toBeDefined(); - }); + await handleKycWebhook(req as any, res as any); - it('should use anchor_id from URL parameters', async () => { - const anchors = ['moneygram', 'circle', 'nexo']; - - for (const anchorId of anchors) { - mockRequest.params.anchor_id = anchorId; - mockRequest.body = { - user_id: `user-${anchorId}`, - status: 'APPROVED', - }; - - // Verify anchor_id is captured - expect(mockRequest.params.anchor_id).toBe(anchorId); - } + expect(res.status).toHaveBeenCalledWith(400); + expect(saveUserKycStatus).not.toHaveBeenCalled(); + }); }); }); diff --git a/backend/src/api.ts b/backend/src/api.ts index 03a05cec..66939abf 100644 --- a/backend/src/api.ts +++ b/backend/src/api.ts @@ -127,7 +127,15 @@ setFxCircuitObserver((provider, open) => { app.use(helmet()); app.use(cors()); -app.use(express.json()); +// Capture the raw request body alongside the parsed JSON so webhook handlers +// (kyc-webhook-handler.ts, ramp-webhook-handler.ts) can verify an HMAC +// signature computed over the exact bytes the sender signed, rather than a +// re-serialization of the parsed object which is not guaranteed to match. +app.use(express.json({ + verify: (req: Request, _res: Response, buf: Buffer) => { + (req as any).rawBody = buf.toString('utf8'); + }, +})); app.use(correlationIdMiddleware); // Request instrumentation (SR-104) — feeds the API availability and latency diff --git a/backend/src/kyc-webhook-handler.ts b/backend/src/kyc-webhook-handler.ts index f4dccde9..07b94493 100644 --- a/backend/src/kyc-webhook-handler.ts +++ b/backend/src/kyc-webhook-handler.ts @@ -1,16 +1,46 @@ /** * SEP-12 KYC Status Webhook Handler - * + * * Receives push notifications from anchors when KYC status changes. * Reduces polling load by allowing anchors to push status updates. + * + * SR-131: this endpoint previously accepted any POST body with zero + * authentication — verifyAnchorSignature existed but was never called, and + * even if it had been, it only checked the caller-controlled anchor_id path + * parameter against an allowlist, not a real signature. Anyone on the + * internet could flip a user's KYC status, which feeds setKycApprovedOnChain + * (stellar-kyc.ts). This mirrors the HMAC + timestamp/nonce replay + * protection already applied to ramp-webhook-handler.ts under SR-045. */ import { Request, Response } from 'express'; +import crypto from 'crypto'; import { saveUserKycStatus } from './database'; import { createLogger } from './correlation-id'; const logger = createLogger('kyc-webhook'); +/** Reject webhooks whose timestamp is further than this from "now". */ +const MAX_TIMESTAMP_SKEW_MS = 5 * 60 * 1000; // 5 minutes + +/** Bound the in-memory replay-nonce cache so it cannot grow unbounded. */ +const NONCE_CACHE_MAX_ENTRIES = 10_000; +const NONCE_CACHE_TTL_MS = MAX_TIMESTAMP_SKEW_MS * 2; + +/** seen (anchor_id + nonce) -> expiry epoch ms. */ +const seenNonces = new Map(); + +function pruneExpiredNonces(now: number): void { + for (const [key, expiresAt] of seenNonces) { + if (expiresAt <= now) seenNonces.delete(key); + } + // Defensive cap in case pruning falls behind under sustained traffic. + if (seenNonces.size > NONCE_CACHE_MAX_ENTRIES) { + const oldestKeys = Array.from(seenNonces.keys()).slice(0, seenNonces.size - NONCE_CACHE_MAX_ENTRIES); + for (const key of oldestKeys) seenNonces.delete(key); + } +} + export interface KycWebhookPayload { user_id?: string; external_id?: string; @@ -20,17 +50,77 @@ export interface KycWebhookPayload { } /** - * Verify anchor webhook signature (if anchor sends one). - * For now, we accept webhooks from known anchors. + * Resolve the per-anchor HMAC secret from the environment, following the + * same WEBHOOK_SECRET_{ANCHOR_ID} convention referenced in the SR-131 issue. + * Anchor IDs are normalised (uppercased, non-alphanumeric replaced with `_`) + * so `sandbox-anchor.example` maps to WEBHOOK_SECRET_SANDBOX_ANCHOR_EXAMPLE. + */ +function anchorSecretEnvVar(anchorId: string): string { + return `WEBHOOK_SECRET_${anchorId.toUpperCase().replace(/[^A-Z0-9]/g, '_')}`; +} + +function getAnchorWebhookSecret(anchorId: string): string | undefined { + return process.env[anchorSecretEnvVar(anchorId)]; +} + +/** + * Verify an anchor webhook's HMAC-SHA256 signature over `${timestamp}.${rawBody}`, + * using a per-anchor secret (WEBHOOK_SECRET_). Also enforces a + * timestamp window and a nonce replay cache, consistent with SR-045's + * treatment of ramp provider webhooks. + * + * Returns a discriminated result rather than a boolean so callers can return + * a specific 401 reason without re-deriving it. */ export function verifyAnchorSignature( anchorId: string, + rawBody: string, signature?: string, -): boolean { - // In production, verify HMAC signature from anchor - // For now, we trust anchors by ID - const trustedAnchors = process.env.TRUSTED_ANCHOR_IDS?.split(',') || []; - return trustedAnchors.includes(anchorId); + timestamp?: string, + nonce?: string, +): { ok: true } | { ok: false; reason: string } { + const secret = getAnchorWebhookSecret(anchorId); + if (!secret) { + return { ok: false, reason: `No webhook secret configured for anchor '${anchorId}'` }; + } + if (!signature) { + return { ok: false, reason: 'Missing webhook signature' }; + } + if (!timestamp) { + return { ok: false, reason: 'Missing webhook timestamp' }; + } + + const timestampMs = Number(timestamp); + if (!Number.isFinite(timestampMs)) { + return { ok: false, reason: 'Malformed webhook timestamp' }; + } + if (Math.abs(Date.now() - timestampMs) > MAX_TIMESTAMP_SKEW_MS) { + return { ok: false, reason: 'Webhook timestamp outside acceptable window' }; + } + + const expected = crypto.createHmac('sha256', secret).update(`${timestamp}.${rawBody}`).digest('hex'); + + let signatureValid: boolean; + try { + signatureValid = crypto.timingSafeEqual(Buffer.from(signature, 'hex'), Buffer.from(expected, 'hex')); + } catch { + signatureValid = false; + } + if (!signatureValid) { + return { ok: false, reason: 'Invalid webhook signature' }; + } + + if (nonce) { + const now = Date.now(); + pruneExpiredNonces(now); + const nonceKey = `${anchorId}:${nonce}`; + if (seenNonces.has(nonceKey)) { + return { ok: false, reason: 'Webhook nonce already used (replay)' }; + } + seenNonces.set(nonceKey, now + NONCE_CACHE_TTL_MS); + } + + return { ok: true }; } /** @@ -46,13 +136,36 @@ export function mapKycStatus(status: string): 'approved' | 'rejected' | 'pending return statusMap[status.toUpperCase()] || 'pending'; } +interface RawBodyRequest extends Request { + rawBody?: string; +} + /** * Handle SEP-12 KYC webhook callback from anchor. + * + * Rejects with 401 before touching the database unless the request carries + * a valid HMAC signature for the target anchor. */ -export async function handleKycWebhook(req: Request, res: Response): Promise { +export async function handleKycWebhook(req: RawBodyRequest, res: Response): Promise { const anchor_id = req.params.anchor_id as string; const payload: KycWebhookPayload = req.body; + const rawBody = req.rawBody ?? JSON.stringify(req.body ?? {}); + const signature = (req.headers['x-webhook-signature'] as string | undefined) + ?? (req.headers['x-anchor-signature'] as string | undefined); + const timestamp = req.headers['x-webhook-timestamp'] as string | undefined; + const nonce = req.headers['x-webhook-nonce'] as string | undefined; + + const verification = verifyAnchorSignature(anchor_id, rawBody, signature, timestamp, nonce); + if (!verification.ok) { + logger.warn('Rejected KYC webhook: signature verification failed', { + anchor_id, + reason: verification.reason, + }); + res.status(401).json({ error: 'Invalid webhook signature', reason: verification.reason }); + return; + } + try { if (!payload.user_id && !payload.external_id) { logger.warn('KYC webhook missing user_id and external_id', { anchor_id, payload });