0e6c114ba7
## 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
104 lines
3.6 KiB
TypeScript
104 lines
3.6 KiB
TypeScript
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.
|
|
* The CSRF check skips them — there is nothing to forge for routes
|
|
* that don't change anything.
|
|
*/
|
|
const SAFE_METHODS = new Set(['GET', 'HEAD', 'OPTIONS']);
|
|
|
|
/**
|
|
* Paths the CSRF middleware lets through unconditionally. These are
|
|
* the OIDC entry points that *create* the session (and the token
|
|
* along with it) — there is no token to verify yet.
|
|
*
|
|
* `GET /auth/logout` is a safe method so it's already covered by
|
|
* `SAFE_METHODS`; if a future PR moves logout to POST, the bypass
|
|
* here would need extending.
|
|
*/
|
|
const CSRF_EXEMPT_PREFIXES = ['/api/auth/login', '/api/auth/callback'];
|
|
|
|
/**
|
|
* Double-submit CSRF middleware (per ADR-0009 §"CSRF defense").
|
|
*
|
|
* Contract:
|
|
* - GET / HEAD / OPTIONS pass through.
|
|
* - Unauthenticated requests pass through. CSRF protects an
|
|
* authenticated identity from being abused by another origin;
|
|
* there is no identity to abuse on anonymous routes. (Anonymous
|
|
* POST endpoints, if they ever exist, will need a different
|
|
* defense — site-key, captcha, etc.)
|
|
* - `/api/auth/login` and `/api/auth/callback` are exempt: they
|
|
* are the routes that *establish* the session and thereby
|
|
* mint the CSRF token.
|
|
* - Any other request must carry `X-CSRF-Token` whose value
|
|
* equals `req.session.csrfToken`. Comparison is
|
|
* `crypto.timingSafeEqual` to avoid leaking the token via
|
|
* response-time differences.
|
|
* - Mismatch / missing → 403 `{ error: 'csrf' }`. No further
|
|
* processing.
|
|
*
|
|
* The middleware reads the token from the **session**, not the
|
|
* cookie. The cookie is the SPA's read-only mirror; an attacker
|
|
* who can plant a cookie (subdomain takeover, etc.) cannot also
|
|
* plant a matching session-stored token without already being the
|
|
* authenticated user. Tying the check to the server-side session
|
|
* is what distinguishes "session-bound double-submit" from the
|
|
* weaker pure cookie-vs-header comparison.
|
|
*/
|
|
export function createCsrfMiddleware(logger: Logger): RequestHandler {
|
|
return function csrf(req: Request, res: Response, next: NextFunction): void {
|
|
if (SAFE_METHODS.has(req.method.toUpperCase())) {
|
|
next();
|
|
return;
|
|
}
|
|
|
|
const session = req.session;
|
|
if (!session?.user) {
|
|
// Anonymous mutating request — out of scope for CSRF v1.
|
|
next();
|
|
return;
|
|
}
|
|
|
|
if (CSRF_EXEMPT_PREFIXES.some((p) => req.path.startsWith(p))) {
|
|
next();
|
|
return;
|
|
}
|
|
|
|
const expected = session.csrfToken;
|
|
const headerToken = req.get('X-CSRF-Token');
|
|
|
|
if (!expected || !headerToken || !equalsTimingSafe(headerToken, expected)) {
|
|
logger.warn(
|
|
{
|
|
event: 'csrf.reject',
|
|
path: req.path,
|
|
method: req.method,
|
|
hasHeaderToken: Boolean(headerToken),
|
|
hasSessionToken: Boolean(expected),
|
|
},
|
|
'Csrf',
|
|
);
|
|
res.status(403).json(errorResponse('csrf', 'CSRF token missing or invalid'));
|
|
return;
|
|
}
|
|
|
|
next();
|
|
};
|
|
}
|
|
|
|
function equalsTimingSafe(a: string, b: string): boolean {
|
|
// `timingSafeEqual` requires equal-length buffers. Different
|
|
// lengths short-circuit as unequal — we accept the early return
|
|
// here because the length itself is not a secret.
|
|
const ba = Buffer.from(a, 'utf8');
|
|
const bb = Buffer.from(b, 'utf8');
|
|
if (ba.length !== bb.length) {
|
|
return false;
|
|
}
|
|
return timingSafeEqual(ba, bb);
|
|
}
|