IPv6 clients can bypass the password-reset rate limit, and express-rate-limit says so at every boot #84

New Issue

2026-08-20T18:30:13-05:00

bermudalamb commented

2026-08-20 18:30:13 -05:00

The QA stack logs this on every start:

ValidationError: Custom keyGenerator appears to use request IP without calling the ipKeyGenerator
helper function for IPv6 addresses. This could allow IPv6 users to bypass limits.
    at Object.<anonymous> (/app/dist/rateLimit.js:29:72)
  code: 'ERR_ERL_KEY_GEN_IPV6'

It is not fatal, and it is not new

The app starts anyway — redefined-designs listening on 3000 appears immediately after. express-rate-limit reports this as a validation warning and carries on.

It is also not from the error-boundary work on feature/62-error-boundary, which is not pushed and would not be in a QA image. dist/rateLimit.js:29 is passwordResetRequestLimiter, which has been there since password reset was built:

function keyByCallerAndEmail(req: Request): string {
  const email = typeof req.body?.email === 'string' ? req.body.email.toLowerCase().trim() : '';
  return `${req.ip}:${email}`;
}

Worth saying plainly since the timing invites the opposite conclusion: the new clientErrorLimiter added under #62 uses the default key generator specifically to avoid this, and the default already handles IPv6 correctly.

The warning is right, and the bypass is real

req.ip for an IPv6 client is a full 128-bit address. A residential IPv6 customer is typically delegated an entire /64 — eighteen quintillion addresses — and can source each request from a different one at no cost. Keyed on the exact address, the limiter counts every request as a new caller and the allowance of five per fifteen minutes never binds.

That matters more here than it would on most endpoints, because of what this particular limiter is for. rateLimit.ts says it in its own comment: "without one, anyone can make the server send unlimited mail to any address." An IPv6 client can do exactly that today. The limiter looks present in code review and does nothing in production for a large and growing share of clients.

Two things follow from it, and the second is the worse one:

Mail amplification. Unlimited password-reset mail to any address anyone chooses, sent by our SMTP identity. That is a deliverability and reputation problem as much as a nuisance to the recipient.
The limit is invisible in its failure. Nothing distinguishes "the limiter is working" from "every caller is being counted separately", because both look like traffic under the threshold. This is the same shape as #67, #60, #61 and the new-code period in #79 — a control that reports success while doing nothing.

The fix

express-rate-limit 8.6.2 is already installed and exports the helper the warning names:

export declare function ipKeyGenerator(ip: string, ipv6Subnet?: number | false): string;

It normalises an IPv6 address to its subnet prefix and leaves IPv4 alone, so one caller counts as one caller either way. The change is to build the key from ipKeyGenerator(req.ip) rather than req.ip.

Worth deciding rather than defaulting: the helper's ipv6Subnet defaults to /56 rather than /64. A /56 groups a whole delegated site, which is the safer choice against a rotating attacker but means several households behind one delegation share an allowance. Given the allowance is per email address as well as per caller, the collision cost looks acceptable — but it is a deliberate trade and should be recorded in the comment next to it, not left implicit.

Also worth doing

trust proxy is set to 1 in app.ts, so req.ip is taken from X-Forwarded-For. That is correct behind Nginx Proxy Manager, and it means the key is the real client rather than the proxy — but it also means the limiter inherits whatever NPM forwards. Confirming NPM actually sets X-Forwarded-For for IPv6 clients in the same form is part of testing this properly, rather than assuming the header shape.

A test would be the honest way to close this, but the existing limiter has none, and the integration suite's rate-limit state is process-wide — #62 recorded that deliberately not asserting a limiter there was the right call, for the same reason. Verifying by unit-testing the key function directly, rather than by exhausting an allowance, avoids that problem entirely.

Severity

Medium-high. Nothing is broken, the app runs, and no incident has occurred — but a security control that exists specifically to prevent mail abuse is not doing its job for IPv6 clients, and has been announcing that at every boot.

