diff --git a/backend/src/rateLimit.ts b/backend/src/rateLimit.ts index d3cc542..8517b9b 100644 --- a/backend/src/rateLimit.ts +++ b/backend/src/rateLimit.ts @@ -1,4 +1,4 @@ -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit, { ipKeyGenerator, MemoryStore } from 'express-rate-limit'; import { Request } from 'express'; // First rate limiting in the codebase. The password-reset endpoints need it @@ -75,3 +75,52 @@ export const clientErrorLimiter = rateLimit({ 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.' + } +}); diff --git a/backend/src/routes/customers.ts b/backend/src/routes/customers.ts index c92fd39..9364729 100755 --- a/backend/src/routes/customers.ts +++ b/backend/src/routes/customers.ts @@ -9,12 +9,46 @@ import { loadStoredTemplate } from './adminEmailTemplates'; import { MARKETING_CONSENT_TEXT, isValidEmail } from '../utils'; import { FAVORITE_ALERTS_CONSENT_TEXT } from '../favoriteAlerts'; import { asyncRoute } from '../asyncRoute'; -import { passwordResetRequestLimiter } from '../rateLimit'; +import { passwordResetRequestLimiter, verificationResendLimiter } from '../rateLimit'; const router = Router(); const SESSION_DAYS = 30; +const VERIFY_TOKEN_TTL_MS = 24 * 60 * 60 * 1000; + +// Registration, changing an address, and resending all need the same three +// steps: supersede any outstanding link, mint a new one, send it. Written out +// three times they would drift, and the step most likely to be forgotten is the +// first — which is the one that matters, since an un-superseded link means an +// older message in the inbox still verifies. +// +// Sending is fire-and-forget by the same rule the rest of this file follows: +// the token row is written first, so a send that fails cannot leave a customer +// believing a link exists that does not, only waiting for one that never came. +async function issueVerificationEmail( + customerId: number, + email: string, + firstName: string | null +): Promise { + await pool.query( + `DELETE FROM customer_tokens WHERE customer_id = $1 AND kind = 'verify_email'`, + [customerId] + ); + const token = crypto.randomBytes(24).toString('hex'); + await pool.query( + `INSERT INTO customer_tokens (token, customer_id, kind, expires_at) VALUES ($1, $2, 'verify_email', $3)`, + [token, customerId, new Date(Date.now() + VERIFY_TOKEN_TTL_MS)] + ); + const verifyUrl = `${process.env.PUBLIC_URL}/verify-email?token=${token}`; + const template = renderTemplate('verification', await loadStoredTemplate('verification'), { + greeting: greeting(firstName), + verifyUrl + }); + sendMail(email, template.subject, template.html) + .catch(err => console.error('verify email send failed', err)); +} + function setSessionCookie(res: Response, token: string) { res.cookie('rd_session', token, { httpOnly: true, @@ -99,18 +133,7 @@ router.post('/register', asyncRoute(async (req: Request, res: Response) => { ); const customer = rows[0]; - const verifyToken = crypto.randomBytes(24).toString('hex'); - await pool.query( - `INSERT INTO customer_tokens (token, customer_id, kind, expires_at) VALUES ($1, $2, 'verify_email', $3)`, - [verifyToken, customer.id, new Date(Date.now() + 24 * 60 * 60 * 1000)] - ); - const verifyUrl = `${process.env.PUBLIC_URL}/verify-email?token=${verifyToken}`; - const verifyTemplate = renderTemplate('verification', await loadStoredTemplate('verification'), { - greeting: greeting(customer.first_name), - verifyUrl - }); - sendMail(customer.email, verifyTemplate.subject, verifyTemplate.html) - .catch(err => console.error('verify email send failed', err)); + await issueVerificationEmail(customer.id, customer.email, customer.first_name); const sessionToken = await createSession(customer.id); setSessionCookie(res, sessionToken); @@ -129,6 +152,28 @@ router.post('/verify-email', asyncRoute(async (req: Request, res: Response) => { res.json({ status: 'verified' }); })); +// The limiter is mounted after requireCustomer, deliberately: it keys on +// req.customerId, which does not exist until requireCustomer has run. Mounted +// the other way round every anonymous caller would share one bucket. +router.post( + '/resend-verification', + requireCustomer, + verificationResendLimiter, + asyncRoute(async (req: Request, res: Response) => { + const { rows } = await pool.query(`SELECT * FROM customers WHERE id = $1`, [req.customerId]); + const customer = rows[0]; + + // Refused rather than quietly sending. A pointless email is worse than an + // answer, and the account page has no reason to offer the button here. + if (customer.email_verified) { + return res.status(400).json({ error: 'your email address is already verified' }); + } + + await issueVerificationEmail(customer.id, customer.email, customer.first_name); + res.status(204).end(); + }) +); + const RESET_TOKEN_TTL_MS = 60 * 60 * 1000; // Always answers 200, whether or not the address has an account. A response @@ -386,28 +431,13 @@ router.put('/me/email', requireCustomer, asyncRoute(async (req: Request, res: Re [normalized, req.customerId] ); - // Supersede any outstanding link, so one already sitting in the old inbox - // cannot be used to verify the new address. - await pool.query( - `DELETE FROM customer_tokens WHERE customer_id = $1 AND kind = 'verify_email'`, - [req.customerId] - ); - const verifyToken = crypto.randomBytes(24).toString('hex'); - await pool.query( - `INSERT INTO customer_tokens (token, customer_id, kind, expires_at) VALUES ($1, $2, 'verify_email', $3)`, - [verifyToken, req.customerId, new Date(Date.now() + 24 * 60 * 60 * 1000)] - ); - + // Supersedes any outstanding link as part of issuing the new one, so a + // message already sitting in the old inbox cannot verify the new address. + // // Both sends happen after the row is written, never before — the same rule // favoriteAlerts follows, so a change that failed cannot produce mail saying // it succeeded. - const verifyUrl = process.env.PUBLIC_URL + '/verify-email?token=' + verifyToken; - const verify = renderTemplate('verification', await loadStoredTemplate('verification'), { - greeting: greeting(customer.first_name), - verifyUrl - }); - sendMail(normalized, verify.subject, verify.html) - .catch(err => console.error('verify email send failed', err)); + await issueVerificationEmail(req.customerId as number, normalized, customer.first_name); const notice = renderTemplate('emailChanged', await loadStoredTemplate('emailChanged'), { greeting: greeting(customer.first_name), diff --git a/backend/tests/integration/resendVerification.integration.test.ts b/backend/tests/integration/resendVerification.integration.test.ts new file mode 100644 index 0000000..9e23535 --- /dev/null +++ b/backend/tests/integration/resendVerification.integration.test.ts @@ -0,0 +1,150 @@ +import request from 'supertest'; +import app from '../../src/app'; +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; +import { verificationResendStore } from '../../src/rateLimit'; + +jest.mock('../../src/mailer', () => ({ + sendMail: jest.fn().mockResolvedValue(undefined) +})); +import { sendMail } from '../../src/mailer'; +const sentMail = sendMail as jest.MockedFunction; + +const PASSWORD = 'supersecret123'; + +beforeEach(async () => { + await resetDb(); + sentMail.mockClear(); + // resetDb truncates with RESTART IDENTITY, so every test's first customer is + // id 1 and the limiter — keyed on customer id, with a process-wide store — + // hands them all the same bucket. Without this, three tests that each send + // once leave the fourth starting at its limit. + await verificationResendStore.resetAll?.(); +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +// Each test registers its own customer. That alone does NOT isolate the +// allowance, which is what the beforeEach above is for: RESTART IDENTITY hands +// every test the same customer id, so "a fresh customer" is a fresh row with a +// recycled identity. This is the same class of leakage #62 and #84 recorded, +// surviving a key that looked like it had solved it. +async function register(email: string) { + const agent = request.agent(app); + const res = await agent + .post('/api/customers/register') + .send({ email, password: PASSWORD, firstName: 'Thom', lastName: 'Lamb' }); + expect(res.status).toBe(200); + sentMail.mockClear(); + return agent; +} + +const tokensFor = async (email: string) => { + const { rows } = await pool.query( + `SELECT t.token FROM customer_tokens t + JOIN customers c ON c.id = t.customer_id + WHERE c.email = $1 AND t.kind = 'verify_email'`, + [email] + ); + return rows.map(r => r.token as string); +}; + +describe('resending your own verification email', () => { + it('refuses an unauthenticated caller', async () => { + const res = await request(app).post('/api/customers/resend-verification'); + expect(res.status).toBe(401); + expect(sentMail).not.toHaveBeenCalled(); + }); + + it('sends to the address on the account', async () => { + const email = 'resend1@example.com'; + const agent = await register(email); + + const res = await agent.post('/api/customers/resend-verification'); + + expect(res.status).toBe(204); + expect(sentMail).toHaveBeenCalledTimes(1); + expect(String(sentMail.mock.calls[0][0])).toBe(email); + }); + + // The point of the whole thing. An un-superseded link means a message still + // sitting in the inbox goes on working, which is the case the supersede in + // issueVerificationEmail exists to prevent. + it('mints a new token and invalidates the previous one', async () => { + const email = 'resend2@example.com'; + const agent = await register(email); + + const [before] = await tokensFor(email); + expect(before).toBeDefined(); + + await agent.post('/api/customers/resend-verification'); + + const after = await tokensFor(email); + expect(after).toHaveLength(1); + expect(after[0]).not.toBe(before); + + // And the old link is genuinely dead, asserted through the endpoint that + // would honour it rather than by counting rows. + const stale = await request(app).post('/api/customers/verify-email').send({ token: before }); + expect(stale.status).toBe(400); + }); + + it('the new link verifies the address', async () => { + const email = 'resend3@example.com'; + const agent = await register(email); + + await agent.post('/api/customers/resend-verification'); + const [token] = await tokensFor(email); + + const res = await request(app).post('/api/customers/verify-email').send({ token }); + + expect(res.status).toBe(200); + const me = await agent.get('/api/customers/me'); + expect(me.body.email_verified).toBe(true); + }); + + it('refuses once the address is already verified', async () => { + const email = 'resend4@example.com'; + const agent = await register(email); + const [token] = await tokensFor(email); + await request(app).post('/api/customers/verify-email').send({ token }); + sentMail.mockClear(); + + const res = await agent.post('/api/customers/resend-verification'); + + expect(res.status).toBe(400); + expect(res.body.error).toBe('your email address is already verified'); + expect(sentMail).not.toHaveBeenCalled(); + }); + + // Three per hour. The fourth is refused, and the message says what actually + // happened rather than only that a limit exists. + it('stops after the allowance, with a message worth reading', async () => { + const agent = await register('resend5@example.com'); + + for (let i = 0; i < 3; i++) { + expect((await agent.post('/api/customers/resend-verification')).status).toBe(204); + } + + const fourth = await agent.post('/api/customers/resend-verification'); + + expect(fourth.status).toBe(429); + expect(String(fourth.body.error)).toContain('spam folder'); + // Refused rather than merely reported: the fourth send must not have gone. + expect(sentMail).toHaveBeenCalledTimes(3); + }); + + // The allowance is per customer, not per address or per caller. Keying it any + // more coarsely would let one customer spend everybody else's. + it('one customer exhausting the allowance does not affect another', async () => { + const first = await register('resend6@example.com'); + for (let i = 0; i < 3; i++) await first.post('/api/customers/resend-verification'); + expect((await first.post('/api/customers/resend-verification')).status).toBe(429); + + const second = await register('resend7@example.com'); + expect((await second.post('/api/customers/resend-verification')).status).toBe(204); + }); +}); diff --git a/frontend/src/customer/Account.tsx b/frontend/src/customer/Account.tsx index 9156c7f..34aa07f 100755 --- a/frontend/src/customer/Account.tsx +++ b/frontend/src/customer/Account.tsx @@ -1,4 +1,4 @@ -import { useEffect } from 'react'; +import { useEffect, useState } from 'react'; import Typography from 'antd/es/typography'; import Switch from 'antd/es/switch'; import Button from 'antd/es/button'; @@ -7,7 +7,7 @@ import message from 'antd/es/message'; import Space from 'antd/es/space'; import Divider from 'antd/es/divider'; import { useNavigate } from 'react-router-dom'; -import { updateConsent, exportMyData, deleteMyAccount } from './customerApi'; +import { updateConsent, exportMyData, deleteMyAccount, resendVerificationEmail } from './customerApi'; import { setFavoriteAlerts } from './favoritesApi'; import { useCustomerAuth } from './CustomerAuthContext'; import AccountDetails from './AccountDetails'; @@ -23,6 +23,7 @@ interface Props { export default function Account({ onClose }: Props) { const { customer, loading, refresh, logout } = useCustomerAuth(); const navigate = useNavigate(); + const [resending, setResending] = useState(false); useEffect(() => { if (!loading && !customer) navigate('/login'); @@ -30,6 +31,20 @@ export default function Account({ onClose }: Props) { if (!customer) return null; + async function handleResendVerification() { + setResending(true); + try { + await resendVerificationEmail(); + message.success('Sent. Check your inbox, and your spam folder.'); + } catch (err) { + // Shown as it arrives: the rate limit's message says the mail probably + // did send and where to look, which a generic failure would throw away. + message.error((err as Error).message); + } finally { + setResending(false); + } + } + async function handleFavoriteAlertsToggle(checked: boolean) { try { await setFavoriteAlerts(checked); @@ -99,9 +114,17 @@ export default function Account({ onClose }: Props) { >
{customer.email} + {/* The button only exists while there is something to verify. Offering + it on a verified account would be a control whose only outcome is a + refusal. */} {!customer.email_verified && (
Email not verified — check your inbox for a verification link. +
+ +
)} diff --git a/frontend/src/customer/customerApi.ts b/frontend/src/customer/customerApi.ts index 8403aec..b1f9e32 100755 --- a/frontend/src/customer/customerApi.ts +++ b/frontend/src/customer/customerApi.ts @@ -142,3 +142,16 @@ export function changeMyEmail(currentPassword: string, email: string): Promise handle(res)); } + +export function resendVerificationEmail(): Promise { + // 204 on success, so handle() would throw parsing an empty body. The failure + // path must still reject: the server's message distinguishes "already + // verified" from the rate limit's "check your spam folder", and both are + // worth showing rather than replacing with something generic. + return fetch('/api/customers/resend-verification', { method: 'POST' }).then(async (res) => { + if (!res.ok) { + const data = await res.json().catch(() => ({})); + throw new Error(data.error || 'Request failed'); + } + }); +} diff --git a/frontend/tests/e2e/resend-verification.spec.ts b/frontend/tests/e2e/resend-verification.spec.ts new file mode 100644 index 0000000..4de796c --- /dev/null +++ b/frontend/tests/e2e/resend-verification.spec.ts @@ -0,0 +1,63 @@ +import { test, expect, Page } from './fixtures'; + +const PASSWORD = 'supersecret123'; + +const uniqueEmail = () => `resend-${Date.now().toString(36)}${Math.random().toString(36).slice(2, 7)}@example.com`; + +// The generous wait matches the other account specs: registration is a bcrypt +// round-trip rather than a render, and runs past Playwright's 5s default when +// the suite's workers all register at once. +async function registerCustomer(page: Page): Promise { + const email = uniqueEmail(); + await page.goto('/register'); + await page.getByRole('textbox', { name: 'Email' }).fill(email); + await page.getByRole('textbox', { name: 'First name' }).fill('Test'); + await page.getByRole('textbox', { name: 'Last name' }).fill('Customer'); + await page.getByLabel('Password').fill(PASSWORD); + await page.getByRole('button', { name: 'Create account' }).click(); + await expect(page.getByRole('button', { name: 'My Account' })).toBeVisible({ timeout: 20000 }); + return email; +} + +const accountModal = (page: Page) => page.getByRole('dialog', { name: 'My Account' }); + +test.describe('Resending your verification email', () => { + test('the account page offers it while the address is unverified', async ({ page }) => { + await registerCustomer(page); + await page.goto('/account'); + + const modal = accountModal(page); + await expect(modal.getByText('Email not verified')).toBeVisible(); + await expect(modal.getByRole('button', { name: 'Send it again' })).toBeVisible(); + }); + + test('confirms when it has sent', async ({ page }) => { + await registerCustomer(page); + await page.goto('/account'); + + await accountModal(page).getByRole('button', { name: 'Send it again' }).click(); + + await expect(page.getByText('Check your inbox, and your spam folder.')).toBeVisible(); + }); + + // The message the customer gets on the fourth attempt is the point of the + // limiter's copy: it says the mail probably did send and where to look, + // rather than only that a limit exists. + test('says something useful once the allowance runs out', async ({ page }) => { + await registerCustomer(page); + await page.goto('/account'); + + const resend = accountModal(page).getByRole('button', { name: 'Send it again' }); + for (let i = 0; i < 3; i++) { + await resend.click(); + await expect(resend).toBeEnabled(); + } + + await resend.click(); + + // Matched on the phrase unique to the refusal. "spam folder" appears in the + // success message too, and the three stacked success toasts from the loop + // above are still on screen, so the looser match finds them instead. + await expect(page.getByText(/already sent several/)).toBeVisible(); + }); +});