Skip to content

Conversation

@everettbu
Copy link

Test 3

Co-authored-by: Peer Richelsen <[email protected]>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR implements backup codes for two-factor authentication (2FA), a critical security feature that provides users with recovery access when their primary TOTP device is unavailable. The implementation spans multiple layers:

Database Changes: Adds a nullable backupCodes field to the User model in Prisma schema with corresponding migration, allowing encrypted storage of recovery codes.

Authentication Flow: Enhances the login process to support backup code entry as an alternative to TOTP codes. The login page now includes a toggle between standard 2FA and backup code modes, with proper state management and error handling for backup code scenarios.

2FA Setup Enhancement: Modifies the 2FA setup flow to generate 10 cryptographically secure backup codes during TOTP setup, encrypt them for database storage, and display them to users with download/copy functionality in a new setup step.

2FA Management: Updates the disable 2FA flow to accept backup codes as an alternative authentication method, allowing users to disable 2FA even without access to their authenticator app.

Security Implementation: Uses AES256 encryption for backup code storage, implements one-time use patterns where backup codes are consumed after validation, and includes proper error handling with new error codes (IncorrectBackupCode, MissingBackupCodes).

UI Components: Introduces a new BackupCode component for backup code input and updates existing components to support the toggle between TOTP and backup code authentication methods.

The feature integrates seamlessly with the existing 2FA infrastructure while following security best practices for backup code implementation.

Confidence score: 3/5

• This PR introduces significant authentication changes with some implementation concerns that need attention before merging
• The core backup codes functionality is well-implemented, but there are naming inconsistencies, validation issues, and potential security concerns in the BackupCode component and authentication flow
• Files needing more attention: apps/web/components/auth/BackupCode.tsx (function naming mismatch, validation issues), packages/features/auth/lib/next-auth-options.ts (error handling, input validation), apps/web/pages/auth/login.tsx (state management complexity)

15 files reviewed, 9 comments

Edit Code Review Bot Settings | Greptile

</div>

{twoFactorRequired && <TwoFactor center />}
{twoFactorRequired ? !twoFactorLostAccess ? <TwoFactor center /> : <BackupCode center /> : null}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Complex conditional rendering logic - consider extracting to a helper function for better readability

Comment on lines +139 to +141
const backupCodes = JSON.parse(
symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY)
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: JSON.parse could throw if backup codes are corrupted - should wrap in try-catch block

Suggested change
const backupCodes = JSON.parse(
symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY)
);
let backupCodes;
try {
backupCodes = JSON.parse(
symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY)
);
} catch (error) {
console.error("Failed to parse backup codes", error);
throw new Error(ErrorCode.InternalServerError);
}

Comment on lines +50 to 53
// 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();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: This FIXME indicates a real test reliability issue. The isChecked() assertion may pass incorrectly. Consider using the same pattern as line 127 with data-state="checked".

Suggested change
// 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();
// 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][data-state="checked"]`)).toBeVisible();

},

async disable(password: string, code: string) {
async disable(password: string, code: string, backupCode: string) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: The backupCode parameter should be optional since users may use either TOTP code or backup code, not both. Consider making it optional: backupCode?: string

return fetch("/api/auth/two-factor/totp/disable", {
method: "POST",
body: JSON.stringify({ password, code }),
body: JSON.stringify({ password, code, backupCode }),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Sending empty/undefined backup codes in the request body may cause issues. Consider only including non-empty values in the JSON payload.

import { useLocale } from "@calcom/lib/hooks/useLocale";
import { Label, TextField } from "@calcom/ui";

export default function TwoFactor({ center = true }) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Function name 'TwoFactor' doesn't match the component's purpose or file name. Should be 'BackupCode' or similar.

Suggested change
export default function TwoFactor({ center = true }) {
export default function BackupCode({ center = true }) {

Comment on lines +22 to +23
minLength={10} // without dash
maxLength={11} // with dash
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Validation logic is flawed. If backup codes are 'XXXXX-XXXXX' format (11 chars), minLength of 10 allows invalid shorter inputs. Consider using exact length validation or regex pattern.

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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Error message mentions 'backup code login' but this is a disable endpoint, not login

Suggested change
throw new Error(ErrorCode.InternalServerError);
console.error("Missing encryption key; cannot proceed with backup code verification.");

setBackupCodes(body.backupCodes);

// create backup codes download url
const textBlob = new Blob([body.backupCodes.map(formatBackupCode).join("\n")], {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: calls formatBackupCode before it's defined (line 163), causing a reference error

Suggested change
const textBlob = new Blob([body.backupCodes.map(formatBackupCode).join("\n")], {
const formatBackupCode = (code: string) => `${code.slice(0, 5)}-${code.slice(5, 10)}`;
const textBlob = new Blob([body.backupCodes.map(formatBackupCode).join("\n")], {

@github-actions
Copy link
Contributor

Thank you for following the naming conventions! 🙏

@github-actions
Copy link
Contributor

This PR is being marked as stale due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants