#28: stop enrolment from silently replacing a secret the authenticator already has

Enrolling a second factor could reach a state where the authenticator showed
perfectly valid codes and every one of them was rejected, with the only
feedback being "TOTP verification failed".

The browser holds the enrolment secret in component state alone. Reloading the
settings page - or leaving and coming back - brought the "enable" button back,
and pressing it called the enrol endpoint again, which deleted the pending row
and stored a brand new secret. The entry already saved in the authenticator
then belonged to a secret the server no longer had. Nothing about that is
visible from either side: the app generates codes correctly, the server
compares them correctly, and they are for different keys.

Enrolment now resumes. The endpoint returns the pending secret if there is
one, so the QR shown after a reload is the same one already scanned and
rescanning is harmless. Starting over is still possible - ?reset=1, exposed in
the settings screen - because someone who deleted the entry from their app
needs a secret they can actually generate codes for. A pending row that can no
longer be decrypted is replaced rather than resumed, since nobody could ever
confirm it.

Also: the submitted code now has every non-digit stripped rather than just
surrounding whitespace. Authenticators display the code grouped as "123 456",
and a value typed or pasted with that space failed the six-character length
check before it was ever compared - which, to someone reading the right number
off their phone, looks like the server being wrong. The inputs sanitise as you
type for the same reason.

Ruled out first, rather than guessed at: the algorithm passes all six RFC 6238
reference vectors, base32 round-trips, and the deployment's clock is exactly in
sync with real time, so neither the implementation nor drift explains it.

Verified: the resume test fails without the change and passes with it; 47 auth
tests green against a real Postgres.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

services/auth/src/auth.test.ts

@@ -173,3 +173,42 @@ describe("challenge tokens are not sessions", () => {
     expect(isSurfaceAuthorized(parsed, "user")).toBe(false);
   });
 });
+
+describe("TOTP code parsing", () => {
+  // Authenticator apps display the code grouped as "123 456". A value typed or
+  // pasted with that space failed the length check before it was ever
+  // compared, which reads - to someone holding a phone showing the right
+  // number - as the server being wrong.
+  it("accepts a code exactly as the authenticator displays it", async () => {
+    const { generateTotpSecret, verifyTotpCode, generateHotp, base32Decode, getCurrentStep } =
+      await import("./totp.js");
+    const { base32Secret } = generateTotpSecret("user@example.test");
+    const code = generateHotp(base32Decode(base32Secret), getCurrentStep());
+
+    for (const typed of [code, `${code.slice(0, 3)} ${code.slice(3)}`, ` ${code} `, `${code}\n`]) {
+      expect(verifyTotpCode(base32Secret, typed).valid).toBe(true);
+    }
+  });
+
+  it("still refuses anything that is not six digits", async () => {
+    const { generateTotpSecret, verifyTotpCode } = await import("./totp.js");
+    const { base32Secret } = generateTotpSecret("user@example.test");
+    for (const bad of ["", "12345", "1234567", "abcdef", "12 34"]) {
+      expect(verifyTotpCode(base32Secret, bad).valid).toBe(false);
+    }
+  });
+
+  it("matches the RFC 6238 reference vectors", async () => {
+    const { generateHotp } = await import("./totp.js");
+    const secret = Buffer.from("12345678901234567890", "ascii");
+    const vectors: Array<[number, string]> = [
+      [59, "287082"],
+      [1111111109, "081804"],
+      [1234567890, "005924"],
+      [20000000000, "353130"],
+    ];
+    for (const [t, want] of vectors) {
+      expect(generateHotp(secret, Math.floor(t / 30))).toBe(want);
+    }
+  });
+});

services/auth/src/repository.ts

@@ -127,16 +127,60 @@ async function registerFailedLogin(mailboxId: string): Promise<void> {
  * accepted back from the client - a secret the client chooses is a secret the
  * client can replay for somebody else's account.
  */
-export async function startTotpEnrollment(mailboxId: string, base32Secret: string, label = "Authenticator"): Promise<void> {
+/**
+ * Starts - or resumes - a TOTP enrollment, returning the secret to display.
+ *
+ * Resuming matters. This used to delete any pending row and store a freshly
+ * generated secret on every call, and the browser only holds the secret in
+ * component state: reloading the settings page, or coming back to it, brought
+ * the "enable" button back, and pressing it silently replaced the secret the
+ * authenticator had already been given. The app then showed perfectly valid
+ * codes for a secret the server no longer had, and every confirmation failed
+ * with no indication why.
+ *
+ * Returning the pending secret instead makes enrollment idempotent: the QR
+ * shown after a reload is the same one already scanned, so an existing entry
+ * keeps working and rescanning is harmless.
+ */
+export async function startTotpEnrollment(
+  mailboxId: string,
+  base32Secret: string,
+  label = "Authenticator",
+): Promise<string> {
+  const pending = await queryOne<{ secret_enc: Buffer; secret_nonce: Buffer; key_version: number }>(
+    `SELECT secret_enc, secret_nonce, key_version
+       FROM mfa_totp WHERE mailbox_id = $1 AND status = 'PENDING'
+       ORDER BY created_at DESC LIMIT 1`,
+    [mailboxId],
+  );
+  if (pending) {
+    try {
+      return decryptSecret(pending.secret_enc, pending.secret_nonce, pending.key_version);
+    } catch {
+      // Undecryptable - the master key changed under it. Nobody can ever
+      // confirm this row, so replacing it is the only way forward.
+      await query(`DELETE FROM mfa_totp WHERE mailbox_id = $1 AND status = 'PENDING'`, [mailboxId]);
+    }
+  }
+
   const { encrypted, nonce, keyVersion } = encryptSecret(base32Secret);
-  // Any earlier unconfirmed attempt is replaced, so a user who restarts
-  // enrollment is not left with rows nobody can complete.
-  await query(`DELETE FROM mfa_totp WHERE mailbox_id = $1 AND status = 'PENDING'`, [mailboxId]);
   await query(
     `INSERT INTO mfa_totp (mailbox_id, label, secret_enc, secret_nonce, key_version, status)
      VALUES ($1, $2, $3, $4, $5, 'PENDING')`,
     [mailboxId, label, encrypted, nonce, keyVersion],
   );
+  return base32Secret;
+}
+
+/**
+ * Abandons a pending enrollment, so the next one starts from a new secret.
+ *
+ * Needed because startTotpEnrollment now resumes: someone who deleted the
+ * entry from their authenticator would otherwise be handed the same secret
+ * they can no longer generate codes for.
+ */
+export async function resetTotpEnrollment(mailboxId: string): Promise<void> {
+  await query(`DELETE FROM mfa_totp WHERE mailbox_id = $1 AND status = 'PENDING'`, [mailboxId]);
 }
 
 interface TotpRow {

services/auth/src/router.test.ts

@@ -193,6 +193,41 @@ describeDb("auth API against a real database", () => {
 
   // PLAN.md F4 acceptance criterion: a /user token must be refused by
   // /api/v1/admin/*.
+  // The browser holds the enrolment secret only in component state, so a
+  // reload brings the "enable" button back. Enrolling used to replace the
+  // stored secret on every call, which silently orphaned the entry the
+  // authenticator had already saved: the app kept showing valid codes for a
+  // secret the server no longer had, and confirmation failed forever with no
+  // way to tell why.
+  it("resumes a pending enrolment instead of replacing the secret", { timeout: 60000 }, async () => {
+    await query(`DELETE FROM mfa_totp WHERE mailbox_id = $1`, [USER_ID]);
+    const login = await post("/api/v1/auth/user/login", { email: userEmail(), password: PASSWORD });
+    const token = login.body.token as unknown as string;
+
+    const first = await post("/api/v1/user/mfa/totp/enroll", {}, token);
+    const second = await post("/api/v1/user/mfa/totp/enroll", {}, token);
+    expect(second.body.secret).toBe(first.body.secret);
+
+    // The code from the secret shown first must still confirm after the
+    // second call - that is the reload case.
+    const code = generateHotp(base32Decode(first.body.secret as unknown as string), getCurrentStep());
+    const confirm = await post("/api/v1/user/mfa/totp/confirm", { code }, token);
+    expect(confirm.status).toBe(200);
+  });
+
+  it("issues a different secret when enrolment is explicitly reset", { timeout: 60000 }, async () => {
+    await query(`DELETE FROM mfa_totp WHERE mailbox_id = $1`, [USER_ID]);
+    const login = await post("/api/v1/auth/user/login", { email: userEmail(), password: PASSWORD });
+    const token = login.body.token as unknown as string;
+
+    const first = await post("/api/v1/user/mfa/totp/enroll", {}, token);
+    const reset = await post("/api/v1/user/mfa/totp/enroll?reset=1", {}, token);
+    expect(reset.body.secret).not.toBe(first.body.secret);
+    // The URI has to carry the secret actually in force, or the QR would
+    // enrol something the server will not accept.
+    expect(reset.body.uri).toContain(reset.body.secret);
+  });
+
   it("rejects a user token on an admin endpoint", async () => {
     const login = await post("/api/v1/auth/user/login", { email: userEmail(), password: PASSWORD });
     const token = (login.body.token ?? login.body.challenge_token) as unknown as string;

services/auth/src/router.ts

@@ -1,6 +1,6 @@
 import { IncomingMessage, ServerResponse } from "node:http";
 import crypto from "node:crypto";
-import { generateTotpSecret } from "./totp.js";
+import { generateTotpSecret, generateTotpUri } from "./totp.js";
 import { generateRecoveryCodes, generateAppPassword } from "./mfaHelpers.js";
 import { createJwt, verifyJwt, isSurfaceAuthorized, type SessionTokenPayload } from "./session.js";
 import * as repo from "./repository.js";
@@ -134,7 +134,7 @@ async function route(req: IncomingMessage, res: ServerResponse, url: string): Pr
     if (url === "/api/v1/user/sieve" && method === "GET") return userGetSieve(res, session!);
     if (url === "/api/v1/user/sieve" && method === "POST") return userSaveSieve(req, res, session!);
     if (url === "/api/v1/user/vacation" && method === "POST") return userSaveVacation(req, res, session!);
-    if (url === "/api/v1/user/mfa/totp/enroll" && method === "POST") return totpEnroll(res, session!);
+    if (url === "/api/v1/user/mfa/totp/enroll" && method === "POST") return totpEnroll(req, res, session!);
     if (url === "/api/v1/user/mfa/totp/confirm" && method === "POST") return totpConfirm(req, res, session!);
     if (url === "/api/v1/user/app-passwords" && method === "GET") return appPasswordsList(res, session!);
     if (url === "/api/v1/user/app-passwords" && method === "POST") return appPasswordCreate(req, res, session!);
@@ -395,12 +395,28 @@ async function updateLocale(req: IncomingMessage, res: ServerResponse, session:
   sendJson(res, 200, { locale });
 }
 
-async function totpEnroll(res: ServerResponse, session: SessionTokenPayload): Promise<void> {
-  const secret = generateTotpSecret(session.email);
+async function totpEnroll(
+  req: IncomingMessage,
+  res: ServerResponse,
+  session: SessionTokenPayload,
+): Promise<void> {
+  // ?reset=1 abandons a pending enrollment. Without it this endpoint resumes,
+  // which is what stops a page reload from invalidating a secret the
+  // authenticator has already stored.
+  if (new URL(req.url ?? "", "http://internal").searchParams.get("reset") === "1") {
+    await repo.resetTotpEnrollment(session.sub);
+  }
+
+  const generated = generateTotpSecret(session.email);
   // Stored server-side; the confirm step reads it from the database rather
-  // than trusting the client to send it back.
-  await repo.startTotpEnrollment(session.sub, secret.base32Secret);
-  sendJson(res, 200, { secret: secret.base32Secret, uri: secret.uri });
+  // than trusting the client to send it back. The value returned is whichever
+  // secret is now pending, which may be one from an earlier attempt.
+  const secret = await repo.startTotpEnrollment(session.sub, generated.base32Secret);
+  const uri =
+    secret === generated.base32Secret
+      ? generated.uri
+      : generateTotpUri(session.email, secret);
+  sendJson(res, 200, { secret, uri });
 }
 
 async function totpConfirm(req: IncomingMessage, res: ServerResponse, session: SessionTokenPayload): Promise<void> {

services/auth/src/totp.ts

@@ -62,13 +62,17 @@ export function generateHotp(secretBuffer: Buffer, counter: number): string {
   return code;
 }
 
+/** Renders the otpauth:// URI an authenticator app scans. */
+export function generateTotpUri(label: string, base32Secret: string, issuer = "LambdaMail"): string {
+  const encodedLabel = encodeURIComponent(label);
+  const encodedIssuer = encodeURIComponent(issuer);
+  return `otpauth://totp/${encodedIssuer}:${encodedLabel}?secret=${base32Secret}&issuer=${encodedIssuer}&algorithm=SHA1&digits=6&period=30`;
+}
+
 export function generateTotpSecret(label: string, issuer = "LambdaMail"): { base32Secret: string; uri: string } {
   const secretBytes = crypto.randomBytes(20);
   const base32Secret = base32Encode(secretBytes);
-  const encodedLabel = encodeURIComponent(label);
-  const encodedIssuer = encodeURIComponent(issuer);
-  const uri = `otpauth://totp/${encodedIssuer}:${encodedLabel}?secret=${base32Secret}&issuer=${encodedIssuer}&algorithm=SHA1&digits=6&period=30`;
-  return { base32Secret, uri };
+  return { base32Secret, uri: generateTotpUri(label, base32Secret, issuer) };
 }
 
 export function verifyTotpCode(
@@ -77,8 +81,13 @@ export function verifyTotpCode(
   lastUsedStep: number | null = null,
   timeMs: number = Date.now(),
 ): { valid: boolean; step: number } {
-  const cleanCode = code.trim();
-  if (cleanCode.length !== 6 || !/^\d{6}$/.test(cleanCode)) {
+  // Every non-digit is stripped, not just surrounding whitespace.
+  // Authenticator apps display the code grouped as "123 456", and a value
+  // pasted or typed with that space was rejected as malformed before it was
+  // ever compared - which reads, to the person holding a phone showing the
+  // right number, as the server being wrong.
+  const cleanCode = code.replace(/\D/g, "");
+  if (cleanCode.length !== 6) {
     return { valid: false, step: 0 };
   }