feat: 2fa backup codes

[akshayutture-augment]

Test 3

---

Replicated from ai-code-review-evaluation/cal.com-greptile#3

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

PR Review: feat: 2fa backup codes

I've completed a comprehensive review of this PR that adds backup code functionality to the two-factor authentication system. Here are my findings:

---

🔒 Critical Security Issues

1. Timing Attack Vulnerability in Backup Code Validation

Location: apps/web/pages/api/auth/two-factor/totp/disable.ts:61 and packages/features/auth/lib/next-auth-options.ts:144

const index = backupCodes.indexOf(credentials.backupCode.replaceAll("-", ""));
if (index === -1) throw new Error(ErrorCode.IncorrectBackupCode);

Issue: Using indexOf() for comparing security-sensitive strings is vulnerable to timing attacks. An attacker can use timing differences to determine if they're getting closer to a valid backup code.

Recommendation: Use constant-time comparison:

import { timingSafeEqual } from 'crypto';

// Compare each code in constant time
let foundIndex = -1;
const inputCode = credentials.backupCode.replaceAll("-", "");
const inputBuffer = Buffer.from(inputCode);

for (let i = 0; i < backupCodes.length; i++) {
  if (backupCodes[i] && backupCodes[i].length === inputCode.length) {
    const codeBuffer = Buffer.from(backupCodes[i]);
    if (timingSafeEqual(inputBuffer, codeBuffer)) {
      foundIndex = i;
      break;
    }
  }
}

2. Backup Code Deletion Logic is Flawed

Location: packages/features/auth/lib/next-auth-options.ts:148

backupCodes[index] = null;

Issue: Setting to null instead of removing the element means:

• The array still contains the position, making it easier to enumerate how many codes have been used
• Could cause issues with array iteration or serialization
• Leaks information about backup code usage patterns

Recommendation: Either remove the element entirely or replace with a secure random value:

backupCodes.splice(index, 1); // Remove completely
// OR
backupCodes[index] = crypto.randomBytes(5).toString("hex"); // Replace with random

3. Missing Rate Limiting on Backup Code Attempts

Location: packages/features/auth/lib/next-auth-options.ts:131-156 and apps/web/pages/api/auth/two-factor/totp/disable.ts:47-65

Issue: While TOTP codes have natural rate limiting (they rotate), backup codes don't. An attacker could potentially brute-force backup codes since:

• Only 10 codes exist (10^10 possibilities = ~33 bits of entropy)
• No rate limiting is applied specifically to backup code attempts
• Failed attempts don't lock the account

Recommendation: Add rate limiting specifically for backup code attempts:

await checkRateLimitAndThrowError({
  identifier: `backup-code:${user.email}`,
  rateLimitingType: "strict" // More restrictive than normal login
});

---

⚠️ Security Concerns

4. Insufficient Entropy in Backup Code Generation

Location: apps/web/pages/api/auth/two-factor/totp/setup.ts:61

const backupCodes = Array.from(Array(10), () => crypto.randomBytes(5).toString("hex"));

Analysis: Each code has 40 bits of entropy (5 bytes = 40 bits). With 10 codes, an attacker needs to guess 1 out of 2^40 values for each attempt.

Consideration: While 40 bits per code may be acceptable with proper rate limiting, consider:

• Industry standards (e.g., Google uses 8-character alphanumeric codes)
• The codes are formatted as "XXXXX-XXXXX" which might lead users to think they're more complex than they are

Recommendation: Consider using 6 bytes (48 bits) or adding complexity requirements.

5. No Backup Code Regeneration Workflow

Issue: Once backup codes are used, there's no apparent way for users to regenerate them without fully disabling and re-enabling 2FA. This could lead to:

• Users being locked out if all codes are used
• Users disabling 2FA unnecessarily

Recommendation: Add a "Regenerate Backup Codes" option that:

• Requires current password + TOTP code
• Invalidates old backup codes
• Generates new set of 10 codes