The QA stack logs this on every start: ``` ValidationError: Custom keyGenerator appears to use request IP without calling the ipKeyGenerator helper function for IPv6 addresses. This could allow IPv6 users to bypass limits. at Object.<anonymous> (/app/dist/rateLimit.js:29:72) code: 'ERR_ERL_KEY_GEN_IPV6' ``` ## It is not fatal, and it is not new The app starts anyway — `redefined-designs listening on 3000` appears immediately after. `express-rate-limit` reports this as a validation warning and carries on. It is also **not** from the error-boundary work on `feature/62-error-boundary`, which is not pushed and would not be in a QA image. `dist/rateLimit.js:29` is `passwordResetRequestLimiter`, which has been there since password reset was built: ```ts function keyByCallerAndEmail(req: Request): string { const email = typeof req.body?.email === 'string' ? req.body.email.toLowerCase().trim() : ''; return `${req.ip}:${email}`; } ``` Worth saying plainly since the timing invites the opposite conclusion: the new `clientErrorLimiter` added under #62 uses the **default** key generator specifically to avoid this, and the default already handles IPv6 correctly. ## The warning is right, and the bypass is real `req.ip` for an IPv6 client is a full 128-bit address. A residential IPv6 customer is typically delegated an entire /64 — eighteen quintillion addresses — and can source each request from a different one at no cost. Keyed on the exact address, the limiter counts every request as a new caller and the allowance of five per fifteen minutes never binds. That matters more here than it would on most endpoints, because of what this particular limiter is for. `rateLimit.ts` says it in its own comment: *"without one, anyone can make the server send unlimited mail to any address."* An IPv6 client can do exactly that today. The limiter looks present in code review and does nothing in production for a large and growing share of clients. Two things follow from it, and the second is the worse one: - **Mail amplification.** Unlimited password-reset mail to any address anyone chooses, sent by our SMTP identity. That is a deliverability and reputation problem as much as a nuisance to the recipient. - **The limit is invisible in its failure.** Nothing distinguishes "the limiter is working" from "every caller is being counted separately", because both look like traffic under the threshold. This is the same shape as #67, #60, #61 and the new-code period in #79 — a control that reports success while doing nothing. ## The fix `express-rate-limit` 8.6.2 is already installed and exports the helper the warning names: ```ts export declare function ipKeyGenerator(ip: string, ipv6Subnet?: number | false): string; ``` It normalises an IPv6 address to its subnet prefix and leaves IPv4 alone, so one caller counts as one caller either way. The change is to build the key from `ipKeyGenerator(req.ip)` rather than `req.ip`. Worth deciding rather than defaulting: the helper's `ipv6Subnet` defaults to /56 rather than /64. A /56 groups a whole delegated site, which is the safer choice against a rotating attacker but means several households behind one delegation share an allowance. Given the allowance is per email address as well as per caller, the collision cost looks acceptable — but it is a deliberate trade and should be recorded in the comment next to it, not left implicit. ## Also worth doing `trust proxy` is set to `1` in `app.ts`, so `req.ip` is taken from `X-Forwarded-For`. That is correct behind Nginx Proxy Manager, and it means the key is the real client rather than the proxy — but it also means the limiter inherits whatever NPM forwards. Confirming NPM actually sets `X-Forwarded-For` for IPv6 clients in the same form is part of testing this properly, rather than assuming the header shape. A test would be the honest way to close this, but the existing limiter has none, and the integration suite's rate-limit state is process-wide — #62 recorded that deliberately not asserting a limiter there was the right call, for the same reason. Verifying by unit-testing the key function directly, rather than by exhausting an allowance, avoids that problem entirely. ## Severity Medium-high. Nothing is broken, the app runs, and no incident has occurred — but a security control that exists specifically to prevent mail abuse is not doing its job for IPv6 clients, and has been announcing that at every boot.

bermudalamb commented

2026-08-20 18:34:48 -05:00

Fixed on `feature/84-ipv6-rate-limit-key` — one commit, not pushed

The caller half of the key now goes through ipKeyGenerator:

export function keyByCallerAndEmail(req: Request): string {
  const email = typeof req.body?.email === 'string' ? req.body.email.toLowerCase().trim() : '';
  return `${ipKeyGenerator(req.ip ?? '')}:${email}`;
}

The /56 default is kept, deliberately

Confirmed against the installed implementation rather than the documentation — ipKeyGenerator(ip, ipv6Subnet = 56). Keeping it means a whole delegated site shares one bucket, so an attacker cannot escape by moving within their own allocation. The cost is that several households behind one delegation share an allowance, which is acceptable only because the key also carries the email address, so they collide just when targeting the same account.

That reasoning is recorded next to the code, because someone tightening it to /64 later on symmetry grounds would silently reopen the hole and nothing would fail.

Verified by firing the guard, not by reasoning about it

	Result
Build `main`, load `dist/rateLimit.js`	`ValidationError … ERR_ERL_KEY_GEN_IPV6` — reproduced
Same load with this change	silent

Both directions, because a warning that stopped appearing for some unrelated reason would look identical to a fix.

The helper's actual behaviour was also checked directly rather than assumed, and the tests assert the real values it returns:

Input	Key
`203.0.113.5`	`203.0.113.5`
`2001:db8:abcd:0100::1`	`2001:db8:abcd:100::/56`
`2001:db8:abcd:01ff::9`	`2001:db8:abcd:100::/56` — same bucket, the bug
`2001:db8:abcd:0200::1`	`2001:db8:abcd:200::/56` — different site, kept apart
`::ffff:203.0.113.5`	`203.0.113.5` — mapped address keys as the IPv4 caller

Testing approach, and what is deliberately not tested

keyByCallerAndEmail is now exported and unit-tested directly. Seven tests: IPv4 unchanged, two addresses in one delegation collapsing, separate delegations staying apart, IPv4-mapped equivalence, email normalisation, a non-string email, and a request with no address at all.

The limiter's allowance is still not asserted anywhere, and should not be. 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 unrelated. #62 reached the same conclusion for the client-error limiter. The key function is pure, and the bug lived in the key rather than the allowance — so this is both the safe thing to test and the right thing to test.

Results: 86 unit (79 + 7), 144 integration, lint 0 errors and 8 warnings, all unchanged from baseline.

Still open from the issue

The X-Forwarded-For question is untouched. trust proxy is 1, so req.ip comes from the header Nginx Proxy Manager sets, and I have not confirmed what NPM actually forwards for an IPv6 client. If it forwards something unexpected the key changes shape, though it would no longer be bypassable — ipKeyGenerator returns a non-IPv6 string unchanged, so the worst case is coarser grouping rather than none. Worth checking against a real IPv6 client before considering this closed for good.

Not pushed, per the usual arrangement.

## Fixed on `feature/84-ipv6-rate-limit-key` — one commit, not pushed The caller half of the key now goes through `ipKeyGenerator`: ```ts export function keyByCallerAndEmail(req: Request): string { const email = typeof req.body?.email === 'string' ? req.body.email.toLowerCase().trim() : ''; return `${ipKeyGenerator(req.ip ?? '')}:${email}`; } ``` ## The /56 default is kept, deliberately Confirmed against the installed implementation rather than the documentation — `ipKeyGenerator(ip, ipv6Subnet = 56)`. Keeping it means a whole delegated site shares one bucket, so an attacker cannot escape by moving within their own allocation. The cost is that several households behind one delegation share an allowance, which is acceptable only because the key also carries the email address, so they collide just when targeting the same account. That reasoning is recorded next to the code, because someone tightening it to /64 later on symmetry grounds would silently reopen the hole and nothing would fail. ## Verified by firing the guard, not by reasoning about it | | Result | | --- | --- | | Build `main`, load `dist/rateLimit.js` | `ValidationError … ERR_ERL_KEY_GEN_IPV6` — reproduced | | Same load with this change | silent | Both directions, because a warning that stopped appearing for some unrelated reason would look identical to a fix. The helper's actual behaviour was also checked directly rather than assumed, and the tests assert the real values it returns: | Input | Key | | --- | --- | | `203.0.113.5` | `203.0.113.5` | | `2001:db8:abcd:0100::1` | `2001:db8:abcd:100::/56` | | `2001:db8:abcd:01ff::9` | `2001:db8:abcd:100::/56` — same bucket, the bug | | `2001:db8:abcd:0200::1` | `2001:db8:abcd:200::/56` — different site, kept apart | | `::ffff:203.0.113.5` | `203.0.113.5` — mapped address keys as the IPv4 caller | ## Testing approach, and what is deliberately not tested `keyByCallerAndEmail` is now exported and unit-tested directly. Seven tests: IPv4 unchanged, two addresses in one delegation collapsing, separate delegations staying apart, IPv4-mapped equivalence, email normalisation, a non-string email, and a request with no address at all. The limiter's **allowance** is still not asserted anywhere, and should not be. 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 unrelated. #62 reached the same conclusion for the client-error limiter. The key function is pure, and the bug lived in the key rather than the allowance — so this is both the safe thing to test and the right thing to test. Results: **86 unit** (79 + 7), **144 integration**, lint 0 errors and 8 warnings, all unchanged from baseline. ## Still open from the issue The `X-Forwarded-For` question is untouched. `trust proxy` is `1`, so `req.ip` comes from the header Nginx Proxy Manager sets, and I have not confirmed what NPM actually forwards for an IPv6 client. If it forwards something unexpected the key changes shape, though it would no longer be *bypassable* — `ipKeyGenerator` returns a non-IPv6 string unchanged, so the worst case is coarser grouping rather than none. Worth checking against a real IPv6 client before considering this closed for good. Not pushed, per the usual arrangement.

bermudalamb referenced this issue from a commit

2026-08-20 18:43:29 -05:00

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

bermudalamb added reference feature/84-ipv6-rate-limit-key

2026-08-20 18:44:02 -05:00

bermudalamb added this to the Code Quality and Hardening project 2026-08-20 18:44:09 -05:00

bermudalamb moved this to Review in Code Quality and Hardening on 2026-08-20 18:44:14 -05:00

bermudalamb self-assigned this 2026-08-20 18:44:20 -05:00

bermudalamb referenced this issue from a commit

2026-08-20 18:44:51 -05:00

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

bermudalamb moved this to Ready for Release in Code Quality and Hardening on 2026-08-21 09:46:08 -05:00