feat: 2fa backup codes#9
Conversation
Co-authored-by: Peer Richelsen <peeroke@gmail.com>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request implements single-use backup codes as an alternative authentication mechanism for two-factor authentication. The feature includes UI components for backup code entry, API endpoints for generation and validation, database schema updates, and integrated error handling across login and settings flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginForm
participant API
participant DB
User->>LoginForm: Enter credentials + 2FA required
LoginForm->>API: POST sign-in
API-->>LoginForm: SecondFactorRequired
Note over User: User has two paths:
rect rgb(200, 220, 255)
Note over LoginForm,DB: Path 1: TOTP
User->>LoginForm: Enter authenticator code
LoginForm->>API: POST with TOTP code
API->>DB: Verify TOTP
API-->>LoginForm: Success
end
rect rgb(220, 255, 220)
Note over LoginForm,DB: Path 2: Backup Code (Lost Access)
User->>LoginForm: Click "Lost access"
LoginForm->>User: Show backup code input
User->>LoginForm: Enter backup code
LoginForm->>API: POST with backup code
API->>DB: Decrypt codes, verify match
API->>DB: Nullify used code, re-encrypt
API-->>LoginForm: Success
end
LoginForm-->>User: Authenticated
sequenceDiagram
participant User
participant SettingsUI
participant API
participant DB
User->>SettingsUI: Enable 2FA
SettingsUI->>API: POST setup with TOTP secret
rect rgb(200, 220, 255)
Note over API,DB: Generate & Store
API->>API: Generate 10 random backup codes
API->>DB: Encrypt & store codes
API-->>SettingsUI: Return codes + secret
end
rect rgb(255, 240, 200)
Note over SettingsUI,User: Display Codes
SettingsUI->>User: Show backup codes in UI
User->>SettingsUI: Copy codes
SettingsUI->>User: Toast confirmation
User->>SettingsUI: Download as file
SettingsUI->>User: cal-backup-codes.txt
User->>SettingsUI: Close dialog
end
SettingsUI-->>User: 2FA enabled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Key areas requiring focused attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
apps/web/pages/api/auth/two-factor/totp/setup.ts (1)
60-61: Consider increasing backup code entropy for enhanced security.Each backup code has 10 hex characters (40 bits of entropy). While acceptable for single-use codes behind authentication, consider:
- Increasing to 12-16 characters for stronger security
- Using alphanumeric (base32/base62) encoding for better entropy-per-character ratio
- Following OWASP or NIST guidelines for backup code strength
For comparison:
- GitHub uses 16-character backup codes
- Google uses 8-character alphanumeric codes
- Auth0 recommends 10-16 characters
If you decide to increase entropy, consider:
- const backupCodes = Array.from(Array(10), () => crypto.randomBytes(5).toString("hex")); + // 8 bytes = 64 bits of entropy per code + const backupCodes = Array.from(Array(10), () => crypto.randomBytes(8).toString("hex"));apps/web/playwright/login.2fa.e2e.ts (1)
110-126: Complete the TODOs for backup code file and clipboard validation.The test verifies the download and copy actions occur but doesn't validate the actual content. This leaves gaps in test coverage:
- Line 117: TODO to check file content
- Line 122: TODO to check clipboard content
Consider completing these validations to ensure:
- Downloaded file contains valid backup codes
- Clipboard contains the expected backup codes
- The codes match the format generated by the backend (10 hex characters each)
Example for file content validation:
// After download const downloadPath = await download.path(); const fileContent = await fs.readFile(downloadPath, 'utf-8'); const codes = fileContent.trim().split('\n'); expect(codes).toHaveLength(10); codes.forEach(code => { expect(code).toMatch(/^[0-9a-f]{10}$/); });apps/web/components/auth/BackupCode.tsx (1)
7-29: Rename component function to match file/usage for clarityThe default export is named
TwoFactorin a file imported elsewhere asBackupCode. This works technically, but the mismatch can be confusing in error stacks and debugging. Consider renaming the function toBackupCode(or similar) so the symbol name, file name, and import name stay aligned; you can keep the default export the same.apps/web/pages/api/auth/two-factor/totp/disable.ts (1)
46-68: Backup-code disable flow is correct; consider de-duplicating validation logicThe backup-code path here correctly:
- Requires a password check first (for CAL users).
- Validates presence of
CALENDSO_ENCRYPTION_KEYanduser.backupCodes.- Decrypts/parses backup codes and checks the normalized input (
replaceAll("-","")) against them.- Returns specific error codes for missing/incorrect codes.
- Finally clears both
backupCodesandtwoFactorSecretwhen disabling 2FA.Semantics (any valid backup code can disable 2FA once, after which all codes are cleared) look sound. Since the decryption and matching logic is nearly identical to the login flow in
next-auth-options.ts, you may want to extract a shared helper to avoid divergence if the format or matching rules change later.Also applies to: 101-110
apps/web/components/settings/DisableTwoFactorModal.tsx (1)
23-27: Disable-2FA modal wiring for backup codes looks solid; consider resetting view on closeThe modal cleanly adds backup-code support:
- Form model and
handleDisablecorrectly includebackupCodeand pass it toTwoFactorAuthAPI.disable.- Error handling now covers
IncorrectBackupCodeandMissingBackupCodeswith appropriate localized messages.- The “Lost access” toggle swaps between
<TwoFactor>and<BackupCode>while preserving the password and clearing codes/errors viaresetForm(false), which is a nice touch.One small UX refinement you might consider: reset
twoFactorLostAccess(and possibly callresetForm()) when the modal is closed viaonCancel/onOpenChange, so it always reopens in the default TOTP view instead of remembering the last mode.Also applies to: 38-49, 50-86, 93-107, 112-121
apps/web/components/settings/EnableTwoFactorModal.tsx (3)
64-65: Tighten backup code state typing and lifecycle (cleanup + privacy).The flow works, but a few small changes would make the backup-code state safer and clearer:
backupCodesis inferred asany[]andbackupCodesUrlasstring; they’re reallystring[]and a nullable URL.resetStatedoesn’t clearbackupCodes,backupCodesUrl,dataUri, orsecret, so sensitive data stays in memory for the lifetime of the component.- The Blob URL is revoked only when regenerating codes, not when the modal is closed/unmounted.
Consider something like:
- const [backupCodes, setBackupCodes] = useState([]); - const [backupCodesUrl, setBackupCodesUrl] = useState(""); + const [backupCodes, setBackupCodes] = useState<string[]>([]); + const [backupCodesUrl, setBackupCodesUrl] = useState<string | null>(null); - const resetState = () => { - setPassword(""); - setErrorMessage(null); - setStep(SetupStep.ConfirmPassword); - }; + const resetState = () => { + setPassword(""); + setErrorMessage(null); + setBackupCodes([]); + if (backupCodesUrl) URL.revokeObjectURL(backupCodesUrl); + setBackupCodesUrl(null); + setDataUri(""); + setSecret(""); + setStep(SetupStep.ConfirmPassword); + };Optionally, you could also add a small
useEffectcleanup that revokesbackupCodesUrlon unmount for completeness.Also applies to: 71-76, 92-99
31-31: Backup-codes step wiring looks sound; consider makingformatBackupCodemore defensive.The new
SetupStep.DisplayBackupCodesand its description, plus the transition inhandleEnableto this step, create a clear final stage where users must acknowledge/save their backup codes, which is good UX.
formatBackupCodecurrently assumes a fixed 10-character code:const formatBackupCode = (code: string) => `${code.slice(0, 5)}-${code.slice(5, 10)}`;If the backend ever changes the code length, this will silently produce odd formatting. A slightly more defensive version would avoid that assumption:
- const formatBackupCode = (code: string) => `${code.slice(0, 5)}-${code.slice(5, 10)}`; + const formatBackupCode = (code: string) => + code.length > 5 ? `${code.slice(0, 5)}-${code.slice(5)}` : code;Functionally everything works today; this just future‑proofs the formatter a bit.
Also applies to: 58-58, 135-135, 163-163, 200-208
8-8: Finalize‑step actions are good; handle clipboard failures and confirm onEnable timing.The close/copy/download actions in the backup-codes footer are a nice completion flow, but two small follow‑ups may help:
- Clipboard errors:
navigator.clipboard.writeText(...)returns a promise that can reject (permissions, unsupported environment). Right now that rejection would be unhandled. Wrapping it in atry/catchor.catchand showing an error toast would avoid noisy unhandled promise rejections and improve UX:- onClick={(e) => { - e.preventDefault(); - navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n")); - showToast(t("backup_codes_copied"), "success"); - }} + onClick={async (e) => { + e.preventDefault(); + try { + await navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n")); + showToast(t("backup_codes_copied"), "success"); + } catch { + showToast(t("something_went_wrong"), "error"); + } +}}
- onEnable timing: Previously
onEnablelikely fired immediately after a successful enable call; now it fires only when the user clicks Close on the backup-codes step. If the parent UI depends ononEnableto reflect that 2FA is active, this is a behavioral change (enable becomes “effective” only after the user dismisses the backup-codes screen). If that’s intentional, all good—otherwise you may want to callonEnablewhen the API returns 200 and keep the Close button purely visual.Also applies to: 222-231, 261-289
apps/web/pages/auth/login.tsx (1)
24-25: Backup-code toggle flow is solid; consider heading text and “go back” behavior.The introduction of
twoFactorLostAccessplus the newBackupCodecomponent cleanly separates normal TOTP and backup-code flows, and the form wiring viaLoginValues.backupCodelooks correct.A couple of small UX refinements you may want to consider:
- Heading for backup-code mode:
AuthContainer.headingis alwayst("2fa_code")whentwoFactorRequiredis true, even whentwoFactorLostAccessis true and you’re actually showing<BackupCode />. For clarity, you could switch tot("backup_code")in that case, leveraging the new locale key:- heading={twoFactorRequired ? t("2fa_code") : t("welcome_back")} + heading={ + twoFactorRequired + ? twoFactorLostAccess + ? t("backup_code") + : t("2fa_code") + : t("welcome_back") + }
- “Go back” semantics: In
TwoFactorFooter, the Go back button:
- From TOTP → sets
twoFactorRequired(false)and clearstotpCode(back to email/password screen).- From backup-code → only flips
twoFactorLostAccesstofalseand clearsbackupCode(back to TOTP, still in 2FA flow).If the intent is “back one step” this is fine; if the intent is “back to login” from both variants, you might instead always clear both codes and set
twoFactorRequired(false)in both branches.Nothing here is strictly wrong—just worth double-checking the intended UX.
Also applies to: 32-33, 40-45, 70-71, 103-132, 221-221
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/web/components/auth/BackupCode.tsx(1 hunks)apps/web/components/auth/TwoFactor.tsx(2 hunks)apps/web/components/settings/DisableTwoFactorModal.tsx(4 hunks)apps/web/components/settings/EnableTwoFactorModal.tsx(9 hunks)apps/web/components/settings/TwoFactorAuthAPI.ts(1 hunks)apps/web/pages/api/auth/two-factor/totp/disable.ts(2 hunks)apps/web/pages/api/auth/two-factor/totp/setup.ts(3 hunks)apps/web/pages/auth/login.tsx(6 hunks)apps/web/playwright/login.2fa.e2e.ts(3 hunks)apps/web/public/static/locales/en/common.json(1 hunks)packages/features/auth/lib/ErrorCode.ts(1 hunks)packages/features/auth/lib/next-auth-options.ts(4 hunks)packages/lib/test/builder.ts(1 hunks)packages/prisma/migrations/20230804153419_add_backup_codes/migration.sql(1 hunks)packages/prisma/schema.prisma(1 hunks)packages/ui/components/form/inputs/Input.tsx(1 hunks)
🔇 Additional comments (12)
packages/ui/components/form/inputs/Input.tsx (1)
47-51: Verify accessibility impact of removing password toggle from tab order.Adding
tabIndex={-1}removes the password visibility toggle button from keyboard navigation. While this can reduce tab stops for keyboard users, ensure that:
- Keyboard-only users have an alternative way to toggle password visibility if needed
- This aligns with your accessibility guidelines and WCAG standards
Some applications intentionally keep password toggles in the tab order for better keyboard accessibility, while others exclude them as supplementary controls. Verify that this change aligns with your project's accessibility requirements.
packages/prisma/schema.prisma (1)
205-205: LGTM!The optional
backupCodesfield addition is appropriate for storing encrypted backup codes. The nullableString?type correctly reflects that backup codes are only present when 2FA is enabled.packages/prisma/migrations/20230804153419_add_backup_codes/migration.sql (1)
1-2: LGTM!The migration correctly adds a nullable TEXT column to store encrypted backup codes. The TEXT type is appropriate for storing the encrypted JSON string representation of backup codes.
packages/lib/test/builder.ts (1)
192-192: LGTM!The addition of
backupCodes: nullcorrectly updates the test builder to match the updated User schema. The null default is appropriate for test users without 2FA enabled.packages/features/auth/lib/ErrorCode.ts (1)
11-12: LGTM!The new error codes for backup code validation are appropriately named and follow the existing naming conventions. Both
IncorrectBackupCodeandMissingBackupCodesserve clear, distinct purposes in the backup code authentication flow.apps/web/components/auth/TwoFactor.tsx (2)
8-8: LGTM!Adding the
autoFocusprop with a default value oftrueprovides flexibility while maintaining backward compatibility. This is particularly useful when the TwoFactor component is conditionally rendered alongside other input components (like the new BackupCode component).
43-43: LGTM!The updated autofocus logic correctly gates the behavior on the new
autoFocusprop, allowing parent components to control focus management. This improves accessibility and component reusability.apps/web/pages/api/auth/two-factor/totp/setup.ts (1)
60-61: Single-use enforcement for backup codes is properly implemented.The backup codes are correctly enforced as single-use in the primary authentication path (
packages/features/auth/lib/next-auth-options.ts, lines 144–154):
- Validation finds the code via
indexOf()- Used code is replaced with
nulland the entire array is re-encrypted and saved- Subsequent attempts to reuse the same code will not find it in the array (returning -1), triggering an
IncorrectBackupCodeerror- This design prevents replay attacks
apps/web/components/settings/TwoFactorAuthAPI.ts (1)
22-26: Backup-code parameter wiring to disable API looks consistent
disable(password, code, backupCode)correctly posts{ password, code, backupCode }to the server, matching the backend’s expected shape and the modal’s call site(password, totpCode, backupCode). No issues spotted here.apps/web/components/settings/EnableTwoFactorModal.tsx (1)
167-170: Dialog header and password-confirm step look consistent.Using
setupDescriptions[step]for theDialogContentdescription and switching the title to “Backup Codes” in the backup step is consistent with the new enum and locale keys. Replacing the raw password input withPasswordFieldkeeps this step aligned with other password UIs in the app.No issues spotted here.
Also applies to: 174-174
apps/web/public/static/locales/en/common.json (1)
2013-2019: Backup-code localization entries are consistent and complete.The new strings cover all necessary backup-code flows (labels, instructions, success and error messages, and the “Lost access” action) and match surrounding tone and casing.
Looks good as-is.
apps/web/pages/auth/login.tsx (1)
145-161: New backup-code error handling aligns with locale additions.Handling
ErrorCode.IncorrectBackupCodeandErrorCode.MissingBackupCodesexplicitly inonSubmitand surfacingt("incorrect_backup_code")/t("missing_backup_codes")keeps backup-code failures distinct from generic 2FA or credential errors.Assuming the backend returns these two error codes as described, this looks correct.
|
|
||
| test.describe.configure({ mode: "parallel" }); | ||
|
|
||
| // TODO: add more backup code tests, e.g. login + disabling 2fa with backup |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Implement comprehensive backup code authentication tests.
The TODO indicates missing test coverage for critical backup code flows. Consider adding tests for:
- Logging in with a backup code instead of TOTP
- Disabling 2FA using a backup code
- Verifying backup codes are single-use (attempting to reuse a code should fail)
- Handling of the
IncorrectBackupCodeerror
These tests are essential for validating the security properties of backup codes. Would you like me to help draft the test scenarios?
🤖 Prompt for AI Agents
In apps/web/playwright/login.2fa.e2e.ts around line 12, there's a TODO noting
missing backup-code tests; add end-to-end Playwright tests that (1) create/setup
a user with 2FA and generated backup codes, (2) perform login using one valid
backup code instead of TOTP and assert successful authentication and subsequent
UI/state, (3) attempt to reuse the same backup code and assert failure and the
IncorrectBackupCode error/state, (4) perform disabling 2FA using a valid backup
code and assert 2FA is removed and backup codes invalidated, and (5) cover
expected server error responses/messages for IncorrectBackupCode; implement test
helpers to generate/inspect backup codes, use unique test users, clean up state
between tests, and include explicit assertions for single-use behavior and
proper error handling.
| // FIXME: this passes even when switch is not checked, compare to test | ||
| // below which checks for data-state="checked" and works as expected |
There was a problem hiding this comment.
Address the FIXME for switch state validation.
The FIXME indicates that the current assertion passes even when the switch is unchecked, suggesting a potential issue with the test reliability. The comparison with the working data-state="checked" approach (line 127) suggests switching to that pattern.
Apply this diff to use the more reliable state check:
- // FIXME: this passes even when switch is not checked, compare to test
- // below which checks for data-state="checked" and works as expected
- await page.waitForSelector(`[data-testid=two-factor-switch]`);
- await expect(page.locator(`[data-testid=two-factor-switch]`).isChecked()).toBeTruthy();
+ await expect(page.locator(`[data-testid=two-factor-switch][data-state="checked"]`)).toBeVisible();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/playwright/login.2fa.e2e.ts around lines 50 to 51, the current
switch assertion is unreliable (passes when unchecked); replace the current
check with the more robust attribute-based check used later: assert the switch
element's data-state attribute equals "checked" (mirroring the pattern at line
127) so the test only passes when the switch is actually checked.
| import { symmetricDecrypt, symmetricEncrypt } from "@calcom/lib/crypto"; | ||
| import { defaultCookies } from "@calcom/lib/default-cookies"; |
There was a problem hiding this comment.
Backup-code login likely blocked for non‑CAL identity providers
The new backup-code branch looks correct in isolation (decrypt, normalize, single-use via backupCodes[index] = null and re-encrypt), but the surrounding guards still only treat totpCode as the “second factor”:
- Line 113:
if (user.identityProvider !== IdentityProvider.CAL && !credentials.totpCode) { throw ThirdPartyIdentityProviderEnabled; } - Line 117:
if (!user.password && user.identityProvider !== IdentityProvider.CAL && !credentials.totpCode) { ... }
For an OAuth/SAML user on the 2FA screen who enters only a backupCode (no totpCode, no password), !credentials.totpCode is true, so these checks will throw before your backupCode branch (lines 131–157) runs. That means backup-code login works for CAL/password users but is probably broken for non‑CAL users even though the login UI exposes backup codes generically.
If backup codes are intended to work for all identity providers, consider relaxing these conditions to also treat backupCode as a valid second factor, e.g. by checking !credentials.totpCode && !credentials.backupCode instead.
Also, backup-code decryption/validation is now duplicated here and in apps/web/pages/api/auth/two-factor/totp/disable.ts; extracting a shared helper would reduce drift risk when changing the format or error handling.
Also applies to: 62-66, 89-90, 113-129, 131-157
🤖 Prompt for AI Agents
In packages/features/auth/lib/next-auth-options.ts around lines 62-66, 89-90,
113-129 and 131-157, the current guards only treat totpCode as the second factor
which blocks backupCode flows for non-CAL identity providers; update all
relevant conditionals to accept either credentials.totpCode OR
credentials.backupCode (i.e., replace checks like !credentials.totpCode with
!credentials.totpCode && !credentials.backupCode) so backup codes are allowed
the same way as TOTP, and refactor the duplicate backup-code
decryption/validation logic into a shared helper used here and in
apps/web/pages/api/auth/two-factor/totp/disable.ts to avoid divergence (move
decryption, normalization, single-use marking and re-encrypting into that helper
and call it from both places).
2 participants
Test 3
Summary by CodeRabbit
Release Notes
New Features
Localization
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/cal.com-coderabbit#3