diff --git a/.env.example b/.env.example index a604c1b..ff597c6 100644 --- a/.env.example +++ b/.env.example @@ -23,11 +23,22 @@ DATABASE_URL="postgresql://parashield:secret@localhost:5432/parashield_db?schema # Redis password used by docker-compose.yml (default: redispass) # REDIS_PASSWORD=redispass +# Distributed locking for scheduled background jobs (#675) — optional. +# Stable lock keys are used per job (lock:job:auth-cleanup, lock:job:oracle-worker, +# lock:job:claims-worker). Lock TTL in milliseconds. Defaults: 300000 (5m) for auth cleanup, +# 900000 (15m) for oracle worker and claims worker. +# AUTH_CLEANUP_LOCK_TTL_MS=300000 +# ORACLE_WORKER_LOCK_TTL_MS=900000 +# CLAIMS_WORKER_LOCK_TTL_MS=900000 + # ─── Authentication ──────────────────────────────────────────────────────────── # Secret used to sign JWT tokens — REQUIRED. In production, this MUST be a cryptographically secure random string of at least 32 bytes (256 bits). # For development/local only, a non-production default of sufficient length (>= 32 chars) is provided below: JWT_SECRET="non-production-dev-jwt-secret-min-32-chars-long-do-not-use-in-production" +# Challenge nonce expiration time in seconds (#681). Default: 300 (5 minutes). +# AUTH_NONCE_TTL_SECONDS=300 + # ─── Webhook secret encryption (#606) ───────────────────────────────────────── # When set, webhook signing secrets are encrypted at rest with AES-256-GCM # before being stored in the database. Strongly recommended in production. diff --git a/README.md b/README.md index d48b857..608407d 100644 --- a/README.md +++ b/README.md @@ -154,32 +154,103 @@ Multiple origins are comma-separated. The server logs the active origin list at --- -## Error handling +## Database Connection Pool Sizing (#676) -All error responses follow the same envelope shape regardless of endpoint: +Prisma connection pool sizing and timeouts are configurable via environment variables or directly as query parameters on `DATABASE_URL`. + +| Environment Variable | Default | Description | Valid Range | +|---|---|---|---| +| `DATABASE_CONNECTION_LIMIT` | `10` | Maximum connections in the pool per application instance | 1 – 500 | +| `DATABASE_POOL_TIMEOUT` | `10` | Seconds a query waits for an available pool connection before throwing a timeout error | 1 – 300 | +| `DATABASE_CONNECT_TIMEOUT` | `5` | Seconds establishing a new TCP connection to PostgreSQL may take before timing out | 1 – 120 | + +### Precedence + +Explicit query parameters in `DATABASE_URL` (e.g. `postgresql://user:pass@host:5432/db?connection_limit=25&pool_timeout=15`) always take precedence over the environment variables. Environment variables are used only when the corresponding query parameter is not present in the URL. + +Startup validation verifies all values are positive integers within sane bounds and fails fast with a clear error on misconfiguration. + +### Sizing Guidance + +When running multiple backend instances (e.g., in Kubernetes, ECS, or behind a load balancer), ensure the aggregate connections from all instances do not exceed PostgreSQL's `max_connections`: + +$$\text{Total Connections} = (\text{Number of Instances} \times \text{DATABASE\_CONNECTION\_LIMIT}) + \text{Reserved Connections}$$ + +**Rule**: $\text{Total Connections} < \text{PostgreSQL } \texttt{max\_connections}$ + +- **Example 1 (Default)**: 5 instances $\times$ 10 connections = 50 connections. Fits comfortably within a default PostgreSQL `max_connections = 100` setting, leaving 50 connections for database migrations, administrative tasks, and replicas. +- **Example 2 (High Throughput)**: 10 instances $\times$ 25 connections = 250 connections. Requires PostgreSQL `max_connections` configured to $\ge 300$. + +### Pool Monitoring & Health Checks + +Pool utilization is monitored continuously via `/api/v1/health`: +- Exposes `active`, `idle`, `waiting`, `max`, `poolTimeout`, and `connectTimeout`. +- Flags health as degraded when connection utilization reaches `DB_POOL_EXHAUSTION_WARN_PERCENT` (default 90%) to detect exhaustion before queries queue or fail. +- Exposes genuine Prisma runtime engine metrics when supported. +- The effective pool configuration is logged at server startup. + +### Load Testing + +A k6 stress test script is provided in `loadtest/k6/pool-stress.js` to evaluate pool behavior and latency under concurrent load: + +```bash +# Run pool stress test with k6 +npm run test:load:pool + +# Or with custom target URL and thresholds +BASE_URL=http://localhost:3001/api/v1 k6 run loadtest/k6/pool-stress.js +``` + +--- + +## Error handling (RFC 7807 Problem Details) + +All error responses use `Content-Type: application/problem+json` conforming to [RFC 7807](https://datatracker.ietf.org/doc/html/rfc7807). Legacy fields remain present in the response body for backward compatibility, but are deprecated: ```json { + "type": "https://parashield.io/errors/not-found", + "title": "Not Found", + "status": 404, + "detail": "Policy not found", + "instance": "/api/v1/policies/abc123", "success": false, "errorCode": "NOT_FOUND", "error": "Policy not found", + "message": "Policy not found", "statusCode": 404, "path": "/api/v1/policies/abc123", "timestamp": "2024-01-15T10:30:00.000Z" } ``` +### RFC 7807 Members + +| Member | Type | Description | +|---|---|---| +| `type` | `string (URI)` | Stable URI reference identifying the error type. | +| `title` | `string` | Short human-readable summary of problem type. | +| `status` | `number` | HTTP status code. | +| `detail` | `string` | Human-readable explanation specific to this occurrence. | +| `instance` | `string (URI)` | Request path that generated the error. | +| `errors` | `array` | (Extension) Validation error details on 400 validation failures. | +| `correlationId` | `string` | (Extension) Request correlation ID when header is provided. | + +### Legacy Fields (Deprecated) + | Field | Type | Description | |---|---|---| -| `success` | `boolean` | Always `false` for errors. | -| `errorCode` | `string` | Stable machine-readable code (see table below). Key off this, not `error`. | -| `error` | `string` \| `object` | Human-readable message, or NestJS validation error details for 400s. May change between versions. | -| `statusCode` | `number` | Mirrors the HTTP status code. | -| `path` | `string` | The request path that produced the error. | +| `success` | `boolean` | Always `false` for errors (Deprecated: use `status`). | +| `errorCode` | `string` | Stable machine-readable code (Deprecated: use `type`). | +| `error` | `string` \| `object` | Error message string (Deprecated: use `detail`). | +| `message` | `string` | Human-readable message (Deprecated: use `detail`). | +| `statusCode` | `number` | Mirrors the HTTP status code (Deprecated: use `status`). | +| `path` | `string` | The request path (Deprecated: use `instance`). | | `timestamp` | `string` | ISO-8601 UTC timestamp. | ### Error codes + | `errorCode` | HTTP status | When it occurs | |---|---|---| | `VALIDATION_ERROR` | 400 | Request body fails class-validator rules (missing/invalid fields, wrong types). The `error` field contains an array of violation objects. | diff --git a/loadtest/k6/pool-stress.js b/loadtest/k6/pool-stress.js new file mode 100644 index 0000000..676d50e --- /dev/null +++ b/loadtest/k6/pool-stress.js @@ -0,0 +1,55 @@ +import http from 'k6/http'; +import { check, sleep } from 'k6'; + +/** + * k6 load test script for database connection pool sizing and stress testing (#676). + * + * Simulates concurrent traffic against database-backed endpoints to evaluate + * pool exhaustion behavior under different DATABASE_CONNECTION_LIMIT and + * DATABASE_POOL_TIMEOUT settings. + * + * Usage: + * k6 run loadtest/k6/pool-stress.js + * Or with custom URL and VUs: + * BASE_URL=http://localhost:3001/api/v1 k6 run loadtest/k6/pool-stress.js + */ +export const options = { + stages: [ + { duration: '30s', target: 20 }, // Ramp up to 20 virtual users (2x default pool) + { duration: '1m', target: 50 }, // Stress test at 50 virtual users (5x default pool) + { duration: '30s', target: 100 }, // Spike to 100 virtual users + { duration: '30s', target: 0 }, // Ramp down + ], + thresholds: { + // Assert that less than 1% of requests fail during steady state + http_req_failed: ['rate<0.05'], + // 95% of database requests should complete within 1.5 seconds + http_req_duration: ['p(95)<1500'], + }, +}; + +const baseUrl = __ENV.BASE_URL || 'http://localhost:3001/api/v1'; + +export default function () { + // Test health endpoint which monitors pool utilization and runs DB queries + const healthRes = http.get(`${baseUrl}/health`); + check(healthRes, { + 'health responds 200 or 503': (res) => res.status === 200 || res.status === 503, + 'pool info is present': (res) => { + try { + const body = JSON.parse(res.body); + return body.checks && body.checks.database && body.checks.database.pool !== undefined; + } catch { + return false; + } + }, + }); + + // Query database-backed products catalog + const productsRes = http.get(`${baseUrl}/products`); + check(productsRes, { + 'products endpoint responds 200': (res) => res.status === 200, + }); + + sleep(0.5); +} diff --git a/package.json b/package.json index d5b171f..c59a2c9 100644 --- a/package.json +++ b/package.json @@ -9,6 +9,7 @@ "start:dev": "nest start --watch", "start:prod": "node dist/main", "test:load": "k6 run loadtest/k6/smoke.js", + "test:load:pool": "k6 run loadtest/k6/pool-stress.js", "lint": "eslint \"{src,test}/**/*.ts\" --fix", "test": "jest", "test:watch": "jest --watch", diff --git a/src/app.module.ts b/src/app.module.ts index 090f9d1..3e8bdf1 100644 --- a/src/app.module.ts +++ b/src/app.module.ts @@ -40,6 +40,15 @@ function validateConfig(config: Record) { errors.push(jwtSecretValidation.error!); } + // #681 — Validate AUTH_NONCE_TTL_SECONDS if configured + const nonceTtl = config['AUTH_NONCE_TTL_SECONDS'] as string | undefined; + if (nonceTtl !== undefined && nonceTtl !== '') { + const parsed = Number(nonceTtl); + if (!Number.isInteger(parsed) || parsed < 1 || parsed > 86400) { + errors.push('AUTH_NONCE_TTL_SECONDS must be a positive integer between 1 and 86400'); + } + } + if (!config['DATABASE_URL']) { errors.push('DATABASE_URL is required'); } else { @@ -50,6 +59,32 @@ function validateConfig(config: Record) { } } + // #676 — Validate Prisma connection pool tuning environment variables + const connLimit = config['DATABASE_CONNECTION_LIMIT'] as string | undefined; + if (connLimit !== undefined && connLimit !== '') { + const parsed = Number(connLimit); + if (!Number.isInteger(parsed) || parsed < 1 || parsed > 500) { + errors.push('DATABASE_CONNECTION_LIMIT must be a positive integer between 1 and 500'); + } + } + + const poolTimeout = config['DATABASE_POOL_TIMEOUT'] as string | undefined; + if (poolTimeout !== undefined && poolTimeout !== '') { + const parsed = Number(poolTimeout); + if (!Number.isInteger(parsed) || parsed < 1 || parsed > 300) { + errors.push('DATABASE_POOL_TIMEOUT must be a positive integer between 1 and 300'); + } + } + + const connectTimeout = config['DATABASE_CONNECT_TIMEOUT'] as string | undefined; + if (connectTimeout !== undefined && connectTimeout !== '') { + const parsed = Number(connectTimeout); + if (!Number.isInteger(parsed) || parsed < 1 || parsed > 120) { + errors.push('DATABASE_CONNECT_TIMEOUT must be a positive integer between 1 and 120'); + } + } + + if (!config['STELLAR_RPC_URL']) { errors.push('STELLAR_RPC_URL is required'); } diff --git a/src/auth/auth-cleanup.worker.ts b/src/auth/auth-cleanup.worker.ts index fbd8a13..d86d0b1 100644 --- a/src/auth/auth-cleanup.worker.ts +++ b/src/auth/auth-cleanup.worker.ts @@ -1,40 +1,56 @@ -import { Injectable, Inject, Logger } from '@nestjs/common'; +import { Injectable, Inject, Logger, Optional } from '@nestjs/common'; import { Cron, CronExpression } from '@nestjs/schedule'; +import { ConfigService } from '@nestjs/config'; import type Redis from 'ioredis'; import { PrismaService } from '../prisma/prisma.service'; import { recordWorkerHeartbeat } from '../common/worker-heartbeat'; +import { DistributedLockService } from '../redis/distributed-lock.service'; /** * AuthCleanupWorker — periodically prunes expired AuthChallenge rows. * - * #489 — this is the only place expired challenges are pruned. The cleanup - * used to also run on every GET /auth/challenge, which put a table-wide - * deleteMany on the hot path of every login. Stale rows are harmless in the - * meantime: login rejects an expired nonce and the next challenge for the - * same wallet overwrites its row via upsert. This runs independently of - * request traffic, mirroring the pattern used by ClaimsWorker. + * Wrapped with DistributedLockService (#675) to prevent multiple instances from + * competing on database table locks during challenge cleanup. */ @Injectable() export class AuthCleanupWorker { private readonly logger = new Logger(AuthCleanupWorker.name); + private readonly lockService: DistributedLockService; constructor( private readonly prisma: PrismaService, @Inject('REDIS_CLIENT') private readonly redis: Redis, - ) {} + @Optional() lockService?: DistributedLockService, + @Optional() private readonly configService?: ConfigService, + ) { + this.lockService = lockService ?? new DistributedLockService(this.redis); + } @Cron(CronExpression.EVERY_10_MINUTES) async cleanupExpiredChallenges(): Promise { - const { count } = await this.prisma.authChallenge.deleteMany({ - where: { expiresAt: { lt: new Date() } }, - }); + const ttlMs = parseInt( + this.configService?.get('AUTH_CLEANUP_LOCK_TTL_MS') ?? '300000', + 10, + ); - if (count > 0) { - this.logger.log(`Cleaned up ${count} expired auth challenge(s)`); - } + await this.lockService.runWithLock( + 'lock:job:auth-cleanup', + ttlMs, + async () => { + const { count } = await this.prisma.authChallenge.deleteMany({ + where: { expiresAt: { lt: new Date() } }, + }); - await recordWorkerHeartbeat(this.redis, 'auth-cleanup').catch((err) => - this.logger.warn(`Failed to record worker heartbeat: ${err instanceof Error ? err.message : String(err)}`), + if (count > 0) { + this.logger.log(`Cleaned up ${count} expired auth challenge(s)`); + } + + await recordWorkerHeartbeat(this.redis, 'auth-cleanup').catch((err) => + this.logger.warn(`Failed to record worker heartbeat: ${err instanceof Error ? err.message : String(err)}`), + ); + }, + { enableHeartbeat: false }, ); } } + diff --git a/src/auth/auth.controller.spec.ts b/src/auth/auth.controller.spec.ts index 53b5075..830d2c1 100644 --- a/src/auth/auth.controller.spec.ts +++ b/src/auth/auth.controller.spec.ts @@ -1,5 +1,6 @@ import { Test, TestingModule } from '@nestjs/testing'; -import { UnauthorizedException } from '@nestjs/common'; +import { BadRequestException, UnauthorizedException } from '@nestjs/common'; +import { ConfigService } from '@nestjs/config'; import { AuthController } from './auth.controller'; import { JwtService } from './jwt.service'; import { PrismaService } from '../prisma/prisma.service'; @@ -16,78 +17,121 @@ describe('AuthController', () => { const mockPrismaService = { authChallenge: { findUnique: jest.fn(), - upsert: jest.fn(), - delete: jest.fn(), + upsert: jest.fn().mockResolvedValue({}), + delete: jest.fn().mockResolvedValue({}), deleteMany: jest.fn().mockResolvedValue({ count: 0 }), }, }; - // Generate keypair for tests + // In-memory Redis simulation supporting TTL, GETDEL, and Lua atomicity + let redisStore = new Map(); + + const mockRedis = { + set: jest.fn(async (key: string, value: string, mode?: string, ttlSeconds?: number) => { + const expiresAt = ttlSeconds ? Date.now() + ttlSeconds * 1000 : Infinity; + redisStore.set(key, { value, expiresAt }); + return 'OK'; + }), + get: jest.fn(async (key: string) => { + const item = redisStore.get(key); + if (!item) return null; + if (Date.now() > item.expiresAt) { + redisStore.delete(key); + return null; + } + return item.value; + }), + getdel: jest.fn(async (key: string) => { + const item = redisStore.get(key); + if (!item) return null; + redisStore.delete(key); + if (Date.now() > item.expiresAt) { + return null; + } + return item.value; + }), + eval: jest.fn(async (script: string, numKeys: number, key: string) => { + const item = redisStore.get(key); + if (!item) return null; + redisStore.delete(key); + if (Date.now() > item.expiresAt) { + return null; + } + return item.value; + }), + del: jest.fn(async (key: string) => { + const existed = redisStore.delete(key); + return existed ? 1 : 0; + }), + }; + + const mockConfigService = { + get: jest.fn((key: string) => { + if (key === 'AUTH_NONCE_TTL_SECONDS') return '300'; + return undefined; + }), + }; + const keypair = Keypair.random(); const walletAddress = keypair.publicKey(); beforeEach(async () => { + redisStore.clear(); + jest.clearAllMocks(); + const module: TestingModule = await Test.createTestingModule({ controllers: [AuthController], providers: [ - { provide: JwtService, useValue: mockJwtService }, - { provide: PrismaService, useValue: mockPrismaService }, + { provide: JwtService, useValue: mockJwtService }, + { provide: PrismaService, useValue: mockPrismaService }, + { provide: 'REDIS_CLIENT', useValue: mockRedis }, + { provide: ConfigService, useValue: mockConfigService }, ], }).compile(); controller = module.get(AuthController); - jest.clearAllMocks(); }); describe('GET /auth/challenge', () => { - it('should generate a 64-character hex nonce and store it', async () => { - mockPrismaService.authChallenge.upsert.mockResolvedValue({ - walletAddress, - nonce: 'mocknonce', - }); - + it('should generate a 64-character hex nonce and store it in Redis with TTL', async () => { const response = await controller.getChallenge(walletAddress); expect(response.success).toBe(true); expect(response.data).toHaveLength(64); // 32 bytes in hex = 64 chars - expect(mockPrismaService.authChallenge.upsert).toHaveBeenCalledWith( - expect.objectContaining({ - where: { walletAddress }, - create: expect.objectContaining({ - walletAddress, - nonce: response.data, - expiresAt: expect.any(Date), - }), - }), + expect(mockRedis.set).toHaveBeenCalledWith( + `auth:nonce:${walletAddress}`, + response.data, + 'EX', + 300, ); }); - it('should not run expired-challenge cleanup on the request path (#489)', async () => { - mockPrismaService.authChallenge.upsert.mockResolvedValue({ walletAddress, nonce: 'mocknonce' }); - - await controller.getChallenge(walletAddress); + it('should respect custom AUTH_NONCE_TTL_SECONDS configuration', async () => { + mockConfigService.get.mockReturnValueOnce('600'); + const response = await controller.getChallenge(walletAddress); - expect(mockPrismaService.authChallenge.deleteMany).not.toHaveBeenCalled(); + expect(response.success).toBe(true); + expect(mockRedis.set).toHaveBeenCalledWith( + `auth:nonce:${walletAddress}`, + response.data, + 'EX', + 600, + ); }); - it('should throw UnauthorizedException for an invalid wallet address format', async () => { - await expect(controller.getChallenge('invalid-wallet')).rejects.toThrow(UnauthorizedException); + it('should throw BadRequestException for invalid wallet address', async () => { + await expect(controller.getChallenge('invalid-wallet')).rejects.toThrow(BadRequestException); }); }); describe('POST /auth/login', () => { - it('should login successfully when signature matches the stored nonce', async () => { - const nonce = 'a'.repeat(64); + it('valid login succeeds once and consumes the nonce atomically', async () => { + const challenge = await controller.getChallenge(walletAddress); + const nonce = challenge.data; + const messageBytes = Buffer.from(nonce, 'utf8'); const signature = keypair.sign(messageBytes).toString('base64'); - mockPrismaService.authChallenge.findUnique.mockResolvedValue({ - walletAddress, - nonce, - expiresAt: new Date(Date.now() + 60000), // future expiry - }); - mockPrismaService.authChallenge.delete.mockResolvedValue({}); - const result = await controller.login({ walletAddress, signature, @@ -97,156 +141,208 @@ describe('AuthController', () => { expect(result.success).toBe(true); expect(result.data.token).toBe('mock-jwt-token'); expect(result.data.walletAddress).toBe(walletAddress); - expect(result.data.expiresIn).toBe(mockJwtService.expiresIn); - expect(mockPrismaService.authChallenge.delete).toHaveBeenCalledWith({ - where: { walletAddress }, - }); + expect(mockRedis.getdel).toHaveBeenCalledWith(`auth:nonce:${walletAddress}`); + + // Nonce is now consumed in Redis + expect(redisStore.has(`auth:nonce:${walletAddress}`)).toBe(false); }); - it('should throw UnauthorizedException if no challenge exists for the wallet', async () => { - mockPrismaService.authChallenge.findUnique.mockResolvedValue(null); + it('replay of the same signature fails with generic 401', async () => { + const challenge = await controller.getChallenge(walletAddress); + const nonce = challenge.data; + + const messageBytes = Buffer.from(nonce, 'utf8'); + const signature = keypair.sign(messageBytes).toString('base64'); + + // First login succeeds + const result = await controller.login({ + walletAddress, + signature, + message: nonce, + }); + expect(result.success).toBe(true); + // Second login replay fails with generic 401 await expect( controller.login({ walletAddress, - signature: 'mismatch-signature', - message: 'nonce', + signature, + message: nonce, }), ).rejects.toThrow(UnauthorizedException); - }); - - it('should throw UnauthorizedException and delete challenge if challenge has expired', async () => { - const nonce = 'a'.repeat(64); - mockPrismaService.authChallenge.findUnique.mockResolvedValue({ - walletAddress, - nonce, - expiresAt: new Date(Date.now() - 1000), // expired 1s ago - }); - mockPrismaService.authChallenge.delete.mockResolvedValue({}); await expect( controller.login({ walletAddress, - signature: 'sig', + signature, message: nonce, }), - ).rejects.toThrow(UnauthorizedException); + ).rejects.toThrow('Invalid or expired authentication challenge'); + }); - expect(mockPrismaService.authChallenge.delete).toHaveBeenCalledWith({ - where: { walletAddress }, - }); + it('expired nonce fails with generic 401 (tested via fake timers / TTL expiry)', async () => { + jest.useFakeTimers(); + try { + const challenge = await controller.getChallenge(walletAddress); + const nonce = challenge.data; + + const messageBytes = Buffer.from(nonce, 'utf8'); + const signature = keypair.sign(messageBytes).toString('base64'); + + // Advance time past the 5-minute (300s) TTL + jest.advanceTimersByTime(301 * 1000); + + await expect( + controller.login({ + walletAddress, + signature, + message: nonce, + }), + ).rejects.toThrow(UnauthorizedException); + + await expect( + controller.login({ + walletAddress, + signature, + message: nonce, + }), + ).rejects.toThrow('Invalid or expired authentication challenge'); + } finally { + jest.useRealTimers(); + } }); - it('should throw UnauthorizedException if the message does not match the stored challenge nonce', async () => { - const storedNonce = 'a'.repeat(64); - const userMessage = 'b'.repeat(64); + it('nonce issued to address A cannot be used for address B', async () => { + const keypairB = Keypair.random(); + const walletB = keypairB.publicKey(); - mockPrismaService.authChallenge.findUnique.mockResolvedValue({ - walletAddress, - nonce: storedNonce, - expiresAt: new Date(Date.now() + 60000), - }); + // Issue challenge to wallet A + const challengeA = await controller.getChallenge(walletAddress); + const nonceA = challengeA.data; + + // Sign nonceA with wallet B's keypair + const messageBytes = Buffer.from(nonceA, 'utf8'); + const signatureB = keypairB.sign(messageBytes).toString('base64'); + // Attempt to login as wallet B using nonceA await expect( controller.login({ - walletAddress, - signature: 'sig', - message: userMessage, + walletAddress: walletB, + signature: signatureB, + message: nonceA, }), ).rejects.toThrow(UnauthorizedException); + + await expect( + controller.login({ + walletAddress: walletB, + signature: signatureB, + message: nonceA, + }), + ).rejects.toThrow('Invalid or expired authentication challenge'); }); - it('should throw UnauthorizedException if signature verification fails', async () => { - const nonce = 'a'.repeat(64); - mockPrismaService.authChallenge.findUnique.mockResolvedValue({ + it('concurrent double-submit yields exactly one success', async () => { + const challenge = await controller.getChallenge(walletAddress); + const nonce = challenge.data; + + const messageBytes = Buffer.from(nonce, 'utf8'); + const signature = keypair.sign(messageBytes).toString('base64'); + + const loginPayload = { walletAddress, - nonce, - expiresAt: new Date(Date.now() + 60000), - }); - mockPrismaService.authChallenge.delete.mockResolvedValue({}); + signature, + message: nonce, + }; + + // Two concurrent login requests with identical valid signature and nonce + const [res1, res2] = await Promise.allSettled([ + controller.login(loginPayload), + controller.login(loginPayload), + ]); + + const fulfilled = [res1, res2].filter((r) => r.status === 'fulfilled'); + const rejected = [res1, res2].filter((r) => r.status === 'rejected'); + + expect(fulfilled).toHaveLength(1); + expect(rejected).toHaveLength(1); + + if (rejected[0].status === 'rejected') { + expect(rejected[0].reason).toBeInstanceOf(UnauthorizedException); + expect((rejected[0].reason as Error).message).toBe( + 'Invalid or expired authentication challenge', + ); + } + }); + + it('failed verification consumes the nonce so it cannot be retried (safer)', async () => { + const challenge = await controller.getChallenge(walletAddress); + const nonce = challenge.data; + // Bad signature attempt + const badSignature = Buffer.from('invalid-signature').toString('base64'); await expect( controller.login({ walletAddress, - signature: Buffer.from('invalid-sig').toString('base64'), + signature: badSignature, message: nonce, }), ).rejects.toThrow(UnauthorizedException); - expect(mockPrismaService.authChallenge.delete).not.toHaveBeenCalled(); - }); - - // #243 — Nonce comparison was plain `!==` (non-constant-time). These tests - // assert the timing-safe path rejects correctly for both same-length and - // different-length mismatches, and that the correct-match path still works. - describe('#243 — constant-time nonce comparison', () => { - it('rejects when the submitted message has the same length as the nonce but different content', async () => { - const storedNonce = 'a'.repeat(64); // 64-char hex nonce - const sameLength = 'b'.repeat(64); // same length, different bytes + // Now attempt valid signature with the same nonce — should FAIL because nonce was consumed + const validMessageBytes = Buffer.from(nonce, 'utf8'); + const validSignature = keypair.sign(validMessageBytes).toString('base64'); - mockPrismaService.authChallenge.findUnique.mockResolvedValue({ + await expect( + controller.login({ walletAddress, - nonce: storedNonce, - expiresAt: new Date(Date.now() + 60000), - }); - - await expect( - controller.login({ walletAddress, signature: 'sig', message: sameLength }), - ).rejects.toThrow(UnauthorizedException); - await expect( - controller.login({ walletAddress, signature: 'sig', message: sameLength }), - ).rejects.toThrow('Invalid challenge message'); - }); - - it('rejects when the submitted message is shorter than the stored nonce', async () => { - const storedNonce = 'a'.repeat(64); - const shorter = 'a'.repeat(32); // different length + signature: validSignature, + message: nonce, + }), + ).rejects.toThrow(UnauthorizedException); - mockPrismaService.authChallenge.findUnique.mockResolvedValue({ + await expect( + controller.login({ walletAddress, - nonce: storedNonce, - expiresAt: new Date(Date.now() + 60000), - }); + signature: validSignature, + message: nonce, + }), + ).rejects.toThrow('Invalid or expired authentication challenge'); + }); - // Must not throw a RangeError from timingSafeEqual — the length guard - // must catch this before the timingSafeEqual call. - await expect( - controller.login({ walletAddress, signature: 'sig', message: shorter }), - ).rejects.toThrow(UnauthorizedException); - }); + it('constant-time comparison rejects mismatching message with generic 401', async () => { + const challenge = await controller.getChallenge(walletAddress); + const nonce = challenge.data; + const mismatchedMessage = 'b'.repeat(64); - it('rejects when the submitted message is longer than the stored nonce', async () => { - const storedNonce = 'a'.repeat(64); - const longer = 'a'.repeat(128); // different length + const messageBytes = Buffer.from(mismatchedMessage, 'utf8'); + const signature = keypair.sign(messageBytes).toString('base64'); - mockPrismaService.authChallenge.findUnique.mockResolvedValue({ + await expect( + controller.login({ walletAddress, - nonce: storedNonce, - expiresAt: new Date(Date.now() + 60000), - }); - - await expect( - controller.login({ walletAddress, signature: 'sig', message: longer }), - ).rejects.toThrow(UnauthorizedException); - }); - - it('accepts when the submitted message exactly matches the stored nonce', async () => { - const nonce = 'a'.repeat(64); - const messageBytes = Buffer.from(nonce, 'utf8'); - const signature = keypair.sign(messageBytes).toString('base64'); + signature, + message: mismatchedMessage, + }), + ).rejects.toThrow(UnauthorizedException); - mockPrismaService.authChallenge.findUnique.mockResolvedValue({ + await expect( + controller.login({ walletAddress, - nonce, - expiresAt: new Date(Date.now() + 60000), - }); - mockPrismaService.authChallenge.delete.mockResolvedValue({}); - - const result = await controller.login({ walletAddress, signature, message: nonce }); + signature, + message: mismatchedMessage, + }), + ).rejects.toThrow('Invalid or expired authentication challenge'); + }); - expect(result.success).toBe(true); - expect(result.data.token).toBe('mock-jwt-token'); - }); + it('rejects malformed wallet address with generic 401 on login', async () => { + await expect( + controller.login({ + walletAddress: 'bad-addr', + signature: 'sig', + message: 'nonce', + }), + ).rejects.toThrow(UnauthorizedException); }); }); }); diff --git a/src/auth/auth.controller.ts b/src/auth/auth.controller.ts index c68de4f..a3915bc 100644 --- a/src/auth/auth.controller.ts +++ b/src/auth/auth.controller.ts @@ -1,7 +1,21 @@ -import { Controller, Post, Body, UnauthorizedException, BadRequestException, Logger, HttpCode, HttpStatus, Get, Query } from '@nestjs/common'; +import { + Controller, + Post, + Body, + UnauthorizedException, + BadRequestException, + Logger, + HttpCode, + HttpStatus, + Get, + Query, + Inject, +} from '@nestjs/common'; import { ApiTags, ApiOperation, ApiResponse, ApiBody } from '@nestjs/swagger'; import { ApiErrorResponse } from '../common/swagger/api-error-responses'; import { Throttle } from '@nestjs/throttler'; +import { ConfigService } from '@nestjs/config'; +import type Redis from 'ioredis'; import { Keypair } from '@stellar/stellar-sdk'; import { JwtService } from './jwt.service'; import { PrismaService } from '../prisma/prisma.service'; @@ -12,12 +26,14 @@ import { timingSafeEqual } from 'crypto'; // Auth endpoints are brute-force/enumeration targets, so they get a tighter // limit than the app-wide default (60 req/60s) configured in app.module.ts. const AUTH_THROTTLE = { default: { limit: 10, ttl: 60000 } }; +const DEFAULT_NONCE_TTL_SECONDS = 300; // 5 minutes /** * AuthController — wallet-based authentication endpoint. * - * The client signs a known message with their Stellar keypair. - * The server verifies the signature and issues a JWT on success. + * The client requests a challenge nonce (stored in Redis with TTL), signs it + * with their Stellar keypair (Ed25519), and submits it to /login. + * Nonces are single-use and consumed atomically to prevent replay attacks. */ @ApiTags('auth') @Controller('auth') @@ -27,8 +43,42 @@ export class AuthController { constructor( private readonly jwtService: JwtService, private readonly prisma: PrismaService, + @Inject('REDIS_CLIENT') private readonly redis: Redis, + private readonly configService: ConfigService, ) {} + /** + * Helper to atomically consume the nonce from Redis for a wallet address. + * Uses Redis GETDEL (Redis >= 6.2) or an atomic Lua script fallback. + * Consuming the nonce atomically guarantees that concurrent login requests + * cannot both succeed using the same nonce. + */ + private async consumeNonce(wallet: string): Promise { + const key = `auth:nonce:${wallet}`; + try { + if (typeof this.redis.getdel === 'function') { + const val = await this.redis.getdel(key); + if (val !== undefined && val !== null) return val; + } + } catch { + // Fallback to Lua script if GETDEL is not supported on older Redis versions + } + + const lua = ` + local val = redis.call('GET', KEYS[1]) + if val then + redis.call('DEL', KEYS[1]) + end + return val + `; + try { + const res = await this.redis.eval(lua, 1, key); + return (res as string | null) ?? null; + } catch { + return null; + } + } + /** * GET /api/v1/auth/challenge * Issue a server-generated nonce for a given wallet address. @@ -50,19 +100,28 @@ export class AuthController { throw new BadRequestException('Invalid or missing Stellar wallet address'); } - // Generate a secure, cryptographically random nonce + // Generate a secure, cryptographically random nonce (32 bytes = 64 hex characters) const nonce = crypto.randomBytes(32).toString('hex'); - const expiresAt = new Date(Date.now() + 5 * 60 * 1000); // 5 minutes TTL - - // #489 — expired-challenge cleanup is a table-wide deleteMany, so it runs - // on a schedule in AuthCleanupWorker rather than on every request here. - - // Store the nonce keyed by wallet address (upsert if they request again) - await this.prisma.authChallenge.upsert({ - where: { walletAddress: wallet }, - update: { nonce, expiresAt, createdAt: new Date() }, - create: { walletAddress: wallet, nonce, expiresAt }, - }); + const ttlSeconds = parseInt( + this.configService?.get('AUTH_NONCE_TTL_SECONDS') ?? `${DEFAULT_NONCE_TTL_SECONDS}`, + 10, + ); + const key = `auth:nonce:${wallet}`; + + // Store in Redis with TTL bound to the specific wallet address + await this.redis.set(key, nonce, 'EX', ttlSeconds); + + // Also mirror to DB for backward compatibility if prisma is configured + if (this.prisma?.authChallenge) { + const expiresAt = new Date(Date.now() + ttlSeconds * 1000); + await this.prisma.authChallenge.upsert({ + where: { walletAddress: wallet }, + update: { nonce, expiresAt, createdAt: new Date() }, + create: { walletAddress: wallet, nonce, expiresAt }, + }).catch((err) => { + this.logger.warn(`Failed to mirror challenge to DB: ${err.message}`); + }); + } return { success: true, @@ -87,63 +146,73 @@ export class AuthController { }) @ApiBody({ type: WalletLoginDto }) @ApiResponse({ status: 200, description: 'Returns a JWT token for the authenticated wallet' }) - @ApiErrorResponse(401, 'Invalid, expired, or missing wallet signature / challenge.', undefined, 'No auth challenge found. Please request a challenge first.') + @ApiErrorResponse(401, 'Invalid, expired, or missing wallet signature / challenge.', undefined, 'Invalid or expired authentication challenge') @ApiErrorResponse(429, 'Rate limit exceeded — auth endpoints allow 10 req / 60 s.', undefined, 'Too many requests. Please try again later.') async login(@Body() dto: WalletLoginDto) { const { walletAddress, signature, message } = dto; - // Fetch the challenge from DB - const challenge = await this.prisma.authChallenge.findUnique({ - where: { walletAddress }, - }); + if (!walletAddress || !/^G[A-Z2-7]{55}$/.test(walletAddress)) { + this.logger.warn(`Login rejected: invalid wallet address format for ${walletAddress}`); + throw new UnauthorizedException('Invalid or expired authentication challenge'); + } - if (!challenge) { - this.logger.warn(`Login rejected: no challenge found for ${walletAddress}`); - throw new UnauthorizedException('No auth challenge found. Please request a challenge first.'); + // Atomically consume the nonce from Redis keyed by wallet address. + // Decision: We deliberately consume the nonce BEFORE signature verification. + // This is safer because consuming the nonce on any verification attempt + // (success or failure) prevents replay attacks, prevents brute-forcing + // signatures against a valid nonce, and eliminates race conditions. + let storedNonce = await this.consumeNonce(walletAddress); + + // Fallback to database challenge if Redis did not have the key (e.g. during migration) + if (!storedNonce && this.prisma?.authChallenge) { + try { + const challenge = await this.prisma.authChallenge.findUnique({ + where: { walletAddress }, + }); + if (challenge) { + await this.prisma.authChallenge.delete({ where: { walletAddress } }).catch(() => {}); + if (challenge.expiresAt >= new Date()) { + storedNonce = challenge.nonce; + } + } + } catch { + // Ignore DB fallback lookup error + } } - // Verify it is not expired - if (challenge.expiresAt < new Date()) { - await this.prisma.authChallenge.delete({ where: { walletAddress } }).catch(() => {}); - this.logger.warn(`Login rejected: challenge expired for ${walletAddress}`); - throw new UnauthorizedException('Auth challenge expired. Please request a new one.'); + // Reject unknown, expired, reused, and wrong-address nonces with the same generic 401 + if (!storedNonce) { + this.logger.warn(`Login rejected: no active challenge found (or expired/reused) for ${walletAddress}`); + throw new UnauthorizedException('Invalid or expired authentication challenge'); } - // #243 — Use constant-time comparison to prevent timing side-channel leaks. - // Even though this nonce is single-use and high-entropy, consistent - // defense-in-depth matches the pattern used for API key comparison elsewhere. - // timingSafeEqual requires equal-length buffers (throws otherwise), so the - // length mismatch is caught by the same early-return false path. - const msgBuf = Buffer.from(message); - const nonceBuf = Buffer.from(challenge.nonce); + // Ensure the signed message actually matches the stored nonce. + // Constant-time comparison prevents timing side-channel leaks. + const msgBuf = Buffer.from(message, 'utf8'); + const nonceBuf = Buffer.from(storedNonce, 'utf8'); const nonceMatches = msgBuf.length === nonceBuf.length && timingSafeEqual(msgBuf, nonceBuf); if (!nonceMatches) { this.logger.warn(`Login rejected: message does not match stored nonce for ${walletAddress}`); - throw new UnauthorizedException('Invalid challenge message'); + throw new UnauthorizedException('Invalid or expired authentication challenge'); } try { - const keypair = Keypair.fromPublicKey(walletAddress); + const keypair = Keypair.fromPublicKey(walletAddress); const messageBytes = Buffer.from(message, 'utf8'); - const sigBytes = Buffer.from(signature, 'base64'); - const isValid = keypair.verify(messageBytes, sigBytes); + const sigBytes = Buffer.from(signature, 'base64'); + const isValid = keypair.verify(messageBytes, sigBytes); if (!isValid) { this.logger.warn(`Login rejected: invalid signature from ${walletAddress}`); - throw new UnauthorizedException('Invalid wallet signature'); + throw new UnauthorizedException('Invalid or expired authentication challenge'); } } catch (err) { if (err instanceof UnauthorizedException) throw err; this.logger.warn(`Signature verification error: ${err}`); - throw new UnauthorizedException('Signature verification failed'); + throw new UnauthorizedException('Invalid or expired authentication challenge'); } - // Invalidate the nonce only after successful verification (one-time use). - await this.prisma.authChallenge.delete({ where: { walletAddress } }).catch((err) => { - this.logger.warn(`Failed to delete challenge for ${walletAddress}: ${err.message}`); - }); - const token = this.jwtService.sign(walletAddress); this.logger.log(`Login successful: wallet=${walletAddress}`); @@ -158,3 +227,4 @@ export class AuthController { } } + diff --git a/src/auth/auth.middleware.ts b/src/auth/auth.middleware.ts index e13122d..00e8145 100644 --- a/src/auth/auth.middleware.ts +++ b/src/auth/auth.middleware.ts @@ -3,6 +3,7 @@ import { Request, Response, NextFunction } from 'express'; import { timingSafeEqual } from 'crypto'; import { Keypair } from '@stellar/stellar-sdk'; import { PrismaService } from '../prisma/prisma.service'; +import { buildProblemDetails } from '../common/errors/problem-details'; /** * AuthMiddleware — verifies a Stellar wallet signature on incoming requests. @@ -30,9 +31,15 @@ export class AuthMiddleware implements NestMiddleware { /** #572 — 401 responses must never be cached or replayed. */ private unauthorized(res: Response, body: { statusCode: number; message: string }): void { res.setHeader('Cache-Control', 'no-store, no-cache'); - res.status(401).json(body); + res.setHeader('Content-Type', 'application/problem+json'); + res.status(401).json(buildProblemDetails({ + status: 401, + message: body.message, + error: body.message, + })); } + /** Truncate a wallet address to first 8 + last 4 characters for safe logging. */ private truncateAddress(address: string): string { if (address.length <= 12) return address; @@ -93,11 +100,16 @@ export class AuthMiddleware implements NestMiddleware { if (isInfraError) { this.logger.error(`Infrastructure error during token verification for ${this.truncateAddress(address)}: ${err}`); res.setHeader('Cache-Control', 'no-store, no-cache'); - res.status(503).json({ - statusCode: 503, - message: 'Authentication service temporarily unavailable', - }); + res.setHeader('Content-Type', 'application/problem+json'); + res.status(503).json( + buildProblemDetails({ + status: 503, + message: 'Authentication service temporarily unavailable', + error: 'Authentication service temporarily unavailable', + }), + ); return; + } this.logger.warn(`Challenge lookup failed for ${this.truncateAddress(address)}: ${err}`); this.unauthorized(res, { statusCode: 401, message: 'Authentication service unavailable' }); diff --git a/src/claims/claims.worker.ts b/src/claims/claims.worker.ts index b15c1b9..8f0f2bd 100644 --- a/src/claims/claims.worker.ts +++ b/src/claims/claims.worker.ts @@ -1,5 +1,6 @@ -import { Injectable, Inject, Logger } from '@nestjs/common'; +import { Injectable, Inject, Logger, Optional } from '@nestjs/common'; import { Cron, CronExpression } from '@nestjs/schedule'; +import { ConfigService } from '@nestjs/config'; import { PolicyStatus } from '@prisma/client'; import type Redis from 'ioredis'; import { ClaimsService } from './claims.service'; @@ -7,6 +8,7 @@ import { PrismaService } from '../prisma/prisma.service'; import { PolicyService, ProductSummary } from '../policy/policy.service'; import { transition } from '../policy/policy-status.machine'; import { recordWorkerHeartbeat } from '../common/worker-heartbeat'; +import { DistributedLockService } from '../redis/distributed-lock.service'; const BATCH_SIZE = 10; const PRODUCTS_PAGE_SIZE = 100; @@ -33,13 +35,18 @@ const jitterMs = (minMs: number, maxMs: number) => @Injectable() export class ClaimsWorker { private readonly logger = new Logger(ClaimsWorker.name); + private readonly lockService: DistributedLockService; constructor( private readonly claims: ClaimsService, private readonly prisma: PrismaService, private readonly policyService: PolicyService, @Inject('REDIS_CLIENT') private readonly redis: Redis, - ) {} + @Optional() lockService?: DistributedLockService, + @Optional() private readonly configService?: ConfigService, + ) { + this.lockService = lockService ?? new DistributedLockService(this.redis); + } private async recordHeartbeat(): Promise { await recordWorkerHeartbeat(this.redis, 'claims').catch((err) => @@ -62,9 +69,26 @@ export class ClaimsWorker { @Cron(CronExpression.EVERY_HOUR) async processActivePolicies(): Promise { + const ttlMs = parseInt( + this.configService?.get('CLAIMS_WORKER_LOCK_TTL_MS') ?? '900000', + 10, + ); + + await this.lockService.runWithLock( + 'lock:job:claims-worker', + ttlMs, + async () => { + await this.executeProcessActivePolicies(); + }, + { enableHeartbeat: true }, + ); + } + + private async executeProcessActivePolicies(): Promise { const tickStart = Date.now(); this.logger.log('Claims worker tick — scanning for expiring policies'); + // #486 — release policies stranded in PROCESSING before scanning, so any // reverted to ACTIVE are retried (or expired) by this same tick. Failure // here must not block the regular scan. diff --git a/src/common/errors/problem-details.ts b/src/common/errors/problem-details.ts new file mode 100644 index 0000000..057888a --- /dev/null +++ b/src/common/errors/problem-details.ts @@ -0,0 +1,158 @@ +import { ErrorCode, errorCodeFromStatus } from './error-codes'; + +/** + * RFC 7807 Problem Details representation (#677). + * + * Conforms to RFC 7807 "Problem Details for HTTP APIs" served with Content-Type + * `application/problem+json`. Backward compatibility is preserved by including + * legacy fields (success, statusCode, error, message, errorCode, path, timestamp) + * alongside the standard RFC 7807 members. + */ +export interface ProblemDetails { + // ── Standard RFC 7807 members ────────────────────────────────────────────── + type: string; + title: string; + status: number; + detail: string; + instance?: string; + + // ── Extension members ────────────────────────────────────────────────────── + errors?: unknown; + correlationId?: string; + + // ── Legacy fields (Deprecated: use RFC 7807 members) ──────────────────────── + success: false; + statusCode: number; + message: string; + error: string | object; + errorCode?: string; + path?: string; + timestamp: string; + retryAfter?: number; + [key: string]: unknown; +} + +export interface BuildProblemDetailsOptions { + status: number; + message?: string; + error?: string | object; + errorCode?: string; + path?: string; + errors?: unknown; + correlationId?: string; + retryAfter?: number; +} + +/** + * Returns a stable URI reference identifying the problem type per RFC 7807. + */ +export function getProblemType(status: number, errorCode?: string): string { + if (errorCode) { + const slug = errorCode.toLowerCase().replace(/_/g, '-'); + return `https://parashield.io/errors/${slug}`; + } + + switch (status) { + case 400: return 'https://parashield.io/errors/bad-request'; + case 401: return 'https://parashield.io/errors/unauthorized'; + case 403: return 'https://parashield.io/errors/forbidden'; + case 404: return 'https://parashield.io/errors/not-found'; + case 408: return 'https://parashield.io/errors/request-timeout'; + case 409: return 'https://parashield.io/errors/conflict'; + case 410: return 'https://parashield.io/errors/gone'; + case 422: return 'https://parashield.io/errors/unprocessable-entity'; + case 429: return 'https://parashield.io/errors/too-many-requests'; + case 500: return 'https://parashield.io/errors/internal-error'; + case 502: return 'https://parashield.io/errors/bad-gateway'; + case 503: return 'https://parashield.io/errors/service-unavailable'; + case 504: return 'https://parashield.io/errors/gateway-timeout'; + default: + return 'about:blank'; + } +} + +/** + * Returns a short, human-readable summary of the problem type per RFC 7807. + */ +export function getProblemTitle(status: number): string { + switch (status) { + case 400: return 'Bad Request'; + case 401: return 'Unauthorized'; + case 403: return 'Forbidden'; + case 404: return 'Not Found'; + case 408: return 'Request Timeout'; + case 409: return 'Conflict'; + case 410: return 'Gone'; + case 422: return 'Unprocessable Entity'; + case 429: return 'Too Many Requests'; + case 500: return 'Internal Server Error'; + case 502: return 'Bad Gateway'; + case 503: return 'Service Unavailable'; + case 504: return 'Gateway Timeout'; + default: + return status >= 500 ? 'Internal Server Error' : 'Error'; + } +} + +/** + * Constructs an RFC 7807 Problem Details object with backward compatibility fields. + * + * Security: 500 errors are sanitized to generic text; stack traces, internal SQL, + * and system secrets are never leaked into `detail`. + */ +export function buildProblemDetails(options: BuildProblemDetailsOptions): ProblemDetails { + const { + status, + message, + error, + errorCode, + path, + errors, + correlationId, + retryAfter, + } = options; + + const type = getProblemType(status, errorCode); + const title = getProblemTitle(status); + + // Security guard: never leak internal exception details for 5xx errors + const safeDetail = status >= 500 + ? 'An unexpected error occurred. Please try again later.' + : (message || (typeof error === 'string' ? error : title)); + + const safeLegacyError = status >= 500 + ? 'Internal server error' + : (error ?? safeDetail); + + const problem: ProblemDetails = { + // RFC 7807 standard members + type, + title, + status, + detail: safeDetail, + instance: path, + + // Backward compatibility fields (deprecated) + success: false, + statusCode: status, + message: safeDetail, + error: safeLegacyError, + errorCode: errorCode ?? errorCodeFromStatus(status), + path, + timestamp: new Date().toISOString(), + }; + + if (errors !== undefined) { + problem.errors = errors; + } + + if (correlationId) { + problem.correlationId = correlationId; + } + + if (retryAfter !== undefined) { + problem.retryAfter = retryAfter; + } + + return problem; +} diff --git a/src/common/filters/http-exception.filter.spec.ts b/src/common/filters/http-exception.filter.spec.ts index 56d2d88..e1b5d82 100644 --- a/src/common/filters/http-exception.filter.spec.ts +++ b/src/common/filters/http-exception.filter.spec.ts @@ -1,19 +1,25 @@ -import { HttpException, HttpStatus } from '@nestjs/common'; +import { HttpException, HttpStatus, BadRequestException, UnauthorizedException } from '@nestjs/common'; import { GlobalExceptionFilter } from './http-exception.filter'; describe('GlobalExceptionFilter', () => { let filter: GlobalExceptionFilter; let logger: { error: jest.Mock; warn: jest.Mock }; let json: jest.Mock; - let res: { status: jest.Mock; json: jest.Mock }; + let setHeader: jest.Mock; + let res: { status: jest.Mock; json: jest.Mock; setHeader: jest.Mock }; - function mockHost(method: string, url: string) { + function mockHost(method: string, url: string, headers: Record = {}) { json = jest.fn(); - res = { status: jest.fn().mockReturnValue({ json }), json }; + setHeader = jest.fn(); + res = { + status: jest.fn().mockReturnValue({ json }), + json, + setHeader, + }; return { switchToHttp: () => ({ getResponse: () => res, - getRequest: () => ({ method, url }), + getRequest: () => ({ method, url, headers }), }), } as any; } @@ -24,124 +30,177 @@ describe('GlobalExceptionFilter', () => { (filter as any).logger = logger; }); - it('responds with HttpException status and message for HttpException errors', () => { - const host = mockHost('GET', '/resource'); - const error = new HttpException('Not found', HttpStatus.NOT_FOUND); - - filter.catch(error, host); + describe('RFC 7807 Problem Details (#677)', () => { + it('validation error (400) returns application/problem+json with type, title, status, detail, instance, errors array, and legacy fields', () => { + const host = mockHost('POST', '/api/v1/policies/buy'); + const validationError = new BadRequestException({ + statusCode: 400, + message: ['wallet must be a string', 'productId should not be empty'], + error: 'Bad Request', + }); + + filter.catch(validationError, host); + + expect(res.setHeader).toHaveBeenCalledWith('Content-Type', 'application/problem+json'); + expect(res.status).toHaveBeenCalledWith(400); + + const body = json.mock.calls[0][0]; + // RFC 7807 standard members + expect(body.type).toBe('https://parashield.io/errors/validation-error'); + expect(body.title).toBe('Bad Request'); + expect(body.status).toBe(400); + expect(body.detail).toBe('wallet must be a string; productId should not be empty'); + expect(body.instance).toBe('/api/v1/policies/buy'); + expect(body.errors).toEqual(['wallet must be a string', 'productId should not be empty']); + + // Backward compatibility fields (deprecated) + expect(body.success).toBe(false); + expect(body.statusCode).toBe(400); + expect(body.error).toBe('wallet must be a string; productId should not be empty'); + expect(body.message).toBe('wallet must be a string; productId should not be empty'); + expect(body.path).toBe('/api/v1/policies/buy'); + expect(typeof body.timestamp).toBe('string'); + }); - expect(res.status).toHaveBeenCalledWith(404); - expect(json).toHaveBeenCalledWith({ - success: false, - error: 'Not found', - statusCode: 404, - path: '/resource', - timestamp: expect.any(String), + it('401 Unauthorized returns application/problem+json with RFC 7807 members and legacy fields', () => { + const host = mockHost('GET', '/api/v1/policies/me'); + const error = new UnauthorizedException('Missing or invalid authentication'); + + filter.catch(error, host); + + expect(res.setHeader).toHaveBeenCalledWith('Content-Type', 'application/problem+json'); + expect(res.status).toHaveBeenCalledWith(401); + + const body = json.mock.calls[0][0]; + expect(body.type).toBe('https://parashield.io/errors/unauthorized'); + expect(body.title).toBe('Unauthorized'); + expect(body.status).toBe(401); + expect(body.detail).toBe('Missing or invalid authentication'); + expect(body.instance).toBe('/api/v1/policies/me'); + + // Backward compatibility fields + expect(body.success).toBe(false); + expect(body.statusCode).toBe(401); + expect(body.error).toBe('Missing or invalid authentication'); + expect(body.message).toBe('Missing or invalid authentication'); + expect(body.errorCode).toBe('UNAUTHORIZED'); + expect(body.path).toBe('/api/v1/policies/me'); }); - expect(logger.warn).toHaveBeenCalledWith('GET /resource → 404'); - expect(logger.error).not.toHaveBeenCalled(); - }); - it('responds with 500 and generic message for non-HttpException errors', () => { - const host = mockHost('POST', '/api/data'); - const error = new Error('database connection lost'); + it('404 Not Found returns application/problem+json with RFC 7807 members and legacy fields', () => { + const host = mockHost('GET', '/api/v1/policies/missing-uuid'); + const error = new HttpException('Policy not found', HttpStatus.NOT_FOUND); - filter.catch(error, host); + filter.catch(error, host); - expect(res.status).toHaveBeenCalledWith(500); - expect(json).toHaveBeenCalledWith({ - success: false, - error: 'Internal server error', - statusCode: 500, - path: '/api/data', - timestamp: expect.any(String), - }); - }); + expect(res.setHeader).toHaveBeenCalledWith('Content-Type', 'application/problem+json'); + expect(res.status).toHaveBeenCalledWith(404); - it('does not leak stack trace in the JSON response for non-HttpException errors', () => { - const host = mockHost('DELETE', '/secret'); - const error = new Error('sensitive details'); + const body = json.mock.calls[0][0]; + expect(body.type).toBe('https://parashield.io/errors/not-found'); + expect(body.title).toBe('Not Found'); + expect(body.status).toBe(404); + expect(body.detail).toBe('Policy not found'); + expect(body.instance).toBe('/api/v1/policies/missing-uuid'); - filter.catch(error, host); + // Backward compatibility fields + expect(body.success).toBe(false); + expect(body.statusCode).toBe(404); + expect(body.error).toBe('Policy not found'); + expect(body.message).toBe('Policy not found'); + expect(body.errorCode).toBe('NOT_FOUND'); + }); - const responseBody = json.mock.calls[0][0]; - expect(responseBody).not.toHaveProperty('stack'); - expect(responseBody).not.toHaveProperty('trace'); - expect(responseBody.error).toBe('Internal server error'); - }); + it('unexpected 500 returns generic message, application/problem+json, and never leaks internal details', () => { + const host = mockHost('POST', '/api/v1/claims/submit'); + const internalError = new Error('SELECT * FROM users WHERE password = secret; FATAL: connection reset'); + (internalError as any).stack = 'Error: SELECT ... at /app/node_modules/pg/...'; + + filter.catch(internalError, host); + + expect(res.setHeader).toHaveBeenCalledWith('Content-Type', 'application/problem+json'); + expect(res.status).toHaveBeenCalledWith(500); + + const body = json.mock.calls[0][0]; + expect(body.type).toBe('https://parashield.io/errors/internal-error'); + expect(body.title).toBe('Internal Server Error'); + expect(body.status).toBe(500); + expect(body.detail).toBe('An unexpected error occurred. Please try again later.'); + + // No stack traces, SQL, or internal details leak + expect(body).not.toHaveProperty('stack'); + expect(body).not.toHaveProperty('trace'); + expect(JSON.stringify(body)).not.toContain('SELECT'); + expect(JSON.stringify(body)).not.toContain('FATAL'); + expect(JSON.stringify(body)).not.toContain('secret'); + + // Backward compatibility fields + expect(body.success).toBe(false); + expect(body.statusCode).toBe(500); + expect(body.error).toBe('Internal server error'); + expect(body.errorCode).toBe('INTERNAL_ERROR'); + }); - it('logs error level with stack trace for 5xx errors', () => { - const host = mockHost('PUT', '/update'); - const error = new Error('timeout'); - (error as any).status = 500; + it('includes request correlationId when x-correlation-id header is provided', () => { + const host = mockHost('GET', '/api/v1/policies', { 'x-correlation-id': 'req-corr-123' }); + const error = new HttpException('Forbidden', HttpStatus.FORBIDDEN); - filter.catch(error, host); + filter.catch(error, host); - expect(logger.error).toHaveBeenCalledWith( - 'PUT /update → 500', - error.stack, - ); + const body = json.mock.calls[0][0]; + expect(body.correlationId).toBe('req-corr-123'); + }); }); - it('logs warn level for 4xx client errors', () => { - const host = mockHost('GET', '/not-found'); - const error = new HttpException('Bad request', HttpStatus.BAD_REQUEST); + describe('Logging and edge cases', () => { + it('logs error level with stack trace for 5xx errors', () => { + const host = mockHost('PUT', '/update'); + const error = new Error('timeout'); + (error as any).status = 500; - filter.catch(error, host); + filter.catch(error, host); - expect(logger.warn).toHaveBeenCalledWith('GET /not-found → 400'); - expect(logger.error).not.toHaveBeenCalled(); - }); + expect(logger.error).toHaveBeenCalledWith( + 'PUT /update → 500', + error.stack, + ); + }); - it('handles string exceptions without crashing', () => { - const host = mockHost('GET', '/crash'); - const error = 'string error'; + it('logs warn level for 4xx client errors', () => { + const host = mockHost('GET', '/not-found'); + const error = new HttpException('Bad request', HttpStatus.BAD_REQUEST); - filter.catch(error, host); + filter.catch(error, host); - expect(res.status).toHaveBeenCalledWith(500); - expect(json).toHaveBeenCalledWith({ - success: false, - error: 'Internal server error', - statusCode: 500, - path: '/crash', - timestamp: expect.any(String), + expect(logger.warn).toHaveBeenCalledWith('GET /not-found → 400'); + expect(logger.error).not.toHaveBeenCalled(); }); - expect(logger.error).toHaveBeenCalledWith( - 'GET /crash → 500', - 'string error', - ); - }); - it('handles null exceptions without crashing', () => { - const host = mockHost('GET', '/null'); + it('handles string exceptions without crashing', () => { + const host = mockHost('GET', '/crash'); + const error = 'string error'; - filter.catch(null, host); + filter.catch(error, host); - expect(res.status).toHaveBeenCalledWith(500); - expect(json).toHaveBeenCalledWith({ - success: false, - error: 'Internal server error', - statusCode: 500, - path: '/null', - timestamp: expect.any(String), + expect(res.status).toHaveBeenCalledWith(500); + const body = json.mock.calls[0][0]; + expect(body.status).toBe(500); + expect(body.detail).toBe('An unexpected error occurred. Please try again later.'); + expect(logger.error).toHaveBeenCalledWith( + 'GET /crash → 500', + 'string error', + ); }); - }); - it('includes success, error, statusCode, path, and timestamp in the response body', () => { - const host = mockHost('PATCH', '/item'); - const error = new HttpException('Conflict', HttpStatus.CONFLICT); + it('handles null exceptions without crashing', () => { + const host = mockHost('GET', '/null'); - filter.catch(error, host); + filter.catch(null, host); - const body = json.mock.calls[0][0]; - expect(body).toEqual({ - success: false, - error: 'Conflict', - statusCode: 409, - path: '/item', - timestamp: expect.any(String), + expect(res.status).toHaveBeenCalledWith(500); + const body = json.mock.calls[0][0]; + expect(body.status).toBe(500); + expect(body.detail).toBe('An unexpected error occurred. Please try again later.'); }); - expect(typeof body.timestamp).toBe('string'); }); }); diff --git a/src/common/filters/http-exception.filter.ts b/src/common/filters/http-exception.filter.ts index 4329a12..ed2bbcc 100644 --- a/src/common/filters/http-exception.filter.ts +++ b/src/common/filters/http-exception.filter.ts @@ -5,6 +5,7 @@ import { import { Request, Response } from 'express'; import { errorCodeFromBody, errorCodeFromStatus } from '../errors/error-codes'; import { serializeBigInt } from '../interceptors/bigint-serializer.interceptor'; +import { buildProblemDetails } from '../errors/problem-details'; @Catch() export class GlobalExceptionFilter implements ExceptionFilter { @@ -32,47 +33,42 @@ export class GlobalExceptionFilter implements ExceptionFilter { this.logger.warn(`${req.method} ${req.url} → ${status}`); } - // #425 — Normalize the error message to always be a plain string so the - // client never has to branch on typeof error. NestJS HttpException bodies - // can be strings or objects ({ statusCode, message, error }); class-validator - // ValidationPipe produces { statusCode, message: string[], error } objects. - // We extract the most descriptive string available in each case. + // Extract human-readable string message const message = this.extractMessage(rawResponse); - // #402 — include a stable machine-readable errorCode so the frontend - // can branch on error type without parsing the human-readable message. - // #425 — retryAfter is promoted to the top level so clients never need - // to dig into a nested error object to get the value they act on. + // Extract retryAfter header for 429 rate limit responses const retryAfter = this.extractRetryAfter(rawResponse); - // #341 — success responses across controllers use { success, data }, - // so error responses carry the same success flag (always false here) - // with the message under `error`, instead of a differently shaped - // { statusCode, message } body the frontend had to special-case. - const body: Record = { - success: false, - errorCode: errorCodeFromBody(rawResponse) ?? errorCodeFromStatus(status), - error: message, - statusCode: status, - path: req.url, - timestamp: new Date().toISOString(), - }; - - if (retryAfter !== undefined) { - body['retryAfter'] = retryAfter; - } - - res.status(status).json(serializeBigInt(body)); + // Extract field-level validation errors from class-validator ValidationPipe + const errors = this.extractValidationErrors(rawResponse); + + // Extract optional correlation ID if present on request + const correlationId = (req.headers['x-correlation-id'] || req.headers['x-request-id']) as string | undefined; + + const errorCode = errorCodeFromBody(rawResponse) ?? errorCodeFromStatus(status); + const path = req.originalUrl || req.url; + + // #677 — Adopt RFC 7807 problem details with Content-Type application/problem+json. + // Includes type, title, status, detail, instance, extension members (errors, correlationId), + // and preserves legacy fields (success, statusCode, error, message, errorCode, path, timestamp) + // for backward compatibility. + const problemBody = buildProblemDetails({ + status, + message, + error: message, + errorCode, + path, + errors, + correlationId, + retryAfter, + }); + + res.setHeader('Content-Type', 'application/problem+json'); + res.status(status).json(serializeBigInt(problemBody)); } /** * Reduce an HttpException response to a human-readable string. - * - * Priority order: - * 1. Plain string body → use directly - * 2. Object with a string `message` property → use that string - * 3. Object with an array `message` property (class-validator) → join with "; " - * 4. Anything else → JSON.stringify as a last resort */ private extractMessage(raw: string | object): string { if (typeof raw === 'string') return raw; @@ -90,6 +86,21 @@ export class GlobalExceptionFilter implements ExceptionFilter { return JSON.stringify(serializeBigInt(raw)); } + /** + * Extract validation error list from ValidationPipe response objects. + */ + private extractValidationErrors(raw: string | object): unknown { + if (typeof raw !== 'object' || raw === null) return undefined; + const obj = raw as Record; + if (Array.isArray(obj['message'])) { + return obj['message']; + } + if (Array.isArray(obj['errors'])) { + return obj['errors']; + } + return undefined; + } + /** * Pull retryAfter out of a ThrottleGuard body `{ retryAfter: number }`, * or return undefined for all other exception types. @@ -100,3 +111,4 @@ export class GlobalExceptionFilter implements ExceptionFilter { return typeof val === 'number' ? val : undefined; } } + diff --git a/src/common/interceptors/logging.interceptor.ts b/src/common/interceptors/logging.interceptor.ts index f4595d5..3e0f32c 100644 --- a/src/common/interceptors/logging.interceptor.ts +++ b/src/common/interceptors/logging.interceptor.ts @@ -10,6 +10,8 @@ import { tap } from 'rxjs/operators'; import { Request, Response } from 'express'; /** Query parameters whose values must never appear in logs. */ +const HEALTH_PATH_PREFIX = '/api/v1/health'; + const SENSITIVE_PARAMS = new Set([ 'wallet', 'secret', 'token', 'key', 'apikey', 'api_key', 'authorization', 'password', 'seed', 'private_key', diff --git a/src/common/middleware/idempotency.middleware.ts b/src/common/middleware/idempotency.middleware.ts index 567cd73..25eaae3 100644 --- a/src/common/middleware/idempotency.middleware.ts +++ b/src/common/middleware/idempotency.middleware.ts @@ -2,6 +2,7 @@ import { Injectable, NestMiddleware, Logger } from '@nestjs/common'; import { Request, Response, NextFunction } from 'express'; import Redis from 'ioredis'; import { randomUUID } from 'crypto'; +import { buildProblemDetails } from '../errors/problem-details'; // #438 — Idempotency keys prevent duplicate processing of the same request. // A client that retries a POST (e.g. after a network timeout) sends the same @@ -67,12 +68,17 @@ export class IdempotencyMiddleware implements NestMiddleware { // Basic validation — reject keys that are too long or contain newlines // (could be used to construct arbitrary Redis keys). if (rawKey.length > IDEMPOTENCY_KEY_MAX_LENGTH || /[\r\n]/.test(rawKey)) { - res.status(400).json({ - success: false, - errorCode: 'BAD_REQUEST', - error: 'Invalid Idempotency-Key header value', - statusCode: 400, - }); + res.setHeader('Content-Type', 'application/problem+json'); + res.status(400).json( + buildProblemDetails({ + status: 400, + errorCode: 'BAD_REQUEST', + message: 'Invalid Idempotency-Key header value', + error: 'Invalid Idempotency-Key header value', + path: req.originalUrl || req.url, + correlationId: (req.headers['x-correlation-id'] || req.headers['x-request-id']) as string | undefined, + }), + ); return; } @@ -87,7 +93,7 @@ export class IdempotencyMiddleware implements NestMiddleware { next(err); }; - this.handle(storeKey, rawKey, res, nextOnce).catch((err: unknown) => { + this.handle(storeKey, rawKey, req, res, nextOnce).catch((err: unknown) => { // Redis unavailable — fail open (process the request normally) so a // Redis outage doesn't take down the API. Log as a warning so ops can // detect the degradation. @@ -105,7 +111,7 @@ export class IdempotencyMiddleware implements NestMiddleware { * the winner releases the lock without caching (non-2xx — we then retry * acquiring the lock and run the handler ourselves), or the wait times out. */ - private async handle(storeKey: string, rawKey: string, res: Response, next: NextFunction): Promise { + private async handle(storeKey: string, rawKey: string, req: Request, res: Response, next: NextFunction): Promise { const lockKey = `${storeKey}:lock`; const lockToken = randomUUID(); const deadline = Date.now() + IDEMPOTENCY_WAIT_TIMEOUT_MS; @@ -128,15 +134,21 @@ export class IdempotencyMiddleware implements NestMiddleware { if (Date.now() >= deadline) { this.logger.warn(`Timed out waiting for in-flight request with idempotency key=${rawKey}`); - res.status(409).json({ - success: false, - errorCode: 'IDEMPOTENCY_CONFLICT', - error: 'A request with this Idempotency-Key is still being processed. Retry later.', - statusCode: 409, - }); + res.setHeader('Content-Type', 'application/problem+json'); + res.status(409).json( + buildProblemDetails({ + status: 409, + errorCode: 'IDEMPOTENCY_CONFLICT', + message: 'A request with this Idempotency-Key is still being processed. Retry later.', + error: 'A request with this Idempotency-Key is still being processed. Retry later.', + path: req.originalUrl || req.url, + correlationId: (req.headers['x-correlation-id'] || req.headers['x-request-id']) as string | undefined, + }), + ); return; } + await new Promise((resolve) => setTimeout(resolve, IDEMPOTENCY_POLL_INTERVAL_MS)); } } diff --git a/src/common/middleware/request-timeout.middleware.ts b/src/common/middleware/request-timeout.middleware.ts index d6ee0cf..7511ff8 100644 --- a/src/common/middleware/request-timeout.middleware.ts +++ b/src/common/middleware/request-timeout.middleware.ts @@ -1,5 +1,6 @@ import { Injectable, NestMiddleware, Logger } from '@nestjs/common'; import { Request, Response, NextFunction } from 'express'; +import { buildProblemDetails } from '../errors/problem-details'; // #409 — configurable request timeout. Long-running requests can exhaust // worker threads; responding with 408 lets the client retry instead of @@ -38,15 +39,21 @@ export class RequestTimeoutMiddleware implements NestMiddleware { this.logger.warn( `Request timeout after ${this.timeoutMs}ms: ${req.method} ${req.url}`, ); - res.status(408).json({ - statusCode: 408, - error: 'Request Timeout', - message: 'The request exceeded the maximum allowed processing time.', - }); + res.setHeader('Content-Type', 'application/problem+json'); + res.status(408).json( + buildProblemDetails({ + status: 408, + message: 'The request exceeded the maximum allowed processing time.', + error: 'Request Timeout', + path: req.originalUrl || req.url, + correlationId: (req.headers['x-correlation-id'] || req.headers['x-request-id']) as string | undefined, + }), + ); // Destroy the socket so the connection slot is released immediately. req.socket?.destroy(); }, this.timeoutMs); + // Guard against double response: if the handler completes after the // timeout already sent a 408, suppress the late write (#693). const originalWrite = res.write.bind(res); diff --git a/src/common/swagger/error-response.dto.ts b/src/common/swagger/error-response.dto.ts index d29b2a5..292aff1 100644 --- a/src/common/swagger/error-response.dto.ts +++ b/src/common/swagger/error-response.dto.ts @@ -2,34 +2,79 @@ import { ApiProperty } from '@nestjs/swagger'; import { ErrorCode } from '../errors/error-codes'; /** - * Canonical error envelope returned by GlobalExceptionFilter for every - * non-2xx response. Controllers should reference this class in their - * @ApiResponse decorators so the Swagger UI shows the actual error shape - * rather than a plain description string. + * RFC 7807 Problem Details response envelope returned by GlobalExceptionFilter + * for all error responses (#677). Served with Content-Type `application/problem+json`. * - * All fields are stable across versions — frontend/backend integrators - * should key off `errorCode`, not the human-readable `error` message. + * Conforms to RFC 7807. Legacy envelope fields (success, statusCode, error, + * message, errorCode, path, timestamp) remain present alongside the standard + * problem members for backward compatibility, but are deprecated. */ export class ErrorResponseDto { + // ── RFC 7807 Standard Members ────────────────────────────────────────────── @ApiProperty({ - description: 'Always false for error responses.', + description: 'A URI reference that identifies the problem type per RFC 7807.', + example: 'https://parashield.io/errors/not-found', + }) + type: string; + + @ApiProperty({ + description: 'A short, human-readable summary of the problem type per RFC 7807.', + example: 'Not Found', + }) + title: string; + + @ApiProperty({ + description: 'The HTTP status code generated by the origin server for this occurrence of the problem.', + example: 404, + }) + status: number; + + @ApiProperty({ + description: 'A human-readable explanation specific to this occurrence of the problem.', + example: 'Policy not found', + }) + detail: string; + + @ApiProperty({ + description: 'A URI reference that identifies the specific occurrence of the problem (request path).', + example: '/api/v1/policies/abc123', + required: false, + }) + instance?: string; + + @ApiProperty({ + description: 'Detailed list of validation or field errors (present on validation failures).', + required: false, + }) + errors?: unknown; + + @ApiProperty({ + description: 'Unique correlation ID associated with the request (from X-Correlation-ID / X-Request-ID header).', + example: 'req_1234567890', + required: false, + }) + correlationId?: string; + + // ── Backward Compatibility Fields (Deprecated) ───────────────────────────── + @ApiProperty({ + description: 'Deprecated: Use RFC 7807 members. Always false for error responses.', example: false, + deprecated: true, }) success: boolean; @ApiProperty({ description: - 'Stable machine-readable error code. Key off this value in client code — ' + - 'the human-readable `error` field may change between API versions.', + 'Deprecated: Use `type` per RFC 7807. Stable machine-readable error code.', enum: ErrorCode, example: ErrorCode.NOT_FOUND, + deprecated: true, }) errorCode: ErrorCode; @ApiProperty({ description: - 'Human-readable error message or, for 400 validation failures, an object ' + - 'containing per-field constraint violations from class-validator.', + 'Deprecated: Use `detail` per RFC 7807. Human-readable error message.', oneOf: [ { type: 'string', example: 'Policy not found' }, { @@ -46,31 +91,41 @@ export class ErrorResponseDto { }, }, ], + deprecated: true, }) error: string | object; @ApiProperty({ - description: 'HTTP status code mirrored in the body for convenience.', + description: 'Deprecated: Use `detail` per RFC 7807. String message.', + example: 'Policy not found', + deprecated: true, + }) + message: string; + + @ApiProperty({ + description: 'Deprecated: Use `status` per RFC 7807. HTTP status code.', example: 404, + deprecated: true, }) statusCode: number; @ApiProperty({ - description: 'The request path that produced the error.', + description: 'Deprecated: Use `instance` per RFC 7807. The request path that produced the error.', example: '/api/v1/policies/abc123', + deprecated: true, }) path: string; @ApiProperty({ - description: 'ISO-8601 UTC timestamp at which the error occurred.', + description: 'Deprecated: ISO-8601 UTC timestamp at which the error occurred.', example: '2024-01-15T10:30:00.000Z', + deprecated: true, }) timestamp: string; @ApiProperty({ description: - 'Seconds to wait before retrying. Present **only** on 429 responses ' + - 'once the rate limit window has been exceeded.', + 'Seconds to wait before retrying. Present **only** on 429 responses once rate limit is exceeded.', example: 42, required: false, }) @@ -78,22 +133,31 @@ export class ErrorResponseDto { } // --------------------------------------------------------------------------- -// Per-errorCode example payloads used by the decorator helpers below. -// These are concrete examples the Swagger UI can render, one per error code. +// Per-errorCode example payloads used by Swagger UI decorators. // --------------------------------------------------------------------------- function errorExample( errorCode: ErrorCode, statusCode: number, - message: string, + title: string, + detail: string, path = '/api/v1/resource', retryAfter?: number, + errors?: unknown, ) { + const slug = errorCode.toLowerCase().replace(/_/g, '-'); return { + type: `https://parashield.io/errors/${slug}`, + title, + status: statusCode, + detail, + instance: path, + ...(errors !== undefined ? { errors } : {}), success: false, - errorCode, - error: message, statusCode, + message: detail, + error: detail, + errorCode, path, timestamp: '2024-01-15T10:30:00.000Z', ...(retryAfter !== undefined ? { retryAfter } : {}), @@ -101,14 +165,22 @@ function errorExample( } export const ERROR_EXAMPLES = { - 400: errorExample(ErrorCode.VALIDATION_ERROR, 400, 'wallet must be a string; productId should not be empty'), - '400_bad': errorExample(ErrorCode.BAD_REQUEST, 400, 'Malformed request parameter'), - 401: errorExample(ErrorCode.UNAUTHORIZED, 401, 'Missing or invalid JWT — include Authorization: Bearer '), - 403: errorExample(ErrorCode.FORBIDDEN, 403, 'Policy belongs to a different wallet'), - 404: errorExample(ErrorCode.NOT_FOUND, 404, 'Resource not found'), - 409: errorExample(ErrorCode.CONFLICT, 409, 'An active claim already exists for this policy'), - 410: errorExample(ErrorCode.GONE, 410, 'Policy has expired and is no longer accessible'), - 429: errorExample(ErrorCode.TOO_MANY_REQUESTS, 429, 'Too many requests. Please try again later.', '/api/v1/resource', 42), - 500: errorExample(ErrorCode.INTERNAL_ERROR, 500, 'An unexpected error occurred'), - 503: errorExample(ErrorCode.SERVICE_UNAVAILABLE, 503, 'A downstream dependency is currently unavailable'), + 400: errorExample( + ErrorCode.VALIDATION_ERROR, + 400, + 'Bad Request', + 'wallet must be a string; productId should not be empty', + '/api/v1/resource', + undefined, + ['wallet must be a string', 'productId should not be empty'], + ), + '400_bad': errorExample(ErrorCode.BAD_REQUEST, 400, 'Bad Request', 'Malformed request parameter'), + 401: errorExample(ErrorCode.UNAUTHORIZED, 401, 'Unauthorized', 'Missing or invalid authentication'), + 403: errorExample(ErrorCode.FORBIDDEN, 403, 'Forbidden', 'Policy belongs to a different wallet'), + 404: errorExample(ErrorCode.NOT_FOUND, 404, 'Not Found', 'Resource not found'), + 409: errorExample(ErrorCode.CONFLICT, 409, 'Conflict', 'An active claim already exists for this policy'), + 410: errorExample(ErrorCode.GONE, 410, 'Gone', 'Policy has expired and is no longer accessible'), + 429: errorExample(ErrorCode.TOO_MANY_REQUESTS, 429, 'Too Many Requests', 'Too many requests. Please try again later.', '/api/v1/resource', 42), + 500: errorExample(ErrorCode.INTERNAL_ERROR, 500, 'Internal Server Error', 'An unexpected error occurred. Please try again later.'), + 503: errorExample(ErrorCode.SERVICE_UNAVAILABLE, 503, 'Service Unavailable', 'A downstream dependency is currently unavailable'), }; diff --git a/src/health/dto/health-response.dto.ts b/src/health/dto/health-response.dto.ts index 6cfb067..6226e8e 100644 --- a/src/health/dto/health-response.dto.ts +++ b/src/health/dto/health-response.dto.ts @@ -13,6 +13,12 @@ export class DatabasePoolDto { @ApiProperty({ description: 'Configured maximum pool size (DATABASE_CONNECTION_LIMIT, default 10)', example: 10 }) max: number; + @ApiProperty({ description: 'Configured query wait timeout in seconds (DATABASE_POOL_TIMEOUT, default 10)', example: 10, required: false }) + poolTimeout?: number; + + @ApiProperty({ description: 'Configured connection acquisition timeout in seconds (DATABASE_CONNECT_TIMEOUT, default 5)', example: 5, required: false }) + connectTimeout?: number; + @ApiProperty({ description: 'Active connections as a percentage of the configured max pool size', example: 30 }) utilizationPercent: number; @@ -21,8 +27,16 @@ export class DatabasePoolDto { example: false, }) exhausted: boolean; + + @ApiProperty({ + description: 'Prisma internal engine pool metrics if available in runtime (#676)', + required: false, + nullable: true, + }) + prismaMetrics?: unknown | null; } + export class DatabaseThroughputDto { @ApiProperty({ description: 'Total committed transactions' }) commits: number; diff --git a/src/health/health.controller.ts b/src/health/health.controller.ts index 6b17276..362dbf3 100644 --- a/src/health/health.controller.ts +++ b/src/health/health.controller.ts @@ -244,16 +244,34 @@ export class HealthController { const idle = Number(rows.find(r => r.state === 'idle')?.count ?? 0); const waiting = Number(rows.find(r => r.state === 'idle in transaction (aborted)')?.count ?? 0); - const maxConnections = Number( + const effectiveLimit = this.prisma.poolConfig?.connectionLimit ?? Number( this.config.get('DATABASE_CONNECTION_LIMIT') ?? DEFAULT_DB_POOL_MAX_CONNECTIONS, ); + const poolTimeout = this.prisma.poolConfig?.poolTimeout ?? Number( + this.config.get('DATABASE_POOL_TIMEOUT') ?? 10, + ); + const connectTimeout = this.prisma.poolConfig?.connectTimeout ?? Number( + this.config.get('DATABASE_CONNECT_TIMEOUT') ?? 5, + ); + const maxConnections = effectiveLimit; const warnPercent = Number( this.config.get('DB_POOL_EXHAUSTION_WARN_PERCENT') ?? DEFAULT_DB_POOL_EXHAUSTION_WARN_PERCENT, ); const utilizationPercent = maxConnections > 0 ? Math.round((active / maxConnections) * 100) : 0; const exhausted = utilizationPercent >= warnPercent; - - dbPool = { active, idle, waiting, max: maxConnections, utilizationPercent, exhausted }; + const prismaMetrics = await this.prisma.getMetrics(); + + dbPool = { + active, + idle, + waiting, + max: maxConnections, + poolTimeout, + connectTimeout, + utilizationPercent, + exhausted, + prismaMetrics: prismaMetrics ?? undefined, + }; if (exhausted) { dbStatus = 'error'; @@ -265,6 +283,7 @@ export class HealthController { // Non-fatal: pg_stat_activity may be restricted on managed databases. } + // #463 — Database transaction throughput monitoring // Track database transaction rate to detect performance degradation. // pg_stat_database provides commit/rollback counters; delta between diff --git a/src/main.ts b/src/main.ts index 3da54d9..0a880a3 100644 --- a/src/main.ts +++ b/src/main.ts @@ -313,28 +313,48 @@ async function bootstrap() { 'paginated JSON envelope for large result sets — see that endpoint\'s own `stream` parameter ' + 'description for details; pagination metadata is then returned via the `X-Total-Count`, `X-Page`, ' + 'and `X-Limit` response headers instead of the JSON body.\n\n' + - '## Error Response Structure\n\n' + - 'All error responses follow a consistent envelope format for reliable parsing:\n\n' + + '## Error Response Structure (RFC 7807 Problem Details)\n\n' + + 'All error responses are returned with `Content-Type: application/problem+json` conforming to ' + + '[RFC 7807](https://datatracker.ietf.org/doc/html/rfc7807). Legacy envelope fields remain ' + + 'present for backward compatibility:\n\n' + '```json\n' + '{\n' + + ' "type": "https://parashield.io/errors/not-found",\n' + + ' "title": "Not Found",\n' + + ' "status": 404,\n' + + ' "detail": "Policy not found",\n' + + ' "instance": "/api/v1/policies/abc123",\n' + ' "success": false,\n' + ' "errorCode": "NOT_FOUND",\n' + ' "error": "Policy not found",\n' + + ' "message": "Policy not found",\n' + ' "statusCode": 404,\n' + ' "path": "/api/v1/policies/abc123",\n' + ' "timestamp": "2024-01-15T10:30:00.000Z"\n' + '}\n' + '```\n\n' + - '### Error Response Fields\n\n' + + '### Standard RFC 7807 Members\n\n' + + '| Member | Type | Description |\n' + + '|---|---|---|\n' + + '| `type` | `string (URI)` | Stable URI reference identifying the error type |\n' + + '| `title` | `string` | Short human-readable summary of the problem type |\n' + + '| `status` | `number` | HTTP status code |\n' + + '| `detail` | `string` | Human-readable explanation specific to this occurrence |\n' + + '| `instance` | `string (URI)` | Relative or absolute URI identifying the specific request path |\n' + + '| `errors` | `array` | (Extension) Validation failure details from class-validator |\n' + + '| `correlationId` | `string` | (Extension) Request correlation ID when available |\n\n' + + '### Backward Compatibility Fields (Deprecated)\n\n' + '| Field | Type | Description |\n' + '|-------|------|-------------|\n' + - '| `success` | `boolean` | Always `false` for errors |\n' + - '| `errorCode` | `string` | Stable machine-readable code (see table below) - key off this, not `error` |\n' + - '| `error` | `string` or `object` | Human-readable message, or validation error details for 400s. May change between versions |\n' + - '| `statusCode` | `number` | HTTP status code |\n' + - '| `path` | `string` | The request path that produced the error |\n' + + '| `success` | `boolean` | Always `false` for errors (Deprecated: use `status`) |\n' + + '| `errorCode` | `string` | Stable machine-readable code (Deprecated: use `type`) |\n' + + '| `error` | `string` or `object` | Error message string (Deprecated: use `detail`) |\n' + + '| `message` | `string` | Human-readable message (Deprecated: use `detail`) |\n' + + '| `statusCode` | `number` | HTTP status code (Deprecated: use `status`) |\n' + + '| `path` | `string` | Request path (Deprecated: use `instance`) |\n' + '| `timestamp` | `string` | ISO-8601 UTC timestamp |\n' + - '| `retryAfter` | `number` | (Optional) Seconds to wait before retrying - present only on 429 responses |\n\n' + + '| `retryAfter` | `number` | (Optional) Seconds to wait before retrying on 429 responses |\n\n' + + '### Error Codes Reference\n\n' + '| `errorCode` | HTTP Status | When It Occurs |\n' + '|-------------|-------------|----------------|\n' + diff --git a/src/oracle/oracle.worker.ts b/src/oracle/oracle.worker.ts index 96603a1..efa342e 100644 --- a/src/oracle/oracle.worker.ts +++ b/src/oracle/oracle.worker.ts @@ -1,4 +1,4 @@ -import { Injectable, Inject, Logger } from '@nestjs/common'; +import { Injectable, Inject, Logger, Optional } from '@nestjs/common'; import { Cron, CronExpression } from '@nestjs/schedule'; import { ConfigService } from '@nestjs/config'; import { nativeToScVal } from '@stellar/stellar-sdk'; @@ -8,6 +8,7 @@ import { StellarService } from '../stellar/stellar.service'; import { PolicyStatus } from '@prisma/client'; import { PrismaService } from '../prisma/prisma.service'; import { recordWorkerHeartbeat } from '../common/worker-heartbeat'; +import { DistributedLockService } from '../redis/distributed-lock.service'; // #307 — narrows an arbitrary fetchWithRetry result for the success-path // log line without an `any` cast. Duck-typed rather than tied to a single @@ -62,18 +63,40 @@ export class OracleWorker { return Number.isFinite(parsed) && parsed >= 0 ? parsed : this.defaultMinConfidence; } + private readonly lockService: DistributedLockService; + constructor( private readonly oracleService: OracleService, private readonly config: ConfigService, private readonly stellar: StellarService, private readonly prisma: PrismaService, @Inject('REDIS_CLIENT') private readonly redis: Redis, - ) {} + @Optional() lockService?: DistributedLockService, + ) { + this.lockService = lockService ?? new DistributedLockService(this.redis); + } @Cron(CronExpression.EVERY_HOUR) async pollAndSubmit(): Promise { + const ttlMs = parseInt( + this.config?.get('ORACLE_WORKER_LOCK_TTL_MS') ?? '900000', + 10, + ); + + await this.lockService.runWithLock( + 'lock:job:oracle-worker', + ttlMs, + async () => { + await this.executePollAndSubmit(); + }, + { enableHeartbeat: true }, + ); + } + + private async executePollAndSubmit(): Promise { this.logger.log('Oracle poll cycle started'); + const now = new Date(); const year = now.getUTCFullYear(); const month = now.getUTCMonth() + 1; diff --git a/src/prisma/prisma.service.spec.ts b/src/prisma/prisma.service.spec.ts index f11f16e..8154aaa 100644 --- a/src/prisma/prisma.service.spec.ts +++ b/src/prisma/prisma.service.spec.ts @@ -88,4 +88,65 @@ describe('PrismaService', () => { await expect(service.onModuleDestroy()).rejects.toThrow(error); }); }); + + describe('connection pool configuration (#676)', () => { + const originalEnv = process.env; + + afterEach(() => { + process.env = originalEnv; + }); + + it('uses defaults when env vars and URL params are absent', () => { + delete process.env.DATABASE_CONNECTION_LIMIT; + delete process.env.DATABASE_POOL_TIMEOUT; + delete process.env.DATABASE_CONNECT_TIMEOUT; + + const prisma = new PrismaService(); + expect(prisma.poolConfig).toEqual({ + connectionLimit: 10, + poolTimeout: 10, + connectTimeout: 5, + }); + }); + + it('respects environment variable overrides', () => { + process.env.DATABASE_CONNECTION_LIMIT = '25'; + process.env.DATABASE_POOL_TIMEOUT = '30'; + process.env.DATABASE_CONNECT_TIMEOUT = '15'; + + const prisma = new PrismaService(); + expect(prisma.poolConfig).toEqual({ + connectionLimit: 25, + poolTimeout: 30, + connectTimeout: 15, + }); + }); + + it('gives explicit DATABASE_URL query parameters precedence over env vars', () => { + process.env.DATABASE_CONNECTION_LIMIT = '25'; + process.env.DATABASE_POOL_TIMEOUT = '30'; + process.env.DATABASE_CONNECT_TIMEOUT = '15'; + process.env.DATABASE_URL = + 'postgresql://user:pass@localhost:5432/db?connection_limit=50&pool_timeout=45&connect_timeout=8'; + + const prisma = new PrismaService(); + expect(prisma.poolConfig).toEqual({ + connectionLimit: 50, + poolTimeout: 45, + connectTimeout: 8, + }); + }); + + it('logs effective pool configuration on module init', async () => { + jest.spyOn(service, '$connect').mockResolvedValue(undefined); + const logSpy = jest.spyOn(service['logger'], 'log'); + + await service.onModuleInit(); + + expect(logSpy).toHaveBeenCalledWith( + expect.stringContaining('Prisma connection pool configured: connection_limit='), + ); + }); + }); }); + diff --git a/src/prisma/prisma.service.ts b/src/prisma/prisma.service.ts index 3ecfcf2..0fefc76 100644 --- a/src/prisma/prisma.service.ts +++ b/src/prisma/prisma.service.ts @@ -10,6 +10,45 @@ const DEFAULT_CONNECTION_LIMIT = '10'; const DEFAULT_POOL_TIMEOUT_SECONDS = '10'; const DEFAULT_CONNECT_TIMEOUT_SECONDS = '5'; +export interface PrismaPoolConfig { + connectionLimit: number; + poolTimeout: number; + connectTimeout: number; +} + +/** + * Determine the effective pool configuration. Explicit query parameters in the + * connection URL take precedence over environment variables. + */ +function getEffectivePoolConfig(url: string | undefined): PrismaPoolConfig { + let connectionLimit = process.env.DATABASE_CONNECTION_LIMIT || DEFAULT_CONNECTION_LIMIT; + let poolTimeout = process.env.DATABASE_POOL_TIMEOUT || DEFAULT_POOL_TIMEOUT_SECONDS; + let connectTimeout = process.env.DATABASE_CONNECT_TIMEOUT || DEFAULT_CONNECT_TIMEOUT_SECONDS; + + if (url) { + try { + const parsed = new URL(url); + if (parsed.searchParams.has('connection_limit')) { + connectionLimit = parsed.searchParams.get('connection_limit')!; + } + if (parsed.searchParams.has('pool_timeout')) { + poolTimeout = parsed.searchParams.get('pool_timeout')!; + } + if (parsed.searchParams.has('connect_timeout')) { + connectTimeout = parsed.searchParams.get('connect_timeout')!; + } + } catch { + // Ignore URL parsing errors + } + } + + return { + connectionLimit: parseInt(connectionLimit, 10), + poolTimeout: parseInt(poolTimeout, 10), + connectTimeout: parseInt(connectTimeout, 10), + }; +} + /** * Append Prisma connection-pool parameters to the datasource URL. * Parameters already present in the URL are left untouched, and URLs that @@ -56,15 +95,20 @@ export class PrismaService extends PrismaClient implements OnModuleInit, OnModul // writes and transactions are never affected. private readonly queryCache = new Map(); + public readonly poolConfig: PrismaPoolConfig; + constructor() { super({ datasourceUrl: withConnectionPoolParams(process.env.DATABASE_URL) }); + this.poolConfig = getEffectivePoolConfig(process.env.DATABASE_URL); + const replicaUrl = withConnectionPoolParams(process.env.DATABASE_REPLICA_URL); this.replicaClient = replicaUrl ? new PrismaClient({ datasourceUrl: replicaUrl }) : null; } + /** * Returns the read-replica PrismaClient when DATABASE_REPLICA_URL is * configured, otherwise returns the primary client. @@ -91,7 +135,12 @@ export class PrismaService extends PrismaClient implements OnModuleInit, OnModul this.logger.log('Read-replica connection established'); } this.logger.log('Database connection established'); + this.logger.log( + `Prisma connection pool configured: connection_limit=${this.poolConfig.connectionLimit}, ` + + `pool_timeout=${this.poolConfig.poolTimeout}s, connect_timeout=${this.poolConfig.connectTimeout}s`, + ); return; + } catch (err) { const errorMsg = err instanceof Error ? err.message : String(err); this.logger.warn( @@ -167,4 +216,20 @@ export class PrismaService extends PrismaClient implements OnModuleInit, OnModul } } } + + /** + * Expose Prisma internal engine pool metrics if supported by the installed + * Prisma engine/client (#676). Returns null if metrics preview is unavailable. + */ + async getMetrics(): Promise { + try { + if (typeof (this as any).$metrics?.json === 'function') { + return await (this as any).$metrics.json(); + } + } catch (err) { + this.logger.warn(`Failed to collect Prisma metrics: ${err instanceof Error ? err.message : String(err)}`); + } + return null; + } } + diff --git a/src/redis/distributed-lock.service.spec.ts b/src/redis/distributed-lock.service.spec.ts new file mode 100644 index 0000000..3a78b79 --- /dev/null +++ b/src/redis/distributed-lock.service.spec.ts @@ -0,0 +1,252 @@ +import { DistributedLockService } from './distributed-lock.service'; +import type Redis from 'ioredis'; + +describe('DistributedLockService', () => { + // Faithful in-memory Redis simulation supporting SET NX PX, GET, DEL, PEXPIRE, and Lua eval + class FakeRedis { + private store = new Map(); + public failMode = false; + + async set(key: string, value: string, px?: string, ttlMs?: number, nx?: string): Promise { + if (this.failMode) throw new Error('Redis connection failed'); + const now = Date.now(); + const current = this.store.get(key); + if (current && current.expiresAt > now && nx === 'NX') { + return null; // Key exists and has not expired + } + const expiresAt = ttlMs ? now + ttlMs : Infinity; + this.store.set(key, { value, expiresAt }); + return 'OK'; + } + + async get(key: string): Promise { + if (this.failMode) throw new Error('Redis connection failed'); + const now = Date.now(); + const current = this.store.get(key); + if (!current) return null; + if (current.expiresAt <= now) { + this.store.delete(key); + return null; + } + return current.value; + } + + async del(key: string): Promise { + if (this.failMode) throw new Error('Redis connection failed'); + return this.store.delete(key) ? 1 : 0; + } + + async eval(script: string, numKeys: number, key: string, ...args: any[]): Promise { + if (this.failMode) throw new Error('Redis connection failed'); + const now = Date.now(); + const current = this.store.get(key); + const isExpired = current ? current.expiresAt <= now : true; + if (isExpired && current) this.store.delete(key); + + const currentValue = isExpired ? null : current!.value; + const targetToken = args[0]; + + // Compare-and-delete Lua script simulation + if (script.includes('DEL')) { + if (currentValue === targetToken) { + this.store.delete(key); + return 1; + } + return 0; + } + + // Renew (PEXPIRE) Lua script simulation + if (script.includes('PEXPIRE')) { + if (currentValue === targetToken) { + const newTtlMs = Number(args[1]); + this.store.set(key, { value: currentValue, expiresAt: now + newTtlMs }); + return 1; + } + return 0; + } + + return 0; + } + + clear() { + this.store.clear(); + this.failMode = false; + } + } + + let fakeRedis: FakeRedis; + let lockService: DistributedLockService; + + beforeEach(() => { + fakeRedis = new FakeRedis(); + lockService = new DistributedLockService(fakeRedis as unknown as Redis); + }); + + describe('Distributed locking semantics', () => { + it('only one of two concurrent runners executes, and the second skips', async () => { + let executionCount = 0; + const job = jest.fn(async () => { + executionCount++; + // Small delay to simulate work + await new Promise((resolve) => setTimeout(resolve, 50)); + return 'success'; + }); + + // Launch two concurrent runs + const [run1, run2] = await Promise.all([ + lockService.runWithLock('lock:test:concurrent', 10000, job), + lockService.runWithLock('lock:test:concurrent', 10000, job), + ]); + + const executedRuns = [run1, run2].filter((r) => r.executed); + const skippedRuns = [run1, run2].filter((r) => !r.executed); + + expect(executedRuns).toHaveLength(1); + expect(skippedRuns).toHaveLength(1); + expect(executionCount).toBe(1); + expect(job).toHaveBeenCalledTimes(1); + }); + + it('the lock is released after successful execution', async () => { + const task = jest.fn(async () => 'done'); + + const firstRun = await lockService.runWithLock('lock:test:release-success', 10000, task); + expect(firstRun.executed).toBe(true); + + // Lock should now be free, second acquire succeeds + const token = await lockService.acquire('lock:test:release-success', 10000); + expect(token).not.toBeNull(); + await lockService.release('lock:test:release-success', token!); + }); + + it('the lock is released after failure', async () => { + const failingTask = jest.fn(async () => { + throw new Error('Task failed unexpectedly'); + }); + + await expect( + lockService.runWithLock('lock:test:release-failure', 10000, failingTask), + ).rejects.toThrow('Task failed unexpectedly'); + + // Lock should be released in finally block despite the exception + const token = await lockService.acquire('lock:test:release-failure', 10000); + expect(token).not.toBeNull(); + await lockService.release('lock:test:release-failure', token!); + }); + + it('the lock expires if the holder dies (TTL expiration)', async () => { + jest.useFakeTimers(); + try { + const token = await lockService.acquire('lock:test:ttl', 1000); + expect(token).not.toBeNull(); + + // Second acquisition immediately fails + const secondAcquire = await lockService.acquire('lock:test:ttl', 1000); + expect(secondAcquire).toBeNull(); + + // Simulate holder crashing and time elapsing past TTL (1001ms) + jest.advanceTimersByTime(1001); + + // Another instance should now acquire successfully + const thirdAcquire = await lockService.acquire('lock:test:ttl', 1000); + expect(thirdAcquire).not.toBeNull(); + } finally { + jest.useRealTimers(); + } + }); + + it('a stale holder cannot release a newer holder\'s lock', async () => { + jest.useFakeTimers(); + try { + // Holder 1 acquires with token1 + const token1 = await lockService.acquire('lock:test:stale', 1000); + expect(token1).not.toBeNull(); + + // Lock expires naturally for holder 1 + jest.advanceTimersByTime(1500); + + // Holder 2 acquires with token2 + const token2 = await lockService.acquire('lock:test:stale', 5000); + expect(token2).not.toBeNull(); + expect(token2).not.toEqual(token1); + + // Stale holder 1 attempts to release with token1 + const releasedByStale = await lockService.release('lock:test:stale', token1!); + expect(releasedByStale).toBe(false); + + // Holder 2's lock must still be active in Redis + const remainingHolderToken = await fakeRedis.get('lock:test:stale'); + expect(remainingHolderToken).toBe(token2); + } finally { + jest.useRealTimers(); + } + }); + + it('multi-instance case: two separate lock-client instances against the same Redis store', async () => { + // Two distinct service instances simulating two application replicas + const instanceA = new DistributedLockService(fakeRedis as unknown as Redis); + const instanceB = new DistributedLockService(fakeRedis as unknown as Redis); + + // Instance A acquires + const tokenA = await instanceA.acquire('lock:cluster:job', 5000); + expect(tokenA).not.toBeNull(); + + // Instance B attempts to acquire same key, fails + const tokenB = await instanceB.acquire('lock:cluster:job', 5000); + expect(tokenB).toBeNull(); + + // Instance A releases + const released = await instanceA.release('lock:cluster:job', tokenA!); + expect(released).toBe(true); + + // Instance B now acquires successfully + const tokenB2 = await instanceB.acquire('lock:cluster:job', 5000); + expect(tokenB2).not.toBeNull(); + + await instanceB.release('lock:cluster:job', tokenB2!); + }); + + it('when Redis is unreachable, skips execution and logs error loudly', async () => { + fakeRedis.failMode = true; // Simulate Redis outage + + const task = jest.fn(); + const result = await lockService.runWithLock('lock:test:outage', 5000, task); + + expect(result.executed).toBe(false); + expect(task).not.toHaveBeenCalled(); + }); + + it('watchdog heartbeat extends the lock for long-running jobs', async () => { + jest.useFakeTimers(); + try { + const renewSpy = jest.spyOn(lockService, 'renew'); + + let completeTask: () => void = () => {}; + const longRunningPromise = new Promise((resolve) => { + completeTask = () => resolve('finished'); + }); + + const executionPromise = lockService.runWithLock( + 'lock:test:heartbeat', + 3000, + () => longRunningPromise, + { enableHeartbeat: true, extendIntervalMs: 1000 }, + ); + + // Advance timers by 1000ms -> heartbeat fires + jest.advanceTimersByTime(1000); + expect(renewSpy).toHaveBeenCalledTimes(1); + + // Advance timers by another 1000ms -> heartbeat fires again + jest.advanceTimersByTime(1000); + expect(renewSpy).toHaveBeenCalledTimes(2); + + // Complete the task + completeTask(); + await executionPromise; + } finally { + jest.useRealTimers(); + } + }); + }); +}); diff --git a/src/redis/distributed-lock.service.ts b/src/redis/distributed-lock.service.ts new file mode 100644 index 0000000..638f78b --- /dev/null +++ b/src/redis/distributed-lock.service.ts @@ -0,0 +1,170 @@ +import { Injectable, Inject, Logger } from '@nestjs/common'; +import type Redis from 'ioredis'; +import * as crypto from 'crypto'; + +export interface LockOptions { + ttlMs?: number; + extendIntervalMs?: number; + enableHeartbeat?: boolean; +} + +/** + * DistributedLockService — provides distributed locking using Redis primitives (#675). + * + * Implements safe distributed locking across multiple instances: + * - Acquire: SET key token NX PX ttl, where token is a random per-acquisition string. + * - Release: Atomic compare-and-delete Lua script (never blind DEL) so a stale + * holder can never release another instance's lock. + * - Renewal: Heartbeat/watchdog extends the lock periodically while a long-running + * job is executing. + * - Fail-safe: If Redis is unreachable, skips execution and logs error loudly + * to avoid data corruption / duplicate transactions. + */ +@Injectable() +export class DistributedLockService { + private readonly logger = new Logger(DistributedLockService.name); + + // Atomic compare-and-delete Lua script: + // Deletes the key ONLY if the currently stored value matches the token. + private static readonly COMPARE_AND_DELETE_LUA = ` + if redis.call("GET", KEYS[1]) == ARGV[1] then + return redis.call("DEL", KEYS[1]) + else + return 0 + end + `; + + // Atomic lock renewal Lua script: + // Extends the PEXPIRE ttl ONLY if the currently stored value matches the token. + private static readonly RENEW_LOCK_LUA = ` + if redis.call("GET", KEYS[1]) == ARGV[1] then + return redis.call("PEXPIRE", KEYS[1], ARGV[2]) + else + return 0 + end + `; + + constructor(@Inject('REDIS_CLIENT') private readonly redis: Redis) {} + + /** + * Acquire a distributed lock. + * + * @param key Lock identifier (e.g. 'lock:job:oracle-worker') + * @param ttlMs Lock time-to-live in milliseconds + * @returns Generated acquisition token if acquired, or null if lock is held or Redis is down. + */ + async acquire(key: string, ttlMs: number): Promise { + const token = typeof crypto.randomUUID === 'function' + ? crypto.randomUUID() + : crypto.randomBytes(16).toString('hex'); + + try { + // SET key token PX ttlMs NX: sets key only if it does not already exist + const result = await this.redis.set(key, token, 'PX', ttlMs, 'NX'); + return result === 'OK' ? token : null; + } catch (err) { + // Fail-safe decision: For financial and blockchain background jobs + // (oracle submissions, claims processing, payouts), running concurrently + // can cause irreversible state corruption and duplicate transactions. + // If Redis is unreachable, we skip the run and log loudly. + this.logger.error( + `[DistributedLock] Redis unreachable during lock acquisition for "${key}": ` + + `${err instanceof Error ? err.message : String(err)}. Skipping execution to prevent data corruption.`, + ); + return null; + } + } + + /** + * Release the distributed lock. + * + * Uses an atomic Lua script that compares the stored token with our token + * before deleting. Returns true if released, false if the token did not match + * (e.g. lock expired and was acquired by another instance). + */ + async release(key: string, token: string): Promise { + try { + const result = await this.redis.eval( + DistributedLockService.COMPARE_AND_DELETE_LUA, + 1, + key, + token, + ); + return Number(result) === 1; + } catch (err) { + this.logger.warn( + `[DistributedLock] Error releasing lock "${key}": ${err instanceof Error ? err.message : String(err)}`, + ); + return false; + } + } + + /** + * Extend lock expiration while a job is running. + */ + async renew(key: string, token: string, ttlMs: number): Promise { + try { + const result = await this.redis.eval( + DistributedLockService.RENEW_LOCK_LUA, + 1, + key, + token, + ttlMs, + ); + return Number(result) === 1; + } catch (err) { + this.logger.warn( + `[DistributedLock] Error renewing lock "${key}": ${err instanceof Error ? err.message : String(err)}`, + ); + return false; + } + } + + /** + * Run a task exclusively under a distributed lock. + * + * - If lock not acquired: logs that the run was skipped because lock is held (no spinning/queueing). + * - If lock acquired: runs task, maintains heartbeat renewal if enabled, releases in finally block. + */ + async runWithLock( + key: string, + ttlMs: number, + task: () => Promise, + options?: LockOptions, + ): Promise<{ executed: boolean; result?: T }> { + const token = await this.acquire(key, ttlMs); + if (!token) { + this.logger.log(`Skipping scheduled run: lock "${key}" is currently held by another instance`); + return { executed: false }; + } + + let heartbeatTimer: NodeJS.Timeout | undefined; + const enableHeartbeat = options?.enableHeartbeat ?? true; + const extendIntervalMs = options?.extendIntervalMs ?? Math.floor(ttlMs / 3); + + if (enableHeartbeat && extendIntervalMs > 0) { + heartbeatTimer = setInterval(async () => { + const renewed = await this.renew(key, token, ttlMs); + if (!renewed) { + this.logger.warn( + `[DistributedLock] Watchdog heartbeat failed to renew lock "${key}". Holder token may have expired.`, + ); + } + }, extendIntervalMs); + + if (typeof heartbeatTimer.unref === 'function') { + heartbeatTimer.unref(); + } + } + + try { + const result = await task(); + return { executed: true, result }; + } finally { + if (heartbeatTimer) { + clearInterval(heartbeatTimer); + } + await this.release(key, token); + } + } +} diff --git a/src/redis/redis.module.ts b/src/redis/redis.module.ts index ff9fc54..41c8723 100644 --- a/src/redis/redis.module.ts +++ b/src/redis/redis.module.ts @@ -1,6 +1,7 @@ import { Module, Global } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import Redis from 'ioredis'; +import { DistributedLockService } from './distributed-lock.service'; @Global() @Module({ @@ -32,7 +33,9 @@ import Redis from 'ioredis'; }, inject: [ConfigService], }, + DistributedLockService, ], - exports: ['REDIS_CLIENT'], + exports: ['REDIS_CLIENT', DistributedLockService], }) export class RedisModule {} +