From f9f5f171eb18cfd3a52f5e03d488754dc3818ba5 Mon Sep 17 00:00:00 2001 From: Julien Gautier Date: Mon, 1 Jun 2026 12:31:54 +0200 Subject: [PATCH] fix(security): normalize IPv6 in rate-limit keyGenerator (ADR-0021) (#260) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fix `ERR_ERL_KEY_GEN_IPV6` raised by `express-rate-limit` at BFF boot: the rate-limit middleware's custom `keyGenerator` was using `req.ip` verbatim, which the library v8 refuses because it lets an IPv6 attacker rotate through the host bits of their own subnet to escape per-IP rate-limiting. Surfaced during the ADR-0030 dockerised dev mode validation (the boot log shows the `ValidationError` immediately after the rate-limit middleware is built). ## Root cause `apps/portal-bff/src/security/rate-limit.middleware.ts` returned: ```ts return `ip:${req.ip ?? 'unknown'}`; ``` `req.ip` is the raw address the IP-trust chain hands Express. For IPv4 that's already the right bucket key. For IPv6, every distinct host in an attacker's allocation hashes to a different bucket — even though the same human controls all of them. An attacker on a residential IPv6 assignment (typically `/56`) thus has ~2^72 trivially-rotatable buckets per `/56`, which makes the per-IP rate limit useless against them. `express-rate-limit` v7+ ships an `ipKeyGenerator` helper that **truncates the address to its `/56` prefix** before keying. The library v8 raises `ERR_ERL_KEY_GEN_IPV6` at boot if a custom `keyGenerator` returns `req.ip` verbatim, precisely to refuse shipping this bypass. ## Fix | File | Change | | --- | --- | | `apps/portal-bff/src/security/rate-limit.middleware.ts` | Import `ipKeyGenerator` from `express-rate-limit`; wrap `req.ip` through it when keying. IPv4 addresses pass through unchanged; IPv6 addresses get truncated to their `/56`. Comment explains the rationale + that the lib's `/56` default matches a typical residential ISP customer allocation. | | `apps/portal-bff/src/security/rate-limit.middleware.spec.ts` | New test asserting two IPv6 addresses in the same `/56` share a bucket (the bypass would have set both apart), and that distinct `/56`s remain isolated (truncation does not collapse all IPv6 traffic into one global bucket). Existing IPv4 / session / `SKIP_PATHS` tests are unchanged and still pass — `ipKeyGenerator` is a no-op for IPv4. | The session-keyed branch (`s:${sessionID}`) is untouched: sessions key on the BFF-issued session id, not the address. ## Why the BFF kept booting despite the error The log shows `bootstrap` reaching `AuthModule` immediately after the `ValidationError` printout. `express-rate-limit` v8's `errorHandler` defaults to logging the error and continuing rather than throwing for this specific validation, so the middleware was effectively running with the unfixed `keyGenerator` until now — i.e. the bypass was live in the dev BFF. Fixed pre-emptively, before any prod consumer. ## Related - Per [ADR-0021](docs/decisions/0021-phase-2-security-baseline.md) — Phase-2 security baseline, rate-limit section. - Surfaced by the ADR-0030 dockerised dev mode validation (the SPA `/api/auth/me` proxy errors visible in the same log run were a side effect of the BFF restarting on every config validator until the env was fully populated; unrelated to this fix). ## Test plan - [x] `pnpm exec nx test portal-bff --testFile=apps/portal-bff/src/security/rate-limit.middleware.spec.ts` — 794 tests pass, including the new `/56` isolation case. - [ ] Restart the BFF (`./infra/local/dev.sh restart portal-bff`) — `ERR_ERL_KEY_GEN_IPV6` no longer appears in the boot log. - [ ] IPv4 traffic still rate-limits as before (existing test coverage; no behavioural change since `ipKeyGenerator` is identity for v4). --------- Co-authored-by: Julien Gautier Reviewed-on: https://git.unespace.com/julien/apf_portal/pulls/260 --- .../security/rate-limit.middleware.spec.ts | 26 +++++++++++++++++++ .../src/security/rate-limit.middleware.ts | 11 ++++++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/apps/portal-bff/src/security/rate-limit.middleware.spec.ts b/apps/portal-bff/src/security/rate-limit.middleware.spec.ts index 14f9641..58eaf47 100644 --- a/apps/portal-bff/src/security/rate-limit.middleware.spec.ts +++ b/apps/portal-bff/src/security/rate-limit.middleware.spec.ts @@ -153,6 +153,32 @@ describe('createRateLimitMiddleware', () => { await mw(makeReq({ ip: '10.0.0.11', path: '/api/x' }), res, next); expect(res.status).not.toHaveBeenCalledWith(429); }); + + it('keys IPv6 addresses by their /56 prefix so per-host rotation cannot bypass the bucket', async () => { + const mw = createRateLimitMiddleware({ perMinute: 1, authPerMinute: 99 }); + const next = jest.fn() as unknown as NextFunction; + + // Two addresses inside `2001:db8:abcd:0000::/56` must share a + // bucket — otherwise an attacker swaps the host suffix on every + // retry and the per-IP limit never bites. This is the bypass the + // lib's `ERR_ERL_KEY_GEN_IPV6` boot-time validation refuses to + // ship. (The lib v8 default mask is `/56`, a typical residential + // ISP customer allocation.) + let res = makeRes(); + await mw(makeReq({ ip: '2001:db8:abcd::1', path: '/api/x' }), res, next); + expect(res.status).not.toHaveBeenCalledWith(429); + + res = makeRes(); + await mw(makeReq({ ip: '2001:db8:abcd::ffff', path: '/api/x' }), res, next); + expect(res.status).toHaveBeenCalledWith(429); + + // Different `/56` (`2001:db8:abce::/56`) — independent bucket. + // Confirms the truncation does not collapse all IPv6 traffic into + // one global bucket. + res = makeRes(); + await mw(makeReq({ ip: '2001:db8:abce::1', path: '/api/x' }), res, next); + expect(res.status).not.toHaveBeenCalledWith(429); + }); }); function restore(name: string, value: string | undefined): void { diff --git a/apps/portal-bff/src/security/rate-limit.middleware.ts b/apps/portal-bff/src/security/rate-limit.middleware.ts index 5d579fd..ddc5706 100644 --- a/apps/portal-bff/src/security/rate-limit.middleware.ts +++ b/apps/portal-bff/src/security/rate-limit.middleware.ts @@ -1,5 +1,5 @@ import type { NextFunction, Request, RequestHandler, Response } from 'express'; -import rateLimit, { type Options } from 'express-rate-limit'; +import rateLimit, { ipKeyGenerator, type Options } from 'express-rate-limit'; import { errorResponse } from './structured-error.filter'; /** @@ -79,7 +79,14 @@ export function createRateLimitMiddleware(config: RateLimitConfig): RequestHandl if (hasSession && sessionId) { return `s:${sessionId}`; } - return `ip:${req.ip ?? 'unknown'}`; + // `ipKeyGenerator` normalises the address before keying — most + // importantly, it truncates IPv6 to its `/56` prefix (the lib v8 + // default — a typical residential ISP customer allocation) so an + // attacker can't rotate through the trailing bits of their own + // subnet to escape the per-IP bucket. The lib raises + // `ERR_ERL_KEY_GEN_IPV6` at boot if a custom keyGenerator returns + // `req.ip` verbatim, exactly to prevent that bypass. + return `ip:${ipKeyGenerator(req.ip ?? 'unknown')}`; }, handler: (_req: Request, res: Response, _next: NextFunction, _optionsUsed: Options) => { res.status(429).json(errorResponse('rate_limited', 'Too many requests'));