diff --git a/services/auth/src/auth.test.ts b/services/auth/src/auth.test.ts index ee22530..ac8d734 100644 --- a/services/auth/src/auth.test.ts +++ b/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); + } + }); +}); diff --git a/services/auth/src/repository.ts b/services/auth/src/repository.ts index 41cdc6a..6fa29cf 100644 --- a/services/auth/src/repository.ts +++ b/services/auth/src/repository.ts @@ -127,16 +127,60 @@ async function registerFailedLogin(mailboxId: string): Promise { * 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 { +/** + * 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 { + 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 { + await query(`DELETE FROM mfa_totp WHERE mailbox_id = $1 AND status = 'PENDING'`, [mailboxId]); } interface TotpRow { diff --git a/services/auth/src/router.test.ts b/services/auth/src/router.test.ts index 2558a5a..4008b83 100644 --- a/services/auth/src/router.test.ts +++ b/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; diff --git a/services/auth/src/router.ts b/services/auth/src/router.ts index 5149e86..7ef9e91 100644 --- a/services/auth/src/router.ts +++ b/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 { - const secret = generateTotpSecret(session.email); +async function totpEnroll( + req: IncomingMessage, + res: ServerResponse, + session: SessionTokenPayload, +): Promise { + // ?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 { diff --git a/services/auth/src/totp.ts b/services/auth/src/totp.ts index b51c602..c477026 100644 --- a/services/auth/src/totp.ts +++ b/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 }; } diff --git a/services/webmail/messages/en.json b/services/webmail/messages/en.json index ffa0fb7..69109ea 100644 --- a/services/webmail/messages/en.json +++ b/services/webmail/messages/en.json @@ -205,6 +205,7 @@ "signatureSaved": "Signature saved.", "signatureTitle": "Email signature", "sixDigitCode": "Six-digit code", + "startOverMfa": "Lost the entry? Start over with a new key", "subtitle": "Two-factor authentication, filters, auto-reply and your signature.", "title": "Settings", "twoFactorIntro": "Protect your account with Google Authenticator, Aegis, Authy or 1Password.", diff --git a/services/webmail/messages/es.json b/services/webmail/messages/es.json index 2e0b861..fc039bc 100644 --- a/services/webmail/messages/es.json +++ b/services/webmail/messages/es.json @@ -205,6 +205,7 @@ "signatureSaved": "Firma guardada.", "signatureTitle": "Firma de correo", "sixDigitCode": "Código de seis dígitos", + "startOverMfa": "¿Perdió el registro? Empiece de nuevo con una clave nueva", "subtitle": "Verificación en dos pasos, filtros, respuesta automática y su firma.", "title": "Configuración", "twoFactorIntro": "Proteja su cuenta con Google Authenticator, Aegis, Authy o 1Password.", diff --git a/services/webmail/messages/pt-BR.json b/services/webmail/messages/pt-BR.json index 6a96c4c..1ae1eab 100644 --- a/services/webmail/messages/pt-BR.json +++ b/services/webmail/messages/pt-BR.json @@ -205,6 +205,7 @@ "signatureSaved": "Assinatura salva.", "signatureTitle": "Assinatura de e-mail", "sixDigitCode": "Código de seis dígitos", + "startOverMfa": "Perdeu o registro? Recomece com uma nova chave", "subtitle": "Verificação em duas etapas, filtros, resposta automática e sua assinatura.", "title": "Configurações", "twoFactorIntro": "Proteja sua conta com o Google Authenticator, Aegis, Authy ou 1Password.", diff --git a/services/webmail/src/app/(auth)/admin/login/page.tsx b/services/webmail/src/app/(auth)/admin/login/page.tsx index 0200cda..d710bd3 100644 --- a/services/webmail/src/app/(auth)/admin/login/page.tsx +++ b/services/webmail/src/app/(auth)/admin/login/page.tsx @@ -138,8 +138,8 @@ export default function AdminLoginPage() { inputMode="numeric" autoComplete="one-time-code" value={mfaCode} - onChange={(e) => setMfaCode(e.target.value)} - maxLength={6} + onChange={(e) => setMfaCode(e.target.value.replace(/\D/g, "").slice(0, 6))} + maxLength={7} required className="w-full rounded-lg border border-slate-800 bg-slate-900 px-4 py-2.5 text-center font-mono text-lg tracking-widest text-white focus:border-emerald-500 focus:outline-none" /> @@ -180,9 +180,9 @@ export default function AdminLoginPage() { setMfaCode(e.target.value)} + onChange={(e) => setMfaCode(e.target.value.replace(/\D/g, "").slice(0, 6))} placeholder="123456" - maxLength={6} + maxLength={7} required className="w-full px-4 py-2.5 rounded-lg bg-slate-900 border border-slate-800 text-white text-center tracking-widest text-lg font-mono focus:outline-none focus:border-emerald-500 transition-colors" /> diff --git a/services/webmail/src/app/(auth)/user/login/page.tsx b/services/webmail/src/app/(auth)/user/login/page.tsx index 339efd1..5ecc471 100644 --- a/services/webmail/src/app/(auth)/user/login/page.tsx +++ b/services/webmail/src/app/(auth)/user/login/page.tsx @@ -131,9 +131,9 @@ export default function UserLoginPage() { setMfaCode(e.target.value)} + onChange={(e) => setMfaCode(e.target.value.replace(/\D/g, "").slice(0, 6))} placeholder="123456" - maxLength={6} + maxLength={7} required className="w-full pl-10 pr-4 py-3 rounded-xl bg-slate-900/90 border border-slate-800 text-white text-center tracking-widest text-lg font-mono focus:outline-none focus:border-indigo-500 transition-colors" /> diff --git a/services/webmail/src/app/(user)/user/settings/page.tsx b/services/webmail/src/app/(user)/user/settings/page.tsx index e974ed4..6199d65 100644 --- a/services/webmail/src/app/(user)/user/settings/page.tsx +++ b/services/webmail/src/app/(user)/user/settings/page.tsx @@ -291,6 +291,24 @@ export default function UserSettingsPage() { {qrSecret && (
+ {/* Enrolment resumes rather than restarting, so this code is the + same one already scanned. Anyone who removed the entry from + their app needs a way to ask for a different secret. */} +
{ e.preventDefault(); @@ -316,9 +334,9 @@ export default function UserSettingsPage() { inputMode="numeric" autoComplete="one-time-code" value={totpCode} - onChange={(e) => setTotpCode(e.target.value)} + onChange={(e) => setTotpCode(e.target.value.replace(/\D/g, "").slice(0, 6))} placeholder={t("settings.sixDigitCode")} - maxLength={6} + maxLength={7} required className="px-4 py-2 rounded-xl bg-slate-950 border border-slate-800 text-white text-xs font-mono text-center focus:outline-none focus:border-indigo-500" />