feat(portal-bff): rate limiting + structured error filter
closes the phase-2 hardening list main.ts has been advertising since #122 (helmet + cors + csrf). two new middlewares + an alignment pass so every bff error follows a single response contract. structured error filter: global ExceptionFilter (registered via app.useGlobalFilters at the top of bootstrap()) normalises every 4xx/5xx response to: { error: { code, message, traceId } } - code: stable token the spa can switch on. either explicit on the HttpException's response object (UnauthorizedException({ code: 'unauthenticated', message })) or derived from status. 500s always 'internal'. - message: safe text. 500s NEVER leak the underlying exception (full message + stack go to pino's err: field, never to the response body). - traceId: otel trace id for cross-correlation. exported errorResponse(code, message) helper for raw-middleware callers (csrf, rate-limit) so the envelope is identical whether the response came through nest's filter or a short-circuit middleware. rate limiting: express-rate-limit mounted after the session middleware. - dynamic max per request: 10/min on /api/auth/login + /api/auth/ callback (RATE_LIMIT_AUTH_PER_MINUTE), 120/min elsewhere (RATE_LIMIT_PER_MINUTE). - bucket key = session id when auth, remote ip otherwise. rotating sessions doesn't dodge the limit; auth'd users get per-account fairness regardless of source ip. - /api/health skipped. orchestrator polls don't burn user quota. - 429 uses the shared errorResponse envelope. - in-memory store. single-instance v1 per adr-0015. redis-backed store is one config arg the day we scale out horizontally. alignment pass on existing error responses: - csrf middleware used to return { error: 'csrf' }. now uses errorResponse('csrf', 'CSRF token missing or invalid'). - /auth/me used to res.status(401).json({ error: 'unauthenticated' }) directly. now throws UnauthorizedException({ code: ..., message: ... }) so the filter formats it. identical shape on the wire. type-resolution fix (transitive): @types/express@4.17.25 was being pulled in by http-proxy-middleware (nx's webpack-dev-server). express-rate-limit's .d.ts files import 'express', and tsc was matching the v4 copy — causing Request type mismatches with our v5-based code. added "@types/express": "^5.0.6" to pnpm.overrides so the workspace pins one version everywhere. specs: - 199/199 portal-bff (was 174 before; +25 specs across StructuredErrorFilter, rate-limit middleware, csrf alignment, /me alignment). - feature-auth 28/28, portal-shell 34/34 unchanged. - clean-env repro: env -u every config var → 261/261 pass. out of scope: - redis-backed rate-limit store (single-instance v1). - per-user rate-limit overrides (admin / service accounts). - csp fine-tuning when caddy serves the static spas.
This commit is contained in:
@@ -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:
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -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));
|
||||
}
|
||||
|
||||
@@ -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<RequestHandler>(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<RequestHandler>(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;
|
||||
|
||||
@@ -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 }),
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
}
|
||||
@@ -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<Options> = {
|
||||
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;
|
||||
}
|
||||
@@ -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();
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -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<number, string> = {
|
||||
[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<Response>();
|
||||
|
||||
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,
|
||||
},
|
||||
};
|
||||
}
|
||||
Reference in New Issue
Block a user