feat: 2fa backup codes

Hi there 👋

🌥️ Tech	🌤️ Feat
1 high, 6 medium, 11 low	1 ok, 2 mitigated

Review of the 2FA backup-codes MR (feat: 2fa backup codes).

The feature is broadly functional end-to-end: backup codes are generated (10 lowercase-hex codes, display formatted as XXXXX-XXXXX), stored encrypted, usable at login through the "Lost access" toggle, and cleared when 2FA is disabled. The values contract (field name backupCode, code format, error codes, DB column, i18n keys, migration) is internally consistent and was verified across setup/display/login/disable, and the new migration and localization keys are correct.

Several important issues should be addressed before merge, the most significant being:

• A TOCTOU race in next-auth-options.ts makes backup-code single-use consumption non-atomic, so the same code can authenticate twice under concurrent requests, violating the documented "each code usable exactly once" guarantee.
• The 2FA setup wizard can be dismissed at the backup-code step via X/ESC/overlay without invalidating the parent UI or resetting state, so a user can leave with 2FA actually enabled while the UI still claims it is off, and reopening shows stale step/codes.
• The new E2E tests cover only code generation (download/copy/close), not the backup-code login path or the new error states — which are the headline of this PR. Several test assertions (clipboard, download content, switch state) are also flaky or under-asserted.
• A global accessibility regression: tabIndex={-1} on the shared PasswordField removes keyboard access to the show/hide toggle across ~440 dependent files.
• Backup codes are committed at setup time (before 2FA is actually enabled) with only 40 bits of entropy per code, clipboard copy reports success unconditionally, and resetState() leaks a blob URL and stale state.

Non-blocking robustness/consistency notes exist on the disable route (unvalidated body, unconsumed code, thrown-vs-JSON 500) and on login UX/normalization. All sub-reviews completed successfully (no errors/partials); the final status is REQUEST_CHANGES given the single-use race and the missing login-path test coverage.

---

These might need a close look

• 🔴 packages/features/auth/lib/next-auth-options.ts (L148)
  Non-atomic single-use consumption of backup codes (TOCTOU race). The code is verified and consumed via read (indexOf) → write (backupCodes[index] = null; prisma.update) without any lock/transaction. Two concurrent login requests submitting the same valid code can both pass the check, authenticate, and write the consumed array afterward, violating the documented "each backup code can be used exactly once" guarantee. Make consumption atomic (serialize per-user, transaction/row lock, or conditional updateMany).

Worth checking

• 🟡 apps/web/components/settings/EnableTwoFactorModal.tsx (L166)
  Closing the dialog via X/ESC/overlay at the DisplayBackupCodes step only fires onOpenChange, which never calls onEnable() (so viewer.me.invalidate() is skipped and the 2FA switch stays visually off even though 2FA is already enabled server-side) and never calls resetState(), leaving step and backupCodes/backupCodesUrl stale so reopening shows the backup step again. Only the explicit "close" button handles this correctly; route outside/ESC/X dismissals through the same close path.
• 🟡 apps/web/pages/api/auth/two-factor/totp/setup.ts (L60)
  Two concerns: (1) backupCodes/twoFactorSecret are persisted at the password/setup step, before 2FA is actually enabled via /totp/enable; if the user abandons the wizard, recovery codes remain stored (encrypted) on an account with twoFactorEnabled=false. Consider generating/committing at enable time or clearing on abandonment. (2) Each code uses crypto.randomBytes(5).toString('hex') = 40 bits of entropy with a small hex alphabet and a plaintext indexOf comparison; for a recovery credential that bypasses 2FA, consider longer codes or a larger alphabet to raise per-code entropy.
• 🟡 apps/web/components/auth/BackupCode.tsx (L9)
  The component is exported as export default function TwoFactor(...) inside BackupCode.tsx (copy-paste leftover) — rename to BackupCode. Also, generated codes are lowercase hex but the input only constrains length (no pattern/case normalization), and the server verifies with a case-sensitive exact indexOf; uppercase hex (A–F) or the dash-space variant typed/pasted by the user would be rejected even though valid. Normalize (trim + lowercase + strip dashes) on input or server, or add a pattern/helper text.
• 🟡 packages/features/auth/lib/next-auth-options.ts (L144)
  Backup-code matching normalizes only with .replaceAll('-', ''); codes pasted with leading/trailing whitespace (common from the downloaded .txt) or typed in uppercase fail to match and are reported as IncorrectBackupCode. Normalize with .trim().toLowerCase().replaceAll('-', '') on the server (and in BackupCode.tsx), and add a server-side length/format sanity check (e.g. exactly 10 hex chars) since the HTML minLength/maxLength are not enforced (noValidate).
• 🟡 apps/web/playwright/login.2fa.e2e.ts (L12)
  The new E2E additions only exercise the backup-code generation screen (download/copy/close); they do not log out, navigate to login, click "Lost access", enter a backup code, or assert IncorrectBackupCode/MissingBackupCodes. The top-of-file // TODO: add more backup code tests, e.g. login + disabling 2fa with backup confirms the core feature path of this MR is untested. Add a login-via-backup-code assertion (success) and assertions for invalid/missing code errors; at minimum remove the TODO if deferring.
• 🟡 packages/ui/components/form/inputs/Input.tsx (L49)
  tabIndex={-1} on the password-visibility toggle is a global change to the shared PasswordField component used across ~440 files, removing keyboard (Tab) access to the show/hide toggle (and its sr-only label) on every password input in the app — an accessibility regression (WCAG 2.1.1) and unrelated to the intended backup-code variant. Prefer removing it, gating via an opt-in prop, or fixing the modal's tab order locally; at minimum document why it was added.

