feat: 2fa backup codes

[ShashankFC]

Test 3nn---nReplicated from ai-code-review-evaluation/cal.com-greptile#3

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 2m 46s —— View job

---

Code Review for 2FA Backup Codes Feature

I've completed a thorough review of the 2FA backup codes implementation. Here are my findings:

---

Critical Security Issues

1. Backup Code Storage Vulnerability (HIGH PRIORITY)

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

The backup code deletion logic has a critical flaw:

backupCodes[index] = null;

Problem: Setting an array element to null doesn't remove it - it creates a null entry that can be matched by future attempts. An attacker could discover a used backup code and use null as input to authenticate.

Impact: Once a user has used any backup code, an attacker can potentially authenticate using a null/empty backup code.

Fix this →

---

High Priority Issues

2. Missing Backup Code Validation in Login Flow

File: apps/web/pages/auth/login.tsx

The login page doesn't validate backup code format before submission. Users can submit malformed codes unnecessarily.

Recommendation: Add client-side validation for backup code format (10 characters, alphanumeric, optional dash).

3. Inconsistent Error Handling

Files: apps/web/pages/api/auth/two-factor/totp/disable.ts:51, next-auth-options.ts:137

The disable endpoint throws errors while the login flow uses throw new Error(). Error handling should be consistent across authentication flows.

Issue: In disable.ts line 51, an error is thrown but wrapped inconsistently compared to other endpoints.

---

Medium Priority Issues

4. Backup Codes Displayed Before 2FA Verification

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

Flow Issue: Backup codes are set in state and download URL created BEFORE the user verifies they can actually use their 2FA (at step 103). If the user fails to verify or abandons setup, these codes are leaked to browser memory/state unnecessarily.

Recommendation: Only generate and display backup codes AFTER successful TOTP verification (line 135), not during setup.

5. Race Condition in Backup Code Usage

File: packages/features/auth/lib/next-auth-options.ts:149-156

If a user submits the same backup code twice rapidly (before the first request completes), both requests could succeed because the code isn't deleted until after authentication.

Recommendation: Use database transactions or optimistic locking to prevent race conditions.

6. Memory Leak in Backup Codes URL

File: apps/web/components/settings/EnableTwoFactorModal.tsx:98-99

if (backupCodesUrl) URL.revokeObjectURL(backupCodesUrl);

Good practice, but this only revokes on new setup. The URL should also be revoked when the component unmounts.

Recommendation: Add cleanup in useEffect return function.

---

Code Quality Issues

7. Hardcoded Magic Numbers

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

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

The number 10 (backup codes) and 5 (bytes) should be constants with descriptive names.

8. Missing Input Sanitization

File: Multiple locations

Backup codes use .replaceAll("-", "") but don't validate the result is exactly 10 alphanumeric characters. Malformed input should be rejected earlier.

9. Inconsistent Component Props

File: apps/web/components/auth/BackupCode.tsx:7

The center prop defaults to true but the component is often called with center={false}. Consider if the default should match most common usage.

10. Missing TypeScript Types

File: apps/web/components/settings/EnableTwoFactorModal.tsx:64

const [backupCodes, setBackupCodes] = useState([]);

Should be useState<string[]>([]) for proper type safety.

---

Documentation & Testing

11. Incomplete Test Coverage

File: apps/web/playwright/login.2fa.e2e.ts:12

The comment explicitly states:

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

Missing test cases:

• Login with backup code
• Disable 2FA using backup code
• Backup code reuse prevention
• Race condition testing
• Invalid backup code formats

12. Missing JSDoc Comments

The new BackupCode.tsx component and backup code utility functions lack documentation explaining their purpose and usage.

---

Minor Issues

13. UI/UX Concerns

File: apps/web/components/settings/EnableTwoFactorModal.tsx:200-207

Backup codes are displayed in a grid without visual indicators of which ones have been used (though they can't be reused in this implementation, the UI doesn't communicate this).

14. Translation Keys

File: apps/web/public/static/locales/en/common.json

Good job adding all necessary translation keys! However, backup_code_instructions could be more specific about:

• Codes are single-use
• Users should store them securely
• They should generate new codes if lost

15. Accessibility

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

The TextField component should have an aria-label or aria-describedby linking to the instructions for better screen reader support.

---

Positive Aspects ✓

1. Good use of encryption for storing backup codes
2. Proper password verification before enabling 2FA
3. Download functionality for backup codes is user-friendly
4. Multi-step modal flow is well-structured
5. Error handling covers most edge cases
6. Database migration is simple and correct

---

Summary

This PR implements a valuable security feature but has one critical security vulnerability (backup code deletion) that must be fixed before merging. The implementation is otherwise solid with good UX considerations, though it would benefit from improved test coverage and fixing the identified medium-priority issues.

Priority Fixes Required:

1. Fix backup code deletion (use splice instead of setting to null) ← CRITICAL
2. Add comprehensive test coverage for backup code flows
3. Move backup code generation to after TOTP verification
4. Add input validation for backup code format

Recommended Before Merge:

• Fix race condition vulnerability
• Add proper TypeScript types
• Clean up memory leaks
• Improve error handling consistency

---

[github-actions]

This PR is being marked as stale due to inactivity.