Skip to content

feat: 2fa backup codes #9

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.

Sign up for GitHub

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

Open
Frawatson wants to merge 1 commit into
base: enhance-two-factor-security-foundation
Choose a base branch
from
Open

feat: 2fa backup codes #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions apps/web/components/auth/BackupCode.tsx
View file
Open in desktop
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 }) {

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Module boundaries / Component naming (confidence: 90%)

The BackupCode component's default export is named TwoFactor, which is identical to the default export name from TwoFactor.tsx in the same directory. This creates confusion for developers, makes stack traces ambiguous, and could lead to import errors if both are used in the same file.

Evidence:

  • Line 7: export default function TwoFactor({ center = true }) — same function name as in TwoFactor.tsx line 8
  • Both components are imported in login.tsx and DisableTwoFactorModal.tsx in the same file scope

Agent: architecture

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>

<TextField
id="backup-code"
label=""
defaultValue=""
placeholder="XXXXX-XXXXX"
minLength={10} // without dash
maxLength={11} // with dash
required
{...methods.register("backupCode")}
/>
</div>
);
}
4 changes: 2 additions & 2 deletions apps/web/components/auth/TwoFactor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { useFormContext } from "react-hook-form";
import { useLocale } from "@calcom/lib/hooks/useLocale";
import { Label, Input } from "@calcom/ui";

export default function TwoFactor({ center = true }) {
export default function TwoFactor({ center = true, autoFocus = true }) {
const [value, onChange] = useState("");
const { t } = useLocale();
const methods = useFormContext();
Expand Down Expand Up @@ -40,7 +40,7 @@ export default function TwoFactor({ center = true }) {
name={`2fa${index + 1}`}
inputMode="decimal"
{...digit}
autoFocus={index === 0}
autoFocus={autoFocus && index === 0}
autoComplete="one-time-code"
/>
))}
Expand Down
42 changes: 35 additions & 7 deletions apps/web/components/settings/DisableTwoFactorModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { ErrorCode } from "@calcom/features/auth/lib/ErrorCode";
import { useLocale } from "@calcom/lib/hooks/useLocale";
import { Button, Dialog, DialogContent, DialogFooter, Form, PasswordField } from "@calcom/ui";

import BackupCode from "@components/auth/BackupCode";
import TwoFactor from "@components/auth/TwoFactor";

import TwoFactorAuthAPI from "./TwoFactorAuthAPI";
Expand All @@ -20,6 +21,7 @@ interface DisableTwoFactorAuthModalProps {
}

