feat: 2fa backup codes#9
Conversation
Co-authored-by: Peer Richelsen <peeroke@gmail.com>
🤖 Grapple PR ReviewSummaryThis PR adds 2 files (BackupCode.tsx, migration.sql), modifies 14 files. Title: "feat: 2fa backup codes" Changes: 16 files (+280 / -36 lines)
Resolves: #3 (SMS workflow reminder retry count tracking) CI: ⏳ 0 passed, 0 failed Impact Diagramgraph TD
pr_0[/"🔀 PR Changes: 16 files"/]
f1["🟢 BackupCode.tsx +29/-0"]
pr_0 --> f1
f2["🟡 TwoFactor.tsx +2/-2"]
pr_0 --> f2
f3["🟡 DisableTwoFactorModal.tsx +35/-7"]
pr_0 --> f3
f4["🟡 EnableTwoFactorModal.tsx +76/-7"]
pr_0 --> f4
f5["🟡 TwoFactorAuthAPI.ts +2/-2"]
pr_0 --> f5
f6["🟡 disable.ts +25/-2"]
pr_0 --> f6
f7["🟡 setup.ts +6/-1"]
pr_0 --> f7
f8["🟡 login.tsx +36/-12"]
pr_0 --> f8
f9["🟡 login.2fa.e2e.ts +21/-0"]
pr_0 --> f9
f10["🟡 common.json +7/-0"]
pr_0 --> f10
f11["🟡 ErrorCode.ts +2/-0"]
pr_0 --> f11
f12["🟡 next-auth-options.ts +30/-2"]
pr_0 --> f12
more_13["... +4 more files"]
pr_0 --> more_13
f1 -.- f2
f3 -.- f4
f4 -.- f5
f6 -.- f7
f11 -.- f12
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
Review details
Powered by Grapple PR — AI that understands your code. |
![grapple-pr[bot]](https://avatars.githubusercontent.com/in/3205222?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR implements 2FA backup codes for account recovery, allowing users to generate one-time-use backup codes during 2FA setup, and use them to authenticate or disable 2FA when they lose access to their authenticator app.
Changes: 16 files (+280 / -36 lines)
- 🟢 2 files added:
apps/web/components/auth/BackupCode.tsx,packages/prisma/migrations/20230804153419_add_backup_codes/migration.sql - 🟡 14 files modified:
apps/web/components/auth/TwoFactor.tsx,apps/web/components/settings/DisableTwoFactorModal.tsx,apps/web/components/settings/EnableTwoFactorModal.tsx,apps/web/components/settings/TwoFactorAuthAPI.ts,apps/web/pages/api/auth/two-factor/totp/disable.ts+9 more
Resolves: #3 (SMS workflow reminder retry count tracking)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 16 files"/]
f1["🟢 BackupCode.tsx +29/-0"]
pr_0 --> f1
f2["🟡 TwoFactor.tsx +2/-2"]
pr_0 --> f2
f3["🟡 DisableTwoFactorModal.tsx +35/-7"]
pr_0 --> f3
f4["🟡 EnableTwoFactorModal.tsx +76/-7"]
pr_0 --> f4
f5["🟡 TwoFactorAuthAPI.ts +2/-2"]
pr_0 --> f5
f6["🟡 disable.ts +25/-2"]
pr_0 --> f6
f7["🟡 setup.ts +6/-1"]
pr_0 --> f7
f8["🟡 login.tsx +36/-12"]
pr_0 --> f8
f9["🟡 login.2fa.e2e.ts +21/-0"]
pr_0 --> f9
f10["🟡 common.json +7/-0"]
pr_0 --> f10
f11["🟡 ErrorCode.ts +2/-0"]
pr_0 --> f11
f12["🟡 next-auth-options.ts +30/-2"]
pr_0 --> f12
more_13["... +4 more files"]
pr_0 --> more_13
f1 -.- f2
f3 -.- f4
f4 -.- f5
f6 -.- f7
f11 -.- f12
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
3 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 3 |
| 🟠 Major | 8 |
| 🟡 Minor | 6 |
| 🔵 Info | 3 |
Review details
- Reviewed at: 2026-04-07T18:29:37.970Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 114.2s
- Low-confidence findings suppressed: 2
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| // 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."); |
There was a problem hiding this comment.
🔴 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
| // 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."); |
There was a problem hiding this comment.
🔴 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 usereturn res.status(400).json({ error: ... }) - The caller (DisableTwoFactorModal) expects a JSON body with an
errorfield; a 500 with no JSON body will causeresponse.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
| const textBlob = new Blob([body.backupCodes.map(formatBackupCode).join("\n")], { | ||
| type: "text/plain", | ||
| }); | ||
| if (backupCodesUrl) URL.revokeObjectURL(backupCodesUrl); |
There was a problem hiding this comment.
🔴 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
useEffectcleanup 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
| <div className="mb-4"> | ||
| <TextField | ||
| <PasswordField | ||
| label={t("password")} |
There was a problem hiding this comment.
🟠 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
| if (user.twoFactorEnabled) { | ||
| if (user.twoFactorEnabled && credentials.backupCode) { | ||
| if (!process.env.CALENDSO_ENCRYPTION_KEY) { | ||
| console.error("Missing encryption key; cannot proceed with backup code login."); |
There was a problem hiding this comment.
🟠 MAJOR — Error Handling (confidence: 100%)
The encryption key guard in the login flow throws a raw Error(ErrorCode.InternalServerError). While NextAuth's authorize function communicates errors via thrown errors (unlike the disable endpoint which uses res.json), this is consistent with the pattern here. However, the same issue exists as in disable.ts: if CALENDSO_ENCRYPTION_KEY is missing, the error InternalServerError is thrown but it may not map to a user-friendly error message on the login page. Looking at the login page's error handling, errorMessages object maps specific error codes to messages, but InternalServerError maps to t('something_went_wrong') which is acceptable.
Evidence:
- throw new Error(ErrorCode.InternalServerError) on line 133
- Login page errorMessages has InternalServerError mapping at the page level
Agent: logic
| @@ -194,7 +218,7 @@ export default function Login({ | |||
| </div> | |||
There was a problem hiding this comment.
🟡 MINOR — Pattern violations (confidence: 93%)
Nested ternary expression is hard to read and violates common readability patterns. The expression {twoFactorRequired ? !twoFactorLostAccess ? : : null} has two levels of ternary nesting without parentheses.
Evidence:
- Line 218:
{twoFactorRequired ? !twoFactorLostAccess ? : : null} - ESLint's no-nested-ternary rule typically flags this pattern
- The codebase's existing patterns use simpler conditional rendering
Agent: architecture
| timeFormat Int? @default(12) | ||
| twoFactorSecret String? | ||
| twoFactorEnabled Boolean @default(false) | ||
| backupCodes String? |
There was a problem hiding this comment.
🟡 MINOR — Migration safety (confidence: 88%)
The backupCodes field is added as a nullable String? column, which is safe for migration. However, it is placed between twoFactorEnabled and identityProvider rather than next to twoFactorSecret, slightly breaking the logical grouping of 2FA-related fields.
Evidence:
- Line 203:
twoFactorSecret String? - Line 204:
twoFactorEnabled Boolean @default(false) - Line 205:
backupCodes String?— logically related to twoFactorSecret but separated from it by twoFactorEnabled
Agent: architecture
| }); | ||
| } else if (user.twoFactorEnabled) { | ||
| if (!credentials.totpCode) { | ||
| throw new Error(ErrorCode.SecondFactorRequired); |
There was a problem hiding this comment.
🔵 INFO — Cross-service impact (confidence: 99%)
The backup code authentication path takes precedence over TOTP when both credentials.backupCode and credentials.totpCode are provided. This is because the if (user.twoFactorEnabled && credentials.backupCode) check comes before the else if (user.twoFactorEnabled) check. While likely intentional, this means a request with both fields populated will consume a backup code even if a valid TOTP was also provided.
Evidence:
- Line 128:
if (user.twoFactorEnabled && credentials.backupCode)— checked first - Line 160:
else if (user.twoFactorEnabled)— TOTP path only reached if backupCode is falsy - The login form can theoretically have both fields populated if the user switches between TOTP and backup code views without clearing values
Agent: architecture
| @@ -70,5 +75,5 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse) | |||
| const keyUri = authenticator.keyuri(name, "Cal", secret); | |||
There was a problem hiding this comment.
🔵 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
| @@ -1,3 +1,4 @@ | |||
| import crypto from "crypto"; | |||
There was a problem hiding this comment.
🔵 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:
- PR implements 2FA backup codes for account recovery
- Issue SMS workflow reminder retry count tracking #3 describes 'SMS workflow reminder retry count tracking'
- Intent spec notes: 'the PR title and code implement 2FA backup codes, but the linked ticket (SMS workflow reminder retry count tracking #3) describes SMS workflow reminder retry count tracking — these are unrelated'
Agent: logic
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds 2FA backup codes to the authentication system, allowing users to generate one-time-use backup codes during 2FA setup and use them to authenticate or disable 2FA when they lose access to their authenticator app.
Changes: 16 files (+280 / -36 lines)
- 🟢 2 files added:
apps/web/components/auth/BackupCode.tsx,packages/prisma/migrations/20230804153419_add_backup_codes/migration.sql - 🟡 14 files modified:
apps/web/components/auth/TwoFactor.tsx,apps/web/components/settings/DisableTwoFactorModal.tsx,apps/web/components/settings/EnableTwoFactorModal.tsx,apps/web/components/settings/TwoFactorAuthAPI.ts,apps/web/pages/api/auth/two-factor/totp/disable.ts+9 more
Resolves: #3 (SMS workflow reminder retry count tracking)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 16 files"/]
f1["🟢 BackupCode.tsx +29/-0"]
pr_0 --> f1
f2["🟡 TwoFactor.tsx +2/-2"]
pr_0 --> f2
f3["🟡 DisableTwoFactorModal.tsx +35/-7"]
pr_0 --> f3
f4["🟡 EnableTwoFactorModal.tsx +76/-7"]
pr_0 --> f4
f5["🟡 TwoFactorAuthAPI.ts +2/-2"]
pr_0 --> f5
f6["🟡 disable.ts +25/-2"]
pr_0 --> f6
f7["🟡 setup.ts +6/-1"]
pr_0 --> f7
f8["🟡 login.tsx +36/-12"]
pr_0 --> f8
f9["🟡 login.2fa.e2e.ts +21/-0"]
pr_0 --> f9
f10["🟡 common.json +7/-0"]
pr_0 --> f10
f11["🟡 ErrorCode.ts +2/-0"]
pr_0 --> f11
f12["🟡 next-auth-options.ts +30/-2"]
pr_0 --> f12
more_13["... +4 more files"]
pr_0 --> more_13
f1 -.- f2
f3 -.- f4
f4 -.- f5
f6 -.- f7
f11 -.- f12
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
2 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 2 |
| 🟠 Major | 4 |
| 🟡 Minor | 3 |
| 🔵 Info | 1 |
Review details
- Reviewed at: 2026-04-08T02:27:40.733Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 100.5s
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| DisplayBackupCodes, | ||
| DisplayQrCode, | ||
| EnterTotpCode, | ||
| } |
There was a problem hiding this comment.
🔴 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
| id: session.user.id, | ||
| }, | ||
| data: { | ||
| backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY), |
There was a problem hiding this comment.
🔴 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
| backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY), | ||
| }, | ||
| }); | ||
| } else if (user.twoFactorEnabled) { |
There was a problem hiding this comment.
🟠 MAJOR — Race condition - Concurrent backup code usage (confidence: 91%)
Two simultaneous login attempts using the same backup code can both succeed because the read-check-update cycle is not atomic. Both requests read the same backupCodes array, both find the code at the same index, and both proceed to null it out and update. This means a single backup code could be used twice.
Evidence:
- Line 138: backupCodes are read via symmetricDecrypt
- Line 142: indexOf check against the decrypted array
- Line 145-153: null out and re-encrypt in a separate update
- No database-level locking or optimistic concurrency check between read and write
- Intent edge case: 'Concurrent backup code usage: two simultaneous login attempts with the same backup code could both pass before either update commits — no locking mechanism is present'
Agent: logic
| // create backup codes download url | ||
| const textBlob = new Blob([body.backupCodes.map(formatBackupCode).join("\n")], { | ||
| type: "text/plain", | ||
| }); |
There was a problem hiding this comment.
🟠 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
| symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) | ||
| ); | ||
|
|
||
| // check if user-supplied code matches one |
There was a problem hiding this comment.
🟠 MAJOR — Null matching in backup codes array (confidence: 89%)
After a backup code is used, it is replaced with null in the array. The indexOf check uses credentials.backupCode.replaceAll('-', '') which always produces a string. JavaScript's indexOf uses strict equality (===), so null === 'somestring' is always false. This is safe because a string can never strictly equal null. However, if the backupCodes array somehow contains the string 'null' instead of the value null (e.g., due to a serialization bug), it would not match a user input of 'null' since that would be a 4-character string and backup codes are 10 characters. This is theoretically safe but worth noting.
Evidence:
- Line 145: backupCodes[index] = null — sets used code to null
- Line 142: backupCodes.indexOf(credentials.backupCode.replaceAll('-', '')) — uses indexOf with strict equality
- Array.prototype.indexOf uses strict equality (===), so null !== any string
- Intent edge case notes this concern but it is safe due to strict equality
Agent: logic
| }: DisableTwoFactorAuthModalProps) => { | ||
| const [isDisabling, setIsDisabling] = useState(false); | ||
| const [errorMessage, setErrorMessage] = useState<string | null>(null); | ||
| const [twoFactorLostAccess, setTwoFactorLostAccess] = useState(false); |
There was a problem hiding this comment.
🟡 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
| const [isSubmitting, setIsSubmitting] = useState(false); | ||
| const [errorMessage, setErrorMessage] = useState<string | null>(null); | ||
|
|
||
| const resetState = () => { |
There was a problem hiding this comment.
🟡 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
| [SetupStep.EnterTotpCode]: t("2fa_enter_six_digit_code"), | ||
| }; | ||
| const [step, setStep] = useState(SetupStep.ConfirmPassword); | ||
| const [password, setPassword] = useState(""); |
There was a problem hiding this comment.
🟡 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
| </div> | ||
|
|
||
| {twoFactorRequired && <TwoFactor center />} | ||
| {twoFactorRequired ? !twoFactorLostAccess ? <TwoFactor center /> : <BackupCode center /> : null} |
There was a problem hiding this comment.
🔵 INFO — Operator precedence readability (confidence: 80%)
The nested ternary expression {twoFactorRequired ? !twoFactorLostAccess ? : : null} is difficult to read and could be misinterpreted. It relies on right-to-left ternary association which is not intuitive for all developers.
Evidence:
- Line 221: {twoFactorRequired ? !twoFactorLostAccess ? : : null}
- Nested ternaries without parentheses reduce readability
Agent: logic
| DisplayBackupCodes, | ||
| DisplayQrCode, | ||
| EnterTotpCode, | ||
| } |
There was a problem hiding this comment.
✅ [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.
| } | |
| DisplayBackupCodes, |
🤖 Grapple PR auto-fix • critical • confidence: 100%
| // create backup codes download url | ||
| const textBlob = new Blob([body.backupCodes.map(formatBackupCode).join("\n")], { | ||
| type: "text/plain", | ||
| }); |
There was a problem hiding this comment.
✅ [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.
| }); | |
| // 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%
| }: DisableTwoFactorAuthModalProps) => { | ||
| const [isDisabling, setIsDisabling] = useState(false); | ||
| const [errorMessage, setErrorMessage] = useState<string | null>(null); | ||
| const [twoFactorLostAccess, setTwoFactorLostAccess] = useState(false); |
There was a problem hiding this comment.
✅ [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.
| const [twoFactorLostAccess, setTwoFactorLostAccess] = useState(false); | |
| <Button | |
| color="secondary" | |
| onClick={() => { | |
| setTwoFactorLostAccess(false); | |
| resetForm(); | |
| onCancel(); | |
| }}> |
🤖 Grapple PR auto-fix • minor • confidence: 93%
| const [isSubmitting, setIsSubmitting] = useState(false); | ||
| const [errorMessage, setErrorMessage] = useState<string | null>(null); | ||
|
|
||
| const resetState = () => { |
There was a problem hiding this comment.
✅ [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.
| const resetState = () => { | |
| setBackupCodes([]); | |
| if (backupCodesUrl) { | |
| URL.revokeObjectURL(backupCodesUrl); | |
| } | |
| setBackupCodesUrl(""); | |
| setDataUri(""); | |
| setSecret(""); | |
| setIsSubmitting(false); |
🤖 Grapple PR auto-fix • minor • confidence: 85%
| [SetupStep.EnterTotpCode]: t("2fa_enter_six_digit_code"), | ||
| }; | ||
| const [step, setStep] = useState(SetupStep.ConfirmPassword); | ||
| const [password, setPassword] = useState(""); |
There was a problem hiding this comment.
✅ [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[]>([]).
| const [password, setPassword] = useState(""); | |
| const [backupCodes, setBackupCodes] = useState<string[]>([]); |
🤖 Grapple PR auto-fix • minor • confidence: 85%
| id: session.user.id, | ||
| }, | ||
| data: { | ||
| backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY), |
There was a problem hiding this comment.
🟡 [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
| backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY), | ||
| }, | ||
| }); | ||
| } else if (user.twoFactorEnabled) { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (30 lines, 1 file) — review recommended)
Two simultaneous login attempts using the same backup code can both succeed because the read-check-update cycle is not atomic. Both requests read the same backupCodes array, both find the code at the same index, and both proceed to null it out and update. This means a single backup code could be used twice.
--- a/packages/features/auth/lib/next-auth-options.ts
+++ b/packages/features/auth/lib/next-auth-options.ts
@@ -131,22 +131,37 @@ const providers: Provider[] = [
if (user.twoFactorEnabled && credentials.backupCode) {
if (!process.env.CALENDSO_ENCRYPTION_KEY) {
console.error("Missing encryption key; cannot proceed with backup code login.");
throw new Error(ErrorCode.InternalServerError);
}
if (!user.backupCodes) throw new Error(ErrorCode.MissingBackupCodes);
- const backupCodes = JSON.parse(
- symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY)
- );
+ const encryptionKey = process.env.CALENDSO_ENCRYPTION_KEY;
+ const currentEncryptedBackupCodes = user.backupCodes;
+
+ const backupCodes = JSON.parse(symmetricDecrypt(currentEncryptedBackupCodes, encryptionKey));
// check if user-supplied code matches one
const index = backupCodes.indexOf(credentials.backupCode.replaceAll("-", ""));
if (index === -1) throw new Error(ErrorCode.IncorrectBackupCode);
- // delete verified backup code and re-encrypt remaining
+ // Atomically null out the used backup code and re-encrypt.
+ // We use updateMany with a filter on the CURRENT encrypted value to guard against
+ // concurrent requests that read the same backupCodes snapshot and attempt to use
+ // the same code simultaneously. If another request already consumed and re-encrypted
+ // the codes, the `backupCodes` column will no longer match `currentEncryptedBackupCodes`
+ // and `count` will be 0 — meaning the code was already used. This makes the
+ // read-check-update cycle effectively atomic at the database row level.
backupCodes[index] = null;
- await prisma.user.update({
+ const updatedBackupCodes = symmetricEncrypt(JSON.stringify(backupCodes), encryptionKey);
+
+ const { count } = await prisma.user.updateMany({
where: {
id: user.id,
+ // Optimistic concurrency check: only update if the backup codes column
+ // still holds the exact encrypted value we read. If a concurrent request
+ // already nulled out this code and re-encrypted, this filter won't match.
+ backupCodes: currentEncryptedBackupCodes,
},
data: {
- backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY),
+ backupCodes: updatedBackupCodes,
},
});
+
+ if (count === 0) {
+ // Another concurrent request consumed this backup code before us.
+ throw new Error(ErrorCode.IncorrectBackupCode);
+ }
} else if (user.twoFactorEnabled) {🤖 Grapple PR auto-fix • major • Review this diff before applying
| symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) | ||
| ); | ||
|
|
||
| // check if user-supplied code matches one |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (14 lines, 1 file) — review recommended)
After a backup code is used, it is replaced with null in the array. The indexOf check uses credentials.backupCode.replaceAll('-', '') which always produces a string. JavaScript's indexOf uses strict equality (===), so
null === 'somestring'is always false. This is safe because a string can never strictly equal null. However, if the backupCodes array somehow contains the string 'null' instead of the value null (e.g., due to a serialization bug), it would not match a user input of 'null' since that would be a 4-character string and backup codes are 10 characters. This is theoretically safe but worth noting.
--- a/packages/features/auth/lib/next-auth-options.ts
+++ b/packages/features/auth/lib/next-auth-options.ts
@@ -139,9 +139,16 @@ const providers: Provider[] = [
const backupCodes = JSON.parse(
symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY)
- );
+ ) as (string | null)[];
- // check if user-supplied code matches one
- const index = backupCodes.indexOf(credentials.backupCode.replaceAll("-", ""));
+ const inputCode = credentials.backupCode.replaceAll("-", "");
+
+ // Only search among unused (non-null) codes.
+ // Used codes are set to null in the array; indexOf uses strict equality
+ // so null would never match a string, but filtering explicitly makes
+ // the "one-time-use" invariant clear and guards against any future
+ // serialization quirks (e.g., null stored as the string "null").
+ const index = backupCodes.findIndex(
+ (code) => code !== null && code === inputCode
+ );
if (index === -1) throw new Error(ErrorCode.IncorrectBackupCode);
// delete verified backup code and re-encrypt remaining🤖 Grapple PR auto-fix • major • Review this diff before applying
|
@grapple-pr review |
🤖 Grapple PR ReviewSummaryThis PR adds 2FA backup code functionality to the authentication system, allowing users to generate one-time-use backup codes during 2FA setup and use them to authenticate or disable 2FA when they lose access to their authenticator app. Changes: 16 files (+280 / -36 lines)
Resolves: #3 (SMS workflow reminder retry count tracking) CI: ⏳ 0 passed, 0 failed Impact Diagramgraph TD
pr_0[/"🔀 PR Changes: 16 files"/]
f1["🟢 BackupCode.tsx +29/-0"]
pr_0 --> f1
f2["🟡 TwoFactor.tsx +2/-2"]
pr_0 --> f2
f3["🟡 DisableTwoFactorModal.tsx +35/-7"]
pr_0 --> f3
f4["🟡 EnableTwoFactorModal.tsx +76/-7"]
pr_0 --> f4
f5["🟡 TwoFactorAuthAPI.ts +2/-2"]
pr_0 --> f5
f6["🟡 disable.ts +25/-2"]
pr_0 --> f6
f7["🟡 setup.ts +6/-1"]
pr_0 --> f7
f8["🟡 login.tsx +36/-12"]
pr_0 --> f8
f9["🟡 login.2fa.e2e.ts +21/-0"]
pr_0 --> f9
f10["🟡 common.json +7/-0"]
pr_0 --> f10
f11["🟡 ErrorCode.ts +2/-0"]
pr_0 --> f11
f12["🟡 next-auth-options.ts +30/-2"]
pr_0 --> f12
more_13["... +4 more files"]
pr_0 --> more_13
f1 -.- f2
f3 -.- f4
f4 -.- f5
f6 -.- f7
f11 -.- f12
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
Review details
Powered by Grapple PR — AI that understands your code. |
2 participants
Benchmark test PR #3 - Replicating Greptile benchmark for Grapple PR evaluation.