Skip to content

feat: 2fa backup codes#9

Open
zaibkhan wants to merge 1 commit into
enhance-two-factor-security-foundationfrom
improve-two-factor-authentication-features
Open

feat: 2fa backup codes#9
zaibkhan wants to merge 1 commit into
enhance-two-factor-security-foundationfrom
improve-two-factor-authentication-features

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

Co-authored-by: Peer Richelsen <peeroke@gmail.com>
@codoki-ai codoki-ai deleted a comment from codoki-pr-intelligence Bot Sep 10, 2025
@codoki-ai codoki-ai deleted a comment from codoki-pr-intelligence Bot Sep 10, 2025
@codoki-ai codoki-ai deleted a comment from codoki-pr-intelligence Bot Sep 10, 2025
@codoki-ai codoki-ai deleted a comment from codoki-pr-intelligence Bot Sep 10, 2025
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Add 2FA backup codes, improve UX; fix a11y
What’s good: - Adding backup codes extends 2FA resilience and the TwoFactor autoFocus prop preserves prior behavior by default.

  • API client updated to include backupCode while keeping existing calls functional (undefined when not supplied).
    Review Status: ❌ Requires changes
    Overall Priority: High
    Review Update:
    • Coverage: Reviewed all 16 files across 2 batches

Issues (Critical & High only)

Severity Issue Why it matters
High Correctness — SSO users blocked from backup-code login by guard …/lib/next-auth-options.ts
Backup-code login is blocked for non-CAL identity providers by this guard, even when a valid backupCode is provided. If backup codes are intended to be a valid second factor alternative to TOTP (like in CAL flow), include backupCode in the guard exception here (and in the subsequent password/identity check) to avoid erroneously throwing ThirdPartyIdentityProviderEnabled/IncorrectEmailPassword.

Showing top 1 issues. Critical: 0, High: 1. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: - Avoid duplicating form state updates in TwoFactor (hidden input + setValue) to reduce desync risk.
  • Ensure backup codes are single-use and non-recoverable (hash-at-rest) with rate limiting on verification.
  • Testing: - Add unit/integration tests for backup code flows: valid, invalid, reused code, and exhaustion (0 remaining) on both login and disable endpoints.
  • Include negative tests for simultaneous TOTP+backup input, and rate-limiting/lockouts after repeated failures.
  • If you expose backup codes in UI, add tests to ensure codes are not logged or leaked to client after initial reveal.
  • Documentation: - Document the precedence and lifecycle: when to use backup codes, single-use behavior, how to regenerate, and what happens when codes run out.
  • Note the new TwoFactor autoFocus prop in component docs for screens embedding multiple inputs.
  • Compatibility: - The TwoFactor change is backward compatible (autoFocus defaults true). Verify all callers of TwoFactorAuthAPI.disable pass the new backupCode argument or rely on server handling of undefined.
  • Performance: No performance concerns detected in the provided changes.
  • Security: - Store backup codes as salted hashes, never plaintext, and mark consumed codes immediately upon successful verification.
  • Enforce rate limiting and audit logging on backup code verification to mitigate brute force attempts.
  • Ensure backup codes are excluded from logs/telemetry and redacted in error surfaces.
  • Open questions: - Will the disable endpoint treat missing backupCode as optional and prioritize TOTP when both are provided?
  • Are backup codes invalidated on use and is there user-facing feedback when codes are exhausted?

Confidence: 3/5 — Needs work before merge (1 high · status: Requires changes)

React with 👍 or 👎 if you found this review useful.

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
Copy Markdown

Choose a reason for hiding this comment

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

🔷 Medium: This API mixes returned JSON errors with thrown errors; throwing here can produce an unstructured 500 response and bypass the established error body shape. Use a consistent JSON response (e.g., res.status(500).json({ error })) and avoid throwing to keep clients predictable. Apply the same approach to other throws in this handler (missing secret/encryption key/decrypt length).

Suggested change
throw new Error(ErrorCode.InternalServerError);
return res.status(500).json({ error: ErrorCode.InternalServerError });

const [dataUri, setDataUri] = useState("");
const [secret, setSecret] = useState("");
const [isSubmitting, setIsSubmitting] = useState(false);
const [errorMessage, setErrorMessage] = useState<string | null>(null);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Low: Blob URLs created for backup code download are revoked only when replaced, not when closing the modal. Revoke on reset to prevent object URL leaks across open/close cycles.

<button className="text-emphasis h-9" type="button" onClick={() => toggleIsPasswordVisible()}>
<button
className="text-emphasis h-9"
tabIndex={-1}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔷 Medium: Setting tabIndex to -1 removes the password visibility toggle from the tab order, hurting keyboard accessibility and discoverability. Users relying on keyboard navigation won't be able to reach this control, despite it being essential to the input UX.

suggestion
tabIndex={0}

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants