-
Notifications
You must be signed in to change notification settings - Fork 4
feat: 2fa backup codes #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: enhance-two-factor-security-foundation
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,29 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import React from "react"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useFormContext } from "react-hook-form"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| 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(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| <div className={center ? "mx-auto !mt-0 max-w-sm" : "!mt-0 max-w-sm"}> | ||||||||||||||||||||||||||||||||||||||||||||||||||
| <Label className="mt-4">{t("backup_code")}</Label> | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| <p className="text-subtle mb-4 text-sm">{t("backup_code_instructions")}</p> | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add proper accessibility connection. The instructions should be properly connected to the input field for screen readers. - <p className="text-subtle mb-4 text-sm">{t("backup_code_instructions")}</p>
+ <p id="backup-code-instructions" className="text-subtle mb-4 text-sm">
+ {t("backup_code_instructions")}
+ </p>📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| <TextField | ||||||||||||||||||||||||||||||||||||||||||||||||||
| id="backup-code" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| label="" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| defaultValue="" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| placeholder="XXXXX-XXXXX" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| minLength={10} // without dash | ||||||||||||||||||||||||||||||||||||||||||||||||||
| maxLength={11} // with dash | ||||||||||||||||||||||||||||||||||||||||||||||||||
| required | ||||||||||||||||||||||||||||||||||||||||||||||||||
| {...methods.register("backupCode")} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+17
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance input validation and accessibility. The current validation only checks length but doesn't validate the backup code format. Also missing accessibility attributes. <TextField
id="backup-code"
label=""
+ aria-label={t("backup_code")}
+ aria-describedby="backup-code-instructions"
defaultValue=""
placeholder="XXXXX-XXXXX"
minLength={10} // without dash
maxLength={11} // with dash
required
+ pattern="[a-fA-F0-9]{5}-?[a-fA-F0-9]{5}"
+ title={t("backup_code_format_hint")}
{...methods.register("backupCode")}
/>Add corresponding localization key for format hint. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,10 +19,10 @@ const TwoFactorAuthAPI = { | |
| }); | ||
| }, | ||
|
|
||
| async disable(password: string, code: string) { | ||
| async disable(password: string, code: string, backupCode: string) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider parameter validation and mutual exclusivity. The method signature accepts both -async disable(password: string, code: string, backupCode: string) {
+async disable(password: string, code?: string, backupCode?: string) {And add validation to ensure exactly one authentication method is provided: if ((!code && !backupCode) || (code && backupCode)) {
throw new Error("Provide either TOTP code or backup code, not both");
}🤖 Prompt for AI Agents |
||
| return fetch("/api/auth/two-factor/totp/disable", { | ||
| method: "POST", | ||
| body: JSON.stringify({ password, code }), | ||
| body: JSON.stringify({ password, code, backupCode }), | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,4 @@ | ||||||||||
| import crypto from "crypto"; | ||||||||||
| import type { NextApiRequest, NextApiResponse } from "next"; | ||||||||||
| import { authenticator } from "otplib"; | ||||||||||
| import qrcode from "qrcode"; | ||||||||||
|
|
@@ -56,11 +57,15 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse) | |||||||||
| // bytes without updating the sanity checks in the enable and login endpoints. | ||||||||||
| const secret = authenticator.generateSecret(20); | ||||||||||
|
|
||||||||||
| // generate backup codes with 10 character length | ||||||||||
| const backupCodes = Array.from(Array(10), () => crypto.randomBytes(5).toString("hex")); | ||||||||||
|
|
||||||||||
| await prisma.user.update({ | ||||||||||
| where: { | ||||||||||
| id: session.user.id, | ||||||||||
| }, | ||||||||||
| data: { | ||||||||||
| backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY), | ||||||||||
| twoFactorEnabled: false, | ||||||||||
| twoFactorSecret: symmetricEncrypt(secret, process.env.CALENDSO_ENCRYPTION_KEY), | ||||||||||
| }, | ||||||||||
|
|
@@ -70,5 +75,5 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse) | |||||||||
| const keyUri = authenticator.keyuri(name, "Cal", secret); | ||||||||||
| const dataUri = await qrcode.toDataURL(keyUri); | ||||||||||
|
|
||||||||||
| return res.json({ secret, keyUri, dataUri }); | ||||||||||
| return res.json({ secret, keyUri, dataUri, backupCodes }); | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Security concern: Backup codes returned in plaintext. While necessary for user display, returning backup codes in the API response creates a brief window where they exist in plaintext. Ensure the frontend handles these securely and doesn't log them. Consider adding a security comment to remind developers: - return res.json({ secret, keyUri, dataUri, backupCodes });
+ // SECURITY: Backup codes are returned in plaintext for user display
+ // Frontend must handle securely and avoid logging
+ return res.json({ secret, keyUri, dataUri, backupCodes });📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| } | ||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve component naming and type safety.
The component is named
TwoFactorbut handles backup codes specifically. Thecenterprop lacks type definition.Also update the filename to match:
BackupCode.tsx→ component name should beBackupCode.📝 Committable suggestion
🤖 Prompt for AI Agents