6. Backup Codes Displayed in Setup Flow Out of Order

Location: apps/web/components/settings/EnableTwoFactorModal.tsx:103

setStep(SetupStep.DisplayQrCode);

Issue: The backup codes are generated during setup but only shown AFTER the user successfully enters a TOTP code. If the user closes the modal or navigates away after enabling 2FA but before seeing the backup codes, they won't have them saved.

Current Flow:

1. Enter password → Get backup codes + QR code
2. Scan QR code
3. Enter TOTP → 2FA enabled
4. THEN see backup codes

Risk: User might not realize they need to save backup codes and close the modal after step 3.

Recommendation: Show backup codes immediately after password confirmation, before displaying the QR code. Make it mandatory to acknowledge/download them before proceeding.

---

🐛 Bugs and Logic Issues

7. Inconsistent Error Handling in Disable API

Location: apps/web/pages/api/auth/two-factor/totp/disable.ts:48-65

if (user.twoFactorEnabled && req.body.backupCode) {
  // backup code logic
} else if (user.twoFactorEnabled) {
  // TOTP code logic
}

Issue: If a user provides BOTH backupCode and code in the request, only the backup code path executes. This could lead to unexpected behavior.

Recommendation: Make the validation mutually exclusive and explicit:

const hasBackupCode = !!req.body.backupCode;
const hasTotpCode = !!req.body.code;

if (hasBackupCode && hasTotpCode) {
  return res.status(400).json({ error: "Provide either backup code or TOTP code, not both" });
}

8. Missing Error Case in Login Flow

Location: apps/web/pages/auth/login.tsx:156-160

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"));

Issue: The MissingBackupCodes error is handled in the login page, but it's unclear when this would actually occur during login. Looking at the auth flow, this error is only thrown in the disable endpoint when backup codes are missing, not during login.

Impact: Dead code or potential future bugs.

9. Memory Leak: Blob URL Not Revoked

Location: apps/web/components/settings/EnableTwoFactorModal.tsx:98

if (backupCodesUrl) URL.revokeObjectURL(backupCodesUrl);
setBackupCodesUrl(URL.createObjectURL(textBlob));

Issue: The blob URL is created but only revoked on re-creation. If the component unmounts, the URL is never revoked, causing a memory leak.

Recommendation: Add cleanup in useEffect:

useEffect(() => {
  return () => {
    if (backupCodesUrl) {
      URL.revokeObjectURL(backupCodesUrl);
    }
  };
}, [backupCodesUrl]);

---

📋 Code Quality Issues

10. Magic Numbers and Hardcoded Values

Location: Multiple files

// apps/web/components/auth/BackupCode.tsx:22-23
minLength={10} // without dash
maxLength={11} // with dash

// apps/web/pages/api/auth/two-factor/totp/setup.ts:61
const backupCodes = Array.from(Array(10), () => crypto.randomBytes(5).toString("hex"));

Recommendation: Extract to constants:

const BACKUP_CODE_COUNT = 10;
const BACKUP_CODE_BYTES = 5;
const BACKUP_CODE_LENGTH_WITHOUT_DASH = 10;
const BACKUP_CODE_LENGTH_WITH_DASH = 11;

11. Inconsistent Backup Code Formatting

Issue: The code uses replaceAll("-", "") in multiple places but doesn't validate the format consistently.

Locations:

• apps/web/pages/api/auth/two-factor/totp/disable.ts:61
• packages/features/auth/lib/next-auth-options.ts:144

Recommendation: Create a utility function:

function normalizeBackupCode(code: string): string {
  return code.replaceAll("-", "").toLowerCase().trim();
}

12. Missing Input Validation

Location: apps/web/components/auth/BackupCode.tsx:17-26

<TextField
  id="backup-code"
  minLength={10}
  maxLength={11}
 
/>

Issue: No pattern validation or input masking. Users could enter invalid characters.

