fix(mfa): enforce single-use backup codes under concurrent verification (#1262)

* fix(mfa): enforce single-use backup codes under concurrent verification

Backup-code consumption read the stored hash set, awaited bcrypt.compare,
then wrote the shrunk set back. Two concurrent /login/mfa requests carrying
the same code could both read the same set, both match, and both persist,
so a single backup code yielded two authenticated sessions.

Move consumption into a synchronous transaction: verifyBackupCode now returns
the matched hash without mutating state, and a new consumeBackupCodeHash
re-reads and shrinks the set atomically, returning whether the hash was still
present. Login gates success on that result, so exactly one concurrent request
wins. Reshape the verify result into a discriminated union so a match always
carries its hash.

Add coverage: a deterministic consume test (same hash twice, distinct hashes,
absent hash), a concurrent same-code login race, backup-code exhaustion, and a
disable-via-backup-code path.

* test(mfa): make the concurrent backup-code test deterministic

The race test relied on incidental scheduling to interleave the two requests,
so it could false-green if they happened to serialize. Add a barrier that holds
both requests just after verification (once both have read the same stored set)
until both arrive, then releases them into the atomic consume. This forces the
race every run, so the test fails against a non-atomic consume and passes only
when exactly one request wins. A timeout releases the barrier if only one
request arrives, so a setup fault fails loudly instead of hanging.

backend/src/__tests__/mfa.test.ts

@@ -106,21 +106,19 @@ describe('MfaService', () => {
     }
   });
 
