Merge pull request 'Feature/84 ipv6 rate limit key' (#86) from feature/84-ipv6-rate-limit-key into main

Reviewed-on: #86

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({

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');
+  });
+});