From d0eeba6f9371ff61667d5680e3890667e27a8c08 Mon Sep 17 00:00:00 2001 From: jagdeep sidhu Date: Sat, 25 Apr 2026 16:07:28 -0700 Subject: [PATCH] fix(auth): rate limit authenticated step-up routes Apply a shared per-session limiter to password/TOTP step-up endpoints and require current-password proof when disabling TOTP. Made-with: Cursor --- lib/appFactory.js | 9 ++- middleware/rateLimit.js | 18 ++++-- middleware/rateLimit.test.js | 23 +++++--- routes/auth.js | 29 ++++++++-- tests/auth.routes.test.js | 107 ++++++++++++++++++++++++++++++++++- 5 files changed, 165 insertions(+), 21 deletions(-) diff --git a/lib/appFactory.js b/lib/appFactory.js index ad7793e..2e9bb6e 100644 --- a/lib/appFactory.js +++ b/lib/appFactory.js @@ -174,11 +174,15 @@ function mountAuthAndVault( } ) { if (!services.sessionMw) finalizeSessionMw(services); + const stepUpLimiter = disableRateLimit + ? rateLimiters.disabled() + : rateLimiters.authenticatedStepUpLimiter(); const limiters = disableRateLimit ? { login: rateLimiters.disabled(), mfaLogin: rateLimiters.disabled(), - verifyPassword: rateLimiters.disabled(), + stepUp: stepUpLimiter, + verifyPassword: stepUpLimiter, register: rateLimiters.disabled(), verifyEmail: rateLimiters.disabled(), vote: rateLimiters.disabled(), @@ -187,7 +191,8 @@ function mountAuthAndVault( : { login: rateLimiters.loginLimiter(), mfaLogin: rateLimiters.mfaLoginLimiter(), - verifyPassword: rateLimiters.verifyPasswordLimiter(), + stepUp: stepUpLimiter, + verifyPassword: stepUpLimiter, register: rateLimiters.registerLimiter(), verifyEmail: rateLimiters.verifyEmailLimiter(), vote: rateLimiters.voteLimiter(), diff --git a/middleware/rateLimit.js b/middleware/rateLimit.js index f80585f..bbd4a9e 100644 --- a/middleware/rateLimit.js +++ b/middleware/rateLimit.js @@ -52,13 +52,15 @@ function mfaLoginKey(req) { // pair; keying by session contains abuse to that stolen session instead of // letting it burn the whole user's account budget. The IP fallback keeps the // limiter safe if a future caller mounts it in the wrong order. -function verifyPasswordKey(req) { +function authenticatedStepUpKey(req) { const sessionId = req.session && req.session.id != null ? String(req.session.id) : null; - if (sessionId) return `verify-password|s${sessionId}`; - return `verify-password|ip|${ipBucket(req)}`; + if (sessionId) return `step-up|s${sessionId}`; + return `step-up|ip|${ipBucket(req)}`; } +const verifyPasswordKey = authenticatedStepUpKey; + function registerKey(req) { return `register|${ipBucket(req)}`; } @@ -129,18 +131,20 @@ function mfaLoginLimiter() { }); } -function verifyPasswordLimiter() { +function authenticatedStepUpLimiter() { return rateLimit({ windowMs: 15 * MINUTE, max: 10, standardHeaders: true, legacyHeaders: false, - keyGenerator: verifyPasswordKey, + keyGenerator: authenticatedStepUpKey, message: { error: 'too_many_attempts' }, - handler: trippedHandler('verify-password'), + handler: trippedHandler('authenticated-step-up'), }); } +const verifyPasswordLimiter = authenticatedStepUpLimiter; + function registerLimiter() { return rateLimit({ windowMs: 60 * MINUTE, @@ -206,6 +210,7 @@ function disabled() { module.exports = { loginLimiter, mfaLoginLimiter, + authenticatedStepUpLimiter, verifyPasswordLimiter, registerLimiter, verifyEmailLimiter, @@ -215,6 +220,7 @@ module.exports = { // Exported for direct unit testing. loginKey, mfaLoginKey, + authenticatedStepUpKey, verifyPasswordKey, registerKey, verifyEmailKey, diff --git a/middleware/rateLimit.test.js b/middleware/rateLimit.test.js index 2946ee5..45734ca 100644 --- a/middleware/rateLimit.test.js +++ b/middleware/rateLimit.test.js @@ -1,6 +1,7 @@ const { loginKey, mfaLoginKey, + authenticatedStepUpKey, verifyPasswordKey, registerKey, reconcileKey, @@ -71,27 +72,33 @@ describe('rate-limit key generators', () => { }); }); - describe('verifyPasswordKey', () => { + describe('authenticatedStepUpKey', () => { function mkSession(body, ip, session) { return { ...mk(body, ip, { id: 42 }), session }; } test('buckets by authenticated session.id when present', () => { - const a = verifyPasswordKey(mkSession({}, '1.2.3.4', { id: 100 })); - const b = verifyPasswordKey(mkSession({}, '9.9.9.9', { id: 100 })); + const a = authenticatedStepUpKey(mkSession({}, '1.2.3.4', { id: 100 })); + const b = authenticatedStepUpKey(mkSession({}, '9.9.9.9', { id: 100 })); expect(a).toBe(b); - expect(a).toBe('verify-password|s100'); + expect(a).toBe('step-up|s100'); }); test('two sessions for the same user get distinct password-check buckets', () => { - const a = verifyPasswordKey(mkSession({}, '1.2.3.4', { id: 100 })); - const b = verifyPasswordKey(mkSession({}, '1.2.3.4', { id: 200 })); + const a = authenticatedStepUpKey(mkSession({}, '1.2.3.4', { id: 100 })); + const b = authenticatedStepUpKey(mkSession({}, '1.2.3.4', { id: 200 })); expect(a).not.toBe(b); }); test('falls back to IP bucket when the user is missing', () => { - const a = verifyPasswordKey(mk({}, '1.2.3.4')); - expect(a).toBe('verify-password|ip|1.2.3.4'); + const a = authenticatedStepUpKey(mk({}, '1.2.3.4')); + expect(a).toBe('step-up|ip|1.2.3.4'); + }); + + test('verifyPasswordKey remains an alias for older imports', () => { + expect(verifyPasswordKey(mkSession({}, '1.2.3.4', { id: 100 }))).toBe( + authenticatedStepUpKey(mkSession({}, '1.2.3.4', { id: 100 })) + ); }); }); diff --git a/routes/auth.js b/routes/auth.js index 5ed4a76..30701a9 100644 --- a/routes/auth.js +++ b/routes/auth.js @@ -39,6 +39,10 @@ const TotpEnableSchema = TotpCodeSchema.extend({ oldAuthHash: HEX_32_SCHEMA, }); +const TotpDisableSchema = TotpCodeSchema.extend({ + oldAuthHash: HEX_32_SCHEMA, +}); + // PR 7 — password change now rotates the vault wrap atomically. // // The client re-derives the new vaultKey from (newPassword, email, @@ -144,8 +148,10 @@ function createAuthRouter({ } const effectiveLimiters = { ...limiters, - verifyPassword: - limiters && typeof limiters.verifyPassword === 'function' + stepUp: + limiters && typeof limiters.stepUp === 'function' + ? limiters.stepUp + : limiters && typeof limiters.verifyPassword === 'function' ? limiters.verifyPassword : noopMiddleware, }; @@ -650,6 +656,7 @@ function createAuthRouter({ '/totp/setup', sessionMw.requireAuth, csrfMw.require, + effectiveLimiters.stepUp, (req, res) => { if (!totp) return res.status(503).json({ error: 'server_misconfigured' }); const parsed = TotpSetupSchema.safeParse(req.body); @@ -676,6 +683,7 @@ function createAuthRouter({ '/totp/enable', sessionMw.requireAuth, csrfMw.require, + effectiveLimiters.stepUp, (req, res) => { if (!totp) return res.status(503).json({ error: 'server_misconfigured' }); const parsed = TotpEnableSchema.safeParse(req.body); @@ -712,10 +720,21 @@ function createAuthRouter({ '/totp/disable', sessionMw.requireAuth, csrfMw.require, + effectiveLimiters.stepUp, (req, res) => { if (!totp) return res.status(503).json({ error: 'server_misconfigured' }); - const parsed = TotpCodeSchema.safeParse(req.body); + const parsed = TotpDisableSchema.safeParse(req.body); if (!parsed.success) return badRequest(res, 'invalid_body'); + if ( + !verifyPasswordStepUp( + req, + res, + parsed.data.oldAuthHash, + 'auth.totp_disable' + ) + ) { + return undefined; + } try { totp.verifyUserCode(req.user.id, parsed.data.code); totp.disable(req.user.id); @@ -742,6 +761,7 @@ function createAuthRouter({ '/change-password', sessionMw.requireAuth, csrfMw.require, + effectiveLimiters.stepUp, asyncHandler(async (req, res) => { const parsed = ChangePasswordSchema.safeParse(req.body); if (!parsed.success) return badRequest(res, 'invalid_body'); @@ -938,7 +958,7 @@ function createAuthRouter({ '/verify-password', sessionMw.requireAuth, csrfMw.require, - effectiveLimiters.verifyPassword, + effectiveLimiters.stepUp, (req, res) => { const parsed = VerifyPasswordSchema.safeParse(req.body); if (!parsed.success) return badRequest(res, 'invalid_body'); @@ -996,6 +1016,7 @@ function createAuthRouter({ '/account', sessionMw.requireAuth, csrfMw.require, + effectiveLimiters.stepUp, asyncHandler(async (req, res) => { const parsed = DeleteAccountSchema.safeParse(req.body); if (!parsed.success) return badRequest(res, 'invalid_body'); diff --git a/tests/auth.routes.test.js b/tests/auth.routes.test.js index 086770e..ba2543f 100644 --- a/tests/auth.routes.test.js +++ b/tests/auth.routes.test.js @@ -613,6 +613,57 @@ describe('auth routes', () => { expect(reuse.body.error).toBe('invalid_totp_code'); }); + test('TOTP disable requires current password step-up and a valid authenticator code', async () => { + const { agent, csrf } = await registerAndLogin(ctx); + const setup = await agent + .post('/auth/totp/setup') + .set('X-CSRF-Token', csrf) + .send({ oldAuthHash: SAMPLE_AUTH }); + await agent + .post('/auth/totp/enable') + .set('X-CSRF-Token', csrf) + .send({ + code: generateTotpCode(setup.body.secret), + oldAuthHash: SAMPLE_AUTH, + }); + + const missingPassword = await agent + .post('/auth/totp/disable') + .set('X-CSRF-Token', csrf) + .send({ code: generateTotpCode(setup.body.secret) }); + expect(missingPassword.status).toBe(400); + + const wrongPassword = await agent + .post('/auth/totp/disable') + .set('X-CSRF-Token', csrf) + .send({ + code: generateTotpCode(setup.body.secret), + oldAuthHash: 'deadbeef'.repeat(8), + }); + expect(wrongPassword.status).toBe(401); + expect(wrongPassword.body.error).toBe('invalid_credentials'); + + const wrongCode = await agent + .post('/auth/totp/disable') + .set('X-CSRF-Token', csrf) + .send({ code: '000000', oldAuthHash: SAMPLE_AUTH }); + expect(wrongCode.status).toBe(400); + expect(wrongCode.body.error).toBe('invalid_totp_code'); + + const good = await agent + .post('/auth/totp/disable') + .set('X-CSRF-Token', csrf) + .send({ + code: generateTotpCode(setup.body.secret), + oldAuthHash: SAMPLE_AUTH, + }); + expect(good.status).toBe(200); + expect(good.body.status).toBe('disabled'); + + const me = await agent.get('/auth/me'); + expect(me.body.user.totpEnabled).toBe(false); + }); + test('POST /auth/change-password (with vault): updates both auth AND vault atomically', async () => { const { agent, csrf } = await registerAndLogin(ctx); @@ -1576,7 +1627,7 @@ describe('createAuthRouter factory contract (Codex round-2 P3)', () => { }, sessionMw: { requireAuth: mw, parse: mw, setSessionCookie: noop, clearSessionCookie: noop }, csrfMw: { require: mw, parse: mw, issueCookie: noop, clearCookie: noop }, - limiters: { login: mw, mfaLogin: mw, verifyPassword: mw, register: mw, verifyEmail: mw, vote: mw }, + limiters: { login: mw, mfaLogin: mw, stepUp: mw, verifyPassword: mw, register: mw, verifyEmail: mw, vote: mw }, baseUrl: 'http://api.test', frontendUrl: 'http://app.test', scheduler: (fn) => fn(), @@ -1628,6 +1679,60 @@ describe('createAuthRouter factory contract (Codex round-2 P3)', () => { ).not.toThrow(); }); + test('mounts the shared step-up limiter on sensitive authenticated routes', async () => { + const express = require('express'); + const stepUp = jest.fn((_req, res) => + res.status(429).json({ error: 'step_up_limited' }) + ); + const mw = (_req, _res, next) => next(); + const app = express(); + app.use(express.json()); + app.use( + '/auth', + createAuthRouter( + buildArgs({ + sessionMw: { + requireAuth: (req, _res, next) => { + req.user = { id: 1, email: 'user@example.com' }; + req.session = { id: 10 }; + next(); + }, + parse: mw, + setSessionCookie: () => {}, + clearSessionCookie: () => {}, + }, + limiters: { + login: mw, + mfaLogin: mw, + stepUp, + register: mw, + verifyEmail: mw, + vote: mw, + }, + }) + ) + ); + + const cases = [ + request(app).post('/auth/verify-password').send({ authHash: SAMPLE_AUTH }), + request(app).post('/auth/totp/setup').send({ oldAuthHash: SAMPLE_AUTH }), + request(app) + .post('/auth/totp/enable') + .send({ code: '123456', oldAuthHash: SAMPLE_AUTH }), + request(app) + .post('/auth/totp/disable') + .send({ code: '123456', oldAuthHash: SAMPLE_AUTH }), + request(app) + .post('/auth/change-password') + .send({ oldAuthHash: SAMPLE_AUTH, newAuthHash: 'b'.repeat(64) }), + request(app).delete('/auth/account').send({ oldAuthHash: SAMPLE_AUTH }), + ]; + + const results = await Promise.all(cases); + expect(results.map((r) => r.status)).toEqual([429, 429, 429, 429, 429, 429]); + expect(stepUp).toHaveBeenCalledTimes(6); + }); + test('still rejects missing runAtomic regardless of vaults', () => { expect(() => createAuthRouter(buildArgs({ vaults: undefined, runAtomic: undefined }))