diff --git a/backend/src/emailTemplates.ts b/backend/src/emailTemplates.ts index 68594d0..1e8cd16 100644 --- a/backend/src/emailTemplates.ts +++ b/backend/src/emailTemplates.ts @@ -68,6 +68,12 @@ export const TEMPLATES: Record = { defaultBody: 'Someone asked to reset the password for this account.\n\n' + '[Choose a new password]({{resetUrl}}). This link expires in {{expiresIn}}.\n\n' + + // Said before the customer follows the link rather than after they have + // used it, because it is the one consequence of a reset they cannot undo + // and might have chosen differently about (#42). Worded so it reads the + // same to someone who has never registered one. + 'Resetting your password also removes any passkeys saved on this account, ' + + 'and signs you out everywhere. You can add your passkeys again afterwards.\n\n' + "If this wasn't you, you can ignore this email — your password has not changed." }, diff --git a/backend/src/routes/customers.ts b/backend/src/routes/customers.ts index f1f9ee3..b90f4a9 100755 --- a/backend/src/routes/customers.ts +++ b/backend/src/routes/customers.ts @@ -374,6 +374,11 @@ router.post('/reset-password', asyncRoute(async (req: Request, res: Response) => const passwordHash = await bcrypt.hash(String(password), PASSWORD_HASH_ROUNDS); + // Reported back so the customer is told, rather than finding an empty list + // the next time they look. Declared out here because it is decided inside the + // transaction and read after it. + let passkeysRemoved = 0; + const client = await pool.connect(); try { await client.query('BEGIN'); @@ -389,6 +394,37 @@ router.post('/reset-password', asyncRoute(async (req: Request, res: Response) => // up to 30 days. await client.query(`DELETE FROM customer_sessions WHERE customer_id = $1`, [customerId]); await client.query(`DELETE FROM customer_tokens WHERE customer_id = $1 AND kind = 'password_reset'`, [customerId]); + + // Passkeys go with the sessions, for the same reason and more of it (#42). + // + // A reset is the recovery path, and recovery has to be complete. The line + // above already takes the position that a reset must evict anyone else + // holding the account — a session an intruder holds lasts up to 30 days, and + // a passkey an intruder registered lasts forever. Leaving those behind would + // mean a customer can recover their password and still not have their + // account back. + // + // The obvious objection is that this lets whoever controls the mailbox strip + // a customer's passkeys. It does, and it costs nothing: anyone who can + // complete a reset already controls the email address, and therefore already + // controls the account. The passkeys were not protecting anything at that + // point. + // + // Deliberately NOT the same rule as change-password, which leaves passkeys + // alone. That one requires the current password from someone already signed + // in — no part of it suggests a lockout or a compromise, and a customer who + // suspects one device can revoke that device by name on the account page + // (#40). This path has no idea which credential is the problem, so it takes + // all of them. + const removed = await client.query(`DELETE FROM customer_credentials WHERE customer_id = $1`, [customerId]); + passkeysRemoved = removed.rowCount ?? 0; + + // Including anything in flight. A registration challenge issued to an + // intruder moments before the reset would otherwise still be completable + // afterwards, which would put a passkey back on the account the reset just + // cleared. + await client.query(`DELETE FROM webauthn_challenges WHERE customer_id = $1`, [customerId]); + await client.query('COMMIT'); } catch (err) { await client.query('ROLLBACK'); @@ -400,7 +436,15 @@ router.post('/reset-password', asyncRoute(async (req: Request, res: Response) => const { rows: fresh } = await pool.query(`SELECT * FROM customers WHERE id = $1`, [customerId]); const sessionToken = await createSession(customerId); setSessionCookie(res, sessionToken); - res.json(publicCustomer(requireRow(fresh, 'the customer whose password was just reset'))); + // The count rides along with the customer rather than being left for the + // account page to imply. A customer who never registered a passkey sees zero + // and is told nothing; one who is told two were removed and only remembers + // registering one has just learned something they could not otherwise find + // out — the row is already gone by the time they could go looking. + res.json({ + ...publicCustomer(requireRow(fresh, 'the customer whose password was just reset')), + passkeysRemoved + }); })); router.post('/login', asyncRoute(async (req: Request, res: Response) => { diff --git a/backend/tests/integration/passwordReset.integration.test.ts b/backend/tests/integration/passwordReset.integration.test.ts index 20792ef..5969a7d 100644 --- a/backend/tests/integration/passwordReset.integration.test.ts +++ b/backend/tests/integration/passwordReset.integration.test.ts @@ -1,6 +1,7 @@ import request from 'supertest'; import app from '../../src/app'; -import { pool } from '../../src/db'; +import { pool, requireRow } from '../../src/db'; +import { createSession } from '../../src/customerSession'; import { resetDb, closeDb } from './setup/testDb'; beforeEach(async () => { @@ -204,4 +205,131 @@ describe('POST /api/customers/reset-password', () => { const retry = await request(app).post('/api/customers/reset-password').send({ token, password: 'long-enough-password' }); expect(retry.status).toBe(200); }); + + // #42. A reset is the recovery path, so it has to leave the account with no + // way in that the customer did not just establish. Sessions were already + // covered above; a passkey outlives a session without bound. + describe('and the passkeys on the account', () => { + async function customerId(email: string): Promise { + const { rows } = await pool.query<{ id: number }>(`SELECT id FROM customers WHERE email = $1`, [email]); + return requireRow(rows, 'the customer this test just registered').id; + } + + // Registering one for real needs an authenticator, which no test has. The + // row is what the reset acts on, so the row is what these insert. + async function giveAPasskey(id: number, credentialId: string): Promise { + await pool.query( + `INSERT INTO customer_credentials (customer_id, credential_id, public_key, name) + VALUES ($1, $2, 'not-a-real-key', 'Test key')`, + [id, credentialId] + ); + } + + async function passkeyCount(id: number): Promise { + const { rows } = await pool.query<{ n: number }>( + `SELECT count(*)::int AS n FROM customer_credentials WHERE customer_id = $1`, + [id] + ); + return requireRow(rows, 'a count of credentials').n; + } + + it('removes every passkey, so one an intruder registered does not survive it', async () => { + await register('haskeys@example.com'); + const id = await customerId('haskeys@example.com'); + await giveAPasskey(id, 'credential-one'); + await giveAPasskey(id, 'credential-two'); + + const token = await requestReset('haskeys@example.com'); + const res = await request(app) + .post('/api/customers/reset-password') + .send({ token, password: 'a-brand-new-password' }); + + expect(res.status).toBe(200); + expect(await passkeyCount(id)).toBe(0); + }); + + it('says how many it removed, because nothing else can report it afterwards', async () => { + await register('counted@example.com'); + const id = await customerId('counted@example.com'); + await giveAPasskey(id, 'credential-counted'); + + const token = await requestReset('counted@example.com'); + const res = await request(app) + .post('/api/customers/reset-password') + .send({ token, password: 'a-brand-new-password' }); + + // The rows are gone by the time the customer could go and look, so a + // reset that removed something and said nothing would hide exactly the + // case worth knowing about. + expect(res.body.passkeysRemoved).toBe(1); + }); + + it('reports zero for a customer who never registered one', async () => { + await register('nokeys@example.com'); + + const token = await requestReset('nokeys@example.com'); + const res = await request(app) + .post('/api/customers/reset-password') + .send({ token, password: 'a-brand-new-password' }); + + // The notice on the reset form is shown on this number, so zero has to + // mean zero rather than undefined. + expect(res.status).toBe(200); + expect(res.body.passkeysRemoved).toBe(0); + }); + + it('leaves another customer’s passkeys alone', async () => { + await register('mine@example.com'); + await register('theirs@example.com'); + const mine = await customerId('mine@example.com'); + const theirs = await customerId('theirs@example.com'); + await giveAPasskey(mine, 'credential-mine'); + await giveAPasskey(theirs, 'credential-theirs'); + + const token = await requestReset('mine@example.com'); + await request(app).post('/api/customers/reset-password').send({ token, password: 'a-brand-new-password' }); + + expect(await passkeyCount(theirs)).toBe(1); + }); + + it('clears a challenge in flight, so a registration cannot land after the reset', async () => { + await register('inflight@example.com'); + const id = await customerId('inflight@example.com'); + await pool.query( + `INSERT INTO webauthn_challenges (challenge, customer_id, kind, expires_at) + VALUES ('challenge-in-flight', $1, 'registration', now() + interval '5 minutes')`, + [id] + ); + + const token = await requestReset('inflight@example.com'); + await request(app).post('/api/customers/reset-password').send({ token, password: 'a-brand-new-password' }); + + // Otherwise an intruder who pressed "add a passkey" moments earlier could + // finish the ceremony afterwards and put a credential back on the account + // the reset had just cleared. + const { rows } = await pool.query<{ n: number }>( + `SELECT count(*)::int AS n FROM webauthn_challenges WHERE customer_id = $1`, + [id] + ); + expect(requireRow(rows, 'a count of challenges').n).toBe(0); + }); + + it('terminates a session established by a passkey, not only one from a password', async () => { + await register('passkeysession@example.com'); + const id = await customerId('passkeysession@example.com'); + + // The call the passkey login route makes. Not an imitation of it — the + // same function, so this asserts the shared session path rather than + // asserting that two paths happen to agree today. + const passkeySession = await createSession(id); + const asPasskeyHolder = () => + request(app).get('/api/customers/me').set('Cookie', `rd_session=${passkeySession}`); + expect((await asPasskeyHolder()).status).toBe(200); + + const token = await requestReset('passkeysession@example.com'); + await request(app).post('/api/customers/reset-password').send({ token, password: 'a-brand-new-password' }); + + expect((await asPasskeyHolder()).status).toBe(401); + }); + }); }); diff --git a/docs/ops/account-recovery.md b/docs/ops/account-recovery.md new file mode 100644 index 0000000..01da6af --- /dev/null +++ b/docs/ops/account-recovery.md @@ -0,0 +1,102 @@ +# Account recovery + +What happens when a customer cannot get into their account, and what the shop +can do about it. Written for #42, which exists because this is the piece most +likely to be skipped and most expensive to discover missing. + +Every other issue in the passkeys project adds a capability. This one is the +safety net. + +## The short version + +| The customer has lost | They can recover by | Self-service | +| --- | --- | --- | +| Their password | A reset link emailed to them | Yes | +| Their passkey or the device holding it | Signing in with their password | Yes | +| Every passkey and their password | A reset link emailed to them | Yes | +| Access to their email address | Nothing. See below. | No | + +The email address is the root of trust. Every self-service route above ends at +it, and none of them can work without it. + +## A password reset removes every passkey + +This is the decision #42 existed to make, and it is deliberate. + +A reset is the recovery path, and recovery has to be complete. The reset already +deletes every session on the account, on the reasoning that a reset prompted by +a compromise must not leave an intruder signed in for the remaining 30 days of +their cookie. A passkey an intruder registered has no expiry at all. Leaving +those behind would mean a customer can recover their password and still not have +their account back. + +The obvious objection is that this lets whoever controls the mailbox strip a +customer's passkeys. It does, and it costs nothing: anyone who can complete a +reset already controls the email address and therefore already controls the +account. The passkeys were not protecting anything by that point. + +Consequences worth knowing: + +- The customer is told before they act. The reset email and the reset form both + say it, unconditionally. The form is not signed in and is never told whether + the account has passkeys, because answering that would make the reset page an + oracle for it. +- The customer is told after they act, with a count, and only when the count is + more than zero. This is the one moment that count can be reported: the rows + are gone by the time anyone could go and look. A customer told two were + removed who only remembers registering one has just learned something they + could not otherwise find out. +- Any WebAuthn challenge in flight goes too. An intruder who pressed "add a + passkey" moments before the reset could otherwise finish the ceremony + afterwards and put a credential straight back. + +## Changing a password does not remove passkeys + +The asymmetry with the paragraph above is intentional. + +`change-password` requires the current password from someone already signed in. +Nothing about that suggests a lockout or a compromise, and it already spares the +current session for the same reason. A customer who suspects one particular +device can revoke that device by name from the account page, which is a better +tool than deleting everything. + +A reset has no idea which credential is the problem, so it takes all of them. A +change knows the customer is present and in control, so it takes none. + +## Losing the authenticator is not a lockout + +A customer who loses the phone or key holding their passkey signs in with their +password as normal, and revokes the lost credential from the account page. This +needs no support involvement and no new capability, which is why #42 implements +nothing for it. + +This holds only while every account has a password. It stops holding when #332 +lands social sign-in, which creates the first customers with no password at all. +Their recovery route is the identity provider, not a reset link, and #332 owns +that question. The revocation guard in `backend/src/routes/passkeys.ts` already +refuses to delete a customer's only way in, written against that condition +rather than against today's schema, so it starts holding on its own the moment +the condition changes. + +## Losing the email address is a lockout + +There is no self-service recovery, and there should not be. Recovering an +account whose email is gone means proving identity some other way, and this shop +holds no other way — no phone number, no security questions, no identity +documents. Anything invented to fill that gap would be a weaker credential than +the one it replaces, and would become the easiest way to take an account over. + +The route is manual, and it runs through the shop owner: + +1. The customer makes contact by whatever means they have. +2. The owner verifies them against order history — items bought, dates, the + shipping address on file. A stranger has none of that. +3. The owner changes the address on the account. + +**Step 3 does not exist yet.** The admin customer screen can disable, enable and +release reservations, but it cannot change an email address. Until it does, the +answer to a locked-out customer is a database edit by hand. That gap is worth +its own issue rather than being smuggled into the passkeys project, because it +is an admin capability with its own audit and notification questions — the +customer whose address is being replaced has to be told, exactly as the +self-service change already tells them. diff --git a/frontend/src/customer/ResetPassword.tsx b/frontend/src/customer/ResetPassword.tsx index 9257e65..ec2899b 100644 --- a/frontend/src/customer/ResetPassword.tsx +++ b/frontend/src/customer/ResetPassword.tsx @@ -22,6 +22,11 @@ export default function ResetPassword({ onClose, onRequestNewLink, onBackToSignI const token = searchParams.get('token'); const [error, setError] = useState(null); const [loading, setLoading] = useState(false); + // Set only when the reset actually removed passkeys (#42). A toast would be + // the wrong shape for this: it dismisses itself, and a customer who is told + // that credentials they do not remember registering have just been deleted + // needs to still be looking at that when they decide what to do about it. + const [passkeysRemoved, setPasskeysRemoved] = useState(0); const { refresh } = useCustomerAuth(); // A link without a token can't do anything, so say so rather than showing a @@ -48,11 +53,17 @@ export default function ResetPassword({ onClose, onRequestNewLink, onBackToSignI setLoading(true); setError(null); try { - await resetPassword(token as string, values.password); + const result = await resetPassword(token as string, values.password); // The server signs the customer in as part of the reset, so pick up the // new session before closing. Closing lands on the storefront: the link // came from an email, so there is no page behind to return to. refresh(); + // Unless there is something to say. The reset succeeded either way, so + // this is a notice to acknowledge rather than a step still to complete. + if (result.passkeysRemoved > 0) { + setPasskeysRemoved(result.passkeysRemoved); + return; + } onClose(); } catch (err) { setError((err as Error).message); @@ -61,6 +72,31 @@ export default function ResetPassword({ onClose, onRequestNewLink, onBackToSignI } } + // The reset is done and the customer is signed in; this is the notice about + // what else it took with it. Shown in place of the form because there is + // nothing left to fill in, and closed by the customer rather than by a timer. + if (passkeysRemoved > 0) { + return ( + + + + + + + ); + } + return ( Signing in elsewhere will end — you'll stay signed in on this device. + {/* Said unconditionally, and it has to be: this form has no session and + is not told whether the account has passkeys, because answering that + would make the reset page an oracle for it. The wording works either + way — someone with none reads it and has nothing to lose. */} + + Any passkeys saved on this account will be removed. You can add them again afterwards. + {error && }
}).then(res => handle<{ status: string }>(res)); } -export function resetPassword(token: string, password: string): Promise { +/** + * A completed reset, and how many passkeys it removed (#42). + * + * The count is part of the answer rather than something to look up afterwards: + * the credentials are already gone by the time the form could go and ask, so + * the only moment this can be reported is this one. + */ +export interface PasswordResetResult extends Customer { + passkeysRemoved: number; +} + +export function resetPassword(token: string, password: string): Promise { return fetch('/api/customers/reset-password', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ token, password }) - }).then(res => handle(res)); + }).then(res => handle(res)); } export function updateMyName(firstName: string, lastName: string): Promise { diff --git a/frontend/tests/e2e/password-reset.spec.ts b/frontend/tests/e2e/password-reset.spec.ts index bfaf10f..58e8b90 100644 --- a/frontend/tests/e2e/password-reset.spec.ts +++ b/frontend/tests/e2e/password-reset.spec.ts @@ -1,5 +1,5 @@ import { test, expect } from './fixtures'; -import { readPasswordResetToken } from './support/db'; +import { giveCustomerAPasskey, readPasswordResetToken } from './support/db'; const NEW_PASSWORD = 'a-brand-new-password'; @@ -118,4 +118,47 @@ test.describe('Password reset', () => { await expect(page.getByText('invalid email or password')).toBeVisible(); await expect(header.myAccountButton).toHaveCount(0); }); + + // #42. A reset removes every passkey on the account, which is the one thing + // it does that a customer cannot undo and might have chosen differently + // about, so both halves of telling them are asserted here. + test('the reset form says passkeys will be removed before the customer commits', async ({ + passwordReset, + page + }) => { + await passwordReset.gotoReset('any-token-will-do'); + + // Said unconditionally, and it has to be: this form has no session and is + // never told whether the account has passkeys, because answering that would + // make the reset page an oracle for it. So the warning shows on a token + // that was never issued, exactly as it would on a real one. + await expect(page.getByText(/passkeys saved on this account will be removed/i)).toBeVisible(); + }); + + test('a reset that removed a passkey says so, and waits to be acknowledged', async ({ + page, + request, + customer, + accountModal, + header, + passwordReset + }) => { + await giveCustomerAPasskey(customer.email); + await accountModal.openAndLogOut(); + await expect(header.logInButton).toBeVisible(); + + await request.post('/api/customers/request-password-reset', { data: { email: customer.email } }); + await passwordReset.gotoReset(await readPasswordResetToken(customer.email)); + await passwordReset.setNewPassword(NEW_PASSWORD); + + // A toast would be the wrong shape: it dismisses itself, and a customer + // told that a credential they do not remember registering has just been + // deleted needs to still be looking at that when they decide what to do. + await expect(page.getByText('Your saved passkey was removed')).toBeVisible(); + + await page.getByRole('button', { name: 'Done' }).click(); + + // The reset still succeeded — this was a notice, not a step that failed. + await header.waitForSignedIn(); + }); }); diff --git a/frontend/tests/e2e/support/db.ts b/frontend/tests/e2e/support/db.ts index 7d31daf..a31e2db 100644 --- a/frontend/tests/e2e/support/db.ts +++ b/frontend/tests/e2e/support/db.ts @@ -67,3 +67,27 @@ export async function readPasswordResetToken(email: string): Promise { return rows[0].token as string; }); } + +/** + * Puts a passkey on an account, for the tests that are about what happens to it + * (#42). + * + * Registering one for real needs an authenticator, which Playwright does not + * have and which no fixture can supply. The credential is a row, and a password + * reset acts on the row rather than on the hardware behind it, so a row is + * enough to test the interaction — nothing here ever tries to sign in with it. + * + * Here rather than in a spec for the same reason as the token read above: the + * next spec that wants a shortcut should have to come and ask for one. + */ +export async function giveCustomerAPasskey(email: string, name = 'Test key'): Promise { + await withClient(async (client) => { + const { rowCount } = await client.query( + `INSERT INTO customer_credentials (customer_id, credential_id, public_key, name) + SELECT id, 'e2e-' || id || '-' || floor(random() * 1e9)::text, 'not-a-real-key', $2 + FROM customers WHERE email = $1`, + [email, name] + ); + if (rowCount === 0) throw new Error(`no customer to give a passkey to: ${email}`); + }); +}