Small things (take or leave)

• 🔵 apps/web/components/settings/EnableTwoFactorModal.tsx (L71)
  resetState() only clears password, errorMessage and step. It never calls URL.revokeObjectURL(backupCodesUrl) nor clears backupCodesUrl (leaking the blob URL created in handleSetup on every cancel/close/re-run), and it leaves backupCodes/dataUri/secret set until the next successful setup. Clear these fields and revoke the URL.
• 🔵 apps/web/components/settings/EnableTwoFactorModal.tsx (L278)
  navigator.clipboard.writeText(...) has no .catch() and showToast(backup_codes_copied, "success") fires unconditionally, so users are told codes were copied even when the operation failed (non-secure context, denied permission, unfocused document). Await/then the promise and only toast success on fulfillment, with a fallback (e.g. execCommand('copy')) or manual-copy prompt.
• 🔵 packages/features/auth/lib/next-auth-options.ts (L139)
  JSON.parse(symmetricDecrypt(user.backupCodes, ...)) is unguarded; a corrupt/legacy/malformed stored backupCodes value or unexpected decrypt result throws out of authorize as a generic 500. Wrap in try/catch and fall back to a controlled error, consistent with the defensive secret.length !== 32 check in the TOTP branch.
• 🔵 apps/web/pages/auth/login.tsx (L198)
  In "lost access" (backup-code) mode the page still renders the "2FA Code" heading and "Submit" button label rather than a backup-code-context label. Minor UX inconsistency; consider switching the heading/label when twoFactorLostAccess is true. (The mode-toggle state handling that clears totpCode/backupCode on switch was verified correct.)
• 🔵 apps/web/pages/api/auth/two-factor/totp/disable.ts (L61)
  Two issues in the backup-code branch: (1) the branch is gated only on truthiness of req.body.backupCode, so a non-string truthy value (e.g. array/object) makes .replaceAll(...) throw a TypeError → unhandled 500; validate typeof req.body.backupCode === 'string' and normalize input. (2) The verified code is never consumed/nullified before the bulk delete (single-use semantics diverge from the login flow and the backup_code_instructions copy); if the final update fails, the same code remains usable for a retry or later login.
• 🔵 apps/web/pages/api/auth/two-factor/totp/disable.ts (L51)
  throw new Error(ErrorCode.InternalServerError) (on missing CALENDSO_ENCRYPTION_KEY, and analogous TOTP throws) yields an HTML 500 page instead of a JSON { error } body; response.json() then throws in the modal and is swallowed into something_went_wrong, so the modal's InternalServerError handling never runs. Normalize to res.status(500).json(...) for the JSON contract the modal expects.
• 🔵 apps/web/playwright/login.2fa.e2e.ts (L120)
  The clipboard-copy test depends on the Clipboard API writeText succeeding (requires clipboard-write permission and focused document); in headless CI the promise may reject, the toast never appears, and the toast-success waitFor times out — a flakiness source. Grant clipboard permissions on the context and/or assert the copied text against the known codes rather than relying solely on the toast.
• 🔵 apps/web/playwright/login.2fa.e2e.ts (L113)
  The download test asserts only the suggestedFilename() (cal-backup-codes.txt); the // TODO: check file content means it never verifies the file contains the generated codes or the XXXXX-XXXXX formatting, nor cross-checks against the persisted backupCodes column. Read download.createReadStream()/download.path() and assert content.
• 🔵 apps/web/playwright/login.2fa.e2e.ts (L52)
  The first test still uses isChecked() on the two-factor switch despite the file's own FIXME noting it "passes even when switch is not checked", while the second test uses data-state="checked" — inconsistent assertions across the suite. Migrate the first to [data-testid=two-factor-switch][data-state="checked"] (or aria-checked).
• 🔵 apps/web/public/static/locales/en/common.json (L2013)
  The 7 new localization keys were added only to the English file; other locales render English fallback (fallbackLng = en) until translated. This matches project convention and needs no change, but was verified. Non-blocking copy note: missing_backup_codes text says to generate codes "in your settings," which reads oddly when surfaced in the disable-2FA modal.
• 🔵 packages/prisma/migrations/20230804153419_add_backup_codes/migration.sql (L2)
  The migration is forward-only (ADD COLUMN backupCodes TEXT, nullable, no default) with no DROP COLUMN rollback. This matches the Prisma schema (backupCodes String?) and the standard forward-only Prisma convention, so it is acceptable; flagged only because rollback was explicitly checked.

---

✅ Feature-level checklist

Looks good

• ✅ Fix the 2FA setup test to include the new backup code screen.
  The new E2E additions cover the backup-code generation screen (download/copy/close). The broader backup-code login path and error states are not yet covered (acknowledged by a TODO), which affects the login feature's test coverage rather than this specific setup-screen requirement.

Partially covered

• ⚠️ Generate and display backup codes during 2FA setup.
  Generation and display work and the values contract (format XXXXX-XXXXX, 10 codes, encrypted storage, client parsing) is consistent. However, backup codes (and the secret) are committed server-side at the password/setup step before 2FA is actually enabled (orphaned if the wizard is abandoned), per-code entropy is only 40 bits, and the wizard's X/ESC/overlay close path leaves stale state and never invalidates the parent UI, so the UI can claim 2FA is off while it is enabled.
• ⚠️ Enable use of backup codes during the 2FA login flow.
  Login via backup codes works, is rate-limited, and error codes map correctly to i18n keys. But the single-use consumption is non-atomic (TOCTOU), so the same code can authenticate twice under concurrent requests, violating the single-use guarantee; input matching is also not robust to whitespace/uppercase, and unguarded decrypt of stored codes can yield a 500.