fix(mfa): consume the factor in the same transaction as the password write (NAN-6586)

A recovery code was spent before the password update ran, so a failed write left
the code permanently burned on a reset that never happened. The TOTP counter
advanced the same way.

verifyTotp and consumeRecoveryCode now take an optional parent transaction, and
both handlers verify the factor inside the transaction that writes the password.
A rollback un-burns the code, and on the reset path the token stays spendable too.

Author: agusayerza

packages/server/lib/controllers/v1/account/mfa/stepUp.ts

@@ -5,6 +5,7 @@ import { mfaService } from '@nangohq/shared';
 import { isMFAEnabled } from './login.js';
 
 import type { DBUser, MFACredential } from '@nangohq/types';
+import type { Knex } from 'knex';
 
 export const mfaCredentialSchema = z.discriminatedUnion('type', [
     z
@@ -26,8 +27,11 @@ export type StepUpOutcome = 'not_required' | 'verified' | 'required' | 'invalid'
 /**
  * Second factor for a sensitive action the user is already part way through, rather than for a login.
  * Returns 'not_required' when the user has nothing enrolled, so callers stay usable for everyone else.
+ *
+ * Call this inside the transaction that performs the action and pass `trx`. A one-time credential is
+ * spent here, so it has to roll back with the action rather than outlive a failed one.
  */
-export async function verifyStepUpMfa(user: DBUser, credential: MFACredential | undefined): Promise<StepUpOutcome> {
+export async function verifyStepUpMfa(user: DBUser, credential: MFACredential | undefined, trx: Knex): Promise<StepUpOutcome> {
     if (!(await isMFAEnabled(user)) || !(await mfaService.hasActiveFactor(user.id))) {
         return 'not_required';
     }
@@ -38,8 +42,8 @@ export async function verifyStepUpMfa(user: DBUser, credential: MFACredential |
 
     const verified = (
         credential.type === 'recoveryCode'
-            ? await mfaService.consumeRecoveryCode(user.id, credential.recoveryCode)
-            : await mfaService.verifyTotp(user.id, credential.code)
+            ? await mfaService.consumeRecoveryCode(user.id, credential.recoveryCode, trx)
+            : await mfaService.verifyTotp(user.id, credential.code, trx)
     ).unwrap();
 
     return verified ? 'verified' : 'invalid';

packages/server/lib/controllers/v1/account/putResetPassword.integration.test.ts

@@ -169,6 +169,33 @@ describe(`PUT ${resetPasswordRoute}`, () => {
         expect(json).toStrictEqual({ error: { code: 'user_not_found' } });
     });
 
+    it('should not spend a recovery code when the reset itself fails', async () => {
+        const { email, password } = await signupVerifiedUser();
+        const session = await signin(email, password);
+        const { recoveryCodes } = await enrollMfa(session);
+
+        // issue the token before spying, issueResetToken writes through editUserPassword too
+        const token = await issueResetToken(email);
+
+        vi.spyOn(userService, 'editUserPassword').mockRejectedValueOnce(new Error('write failed'));
+        const failed = await api.fetch(resetPasswordRoute, {
+            method: 'PUT',
+            body: { token, password: 'aZ1-newpass!?', mfa: { type: 'recoveryCode', recoveryCode: recoveryCodes[0]! } }
+        });
+        expect(failed.res.status).toBe(500);
+
+        // the password never changed
+        await signin(email, password);
+
+        // and the rollback left both the recovery code and the reset token spendable
+        const retry = await api.fetch(resetPasswordRoute, {
+            method: 'PUT',
+            body: { token, password: 'aZ1-newpass!?', mfa: { type: 'recoveryCode', recoveryCode: recoveryCodes[0]! } }
+        });
+        expect(retry.res.status).toBe(200);
+        isSuccess(retry.json);
+    });
+
     it('should skip the second factor when the feature is off for the account', async () => {
         const { email, password } = await signupVerifiedUser();
         const session = await signin(email, password);

packages/server/lib/controllers/v1/account/putResetPassword.ts

@@ -55,27 +55,33 @@ export const putResetPassword = asyncWrapper<PutResetPassword>(async (req, res)
         return;
     }
 
-    // Without this the emailed token is the only thing standing between someone with mailbox access
-    // and the account, which is exactly what the second factor is meant to prevent.
-    const stepUp = await verifyStepUpMfa(user, mfa);
-    if (stepUp === 'required') {
+    const hashedPassword = (await pbkdf2(password, user.salt, PBKDF2_ITERATIONS, 32, 'sha256')).toString('base64');
+
+    // Without the second factor the emailed token is the only thing standing between someone with
+    // mailbox access and the account, which is exactly what MFA is meant to prevent. Verified in the
+    // same transaction as the write so a failed reset does not spend a one-time code.
+    const outcome = await db.knex.transaction(async (trx) => {
+        const stepUp = await verifyStepUpMfa(user, mfa, trx);
+        if (stepUp !== 'verified' && stepUp !== 'not_required') {
+            return stepUp;
+        }
+
+        user.hashed_password = hashedPassword;
+        user.reset_password_token = null;
+        await userService.editUserPassword(user, trx);
+        await deleteUserSessions(user.id, { trx });
+        return 'reset' as const;
+    });
+
+    if (outcome === 'required') {
         res.status(400).send({ error: { code: 'mfa_code_required' } });
         return;
     }
-    if (stepUp === 'invalid') {
+    if (outcome === 'invalid') {
         res.status(400).send({ error: { code: 'invalid_mfa_code' } });
         return;
     }
 
-    const hashedPassword = (await pbkdf2(password, user.salt, PBKDF2_ITERATIONS, 32, 'sha256')).toString('base64');
-
-    user.hashed_password = hashedPassword;
-    user.reset_password_token = null;
-    await db.knex.transaction(async (trx) => {
-        await userService.editUserPassword(user, trx);
-        await deleteUserSessions(user.id, { trx });
-    });
-
     res.status(200).json({
         success: true
     });

packages/server/lib/controllers/v1/user/password/putPassword.integration.test.ts

@@ -200,6 +200,29 @@ describe(`PUT ${passwordRoute}`, () => {
         isSuccess(retry.json);
     });
 
+    it('should not spend a recovery code when the change itself fails', async () => {
+        const { email, password } = await signupVerifiedUser();
+        const session = await signin(email, password);
+        const { recoveryCodes } = await enrollMfa(session);
+
+        vi.spyOn(userService, 'update').mockRejectedValueOnce(new Error('write failed'));
+        const failed = await api.fetch(passwordRoute, {
+            method: 'PUT',
+            session,
+            body: { oldPassword: password, newPassword: 'aZ1-newpass!?', mfa: { type: 'recoveryCode', recoveryCode: recoveryCodes[0]! } }
+        });
+        expect(failed.res.status).toBe(500);
+
+        // the password did not change, so the code it consumed has to be spendable again
+        const retry = await api.fetch(passwordRoute, {
+            method: 'PUT',
+            session,
+            body: { oldPassword: password, newPassword: 'aZ1-newpass!?', mfa: { type: 'recoveryCode', recoveryCode: recoveryCodes[0]! } }
+        });
+        expect(retry.res.status).toBe(200);
+        isSuccess(retry.json);
+    });
+
     it('should skip the second factor when the feature is off for the account', async () => {
         const { email, password } = await signupVerifiedUser();
         const session = await signin(email, password);

packages/server/lib/controllers/v1/user/password/putPassword.ts

@@ -45,25 +45,31 @@ export const putUserPassword = asyncWrapper<PutUserPassword, never>(async (req,
         return;
     }
 
-    // After the password check so a wrong password never consumes a code or a recovery code.
-    const stepUp = await verifyStepUpMfa(user, body.mfa);
-    if (stepUp === 'required') {
-        res.status(400).send({ error: { code: 'mfa_code_required' } });
-        return;
-    }
-    if (stepUp === 'invalid') {
-        res.status(400).send({ error: { code: 'invalid_mfa_code' } });
-        return;
-    }
-
     const salt = crypto.randomBytes(16).toString('base64');
     const hashedPassword = (await pbkdf2(body.newPassword, salt, PBKDF2_ITERATIONS, 32, 'sha256')).toString('base64');
 
-    await db.knex.transaction(async (trx) => {
+    // The factor is verified after the old password, so a wrong password never spends a code, and
+    // inside the same transaction as the write, so a failed write does not spend one either.
+    const outcome = await db.knex.transaction(async (trx) => {
+        const stepUp = await verifyStepUpMfa(user, body.mfa, trx);
+        if (stepUp !== 'verified' && stepUp !== 'not_required') {
+            return stepUp;
+        }
+
         await userService.update({ id: user.id, hashed_password: hashedPassword, salt }, trx);
         await deleteUserSessions(user.id, { trx });
+        return 'changed' as const;
     });
 
+    if (outcome === 'required') {
+        res.status(400).send({ error: { code: 'mfa_code_required' } });
+        return;
+    }
+    if (outcome === 'invalid') {
+        res.status(400).send({ error: { code: 'invalid_mfa_code' } });
+        return;
+    }
+
     // Re-issue a fresh session so the user who just changed their password stays logged in seamlessly.
     // req.logIn regenerates the session id internally (passport's fixation guard), rotating the current
     // session. Best effort: if it fails the user can simply re-authenticate with the new password.

packages/shared/lib/services/mfa.service.ts

@@ -122,9 +122,13 @@ class MFAService {
         return new Set(rows.map((row) => row.user_id));
     }
 
-    public async verifyTotp(userId: number, token: string): Promise<Result<boolean>> {
+    /**
+     * Pass `parentTrx` to consume the factor in the caller's transaction, so rolling that back
+     * un-burns the code rather than leaving it spent on an action that never happened.
+     */
+    public async verifyTotp(userId: number, token: string, parentTrx?: Knex): Promise<Result<boolean>> {
         try {
-            const verified = await db.knex.transaction(async (trx) => {
+            const verified = await this.inTransaction(parentTrx, async (trx) => {
                 const factor = await trx<DBMFAFactor>(FACTORS_TABLE).where({ user_id: userId }).whereNotNull('enabled_at').forUpdate().first();
                 if (!factor) {
                     return false;
@@ -154,10 +158,11 @@ class MFAService {
         }
     }
 
-    public async consumeRecoveryCode(userId: number, code: string): Promise<Result<boolean>> {
+    /** See {@link verifyTotp} for `parentTrx`. */
+    public async consumeRecoveryCode(userId: number, code: string, parentTrx?: Knex): Promise<Result<boolean>> {
         try {
             const codeHash = this.hashRecoveryCode(code);
-            const consumed = await db.knex.transaction(async (trx) => {
+            const consumed = await this.inTransaction(parentTrx, async (trx) => {
                 const factor = await trx<DBMFAFactor>(FACTORS_TABLE).where({ user_id: userId }).whereNotNull('enabled_at').forUpdate().first();
                 if (!factor) {
                     return false;
@@ -253,6 +258,10 @@ class MFAService {
         await trx<DBMFARecoveryCode>(RECOVERY_CODES_TABLE).insert(recoveryCodes.map((code) => ({ user_id: userId, code_hash: this.hashRecoveryCode(code) })));
     }
 
+    private async inTransaction<T>(parentTrx: Knex | undefined, handler: (trx: Knex) => Promise<T>): Promise<T> {
+        return parentTrx ? await handler(parentTrx) : await db.knex.transaction(handler);
+    }
+
     private async acquireUserLock(trx: Knex, userId: number): Promise<void> {
         await trx.raw('SELECT pg_advisory_xact_lock(?)', [userId]);
     }