fix(backend): stop IPv6 callers bypassing the password-reset rate limit (#84)

The QA stack has been logging ERR_ERL_KEY_GEN_IPV6 at every boot, and express-rate-limit was right to complain.

keyByCallerAndEmail built its key from req.ip raw. For an IPv4 caller that is one address and the limiter worked as intended. For an IPv6 caller it is the full 128 bits — and a residential IPv6 customer is delegated an entire prefix and can source every request from a different address inside it at no cost. Keyed that way, each request counted as a new caller and the allowance of five per fifteen minutes never bound at all.

That matters more here than it would elsewhere, because of what this limiter is for. Its own comment says it: without one, anyone can make the server send unlimited mail to any address they choose. For IPv6 clients there effectively was no limiter, while the code read as though there were.

The caller half of the key now goes through express-rate-limit's ipKeyGenerator, which groups IPv6 by prefix and returns IPv4 unchanged. The helper's default is /56 rather than /64, and that default is kept deliberately: /56 covers a whole delegated site, so an attacker cannot escape their bucket by moving within their own allocation. It does mean several households behind one delegation share an allowance — acceptable only because the key also carries the email address, so they collide just when targeting the same account. The reasoning sits next to the code, because a future reader tightening it to /64 would silently reopen the hole.

keyByCallerAndEmail is now exported so it can be tested directly. The limiter's allowance is still not asserted anywhere, and should not be: its store is process-wide, so a test that exhausts it leaks into every later test from the same address and fails something unrelated later. The key function is pure, and it is where the bug was.

Verified by firing the guard rather than reasoning about it: building main and loading the module reproduces the ValidationError, and the same load with this change is silent. Seven new unit tests cover an IPv4 caller unchanged, two addresses in one delegation collapsing to a single key, separate delegations staying apart, an IPv4-mapped address keying the same as the plain IPv4 one, email normalisation, a non-string email, and a request with no address at all.

86 unit tests pass, 144 integration, lint 0 errors and 8 warnings — unchanged.

Closes #84
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-08-20 18:44:48 -05:00
parent cf45b7a8eb
commit 2f6e855596
2 changed files with 90 additions and 3 deletions
+66
View File
@@ -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');
});
});