A verification email was sent once, at registration. If it was lost, filtered, or arrived after the 24-hour token had expired, the customer had no route back except registering again with a different address. POST /api/customers/resend-verification mints a fresh token and sends the mail, and the account page grows a "Send it again" button beside the warning that was already there. The button only exists while the address is unverified: on a verified account it would be a control whose only outcome is a refusal, and the endpoint refuses that case rather than sending a pointless email. The mint-token-and-send sequence now lives in one place. Registration and the email change already did the same three steps - supersede any outstanding token, mint a new one, send it - and this would have been a third copy. The step most likely to be dropped in a copy is the supersede, and it is the one that matters: without it an older message still sitting in the inbox goes on verifying. Anything that makes the server send mail on request is an abuse vector, so this is rate limited to three an hour, keyed on the customer id. That is tighter than either existing limiter and sidesteps #84's IPv6 problem entirely, since a signed-in caller has an identity better than an address to count against and cannot escape the bucket by moving within a delegated prefix. The refusal says the mail probably did send and to check the spam folder, which is both more useful and more honest than a bare 429. The claim that keying on customer id also solved test isolation was wrong, and the tests caught it. resetDb truncates with RESTART IDENTITY, so every integration test's first customer is id 1: three tests that each sent once left the fourth starting at its limit, and two tests failed on a 429 they never asked for. A "fresh customer per test" is a fresh row with a recycled identity. The limiter now has an explicit exported store the suite clears between tests, and the comment that claimed otherwise has been corrected rather than left to mislead the next reader. Verification: seven integration tests covering the unauthenticated refusal, the send, the new token invalidating the old one - asserted through the endpoint that would honour the stale link rather than by counting rows - the new link actually verifying, the already-verified refusal, the allowance stopping the fourth send rather than merely reporting it, and one customer's exhausted allowance leaving another's intact. Three end-to-end tests for the button, its confirmation and the message on the fourth click. The 33 integration tests across the three suites this touched all pass, as do the 199 backend unit tests. tsc clean on both sides, ESLint no errors. Closes #110 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
127 lines
6.2 KiB
TypeScript
127 lines
6.2 KiB
TypeScript
import rateLimit, { ipKeyGenerator, MemoryStore } from 'express-rate-limit';
|
|
import { Request } from 'express';
|
|
|
|
// First rate limiting in the codebase. The password-reset endpoints need it
|
|
// most: without one, anyone can make the server send unlimited mail to any
|
|
// address. Login and registration are the obvious next candidates.
|
|
//
|
|
// The default in-memory store suits a single-instance deployment, which this
|
|
// is. Running more than one app container would need a shared store, or each
|
|
// instance would enforce its own separate allowance.
|
|
|
|
const WINDOW_MS = 15 * 60 * 1000;
|
|
const MAX_REQUESTS = 5;
|
|
|
|
// Keyed on caller *and* address rather than caller alone. Keying on IP only
|
|
// would let one person's reset attempts lock out everyone behind the same
|
|
// NAT or reverse proxy — and everything here arrives via Nginx Proxy Manager,
|
|
// so a great many customers share an apparent address.
|
|
//
|
|
// 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.
|
|
//
|
|
// 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 `${ipKeyGenerator(req.ip ?? '')}:${email}`;
|
|
}
|
|
|
|
export const passwordResetRequestLimiter = rateLimit({
|
|
windowMs: WINDOW_MS,
|
|
limit: MAX_REQUESTS,
|
|
keyGenerator: keyByCallerAndEmail,
|
|
standardHeaders: 'draft-7',
|
|
legacyHeaders: false,
|
|
message: { error: 'too many attempts, please try again later' }
|
|
});
|
|
|
|
// Client error reports carry no email, so this one is keyed on the caller
|
|
// alone — deliberately not reusing passwordResetRequestLimiter, whose comment
|
|
// above explains why its key is wrong for an endpoint without an email.
|
|
//
|
|
// `trust proxy` is set in app.ts, so `req.ip` is the real client address from
|
|
// X-Forwarded-For rather than Nginx Proxy Manager's, making this a per-customer
|
|
// allowance rather than one shared by everybody behind the proxy.
|
|
//
|
|
// Generous, because hitting the limit is harmless: the reporter ignores the
|
|
// response either way. It exists so a render loop cannot fill the log.
|
|
const CLIENT_ERROR_WINDOW_MS = 15 * 60 * 1000;
|
|
const CLIENT_ERROR_MAX_REQUESTS = 30;
|
|
|
|
export const clientErrorLimiter = rateLimit({
|
|
windowMs: CLIENT_ERROR_WINDOW_MS,
|
|
limit: CLIENT_ERROR_MAX_REQUESTS,
|
|
standardHeaders: 'draft-7',
|
|
legacyHeaders: false,
|
|
message: { error: 'too many reports' }
|
|
});
|
|
|
|
// Resending a verification email makes the server send mail on request, which
|
|
// is the same class of endpoint as password reset and needs the same treatment.
|
|
//
|
|
// Keyed on the customer id, which is tighter than either limiter above and
|
|
// sidesteps the IPv6 problem of #84 entirely: the caller is signed in, so there
|
|
// is an identity better than an address to count against, and no amount of
|
|
// moving within a delegated prefix changes it. It also means one customer
|
|
// cannot spend anyone else's allowance, which keying on IP would allow.
|
|
//
|
|
// It does NOT make the store's process-wide lifetime a non-issue for tests, as
|
|
// was assumed at first. resetDb truncates with RESTART IDENTITY, so every
|
|
// integration test's first customer is id 1 and they all share one bucket:
|
|
// three tests that each send once exhaust the allowance for the fourth. The
|
|
// store below is explicit and exported so a test can clear it, rather than
|
|
// tests being written around an allowance they cannot see.
|
|
//
|
|
// Must be mounted *after* requireCustomer. Before it, req.customerId is
|
|
// undefined and every anonymous caller would share a single bucket — the same
|
|
// collapse the passwordResetRequestLimiter comment warns about.
|
|
export function keyByCustomer(req: Request): string {
|
|
return `customer:${req.customerId ?? 'anonymous'}`;
|
|
}
|
|
|
|
// Three an hour is generous for someone who genuinely lost the mail, and
|
|
// useless to anybody hammering it. The window is longer than the 15 minutes
|
|
// used above because the failure it guards against is slower: a verification
|
|
// link lasts 24 hours, so there is no reason to want a fourth inside an hour.
|
|
const VERIFICATION_RESEND_WINDOW_MS = 60 * 60 * 1000;
|
|
const VERIFICATION_RESEND_MAX = 3;
|
|
|
|
// Exported only so the integration suite can clear it between tests. See the
|
|
// note above: recycled customer ids make the allowance leak across tests.
|
|
export const verificationResendStore = new MemoryStore();
|
|
|
|
export const verificationResendLimiter = rateLimit({
|
|
windowMs: VERIFICATION_RESEND_WINDOW_MS,
|
|
limit: VERIFICATION_RESEND_MAX,
|
|
keyGenerator: keyByCustomer,
|
|
store: verificationResendStore,
|
|
standardHeaders: 'draft-7',
|
|
legacyHeaders: false,
|
|
// Says what actually happened rather than only that a limit was hit. The mail
|
|
// almost certainly did send, so "check your spam folder" is both the more
|
|
// useful instruction and the more honest one.
|
|
message: {
|
|
error: 'we have already sent several verification emails recently. Check your spam folder, and try again later.'
|
|
}
|
|
});
|