diff --git a/server/lib/login-lockout.js b/server/lib/login-lockout.js new file mode 100644 index 0000000..7204964 --- /dev/null +++ b/server/lib/login-lockout.js @@ -0,0 +1,45 @@ +'use strict'; + +// Per-ACCOUNT brute-force lockout for POST /api/auth/login. Same shape as +// lib/totp-lockout.js (#100) and lib/pair-lockout.js (#87) — deliberately, so there is one +// recognisable lockout idiom in this codebase rather than three. +// +// WHY per-account and not just per-IP: the existing login throttle in server.js is keyed on +// the client IP, which is the right control for a single noisy source but bounds nothing +// against a distributed one — and IP attribution is only as good as the deployment's proxy +// configuration. A counter tied to the account being attacked is independent of where the +// attempts come from. +// +// KEYED ON user.id, never on the submitted email. The email is attacker-supplied and +// unbounded, so keying on it would let anyone grow this Map without limit — the same +// mistake this campaign fixed elsewhere. A user id only exists for a real account, so the +// key space is bounded by the user table and needs no eviction sweep (matching +// totp-lockout, which is bounded the same way). +// +// In-memory; resets on restart. That is a deliberate trade, not an oversight: a restart +// forgiving an in-progress lockout is preferable to persisting one, and the per-IP limiter +// still applies across restarts. + +const MAX_FAILS = 10; // failed passwords before the account is locked +const LOCKOUT_MS = 15 * 60 * 1000; // how long it is then locked + +const failures = new Map(); // user.id -> { count, lockedUntil } + +function isLocked(key, now = Date.now()) { + const rec = failures.get(key); + return !!(rec && rec.lockedUntil > now); +} + +function recordFailure(key, now = Date.now()) { + const rec = failures.get(key) || { count: 0, lockedUntil: 0 }; + rec.count += 1; + if (rec.count >= MAX_FAILS) { rec.lockedUntil = now + LOCKOUT_MS; rec.count = 0; } + failures.set(key, rec); + return rec; +} + +// A correct password clears the key — including for accounts that then go on to a TOTP or +// email-verification step, since the password itself has been proven at that point. +function reset(key) { failures.delete(key); } + +module.exports = { isLocked, recordFailure, reset, MAX_FAILS, LOCKOUT_MS }; diff --git a/server/routes/auth.js b/server/routes/auth.js index 251791c..8d028fb 100644 --- a/server/routes/auth.js +++ b/server/routes/auth.js @@ -10,6 +10,7 @@ const { resolveTenancy } = require('../lib/tenancy'); const { logActivity, getClientIp } = require('../services/activity'); const totp = require('../lib/totp'); const totpLockout = require('../lib/totp-lockout'); +const loginLockout = require('../lib/login-lockout'); const QRCode = require('qrcode'); const { sendSignupEmails, sendVerificationEmail } = require('../services/signupEmails'); const emailVerify = require('../lib/emailVerify'); @@ -168,11 +169,30 @@ router.post('/login', (req, res) => { return res.status(401).json({ error: 'Invalid email or password' }); } + // Per-ACCOUNT brute-force lockout (lib/login-lockout), on top of the per-IP limiter in + // server.js. Checked BEFORE bcrypt so a locked account costs no hashing work. + // + // The response is deliberately IDENTICAL to a wrong password: a distinct 429 would tell + // an attacker "this account exists and is under attack", turning the endpoint into an + // account-existence oracle. The trade is that a locked-out legitimate user sees the + // generic message, so the trip is written to activity_log for the operator instead. + if (loginLockout.isLocked(user.id)) { + logFailedLogin(email, getClientIp(req), 'Locked out (too many failed passwords)'); + return res.status(401).json({ error: 'Invalid email or password' }); + } + if (!bcrypt.compareSync(password, user.password_hash)) { + const rec = loginLockout.recordFailure(user.id); + if (rec.lockedUntil) logActivity(null, 'auth:login_locked', `${email} - locked after repeated failures`, null, getClientIp(req)); logFailedLogin(email, getClientIp(req), 'Wrong password'); return res.status(401).json({ error: 'Invalid email or password' }); } + // Password proven. Clear the counter HERE rather than in issueSession: the TOTP and + // email-verification branches below return before issueSession is ever reached, so a + // reset placed there would never fire for those accounts. + loginLockout.reset(user.id); + // Email verification gate. Unverified LOCAL accounts are asked to confirm on login — this // covers both new signups AND existing users who predate the feature (grandfathered locals are // email_verified=0). Gated ONLY where we can actually send the mail (isConfigured), so an diff --git a/server/test/login-lockout.test.js b/server/test/login-lockout.test.js new file mode 100644 index 0000000..f071299 --- /dev/null +++ b/server/test/login-lockout.test.js @@ -0,0 +1,149 @@ +'use strict'; + +// Password login must be bounded per ACCOUNT, not only per IP. The IP limiter in server.js +// bounds one noisy source; it bounds nothing against a distributed one, and it is only as +// accurate as the deployment's proxy configuration. +// +// Two properties matter and are easy to get wrong: +// - a locked account must answer EXACTLY like a wrong password, or the endpoint becomes +// an account-existence oracle (a distinct 429 says "this account is real"); +// - a correct password must clear the counter immediately — including for accounts that +// then go on to a TOTP or email-verification step, which return before issueSession. + +const os = require('node:os'); +const path = require('node:path'); +const fs = require('node:fs'); +const crypto = require('node:crypto'); +const TMP = fs.mkdtempSync(path.join(os.tmpdir(), 'st-loginlock-')); +process.env.DATA_DIR = TMP; +process.env.SELF_HOSTED = 'true'; +process.env.NODE_ENV = 'test'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const lockout = require('../lib/login-lockout'); + +// --------------------------------------------------------------------------- +// Unit: the counter itself (mirrors test/pair-lockout.test.js — unique key per +// test, because the Map is module-level) +// --------------------------------------------------------------------------- +const k = () => 'user-' + crypto.randomBytes(6).toString('hex'); + +test('an account is not locked until it crosses the threshold', () => { + const key = k(); + for (let i = 0; i < lockout.MAX_FAILS - 1; i++) lockout.recordFailure(key); + assert.equal(lockout.isLocked(key), false, `${lockout.MAX_FAILS - 1} failures must not lock`); +}); + +test('crossing the threshold locks the account for the full window', () => { + const key = k(); + const t0 = 1_000_000; + for (let i = 0; i < lockout.MAX_FAILS; i++) lockout.recordFailure(key, t0); + assert.equal(lockout.isLocked(key, t0), true, 'locked at the threshold'); + assert.equal(lockout.isLocked(key, t0 + lockout.LOCKOUT_MS - 1), true, 'still locked inside the window'); + assert.equal(lockout.isLocked(key, t0 + lockout.LOCKOUT_MS + 1), false, 'released after the window'); +}); + +test('a correct password clears the counter', () => { + const key = k(); + for (let i = 0; i < lockout.MAX_FAILS - 1; i++) lockout.recordFailure(key); + lockout.reset(key); + for (let i = 0; i < lockout.MAX_FAILS - 1; i++) lockout.recordFailure(key); + assert.equal(lockout.isLocked(key), false, 'reset gave the account its full budget back'); +}); + +test('accounts are independent — one locked account does not lock another', () => { + const a = k(), b = k(); + for (let i = 0; i < lockout.MAX_FAILS; i++) lockout.recordFailure(a); + assert.equal(lockout.isLocked(a), true); + assert.equal(lockout.isLocked(b), false, 'a different account is unaffected'); +}); + +test('an unknown key is never locked', () => { + assert.equal(lockout.isLocked(k()), false); +}); + +// --------------------------------------------------------------------------- +// Route: the lockout is wired into POST /api/auth/login, and is not an oracle +// --------------------------------------------------------------------------- +const http = require('node:http'); +const express = require('express'); +const { db } = require('../db/database'); +const bcrypt = require('bcryptjs'); + +let server, base; +const PW = 'Passw0rd123'; + +function post(body) { + const data = JSON.stringify(body); + return new Promise((resolve, reject) => { + const req = http.request(base + '/login', { + method: 'POST', headers: { 'Content-Type': 'application/json', 'Content-Length': Buffer.byteLength(data) }, + }, (res) => { + let out = ''; + res.on('data', (c) => (out += c)); + res.on('end', () => resolve({ status: res.statusCode, body: out ? JSON.parse(out) : null })); + }); + req.on('error', reject); + req.end(data); + }); +} + +test('route: a locked account is indistinguishable from a wrong password', async (t) => { + const app = express(); + app.use(express.json()); + app.use('/', require('../routes/auth')); + server = http.createServer(app); + await new Promise((r) => server.listen(0, r)); + base = `http://127.0.0.1:${server.address().port}`; + t.after(() => new Promise((r) => server.close(r))); + + const email = 'lock' + crypto.randomBytes(5).toString('hex') + '@x.local'; + const id = crypto.randomUUID(); + db.prepare("INSERT INTO users (id, email, password_hash, auth_provider, plan_id, email_verified) VALUES (?,?,?,'local','free',1)") + .run(id, email, bcrypt.hashSync(PW, 10)); + + // Baseline: what a wrong password looks like. + const wrong = await post({ email, password: 'nope' }); + assert.equal(wrong.status, 401); + assert.equal(wrong.body.error, 'Invalid email or password'); + + // Drive it past the threshold. + for (let i = 0; i < lockout.MAX_FAILS + 2; i++) await post({ email, password: 'nope' }); + assert.equal(lockout.isLocked(id), true, 'the account is locked after repeated failures'); + + // The CORRECT password is now refused — and the response is byte-identical to a wrong + // one, so an attacker learns nothing about whether the account exists or is locked. + const locked = await post({ email, password: PW }); + assert.equal(locked.status, wrong.status, 'locked status must match the wrong-password status'); + assert.deepEqual(locked.body, wrong.body, 'locked body must match the wrong-password body exactly'); + + // A non-existent account still answers the same way. + const ghost = await post({ email: 'ghost' + crypto.randomBytes(4).toString('hex') + '@x.local', password: 'nope' }); + assert.equal(ghost.status, wrong.status); + assert.deepEqual(ghost.body, wrong.body, 'unknown account is indistinguishable too'); +}); + +test('route: a correct password clears the counter before any TOTP/verify step', async (t) => { + const app = express(); + app.use(express.json()); + app.use('/', require('../routes/auth')); + const srv = http.createServer(app); + await new Promise((r) => srv.listen(0, r)); + base = `http://127.0.0.1:${srv.address().port}`; + t.after(() => new Promise((r) => srv.close(r))); + + const email = 'clear' + crypto.randomBytes(5).toString('hex') + '@x.local'; + const id = crypto.randomUUID(); + db.prepare("INSERT INTO users (id, email, password_hash, auth_provider, plan_id, email_verified) VALUES (?,?,?,'local','free',1)") + .run(id, email, bcrypt.hashSync(PW, 10)); + + for (let i = 0; i < lockout.MAX_FAILS - 1; i++) await post({ email, password: 'nope' }); + const ok = await post({ email, password: PW }); + assert.equal(ok.status, 200, 'the correct password still logs in'); + assert.equal(lockout.isLocked(id), false); + + // Full budget restored: another MAX_FAILS-1 failures must still not lock. + for (let i = 0; i < lockout.MAX_FAILS - 1; i++) await post({ email, password: 'nope' }); + assert.equal(lockout.isLocked(id), false, 'the successful login reset the counter'); +});