feat: 2fa backup codes#10
Conversation
Co-authored-by: Peer Richelsen <peeroke@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request introduces a backup code feature for two-factor authentication (2FA) recovery. It adds new components for backup code input, generates and displays codes during 2FA setup, enables disabling 2FA with backup codes, supports backup code validation in login and authentication flows, and updates the database schema to store encrypted backup codes. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Client as Web Client
participant API as Backend API
participant DB as Database
participant Crypto as Encryption
Note over User,Crypto: Enable 2FA with Backup Codes
User->>Client: Click Enable 2FA
Client->>Client: Show password confirmation
User->>Client: Enter password & initiate setup
Client->>API: POST /setup (password)
API->>Crypto: Generate 10 backup codes
Crypto-->>API: Return codes array
API->>Crypto: Encrypt backup codes
Crypto-->>API: Return encrypted string
API->>DB: Store encrypted codes + TOTP secret
API-->>Client: Return secret, QR code, backup codes
Client->>Client: Display backup codes with copy/download
User->>Client: Copy or download codes
User->>Client: Click close
Client->>Client: Mark 2FA enabled
Note over User,Crypto: Disable 2FA with Backup Code (Lost Access)
User->>Client: Click Disable 2FA
Client->>Client: Show password field
User->>Client: Enter password
User->>Client: Toggle Lost Access
Client->>Client: Switch to backup code input
User->>Client: Enter backup code
Client->>API: POST /disable (password, backupCode)
API->>DB: Fetch user with encrypted backup codes
API->>Crypto: Decrypt backup codes
Crypto-->>API: Return codes array
API->>API: Validate entered code
API->>DB: Clear backup codes, disable 2FA
API-->>Client: Success (200)
Client->>Client: Trigger onDisable callback
sequenceDiagram
participant User as User
participant Client as Web Client (Login)
participant API as Backend API
participant DB as Database
participant Crypto as Encryption
Note over User,Crypto: Login with Backup Code Fallback
User->>Client: Enter credentials
Client->>API: POST /login (credentials)
API->>DB: Verify password
API->>API: Check if 2FA enabled
API-->>Client: 2FA required (SecondFactorRequired)
Client->>Client: Show TOTP input
User->>Client: Click Lost Access
Client->>Client: Switch to backup code input
User->>Client: Enter backup code
Client->>API: POST /login (credentials + backupCode)
API->>DB: Fetch user with encrypted codes
API->>Crypto: Decrypt backup codes
Crypto-->>API: Return codes array
API->>API: Validate entered backup code
API->>API: Remove used code from array
API->>Crypto: Encrypt updated codes
API->>DB: Store encrypted codes
API-->>Client: Auth successful (200)
Client->>User: Redirect to dashboard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR spans multiple layers (frontend, backend, database) with heterogeneous changes including new components, API endpoint modifications, authentication flow extensions, database schema updates, and encrypted data handling. The backup code feature requires careful review of encryption/decryption logic, state management across modals, error handling paths, and data persistence. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @apps/web/components/auth/BackupCode.tsx:
- Line 7: The exported component is misnamed: change the function declaration
currently named TwoFactor to BackupCode so the default export and component name
match the file and purpose; update the function signature (TwoFactor ->
BackupCode) and any internal references or prop usages inside this component
(e.g., its props and JSX) to use BackupCode to avoid conflicts with the existing
TwoFactor component.
In @apps/web/components/settings/DisableTwoFactorModal.tsx:
- Around line 113-121: The toggle Button inside the Form (the Button rendering
the {twoFactorLostAccess ? t("go_back") : t("lost_access")} label and calling
setTwoFactorLostAccess and resetForm onClick) lacks an explicit type, so
clicking it may submit the form; update that Button to include type="button" to
ensure it does not trigger form submission when clicked.
In @apps/web/pages/api/auth/two-factor/totp/disable.ts:
- Around line 48-67: The backup code validation block has three issues: add a
defensive type check for req.body.backupCode (ensure it's a string before
calling replaceAll, normalize by removing dashes), wrap the
symmetricDecrypt/JSON.parse call in try-catch and return a suitable error
response on decryption/parsing failure, and enforce single-use backup codes by
removing the matched code from the backupCodes array, re-encrypting it with
symmetricEncrypt and storing it back to user.backupCodes (and persisting the
user record) after successful validation; reference the symbols
req.body.backupCode, symmetricDecrypt, JSON.parse, backupCodes,
symmetricEncrypt, and user.backupCodes in your changes and return appropriate
ErrorCode responses for each failure path.
In @apps/web/playwright/login.2fa.e2e.ts:
- Line 12: Add end-to-end Playwright tests in
apps/web/playwright/login.2fa.e2e.ts that cover backup-code flows: create tests
named "login with backup code", "disable 2FA using backup code", and "reject
invalid backup code"; for each test, reuse existing helper functions (e.g., the
2FA setup helper and the app login helper) to generate a user with backup codes,
perform the UI steps to submit a backup code at the 2FA prompt, assert
successful authentication for a valid backup code, assert that using a valid
backup code disables 2FA by verifying account settings after the flow, and
assert proper error messages and no session for an invalid backup code; ensure
cleanup (consume or restore backup codes) so tests are deterministic.
- Around line 110-126: Expand the backup-codes UI test to validate file and
clipboard content and exercise backup-code authentication: after awaiting the
download (download variable) read its file contents (download.path()/stream) and
assert the codes match the expected fixture/response format; read the clipboard
via page.evaluate(() => navigator.clipboard.readText()) and assert it equals the
same codes; pick one valid code from the expected set and run the real auth flow
(submit it in the 2FA input flow used elsewhere in tests and assert successful
login/redirect), then submit an invalid code and assert the error UI appears;
finally add a test for the "lost access" flow by clicking the lost-access test
id (e.g., getByTestId("lost-access")) and asserting the localized UI and
navigation are correct.
- Around line 50-53: The test's unreliable check using isChecked() on the
`[data-testid=two-factor-switch]` passes even when the switch is off; replace
that with the same DOM-attribute check used elsewhere: assert the switch element
has data-state="checked" (e.g., via expecting the locator to have attribute
data-state equal to "checked" or querying getAttribute('data-state') and
comparing) instead of calling isChecked(). Target the
`[data-testid=two-factor-switch]` locator and update the assertion to verify
data-state="checked".
In @packages/ui/components/form/inputs/Input.tsx:
- Around line 47-51: The password visibility toggle button currently sets
tabIndex={-1} which removes it from keyboard focus; update the Input component
so the toggle button is keyboard accessible by removing tabIndex={-1} from the
button element (the element that calls toggleIsPasswordVisible()), or
alternatively implement and document a keyboard shortcut handler that invokes
toggleIsPasswordVisible() and is wired to the same button for screen-reader and
focus announcements; ensure the button remains type="button" and retains its
onClick and accessible labeling (e.g., aria-label) so keyboard and assistive
tech users can toggle password visibility.
🧹 Nitpick comments (9)
packages/features/auth/lib/next-auth-options.ts (1)
131-157: Backup code validation logic looks secure with minor suggestions.The implementation correctly:
- Validates encryption key presence
- Checks for backup codes existence
- Decrypts, validates, and re-encrypts codes
- Removes used codes (by nulling)
- Maintains fallback to TOTP flow
💡 Optional: Add backup code format validation
Consider adding format validation before attempting to use the backup code to provide better error messages:
if (user.twoFactorEnabled && credentials.backupCode) { if (!process.env.CALENDSO_ENCRYPTION_KEY) { console.error("Missing encryption key; cannot proceed with backup code login."); throw new Error(ErrorCode.InternalServerError); } if (!user.backupCodes) throw new Error(ErrorCode.MissingBackupCodes); + + // Validate backup code format (10 hex characters, optionally with dashes) + const sanitizedCode = credentials.backupCode.replaceAll("-", ""); + if (!/^[0-9a-f]{10}$/i.test(sanitizedCode)) { + throw new Error(ErrorCode.IncorrectBackupCode); + } const backupCodes = JSON.parse( symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) ); - const index = backupCodes.indexOf(credentials.backupCode.replaceAll("-", "")); + const index = backupCodes.indexOf(sanitizedCode); if (index === -1) throw new Error(ErrorCode.IncorrectBackupCode);This would catch malformed codes earlier and provide a consistent error message.
apps/web/pages/api/auth/two-factor/totp/setup.ts (1)
60-61: Consider increasing backup code entropy.The current implementation generates 10-character hexadecimal backup codes (40 bits of entropy per code). While this follows some industry practices and codes are single-use, consider increasing to 16 characters (64 bits) for stronger security, especially since these codes may be stored by users in less secure locations.
💡 Suggested enhancement
-// generate backup codes with 10 character length -const backupCodes = Array.from(Array(10), () => crypto.randomBytes(5).toString("hex")); +// generate backup codes with 16 character length (64 bits of entropy) +const backupCodes = Array.from(Array(10), () => crypto.randomBytes(8).toString("hex"));Or alternatively, format with dashes for readability:
-const backupCodes = Array.from(Array(10), () => crypto.randomBytes(5).toString("hex")); +const backupCodes = Array.from(Array(10), () => { + const code = crypto.randomBytes(8).toString("hex"); + return `${code.slice(0, 4)}-${code.slice(4, 8)}-${code.slice(8, 12)}-${code.slice(12, 16)}`; +});This would produce codes like
a3f2-4b8e-c9d1-7e5awhich are easier to read and type.apps/web/components/settings/TwoFactorAuthAPI.ts (1)
22-25: Consider makingcodeandbackupCodeoptional since they're mutually exclusive.The current signature requires all three parameters, but in practice the caller provides either a TOTP code or a backup code, not both. This leads to empty strings being passed for the unused field.
♻️ Suggested improvement
- async disable(password: string, code: string, backupCode: string) { + async disable(password: string, code?: string, backupCode?: string) { return fetch("/api/auth/two-factor/totp/disable", { method: "POST", - body: JSON.stringify({ password, code, backupCode }), + body: JSON.stringify({ password, ...(code && { code }), ...(backupCode && { backupCode }) }), headers: { "Content-Type": "application/json", }, }); },apps/web/components/auth/BackupCode.tsx (1)
1-28: Add TypeScript interface for props to match the pattern inTwoFactor.tsx.The component accepts a
centerprop but lacks explicit typing. Adding a props interface improves type safety and documentation.♻️ Suggested improvement
import React from "react"; import { useFormContext } from "react-hook-form"; import { useLocale } from "@calcom/lib/hooks/useLocale"; import { Label, TextField } from "@calcom/ui"; +interface BackupCodeProps { + center?: boolean; +} + -export default function TwoFactor({ center = true }) { +export default function BackupCode({ center = true }: BackupCodeProps) {apps/web/pages/auth/login.tsx (2)
73-80: Inconsistent error message handling pattern.The new backup code errors (Lines 157-158) use inline
t()calls while existing errors use theerrorMessageslookup object (Lines 73-80). Consider adding the new error codes to theerrorMessagesobject for consistency.♻️ Suggested improvement
const errorMessages: { [key: string]: string } = { // [ErrorCode.SecondFactorRequired]: t("2fa_enabled_instructions"), // Don't leak information about whether an email is registered or not [ErrorCode.IncorrectEmailPassword]: t("incorrect_email_password"), [ErrorCode.IncorrectTwoFactorCode]: `${t("incorrect_2fa_code")} ${t("please_try_again")}`, [ErrorCode.InternalServerError]: `${t("something_went_wrong")} ${t("please_try_again_and_contact_us")}`, [ErrorCode.ThirdPartyIdentityProviderEnabled]: t("account_created_with_identity_provider"), + [ErrorCode.IncorrectBackupCode]: t("incorrect_backup_code"), + [ErrorCode.MissingBackupCodes]: t("missing_backup_codes"), };Then simplify the error handling:
else if (res.error === ErrorCode.SecondFactorRequired) setTwoFactorRequired(true); - else if (res.error === ErrorCode.IncorrectBackupCode) setErrorMessage(t("incorrect_backup_code")); - else if (res.error === ErrorCode.MissingBackupCodes) setErrorMessage(t("missing_backup_codes")); // fallback if error not found else setErrorMessage(errorMessages[res.error] || t("something_went_wrong"));Also applies to: 157-158
221-221: Nested ternary reduces readability.Consider extracting this logic into a variable or using early returns for clarity.
♻️ Suggested improvement
+ {twoFactorRequired && ( + twoFactorLostAccess ? <BackupCode center /> : <TwoFactor center /> + )} - {twoFactorRequired ? !twoFactorLostAccess ? <TwoFactor center /> : <BackupCode center /> : null}apps/web/components/settings/EnableTwoFactorModal.tsx (3)
273-282: Add error handling for clipboard operations.
navigator.clipboard.writeText()can fail (e.g., due to permissions or unsupported browsers). Consider wrapping in try-catch and showing an error toast on failure.♻️ Suggested improvement
<Button color="secondary" data-testid="backup-codes-copy" - onClick={(e) => { + onClick={async (e) => { e.preventDefault(); - navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n")); - showToast(t("backup_codes_copied"), "success"); + try { + await navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n")); + showToast(t("backup_codes_copied"), "success"); + } catch (err) { + showToast(t("something_went_wrong"), "error"); + } }}> {t("copy")} </Button>
64-64: Add explicit type annotation forbackupCodesstate.The state is initialized as an empty array without type information. Adding explicit typing improves code clarity and catches potential type errors.
♻️ Suggested improvement
- const [backupCodes, setBackupCodes] = useState([]); + const [backupCodes, setBackupCodes] = useState<string[]>([]);
163-163: Consider movingformatBackupCodeoutside the component.This pure function doesn't depend on component state or props. Moving it outside prevents recreation on every render.
♻️ Suggested improvement
+const formatBackupCode = (code: string) => `${code.slice(0, 5)}-${code.slice(5, 10)}`; + const EnableTwoFactorModal = ({ onEnable, onCancel, open, onOpenChange }: EnableTwoFactorModalProps) => { // ... component code - const formatBackupCode = (code: string) => `${code.slice(0, 5)}-${code.slice(5, 10)}`;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/web/components/auth/BackupCode.tsxapps/web/components/auth/TwoFactor.tsxapps/web/components/settings/DisableTwoFactorModal.tsxapps/web/components/settings/EnableTwoFactorModal.tsxapps/web/components/settings/TwoFactorAuthAPI.tsapps/web/pages/api/auth/two-factor/totp/disable.tsapps/web/pages/api/auth/two-factor/totp/setup.tsapps/web/pages/auth/login.tsxapps/web/playwright/login.2fa.e2e.tsapps/web/public/static/locales/en/common.jsonpackages/features/auth/lib/ErrorCode.tspackages/features/auth/lib/next-auth-options.tspackages/lib/test/builder.tspackages/prisma/migrations/20230804153419_add_backup_codes/migration.sqlpackages/prisma/schema.prismapackages/ui/components/form/inputs/Input.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/components/auth/BackupCode.tsx (1)
apps/web/components/auth/TwoFactor.tsx (1)
TwoFactor(8-50)
apps/web/pages/auth/login.tsx (2)
apps/web/components/auth/BackupCode.tsx (1)
TwoFactor(7-29)apps/web/components/auth/TwoFactor.tsx (1)
TwoFactor(8-50)
apps/web/components/settings/DisableTwoFactorModal.tsx (3)
apps/web/components/auth/BackupCode.tsx (1)
TwoFactor(7-29)apps/web/components/auth/TwoFactor.tsx (1)
TwoFactor(8-50)packages/ui/components/dialog/Dialog.tsx (1)
DialogFooter(154-170)
🔇 Additional comments (16)
packages/prisma/schema.prisma (1)
205-205: LGTM: Schema addition for backup codes is correct.The nullable
String?type is appropriate for storing encrypted backup codes while maintaining backwards compatibility.packages/lib/test/builder.ts (1)
192-192: LGTM: Test builder updated correctly.The
backupCodes: nullfield properly aligns with the User model schema addition.packages/prisma/migrations/20230804153419_add_backup_codes/migration.sql (1)
1-2: LGTM: Migration correctly adds the backup codes column.The TEXT type is appropriate for storing encrypted backup codes.
packages/features/auth/lib/ErrorCode.ts (1)
11-12: LGTM: Error codes for backup code validation are well-defined.The naming follows existing conventions and supports the new backup code authentication flow.
apps/web/pages/api/auth/two-factor/totp/disable.ts (1)
107-107: LGTM: Backup codes properly cleared when disabling 2FA.Setting
backupCodes: nullcorrectly removes all backup codes when two-factor authentication is disabled.apps/web/public/static/locales/en/common.json (1)
2013-2019: LGTM! Localization keys are well-structured.The new backup code localization strings are clear, follow existing naming conventions, and provide appropriate user-facing messages for the 2FA backup code feature.
packages/features/auth/lib/next-auth-options.ts (3)
14-14: LGTM! Import addition is appropriate.Adding
symmetricEncryptalongside the existingsymmetricDecryptis necessary for re-encrypting the backup codes array after a code is used.
64-65: LGTM! Credential field properly defined.The
backupCodefield is correctly added to the credentials schema with appropriate label and placeholder text, following the same pattern as the existingtotpCodefield.
89-89: LGTM! User field selection is correct.Adding
backupCodes: trueto the user selection is necessary to retrieve the encrypted backup codes for validation during authentication.apps/web/components/auth/TwoFactor.tsx (1)
8-8: LGTM! Autofocus enhancement improves flexibility.Making autofocus configurable with a sensible default maintains backward compatibility while allowing parent components to control focus behavior. This is particularly useful when toggling between TwoFactor and BackupCode inputs.
Also applies to: 43-43
apps/web/pages/api/auth/two-factor/totp/setup.ts (3)
1-1: LGTM! Appropriate crypto module usage.Using Node.js's built-in
cryptomodule for generating backup codes is the correct choice for cryptographically secure random values.
68-68: LGTM! Backup codes are properly encrypted.Storing backup codes as an encrypted JSON string using
symmetricEncryptfollows the same secure pattern used fortwoFactorSecretand appropriately protects these sensitive recovery credentials.
78-78: Backup codes handling is secure, but consider enhancing the security warning.The API correctly returns plaintext backup codes on first setup (necessary for user to save them), and they are encrypted in the database. Verification confirms:
- No console logging or analytics tracking of codes
- Component unmounts when modal closes, automatically clearing state
- Download and copy functionality use proper browser APIs with cleanup
- HTTPS enforcement handled by production deployment
The UI warning ("Each backup code can be used exactly once...") adequately explains single-use nature but could be more explicit about storing them securely offline. Consider enhancing the description to explicitly recommend secure offline storage.
apps/web/pages/auth/login.tsx (1)
103-133: Well-implemented state management for the two-factor flow.The TwoFactorFooter correctly handles transitions between TOTP and backup code flows, properly clearing the relevant form fields when switching modes. The UX flow with Back and Lost Access buttons is intuitive.
apps/web/components/settings/DisableTwoFactorModal.tsx (1)
43-48: Clean implementation of the disable flow with backup code support.The
resetFormhelper nicely encapsulates form cleanup logic, and the error handling comprehensively covers all relevant error codes. The flow correctly resetstwoFactorLostAccessstate on successful disable.Also applies to: 50-86
apps/web/components/settings/EnableTwoFactorModal.tsx (1)
200-208: Well-implemented backup codes display and actions.The grid layout for displaying codes is clean, the download functionality using object URLs is properly implemented with cleanup on new URL creation, and the user flow of showing backup codes after enabling 2FA follows security best practices.
Also applies to: 261-289
| import { useLocale } from "@calcom/lib/hooks/useLocale"; | ||
| import { Label, TextField } from "@calcom/ui"; | ||
|
|
||
| export default function TwoFactor({ center = true }) { |
There was a problem hiding this comment.
Function name should be BackupCode, not TwoFactor.
The exported function is named TwoFactor but the file is BackupCode.tsx and it renders backup code input, not TOTP input. This creates confusion since there's already a TwoFactor component in TwoFactor.tsx. The naming mismatch could cause maintainability issues and confusion when reading the code.
🔧 Proposed fix
-export default function TwoFactor({ center = true }) {
+export default function BackupCode({ center = true }: { center?: boolean }) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default function TwoFactor({ center = true }) { | |
| export default function BackupCode({ center = true }: { center?: boolean }) { |
🤖 Prompt for AI Agents
In @apps/web/components/auth/BackupCode.tsx at line 7, The exported component is
misnamed: change the function declaration currently named TwoFactor to
BackupCode so the default export and component name match the file and purpose;
update the function signature (TwoFactor -> BackupCode) and any internal
references or prop usages inside this component (e.g., its props and JSX) to use
BackupCode to avoid conflicts with the existing TwoFactor component.
| <Button | ||
| color="minimal" | ||
| className="mr-auto" | ||
| onClick={() => { | ||
| setTwoFactorLostAccess(!twoFactorLostAccess); | ||
| resetForm(false); | ||
| }}> | ||
| {twoFactorLostAccess ? t("go_back") : t("lost_access")} | ||
| </Button> |
There was a problem hiding this comment.
Add type="button" to prevent accidental form submission.
The toggle button is inside a <Form> element. Without explicit type="button", clicking it could trigger form submission in some browsers since the default button type is "submit".
🔧 Proposed fix
<Button
+ type="button"
color="minimal"
className="mr-auto"
onClick={() => {
setTwoFactorLostAccess(!twoFactorLostAccess);
resetForm(false);
}}>
{twoFactorLostAccess ? t("go_back") : t("lost_access")}
</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button | |
| color="minimal" | |
| className="mr-auto" | |
| onClick={() => { | |
| setTwoFactorLostAccess(!twoFactorLostAccess); | |
| resetForm(false); | |
| }}> | |
| {twoFactorLostAccess ? t("go_back") : t("lost_access")} | |
| </Button> | |
| <Button | |
| type="button" | |
| color="minimal" | |
| className="mr-auto" | |
| onClick={() => { | |
| setTwoFactorLostAccess(!twoFactorLostAccess); | |
| resetForm(false); | |
| }}> | |
| {twoFactorLostAccess ? t("go_back") : t("lost_access")} | |
| </Button> |
🤖 Prompt for AI Agents
In @apps/web/components/settings/DisableTwoFactorModal.tsx around lines 113 -
121, The toggle Button inside the Form (the Button rendering the
{twoFactorLostAccess ? t("go_back") : t("lost_access")} label and calling
setTwoFactorLostAccess and resetForm onClick) lacks an explicit type, so
clicking it may submit the form; update that Button to include type="button" to
ensure it does not trigger form submission when clicked.
| if (user.twoFactorEnabled && req.body.backupCode) { | ||
| if (!process.env.CALENDSO_ENCRYPTION_KEY) { | ||
| console.error("Missing encryption key; cannot proceed with backup code login."); | ||
| throw new Error(ErrorCode.InternalServerError); | ||
| } | ||
|
|
||
| if (!user.backupCodes) { | ||
| return res.status(400).json({ error: ErrorCode.MissingBackupCodes }); | ||
| } | ||
|
|
||
| const backupCodes = JSON.parse(symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY)); | ||
|
|
||
| // check if user-supplied code matches one | ||
| const index = backupCodes.indexOf(req.body.backupCode.replaceAll("-", "")); | ||
| if (index === -1) { | ||
| return res.status(400).json({ error: ErrorCode.IncorrectBackupCode }); | ||
| } | ||
|
|
||
| // we delete all stored backup codes at the end, no need to do this here | ||
|
|
There was a problem hiding this comment.
Critical: Multiple security and reliability issues in backup code validation.
Three critical issues must be addressed:
-
Type validation missing (Line 61):
req.body.backupCode.replaceAll("-", "")is called without checking ifbackupCodeis a string. If it'sundefined,null, or another type, this will cause a runtime error. -
Missing error handling (Line 58):
JSON.parse(symmetricDecrypt(...))lacks try-catch. If the encrypted data is corrupted or decryption fails, the endpoint will crash instead of returning a proper error response. -
Security vulnerability - backup code reuse (Lines 61-66): Once a backup code is validated, it's not removed from the array or marked as used. This allows the same backup code to be reused unlimited times until 2FA is fully disabled. Standard security practice requires single-use backup codes.
🔒 Proposed fixes
// if user has 2fa and using backup code
if (user.twoFactorEnabled && req.body.backupCode) {
+ // Validate backupCode is a string
+ if (typeof req.body.backupCode !== 'string') {
+ return res.status(400).json({ error: ErrorCode.IncorrectBackupCode });
+ }
+
if (!process.env.CALENDSO_ENCRYPTION_KEY) {
console.error("Missing encryption key; cannot proceed with backup code login.");
throw new Error(ErrorCode.InternalServerError);
}
if (!user.backupCodes) {
return res.status(400).json({ error: ErrorCode.MissingBackupCodes });
}
- const backupCodes = JSON.parse(symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY));
+ let backupCodes: string[];
+ try {
+ backupCodes = JSON.parse(symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY));
+ } catch (error) {
+ console.error("Failed to decrypt backup codes:", error);
+ return res.status(500).json({ error: ErrorCode.InternalServerError });
+ }
// check if user-supplied code matches one
const index = backupCodes.indexOf(req.body.backupCode.replaceAll("-", ""));
if (index === -1) {
return res.status(400).json({ error: ErrorCode.IncorrectBackupCode });
}
- // we delete all stored backup codes at the end, no need to do this here
+ // Remove the used backup code to prevent reuse
+ backupCodes.splice(index, 1);
+
+ // Update user with remaining backup codes
+ await prisma.user.update({
+ where: { id: session.user.id },
+ data: {
+ backupCodes: backupCodes.length > 0
+ ? symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY)
+ : null,
+ },
+ });
// if user has 2fa and NOT using backup code, try totp
} else if (user.twoFactorEnabled) {Note: You'll need to import symmetricEncrypt from @calcom/lib/crypto.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/web/pages/api/auth/two-factor/totp/disable.ts around lines 48 - 67, The
backup code validation block has three issues: add a defensive type check for
req.body.backupCode (ensure it's a string before calling replaceAll, normalize
by removing dashes), wrap the symmetricDecrypt/JSON.parse call in try-catch and
return a suitable error response on decryption/parsing failure, and enforce
single-use backup codes by removing the matched code from the backupCodes array,
re-encrypting it with symmetricEncrypt and storing it back to user.backupCodes
(and persisting the user record) after successful validation; reference the
symbols req.body.backupCode, symmetricDecrypt, JSON.parse, backupCodes,
symmetricEncrypt, and user.backupCodes in your changes and return appropriate
ErrorCode responses for each failure path.
|
|
||
| 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.
Address the TODO: Add backup code authentication tests.
The PR introduces backup codes but lacks test coverage for critical flows like:
- Logging in with a backup code
- Disabling 2FA using a backup code
- Handling invalid backup codes
These scenarios should be tested before merging to ensure the feature works end-to-end.
Do you want me to generate test cases for these scenarios?
🤖 Prompt for AI Agents
In @apps/web/playwright/login.2fa.e2e.ts at line 12, Add end-to-end Playwright
tests in apps/web/playwright/login.2fa.e2e.ts that cover backup-code flows:
create tests named "login with backup code", "disable 2FA using backup code",
and "reject invalid backup code"; for each test, reuse existing helper functions
(e.g., the 2FA setup helper and the app login helper) to generate a user with
backup codes, perform the UI steps to submit a backup code at the 2FA prompt,
assert successful authentication for a valid backup code, assert that using a
valid backup code disables 2FA by verifying account settings after the flow, and
assert proper error messages and no session for an invalid backup code; ensure
cleanup (consume or restore backup codes) so tests are deterministic.
| // 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(); |
There was a problem hiding this comment.
Fix the unreliable switch state check.
The current check using isChecked() passes even when the switch is unchecked, potentially masking 2FA enablement failures. Use the same approach as line 127.
🔧 Suggested fix
- // 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 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(); |
🤖 Prompt for AI Agents
In @apps/web/playwright/login.2fa.e2e.ts around lines 50 - 53, The test's
unreliable check using isChecked() on the `[data-testid=two-factor-switch]`
passes even when the switch is off; replace that with the same DOM-attribute
check used elsewhere: assert the switch element has data-state="checked" (e.g.,
via expecting the locator to have attribute data-state equal to "checked" or
querying getAttribute('data-state') and comparing) instead of calling
isChecked(). Target the `[data-testid=two-factor-switch]` locator and update the
assertion to verify data-state="checked".
| // backup codes are now showing, so run a few tests | ||
|
|
||
| // click download button | ||
| const promise = page.waitForEvent("download"); | ||
| await page.getByTestId("backup-codes-download").click(); | ||
| const download = await promise; | ||
| expect(download.suggestedFilename()).toBe("cal-backup-codes.txt"); | ||
| // TODO: check file content | ||
|
|
||
| // click copy button | ||
| await page.getByTestId("backup-codes-copy").click(); | ||
| await page.getByTestId("toast-success").waitFor(); | ||
| // TODO: check clipboard content | ||
|
|
||
| // close backup code dialog | ||
| await page.getByTestId("backup-codes-close").click(); | ||
|
|
There was a problem hiding this comment.
Backup code UI tests are incomplete.
The current tests only verify basic UI interactions (download, copy, close buttons) but don't validate:
- The content of downloaded/copied backup codes
- Whether backup codes can actually be used for authentication
- Error handling for invalid backup codes
Consider adding:
- Content validation for downloaded file and clipboard
- A full authentication flow test using a backup code
- Tests for the "lost access" UI flow mentioned in the localization keys
These tests align with the TODO comment on line 12.
🤖 Prompt for AI Agents
In @apps/web/playwright/login.2fa.e2e.ts around lines 110 - 126, Expand the
backup-codes UI test to validate file and clipboard content and exercise
backup-code authentication: after awaiting the download (download variable) read
its file contents (download.path()/stream) and assert the codes match the
expected fixture/response format; read the clipboard via page.evaluate(() =>
navigator.clipboard.readText()) and assert it equals the same codes; pick one
valid code from the expected set and run the real auth flow (submit it in the
2FA input flow used elsewhere in tests and assert successful login/redirect),
then submit an invalid code and assert the error UI appears; finally add a test
for the "lost access" flow by clicking the lost-access test id (e.g.,
getByTestId("lost-access")) and asserting the localized UI and navigation are
correct.
| <button | ||
| className="text-emphasis h-9" | ||
| tabIndex={-1} | ||
| type="button" | ||
| onClick={() => toggleIsPasswordVisible()}> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the Input.tsx file
find . -name "Input.tsx" -path "*/form/inputs/*" | head -5Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 133
🏁 Script executed:
# Read the relevant section of the file
head -80 packages/ui/components/form/inputs/Input.tsx | tail -40Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1207
🏁 Script executed:
# Search for keyboard event handlers related to password visibility
rg -n "onKeyDown|onKeyPress|onKeyUp|KeyboardEvent" packages/ui/components/form/inputs/ -A 3Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 69
🏁 Script executed:
# Search for password visibility toggle logic
rg -n "toggleIsPasswordVisible|isPasswordVisible" packages/ui/components/form/inputs/ -B 2 -A 2Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1957
🏁 Script executed:
# Check if there's a general pattern for buttons with tabIndex={-1}
rg -n 'tabIndex=\{-1\}' packages/ui/components/form/ -B 2 -A 2Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 864
Password toggle button should remain keyboard accessible.
The tabIndex={-1} attribute removes the password visibility toggle from the keyboard tab order, preventing keyboard-only users from toggling password visibility. There is no alternative keyboard mechanism (e.g., keyboard shortcut) to compensate for this removal.
Either restore keyboard access by removing tabIndex={-1}, or implement a keyboard shortcut to make the toggle function available to keyboard users per WCAG 2.1 Level AA requirements (2.1.1 Keyboard).
🤖 Prompt for AI Agents
In @packages/ui/components/form/inputs/Input.tsx around lines 47 - 51, The
password visibility toggle button currently sets tabIndex={-1} which removes it
from keyboard focus; update the Input component so the toggle button is keyboard
accessible by removing tabIndex={-1} from the button element (the element that
calls toggleIsPasswordVisible()), or alternatively implement and document a
keyboard shortcut handler that invokes toggleIsPasswordVisible() and is wired to
the same button for screen-reader and focus announcements; ensure the button
remains type="button" and retains its onClick and accessible labeling (e.g.,
aria-label) so keyboard and assistive tech users can toggle password visibility.
2 participants
Test 3
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/cal.com-coderabbit#3