2b93710eaa496bb1421e0ec05be0d5501c9527bd
2 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 julien
|
0e6c114ba7 |
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 |
||
|
julien
|
5bbe2304ff |
feat(portal-bff): helmet + env-driven CORS allowlist + double-submit CSRF (#122)
## Summary Phase-2 security baseline that the `main.ts` placeholder note has been advertising since the auth/session work began. Three independent middlewares + their SPA counterparts, all mounted in a single PR because they only become meaningful together. ### Helmet on the BFF `helmet()` with three overrides matching our specific shape: - **HSTS only in production** — dev runs on plain HTTP, HSTS is just noise. - **`crossOriginResourcePolicy: 'cross-origin'`** — the SPA on its own origin reads JSON from the BFF; the default `same-origin` would block it. - **CSP disabled in non-production** — the BFF doesn't render HTML, so CSP on JSON responses is mostly inert, but Helmet's default CSP triggers noisy `connect-src` violations in browser devtools that we don't need. Everything else is Helmet defaults: `X-Frame-Options=SAMEORIGIN`, `X-Content-Type-Options=nosniff`, `Referrer-Policy=no-referrer`, `X-Powered-By` removed, etc. ### CORS allowlist, env-driven `CORS_ALLOWED_ORIGINS` env (comma-separated) is now **mandatory** at boot. The BFF refuses to start without it via `readCorsAllowlist()` — same boot-time validator family as `assertSessionSecret` etc. The previous hardcoded `http://localhost:4200` fallback is gone; getting CORS wrong silently is the kind of "works in dev, breaks in prod" trap the validator is specifically designed to catch. `X-CSRF-Token` is now in the allowed headers. ### Double-submit CSRF - BFF mints a 256-bit `csrfToken` at session creation (`/auth/callback`), stored on `req.session.csrfToken` and mirrored to a JS-readable cookie (`__Host-portal_csrf` prod / `portal_csrf` dev). The cookie is the SPA's read-only view; the server-side session is the source of truth. - `createCsrfMiddleware` (mounted after the session middleware in `main.ts`) compares the `X-CSRF-Token` header with `req.session.csrfToken` using `crypto.timingSafeEqual`. Skips: - safe methods (`GET / HEAD / OPTIONS`), - anonymous requests (no `req.session.user`), - `/api/auth/login` and `/api/auth/callback` (those mint the token themselves). - Mismatch → `403 {"error":"csrf"}` with a structured Pino warn. - SPA's `csrfInterceptor` reads the cookie via `document.cookie` and copies its value into `X-CSRF-Token` on every mutating BFF request. The header is omitted on `GET / HEAD / OPTIONS` (BFF skips them anyway) and on non-BFF origins. - Logout and the absolute-timeout middleware both clear the CSRF cookie alongside the session cookie. ## Notable choices **Session-bound double-submit, not pure cookie-vs-header.** A naive "compare cookie with header" check is defeated when an attacker can plant a cookie (subdomain takeover, etc.). Comparing the header to the server-side session-stored token instead means the attacker would also need to be the authenticated user — which is what CSRF defense is supposed to prevent in the first place. **No CSRF for anonymous mutating routes (v1).** None exist today; we don't have an unauthenticated POST endpoint anywhere. Generating a CSRF token for anonymous sessions would conflict with `saveUninitialized: false` on express-session and add complexity we don't need yet. Anonymous public-form CSRF defenses (site-key, captcha) land if and when those routes ship. **`SameSite=Lax`, not `Strict`, on the CSRF cookie.** Matches the session cookie's policy so the two travel together on the SPA→BFF cross-origin same-site fetch (different ports = different origin, same registrable domain). The double-submit pattern is what gives the protection; `SameSite=Lax` is a belt-and-braces layer. **`csrfInterceptor` runs after `bffCredentialsInterceptor` and before `bffUnauthorizedInterceptor` in the chain.** Order: credentials first (set `withCredentials`), then CSRF (set the header), then unauthorized handling (catch 401s). Forward order, no surprises. **`CORS_ALLOWED_ORIGINS` has no localhost fallback.** I considered keeping the fallback for ergonomics but it makes the BFF silently misconfigured if someone forgets the env. The error message points straight at the file to edit. ## Out of scope (next PRs) - Rate limiting + structured error filter (still in the phase-2 to-do). - CSP fine-tuning when we have actual HTML pages (portal-shell + portal-admin static serving). - CSRF token rotation on idle-extension (today the token lives the session's lifetime; refreshing on each request would invalidate in-flight mutations). ## Test plan - [x] `pnpm nx run-many -t test --projects=portal-bff,feature-auth,portal-shell` clean env → **177 + 28 + 34 = 239/239 pass** (was 144 + 19 + 34 = 197 before; +42 specs across CSRF middleware, CSRF cookie helpers, CORS allowlist parser, csrfInterceptor, and extended auth.controller / absolute-timeout coverage). - [x] `pnpm nx run-many -t lint build --projects=portal-bff,feature-auth,portal-shell` → clean. - [x] **CI clean-env repro** (lesson from prior PRs): every env var unset (including new `CORS_ALLOWED_ORIGINS`) → tests still pass. The BFF refuses to boot without `CORS_ALLOWED_ORIGINS`, which is the intended behaviour. - [x] Prettier-clean. - [ ] Manual smoke against running BFF: - [ ] Sign in → `__Host-portal_csrf` (prod) / `portal_csrf` (dev) cookie set, value matches `audit.events.payload->>actorIdHash`-style traceability via `req.session.csrfToken` in Redis. - [ ] Hit a future POST route from the SPA → request carries `X-CSRF-Token`, BFF accepts. - [ ] Forge a POST without the header (curl) → 403 `{"error":"csrf"}`. - [ ] Sign out → both cookies cleared. --------- Co-authored-by: Julien Gautier <julien.gautier@apf.asso.fr> Reviewed-on: #122 |