feat(portal-bff): rate limiting + structured error filter (#123)
## Summary Closes the phase-2 hardening list that `main.ts` has been advertising since the security PR (#122). Two new middlewares + one alignment pass on the response shape so every BFF error follows a single contract. ### Structured error filter A global `ExceptionFilter` (registered via `app.useGlobalFilters(...)` at the top of `bootstrap()`) normalises every 4xx/5xx response to a single envelope : ```json { "error": { "code": "csrf", "message": "CSRF token missing or invalid", "traceId": "abc123…" } } ``` - `code` — stable token the SPA can `switch` on. Either explicit on the `HttpException`'s response object (`new UnauthorizedException({ code: 'unauthenticated', message: '...' })`) or derived from the status (`STATUS_CODE_MAP` for the common cases, `'http_error'` fallback). 500s always use `'internal'`. - `message` — safe human-readable text. **500s never leak the underlying exception** (the full message + stack go to the Pino `error` log line as `err: exception` — Pino's stack-serialiser does the rest). - `traceId` — current OTel trace id (or `null` when no span is active). Makes cross-correlation with the audit log + Pino lines trivial. An exported `errorResponse(code, message)` helper produces the same envelope for code paths that write the response directly (raw Express middlewares like the CSRF one, the rate-limit handler) — single contract everywhere. ### 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` env), 120/min everywhere else (`RATE_LIMIT_PER_MINUTE`). - **Bucket key** = session id when the request carries an active session, remote IP otherwise. A single attacker can't dodge the limit by rotating sessions; an authenticated user gets per-account fairness regardless of source IP. - **`/api/health` is skipped** so orchestrator polls don't burn the user quota. - 429 response uses the same envelope as everything else (`{ error: { code: 'rate_limited', … } }`) via the shared `errorResponse()` helper. - In-memory store (single-instance v1 per ADR-0015). Redis-backed store is a one-line config change when we scale out. ### Alignment pass - **CSRF middleware** previously returned `{ error: 'csrf' }`. Now returns the full envelope via `errorResponse('csrf', 'CSRF token missing or invalid')`. - **`/auth/me` 401** previously wrote `{ error: 'unauthenticated' }` directly. Now throws `UnauthorizedException({ code: 'unauthenticated', message: 'Unauthenticated' })` so the filter formats it. Identical response shape on the wire as the CSRF path. Both spec assertions updated to the new shape. ### Type-resolution fix (transitive) `@types/express@4.17.25` was being pulled in transitively by `http-proxy-middleware` (Nx's webpack-dev-server). `express-rate-limit`'s `.d.ts` files import `'express'` and the type resolver 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 a single version everywhere. ## Notable choices **`StructuredErrorFilter` is the source of truth, but raw middlewares are still allowed to write responses directly** (rate-limit, CSRF). The reason: Nest's filter chain only handles exceptions thrown from controllers/guards/interceptors. Express middleware short-circuits before that. Both paths now use the same envelope shape through the `errorResponse()` helper. **No `traceId` in non-5xx responses?** It IS included. The filter writes it on every status — useful for any client-server debugging conversation ("send me your traceId from the 403 you got"). **500s strip the exception message.** Even if a developer accidentally surfaces a sensitive detail via `throw new Error('connection to postgres://user:secret@host failed')`, the response body just says "Internal server error". The full message goes to the log — visible to ops, never to clients. This is the standard secure-by-default for unhandled errors. **Dynamic `max` per request, not two separate `rateLimit()` instances.** Two instances would each maintain a separate store, so the `/auth/login` bucket would be independent of the general one for the same IP. A single instance with a path-conditional max gives consistent bucket accounting. ## Out of scope - Redis-backed rate-limit store. v1 ships in-memory; the BFF runs as a single instance. The migration is `new RedisStore({ ... })` when we scale out (ADR-0015 mentions this). - Per-user override of `RATE_LIMIT_PER_MINUTE` (e.g. admins / service accounts with higher quotas). No code path for this in v1. - CSP fine-tuning for portal-shell + portal-admin once Caddy serves them. ## Test plan - [x] `pnpm nx test portal-bff` (clean env) → **199/199 pass** (+25 specs: StructuredErrorFilter, rate-limit middleware, CSRF + /me alignments). - [x] `pnpm nx test feature-auth` (clean env) → **28/28 pass**. - [x] `pnpm nx test portal-shell` (clean env) → **34/34 pass**. - [x] `pnpm nx run-many -t lint build --projects=portal-bff,feature-auth,portal-shell` → clean. - [x] Prettier-clean. - [x] CI clean-env repro: every env var unset (including new `RATE_LIMIT_*`) → 261/261 pass. - [ ] Manual smoke against running BFF: - [ ] Throw any error from a controller → response is `{ error: { code, message, traceId } }`. Pino log has the full exception under `err`. - [ ] Curl `/api/auth/me` without a session cookie → 401 + same envelope, `code: 'unauthenticated'`. - [ ] Hit `/api/auth/login` 11 times in a minute → 11th returns 429 + `code: 'rate_limited'`. `/api/health` hit 100 times → all 200. - [ ] POST without `X-CSRF-Token` → 403 + `code: 'csrf'`. --------- Co-authored-by: Julien Gautier <julien.gautier@apf.asso.fr> Reviewed-on: #123
This commit was merged in pull request #123.
This commit is contained in:
@@ -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;
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user