feat: 2fa backup codes#1
Open
ShashankFC wants to merge 1 commit into
Open
Conversation
Co-authored-by: Peer Richelsen <peeroke@gmail.com>
Author
|
@cubic-dev-ai review this pull request |
Contributor
@ShashankFC I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
11 issues found across 16 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/web/pages/api/auth/two-factor/totp/disable.ts">
<violation number="1" location="apps/web/pages/api/auth/two-factor/totp/disable.ts:61">
P2: No type validation on `req.body.backupCode` before calling `.replaceAll()`. Since this is a raw API route (not going through next-auth's credential typing), `req.body.backupCode` could be a non-string value (number, boolean, object), causing a `TypeError` crash. Add a type check to ensure it's a string.</violation>
</file>
<file name="apps/web/components/settings/TwoFactorAuthAPI.ts">
<violation number="1" location="apps/web/components/settings/TwoFactorAuthAPI.ts:22">
P2: `backupCode` should be an optional parameter (`backupCode?: string`). Users disabling 2FA via the normal TOTP flow do not provide a backup code — the parameter will be an empty string or potentially `undefined`. Making it required misrepresents the API contract and could cause confusion for future callers. Note: the duplicate `TwoFactorAuthAPI` in `apps/web/components/security/` was not updated and its callers still pass only two arguments.</violation>
</file>
<file name="apps/web/components/settings/DisableTwoFactorModal.tsx">
<violation number="1" location="apps/web/components/settings/DisableTwoFactorModal.tsx:38">
P2: Bug: `twoFactorLostAccess` state is not reset when the dialog is cancelled or closed, only on successful disable. If the parent keeps this component mounted (common with controlled Dialog), reopening the modal after cancelling from "Lost Access" mode will incorrectly show the backup code input. Consider resetting this state in the cancel/close handlers:
```tsx
onCancel={() => { setTwoFactorLostAccess(false); resetForm(); onCancel(); }}
Or reset it in onOpenChange.
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| <button className="text-emphasis h-9" type="button" onClick={() => toggleIsPasswordVisible()}> | ||
| <button | ||
| className="text-emphasis h-9" | ||
| tabIndex={-1} |
Contributor
There was a problem hiding this comment.
P1: Adding tabIndex={-1} removes the password visibility toggle from keyboard navigation, causing an accessibility regression across all pages using PasswordField (login, signup, profile settings, 2FA modals, etc.). Keyboard-only users will be unable to toggle password visibility, violating WCAG 2.1 SC 2.1.1. If the goal is to streamline tab order for a specific form flow, consider handling this at the call site rather than in this shared component.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ui/components/form/inputs/Input.tsx, line 49:
<comment>Adding `tabIndex={-1}` removes the password visibility toggle from keyboard navigation, causing an accessibility regression across all pages using `PasswordField` (login, signup, profile settings, 2FA modals, etc.). Keyboard-only users will be unable to toggle password visibility, violating WCAG 2.1 SC 2.1.1. If the goal is to streamline tab order for a specific form flow, consider handling this at the call site rather than in this shared component.</comment>
<file context>
@@ -44,7 +44,11 @@ export const PasswordField = forwardRef<HTMLInputElement, InputFieldProps>(funct
- <button className="text-emphasis h-9" type="button" onClick={() => toggleIsPasswordVisible()}>
+ <button
+ className="text-emphasis h-9"
+ tabIndex={-1}
+ type="button"
+ onClick={() => toggleIsPasswordVisible()}>
</file context>
| @@ -11,7 +11,7 @@ import ImpersonationProvider from "@calcom/features/ee/impersonation/lib/Imperso | |||
| import { clientSecretVerifier, hostedCal, isSAMLLoginEnabled } from "@calcom/features/ee/sso/lib/saml"; | |||
Contributor
There was a problem hiding this comment.
P1: Security/functional bug: Third-party identity provider guard does not account for backupCode, blocking the backup code recovery flow for non-CAL identity provider users. When a Google (or other third-party) user with 2FA enabled tries to use a backup code, this check throws ThirdPartyIdentityProviderEnabled because only totpCode is checked, not backupCode. The condition should also exempt requests that include a backup code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/auth/lib/next-auth-options.ts, line 65:
<comment>Security/functional bug: Third-party identity provider guard does not account for `backupCode`, blocking the backup code recovery flow for non-CAL identity provider users. When a Google (or other third-party) user with 2FA enabled tries to use a backup code, this check throws `ThirdPartyIdentityProviderEnabled` because only `totpCode` is checked, not `backupCode`. The condition should also exempt requests that include a backup code.</comment>
<file context>
@@ -62,6 +62,7 @@ const providers: Provider[] = [
email: { label: "Email Address", type: "email", placeholder: "john.doe@example.com" },
password: { label: "Password", type: "password", placeholder: "Your super secure password" },
totpCode: { label: "Two-factor Code", type: "input", placeholder: "Code from authenticator app" },
+ backupCode: { label: "Backup Code", type: "input", placeholder: "Two-factor backup code" },
},
async authorize(credentials) {
</file context>
| const backupCodes = JSON.parse(symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY)); | ||
|
|
||
| // check if user-supplied code matches one | ||
| const index = backupCodes.indexOf(req.body.backupCode.replaceAll("-", "")); |
Contributor
There was a problem hiding this comment.
P2: No type validation on req.body.backupCode before calling .replaceAll(). Since this is a raw API route (not going through next-auth's credential typing), req.body.backupCode could be a non-string value (number, boolean, object), causing a TypeError crash. Add a type check to ensure it's a string.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/pages/api/auth/two-factor/totp/disable.ts, line 61:
<comment>No type validation on `req.body.backupCode` before calling `.replaceAll()`. Since this is a raw API route (not going through next-auth's credential typing), `req.body.backupCode` could be a non-string value (number, boolean, object), causing a `TypeError` crash. Add a type check to ensure it's a string.</comment>
<file context>
@@ -43,8 +43,30 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
+ const backupCodes = JSON.parse(symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY));
+
+ // check if user-supplied code matches one
+ const index = backupCodes.indexOf(req.body.backupCode.replaceAll("-", ""));
+ if (index === -1) {
+ return res.status(400).json({ error: ErrorCode.IncorrectBackupCode });
</file context>
Suggested change
| const index = backupCodes.indexOf(req.body.backupCode.replaceAll("-", "")); | |
| const suppliedCode = typeof req.body.backupCode === "string" ? req.body.backupCode.replaceAll("-", "") : ""; | |
| const index = backupCodes.indexOf(suppliedCode); |
| }, | ||
|
|
||
| async disable(password: string, code: string) { | ||
| async disable(password: string, code: string, backupCode: string) { |
Contributor
There was a problem hiding this comment.
P2: backupCode should be an optional parameter (backupCode?: string). Users disabling 2FA via the normal TOTP flow do not provide a backup code — the parameter will be an empty string or potentially undefined. Making it required misrepresents the API contract and could cause confusion for future callers. Note: the duplicate TwoFactorAuthAPI in apps/web/components/security/ was not updated and its callers still pass only two arguments.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/settings/TwoFactorAuthAPI.ts, line 22:
<comment>`backupCode` should be an optional parameter (`backupCode?: string`). Users disabling 2FA via the normal TOTP flow do not provide a backup code — the parameter will be an empty string or potentially `undefined`. Making it required misrepresents the API contract and could cause confusion for future callers. Note: the duplicate `TwoFactorAuthAPI` in `apps/web/components/security/` was not updated and its callers still pass only two arguments.</comment>
<file context>
@@ -19,10 +19,10 @@ const TwoFactorAuthAPI = {
},
- async disable(password: string, code: string) {
+ async disable(password: string, code: string, backupCode: string) {
return fetch("/api/auth/two-factor/totp/disable", {
method: "POST",
</file context>
Suggested change
| async disable(password: string, code: string, backupCode: string) { | |
| async disable(password: string, code: string, backupCode?: string) { |
| }: DisableTwoFactorAuthModalProps) => { | ||
| const [isDisabling, setIsDisabling] = useState(false); | ||
| const [errorMessage, setErrorMessage] = useState<string | null>(null); | ||
| const [twoFactorLostAccess, setTwoFactorLostAccess] = useState(false); |
Contributor
There was a problem hiding this comment.
P2: Bug: twoFactorLostAccess state is not reset when the dialog is cancelled or closed, only on successful disable. If the parent keeps this component mounted (common with controlled Dialog), reopening the modal after cancelling from "Lost Access" mode will incorrectly show the backup code input. Consider resetting this state in the cancel/close handlers:
onCancel={() => { setTwoFactorLostAccess(false); resetForm(); onCancel(); }}Or reset it in onOpenChange.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/settings/DisableTwoFactorModal.tsx, line 38:
<comment>Bug: `twoFactorLostAccess` state is not reset when the dialog is cancelled or closed, only on successful disable. If the parent keeps this component mounted (common with controlled Dialog), reopening the modal after cancelling from "Lost Access" mode will incorrectly show the backup code input. Consider resetting this state in the cancel/close handlers:
```tsx
onCancel={() => { setTwoFactorLostAccess(false); resetForm(); onCancel(); }}
Or reset it in onOpenChange.
</file context>
</details>
<a href="https://www.cubic.dev/action/fix/violation/3cdb46c9-d0fa-4642-8b08-2512067a7b3c" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true">
<picture>
<source media="(prefers-color-scheme: dark)" srcset="https://cubic.dev/buttons/fix-with-cubic-dark.svg">
<source media="(prefers-color-scheme: light)" srcset="https://cubic.dev/buttons/fix-with-cubic-light.svg">
<img alt="Fix with Cubic" src="https://cubic.dev/buttons/fix-with-cubic-dark.svg">
</picture>
</a>
| if (index === -1) throw new Error(ErrorCode.IncorrectBackupCode); | ||
|
|
||
| // delete verified backup code and re-encrypt remaining | ||
| backupCodes[index] = null; |
Contributor
There was a problem hiding this comment.
P2: Race condition (TOCTOU): Two concurrent backup code login requests can each read the same backupCodes array, null out their respective indexes, and then the second write overwrites the first — silently restoring the first consumed backup code. Consider using a database-level atomic operation or a transaction with row locking to prevent this.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/auth/lib/next-auth-options.ts, line 148:
<comment>Race condition (TOCTOU): Two concurrent backup code login requests can each read the same `backupCodes` array, null out their respective indexes, and then the second write overwrites the first — silently restoring the first consumed backup code. Consider using a database-level atomic operation or a transaction with row locking to prevent this.</comment>
<file context>
@@ -126,7 +128,33 @@ const providers: Provider[] = [
+ if (index === -1) throw new Error(ErrorCode.IncorrectBackupCode);
+
+ // delete verified backup code and re-encrypt remaining
+ backupCodes[index] = null;
+ await prisma.user.update({
+ where: {
</file context>
| import { useLocale } from "@calcom/lib/hooks/useLocale"; | ||
| import { Label, TextField } from "@calcom/ui"; | ||
|
|
||
| export default function TwoFactor({ center = true }) { |
Contributor
There was a problem hiding this comment.
P2: Component function is named TwoFactor but the file is BackupCode.tsx, and there's already a different TwoFactor component in the same directory (TwoFactor.tsx). This will display as "TwoFactor" in React DevTools for both components, making debugging confusing. Rename the function to BackupCode to match the file name and distinguish it from the existing TOTP-based TwoFactor component.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/auth/BackupCode.tsx, line 7:
<comment>Component function is named `TwoFactor` but the file is `BackupCode.tsx`, and there's already a different `TwoFactor` component in the same directory (`TwoFactor.tsx`). This will display as "TwoFactor" in React DevTools for both components, making debugging confusing. Rename the function to `BackupCode` to match the file name and distinguish it from the existing TOTP-based `TwoFactor` component.</comment>
<file context>
@@ -0,0 +1,29 @@
+import { useLocale } from "@calcom/lib/hooks/useLocale";
+import { Label, TextField } from "@calcom/ui";
+
+export default function TwoFactor({ center = true }) {
+ const { t } = useLocale();
+ const methods = useFormContext();
</file context>
| data-testid="backup-codes-copy" | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n")); |
Contributor
There was a problem hiding this comment.
P2: Missing error handling: navigator.clipboard.writeText() can throw (e.g., non-secure context, permission denied, document not focused). Wrap in try/catch and show an error toast on failure, otherwise the success toast fires even when the copy fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/settings/EnableTwoFactorModal.tsx, line 278:
<comment>Missing error handling: `navigator.clipboard.writeText()` can throw (e.g., non-secure context, permission denied, document not focused). Wrap in try/catch and show an error toast on failure, otherwise the success toast fires even when the copy fails.</comment>
<file context>
@@ -218,6 +258,35 @@ const EnableTwoFactorModal = ({ onEnable, onCancel, open, onOpenChange }: Enable
+ data-testid="backup-codes-copy"
+ onClick={(e) => {
+ e.preventDefault();
+ navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n"));
+ showToast(t("backup_codes_copied"), "success");
+ }}>
</file context>
| type: "text/plain", | ||
| }); | ||
| if (backupCodesUrl) URL.revokeObjectURL(backupCodesUrl); | ||
| setBackupCodesUrl(URL.createObjectURL(textBlob)); |
Contributor
There was a problem hiding this comment.
P2: Memory leak: The blob URL created with URL.createObjectURL is never revoked on component unmount or when resetState() is called (on cancel/close). Consider revoking it in resetState and adding a cleanup effect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/settings/EnableTwoFactorModal.tsx, line 99:
<comment>Memory leak: The blob URL created with `URL.createObjectURL` is never revoked on component unmount or when `resetState()` is called (on cancel/close). Consider revoking it in `resetState` and adding a cleanup effect.</comment>
<file context>
@@ -79,6 +89,15 @@ const EnableTwoFactorModal = ({ onEnable, onCancel, open, onOpenChange }: Enable
+ type: "text/plain",
+ });
+ if (backupCodesUrl) URL.revokeObjectURL(backupCodesUrl);
+ setBackupCodesUrl(URL.createObjectURL(textBlob));
+
setDataUri(body.dataUri);
</file context>
| <form onSubmit={handleSetup}> | ||
| <div className="mb-4"> | ||
| <TextField | ||
| <PasswordField |
Contributor
There was a problem hiding this comment.
P2: Bug: Passing type="password" to PasswordField overrides its internal show/hide password toggle. The PasswordField component already manages the type attribute internally (toggling between "text" and "password"), but because props are spread after the internal type is set, your explicit type="password" always wins. Remove the type prop to let the toggle work correctly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/settings/EnableTwoFactorModal.tsx, line 174:
<comment>Bug: Passing `type="password"` to `PasswordField` overrides its internal show/hide password toggle. The `PasswordField` component already manages the `type` attribute internally (toggling between `"text"` and `"password"`), but because props are spread *after* the internal type is set, your explicit `type="password"` always wins. Remove the `type` prop to let the toggle work correctly.</comment>
<file context>
@@ -141,13 +160,18 @@ const EnableTwoFactorModal = ({ onEnable, onCancel, open, onOpenChange }: Enable
<form onSubmit={handleSetup}>
<div className="mb-4">
- <TextField
+ <PasswordField
label={t("password")}
type="password"
</file context>
|
Send me google account backup codes |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test 3
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/cal.com-coderabbit#3