interface DisableTwoFactorValues {
backupCode: string;
totpCode: string;
password: string;
}
Expand All @@ -33,33 +35,45 @@ const DisableTwoFactorAuthModal = ({
}: DisableTwoFactorAuthModalProps) => {
const [isDisabling, setIsDisabling] = useState(false);
const [errorMessage, setErrorMessage] = useState<string | null>(null);
const [twoFactorLostAccess, setTwoFactorLostAccess] = useState(false);

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — State persistence across modal opens (confidence: 93%)

The twoFactorLostAccess state is reset on successful disable (line 63), but if the user closes the modal via the cancel button without success, the state is not reset. Next time the modal opens, it may show the backup code input instead of the TOTP input. The resetForm function exists but is not called from onCancel.

Evidence:

  • Line 38: const [twoFactorLostAccess, setTwoFactorLostAccess] = useState(false)
  • Line 63: setTwoFactorLostAccess(false) — only in success path
  • Cancel button on line ~105 calls onCancel() directly without resetting twoFactorLostAccess
  • Intent edge case: 'if the modal is closed without success, the state persists and the backup code field may reappear unexpectedly on next open'

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — logic agent (Small fix (10 lines, 1 file))

The twoFactorLostAccess state is reset on successful disable (line 63), but if the user closes the modal via the cancel button without success, the state is not reset. Next time the modal opens, it may show the backup code input instead of the TOTP input. The resetForm function exists but is not called from onCancel.

Suggested change
const [twoFactorLostAccess, setTwoFactorLostAccess] = useState(false);
<Button
color="secondary"
onClick={() => {
setTwoFactorLostAccess(false);
resetForm();
onCancel();
}}>

🤖 Grapple PR auto-fix • minor • confidence: 93%

const { t } = useLocale();

const form = useForm<DisableTwoFactorValues>();

async function handleDisable({ totpCode, password }: DisableTwoFactorValues) {
const resetForm = (clearPassword = true) => {
if (clearPassword) form.setValue("password", "");
form.setValue("backupCode", "");
form.setValue("totpCode", "");
setErrorMessage(null);
};

async function handleDisable({ password, totpCode, backupCode }: DisableTwoFactorValues) {
if (isDisabling) {
return;
}
setIsDisabling(true);
setErrorMessage(null);

try {
const response = await TwoFactorAuthAPI.disable(password, totpCode);
const response = await TwoFactorAuthAPI.disable(password, totpCode, backupCode);
if (response.status === 200) {
setTwoFactorLostAccess(false);
resetForm();
onDisable();
return;
}

const body = await response.json();
if (body.error === ErrorCode.IncorrectPassword) {
setErrorMessage(t("incorrect_password"));
}
if (body.error === ErrorCode.SecondFactorRequired) {
} else if (body.error === ErrorCode.SecondFactorRequired) {
setErrorMessage(t("2fa_required"));
}
if (body.error === ErrorCode.IncorrectTwoFactorCode) {
} else if (body.error === ErrorCode.IncorrectTwoFactorCode) {
setErrorMessage(t("incorrect_2fa"));
} else if (body.error === ErrorCode.IncorrectBackupCode) {
setErrorMessage(t("incorrect_backup_code"));
} else if (body.error === ErrorCode.MissingBackupCodes) {
setErrorMessage(t("missing_backup_codes"));
} else {
setErrorMessage(t("something_went_wrong"));
}
Expand All @@ -78,19 +92,33 @@ const DisableTwoFactorAuthModal = ({
<div className="mb-8">
{!disablePassword && (
<PasswordField
required
labelProps={{
className: "block text-sm font-medium text-default",
}}
{...form.register("password")}
className="border-default mt-1 block w-full rounded-md border px-3 py-2 text-sm focus:border-black focus:outline-none focus:ring-black"
/>
)}
<TwoFactor center={false} />
{twoFactorLostAccess ? (
<BackupCode center={false} />
) : (
<TwoFactor center={false} autoFocus={false} />
)}

{errorMessage && <p className="mt-1 text-sm text-red-700">{errorMessage}</p>}
</div>

<DialogFooter showDivider className="relative mt-5">
<Button
color="minimal"
className="mr-auto"
onClick={() => {
setTwoFactorLostAccess(!twoFactorLostAccess);
resetForm(false);
}}>
{twoFactorLostAccess ? t("go_back") : t("lost_access")}
</Button>
<Button color="secondary" onClick={onCancel}>
{t("cancel")}
</Button>
Expand Down
83 changes: 76 additions & 7 deletions apps/web/components/settings/EnableTwoFactorModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { useForm } from "react-hook-form";
import { ErrorCode } from "@calcom/features/auth/lib/ErrorCode";
import { useCallbackRef } from "@calcom/lib/hooks/useCallbackRef";
import { useLocale } from "@calcom/lib/hooks/useLocale";
import { Button, Dialog, DialogContent, DialogFooter, Form, TextField } from "@calcom/ui";
import { Button, Dialog, DialogContent, DialogFooter, Form, PasswordField, showToast } from "@calcom/ui";

import TwoFactor from "@components/auth/TwoFactor";

Expand All @@ -28,6 +28,7 @@ interface EnableTwoFactorModalProps {

enum SetupStep {
ConfirmPassword,
DisplayBackupCodes,
DisplayQrCode,

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Business Logic Correctness (confidence: 99%)

The SetupStep enum has DisplayBackupCodes (value 1) ordered BEFORE DisplayQrCode (value 2), but the actual flow goes ConfirmPassword → DisplayQrCode → EnterTotpCode → DisplayBackupCodes. While the enum values aren't used for ordering logic (the WithStep component just checks equality), the code in handleSetup sets step to SetupStep.DisplayQrCode and handleEnable sets step to SetupStep.DisplayBackupCodes, so the flow is correct. This is not a bug but could be confusing.

Evidence:

  • ConfirmPassword = 0, DisplayBackupCodes = 1, DisplayQrCode = 2, EnterTotpCode = 3
  • Actual flow: ConfirmPassword → DisplayQrCode → EnterTotpCode → DisplayBackupCodes
  • WithStep only checks step === current equality, so ordering doesn't matter functionally

Agent: logic

EnterTotpCode,
}

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — Intent alignment - Step ordering (confidence: 100%)

The SetupStep enum defines DisplayBackupCodes (value 1) before DisplayQrCode (value 2), but the intent specifies the flow order should be ConfirmPassword → DisplayQrCode → EnterTotpCode → DisplayBackupCodes. While the actual step transitions are controlled explicitly via setStep() calls and the enum numeric values don't drive the flow, this ordering is misleading and fragile. More importantly, the enum assigns DisplayBackupCodes=1 and DisplayQrCode=2, EnterTotpCode=3. The actual flow correctly goes ConfirmPassword(0) → DisplayQrCode(2) → EnterTotpCode(3) → DisplayBackupCodes(1) via explicit setStep calls, so the runtime behavior is correct. However, this is a maintenance hazard.

Evidence:

  • enum SetupStep { ConfirmPassword, DisplayBackupCodes, DisplayQrCode, EnterTotpCode }
  • Intent says: 'The EnableTwoFactorModal flow order is: ConfirmPassword → DisplayQrCode → EnterTotpCode → DisplayBackupCodes → onEnable callback'
  • handleSetup sets setStep(SetupStep.DisplayQrCode) on success (line 99)
  • handleEnable sets setStep(SetupStep.DisplayBackupCodes) on success (line 135)
  • The runtime flow is correct but the enum ordering is confusing

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))

The SetupStep enum defines DisplayBackupCodes (value 1) before DisplayQrCode (value 2), but the intent specifies the flow order should be ConfirmPassword → DisplayQrCode → EnterTotpCode → DisplayBackupCodes. While the actual step transitions are controlled explicitly via setStep() calls and the enum numeric values don't drive the flow, this ordering is misleading and fragile. More importantly, the enum assigns DisplayBackupCodes=1 and DisplayQrCode=2, EnterTotpCode=3. The actual flow correctly goes ConfirmPassword(0) → DisplayQrCode(2) → EnterTotpCode(3) → DisplayBackupCodes(1) via explicit setStep calls, so the runtime behavior is correct. However, this is a maintenance hazard.

Suggested change
}
DisplayBackupCodes,

🤖 Grapple PR auto-fix • critical • confidence: 100%

Expand All @@ -54,16 +55,25 @@ const EnableTwoFactorModal = ({ onEnable, onCancel, open, onOpenChange }: Enable

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Cross-module impact / State management (confidence: 93%)

The backupCodes state is initialized as an untyped empty array (useState([])), which TypeScript will infer as never[]. This will cause type errors when calling .map() on it with a string callback like formatBackupCode. It should be explicitly typed as string[].

Evidence:

  • Line 55: const [backupCodes, setBackupCodes] = useState([]); — inferred as never[]
  • Line 200: backupCodes.map((code) => ...) and line 93: body.backupCodes.map(formatBackupCode) expect string elements
  • Line 271: backupCodes.map(formatBackupCode) also expects strings

Agent: architecture

const setupDescriptions = {
[SetupStep.ConfirmPassword]: t("2fa_confirm_current_password"),
[SetupStep.DisplayBackupCodes]: t("backup_code_instructions"),
[SetupStep.DisplayQrCode]: t("2fa_scan_image_or_use_code"),
[SetupStep.EnterTotpCode]: t("2fa_enter_six_digit_code"),
};
const [step, setStep] = useState(SetupStep.ConfirmPassword);
const [password, setPassword] = useState("");

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Type Safety (confidence: 93%)

The backupCodes state is initialized with useState([]) without a type annotation, giving it the type never[]. This means setBackupCodes(body.backupCodes) will produce a TypeScript error since body.backupCodes is string[] but the state expects never[].

Evidence:

  • const [backupCodes, setBackupCodes] = useState([]) — TypeScript infers never[]
  • Later setBackupCodes(body.backupCodes) expects to set string[]
  • backupCodes.map(formatBackupCode) also expects string elements

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — TypeScript type safety - backupCodes state (confidence: 85%)

The backupCodes state is initialized as an empty array without a type annotation: useState([]). This infers as never[], which means TypeScript won't catch type errors when setting string values. It should be typed as useState([]).

Evidence:

  • Line 63: const [backupCodes, setBackupCodes] = useState([])
  • Later used as string array: backupCodes.map(formatBackupCode)
  • formatBackupCode expects a string parameter

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))

The backupCodes state is initialized as an empty array without a type annotation: useState([]). This infers as never[], which means TypeScript won't catch type errors when setting string values. It should be typed as useState<string[]>([]).

Suggested change
const [password, setPassword] = useState("");
const [backupCodes, setBackupCodes] = useState<string[]>([]);

🤖 Grapple PR auto-fix • minor • confidence: 85%

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

const resetState = () => {

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — resetState incomplete - missing backupCodes and backupCodesUrl cleanup (confidence: 85%)

The resetState function resets password, errorMessage, and step, but does not reset backupCodes, backupCodesUrl, dataUri, or secret. When the modal is cancelled and reopened, stale state from a previous setup attempt could persist. While the states would be overwritten on a new successful setup, if the component instance is reused (which it likely is since open/close is controlled by a prop), this could cause the previous backup codes to briefly flash on screen.

Evidence:

  • resetState only resets: setPassword(''), setErrorMessage(null), setStep(SetupStep.ConfirmPassword)
  • Does not reset: backupCodes, backupCodesUrl, dataUri, secret, isSubmitting
  • resetState is called on cancel (line 231) and close (line 267)

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — logic agent (Small fix (8 lines, 1 file))

The resetState function resets password, errorMessage, and step, but does not reset backupCodes, backupCodesUrl, dataUri, or secret. When the modal is cancelled and reopened, stale state from a previous setup attempt could persist. While the states would be overwritten on a new successful setup, if the component instance is reused (which it likely is since open/close is controlled by a prop), this could cause the previous backup codes to briefly flash on screen.

Suggested change
const resetState = () => {
setBackupCodes([]);
if (backupCodesUrl) {
URL.revokeObjectURL(backupCodesUrl);
}
setBackupCodesUrl("");
setDataUri("");
setSecret("");
setIsSubmitting(false);

🤖 Grapple PR auto-fix • minor • confidence: 85%

setPassword("");
setErrorMessage(null);

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — State Management (confidence: 93%)

The resetState function resets password, errorMessage, and step, but does not reset backupCodes, backupCodesUrl, dataUri, or secret. If the modal is cancelled and reopened, these stale values could persist. While the flow re-fetches them on setup, the stale backup codes could flash briefly.

Evidence:

  • resetState only resets password, errorMessage, and step
  • backupCodes, backupCodesUrl, dataUri, secret are not reset
  • Called from cancel button onClick and backup-codes-close onClick

Agent: logic

setStep(SetupStep.ConfirmPassword);
};

async function handleSetup(e: React.FormEvent) {
e.preventDefault();

Expand All @@ -79,6 +89,15 @@ const EnableTwoFactorModal = ({ onEnable, onCancel, open, onOpenChange }: Enable
const body = await response.json();

if (response.status === 200) {
setBackupCodes(body.backupCodes);

// create backup codes download url
const textBlob = new Blob([body.backupCodes.map(formatBackupCode).join("\n")], {
type: "text/plain",
});

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Memory leak - Blob URL not revoked on unmount (confidence: 91%)

The Blob URL created via URL.createObjectURL is only revoked if handleSetup is called again (when backupCodesUrl is truthy). If the modal unmounts without the user re-running setup, the Blob URL is never revoked, causing a memory leak. There is no useEffect cleanup that revokes the URL on unmount.

Evidence:

  • Line 96: if (backupCodesUrl) URL.revokeObjectURL(backupCodesUrl) — only called during re-setup
  • Line 97: setBackupCodesUrl(URL.createObjectURL(textBlob)) — creates new blob URL
  • No useEffect with cleanup function to revoke the URL on component unmount
  • Intent edge case: 'The backupCodesUrl blob URL is created with URL.createObjectURL but only revoked if backupCodesUrl is truthy on re-setup; if the modal unmounts without revoking, a memory leak occurs'

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — logic agent (Small fix (9 lines, 1 file))

The Blob URL created via URL.createObjectURL is only revoked if handleSetup is called again (when backupCodesUrl is truthy). If the modal unmounts without the user re-running setup, the Blob URL is never revoked, causing a memory leak. There is no useEffect cleanup that revokes the URL on unmount.

Suggested change
});
// Revoke the Blob URL on unmount to prevent memory leaks.
// The handleSetup function already revokes the previous URL before creating a new one,
// but if the modal unmounts without re-running setup, the URL would never be released.
useEffect(() => {
return () => {
if (backupCodesUrl) URL.revokeObjectURL(backupCodesUrl);
};
}, [backupCodesUrl]);

🤖 Grapple PR auto-fix • major • confidence: 91%

if (backupCodesUrl) URL.revokeObjectURL(backupCodesUrl);

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — Resource Leak / State Management (confidence: 73%)

The formatBackupCode function is defined at line 162 (after the handleSetup function), but it is used inside handleSetup at line 94 in the Blob creation (body.backupCodes.map(formatBackupCode)). Since handleSetup is defined before formatBackupCode in the component body, and formatBackupCode is declared with const (not hoisted like function declarations), when handleSetup runs, the closure captures formatBackupCode from the render scope. However, handleSetup is an async function defined in the same render scope, and by the time it executes (after awaiting the fetch), formatBackupCode IS defined in that scope because const declarations are in scope for the entire block (temporal dead zone only applies before the declaration line during synchronous execution of the block, but handleSetup is called later). Actually, re-examining: handleSetup is called from the form's onSubmit, which happens during a user action - by that time the component function has fully executed and formatBackupCode IS defined. The closure over formatBackupCode works correctly because the variable is accessed at call time, not definition time. This is actually fine due to JavaScript closures.

Evidence:

  • handleSetup defined around line 82, formatBackupCode defined at line 162, both in the same component function body
  • const formatBackupCode is a const arrow function - not hoisted
  • However, handleSetup is invoked via onSubmit (user action), which happens AFTER the component function has completed execution, so formatBackupCode is already initialized
  • backupCodesUrl is created via URL.createObjectURL at line 97
  • URL.revokeObjectURL is only called inside handleSetup if backupCodesUrl already exists (line 96)
  • No useEffect cleanup to revoke the URL on unmount
  • Intent spec edge case: 'if the component unmounts without revocation, the object URL leaks — verify cleanup on unmount'
  • Line 97: URL.createObjectURL(textBlob) creates a persistent reference
  • Line 96: if (backupCodesUrl) URL.revokeObjectURL(backupCodesUrl); only runs on re-invocation of handleSetup
  • No useEffect cleanup function exists to revoke the URL on unmount
  • The intent specification explicitly calls this out: 'if the component unmounts without revocation, the object URL leaks'

Agent: logic

setBackupCodesUrl(URL.createObjectURL(textBlob));

setDataUri(body.dataUri);
setSecret(body.secret);
setStep(SetupStep.DisplayQrCode);
Expand Down Expand Up @@ -113,7 +132,7 @@ const EnableTwoFactorModal = ({ onEnable, onCancel, open, onOpenChange }: Enable
const body = await response.json();

if (response.status === 200) {
onEnable();
setStep(SetupStep.DisplayBackupCodes);
return;
}

Expand Down Expand Up @@ -141,13 +160,18 @@ const EnableTwoFactorModal = ({ onEnable, onCancel, open, onOpenChange }: Enable
}
}, [form, handleEnableRef, totpCode]);

const formatBackupCode = (code: string) => `${code.slice(0, 5)}-${code.slice(5, 10)}`;

return (
<Dialog open={open} onOpenChange={onOpenChange}>
<DialogContent title={t("enable_2fa")} description={setupDescriptions[step]} type="creation">
<DialogContent
title={step === SetupStep.DisplayBackupCodes ? t("backup_codes") : t("enable_2fa")}
description={setupDescriptions[step]}
type="creation">
<WithStep step={SetupStep.ConfirmPassword} current={step}>
<form onSubmit={handleSetup}>
<div className="mb-4">
<TextField
<PasswordField
label={t("password")}

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — State Management (confidence: 100%)

When the modal's onOpenChange is triggered (e.g., user clicks outside the modal or presses Escape) while on the DisplayBackupCodes step, onEnable is never called. The modal closes but the parent component is never notified that 2FA was successfully enabled. This means the 2FA toggle/state in the parent won't update, even though 2FA was actually enabled on the server. The intent spec explicitly flags this edge case.

Evidence:

  • onEnable() is only called in the backup-codes-close button onClick handler (line 270)
  • If the user closes the modal via onOpenChange (clicking outside, pressing Escape), the Dialog closes without calling onEnable()
  • The TOTP code was already verified successfully at this point (step transitioned to DisplayBackupCodes)
  • Intent spec edge case: 'If the user closes the EnableTwoFactorModal on the DisplayBackupCodes step without clicking the close/done button, onEnable is never called'

Agent: logic

type="password"
name="password"
Expand All @@ -173,6 +197,15 @@ const EnableTwoFactorModal = ({ onEnable, onCancel, open, onOpenChange }: Enable
</p>
</>
</WithStep>
<WithStep step={SetupStep.DisplayBackupCodes} current={step}>
<>
<div className="mt-5 grid grid-cols-2 gap-1 text-center font-mono md:pl-10 md:pr-10">
{backupCodes.map((code) => (
<div key={code}>{formatBackupCode(code)}</div>
))}
</div>
</>
</WithStep>
<Form handleSubmit={handleEnable} form={form}>
<WithStep step={SetupStep.EnterTotpCode} current={step}>
<div className="-mt-4 pb-2">
Expand All @@ -186,9 +219,16 @@ const EnableTwoFactorModal = ({ onEnable, onCancel, open, onOpenChange }: Enable
</div>
</WithStep>
<DialogFooter className="mt-8" showDivider>
<Button color="secondary" onClick={onCancel}>
{t("cancel")}
</Button>
{step !== SetupStep.DisplayBackupCodes ? (
<Button
color="secondary"
onClick={() => {
onCancel();
resetState();
}}>
{t("cancel")}
</Button>
) : null}
<WithStep step={SetupStep.ConfirmPassword} current={step}>
<Button
type="submit"
Expand Down Expand Up @@ -218,6 +258,35 @@ const EnableTwoFactorModal = ({ onEnable, onCancel, open, onOpenChange }: Enable
{t("enable")}
</Button>
</WithStep>
<WithStep step={SetupStep.DisplayBackupCodes} current={step}>
<>
<Button
color="secondary"
data-testid="backup-codes-close"
onClick={(e) => {
e.preventDefault();
resetState();
onEnable();
}}>
{t("close")}
</Button>
<Button
color="secondary"
data-testid="backup-codes-copy"
onClick={(e) => {
e.preventDefault();
navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n"));
showToast(t("backup_codes_copied"), "success");
}}>
{t("copy")}
</Button>
<a download="cal-backup-codes.txt" href={backupCodesUrl}>
<Button color="primary" data-testid="backup-codes-download">
{t("download")}
</Button>
</a>
</>
</WithStep>
</DialogFooter>
</Form>
</DialogContent>
Expand Down
4 changes: 2 additions & 2 deletions apps/web/components/settings/TwoFactorAuthAPI.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
body: JSON.stringify({ password, code }),
body: JSON.stringify({ password, code, backupCode }),
headers: {
"Content-Type": "application/json",
},
Expand Down
27 changes: 25 additions & 2 deletions apps/web/pages/api/auth/two-factor/totp/disable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,30 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
return res.status(400).json({ error: ErrorCode.IncorrectPassword });
}
}
// if user has 2fa
if (user.twoFactorEnabled) {

// if user has 2fa and using backup code
if (user.twoFactorEnabled && req.body.backupCode) {
if (!process.env.CALENDSO_ENCRYPTION_KEY) {
console.error("Missing encryption key; cannot proceed with backup code login.");

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — Error Handling (confidence: 100%)

The encryption key guard throws a raw Error instead of returning a structured JSON error response. This will result in an unhandled 500 error with potentially leaked internal details, unlike every other error path in this handler which returns res.status(400).json(...). The intent specification explicitly calls this out as a risk area.

Evidence:

  • throw new Error(ErrorCode.InternalServerError) on line 50 instead of return res.status(500).json({ error: ErrorCode.InternalServerError })
  • All other error paths in this file use res.status(400).json({ error: ... })
  • The intent spec risk areas state: 'disable.ts: the encryption key guard throws a raw Error instead of returning a structured JSON error response, which could expose internal error details or cause an unhandled 500'

Agent: logic

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — API contract violation (confidence: 100%)

The disable endpoint throws a raw Error when CALENDSO_ENCRYPTION_KEY is missing, instead of returning a structured JSON error response. This will result in an unhandled 500 with potentially leaked internal error details, unlike every other error path in this handler which returns res.status(400).json({ error: ... }).

Evidence:

  • Line 50: throw new Error(ErrorCode.InternalServerError); — all other error paths in this file use return res.status(400).json({ error: ... })
  • The caller (DisableTwoFactorModal) expects a JSON body with an error field; a 500 with no JSON body will cause response.json() to throw in the client
  • The intent specification explicitly flags this: 'the encryption key guard throws a raw Error instead of returning a structured JSON error response'

Agent: architecture

throw new Error(ErrorCode.InternalServerError);
}

if (!user.backupCodes) {
return res.status(400).json({ error: ErrorCode.MissingBackupCodes });
}

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

// we delete all stored backup codes at the end, no need to do this here

// if user has 2fa and NOT using backup code, try totp
} else if (user.twoFactorEnabled) {
if (!req.body.code) {
return res.status(400).json({ error: ErrorCode.SecondFactorRequired });
// throw new Error(ErrorCode.SecondFactorRequired);
Expand Down Expand Up @@ -82,6 +104,7 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
id: session.user.id,
},
data: {
backupCodes: null,
twoFactorEnabled: false,
twoFactorSecret: null,
},
Expand Down
7 changes: 6 additions & 1 deletion apps/web/pages/api/auth/two-factor/totp/setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import crypto from "crypto";

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Linked Issue Mismatch (confidence: 81%)

The linked issue (#3) describes 'SMS workflow reminder retry count tracking' which is completely unrelated to the 2FA backup codes feature implemented in this PR. The PR cannot be considered as addressing the linked issue.

Evidence:

Agent: logic

import type { NextApiRequest, NextApiResponse } from "next";
import { authenticator } from "otplib";
import qrcode from "qrcode";
Expand Down Expand Up @@ -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),

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — Missing encryption key guard (confidence: 100%)

setup.ts calls symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY) without first checking if CALENDSO_ENCRYPTION_KEY is defined. If the env var is missing, this will pass undefined to symmetricEncrypt, which will either throw a cryptic error or produce corrupted data. Both disable.ts and next-auth-options.ts explicitly check for this key before proceeding, but setup.ts does not.

Evidence:

  • setup.ts line 68: backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY)
  • disable.ts line 49-52: explicit check for !process.env.CALENDSO_ENCRYPTION_KEY with error throw
  • next-auth-options.ts line 131-134: explicit check for !process.env.CALENDSO_ENCRYPTION_KEY with error throw
  • Intent edge case: 'If CALENDSO_ENCRYPTION_KEY is missing in setup.ts, symmetricEncrypt will silently fail or throw — there is no guard check in setup.ts unlike in disable.ts and next-auth-options.ts'

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — logic agent (Small fix (8 lines, 1 file))

setup.ts calls symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY) without first checking if CALENDSO_ENCRYPTION_KEY is defined. If the env var is missing, this will pass undefined to symmetricEncrypt, which will either throw a cryptic error or produce corrupted data. Both disable.ts and next-auth-options.ts explicitly check for this key before proceeding, but setup.ts does not.

--- a/apps/web/pages/api/auth/two-factor/totp/setup.ts
+++ b/apps/web/pages/api/auth/two-factor/totp/setup.ts
@@ -57,6 +57,10 @@ 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);
 
+  if (!process.env.CALENDSO_ENCRYPTION_KEY) {
+    return res.status(500).json({ error: ErrorCode.InternalServerError });
+  }
+
   // generate backup codes with 10 character length
   const backupCodes = Array.from(Array(10), () => crypto.randomBytes(5).toString("hex"));
 
@@ -66,8 +70,8 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
     },
     data: {
-      backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY),
+      backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY as string),
       twoFactorEnabled: false,
-      twoFactorSecret: symmetricEncrypt(secret, process.env.CALENDSO_ENCRYPTION_KEY),
+      twoFactorSecret: symmetricEncrypt(secret, process.env.CALENDSO_ENCRYPTION_KEY as string),
     },
   });

🤖 Grapple PR auto-fix • critical • Review this diff before applying

twoFactorEnabled: false,
twoFactorSecret: symmetricEncrypt(secret, process.env.CALENDSO_ENCRYPTION_KEY),
},
Expand All @@ -70,5 +75,5 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
const keyUri = authenticator.keyuri(name, "Cal", secret);

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — API contract violation (confidence: 100%)

The setup endpoint returns plaintext backup codes in the JSON response (backupCodes array). While necessary for initial display, there is no one-time retrieval mechanism — any replay of this request regenerates and overwrites all backup codes and the TOTP secret, and the response is available in browser dev tools/network history. More critically, if the user calls setup again (e.g., by re-entering their password), the previously stored backup codes are silently overwritten with new ones, invalidating any codes the user may have saved.

Evidence:

  • Line 75: return res.json({ secret, keyUri, dataUri, backupCodes }); — backup codes returned as plaintext array
  • Lines 63-71: Every call to this endpoint regenerates and overwrites both twoFactorSecret and backupCodes unconditionally
  • The intent specification notes: 'backup codes are returned in the plaintext API response... no additional masking or one-time-retrieval mechanism exists'

Agent: architecture

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Security (confidence: 87%)

Backup codes are returned in plaintext in the API response. This is necessary for the initial display but means the codes travel over the wire. The intent spec notes this relies entirely on HTTPS for protection. Consider adding a comment documenting this security decision.

Evidence:

  • return res.json({ secret, keyUri, dataUri, backupCodes }) — backupCodes is an array of plaintext hex strings
  • Intent spec risk area: 'backup codes are returned in the plaintext API response... must rely on HTTPS'

Agent: logic

const dataUri = await qrcode.toDataURL(keyUri);

return res.json({ secret, keyUri, dataUri });
return res.json({ secret, keyUri, dataUri, backupCodes });
}
Loading