diff --git a/backend/src/rateLimit.ts b/backend/src/rateLimit.ts index 3a190e3..d3cc542 100644 --- a/backend/src/rateLimit.ts +++ b/backend/src/rateLimit.ts @@ -1,4 +1,4 @@ -import rateLimit from 'express-rate-limit'; +import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; import { Request } from 'express'; // First rate limiting in the codebase. The password-reset endpoints need it @@ -20,9 +20,30 @@ const MAX_REQUESTS = 5; // This key only makes sense on a request that carries an email. Applying the // same limiter to an endpoint without one collapses every caller into a single // `ip:` bucket, which is a shared allowance rather than a per-caller one. -function keyByCallerAndEmail(req: Request): string { +// +// The caller half goes through ipKeyGenerator rather than using req.ip raw. +// A raw IPv6 address is the full 128 bits, but a residential IPv6 customer is +// delegated an entire prefix and can source every request from a different +// address inside it for free — so keyed on the exact address this limiter +// counted each request as a new caller and never bound at all. That is not a +// small miss: this limiter is the only thing stopping anyone making the server +// send unlimited mail to any address they choose. express-rate-limit reported +// it as ERR_ERL_KEY_GEN_IPV6 on every boot; see #84. +// +// The helper's default groups IPv6 by /56 rather than /64. That is the +// deliberate choice: /56 covers a whole delegated site, so an attacker cannot +// escape the bucket by moving within their own allocation. It does mean +// several households behind one delegation share an allowance — acceptable +// here only because the key also contains the email address, so they collide +// just when targeting the same account. IPv4 is returned unchanged. +// +// Exported for the unit test. The limiter's own allowance is not worth +// asserting in a test — its store is process-wide, so exhausting it leaks into +// every later test from the same address — but the key function is pure and +// is where the bug actually was. +export function keyByCallerAndEmail(req: Request): string { const email = typeof req.body?.email === 'string' ? req.body.email.toLowerCase().trim() : ''; - return `${req.ip}:${email}`; + return `${ipKeyGenerator(req.ip ?? '')}:${email}`; } export const passwordResetRequestLimiter = rateLimit({ diff --git a/backend/tests/unit/rateLimitKey.test.ts b/backend/tests/unit/rateLimitKey.test.ts new file mode 100644 index 0000000..1faac97 --- /dev/null +++ b/backend/tests/unit/rateLimitKey.test.ts @@ -0,0 +1,66 @@ +import { Request } from 'express'; +import { keyByCallerAndEmail } from '../../src/rateLimit'; + +// The limiter's allowance is deliberately not asserted anywhere. Its store is +// in-memory and process-wide, so a test that exhausts it leaks that state into +// every later test keyed on the same address and produces an order-dependent +// failure somewhere else entirely. The key function is pure, and the IPv6 +// bypass in #84 lived here rather than in the allowance, so this is both the +// safe thing to test and the thing worth testing. +function requestFrom(ip: string | undefined, email?: unknown): Request { + return { ip, body: email === undefined ? {} : { email } } as unknown as Request; +} + +describe('keyByCallerAndEmail', () => { + it('leaves an IPv4 caller as it is', () => { + expect(keyByCallerAndEmail(requestFrom('203.0.113.5', 'someone@example.com'))) + .toBe('203.0.113.5:someone@example.com'); + }); + + // The bug #84 fixed. Before this, these two produced different keys, so an + // IPv6 caller got a fresh allowance for every address in their own prefix — + // which is to say, no limit at all. + it('gives two addresses in one IPv6 delegation the same key', () => { + const first = keyByCallerAndEmail(requestFrom('2001:db8:abcd:0100::1', 'target@example.com')); + const second = keyByCallerAndEmail(requestFrom('2001:db8:abcd:01ff::9', 'target@example.com')); + + expect(first).toBe(second); + expect(first).toBe('2001:db8:abcd:100::/56:target@example.com'); + }); + + // The other half of the same claim: grouping must not be so coarse that + // unrelated callers share one allowance. + it('keeps separate IPv6 delegations apart', () => { + const inside = keyByCallerAndEmail(requestFrom('2001:db8:abcd:0100::1', 'target@example.com')); + const elsewhere = keyByCallerAndEmail(requestFrom('2001:db8:abcd:0200::1', 'target@example.com')); + + expect(inside).not.toBe(elsewhere); + }); + + // Node reports an IPv4 client as an IPv4-mapped IPv6 address in some + // configurations. It must key the same either way, or the same caller would + // get two allowances depending on how the socket happened to be opened. + it('treats an IPv4-mapped address as the IPv4 caller it is', () => { + expect(keyByCallerAndEmail(requestFrom('::ffff:203.0.113.5', 'someone@example.com'))) + .toBe(keyByCallerAndEmail(requestFrom('203.0.113.5', 'someone@example.com'))); + }); + + it('normalizes the email so case and padding cannot buy a second allowance', () => { + expect(keyByCallerAndEmail(requestFrom('203.0.113.5', ' SomeOne@Example.COM '))) + .toBe('203.0.113.5:someone@example.com'); + }); + + it('uses an empty email rather than stringifying a non-string one', () => { + expect(keyByCallerAndEmail(requestFrom('203.0.113.5', { not: 'a string' }))) + .toBe('203.0.113.5:'); + expect(keyByCallerAndEmail(requestFrom('203.0.113.5'))) + .toBe('203.0.113.5:'); + }); + + // Only reachable if `trust proxy` were misconfigured, but it must not throw + // from inside the limiter if it ever happens. + it('survives a request with no address at all', () => { + expect(keyByCallerAndEmail(requestFrom(undefined, 'someone@example.com'))) + .toBe(':someone@example.com'); + }); +});