Recommendation: Add pattern validation:

pattern="[0-9a-fA-F]{5}-?[0-9a-fA-F]{5}"

---

🧪 Testing Issues

13. Incomplete E2E Test Coverage

Location: apps/web/playwright/login.2fa.e2e.ts:11

// TODO: add more backup code tests, e.g. login + disabling 2fa with backup

Missing Test Cases:

• ✅ Enable 2FA and view backup codes (tested)
• ✅ Disable 2FA with TOTP code (tested)
• ❌ Login with backup code (TODO)
• ❌ Disable 2FA with backup code (TODO)
• ❌ Using same backup code twice (should fail)
• ❌ Backup code with invalid format
• ❌ Exhausting all backup codes

Recommendation: Implement the TODO tests before merging.

14. No Unit Tests for Crypto Operations

Issue: The security-critical encryption/decryption of backup codes has no unit tests.

Recommendation: Add tests for:

• Backup code generation (entropy, uniqueness)
• Encryption/decryption round-trip
• Handling of null/empty backup codes

---

📝 Documentation and UX Issues

15. Unclear User Guidance

Location: apps/web/components/settings/EnableTwoFactorModal.tsx:58

[SetupStep.DisplayBackupCodes]: t("backup_code_instructions"),

The instruction is: "Each backup code can be used exactly once to grant access without your authenticator."

Issues:

• Doesn't emphasize that codes are consumed on use
• Doesn't warn about the importance of storing them securely
• Doesn't mention what happens when codes run out

Recommendation: Update the localization to be more explicit:

"backup_code_instructions": "Save these backup codes in a secure location. Each code can only be used once. If you lose access to your authenticator and use all backup codes, you'll need to contact support to regain access."

16. No Warning About Backup Code Storage

Issue: Users might screenshot or save codes insecurely without understanding the security implications.

Recommendation: Add a warning in the UI:

<Alert severity="warning">
  Store these codes securely. Anyone with access to these codes can bypass your two-factor authentication.
</Alert>

---

✅ Positive Aspects

1. Proper encryption: Backup codes are encrypted at rest using symmetricEncrypt
2. Good DB design: Single nullable backupCodes field, clean migration
3. Proper TOTP secret validation: Length checks prevent decryption issues
4. User experience: Good modal flow with download/copy options
5. Error codes: New error codes properly defined and handled
6. Test infrastructure: E2E test foundation is solid

---

📊 Summary

Category	Critical	High	Medium	Low
Security	3	3	0	0
Bugs	0	2	1	0
Code Quality	0	0	3	0
Testing	0	1	1	0
Documentation	0	0	2	0
Total	3	6	7	0

---

🎯 Recommendations Before Merge

Must Fix (Blocking):

1. ✋ Fix timing attack vulnerability in backup code comparison
2. ✋ Add rate limiting for backup code attempts
3. ✋ Fix backup code deletion logic (null vs removal)
4. ✋ Fix backup code display flow (show before enabling 2FA)

Should Fix (High Priority):

1. Implement complete E2E tests for backup code login
2. Fix memory leak with blob URL
3. Add backup code regeneration feature
4. Improve user warnings about secure storage

Nice to Have:

1. Extract magic numbers to constants
2. Add input validation patterns
3. Create backup code utility functions
4. Add unit tests for crypto operations

---

🔗 Related Files Reviewed

• packages/prisma/schema.prisma:205 - DB schema
• apps/web/pages/api/auth/two-factor/totp/setup.ts:61 - Code generation
• apps/web/pages/api/auth/two-factor/totp/disable.ts:48-65 - Disable with backup code
• packages/features/auth/lib/next-auth-options.ts:131-156 - Login with backup code
• apps/web/components/auth/BackupCode.tsx - UI component
• apps/web/components/settings/EnableTwoFactorModal.tsx - Setup flow

---

[github-actions]

This PR is being marked as stale due to inactivity.