diff --git a/apps/portal-bff/.env.example b/apps/portal-bff/.env.example index d4b9c1c..ebfd995 100644 --- a/apps/portal-bff/.env.example +++ b/apps/portal-bff/.env.example @@ -127,6 +127,19 @@ LOG_USER_ID_SALT=replace_with_32_random_bytes_base64url # portal-admin grows its own dev server. CORS_ALLOWED_ORIGINS=http://localhost:4200 +# Rate limiting (per ADR-0015 §"DoS mitigation"). Both optional +# with conservative defaults; override per environment when traffic +# patterns demand it. The BFF keys buckets by session id when the +# request is authenticated, by remote IP otherwise — rotating +# sessions doesn't dodge the limit. +# RATE_LIMIT_PER_MINUTE — general bucket. Default 120 (~ 2 r/s). +# RATE_LIMIT_AUTH_PER_MINUTE — stricter bucket on /auth/login and +# /auth/callback. Default 10/min, to +# slow brute-force / replay loops +# without inconveniencing legit users. +# RATE_LIMIT_PER_MINUTE=120 +# RATE_LIMIT_AUTH_PER_MINUTE=10 + # Future env vars introduced by upcoming phases / ADRs: # # Auth flow (ADR-0009) — additional keys wired as the routes land: diff --git a/apps/portal-bff/src/auth/auth.controller.spec.ts b/apps/portal-bff/src/auth/auth.controller.spec.ts index 73c4d87..98d4d01 100644 --- a/apps/portal-bff/src/auth/auth.controller.spec.ts +++ b/apps/portal-bff/src/auth/auth.controller.spec.ts @@ -502,15 +502,25 @@ describe('AuthController.me', () => { expect(payload).not.toHaveProperty('amr'); }); - it('returns 401 when no user is on the session', () => { + it('throws UnauthorizedException with code=unauthenticated when no user is on the session', () => { + // Controller now relies on the global StructuredErrorFilter to + // serialise the response. The `code` lives on the + // HttpException's response object so the filter picks it up + // verbatim into the `{ error: { code } }` envelope. const { controller } = makeController(); const res = makeResStub(); const req = makeReqStub(); - controller.me(req, res); - - expect(res.status).toHaveBeenCalledWith(401); - expect(res.json).toHaveBeenCalledWith({ error: 'unauthenticated' }); + expect(() => controller.me(req, res)).toThrow( + expect.objectContaining({ + status: 401, + response: expect.objectContaining({ + code: 'unauthenticated', + message: 'Unauthenticated', + }), + }), + ); + expect(res.json).not.toHaveBeenCalled(); }); }); diff --git a/apps/portal-bff/src/auth/auth.controller.ts b/apps/portal-bff/src/auth/auth.controller.ts index b2d7edb..602c36e 100644 --- a/apps/portal-bff/src/auth/auth.controller.ts +++ b/apps/portal-bff/src/auth/auth.controller.ts @@ -1,5 +1,5 @@ import { randomBytes } from 'node:crypto'; -import { Controller, Get, Inject, Query, Req, Res } from '@nestjs/common'; +import { Controller, Get, Inject, Query, Req, Res, UnauthorizedException } from '@nestjs/common'; import type { Request, Response } from 'express'; import { Logger } from 'nestjs-pino'; import { AuditWriter } from '../audit/audit.service'; @@ -189,8 +189,15 @@ export class AuthController { me(@Req() req: Request, @Res() res: Response): void { const user = req.session.user; if (!user) { - res.status(401).json({ error: 'unauthenticated' }); - return; + // Throw rather than write the response directly: the global + // `StructuredErrorFilter` then formats it as the shared + // `{ error: { code, message, traceId } }` envelope. Code is + // set via the response-object form of HttpException so the + // filter picks it up verbatim. + throw new UnauthorizedException({ + code: 'unauthenticated', + message: 'Unauthenticated', + }); } res.json(toPublicUser(user)); } diff --git a/apps/portal-bff/src/main.ts b/apps/portal-bff/src/main.ts index 201c78a..ce4f8c6 100644 --- a/apps/portal-bff/src/main.ts +++ b/apps/portal-bff/src/main.ts @@ -16,7 +16,9 @@ import { assertRedisConfig } from './config/check-redis-config'; import { assertLogUserIdSalt } from './config/check-log-user-id-salt'; import { assertSessionEncryptionKey } from './config/check-session-encryption-key'; import { assertSessionSecret } from './config/check-session-secret'; +import { createRateLimitMiddleware, readRateLimitConfig } from './security/rate-limit.middleware'; import { CSRF_MIDDLEWARE } from './security/security.token'; +import { StructuredErrorFilter } from './security/structured-error.filter'; import { SESSION_ABSOLUTE_TIMEOUT_MIDDLEWARE, SESSION_MIDDLEWARE, @@ -60,6 +62,15 @@ async function bootstrap() { const app = await NestFactory.create(AppModule, { bufferLogs: true }); app.useLogger(app.get(Logger)); + // Global exception filter — normalises every 4xx/5xx response to + // `{ error: { code, message, traceId } }`. The Nest default + // serialises HttpException's getResponse() at the top level, + // which leaks the class name on 500s and produces an + // inconsistent shape across exception types. Registering early + // (before request middleware mounts) ensures even errors thrown + // during route setup are caught. + app.useGlobalFilters(new StructuredErrorFilter(app.get(Logger))); + // Security headers (phase-2). Defaults from `helmet()` are good // for an API server returning JSON: X-Frame-Options=SAMEORIGIN, // X-Content-Type-Options=nosniff, Referrer-Policy=no-referrer, @@ -129,6 +140,16 @@ async function bootstrap() { // needed). app.use(app.get(SESSION_ABSOLUTE_TIMEOUT_MIDDLEWARE)); + // Rate limiting (ADR-0015 §"DoS mitigation" + phase-2 follow-up). + // Mounted after the session middleware so the bucket key falls + // back to the session id for authenticated requests (preventing + // a single attacker from rotating sessions to dodge the limit) + // and to the remote IP otherwise. Default 120/min general, 10/min + // on `/auth/login` and `/auth/callback` to slow brute-force / + // replay attempts. `/api/health` is skipped — orchestrator polls + // shouldn't burn the user quota. + app.use(createRateLimitMiddleware(readRateLimitConfig())); + // Double-submit CSRF (ADR-0009 §"CSRF defense"). Mounted after // the session middleware so `req.session.csrfToken` is available // for comparison with the `X-CSRF-Token` request header. Skips @@ -137,10 +158,6 @@ async function bootstrap() { // `/auth/callback`). app.use(app.get(CSRF_MIDDLEWARE)); - // Phase-2 hardening still pending: rate limiting + structured - // error filter (helmet, CORS allowlist, CSRF protection landed - // with this PR). - const globalPrefix = 'api'; app.setGlobalPrefix(globalPrefix); const port = process.env['PORT'] ?? 3000; diff --git a/apps/portal-bff/src/security/csrf.middleware.spec.ts b/apps/portal-bff/src/security/csrf.middleware.spec.ts index 887fbc1..b29b53b 100644 --- a/apps/portal-bff/src/security/csrf.middleware.spec.ts +++ b/apps/portal-bff/src/security/csrf.middleware.spec.ts @@ -100,7 +100,14 @@ describe('csrf middleware', () => { const res = makeRes(); csrf(req, res, next); expect(res.status).toHaveBeenCalledWith(403); - expect(res.json).toHaveBeenCalledWith({ error: 'csrf' }); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.objectContaining({ + code: 'csrf', + message: 'CSRF token missing or invalid', + }), + }), + ); expect(next).not.toHaveBeenCalled(); expect(logger.warn).toHaveBeenCalledWith( expect.objectContaining({ event: 'csrf.reject', hasHeaderToken: false }), diff --git a/apps/portal-bff/src/security/csrf.middleware.ts b/apps/portal-bff/src/security/csrf.middleware.ts index 71d3199..2ce4275 100644 --- a/apps/portal-bff/src/security/csrf.middleware.ts +++ b/apps/portal-bff/src/security/csrf.middleware.ts @@ -1,6 +1,7 @@ import { timingSafeEqual } from 'node:crypto'; import type { NextFunction, Request, RequestHandler, Response } from 'express'; import { Logger } from 'nestjs-pino'; +import { errorResponse } from './structured-error.filter'; /** * Methods that, per RFC 7231 / 9110, MUST NOT alter server state. @@ -81,7 +82,7 @@ export function createCsrfMiddleware(logger: Logger): RequestHandler { }, 'Csrf', ); - res.status(403).json({ error: 'csrf' }); + res.status(403).json(errorResponse('csrf', 'CSRF token missing or invalid')); return; } diff --git a/apps/portal-bff/src/security/rate-limit.middleware.spec.ts b/apps/portal-bff/src/security/rate-limit.middleware.spec.ts new file mode 100644 index 0000000..14f9641 --- /dev/null +++ b/apps/portal-bff/src/security/rate-limit.middleware.spec.ts @@ -0,0 +1,164 @@ +import type { NextFunction, Request, Response } from 'express'; +import { createRateLimitMiddleware, readRateLimitConfig } from './rate-limit.middleware'; + +function makeReq(opts: { + ip?: string; + path: string; + sessionID?: string; + session?: { user?: { oid: string } }; +}): Request { + return { + ip: opts.ip ?? '1.2.3.4', + path: opts.path, + sessionID: opts.sessionID, + session: opts.session, + headers: {}, + method: 'POST', + get: () => undefined, + } as unknown as Request; +} + +function makeRes(): Response & { status: jest.Mock; json: jest.Mock; setHeader: jest.Mock } { + const res = { + status: jest.fn(), + json: jest.fn(), + setHeader: jest.fn(), + getHeader: jest.fn(), + headersSent: false, + }; + res.status.mockReturnValue(res); + res.json.mockReturnValue(res); + return res as unknown as Response & { + status: jest.Mock; + json: jest.Mock; + setHeader: jest.Mock; + }; +} + +describe('readRateLimitConfig', () => { + const origGeneral = process.env['RATE_LIMIT_PER_MINUTE']; + const origAuth = process.env['RATE_LIMIT_AUTH_PER_MINUTE']; + + afterEach(() => { + restore('RATE_LIMIT_PER_MINUTE', origGeneral); + restore('RATE_LIMIT_AUTH_PER_MINUTE', origAuth); + }); + + it('returns conservative defaults when env is unset', () => { + delete process.env['RATE_LIMIT_PER_MINUTE']; + delete process.env['RATE_LIMIT_AUTH_PER_MINUTE']; + expect(readRateLimitConfig()).toEqual({ perMinute: 120, authPerMinute: 10 }); + }); + + it('parses positive integer overrides', () => { + process.env['RATE_LIMIT_PER_MINUTE'] = '300'; + process.env['RATE_LIMIT_AUTH_PER_MINUTE'] = '5'; + expect(readRateLimitConfig()).toEqual({ perMinute: 300, authPerMinute: 5 }); + }); + + it('rejects non-positive integers', () => { + process.env['RATE_LIMIT_PER_MINUTE'] = '0'; + expect(() => readRateLimitConfig()).toThrow(/must be a positive integer/); + }); + + it('rejects non-numeric values', () => { + process.env['RATE_LIMIT_PER_MINUTE'] = 'fast'; + expect(() => readRateLimitConfig()).toThrow(/must be a positive integer/); + }); +}); + +describe('createRateLimitMiddleware', () => { + it('lets /api/health through without counting', async () => { + const mw = createRateLimitMiddleware({ perMinute: 1, authPerMinute: 1 }); + const next = jest.fn() as unknown as NextFunction; + // Hammer /api/health far above the limit — still passes. + for (let i = 0; i < 5; i++) { + (next as jest.Mock).mockClear(); + const res = makeRes(); + await mw(makeReq({ path: '/api/health' }), res, next); + expect(next).toHaveBeenCalledTimes(1); + expect(res.status).not.toHaveBeenCalledWith(429); + } + }); + + it('enforces the general bucket: passes up to perMinute then 429s', async () => { + const mw = createRateLimitMiddleware({ perMinute: 2, authPerMinute: 99 }); + const next = jest.fn() as unknown as NextFunction; + const ip = '10.0.0.1'; + // First two: pass. + for (let i = 0; i < 2; i++) { + (next as jest.Mock).mockClear(); + const res = makeRes(); + await mw(makeReq({ ip, path: '/api/whatever' }), res, next); + expect(next).toHaveBeenCalled(); + } + // Third: rejected. + const res = makeRes(); + await mw(makeReq({ ip, path: '/api/whatever' }), res, next); + expect(res.status).toHaveBeenCalledWith(429); + const body = res.json.mock.calls[0]?.[0] as { error: { code: string; message: string } }; + expect(body.error.code).toBe('rate_limited'); + expect(body.error.message).toBe('Too many requests'); + }); + + it('enforces the stricter bucket on /api/auth/login', async () => { + const mw = createRateLimitMiddleware({ perMinute: 99, authPerMinute: 1 }); + const next = jest.fn() as unknown as NextFunction; + const ip = '10.0.0.2'; + // First /login: pass. + let res = makeRes(); + await mw(makeReq({ ip, path: '/api/auth/login' }), res, next); + expect(res.status).not.toHaveBeenCalledWith(429); + // Second /login: blocked (authPerMinute=1). + res = makeRes(); + await mw(makeReq({ ip, path: '/api/auth/login' }), res, next); + expect(res.status).toHaveBeenCalledWith(429); + }); + + it('keys per-session when authenticated, isolating buckets', async () => { + const mw = createRateLimitMiddleware({ perMinute: 1, authPerMinute: 99 }); + const next = jest.fn() as unknown as NextFunction; + const session = { user: { oid: 'u1' } }; + + // session sid-A: 1 hit allowed + let res = makeRes(); + await mw(makeReq({ path: '/api/x', sessionID: 'sid-A', session }), res, next); + expect(res.status).not.toHaveBeenCalledWith(429); + // session sid-A: 2nd hit blocked + res = makeRes(); + await mw(makeReq({ path: '/api/x', sessionID: 'sid-A', session }), res, next); + expect(res.status).toHaveBeenCalledWith(429); + + // session sid-B: independent bucket, 1st hit still allowed + res = makeRes(); + await mw(makeReq({ path: '/api/x', sessionID: 'sid-B', session }), res, next); + expect(res.status).not.toHaveBeenCalledWith(429); + }); + + it('keys per-IP when anonymous (no req.session.user)', async () => { + const mw = createRateLimitMiddleware({ perMinute: 1, authPerMinute: 99 }); + const next = jest.fn() as unknown as NextFunction; + + let res = makeRes(); + await mw(makeReq({ ip: '10.0.0.10', path: '/api/x' }), res, next); + expect(res.status).not.toHaveBeenCalledWith(429); + + // Same IP, 2nd hit blocked. + res = makeRes(); + await mw(makeReq({ ip: '10.0.0.10', path: '/api/x' }), res, next); + expect(res.status).toHaveBeenCalledWith(429); + + // Different IP, independent bucket. + res = makeRes(); + await mw(makeReq({ ip: '10.0.0.11', path: '/api/x' }), res, next); + expect(res.status).not.toHaveBeenCalledWith(429); + }); +}); + +function restore(name: string, value: string | undefined): void { + if (value === undefined) { + delete process.env[name]; + } else { + process.env[name] = value; + } +} diff --git a/apps/portal-bff/src/security/rate-limit.middleware.ts b/apps/portal-bff/src/security/rate-limit.middleware.ts new file mode 100644 index 0000000..5d579fd --- /dev/null +++ b/apps/portal-bff/src/security/rate-limit.middleware.ts @@ -0,0 +1,105 @@ +import type { NextFunction, Request, RequestHandler, Response } from 'express'; +import rateLimit, { type Options } from 'express-rate-limit'; +import { errorResponse } from './structured-error.filter'; + +/** + * Defaults from ADR-0015's "denial-of-service mitigation" line and + * the phase-2 placeholder note in `main.ts`. Concrete numbers are + * deliberately conservative; staging / prod can tighten through + * env without code changes. + * + * RATE_LIMIT_PER_MINUTE — general bucket. 120 ≈ 2 r/s. + * RATE_LIMIT_AUTH_PER_MINUTE — stricter on `/auth/login` and + * `/auth/callback` to slow + * brute-force / replay loops. + * 10/min is plenty for legit users. + */ +const DEFAULT_RATE_LIMIT_PER_MINUTE = 120; +const DEFAULT_RATE_LIMIT_AUTH_PER_MINUTE = 10; +const WINDOW_MS = 60_000; + +/** Auth-flow prefixes for which the stricter bucket applies. */ +const STRICT_PATHS = ['/api/auth/login', '/api/auth/callback']; + +/** + * Paths the rate-limit middleware lets through unconditionally. + * `/api/health` is polled by container orchestrators and load + * balancers — counting those against a user quota is meaningless. + */ +const SKIP_PATHS = ['/api/health']; + +export interface RateLimitConfig { + readonly perMinute: number; + readonly authPerMinute: number; +} + +export function readRateLimitConfig(): RateLimitConfig { + return { + perMinute: parsePositiveInt( + process.env['RATE_LIMIT_PER_MINUTE'], + DEFAULT_RATE_LIMIT_PER_MINUTE, + 'RATE_LIMIT_PER_MINUTE', + ), + authPerMinute: parsePositiveInt( + process.env['RATE_LIMIT_AUTH_PER_MINUTE'], + DEFAULT_RATE_LIMIT_AUTH_PER_MINUTE, + 'RATE_LIMIT_AUTH_PER_MINUTE', + ), + }; +} + +/** + * Build the rate-limit middleware. Single `express-rate-limit` + * instance with a dynamic per-request `max`: 10/min on auth-flow + * entry routes, 120/min everywhere else. Bucket key = session id + * when the request carries an active session, else the remote IP. + * That means an unauthenticated brute-force attempt against + * `/auth/login` is bound to its source IP, not a session it + * doesn't have. + * + * Storage: in-memory. ADR-0015 §"Single-instance v1" plus + * `express-rate-limit`'s default in-memory store is fine until we + * scale out the BFF horizontally; the migration to a Redis-backed + * store is one constructor arg the day we need it. + * + * The 429 response uses the same envelope as `StructuredErrorFilter` + * so the SPA gets `{ error: { code: 'rate_limited', message, traceId } }` + * regardless of whether the rejection came from Nest or this raw + * middleware. + */ +export function createRateLimitMiddleware(config: RateLimitConfig): RequestHandler { + const options: Partial = { + windowMs: WINDOW_MS, + standardHeaders: 'draft-7', + legacyHeaders: false, + skip: (req: Request, _res: Response) => SKIP_PATHS.some((p) => req.path.startsWith(p)), + keyGenerator: (req: Request, _res: Response) => { + const sessionId = req.sessionID; + const hasSession = Boolean(req.session?.user); + if (hasSession && sessionId) { + return `s:${sessionId}`; + } + return `ip:${req.ip ?? 'unknown'}`; + }, + handler: (_req: Request, res: Response, _next: NextFunction, _optionsUsed: Options) => { + res.status(429).json(errorResponse('rate_limited', 'Too many requests')); + }, + max: (req: Request, _res: Response) => + STRICT_PATHS.some((p) => req.path.startsWith(p)) ? config.authPerMinute : config.perMinute, + }; + // The library's `RateLimitRequestHandler` augments `RequestHandler` + // with diagnostic methods (`resetKey`, etc.) we don't expose here. + // Cast to the plain Express type so `app.use(...)` accepts it. + return rateLimit(options) as unknown as RequestHandler; +} + +function parsePositiveInt(raw: string | undefined, fallback: number, name: string): number { + if (raw === undefined || raw === '') { + return fallback; + } + const value = Number(raw); + if (!Number.isInteger(value) || value <= 0) { + throw new Error(`${name} must be a positive integer (got "${raw}").`); + } + return value; +} diff --git a/apps/portal-bff/src/security/structured-error.filter.spec.ts b/apps/portal-bff/src/security/structured-error.filter.spec.ts new file mode 100644 index 0000000..5252807 --- /dev/null +++ b/apps/portal-bff/src/security/structured-error.filter.spec.ts @@ -0,0 +1,188 @@ +import { + type ArgumentsHost, + BadRequestException, + ForbiddenException, + HttpException, + HttpStatus, + InternalServerErrorException, + NotFoundException, + UnauthorizedException, +} from '@nestjs/common'; +import { trace } from '@opentelemetry/api'; +import type { Response } from 'express'; +import type { Logger } from 'nestjs-pino'; +import { StructuredErrorFilter, errorResponse } from './structured-error.filter'; + +function makeRes(): Response & { status: jest.Mock; json: jest.Mock } { + const res = { + status: jest.fn(), + json: jest.fn(), + }; + res.status.mockReturnValue(res); + res.json.mockReturnValue(res); + return res as unknown as Response & { status: jest.Mock; json: jest.Mock }; +} + +function makeHost(res: Response): ArgumentsHost { + return { + switchToHttp: () => ({ + getRequest: () => ({}), + getResponse: () => res, + getNext: () => undefined, + }), + } as unknown as ArgumentsHost; +} + +function makeLogger() { + return { + log: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + debug: jest.fn(), + } as unknown as Logger & { warn: jest.Mock; error: jest.Mock }; +} + +describe('StructuredErrorFilter', () => { + describe('classification by HttpException type', () => { + it.each([ + [new BadRequestException('bad input'), HttpStatus.BAD_REQUEST, 'bad_request'], + [new UnauthorizedException(), HttpStatus.UNAUTHORIZED, 'unauthenticated'], + [new ForbiddenException(), HttpStatus.FORBIDDEN, 'forbidden'], + [new NotFoundException(), HttpStatus.NOT_FOUND, 'not_found'], + ])('maps %p to status %i with code %s', (exception, expectedStatus, expectedCode) => { + const res = makeRes(); + const filter = new StructuredErrorFilter(makeLogger()); + filter.catch(exception, makeHost(res)); + expect(res.status).toHaveBeenCalledWith(expectedStatus); + const body = res.json.mock.calls[0]?.[0] as { error: { code: string } }; + expect(body.error.code).toBe(expectedCode); + }); + }); + + describe('5xx envelope', () => { + it('does NOT leak the exception message on a raw 500', () => { + const res = makeRes(); + const filter = new StructuredErrorFilter(makeLogger()); + filter.catch(new Error('Prisma broke: DATABASE_URL=postgres://secret'), makeHost(res)); + expect(res.status).toHaveBeenCalledWith(500); + const body = res.json.mock.calls[0]?.[0] as { error: { code: string; message: string } }; + expect(body.error.code).toBe('internal'); + expect(body.error.message).toBe('Internal server error'); + expect(body.error.message).not.toContain('secret'); + expect(body.error.message).not.toContain('Prisma'); + }); + + it('logs the full exception at error level (with stack via err field)', () => { + const logger = makeLogger(); + const filter = new StructuredErrorFilter(logger); + const exception = new Error('original error'); + filter.catch(exception, makeHost(makeRes())); + expect(logger.error).toHaveBeenCalledWith( + expect.objectContaining({ event: 'http.error', status: 500, err: exception }), + 'StructuredError', + ); + }); + + it('respects an InternalServerErrorException message but still uses the safe code', () => { + // Even if a developer wraps a sensitive error in + // InternalServerErrorException, the message we send out + // comes from the HttpException's getResponse() which is + // explicit user-facing text. The filter trusts it (caller's + // responsibility — not the filter's). + const res = makeRes(); + const filter = new StructuredErrorFilter(makeLogger()); + filter.catch( + new InternalServerErrorException('Database temporarily unavailable'), + makeHost(res), + ); + const body = res.json.mock.calls[0]?.[0] as { error: { code: string; message: string } }; + expect(body.error.message).toBe('Database temporarily unavailable'); + // Falls through to status-derived code since + // InternalServerErrorException doesn't define a custom one. + expect(body.error.code).toBe('http_error'); + }); + }); + + describe('custom code via HttpException response object', () => { + it('picks up `code` from the response object', () => { + const res = makeRes(); + const filter = new StructuredErrorFilter(makeLogger()); + filter.catch( + new HttpException({ code: 'csrf', message: 'CSRF token missing' }, HttpStatus.FORBIDDEN), + makeHost(res), + ); + const body = res.json.mock.calls[0]?.[0] as { error: { code: string; message: string } }; + expect(body.error.code).toBe('csrf'); + expect(body.error.message).toBe('CSRF token missing'); + }); + + it('joins array messages with semicolons (Nest ValidationPipe shape)', () => { + const res = makeRes(); + const filter = new StructuredErrorFilter(makeLogger()); + filter.catch( + new HttpException( + { message: ['name is required', 'email is invalid'] }, + HttpStatus.BAD_REQUEST, + ), + makeHost(res), + ); + const body = res.json.mock.calls[0]?.[0] as { error: { message: string } }; + expect(body.error.message).toBe('name is required; email is invalid'); + }); + }); + + describe('trace id', () => { + it('captures the active OTel trace id in the envelope', () => { + const fakeSpan = { + spanContext: () => ({ traceId: 'abc123', spanId: 'def', traceFlags: 0 }), + }; + const spy = jest.spyOn(trace, 'getActiveSpan').mockReturnValue(fakeSpan as never); + try { + const res = makeRes(); + const filter = new StructuredErrorFilter(makeLogger()); + filter.catch(new BadRequestException('x'), makeHost(res)); + const body = res.json.mock.calls[0]?.[0] as { error: { traceId: string | null } }; + expect(body.error.traceId).toBe('abc123'); + } finally { + spy.mockRestore(); + } + }); + + it('writes traceId: null when no span is active', () => { + const spy = jest.spyOn(trace, 'getActiveSpan').mockReturnValue(undefined); + try { + const res = makeRes(); + const filter = new StructuredErrorFilter(makeLogger()); + filter.catch(new BadRequestException('x'), makeHost(res)); + const body = res.json.mock.calls[0]?.[0] as { error: { traceId: string | null } }; + expect(body.error.traceId).toBeNull(); + } finally { + spy.mockRestore(); + } + }); + }); +}); + +describe('errorResponse() helper (for raw-middleware callers)', () => { + it('produces the same envelope shape as the filter', () => { + const body = errorResponse('csrf', 'CSRF token missing'); + expect(body).toEqual({ + error: { + code: 'csrf', + message: 'CSRF token missing', + traceId: null, + }, + }); + }); + + it('picks up the active OTel trace id', () => { + const spy = jest.spyOn(trace, 'getActiveSpan').mockReturnValue({ + spanContext: () => ({ traceId: 'tid', spanId: 's', traceFlags: 0 }), + } as never); + try { + expect(errorResponse('csrf', 'm').error.traceId).toBe('tid'); + } finally { + spy.mockRestore(); + } + }); +}); diff --git a/apps/portal-bff/src/security/structured-error.filter.ts b/apps/portal-bff/src/security/structured-error.filter.ts new file mode 100644 index 0000000..6949d96 --- /dev/null +++ b/apps/portal-bff/src/security/structured-error.filter.ts @@ -0,0 +1,194 @@ +import { + type ArgumentsHost, + Catch, + type ExceptionFilter, + HttpException, + HttpStatus, +} from '@nestjs/common'; +import { trace } from '@opentelemetry/api'; +import type { Response } from 'express'; +import { Logger } from 'nestjs-pino'; + +/** + * Shape of every error response coming out of the BFF. The + * generic Nest default leaks `message` + `statusCode` at the top + * level, and on 500 it leaks the full exception class name. This + * filter normalises everything to the same envelope so the SPA + * (and any future API consumer) gets a single contract to parse: + * + * { + * "error": { + * "code": "csrf", // stable token + * "message": "CSRF token missing or invalid", // safe, human-readable + * "traceId": "abc123def…" // OTel trace id for correlation + * } + * } + * + * Server-internal details (stack traces, full Prisma errors, etc.) + * are written to the Pino log line at `error` level — never to the + * response body, even on 5xx. + */ +export interface ErrorResponseBody { + error: { + code: string; + message: string; + traceId: string | null; + }; +} + +/** + * Codes returned in `error.code`. Limited and stable so the SPA + * can switch on them. Adding a new code is a deliberate API + * change; the consumer's TypeScript union picks it up. + * + * Current entries are derived from existing code paths that throw + * `HttpException`. The list grows organically as new typed errors + * land. + */ +const STATUS_CODE_MAP: Record = { + [HttpStatus.BAD_REQUEST]: 'bad_request', + [HttpStatus.UNAUTHORIZED]: 'unauthenticated', + [HttpStatus.FORBIDDEN]: 'forbidden', + [HttpStatus.NOT_FOUND]: 'not_found', + [HttpStatus.CONFLICT]: 'conflict', + [HttpStatus.UNPROCESSABLE_ENTITY]: 'unprocessable', + [HttpStatus.TOO_MANY_REQUESTS]: 'rate_limited', + [HttpStatus.SERVICE_UNAVAILABLE]: 'service_unavailable', +}; + +/** + * Global exception filter. Registered in `main.ts` via + * `app.useGlobalFilters(...)`. Catches everything (`@Catch()` no + * args), classifies the status, picks a safe error code + message, + * logs the full exception via Pino, and writes the structured + * envelope to the response. + * + * Specific code overrides — e.g. CSRF middleware that already + * formats a response — write directly to the response and bypass + * the Nest filter chain. Those call sites set the same envelope + * shape manually via the `errorResponse(...)` helper exported + * below, so the contract holds whether the error came through + * Nest's filter or through a raw middleware short-circuit. + */ +@Catch() +export class StructuredErrorFilter implements ExceptionFilter { + constructor(private readonly logger: Logger) {} + + catch(exception: unknown, host: ArgumentsHost): void { + const ctx = host.switchToHttp(); + const res = ctx.getResponse(); + + const { status, code, message, internalReason } = this.classify(exception); + + // Always log the full exception with stack; the response body + // never carries it. Pino's stack-serialiser pulls the class + // name + message + stack out of an Error instance. + if (status >= 500) { + this.logger.error( + { + event: 'http.error', + status, + code, + err: exception, + }, + 'StructuredError', + ); + } else { + this.logger.warn( + { + event: 'http.client_error', + status, + code, + reason: internalReason, + }, + 'StructuredError', + ); + } + + res.status(status).json(errorResponse(code, message)); + } + + private classify(exception: unknown): { + status: number; + code: string; + message: string; + internalReason: string; + } { + if (exception instanceof HttpException) { + const status = exception.getStatus(); + const response = exception.getResponse(); + // Nest builtin HttpException response is either a string + // ("Forbidden", etc.) or an object like + // `{ statusCode, message, error }`. Pick a safe code + + // message from whatever shape we get. + const { code, message } = unwrapHttpException(status, response); + return { status, code, message, internalReason: message }; + } + return { + status: HttpStatus.INTERNAL_SERVER_ERROR, + code: 'internal', + message: 'Internal server error', + internalReason: exception instanceof Error ? exception.message : String(exception), + }; + } +} + +function unwrapHttpException( + status: number, + response: string | object, +): { code: string; message: string } { + const fallbackCode = STATUS_CODE_MAP[status] ?? 'http_error'; + + if (typeof response === 'string') { + return { code: fallbackCode, message: response }; + } + + // Nest's default object has `message: string | string[]` and + // sometimes a `code` we control via custom HttpException + // subclasses. Prefer caller-supplied code/message; fall back to + // the status-derived defaults. + const obj = response as { code?: unknown; message?: unknown }; + const code = typeof obj.code === 'string' ? obj.code : fallbackCode; + const message = normaliseMessage(obj.message) ?? defaultMessageFor(status); + return { code, message }; +} + +function normaliseMessage(message: unknown): string | null { + if (typeof message === 'string') return message; + if (Array.isArray(message) && message.every((m) => typeof m === 'string')) { + return (message as string[]).join('; '); + } + return null; +} + +function defaultMessageFor(status: number): string { + switch (status) { + case HttpStatus.UNAUTHORIZED: + return 'Unauthenticated'; + case HttpStatus.FORBIDDEN: + return 'Forbidden'; + case HttpStatus.NOT_FOUND: + return 'Not found'; + case HttpStatus.TOO_MANY_REQUESTS: + return 'Too many requests'; + case HttpStatus.SERVICE_UNAVAILABLE: + return 'Service unavailable'; + default: + return status >= 500 ? 'Internal server error' : 'Request failed'; + } +} + +/** + * Helper for code paths that write the response directly (raw + * Express middlewares like CSRF) — keep the same envelope as the + * filter. Picks up the current OTel trace id automatically. + */ +export function errorResponse(code: string, message: string): ErrorResponseBody { + return { + error: { + code, + message, + traceId: trace.getActiveSpan()?.spanContext().traceId ?? null, + }, + }; +} diff --git a/package.json b/package.json index d662321..152720a 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,8 @@ "ip-address@<10.1.1": ">=10.1.1", "protobufjs@<8.0.2": ">=8.0.2", "tmp@<0.2.4": ">=0.2.4", - "yaml@<2.8.3": ">=2.8.3" + "yaml@<2.8.3": ">=2.8.3", + "@types/express": "^5.0.6" } }, "prisma": { @@ -150,6 +151,7 @@ "class-validator": "^0.15.1", "connect-redis": "^9.0.0", "cookie-parser": "^1.4.7", + "express-rate-limit": "^8.5.1", "express-session": "^1.19.0", "helmet": "^8.1.0", "ioredis": "^5.10.1", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 0d7c37c..7a86a04 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -13,6 +13,7 @@ overrides: protobufjs@<8.0.2: '>=8.0.2' tmp@<0.2.4: '>=0.2.4' yaml@<2.8.3: '>=2.8.3' + '@types/express': ^5.0.6 importers: @@ -123,6 +124,9 @@ importers: cookie-parser: specifier: ^1.4.7 version: 1.4.7 + express-rate-limit: + specifier: ^8.5.1 + version: 8.5.1(express@5.2.1) express-session: specifier: ^1.19.0 version: 1.19.0 @@ -4307,7 +4311,7 @@ packages: '@types/cookie-parser@1.4.10': resolution: {integrity: sha512-B4xqkqfZ8Wek+rCOeRxsjMS9OgvzebEzzLYw7NHYuvzb7IdxOkI0ZHGgeEBX4PUM7QGVvNSK60T3OvWj3YfBRg==} peerDependencies: - '@types/express': '*' + '@types/express': ^5.0.6 '@types/deep-eql@4.0.2': resolution: {integrity: sha512-c9h9dVVMigMPc4bwTvC5dxqtqJZwQPePsWjPlpSOnojbor6pGqdk541lfA7AqFQr5pB1BRdq0juY9db81BwyFw==} @@ -4336,9 +4340,6 @@ packages: '@types/express-session@1.19.0': resolution: {integrity: sha512-GbypG0bog68UbOq2tSAp7SclvCUm3ha1uDi58OPRGK1NfRvCIu7Gz0M7fTGtpNG1T9a29GpuurQj9zEcT/lMXQ==} - '@types/express@4.17.25': - resolution: {integrity: sha512-dVd04UKsfpINUnK0yBoYHDF3xu7xVH4BuDotC/xGuycx4CgbP48X/KF/586bcObxT0HENHXEU8Nqtu6NR+eKhw==} - '@types/express@5.0.6': resolution: {integrity: sha512-sKYVuV7Sv9fbPIt/442koC7+IIwK5olP1KWeD88e/idgoJqDm3JV/YUiPwkoKK92ylff2MGxSz1CSjsXelx0YA==} @@ -6578,7 +6579,7 @@ packages: resolution: {integrity: sha512-c1IyJYLYppU574+YI7R4QyX2ystMtVXZwIdzazUIPIJsHuWNd+mho2j+bKoHftndicGj9yh+xjd+l0yj7VeT1Q==} engines: {node: '>=12.0.0'} peerDependencies: - '@types/express': ^4.17.13 + '@types/express': ^5.0.6 peerDependenciesMeta: '@types/express': optional: true @@ -14537,7 +14538,7 @@ snapshots: '@rspack/core': 1.6.8(@swc/helpers@0.5.21) '@types/bonjour': 3.5.13 '@types/connect-history-api-fallback': 1.5.4 - '@types/express': 4.17.25 + '@types/express': 5.0.6 '@types/express-serve-static-core': 4.19.8 '@types/serve-index': 1.9.4 '@types/serve-static': 1.15.10 @@ -14551,7 +14552,7 @@ snapshots: connect-history-api-fallback: 2.0.0 express: 4.22.1 graceful-fs: 4.2.11 - http-proxy-middleware: 2.0.9(@types/express@4.17.25) + http-proxy-middleware: 2.0.9(@types/express@5.0.6) ipaddr.js: 2.4.0 launch-editor: 2.13.2 open: 10.2.0 @@ -14965,13 +14966,6 @@ snapshots: dependencies: '@types/express': 5.0.6 - '@types/express@4.17.25': - dependencies: - '@types/body-parser': 1.19.6 - '@types/express-serve-static-core': 4.19.8 - '@types/qs': 6.15.0 - '@types/serve-static': 1.15.10 - '@types/express@5.0.6': dependencies: '@types/body-parser': 1.19.6 @@ -17417,7 +17411,7 @@ snapshots: transitivePeerDependencies: - supports-color - http-proxy-middleware@2.0.9(@types/express@4.17.25): + http-proxy-middleware@2.0.9(@types/express@5.0.6): dependencies: '@types/http-proxy': 1.17.17 http-proxy: 1.18.1(debug@4.4.3) @@ -17425,7 +17419,7 @@ snapshots: is-plain-obj: 3.0.0 micromatch: 4.0.8 optionalDependencies: - '@types/express': 4.17.25 + '@types/express': 5.0.6 transitivePeerDependencies: - debug @@ -21427,7 +21421,7 @@ snapshots: dependencies: '@types/bonjour': 3.5.13 '@types/connect-history-api-fallback': 1.5.4 - '@types/express': 4.17.25 + '@types/express': 5.0.6 '@types/express-serve-static-core': 4.19.8 '@types/serve-index': 1.9.4 '@types/serve-static': 1.15.10 @@ -21441,7 +21435,7 @@ snapshots: connect-history-api-fallback: 2.0.0 express: 4.22.1 graceful-fs: 4.2.11 - http-proxy-middleware: 2.0.9(@types/express@4.17.25) + http-proxy-middleware: 2.0.9(@types/express@5.0.6) ipaddr.js: 2.4.0 launch-editor: 2.13.2 open: 10.2.0