-  it('verifyBackupCode matches and returns remaining set with the matched hash removed', async () => {
+  it('verifyBackupCode matches and returns the matched hash for the caller to consume', async () => {
     const codes = MfaService.generateBackupCodes();
     const hashes = await MfaService.hashBackupCodes(codes);
     const result = await MfaService.verifyBackupCode(hashes, codes[3]);
     expect(result.matched).toBe(true);
-    expect(result.remainingHashes).toHaveLength(hashes.length - 1);
-    expect(result.remainingHashes).not.toContain(hashes[3]);
+    if (result.matched) expect(result.matchedHash).toBe(hashes[3]);
   });
 
-  it('verifyBackupCode on non-match returns original hashes', async () => {
+  it('verifyBackupCode on non-match reports no match', async () => {
     const codes = MfaService.generateBackupCodes();
     const hashes = await MfaService.hashBackupCodes(codes);
     const result = await MfaService.verifyBackupCode(hashes, 'NOTACODE99');
     expect(result.matched).toBe(false);
-    expect(result.remainingHashes).toBe(hashes);
   });
 
   it('normalizeBackupCode strips spaces/dashes and uppercases', () => {
@@ -129,6 +127,44 @@ describe('MfaService', () => {
   });
 });
 
+// ─── consumeBackupCodeHash: the atomic single-use enforcement point ────────────
+
+describe('DatabaseService.consumeBackupCodeHash', () => {
+  it('consumes a present hash once: a second consume of the same hash returns false', async () => {
+    const { userId, backupCodes } = await seedMfaUser('consume-same', 'mfapassword123');
+    const db = DatabaseService.getInstance();
+    const hashes = JSON.parse(db.getUserMfa(userId)!.backup_codes_json!) as string[];
+    const target = hashes[2];
+
+    // First consume wins, second loses: this is what guarantees single-use
+    // when two concurrent logins race on the same code.
+    expect(db.consumeBackupCodeHash(userId, target)).toBe(true);
+    expect(db.consumeBackupCodeHash(userId, target)).toBe(false);
+
+    const remaining = JSON.parse(db.getUserMfa(userId)!.backup_codes_json!) as string[];
+    expect(remaining).toHaveLength(backupCodes.length - 1);
+    expect(remaining).not.toContain(target);
+  });
+
+  it('consumes two distinct hashes independently, dropping the set by two', async () => {
+    const { userId, backupCodes } = await seedMfaUser('consume-distinct', 'mfapassword123');
+    const db = DatabaseService.getInstance();
+    const hashes = JSON.parse(db.getUserMfa(userId)!.backup_codes_json!) as string[];
+
+    expect(db.consumeBackupCodeHash(userId, hashes[0])).toBe(true);
+    expect(db.consumeBackupCodeHash(userId, hashes[1])).toBe(true);
+
+    const remaining = JSON.parse(db.getUserMfa(userId)!.backup_codes_json!) as string[];
+    expect(remaining).toHaveLength(backupCodes.length - 2);
+  });
+
+  it('returns false for a hash that is not in the stored set', async () => {
+    const { userId } = await seedMfaUser('consume-absent', 'mfapassword123');
+    const db = DatabaseService.getInstance();
+    expect(db.consumeBackupCodeHash(userId, 'not-a-stored-hash')).toBe(false);
+  });
+});
+
 // ─── Login flow ───────────────────────────────────────────────────────────────
 
 describe('POST /api/auth/login with MFA-enabled user', () => {
@@ -263,6 +299,87 @@ describe('POST /api/auth/login/mfa', () => {
     expect(hashes.length).toBe(remaining - 1);
   });
 
+  it('enforces single-use when the same backup code is submitted concurrently', async () => {
+    const u = 'mfauser-backup-race';
+    const p = 'mfapassword123';
+    const { backupCodes: codes } = await seedMfaUser(u, p);
+    const chosen = codes[0];
+
+    // Force the race deterministically rather than relying on scheduling: hold
+    // both requests just after verifyBackupCode (so both have already read the
+    // same stored set and matched) until both have arrived, then let them race
+    // into the atomic consume. The pre-fix non-atomic path would let both win
+    // here; the fix must still let exactly one through. A timeout releases the
+    // barrier if only one request ever arrives, so a setup fault fails loudly
+    // instead of hanging.
+    let arrived = 0;
+    let releaseBarrier!: () => void;
+    const bothVerified = new Promise<void>((resolve) => { releaseBarrier = resolve; });
+    const realVerify = MfaService.verifyBackupCode.bind(MfaService);
+    const spy = vi.spyOn(MfaService, 'verifyBackupCode').mockImplementation(async (hashes, code) => {
+      const result = await realVerify(hashes, code);
+      arrived += 1;
+      if (arrived >= 2) releaseBarrier();
+      await Promise.race([bothVerified, new Promise<void>((r) => setTimeout(r, 3000))]);
+      return result;
+    });
+
+    try {
+      const [login1, login2] = await Promise.all([
+        request(app).post('/api/auth/login').send({ username: u, password: p }),
+        request(app).post('/api/auth/login').send({ username: u, password: p }),
+      ]);
+      const pending1 = findCookie(login1.headers, 'sencho_mfa_pending')!;
+      const pending2 = findCookie(login2.headers, 'sencho_mfa_pending')!;
+
+      const [r1, r2] = await Promise.all([
+        request(app).post('/api/auth/login/mfa').set('Cookie', pending1).send({ code: chosen, isBackupCode: true }),
+        request(app).post('/api/auth/login/mfa').set('Cookie', pending2).send({ code: chosen, isBackupCode: true }),
+      ]);
+
+      expect([r1, r2].filter((r) => r.status === 200)).toHaveLength(1);
+      expect([r1, r2].filter((r) => r.status === 401)).toHaveLength(1);
+
+      // Exactly one code was consumed from the stored set.
+      const db = DatabaseService.getInstance();
+      const mfa = db.getUserMfa(db.getUserByUsername(u)!.id)!;
+      const hashes = mfa.backup_codes_json ? (JSON.parse(mfa.backup_codes_json) as string[]) : [];
+      expect(hashes.length).toBe(codes.length - 1);
+    } finally {
+      spy.mockRestore();
+    }
+  });
+
+  it('exhausts all backup codes: each works once, then none remain', async () => {
+    const u = 'mfauser-backup-exhaust';
+    const p = 'mfapassword123';
+    const { backupCodes: codes } = await seedMfaUser(u, p);
+
+    for (const code of codes) {
+      const login = await request(app).post('/api/auth/login').send({ username: u, password: p });
+      const pending = findCookie(login.headers, 'sencho_mfa_pending')!;
+      const res = await request(app)
+        .post('/api/auth/login/mfa')
+        .set('Cookie', pending)
+        .send({ code, isBackupCode: true });
+      expect(res.status).toBe(200);
+    }
+
+    const db = DatabaseService.getInstance();
+    const mfa = db.getUserMfa(db.getUserByUsername(u)!.id)!;
+    const remaining = mfa.backup_codes_json ? (JSON.parse(mfa.backup_codes_json) as string[]) : [];
+    expect(remaining.length).toBe(0);
+
+    // A further attempt with a spent code is rejected cleanly, not crashed.
+    const login = await request(app).post('/api/auth/login').send({ username: u, password: p });
+    const pending = findCookie(login.headers, 'sencho_mfa_pending')!;
+    const after = await request(app)
+      .post('/api/auth/login/mfa')
+      .set('Cookie', pending)
+      .send({ code: codes[0], isBackupCode: true });
+    expect(after.status).toBe(401);
+  });
+
   it('locks the user after MFA_MAX_FAILED (5) wrong codes and returns 423', async () => {
     const u = 'mfauser-lock';
     const p = 'mfapassword123';
@@ -383,6 +500,24 @@ describe('MFA enrol + confirm', () => {
     expect(res.status).toBe(401);
     expect(db.getUserMfa(user.id)?.enabled).toBe(1);
   });
+
+  it('disables MFA when a valid backup code is supplied as proof of possession', async () => {
+    const db = DatabaseService.getInstance();
+    const { userId, backupCodes } = await seedMfaUser('disabler-backup', 'mfapassword123');
+    const user = db.getUser(userId)!;
+    const token = jwt.sign(
+      { username: 'disabler-backup', role: 'viewer', tv: user.token_version },
+      TEST_JWT_SECRET,
+      { expiresIn: '1m' },
+    );
+    const res = await request(app)
+      .post('/api/auth/mfa/disable')
+      .set('Authorization', `Bearer ${token}`)
+      .send({ code: backupCodes[0], isBackupCode: true });
+    expect(res.status).toBe(200);
+    // Disable wipes the whole MFA record, so single-use of the code is moot here.
+    expect(db.getUserMfa(userId)).toBeUndefined();
+  });
 });
 
 // ─── Admin reset ──────────────────────────────────────────────────────────────

backend/src/routes/mfa.ts

@@ -129,8 +129,15 @@ mfaRouter.post('/login/mfa', authRateLimiter, async (req: Request, res: Response
       if (isDebugEnabled()) console.log('[MFA:diag] login/mfa: branch=backup user=', user.username, 'matched=', result.matched, 'bcryptMs=', bcryptMs, 'hashesChecked=', hashes.length);
       if (bcryptMs > 500) console.warn('[MFA] Slow backup-code verify for user=', user.username, 'durationMs=', bcryptMs);
       if (result.matched) {
-        db.upsertUserMfa(decoded.user_id, { backup_codes_json: JSON.stringify(result.remainingHashes) });
-        verified = true;
+        // Consume the matched hash atomically. A concurrent request carrying
+        // the same code that already consumed it makes this return false, so
+        // exactly one login wins: single-use is enforced at the write, not from
+        // the in-memory snapshot read above.
+        if (db.consumeBackupCodeHash(decoded.user_id, result.matchedHash)) {
+          verified = true;
+        } else if (isDebugEnabled()) {
+          console.log('[MFA:diag] login/mfa: backup code already consumed (concurrent use) user=', user.username);
+        }
       }
     } else {
       const trimmed = rawCode.trim().replace(/\s+/g, '');

backend/src/services/DatabaseService.ts

@@ -2752,6 +2752,33 @@ export class DatabaseService {
         return result.changes;
     }
 
+    /**
+     * Atomically consume a single backup-code hash. Re-reads the stored set,
+     * removes `matchedHash` if still present, and persists the shrunk set in
+     * one synchronous transaction. Returns true when the hash was present (and
+     * is now consumed), false when it was already gone (e.g. a concurrent
+     * /login/mfa request carrying the same code consumed it first). This is the
+     * single-use enforcement point: callers verify the code, then gate success
+     * on this returning true, so two concurrent verifications of the same code
+     * cannot both succeed.
+     */
+    public consumeBackupCodeHash(userId: number, matchedHash: string): boolean {
+        const consume = this.db.transaction((): boolean => {
+            const row = this.db
+                .prepare('SELECT backup_codes_json FROM user_mfa WHERE user_id = ?')
+                .get(userId) as { backup_codes_json: string | null } | undefined;
+            const hashes: string[] = row?.backup_codes_json ? JSON.parse(row.backup_codes_json) : [];
+            const idx = hashes.indexOf(matchedHash);
+            if (idx === -1) return false;
+            hashes.splice(idx, 1);
+            this.db
+                .prepare('UPDATE user_mfa SET backup_codes_json = ?, updated_at = ? WHERE user_id = ?')
+                .run(JSON.stringify(hashes), Date.now(), userId);
+            return true;
+        });
+        return consume();
+    }
+
     // --- Role Assignments ---
 
     public getRoleAssignments(userId: number, resourceType: ResourceType, resourceId: string): RoleAssignment[] {

backend/src/services/MfaService.ts

@@ -15,10 +15,15 @@ const BACKUP_CODE_LENGTH = 10;
 const BACKUP_CODE_COUNT = 10;
 const BACKUP_HASH_COST = 10;
 
-export interface BackupVerifyResult {
-    matched: boolean;
-    remainingHashes: string[];
-}
+/**
+ * Result of checking a backup code. On a match it carries the exact stored
+ * hash so the caller can consume that entry atomically (see
+ * DatabaseService.consumeBackupCodeHash); the discriminated shape makes the
+ * "matched implies a hash to consume" invariant unrepresentable otherwise.
+ */
+export type BackupVerifyResult =
+    | { matched: true; matchedHash: string }
+    | { matched: false };
 
 export class MfaService {
     private static instance: MfaService;
@@ -121,25 +126,25 @@ export class MfaService {
 
     /**
      * Check a user-supplied backup code against the stored hashes. Returns
-     * `{ matched, remainingHashes }`; when matched, the matched hash is
-     * removed so callers can persist the shrunk set and enforce single-use
-     * semantics.
+     * `{ matched, matchedHash }`. The caller must consume `matchedHash`
+     * atomically to enforce single-use; this method does not mutate state, so
+     * two concurrent verifications of the same code cannot both win at the
+     * write (see DatabaseService.consumeBackupCodeHash).
      */
     public static async verifyBackupCode(hashes: string[], code: string): Promise<BackupVerifyResult> {
         const normalized = this.normalizeBackupCode(code);
-        if (!normalized) return { matched: false, remainingHashes: hashes };
+        if (!normalized) return { matched: false };
 
-        for (let i = 0; i < hashes.length; i++) {
-            // bcrypt.compare is constant-time for a given hash. We still check
-            // every hash regardless of an early hit to avoid leaking which
-            // slot matched via timing.
-            const ok = await bcrypt.compare(normalized, hashes[i]);
-            if (ok) {
-                const remaining = hashes.slice(0, i).concat(hashes.slice(i + 1));
-                return { matched: true, remainingHashes: remaining };
+        for (const hash of hashes) {
+            // bcrypt.compare is constant-time per hash; we return on the first
+            // match. The matched slot's position carries no useful signal: the
+            // codes are random and single-use, so leaking "which slot" via an
+            // early return tells an attacker nothing.
+            if (await bcrypt.compare(normalized, hash)) {
+                return { matched: true, matchedHash: hash };
             }
         }
-        return { matched: false, remainingHashes: hashes };
+        return { matched: false };
     }
 
     /**