fix(backend): reject a replayed TOTP code within its validity window (#1389)

* fix(backend): reject a replayed TOTP code within its validity window

verifyTotp() was stateless — otplib's window:1 tolerance meant the
same 6-digit code could complete two independent logins inside its
~90s validity window. Track each admin's last-consumed step and
reject a code that doesn't advance past it.

* fix(backend): make the TOTP replay-tracking persist atomic

verifyTotpEncryptedStep() read two_factor_last_used_step, then a plain
UPDATE wrote the new step with no conditional guard — two concurrent
requests carrying the same captured code could both pass the check
before either UPDATE landed. The persist is now a conditional UPDATE
(only advances the step, checked via affected-row count), so a losing
concurrent request is correctly treated as a replay.

---------

Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
This commit is contained in:
Paul Nothaft
2026-09-11 10:40:27 +02:00
committed by GitHub
parent e3247911a0
commit cdde937d7f
7 changed files with 426 additions and 13 deletions
+213
View File
@@ -38,6 +38,7 @@ const { authenticator } = require('otplib');
const {
bootCrmDb, mintAdminToken, buildRouteApp,
} = require('../integration/helpers/crmDb');
const mfaService = require('../../src/services/mfaService');
jest.setTimeout(120000);
@@ -235,6 +236,138 @@ describe('MFA disable — /api/admin/auth/mfa/disable', () => {
expect(row.two_factor_secret).toBeNull();
expect(row.two_factor_recovery_codes).toBeNull();
});
// Concurrency regression: a plain UPDATE with no conditional guard let two
// requests carrying the same captured code both read the same
// two_factor_last_used_step and both persist, defeating replay protection.
// The guarded UPDATE (mfaService.persistTotpStep) makes only the first
// writer's affected-row count > 0; the loser must be rejected.
it('two concurrent disable requests with the SAME captured code: only one succeeds', async () => {
const admin = await seedAdmin();
const { secret, token } = await enroll(admin.id);
const code = authenticator.generate(secret);
const [r1, r2] = await Promise.all([
request(adminApp)
.post('/api/admin/auth/mfa/disable')
.set('Authorization', `Bearer ${token}`)
.send({ code }),
request(adminApp)
.post('/api/admin/auth/mfa/disable')
.set('Authorization', `Bearer ${token}`)
.send({ code }),
]);
expect([r1.status, r2.status].sort()).toEqual([200, 400]);
const status = await request(adminApp)
.get('/api/admin/auth/mfa/status')
.set('Authorization', `Bearer ${token}`);
expect(status.body.enabled).toBe(false);
});
});
describe('MFA regenerate recovery codes — /api/admin/auth/mfa/recovery-codes', () => {
it('a valid TOTP regenerates the recovery codes and persists the step', async () => {
const admin = await seedAdmin();
const { secret, token } = await enroll(admin.id);
const res = await request(adminApp)
.post('/api/admin/auth/mfa/recovery-codes')
.set('Authorization', `Bearer ${token}`)
.send({ code: authenticator.generate(secret) });
expect(res.status).toBe(200);
expect(res.body.recoveryCodes).toHaveLength(10);
});
it('a wrong code is rejected (400)', async () => {
const admin = await seedAdmin();
const { secret, token } = await enroll(admin.id);
const valid = authenticator.generate(secret);
const wrong = valid === '000000' ? '111111' : '000000';
const res = await request(adminApp)
.post('/api/admin/auth/mfa/recovery-codes')
.set('Authorization', `Bearer ${token}`)
.send({ code: wrong });
expect(res.status).toBe(400);
});
// Concurrency regression (see the disable test above for the mechanism):
// this is the endpoint called out as the worst lost-update case, since it
// both rotates the recovery codes and (previously) persisted the step in
// one unconditional UPDATE.
it('two concurrent regenerations with the SAME captured code: only one succeeds', async () => {
const admin = await seedAdmin();
const { secret, token } = await enroll(admin.id);
const code = authenticator.generate(secret);
const [r1, r2] = await Promise.all([
request(adminApp)
.post('/api/admin/auth/mfa/recovery-codes')
.set('Authorization', `Bearer ${token}`)
.send({ code }),
request(adminApp)
.post('/api/admin/auth/mfa/recovery-codes')
.set('Authorization', `Bearer ${token}`)
.send({ code }),
]);
expect([r1.status, r2.status].sort()).toEqual([200, 400]);
const winner = r1.status === 200 ? r1 : r2;
expect(winner.body.recoveryCodes).toHaveLength(10);
const row = await db('admin_users').where({ id: admin.id }).first();
expect(row.two_factor_last_used_step).not.toBeNull();
});
});
describe('mfaService.persistTotpStep — atomic replay-tracking persist', () => {
// Deterministic simulation of the race: two "concurrent" requests that
// read the SAME two_factor_last_used_step and computed the SAME totpStep
// from the same captured code. Calling persistTotpStep twice in a row with
// that identical totpStep reproduces exactly the DB-level outcome of a
// true race, without relying on event-loop timing.
it('the second writer with the same totpStep affects 0 rows and is rejected', async () => {
const admin = await seedAdmin();
const { secret } = await enroll(admin.id);
const row = await db('admin_users').where({ id: admin.id }).first();
const code = authenticator.generate(secret);
const totpStep = mfaService.verifyTotpEncryptedStep(code, row.two_factor_secret, null);
expect(totpStep).toEqual(expect.any(Number));
const first = await mfaService.persistTotpStep(db, admin.id, totpStep, { updated_at: new Date() });
expect(first).toBe(true);
// The row's two_factor_last_used_step has now already advanced to
// totpStep by the time this "losing" write runs — the guard condition
// (whereNull OR < totpStep) is false, so 0 rows are affected.
const second = await mfaService.persistTotpStep(db, admin.id, totpStep, { updated_at: new Date() });
expect(second).toBe(false);
const after = await db('admin_users').where({ id: admin.id }).first();
expect(Number(after.two_factor_last_used_step)).toBe(totpStep);
});
it('succeeds when the new step advances past the current one', async () => {
const admin = await seedAdmin();
const { secret } = await enroll(admin.id);
const row = await db('admin_users').where({ id: admin.id }).first();
const code = authenticator.generate(secret);
const totpStep = mfaService.verifyTotpEncryptedStep(code, row.two_factor_secret, null);
const ok = await mfaService.persistTotpStep(db, admin.id, totpStep, {});
expect(ok).toBe(true);
const nextStepAuthenticator = authenticator.clone({ epoch: Date.now() + 30000 });
const nextCode = nextStepAuthenticator.generate(secret);
const nextStep = mfaService.verifyTotpEncryptedStep(nextCode, row.two_factor_secret, totpStep);
expect(nextStep).toBeGreaterThan(totpStep);
const advanced = await mfaService.persistTotpStep(db, admin.id, nextStep, {});
expect(advanced).toBe(true);
});
});
describe('Admin login challenge — /api/auth/admin/login[/mfa]', () => {
@@ -284,6 +417,86 @@ describe('Admin login challenge — /api/auth/admin/login[/mfa]', () => {
expect(res.body.user.id).toBe(admin.id);
});
// GHSA-qcwx-r25m-j869: verifyTotp() was stateless, so otplib's window:1
// tolerance let the same 6-digit code complete two independent logins
// within its ~90s validity window. mfaService now tracks each admin's
// last-consumed TOTP step and rejects a code that doesn't advance past it.
it('#GHSA-qcwx-r25m-j869 — a TOTP code cannot be replayed into a second login', async () => {
const admin = await seedAdmin();
const { secret } = await enroll(admin.id);
const code = authenticator.generate(secret);
// First use of the code completes a login.
const c1 = await request(authApp)
.post('/api/auth/admin/login')
.send({ username: admin.username, password: admin.password });
const first = await request(authApp)
.post('/api/auth/admin/login/mfa')
.send({ mfaToken: c1.body.mfaToken, code });
expect(first.status).toBe(200);
expect(first.body.user).toBeDefined();
// Replaying the SAME code for an independent second login must fail,
// even though otplib's window:1 tolerance still considers it valid.
const c2 = await request(authApp)
.post('/api/auth/admin/login')
.send({ username: admin.username, password: admin.password });
const replay = await request(authApp)
.post('/api/auth/admin/login/mfa')
.send({ mfaToken: c2.body.mfaToken, code });
expect(replay.status).toBe(401);
expect(replay.body.code).toBe('MFA_INVALID');
expect(replay.body.user).toBeUndefined();
// A freshly generated code for the NEXT TOTP step is not a replay and
// succeeds. Generated via a cloned authenticator with a future epoch
// rather than mocking Date.now(), so mfaService's own step computation
// (real Date.now()) still lands the match one step ahead.
const nextStepAuthenticator = authenticator.clone({ epoch: Date.now() + 30000 });
const nextCode = nextStepAuthenticator.generate(secret);
const c3 = await request(authApp)
.post('/api/auth/admin/login')
.send({ username: admin.username, password: admin.password });
const third = await request(authApp)
.post('/api/auth/admin/login/mfa')
.send({ mfaToken: c3.body.mfaToken, code: nextCode });
expect(third.status).toBe(200);
expect(third.body.user).toBeDefined();
expect(third.body.user.id).toBe(admin.id);
});
// Concurrency regression: verifyTotpEncryptedStep()'s "does this advance"
// check was read against a snapshot taken earlier in the request, then a
// PLAIN update persisted the step — two concurrent requests carrying the
// SAME captured code could both pass the check and both complete a login
// before either write landed. The persist is now a conditional UPDATE
// (mfaService.persistTotpStep), so only the first writer's affected-row
// count is > 0 and the other is correctly treated as a replay.
it('two concurrent login/mfa requests with the SAME captured code: only one completes', async () => {
const admin = await seedAdmin();
const { secret } = await enroll(admin.id);
const code = authenticator.generate(secret);
const c1 = await request(authApp)
.post('/api/auth/admin/login')
.send({ username: admin.username, password: admin.password });
const c2 = await request(authApp)
.post('/api/auth/admin/login')
.send({ username: admin.username, password: admin.password });
const [r1, r2] = await Promise.all([
request(authApp).post('/api/auth/admin/login/mfa').send({ mfaToken: c1.body.mfaToken, code }),
request(authApp).post('/api/auth/admin/login/mfa').send({ mfaToken: c2.body.mfaToken, code }),
]);
expect([r1.status, r2.status].sort()).toEqual([200, 401]);
const winner = r1.status === 200 ? r1 : r2;
const loser = r1.status === 200 ? r2 : r1;
expect(winner.body.user).toBeDefined();
expect(loser.body.user).toBeUndefined();
expect(loser.body.code).toBe('MFA_INVALID');
});
it('login/mfa with a wrong code is 401 MFA_INVALID', async () => {
const admin = await seedAdmin();
const { secret } = await enroll(admin.id);
@@ -92,6 +92,52 @@ describe('mfaService — TOTP verification', () => {
});
});
describe('mfaService — replay protection (GHSA-qcwx-r25m-j869)', () => {
it('verifyTotp accepts a code once and rejects the same code as a replay', () => {
const secret = mfaService.generateSecret();
const code = authenticator.generate(secret);
// First use: no lastUsedStep yet, so it's accepted.
expect(mfaService.verifyTotp(code, secret)).toBe(true);
// Simulate persisting the matched step and replaying the same code: the
// matched step must strictly advance past lastUsedStep, so this fails.
const step = mfaService.currentTotpStep();
expect(mfaService.verifyTotp(code, secret, step)).toBe(false);
// A lastUsedStep the code hasn't caught up to yet also rejects it.
expect(mfaService.verifyTotp(code, secret, step + 1)).toBe(false);
});
it('verifyTotpEncryptedStep returns the matched step on success and null on replay', () => {
const secret = mfaService.generateSecret();
const stored = mfaService.encryptSecret(secret);
const code = authenticator.generate(secret);
const step = mfaService.verifyTotpEncryptedStep(code, stored, null);
expect(step).toEqual(expect.any(Number));
expect(step).toBeGreaterThan(0);
// Replaying the same code against the just-persisted step is rejected.
expect(mfaService.verifyTotpEncryptedStep(code, stored, step)).toBeNull();
});
it('a freshly generated code for the next TOTP step is accepted after a replay is rejected', () => {
const secret = mfaService.generateSecret();
const code = authenticator.generate(secret);
const step = mfaService.verifyTotpEncryptedStep(code, mfaService.encryptSecret(secret), null)
|| mfaService.currentTotpStep();
// Same-step replay: rejected.
expect(mfaService.verifyTotp(code, secret, step)).toBe(false);
// A code minted for the next step (via a cloned authenticator with a
// future epoch, not by mocking Date.now()) advances past last_used_step.
const nextStepAuthenticator = authenticator.clone({ epoch: Date.now() + 30000 });
const nextCode = nextStepAuthenticator.generate(secret);
expect(mfaService.verifyTotp(nextCode, secret, step)).toBe(true);
});
});
describe('mfaService — otpauth URI / QR', () => {
it('builds an otpauth:// URI containing issuer, account and secret', () => {
const secret = mfaService.generateSecret();
@@ -0,0 +1,29 @@
/**
* Migration 213: TOTP replay protection for admin MFA (GHSA-qcwx-r25m-j869).
*
* verifyTotp()/verifyTotpEncrypted() were stateless: otplib's window:1
* tolerance means a captured 6-digit code stays valid across several real
* time-steps (~90s), so the same code could complete two independent admin
* logins. `two_factor_last_used_step` tracks, per admin, the absolute TOTP
* time-step (Math.floor(Date.now() / 30000)) that their last successfully
* consumed code matched; mfaService now rejects a code whose matched step
* doesn't advance past it.
*
* Additive and idempotent: only adds a column, guarded by hasColumn, so it
* is safe to re-run and touches no existing data.
*/
exports.up = async function (knex) {
if (!(await knex.schema.hasColumn('admin_users', 'two_factor_last_used_step'))) {
await knex.schema.alterTable('admin_users', (t) => {
t.integer('two_factor_last_used_step').nullable();
});
}
};
exports.down = async function (knex) {
if (await knex.schema.hasColumn('admin_users', 'two_factor_last_used_step')) {
await knex.schema.alterTable('admin_users', (t) => {
t.dropColumn('two_factor_last_used_step');
});
}
};
+1
View File
@@ -36,6 +36,7 @@ const MFA_CLEAR = {
two_factor_secret: null,
two_factor_recovery_codes: null,
two_factor_enrolled_at: null,
two_factor_last_used_step: null,
updated_at: new Date(),
};
+27 -2
View File
@@ -264,6 +264,11 @@ router.post('/mfa/setup', adminAuth, handleAsync(async (req, res) => {
// Complete enrollment: verify a code against the provisional secret, enable
// MFA, and return one-time recovery codes (shown exactly once).
//
// No replay tracking here: this confirms an already-authenticated session
// still holds the authenticator (no new session is granted), and starting
// the last-used-step counter here would reject the very next login if it
// lands in the same 30s TOTP step as this call.
router.post('/mfa/enable', [
adminAuth,
body('code').notEmpty().withMessage('Verification code is required')
@@ -314,7 +319,17 @@ router.post('/mfa/disable', [
throw new ValidationError('Two-factor authentication is not enabled');
}
const totpOk = mfaService.verifyTotpEncrypted(req.body.code, admin.two_factor_secret);
// Persist the matched step atomically right here (see mfaService.persistTotpStep):
// two concurrent requests carrying the same captured code can't both read the
// same last-used step and both win — only the first writer's UPDATE affects a
// row, so a losing concurrent request is correctly treated as invalid below.
const totpStep = mfaService.verifyTotpEncryptedStep(
req.body.code, admin.two_factor_secret, admin.two_factor_last_used_step
);
let totpOk = false;
if (totpStep !== null) {
totpOk = await mfaService.persistTotpStep(db, admin.id, totpStep, { updated_at: new Date() });
}
let recoveryOk = false;
if (!totpOk) {
const stored = mfaService.parseRecoveryCodes(admin.two_factor_recovery_codes);
@@ -329,6 +344,7 @@ router.post('/mfa/disable', [
two_factor_secret: null,
two_factor_recovery_codes: null,
two_factor_enrolled_at: null,
two_factor_last_used_step: null,
updated_at: new Date()
});
@@ -353,7 +369,16 @@ router.post('/mfa/recovery-codes', [
if (!isMfaEnabled(admin)) {
throw new ValidationError('Two-factor authentication is not enabled');
}
if (!mfaService.verifyTotpEncrypted(req.body.code, admin.two_factor_secret)) {
// Persist the matched step atomically right here (see mfaService.persistTotpStep):
// two concurrent requests carrying the same captured code can't both read the
// same last-used step and both win — only the first writer's UPDATE affects a
// row, so a losing concurrent request is correctly treated as invalid below.
const totpStep = mfaService.verifyTotpEncryptedStep(
req.body.code, admin.two_factor_secret, admin.two_factor_last_used_step
);
const totpOk = totpStep !== null
&& await mfaService.persistTotpStep(db, admin.id, totpStep, { updated_at: new Date() });
if (!totpOk) {
throw new ValidationError('Invalid verification code');
}
+17 -2
View File
@@ -298,8 +298,22 @@ router.post('/admin/login/mfa', [
return res.status(401).json({ error: getGenericAuthError() });
}
// TOTP first, then a one-time recovery code.
let ok = mfaService.verifyTotpEncrypted(code, admin.two_factor_secret);
// TOTP first, then a one-time recovery code. verifyTotpEncryptedStep also
// enforces replay protection (GHSA-qcwx-r25m-j869): a code whose matched
// step doesn't advance past this admin's two_factor_last_used_step is
// rejected, so the same code can't complete two logins. The step is
// persisted atomically (persistTotpStep) right here, immediately after a
// match, so two concurrent requests carrying the same captured code
// can't both read the same last-used step and both win — only the first
// writer's UPDATE affects a row; the loser falls through and is treated
// as a replay below.
const totpStep = mfaService.verifyTotpEncryptedStep(
code, admin.two_factor_secret, admin.two_factor_last_used_step
);
let ok = false;
if (totpStep !== null) {
ok = await mfaService.persistTotpStep(db, admin.id, totpStep, { updated_at: new Date() });
}
let usedRecovery = false;
let remainingHashes = null;
if (!ok) {
@@ -328,6 +342,7 @@ router.post('/admin/login/mfa', [
{ type: 'admin', id: admin.id, name: admin.username }
);
}
// else: the TOTP step was already persisted atomically above.
await logActivity('admin_mfa_login',
{ admin_id: admin.id, method: usedRecovery ? 'recovery_code' : 'totp' },
+93 -9
View File
@@ -3,7 +3,11 @@
*
* Responsibilities:
* - generate/verify TOTP secrets (otplib, standard SHA1/6-digit/30s so
* Google Authenticator / Authy / 1Password all work);
* Google Authenticator / Authy / 1Password all work), with replay
* protection: verifyTotpEncryptedStep() rejects a code whose matched
* time-step doesn't advance past the admin's last consumed one
* (GHSA-qcwx-r25m-j869 — otplib's window:1 tolerance alone lets a
* captured code stay valid across several time-steps, ~90s);
* - encrypt the secret at rest (AES-256-GCM) so a DB leak alone doesn't
* yield working authenticator seeds;
* - generate/verify one-time recovery codes, hashed (bcrypt) and single-use;
@@ -67,25 +71,102 @@ function decryptSecret(stored) {
return pt.toString('utf8');
}
/** Verify a 6-digit TOTP code against the (plaintext) secret. */
function verifyTotp(code, plainSecret) {
if (!code || !plainSecret) return false;
/** Absolute TOTP time-step for "now" (Math.floor(Date.now() / 30000)). */
function currentTotpStep() {
return Math.floor(Date.now() / 30000);
}
/**
* Core TOTP check. Returns the matched absolute time-step (always a
* positive, truthy integer) when `code` is valid for `plainSecret`;
* otherwise `null`.
*
* When `lastUsedStep` is given, a code whose matched step doesn't advance
* past it is treated as invalid — replay protection. Without this, otplib's
* window:1 tolerance lets a captured code stay valid across several real
* time-steps (~90s), so the same code could complete two independent admin
* logins (GHSA-qcwx-r25m-j869).
*/
function matchTotpStep(code, plainSecret, lastUsedStep) {
if (!code || !plainSecret) return null;
try {
return authenticator.verify({ token: String(code).replace(/\s+/g, ''), secret: plainSecret });
const token = String(code).replace(/\s+/g, '');
const delta = authenticator.checkDelta(token, plainSecret);
if (typeof delta !== 'number') return null;
const step = currentTotpStep() + delta;
if (typeof lastUsedStep === 'number' && step <= lastUsedStep) return null;
return step;
} catch {
return null;
}
}
/**
* Verify a 6-digit TOTP code against the (plaintext) secret. Pass
* `lastUsedStep` (the admin's previously-consumed step) to also enforce
* replay protection — see matchTotpStep().
*/
function verifyTotp(code, plainSecret, lastUsedStep) {
return matchTotpStep(code, plainSecret, lastUsedStep) !== null;
}
/** Verify a code against a STORED (encrypted) secret. */
function verifyTotpEncrypted(code, storedSecret, lastUsedStep) {
try {
return verifyTotp(code, decryptSecret(storedSecret), lastUsedStep);
} catch {
return false;
}
}
/** Verify a code against a STORED (encrypted) secret. */
function verifyTotpEncrypted(code, storedSecret) {
/**
* Like verifyTotpEncrypted(), but returns the matched step (or `null` when
* the code is invalid/replayed) instead of a boolean, so a caller that
* grants a session or a sensitive action can persist it as the admin's new
* `two_factor_last_used_step`.
*/
function verifyTotpEncryptedStep(code, storedSecret, lastUsedStep) {
try {
return verifyTotp(code, decryptSecret(storedSecret));
return matchTotpStep(code, decryptSecret(storedSecret), lastUsedStep);
} catch {
return false;
return null;
}
}
/**
* Persist a newly-matched TOTP step, but only if it still advances
* `two_factor_last_used_step` at write time (`db('admin_users').where('id',
* adminId).whereNull(...).orWhere(...).update(...)`).
*
* matchTotpStep()'s "does this advance past lastUsedStep" check is read
* against a snapshot taken earlier in the request. Two concurrent requests
* carrying the same captured code can both read the same lastUsedStep and
* both pass that check before either write lands — a plain, unconditional
* UPDATE would let both persist, defeating replay protection. Guarding the
* UPDATE with the same condition and checking the affected-row count makes
* only the first writer succeed; a losing concurrent request gets 0 affected
* rows and must be treated as a replay by the caller.
*
* @param {object} db - knex instance
* @param {number} adminId
* @param {number} totpStep - matched step from verifyTotpEncryptedStep()
* @param {object} [extraFields] - additional columns to set in the same UPDATE
* @returns {Promise<boolean>} true if this call won the race and persisted
*/
async function persistTotpStep(db, adminId, totpStep, extraFields = {}) {
const affected = await db('admin_users')
.where('id', adminId)
.where(function () {
this.whereNull('two_factor_last_used_step')
.orWhere('two_factor_last_used_step', '<', totpStep);
})
.update({
two_factor_last_used_step: totpStep,
...extraFields
});
return affected > 0;
}
/** otpauth:// URI for an authenticator app. */
function buildOtpauthUri(accountName, plainSecret) {
return authenticator.keyuri(accountName, ISSUER, plainSecret);
@@ -169,8 +250,11 @@ module.exports = {
generateSecret,
encryptSecret,
decryptSecret,
currentTotpStep,
verifyTotp,
verifyTotpEncrypted,
verifyTotpEncryptedStep,
persistTotpStep,
buildOtpauthUri,
buildQrDataUrl,
generateRecoveryCodes,