diff --git a/docs/multi-user-plan.md b/docs/multi-user-plan.md index c8ae1cdc..f44902ff 100644 --- a/docs/multi-user-plan.md +++ b/docs/multi-user-plan.md @@ -105,7 +105,7 @@ Keep the existing single-user branch untouched. Add a parallel multi-user branch 1. **Credential check**: Basic header parsed into `username:password`, verified against the user store (scrypt + `timingSafeEqual`). Disabled users fail closed. 2. **Cookie sessions**: same `codeman_session` cookie and `StaleExpirationMap`, but `AuthSessionRecord` gains `username` and `role`. All existing TTL/sliding/eviction logic reused. Eviction cap becomes per-user aware (evict oldest _of that user_ first) so one user cannot flush everyone's sessions by logging in 100 times. 3. **Request identity**: decorate `req.authUser = { username, role }` (Fastify decorateRequest). In single-user mode `req.authUser` is `{ username: 'admin', role: 'admin' }` when auth is on, and a synthetic admin when auth is off, so downstream code has ONE code path. -4. **Rate limiting**: keep the per-IP bucket; add a per-username failure bucket (same `StaleExpirationMap` pattern) so a botnet cannot brute-force one account across IPs, and one flaky user behind a NAT cannot lock out the rest. +4. **Rate limiting**: keep the per-IP bucket; add a per-username failure bucket (same `StaleExpirationMap` pattern) so one flaky user behind a NAT cannot lock out the rest. As built, the per-username bucket is checked after the verify: it answers 429 to failed guesses once full and never denies a correct password, so it does not cap verified guesses. What caps them is a per-(username, IP) bucket checked before the verify (10 per pair), beside a per-IP pre-gate of 50 (#684). 5. **`mustChangePassword` gate**: when set, every API request except `GET /api/me`, `POST /api/me/password`, and static assets returns 403 with `errorCode: 'PASSWORD_CHANGE_REQUIRED'`; the frontend intercepts that code and shows the change-password modal. 6. **Password change vs Basic-auth caching**: browsers cache Basic credentials. After a password change we revoke all of that user's cookie sessions; the next request falls to Basic with stale creds, gets 401, and the browser re-prompts. Acceptable for v1; a proper login form is Phase 6 (see 15). 7. **Unchanged**: hook-secret loopback bypass (hooks authenticate the _instance_, not a user; the event maps to a session which has an owner), host guard, Origin/CSRF guard, security headers. diff --git a/src/web/middleware/auth.ts b/src/web/middleware/auth.ts index 0d63dac1..08d8e573 100644 --- a/src/web/middleware/auth.ts +++ b/src/web/middleware/auth.ts @@ -364,9 +364,9 @@ export function registerAuthMiddleware(app: FastifyInstance, https: boolean, bas // Per-(username, IP) failure bucket: checked BEFORE the verify, so it is what // caps verified guesses against one account from one source. Keyed on the pair, // so other IPs can't lock the account out and two users behind one shared IP - // don't lock each other out. Same decay as the other buckets; a key is only - // created by a failed verify that the per-IP pre-gate also counts, so it grows - // no faster than userFailures. + // don't lock each other out. Same decay as the other buckets: it holds one key + // per (IP, username) pair that failed a verify, swept by the same TTL, so like + // them it grows only with failed scrypt verifies. state.userIpFailures = new StaleExpirationMap({ ttlMs: AUTH_FAILURE_WINDOW_MS, refreshOnGet: false, @@ -654,10 +654,12 @@ function registerMultiUserAuthHook( // Failed guess: count it against all three buckets. Once the per-username bucket // reaches the cap, further FAILED attempts get 429 from any IP, but this path is // only reached on a wrong password, so it can never deny a correct one. + // Every count is re-read after the await: `ipFail` was read before it, and + // concurrent failures from one IP would otherwise all write the same value. const uFail = (userFailures.get(normUser) ?? 0) + 1; userFailures.set(normUser, uFail); userIpFailures.set(pairKey, (userIpFailures.get(pairKey) ?? 0) + 1); - authFailures.set(clientIp, ipFail + 1); + authFailures.set(clientIp, (authFailures.get(clientIp) ?? 0) + 1); if (uFail >= AUTH_FAILURE_MAX) { sendAuthRateLimit(reply, userFailures, normUser); return; diff --git a/test/multiuser-auth.test.ts b/test/multiuser-auth.test.ts index 12f6e4fa..54bbf11c 100644 --- a/test/multiuser-auth.test.ts +++ b/test/multiuser-auth.test.ts @@ -186,9 +186,10 @@ describe('multi-user auth', () => { const res = await fetch(rurl('/api/status'), { headers: { Authorization: basic('bob', `bad-${i}`) } }); expect(res.status).toBe(401); } - // Finding #17: the CORRECT password must ALWAYS win (verified BEFORE the per-username - // throttle) — the accumulated failures can never lock the account out — and success - // clears the failure buckets. Previously this returned 429 (the DoS being fixed). + // Finding #17: the per-username bucket is checked AFTER the verify, so it never denies + // a correct password, and success clears the failure buckets. Previously this returned + // 429 (the DoS being fixed). Nine failures also stay under the per-(username, IP) gate, + // which is checked before the verify; that is why the correct password is 200 here. const good = await fetch(rurl('/api/status'), { headers: { Authorization: basic('bob', 'bobpass123') } }); expect(good.status).toBe(200); // Self-heal: a fresh wrong attempt is 401 again (the counter was reset by the success). @@ -256,6 +257,24 @@ describe('multi-user auth', () => { expect((await login(ipA, 'bob', 'bobpass123')).statusCode).toBe(429); }); + it('per-IP pre-gate: concurrent failures from one IP are all counted', async () => { + const login = await startIpServer(); + const ip = '203.0.113.40'; + + // AUTH_IP_FAILURE_MAX wrong passwords in flight at once, spread so neither the pair + // nor the per-username bucket can refuse any of them. The per-IP count is written + // after the scrypt await, so each write must re-read it or the burst counts as one. + const perUser = AUTH_FAILURE_MAX - 1; + const burst = []; + for (let i = 0; i < AUTH_IP_FAILURE_MAX; i++) { + burst.push(login(ip, `burst${Math.floor(i / perUser)}`, `bad-${i}`)); + } + await Promise.all(burst); + + // The bucket is full, so the next request from that IP is refused before scrypt. + expect((await login(ip, 'burst-next', 'bad')).statusCode).toBe(429); + }); + it('per-(username, IP) gate: a correct password clears the pair bucket', async () => { const login = await startIpServer(); const ip = '203.0.113.30';