mirror of
https://github.com/Studio-Saelix/sencho.git
synced 2026-08-24 17:36:42 +00:00
feat(mfa): UX hardening — auto-submit, paste tolerance, low-codes warning, dev-mode diagnostics (#620)
* feat(mfa): auto-submit 6-digit TOTPs and normalize pasted backup codes Match the UX every major MFA prompt has (GitHub, GitLab, 1Password): the challenge screen and every code-entry dialog now submit automatically once the sixth TOTP digit lands, and the backup-code input accepts pastes with smart-dashes, trailing whitespace, or mixed case without silently truncating the value. Also caps the backup-code input at the correct 11 characters (10 plus a single separator) instead of 12. Shared normalization helpers live in frontend/src/lib/mfa.ts so the challenge and the three account-settings dialogs stay in lockstep. * feat(mfa): warn users when backup codes run low The Account & Security card silently showed a dim count of backup codes remaining, which meant users could drift toward zero without noticing until their phone was already lost. The card now surfaces a warning tone with an alert icon when 1 or 2 codes remain, and swaps to a dedicated destructive warning card with a "Regenerate now" action when the user has used every code. * feat(mfa): gate diagnostic logs behind developer mode Reuses the existing isDebugEnabled() gate so operators investigating a 2FA support ticket can flip Developer Mode on to get per-branch diagnostics (login path taken, replay check outcome, failure counter after a verify, replay-table purge counts), and flip it back off when they are done. Standard lifecycle logs stay on by default: enrolment completed, 2FA disabled, backup codes regenerated, admin reset, SSO bypass toggled, lockout engaged. Nothing that could reveal a TOTP code, base32 secret, backup-code cleartext, or partial-auth JWT is ever logged. * test(mfa): cover drift, invalid formats, lockout recovery, and paste normalization Backend: a TOTP generated for a window that has already slid out is rejected, malformed backup codes (too short, non-alphanumeric, 11-char alphanumeric that matches no hash) all increment failed_attempts, a successful verify clears a below-threshold failure streak, a successful verify after locked_until has passed clears the lockout, a second enroll/start overwrites the prior pending secret, and the backup-code normalizer treats en-dash/em-dash/figure-dash with stray whitespace the same as the canonical form. E2E: low-backup-codes warning renders in the warning tone and the exhausted-codes state flips to the dedicated warning card, a 6-digit TOTP auto-submits without a button click, and a backup code pasted without the separator still signs in. * docs(mfa): auto-submit, paste guidance, and expanded troubleshooting Document that the challenge screen submits automatically on the sixth digit, that backup codes accept the separator and any case, and that the Account & Security card nudges at low code counts. Expands the troubleshooting section with entries for lost or exhausted backup codes and adds a short note to the admin guide about surfacing auth diagnostics via Developer Mode.
This commit is contained in:
@@ -477,3 +477,168 @@ describe('resetMfaForUser CLI helper', () => {
|
||||
expect(result.ok).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── Edge cases surfaced by Phase 1 audit ─────────────────────────────────────
|
||||
|
||||
describe('MfaService.verifyTotp drift handling', () => {
|
||||
it('rejects a code generated more than one step outside the window', () => {
|
||||
const secret = MfaService.generateSecret();
|
||||
// Freeze clock at a known step boundary.
|
||||
const baseMs = 1_700_000_000_000;
|
||||
vi.useFakeTimers();
|
||||
try {
|
||||
vi.setSystemTime(baseMs);
|
||||
const code = authenticator.generate(secret);
|
||||
// Advance three full 30s windows so the code is outside the +-1 tolerance.
|
||||
vi.setSystemTime(baseMs + 3 * 30_000);
|
||||
expect(MfaService.verifyTotp(secret, code)).toBe(false);
|
||||
} finally {
|
||||
vi.useRealTimers();
|
||||
}
|
||||
});
|
||||
|
||||
it('still accepts a fresh code generated in the current window', () => {
|
||||
const secret = MfaService.generateSecret();
|
||||
const code = authenticator.generate(secret);
|
||||
expect(MfaService.verifyTotp(secret, code)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('MfaService.normalizeBackupCode canonicalisation', () => {
|
||||
it('canonicalises smart-dash and trailing whitespace to the hyphenless form', () => {
|
||||
// en-dash and em-dash variants a user may paste from a word processor
|
||||
expect(MfaService.normalizeBackupCode('abcde\u2013fghij ')).toBe('ABCDEFGHIJ');
|
||||
expect(MfaService.normalizeBackupCode('abcde\u2014fghij')).toBe('ABCDEFGHIJ');
|
||||
expect(MfaService.normalizeBackupCode(' ABCDE-FGHIJ\n')).toBe('ABCDEFGHIJ');
|
||||
});
|
||||
});
|
||||
|
||||
describe('POST /api/auth/login/mfa edge cases', () => {
|
||||
const password = 'edgepass12345';
|
||||
|
||||
async function challenge(username: string): Promise<string> {
|
||||
const res = await request(app).post('/api/auth/login').send({ username, password });
|
||||
return findCookie(res.headers, 'sencho_mfa_pending')!;
|
||||
}
|
||||
|
||||
it('rejects backup codes with invalid format without reaching the bcrypt path', async () => {
|
||||
const username = 'mfa-badformat';
|
||||
const { userId } = await seedMfaUser(username, password);
|
||||
const db = DatabaseService.getInstance();
|
||||
const pending = await challenge(username);
|
||||
|
||||
// Too short, non-alphanumeric garbage, and an 11-char alphanumeric that
|
||||
// matches no stored hash. All should produce 401 and increment the counter.
|
||||
const bad = ['12345', '!!!!!!!!!!!', 'ZZZZZZZZZZZ'];
|
||||
for (const code of bad) {
|
||||
const r = await request(app)
|
||||
.post('/api/auth/login/mfa')
|
||||
.set('Cookie', pending)
|
||||
.send({ code, isBackupCode: true });
|
||||
expect(r.status).toBe(401);
|
||||
}
|
||||
|
||||
const mfa = db.getUserMfa(userId)!;
|
||||
expect(mfa.failed_attempts).toBe(bad.length);
|
||||
});
|
||||
|
||||
it('clears failed_attempts on a successful verify after prior failures below the threshold', async () => {
|
||||
const username = 'mfa-reset-counter';
|
||||
const { userId, secret } = await seedMfaUser(username, password);
|
||||
const db = DatabaseService.getInstance();
|
||||
|
||||
// Seed three failed attempts (below the 5-failure lockout threshold).
|
||||
db.upsertUserMfa(userId, { failed_attempts: 3, locked_until: null });
|
||||
expect(db.getUserMfa(userId)!.failed_attempts).toBe(3);
|
||||
|
||||
const pending = await challenge(username);
|
||||
const ok = await request(app)
|
||||
.post('/api/auth/login/mfa')
|
||||
.set('Cookie', pending)
|
||||
.send({ code: authenticator.generate(secret) });
|
||||
expect(ok.status).toBe(200);
|
||||
|
||||
const after = db.getUserMfa(userId)!;
|
||||
expect(after.failed_attempts).toBe(0);
|
||||
expect(after.locked_until).toBeNull();
|
||||
});
|
||||
|
||||
it('lets a locked user sign in again once locked_until has passed', async () => {
|
||||
const username = 'mfa-lock-expired';
|
||||
const { userId, secret } = await seedMfaUser(username, password);
|
||||
const db = DatabaseService.getInstance();
|
||||
|
||||
// Simulate a stale lockout that has already expired.
|
||||
db.upsertUserMfa(userId, {
|
||||
failed_attempts: 5,
|
||||
locked_until: Date.now() - 60_000,
|
||||
});
|
||||
|
||||
const pending = await challenge(username);
|
||||
const ok = await request(app)
|
||||
.post('/api/auth/login/mfa')
|
||||
.set('Cookie', pending)
|
||||
.send({ code: authenticator.generate(secret) });
|
||||
expect(ok.status).toBe(200);
|
||||
|
||||
const after = db.getUserMfa(userId)!;
|
||||
expect(after.failed_attempts).toBe(0);
|
||||
expect(after.locked_until).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('MFA enrol/start overwrites a prior pending secret', () => {
|
||||
it('only the most recent enroll/start secret is valid on confirm', async () => {
|
||||
const username = 'mfa-overwrite';
|
||||
const password = 'overwritepass12345';
|
||||
// Create a plain user (no MFA seeded); we want to exercise the enrol path.
|
||||
const db = DatabaseService.getInstance();
|
||||
const bcryptMod = (await import('bcrypt')).default;
|
||||
const passwordHash = await bcryptMod.hash(password, 1);
|
||||
const userId = db.addUser({ username, password_hash: passwordHash, role: 'viewer' });
|
||||
|
||||
const user = db.getUser(userId)!;
|
||||
const token = jwt.sign(
|
||||
{ username, role: 'viewer', tv: user.token_version },
|
||||
TEST_JWT_SECRET,
|
||||
{ expiresIn: '1m' },
|
||||
);
|
||||
|
||||
const first = await request(app)
|
||||
.post('/api/auth/mfa/enroll/start')
|
||||
.set('Authorization', `Bearer ${token}`);
|
||||
expect(first.status).toBe(200);
|
||||
const firstSecret = first.body.secret as string;
|
||||
|
||||
const second = await request(app)
|
||||
.post('/api/auth/mfa/enroll/start')
|
||||
.set('Authorization', `Bearer ${token}`);
|
||||
expect(second.status).toBe(200);
|
||||
const secondSecret = second.body.secret as string;
|
||||
expect(secondSecret).not.toBe(firstSecret);
|
||||
|
||||
// First secret no longer verifies against the stored (now-overwritten) secret.
|
||||
const wrongCode = authenticator.generate(firstSecret);
|
||||
const rejected = await request(app)
|
||||
.post('/api/auth/mfa/enroll/confirm')
|
||||
.set('Authorization', `Bearer ${token}`)
|
||||
.send({ code: wrongCode });
|
||||
// The rejected code may still happen to equal the new secret's current
|
||||
// code (1-in-a-million), so retry with the second secret on a clean run.
|
||||
if (rejected.status === 200) {
|
||||
// Extremely unlikely collision; the assertion proves the overwrite
|
||||
// path at least did not reject a valid-for-secondSecret code.
|
||||
expect(rejected.body.backupCodes).toHaveLength(10);
|
||||
return;
|
||||
}
|
||||
expect(rejected.status).toBe(401);
|
||||
|
||||
// Second secret verifies on confirm.
|
||||
const ok = await request(app)
|
||||
.post('/api/auth/mfa/enroll/confirm')
|
||||
.set('Authorization', `Bearer ${token}`)
|
||||
.send({ code: authenticator.generate(secondSecret) });
|
||||
expect(ok.status).toBe(200);
|
||||
expect(ok.body.backupCodes).toHaveLength(10);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user