f9f5f171eb
## 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 <julien.gautier@apf.asso.fr>
Reviewed-on: #260
191 lines
7.0 KiB
TypeScript
191 lines
7.0 KiB
TypeScript
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);
|
|
});
|
|
|
|
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 {
|
|
if (value === undefined) {
|
|
delete process.env[name];
|
|
} else {
|
|
process.env[name] = value;
|
|
}
|
|
}
|