feat: 2fa backup codes#3
Conversation
Co-authored-by: Peer Richelsen <peeroke@gmail.com>
WalkthroughThis update introduces backup code functionality for two-factor authentication (2FA) across the authentication, user interface, API, and database layers. It adds backup code generation, storage, validation, and user flows for login and 2FA disablement, along with corresponding UI components, error handling, localization, and end-to-end tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant API
participant DB
User->>UI: Initiate 2FA setup
UI->>API: Request 2FA setup
API->>DB: Generate TOTP secret & backup codes, store encrypted
API-->>UI: Return TOTP setup data + backup codes
UI->>User: Display QR, backup codes, copy/download options
User->>UI: Login with backup code
UI->>API: Submit credentials + backup code
API->>DB: Validate backup code, invalidate if used
API-->>UI: Success or error
sequenceDiagram
participant User
participant UI
participant API
participant DB
User->>UI: Request 2FA disable (lost device)
UI->>API: Submit password + backup code
API->>DB: Validate backup code, clear 2FA & backup codes
API-->>UI: Return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
apps/web/public/static/locales/en/common.json (1)
2013-2019: Minor consistency nitpick on new i18n keysThe new keys are valid JSON and follow the surrounding style.
Small polish you might consider:- "backup_code_instructions": "Each backup code can be used exactly once to grant access without your authenticator.", + "backup_code_instructions": "Each backup code can be used exactly once to grant access without an authenticator.",(avoids the ambiguous “your”).
Not blocking.apps/web/pages/api/auth/two-factor/totp/setup.ts (1)
60-61: Consider strengthening backup code entropy and format.The current implementation generates 10-character hex codes from 5 random bytes. While functional, consider these improvements for better security and usability:
- // generate backup codes with 10 character length - const backupCodes = Array.from(Array(10), () => crypto.randomBytes(5).toString("hex")); + // Generate backup codes with higher entropy (8 bytes = 16 hex chars) and standard format + const backupCodes = Array.from(Array(10), () => { + const code = crypto.randomBytes(8).toString("hex"); + // Format as XXXXX-XXXXX for better readability + return `${code.slice(0, 5)}-${code.slice(5, 10)}`; + });Benefits:
- Increased entropy (64 bits vs 40 bits)
- Standard XXXXX-XXXXX format matches UI placeholder
- Better resistance to brute force attacks
apps/web/pages/api/auth/two-factor/totp/disable.ts (1)
69-69: Improve conditional structure clarity.The nested if-else structure for 2FA handling could be clearer. Consider restructuring for better readability.
- // if user has 2fa and NOT using backup code, try totp - } else if (user.twoFactorEnabled) { + // if user has 2fa and NOT using backup code, verify TOTP + } else if (user.twoFactorEnabled && !req.body.backupCode) {This makes the logic flow more explicit: backup code → TOTP → proceed to disable.
apps/web/playwright/login.2fa.e2e.ts (1)
117-117: Implement content verification TODOs for robust testing.The TODOs for file and clipboard content verification should be implemented to ensure backup codes are correctly generated and copied.
// Example implementation for file content verification const fileContent = await download.path(); const content = await fs.readFile(fileContent, 'utf-8'); const lines = content.trim().split('\n'); expect(lines).toHaveLength(10); // 10 backup codes lines.forEach(line => { expect(line).toMatch(/^[a-f0-9]{5}-[a-f0-9]{5}$/); // Format validation }); // Example for clipboard content (requires proper test setup) const clipboardContent = await page.evaluate(() => navigator.clipboard.readText()); expect(clipboardContent).toContain('Backup codes:');Would you like me to implement these content verification tests?
Also applies to: 122-122
apps/web/pages/auth/login.tsx (2)
104-133: Well-implemented backup code recovery flowThe state management and UI transitions are correctly implemented. The clearing of appropriate form fields when switching modes prevents data leakage between states.
Consider adding aria-labels or data-testid attributes to these buttons for better accessibility and testing.
221-221: Consider improving readability of nested ternaryThe logic is correct, but the nested ternary could be more readable.
Consider extracting this to a more readable format:
-{twoFactorRequired ? !twoFactorLostAccess ? <TwoFactor center /> : <BackupCode center /> : null} +{twoFactorRequired && ( + twoFactorLostAccess ? <BackupCode center /> : <TwoFactor center /> +)}apps/web/components/settings/EnableTwoFactorModal.tsx (1)
92-99: Well-implemented download URL generation with proper cleanupGood practice revoking the previous blob URL before creating a new one. The formatting of backup codes for download matches the display format.
Consider also cleaning up the blob URL in a cleanup function when the component unmounts.
📜 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 context used
🧬 Code Graph Analysis (1)
apps/web/pages/auth/login.tsx (3)
packages/ui/components/button/Button.tsx (1)
Button(116-220)apps/web/components/auth/BackupCode.tsx (1)
TwoFactor(7-29)apps/web/components/auth/TwoFactor.tsx (1)
TwoFactor(8-50)
🔇 Additional comments (23)
packages/features/auth/lib/ErrorCode.ts (1)
11-12: Enum extension looks goodThe new error-code identifiers follow the existing kebab-case pattern and don’t collide with any existing values.
packages/prisma/schema.prisma (1)
205-205: LGTM! Schema design aligns with security best practices.The nullable
backupCodesfield is appropriately designed for storing encrypted backup codes. The optional nature makes sense since not all users will have 2FA enabled.packages/lib/test/builder.ts (1)
192-192: LGTM! Test builder properly updated for new schema field.The addition of
backupCodes: nullensures test user objects remain compatible with the updated User model schema.packages/ui/components/form/inputs/Input.tsx (1)
47-51: LGTM! Accessibility improvement for password toggle button.Adding
tabIndex={-1}removes the password visibility toggle from keyboard navigation, which is appropriate for this secondary UI control and improves the overall form accessibility.apps/web/components/auth/TwoFactor.tsx (2)
8-8: LGTM! Good addition of configurable autoFocus prop.The optional
autoFocusprop with a sensible default maintains backward compatibility while providing flexibility for different usage contexts.
43-43: LGTM! Correct conditional autoFocus implementation.The logic
autoFocus && index === 0properly applies autofocus only to the first input when the prop is enabled.apps/web/components/settings/TwoFactorAuthAPI.ts (1)
25-25: LGTM! JSON payload correctly includes backup code parameter.The backup code is properly included in the request payload for server-side processing.
apps/web/pages/api/auth/two-factor/totp/setup.ts (2)
1-1: LGTM: Proper crypto import for secure backup code generation.The Node.js crypto module is the appropriate choice for generating cryptographically secure backup codes.
68-68: LGTM: Proper encryption of backup codes before storage.The backup codes are correctly encrypted using the same symmetric encryption key as the TOTP secret, maintaining consistent security practices.
apps/web/pages/api/auth/two-factor/totp/disable.ts (2)
47-67: LGTM: Robust backup code validation with proper security measures.The implementation correctly:
- Validates encryption key presence
- Checks for backup codes existence
- Normalizes input by removing dashes
- Uses secure comparison for code matching
- Handles error cases appropriately
The logic is sound and secure.
107-107: LGTM: Proper cleanup of backup codes on 2FA disable.Setting
backupCodes: nullensures complete cleanup when disabling 2FA, preventing any residual backup codes from being usable.apps/web/playwright/login.2fa.e2e.ts (1)
110-126: LGTM: Comprehensive backup codes UI testing.The test coverage for backup codes UI interactions is thorough:
- Download functionality with filename verification
- Copy functionality with success toast validation
- Dialog closure
The TODO comments for content verification are valid future improvements.
packages/features/auth/lib/next-auth-options.ts (4)
14-14: LGTM: Proper import addition for backup code re-encryption.Adding
symmetricEncryptalongsidesymmetricDecryptis necessary for re-encrypting backup codes after use.
65-65: LGTM: Clean credential field addition.The backup code credential field is properly integrated into the existing credentials structure.
89-89: LGTM: Proper database field selection.Adding
backupCodesto the user selection ensures the encrypted backup codes are available for validation.
157-157: LGTM: Clean fallback to TOTP validation.The conditional structure properly handles the fallback from backup codes to TOTP validation when no backup code is provided.
apps/web/pages/auth/login.tsx (1)
157-158: LGTM! Error handling for backup codes is consistentThe error handling follows the existing pattern and uses localized messages appropriately.
apps/web/components/settings/DisableTwoFactorModal.tsx (3)
43-48: Clean implementation of form reset logicThe
resetFormfunction with the optionalclearPasswordparameter is well-designed, allowing password to persist when toggling between TOTP and backup code modes while clearing it on successful completion.
69-76: Comprehensive error handling for all 2FA scenariosGood addition of all relevant error cases including the SecondFactorRequired check. The error messages are properly localized.
103-107: Good UX consideration with autoFocus controlSetting
autoFocus={false}on the TwoFactor component prevents unwanted focus behavior when the modal opens, improving the user experience.apps/web/components/settings/EnableTwoFactorModal.tsx (3)
8-8: Good security improvement using PasswordFieldSwitching from a generic text field to PasswordField improves security by properly masking the password input.
135-135: Important UX improvement ensuring users see backup codesGood change to display backup codes before completing the setup. This ensures users have a chance to save their backup codes.
261-289: Excellent implementation of backup code actionsThe three-button approach (Close, Copy, Download) gives users multiple ways to save their backup codes. Good use of:
- Toast notification for copy confirmation
- HTML5 download attribute for file download
- Test IDs for all actions
| 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.
🛠️ Refactor suggestion
Improve component naming and type safety.
The component is named TwoFactor but handles backup codes specifically. The center prop lacks type definition.
-export default function TwoFactor({ center = true }) {
+interface BackupCodeProps {
+ center?: boolean;
+}
+
+export default function BackupCode({ center = true }: BackupCodeProps) {Also update the filename to match: BackupCode.tsx → component name should be BackupCode.
📝 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 }) { | |
| interface BackupCodeProps { | |
| center?: boolean; | |
| } | |
| export default function BackupCode({ center = true }: BackupCodeProps) { |
🤖 Prompt for AI Agents
In apps/web/components/auth/BackupCode.tsx at line 7, rename the component from
TwoFactor to BackupCode to match the filename and its specific functionality.
Add a proper TypeScript type definition for the center prop, such as defining an
interface or type for the props with center as a boolean. This improves clarity
and type safety.
| <div className={center ? "mx-auto !mt-0 max-w-sm" : "!mt-0 max-w-sm"}> | ||
| <Label className="mt-4">{t("backup_code")}</Label> | ||
|
|
||
| <p className="text-subtle mb-4 text-sm">{t("backup_code_instructions")}</p> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper accessibility connection.
The instructions should be properly connected to the input field for screen readers.
- <p className="text-subtle mb-4 text-sm">{t("backup_code_instructions")}</p>
+ <p id="backup-code-instructions" className="text-subtle mb-4 text-sm">
+ {t("backup_code_instructions")}
+ </p>📝 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.
| <p className="text-subtle mb-4 text-sm">{t("backup_code_instructions")}</p> | |
| <p id="backup-code-instructions" className="text-subtle mb-4 text-sm"> | |
| {t("backup_code_instructions")} | |
| </p> |
🤖 Prompt for AI Agents
In apps/web/components/auth/BackupCode.tsx at line 15, the paragraph with backup
code instructions is not properly connected to the input field for
accessibility. Add an aria-describedby attribute to the input element
referencing the id of this paragraph to ensure screen readers associate the
instructions with the input field.
| <TextField | ||
| id="backup-code" | ||
| label="" | ||
| defaultValue="" | ||
| placeholder="XXXXX-XXXXX" | ||
| minLength={10} // without dash | ||
| maxLength={11} // with dash | ||
| required | ||
| {...methods.register("backupCode")} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance input validation and accessibility.
The current validation only checks length but doesn't validate the backup code format. Also missing accessibility attributes.
<TextField
id="backup-code"
label=""
+ aria-label={t("backup_code")}
+ aria-describedby="backup-code-instructions"
defaultValue=""
placeholder="XXXXX-XXXXX"
minLength={10} // without dash
maxLength={11} // with dash
required
+ pattern="[a-fA-F0-9]{5}-?[a-fA-F0-9]{5}"
+ title={t("backup_code_format_hint")}
{...methods.register("backupCode")}
/>Add corresponding localization key for format hint.
📝 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.
| <TextField | |
| id="backup-code" | |
| label="" | |
| defaultValue="" | |
| placeholder="XXXXX-XXXXX" | |
| minLength={10} // without dash | |
| maxLength={11} // with dash | |
| required | |
| {...methods.register("backupCode")} | |
| /> | |
| <TextField | |
| id="backup-code" | |
| label="" | |
| aria-label={t("backup_code")} | |
| aria-describedby="backup-code-instructions" | |
| defaultValue="" | |
| placeholder="XXXXX-XXXXX" | |
| minLength={10} // without dash | |
| maxLength={11} // with dash | |
| required | |
| pattern="[a-fA-F0-9]{5}-?[a-fA-F0-9]{5}" | |
| title={t("backup_code_format_hint")} | |
| {...methods.register("backupCode")} | |
| /> |
🤖 Prompt for AI Agents
In apps/web/components/auth/BackupCode.tsx around lines 17 to 26, the TextField
input only validates length but lacks format validation and accessibility
attributes. Enhance validation by adding a pattern attribute or custom
validation to enforce the backup code format (e.g., five characters, a dash,
then five characters). Add an aria-describedby attribute linking to a localized
format hint message for accessibility. Also, create and use a localization key
for the format hint text to support internationalization.
| }, | ||
|
|
||
| async disable(password: string, code: string) { | ||
| async disable(password: string, code: string, backupCode: string) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider parameter validation and mutual exclusivity.
The method signature accepts both code and backupCode parameters, but typically only one should be provided. Consider validating that exactly one authentication method is provided.
-async disable(password: string, code: string, backupCode: string) {
+async disable(password: string, code?: string, backupCode?: string) {And add validation to ensure exactly one authentication method is provided:
if ((!code && !backupCode) || (code && backupCode)) {
throw new Error("Provide either TOTP code or backup code, not both");
}🤖 Prompt for AI Agents
In apps/web/components/settings/TwoFactorAuthAPI.ts at line 22, the disable
method accepts both code and backupCode parameters but should enforce that
exactly one is provided. Add validation at the start of the method to check if
either code or backupCode is provided exclusively, and throw an error if neither
or both are given, using a condition like: if ((!code && !backupCode) || (code
&& backupCode)) { throw new Error("Provide either TOTP code or backup code, not
both"); }.
| const dataUri = await qrcode.toDataURL(keyUri); | ||
|
|
||
| return res.json({ secret, keyUri, dataUri }); | ||
| return res.json({ secret, keyUri, dataUri, backupCodes }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security concern: Backup codes returned in plaintext.
While necessary for user display, returning backup codes in the API response creates a brief window where they exist in plaintext. Ensure the frontend handles these securely and doesn't log them.
Consider adding a security comment to remind developers:
- return res.json({ secret, keyUri, dataUri, backupCodes });
+ // SECURITY: Backup codes are returned in plaintext for user display
+ // Frontend must handle securely and avoid logging
+ return res.json({ secret, keyUri, dataUri, backupCodes });📝 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.
| return res.json({ secret, keyUri, dataUri, backupCodes }); | |
| // SECURITY: Backup codes are returned in plaintext for user display | |
| // Frontend must handle securely and avoid logging | |
| return res.json({ secret, keyUri, dataUri, backupCodes }); |
🤖 Prompt for AI Agents
In apps/web/pages/api/auth/two-factor/totp/setup.ts at line 78, the backup codes
are returned in plaintext in the API response, which poses a security risk if
mishandled. Add a clear security comment above this return statement to remind
developers that backup codes must be handled securely on the frontend, avoiding
logging or exposing them unnecessarily.
|
|
||
| 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.
💡 Verification agent
🧩 Analysis chain
Address the TODO: Expand backup code test coverage.
The TODO correctly identifies missing test scenarios. Consider implementing these critical backup code flows:
Would you like me to help implement the missing test cases for:
- Login using backup codes
- Disabling 2FA with backup codes
- Backup code exhaustion scenarios
🏁 Script executed:
#!/bin/bash
# Verify if there are existing tests for backup code login and disable flows
ast-grep --pattern $'test($_, async ({ page, users }) => {
$$$
backupCode
$$$
})'Length of output: 88
🏁 Script executed:
#!/bin/bash
# Search for any references to backup code flows in e2e tests
rg -ni 'backup' -g '*.e2e.ts'
# Also search across the entire repository for backup code mentions
rg -ni 'backup' Length of output: 11779
Expand Backup Code E2E Test Coverage
We currently only verify download/copy/close UI flows in apps/web/playwright/login.2fa.e2e.ts (lines 110–125), but are missing end-to-end tests for the actual backup code login and disable flows, as well as exhaustion scenarios. Please add tests for:
- Logging in with a valid backup code when TOTP isn’t available
- Disabling 2FA via backup code in the settings modal
- Handling all backup codes being used (exhaustion/failure case)
Let me know if you’d like help scaffolding these test cases.
🤖 Prompt for AI Agents
In apps/web/playwright/login.2fa.e2e.ts around line 12, the current tests only
cover UI flows for backup codes but lack end-to-end tests for critical backup
code functionality. Add new test cases to cover logging in with a valid backup
code when TOTP is unavailable, disabling 2FA using a backup code through the
settings modal, and handling the scenario where all backup codes have been used
and login fails. Structure these tests to simulate real user interactions and
verify expected outcomes for each case.
| // 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.
🛠️ Refactor suggestion
Fix the isChecked() reliability issue.
The FIXME comment identifies a real problem. The isChecked() method can be unreliable for custom components.
- // 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 page.waitForSelector(`[data-testid=two-factor-switch]`); | |
| 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 to 53, the use of
isChecked() on the two-factor-switch locator is unreliable for this custom
component. Replace the isChecked() check with an assertion that verifies the
element's attribute data-state equals "checked" to ensure the switch is actually
in the checked state.
| 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); | ||
|
|
||
| const backupCodes = JSON.parse( | ||
| symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) | ||
| ); | ||
|
|
||
| // check if user-supplied code matches one | ||
| const index = backupCodes.indexOf(credentials.backupCode.replaceAll("-", "")); | ||
| if (index === -1) throw new Error(ErrorCode.IncorrectBackupCode); | ||
|
|
||
| // delete verified backup code and re-encrypt remaining | ||
| backupCodes[index] = null; | ||
| await prisma.user.update({ | ||
| where: { | ||
| id: user.id, | ||
| }, | ||
| data: { | ||
| backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Security issue: Potential timing attack vulnerability.
The backup code validation uses indexOf() which could leak timing information about code position. Also, the code invalidation approach needs security review.
Issues identified:
- Timing attack:
indexOf()may leak position information - Array mutation: Setting
backupCodes[index] = nullcreates sparse array - No rate limiting on backup code attempts
Recommended fixes:
- // check if user-supplied code matches one
- const index = backupCodes.indexOf(credentials.backupCode.replaceAll("-", ""));
- if (index === -1) throw new Error(ErrorCode.IncorrectBackupCode);
-
- // delete verified backup code and re-encrypt remaining
- backupCodes[index] = null;
+ // Use constant-time comparison to prevent timing attacks
+ const normalizedInput = credentials.backupCode.replaceAll("-", "");
+ let isValidCode = false;
+ let validIndex = -1;
+
+ for (let i = 0; i < backupCodes.length; i++) {
+ if (backupCodes[i] && backupCodes[i] === normalizedInput) {
+ isValidCode = true;
+ validIndex = i;
+ break;
+ }
+ }
+
+ if (!isValidCode) throw new Error(ErrorCode.IncorrectBackupCode);
+
+ // Remove used backup code and filter out nulls
+ backupCodes[validIndex] = null;
+ const remainingCodes = backupCodes.filter(code => code !== null);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/features/auth/lib/next-auth-options.ts lines 131 to 156, replace the
use of indexOf() for backup code validation with a constant-time comparison
function to prevent timing attacks. Instead of setting backupCodes[index] to
null, remove the used backup code from the array to avoid creating sparse
arrays. Additionally, implement rate limiting on backup code login attempts to
mitigate brute-force attacks.
| await prisma.user.update({ | ||
| where: { | ||
| id: user.id, | ||
| }, | ||
| data: { | ||
| backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider backup code exhaustion handling.
The current implementation doesn't handle the case when all backup codes are exhausted. This could lead to a user being locked out.
await prisma.user.update({
where: {
id: user.id,
},
data: {
- backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY),
+ backupCodes: remainingCodes.length > 0
+ ? symmetricEncrypt(JSON.stringify(remainingCodes), process.env.CALENDSO_ENCRYPTION_KEY)
+ : null, // Clear backup codes if all exhausted
},
});
+
+ // Log backup code usage for security monitoring
+ console.log(`Backup code used for user ${user.id}. Remaining codes: ${remainingCodes.length}`);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/features/auth/lib/next-auth-options.ts around lines 149 to 156, the
code updates user backup codes without checking if all backup codes are
exhausted, risking user lockout. Add logic to detect when no backup codes remain
and handle this case appropriately, such as prompting the user to regenerate
backup codes or providing an alternative recovery method before updating the
database.
| @@ -0,0 +1,2 @@ | |||
| -- AlterTable | |||
| ALTER TABLE "users" ADD COLUMN "backupCodes" TEXT; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm backup codes are irreversibly hashed before persistence
Adding a plain TEXT column is fine for flexibility, but backup codes are effectively second-factor credentials.
Double-check that the application layer stores ONLY a salted hash (or equivalent one-way representation) rather than the raw codes, and that no process can emit them in logs.
If you’d like a quick sanity-check across the codebase:
🏁 Script executed:
#!/bin/bash
# Search for direct INSERT/UPDATEs of `backupCodes` that do NOT use a hashing helper
rg --max-depth 4 'backupCodes' packages/apps | grep -n --invert-match 'hash\|encrypt'Length of output: 139
🏁 Script executed:
#!/usr/bin/env bash
# Search for any application-level references to `backupCodes` (excludes migration SQL files)
rg -n 'backupCodes' . | grep -v 'migrations/'Length of output: 2887
Prefer one-way hashing over reversible encryption for backup codes
The current code uses symmetricEncrypt/symmetricDecrypt to persist backup codes, which means they remain recoverable if the encryption key is compromised. Since backup codes function as a second-factor credential, they should be stored using a one-way, salted hash (e.g. Argon2 or bcrypt), not reversible encryption.
Please update the following areas:
• apps/web/pages/api/auth/two-factor/totp/setup.ts
– Remove symmetricEncrypt(JSON.stringify(backupCodes), …)
– Store each code’s hash instead of the encrypted blob
• apps/web/pages/api/auth/two-factor/totp/disable.ts
– Replace JSON-decrypt + array lookup with direct hash comparisons of incoming codes
– Persist updated code list as hashes
• packages/features/auth/lib/next-auth-options.ts
– Swap out symmetricDecrypt/symmetricEncrypt for hash-and-compare logic
– Ensure the salt and hashing parameters follow current best practices
Also audit the codebase (especially in logging or error paths) to ensure raw backup codes never appear in logs or error messages.
🤖 Prompt for AI Agents
In packages/prisma/migrations/20230804153419_add_backup_codes/migration.sql at
line 2, the backupCodes column is currently stored as plain text, which supports
reversible encryption. To comply with security best practices, update the
application code in apps/web/pages/api/auth/two-factor/totp/setup.ts,
disable.ts, and packages/features/auth/lib/next-auth-options.ts to store backup
codes as one-way salted hashes (e.g., using Argon2 or bcrypt) instead of
encrypting them. Remove all uses of symmetricEncrypt and symmetricDecrypt,
replace JSON serialization and decryption with hash storage and comparison, and
ensure no raw backup codes are logged anywhere in the codebase.
|
This PR is being marked as stale due to inactivity. |
2 participants
Test 3
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores