diff --git a/backend/src/__tests__/mfa.test.ts b/backend/src/__tests__/mfa.test.ts index 92a83dbb..67d06d6d 100644 --- a/backend/src/__tests__/mfa.test.ts +++ b/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((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((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 ────────────────────────────────────────────────────────────── diff --git a/backend/src/routes/mfa.ts b/backend/src/routes/mfa.ts index 0feb265b..4f8c3050 100644 --- a/backend/src/routes/mfa.ts +++ b/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, ''); diff --git a/backend/src/services/DatabaseService.ts b/backend/src/services/DatabaseService.ts index 098414dc..44304bd4 100644 --- a/backend/src/services/DatabaseService.ts +++ b/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[] { diff --git a/backend/src/services/MfaService.ts b/backend/src/services/MfaService.ts index 4ba1e906..f8fd20a4 100644 --- a/backend/src/services/MfaService.ts +++ b/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 { 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 }; } /**