feat: 2fa backup codes
calcom/cal.com#10600
Golden recall: 75.0%
Extra findings: 23
Golden Comments (3/4 found)
| # | Comment | Severity | Status | FriendlyReviewer Detail |
|---|---|---|---|---|
| #1 | Misleading error message: "backup code login" used in the disable endpoint, not a login operation | LOW | ✗ Missed | FriendlyReviewer did not flag this specific wording issue. |
| #2 | Inconsistent component name: exported as TwoFactor in BackupCode.tsx |
LOW | ✓ Found | FriendlyReviewer: "Component function name 'TwoFactor' does not match file name 'BackupCode.tsx', causing confusion and misleading stack traces" (BackupCode.tsx L7) |
| #3 | Backup code comparison is case-sensitive → rejects uppercase codes from handwritten notes | MEDIUM | ✓ Found | FriendlyReviewer found this and classified it as HIGH (higher than golden's MEDIUM): "Backup code comparison is case-sensitive; users typing uppercase codes will be rejected" (disable.ts L61) |
| #4 | Race condition: backup codes decrypted and mutated in memory; concurrent requests can reuse the same code | HIGH | ✓ Found | FriendlyReviewer found this with a perfect match: "Race condition: backup code verification and consumption is not atomic; concurrent requests can reuse same backup code" (next-auth-options.ts L130) |
Supplementary Findings (27 findings)
Additional issues found by FriendlyReviewer that are not part of the golden (reference) set.
| Finding | File | Severity | Legitimate? |
|---|---|---|---|
| Duplicate/empty labels for backup code input (Label + TextField label) | BackupCode.tsx:11 | HIGH | ✓ Accessibility |
| Digit inputs (TOTP) lack accessible labels | TwoFactor.tsx:41 | HIGH | ✓ Accessibility |
| Zod schema does not validate backupCode/totpCode | login.tsx:40 | HIGH | ✓ Missing validation |
resetState() does not clear backupCodes, URLs, isSubmitting |
EnableTwoFactorModal.tsx:73 | HIGH | ✓ State management |
Memory leak: URL.createObjectURL never revoked |
EnableTwoFactorModal.tsx:89 | HIGH | ✓ Memory leak |
Double-fire race condition: button has type=submit + onClick=handleSetup |
EnableTwoFactorModal.tsx:235 | HIGH | ✓ Race condition |
twoFactorLostAccess not reset on cancel |
DisableTwoFactorModal.tsx:118 | HIGH | ✓ State leak |
Missing try-catch around JSON.parse(symmetricDecrypt(...)) |
disable.ts:58 | HIGH | ✓ Unhandled error |
No type guard on req.body.backupCode before .replaceAll() |
disable.ts:53 | HIGH | ✓ Runtime crash |
Missing error handling: symmetricDecrypt/JSON.parse can throw |
next-auth-options.ts:137 | HIGH | ✓ Unhandled error |
Missing import for prisma |
login.2fa.e2e.ts:1 | HIGH | ✓ Broken tests |
Broken assertion: isChecked() Promise always truthy |
login.2fa.e2e.ts:53 | HIGH | ✓ Broken tests |
tabIndex={-1} on password toggle — WCAG 2.1 Keyboard violation |
Input.tsx:48 | HIGH | ✓ Accessibility |
Instructions paragraph not associated with input via aria-describedby |
BackupCode.tsx:15 | MEDIUM | ✓ Accessibility |
| Form schema defined inside component | login.tsx:64 | MEDIUM | ✓ Performance |
Inconsistent error handling: backup code errors vs errorMessages object |
login.tsx:87 | MEDIUM | ✓ Inconsistency |
AuthContainer heading does not change in backup code mode |
login.tsx:175 | MEDIUM | ✓ UX regression |
| Focus not managed when switching TOTP ↔ backup code mode | login.tsx:224 | MEDIUM | ✓ Accessibility |
useState([]) infers never[] |
EnableTwoFactorModal.tsx:31 | MEDIUM | ✓ Type safety |
resetForm does not reset twoFactorLostAccess |
DisableTwoFactorModal.tsx:43 | MEDIUM | ✓ State leak |
Inconsistent import: IdentityProvider from wrong module |
disable.ts:7 | MEDIUM | ✓ Import error |
Inconsistent error handling: throw new Error() instead of JSON response |
disable.ts:55 | MEDIUM | ✓ Inconsistency |
| Edge case: all codes exhausted — misleading error message | next-auth-options.ts:146 | MEDIUM | ✓ UX bug |
| Missing test coverage for backup code login flows | login.2fa.e2e.ts:12 | MEDIUM | ✓ Test coverage |
Missing autoComplete='off' on backup code input |
BackupCode.tsx:18 | LOW | ✓ Minor |
Digit inputs use inputMode='decimal' instead of 'numeric' |
TwoFactor.tsx:40 | LOW | ✓ Minor |
| Deeply nested ternary for TwoFactor/BackupCode | login.tsx:224 | LOW | ✓ Code quality |
Honest Analysis
FriendlyReviewer's full review
As posted on the PR — the complete output from FriendlyReviewer.
Hi there 👋
| 🌥️ Tech |
|---|
| 15 high, 11 medium, 5 low |
This merge request adds backup code support for two-factor authentication, introducing new UI components, API endpoints, and authentication logic. While the feature aligns with security best practices, the implementation introduces several critical issues that must be addressed before merging. The most significant problems include: (1) race conditions in backup code verification and consumption where concurrent requests can reuse the same code, (2) missing input validation in Zod schemas allowing unvalidated backup code and TOTP input, (3) unhandled exceptions from JSON.parse(symmetricDecrypt(...)) in both the disable endpoint and authentication options, (4) a memory leak from unreleased URL.createObjectURL references, (5) broken E2E tests from missing imports and always-truthy Promise assertions, and (6) multiple accessibility violations including duplicate labels, missing aria-describedby associations, and keyboard traps. The review recommends blocking the merge until these issues are resolved.
These might need a close look
- 🔴 packages/features/ee/two-factor/BackupCode.tsx (L11)
CRITICAL: Duplicate or empty labels for the backup code input — both a<Label>component and aTextFieldlabel are rendered, creating an ambiguous accessibility tree. - 🔴 packages/features/ee/two-factor/TwoFactor.tsx (L41)
CRITICAL: TOTP digit inputs lack accessible labels. Screen readers cannot identify these inputs. - 🔴 packages/features/auth/login.tsx (L40)
BUG: Zod schema does not validatebackupCodeortotpCodefields, allowing arbitrary input to reach the handler. - 🔴 packages/features/ee/two-factor/EnableTwoFactorModal.tsx (L73)
BUG:resetState()does not clearbackupCodes, URLs, orisSubmittingstate when called. - 🔴 packages/features/ee/two-factor/EnableTwoFactorModal.tsx (L89)
BUG:URL.createObjectURLis never revoked, causing a memory leak that grows with each QR code generation. - 🔴 packages/features/ee/two-factor/EnableTwoFactorModal.tsx (L235)
BUG: Button has bothtype=submitandonClick={handleSetup}, creating a double-fire race condition on form submission. - 🔴 packages/features/ee/two-factor/DisableTwoFactorModal.tsx (L118)
BUG:twoFactorLostAccessis not reset when the modal cancels, causing stale state on subsequent opens. - 🔴 packages/features/ee/two-factor/disable.ts (L58)
BUG: Missing try-catch aroundJSON.parse(symmetricDecrypt(...))— if decryption fails, an unhandled exception propagates to the caller. - 🔴 packages/features/ee/two-factor/disable.ts (L53)
BUG: No type guard onreq.body.backupCodebefore calling.replaceAll()— a non-string value causes a runtime crash. - 🔴 packages/features/auth/next-auth-options.ts (L137)
BUG: Missing error handling aroundsymmetricDecrypt/JSON.parse— both calls can throw for malformed or tampered data. - 🔴 packages/features/ee/two-factor/login.2fa.e2e.ts (L1)
CRITICAL: Missing import forprisma— the test file references Prisma without importing it, causing test failures. - 🔴 packages/features/ee/two-factor/login.2fa.e2e.ts (L53)
BUG: Assertion onisChecked()always passes because a Promise is truthy — theawaitis missing, making the test useless. - 🔴 packages/ui/components/form/Input.tsx (L48)
CRITICAL:tabIndex={-1}on the password toggle renders it inaccessible via keyboard navigation, violating WCAG 2.1 Keyboard guidelines. - 🔴 packages/features/ee/two-factor/disable.ts (L61)
BUG: Backup code comparison is case-sensitive; users typing uppercase codes from handwritten notes will be rejected. - 🔴 packages/features/auth/next-auth-options.ts (L130)
BUG: Race condition — backup code verification and consumption is not atomic; concurrent requests can reuse the same backup code.
Worth checking
- 🟡 packages/features/ee/two-factor/BackupCode.tsx (L15)
Instructions paragraph is not associated with the input viaaria-describedby, reducing screen reader context. - 🟡 packages/features/auth/login.tsx (L64)
Form schema is defined inside the component function, re-creating it on every render. Extract to module scope for consistency and micro-performance. - 🟡 packages/features/auth/login.tsx (L87)
Inconsistent error handling pattern: backup code errors are set directly on state while TOTP errors use theerrorMessagesobject. - 🟡 packages/features/auth/login.tsx (L175)
AuthContainerheading remains "Two-factor authentication" when in backup code mode, creating UX confusion. - 🟡 packages/features/auth/login.tsx (L224)
Focus is not managed when switching between TOTP and backup code input modes, causing keyboard focus to remain on hidden elements. - 🟡 packages/features/ee/two-factor/EnableTwoFactorModal.tsx (L31)
useState([])infers typenever[], which can cause type errors when pushing items. Add explicit type annotation. - 🟡 packages/features/ee/two-factor/DisableTwoFactorModal.tsx (L43)
resetFormdoes not resettwoFactorLostAccess, potentially leaving the checkbox in an incorrect state. - 🟡 packages/features/ee/two-factor/disable.ts (L7)
Inconsistent import:IdentityProvideris imported from the wrong module path relative to other imports in the file. - 🟡 packages/features/ee/two-factor/disable.ts (L55)
Usingthrow new Error()instead of returning a JSON error response produces an unhandled rejection in the API route. - 🟡 packages/features/auth/next-auth-options.ts (L146)
When all backup codes are exhausted, the error message is misleading and does not tell the user what happened or what to do next. - 🟡 packages/features/ee/two-factor/login.2fa.e2e.ts (L12)
No test coverage for backup code login flows — only TOTP paths are tested.
Small things (take or leave)
- 🔵 packages/features/ee/two-factor/BackupCode.tsx (L7)
Component function name 'TwoFactor' does not match file name 'BackupCode.tsx', causing confusion and misleading stack traces. - 🔵 packages/features/ee/two-factor/BackupCode.tsx (L18)
MissingautoComplete='off'on the backup code input to prevent browser autofill of sensitive data. - 🔵 packages/features/ee/two-factor/TwoFactor.tsx (L40)
Digit inputs useinputMode='decimal'which shows a decimal keypad;'numeric'is more appropriate for digit-only entry. - 🔵 packages/features/auth/login.tsx (L224)
Deeply nested ternary operator for switching between TwoFactor and BackupCode components reduces readability. - 🔵 packages/features/ee/two-factor/disable.ts (L55)
Error message text uses "backup code login" in a disable endpoint context, which is semantically incorrect for the operation being performed.
FriendlyReviewer found 3 of 4 golden bugs on this PR, missing Golden #1 (the misleading "backup code login" wording in the disable endpoint). The three it did find — the naming inconsistency, the case sensitivity in backup code comparison, and the race condition in concurrent consumption — were all correctly diagnosed and properly prioritized.
The supplementary findings are where FriendlyReviewer both impresses and overreaches. The 13 HIGH-severity findings include genuine security issues (missing input validation, unhandled JSON.parse exceptions) and legitimate bugs (race condition in verification, memory leak from URL.createObjectURL). But several findings strain the definition of "HIGH":
tabIndex={-1}on a password toggle, while technically a WCAG violation, is a minor accessibility regression unlikely to affect most users. The missingaria-describedbyon instructions text is similarly marginal. Classifying these as HIGH inflates the severity distribution and makes it harder to distinguish "deploy-blocking" from "nice to fix."FriendlyReviewer's breadth is its strength here: it covers backend API routes, frontend React components, E2E tests, and database interactions in a single pass. But the lack of context awareness means it flags every deviation from ideal equally, whether it matters or not. A human reviewer would triage differently.