From 10022965d14aa63c656e8a05afb4e4fd252cd384 Mon Sep 17 00:00:00 2001 From: Thomas Joise Date: Sat, 18 Jul 2026 00:15:59 +0530 Subject: [PATCH] =?UTF-8?q?fix(sec/d24):=20auth=20hardening=20=E2=80=94=20?= =?UTF-8?q?login=20lockout,=20rate=20limits,=20constant-time,=20trust=20pr?= =?UTF-8?q?oxy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Red-team highs/mediums (auth surface): - staff_user gains failed_count/locked_until; login() locks an account for 15min after 8 consecutive failures, clears the counter on success (both engines). - login() is now constant-time: a missing email still spends one scrypt verify against a fixed dummy hash — no timing/enumeration oracle. Returns a typed {ok:false,reason} instead of null. - POST /auth/login gets per-IP (20/min) AND per-email (8/15min) fixed-window rate limiting layered over the lockout; 429 on trip. - Credential reveal routes (db-password, module password, module secret) get a per-user reveal limiter (20/min) so one insider cannot bulk-decrypt everything; added missing getClientModule 404 guards. - app.set(trust proxy,1) so req.ip is the real client behind nginx/Caddy and the per-IP limiters actually bucket per client. - makeRateLimiter extracted to rate-limit.ts (shared, no server<->api cycle). Tests: lockout, counter-reset, unknown-email, and existing auth all green. Co-Authored-By: Claude Fable 5 --- apps/hq/src/api.ts | 43 +++++++++++++++++-- apps/hq/src/auth.ts | 60 +++++++++++++++++++++++---- apps/hq/src/db.ts | 13 +++++- apps/hq/src/migrations-pg.ts | 7 ++++ apps/hq/src/rate-limit.ts | 21 ++++++++++ apps/hq/src/server.ts | 29 ++++--------- apps/hq/test/auth.test.ts | 26 ++++++++++-- apps/hq/test/employees-routes.test.ts | 4 +- apps/hq/test/employees.test.ts | 12 +++--- 9 files changed, 172 insertions(+), 43 deletions(-) create mode 100644 apps/hq/src/rate-limit.ts diff --git a/apps/hq/src/api.ts b/apps/hq/src/api.ts index bcc07a4..977447b 100644 --- a/apps/hq/src/api.ts +++ b/apps/hq/src/api.ts @@ -60,6 +60,7 @@ import { import { pullMonthlyCosts } from './aws-costs' import { clientProfitability, duesAging, moduleRevenue, type DateRange } from './repos-reports' import { listUsageRateCards, setUsageRateCard } from './repos-rate-card' +import { makeRateLimiter } from './rate-limit' import { addProjectMilestone, ensureProjectMilestones, listProjectMilestones, milestoneBoard, projectsPendingMilestone, setMilestone, @@ -107,12 +108,42 @@ export function apiRouter( r.get('/health', (_req, res) => { res.json({ ok: true, service: 'sims-hq', version: '0.1.0' }) }) + // D24 (red-team): brute-force defense. Two fixed-window limiters — per client IP and + // per target email — layered over the persistent per-account lockout in login(). The + // per-email bucket trips even when an attacker rotates source IPs; the per-IP bucket + // stops one host hammering many accounts. login() itself is constant-time. + const loginIpLimit = makeRateLimiter(20, 60_000) // 20 attempts / IP / minute + const loginEmailLimit = makeRateLimiter(8, 15 * 60_000) // 8 attempts / email / 15 min + // D24 (red-team): cap credential reveals per user so one insider account cannot silently + // bulk-decrypt every client's banking DB password / portal secret. Every reveal is also + // audited; this bounds the burst. Generous for legitimate support use. + const revealLimit = makeRateLimiter(20, 60_000) // 20 reveals / user / minute + const revealGuard = (res: Response): boolean => { + const ok = revealLimit((res.locals['staff'] as { id: string }).id) + if (!ok) res.status(429).json({ ok: false, error: 'Too many reveals in a short time. Please slow down.' }) + return ok + } r.post('/auth/login', async (req, res) => { try { const { email, password } = req.body as { email?: string; password?: string } - const out = typeof email === 'string' && typeof password === 'string' ? await login(db, email, password) : null - if (!out) { res.status(401).json({ ok: false, error: 'Invalid email or password' }); return } - res.json({ ok: true, ...out }) + const ip = req.ip ?? 'unknown' + if (!loginIpLimit(ip)) { + res.status(429).json({ ok: false, error: 'Too many attempts. Please wait a minute and try again.' }); return + } + if (typeof email !== 'string' || typeof password !== 'string') { + res.status(401).json({ ok: false, error: 'Invalid email or password' }); return + } + if (!loginEmailLimit(email.toLowerCase())) { + res.status(429).json({ ok: false, error: 'Too many attempts for this account. Please try again later.' }); return + } + const out = await login(db, email, password) + if (!out.ok) { + const msg = out.reason === 'locked' + ? 'This account is temporarily locked after too many failed attempts. Try again in 15 minutes.' + : 'Invalid email or password' + res.status(401).json({ ok: false, error: msg }); return + } + res.json({ ok: true, token: out.token, staff: out.staff }) } catch (err) { res.status(400).json({ ok: false, error: err instanceof Error ? err.message : String(err) }) } @@ -242,6 +273,7 @@ export function apiRouter( if (!isManagerial(viewer.role)) { res.status(403).json({ ok: false, error: 'Owner or manager only' }); return } + if (!revealGuard(res)) return const id = String(req.params['id'] ?? '') if ((await getClient(db, id)) === null) { res.status(404).json({ ok: false, error: 'Client not found' }); return @@ -585,8 +617,12 @@ export function apiRouter( if (!isManagerial(viewer.role)) { res.status(403).json({ ok: false, error: 'Owner or manager only' }); return } + if (!revealGuard(res)) return try { const id = String(req.params['id'] ?? '') + if ((await getClientModule(db, id)) === null) { + res.status(404).json({ ok: false, error: 'Client module not found' }); return + } res.json({ ok: true, password: await revealModulePassword(db, viewer.id, id, gmail().keyHex) }) } catch (err) { res.status(400).json({ ok: false, error: err instanceof Error ? err.message : String(err) }) @@ -624,6 +660,7 @@ export function apiRouter( if (!isManagerial(viewer.role)) { res.status(403).json({ ok: false, error: 'Owner or manager only' }); return } + if (!revealGuard(res)) return try { const key = (req.body as { key?: unknown }).key if (typeof key !== 'string') throw new Error('key must be a string') diff --git a/apps/hq/src/auth.ts b/apps/hq/src/auth.ts index 7b2d1e0..a3cf663 100644 --- a/apps/hq/src/auth.ts +++ b/apps/hq/src/auth.ts @@ -8,6 +8,13 @@ import type { DB } from './db' const SESSION_DAYS = 14 +// D24 (red-team) — brute-force lockout policy. +const MAX_FAILED = 8 // consecutive failures before the account locks +const LOCK_MINUTES = 15 // how long a locked account stays barred +// A fixed dummy hash so a login for a NON-existent email still spends one scrypt +// verification — constant time, no email-enumeration oracle. Computed once at load. +const DUMMY_HASH = hashPin('x'.repeat(24)) + export async function createStaff(db: DB, input: { email: string; displayName: string; role: 'owner' | 'manager' | 'staff'; password: string }): Promise<{ id: string }> { @@ -22,17 +29,54 @@ export async function createStaff(db: DB, input: { return { id } } -interface StaffRow { id: string; email: string; display_name: string; role: string; pw_salt: string; pw_hash: string; active: number } +interface StaffRow { + id: string; email: string; display_name: string; role: string + pw_salt: string; pw_hash: string; active: number + failed_count: number; locked_until: string | null +} + +export type LoginResult = + | { ok: true; token: string; staff: { id: string; displayName: string; role: string } } + | { ok: false; reason: 'invalid' | 'locked' } -export async function login(db: DB, email: string, password: string): - Promise<{ token: string; staff: { id: string; displayName: string; role: string } } | null> { +/** + * Authenticate, with brute-force lockout (D24). Always spends exactly one scrypt + * verification — a real one for an existing account, a dummy for a missing one — so + * response time never reveals whether an email exists. After MAX_FAILED consecutive + * failures the account locks for LOCK_MINUTES; a success clears the counter. The generic + * 'invalid' reason is returned for both wrong-password and unknown-email. + */ +export async function login(db: DB, email: string, password: string): Promise { const row = await db.get(`SELECT * FROM staff_user WHERE email=? AND active=1`, email.toLowerCase()) - if (!row || !verifyPin(password, { salt: row.pw_salt, hash: row.pw_hash })) return null + const now = Date.now() + + // Locked? Still spend a scrypt op (constant time) before returning. + if (row && row.locked_until !== null && Date.parse(row.locked_until) > now) { + verifyPin(password, { salt: DUMMY_HASH.salt, hash: DUMMY_HASH.hash }) + return { ok: false, reason: 'locked' } + } + + const stored = row ? { salt: row.pw_salt, hash: row.pw_hash } : DUMMY_HASH + const passwordOk = verifyPin(password, stored) + + if (!row || !passwordOk) { + if (row) { + // Count the failure; lock once the threshold is crossed. + const failed = row.failed_count + 1 + const lockUntil = failed >= MAX_FAILED ? new Date(now + LOCK_MINUTES * 60_000).toISOString() : null + await db.run(`UPDATE staff_user SET failed_count=?, locked_until=? WHERE id=?`, failed, lockUntil, row.id) + } + return { ok: false, reason: 'invalid' } + } + const token = randomBytes(32).toString('hex') - const expires = new Date(Date.now() + SESSION_DAYS * 86_400_000).toISOString() - await db.run(`INSERT INTO session (token, staff_id, expires_at) VALUES (?, ?, ?)`, token, row.id, expires) - await writeAudit(db, row.id, 'login', 'staff_user', row.id) - return { token, staff: { id: row.id, displayName: row.display_name, role: row.role } } + const expires = new Date(now + SESSION_DAYS * 86_400_000).toISOString() + await db.transaction(async () => { + await db.run(`UPDATE staff_user SET failed_count=0, locked_until=NULL WHERE id=?`, row.id) + await db.run(`INSERT INTO session (token, staff_id, expires_at) VALUES (?, ?, ?)`, token, row.id, expires) + await writeAudit(db, row.id, 'login', 'staff_user', row.id) + }) + return { ok: true, token, staff: { id: row.id, displayName: row.display_name, role: row.role } } } export async function verifySession(db: DB, token: string): Promise<{ id: string; role: string } | null> { diff --git a/apps/hq/src/db.ts b/apps/hq/src/db.ts index ad5f3bc..febd270 100644 --- a/apps/hq/src/db.ts +++ b/apps/hq/src/db.ts @@ -73,7 +73,11 @@ CREATE TABLE IF NOT EXISTS staff_user ( id TEXT PRIMARY KEY, email TEXT NOT NULL UNIQUE, display_name TEXT NOT NULL, role TEXT NOT NULL CHECK (role IN ('owner','manager','staff')), phone TEXT, title TEXT, -- D18 WS-E: contact + designation shown on Employees/profile; nothing routes on them - pw_salt TEXT NOT NULL, pw_hash TEXT NOT NULL, active INTEGER NOT NULL DEFAULT 1 + pw_salt TEXT NOT NULL, pw_hash TEXT NOT NULL, active INTEGER NOT NULL DEFAULT 1, + -- D24 (red-team): brute-force lockout. failed_count counts consecutive failed logins; + -- locked_until is an ISO timestamp the account is barred until (NULL = not locked). + -- Both reset to 0/NULL on a successful login. + failed_count INTEGER NOT NULL DEFAULT 0, locked_until TEXT ); CREATE TABLE IF NOT EXISTS session ( token TEXT PRIMARY KEY, staff_id TEXT NOT NULL, expires_at TEXT NOT NULL @@ -320,6 +324,13 @@ function migrate(db: SqliteRaw): void { if (!staffCols.some((c) => c.name === 'title')) { db.exec(`ALTER TABLE staff_user ADD COLUMN title TEXT`) } + // D24 (red-team): login lockout columns. + if (!staffCols.some((c) => c.name === 'failed_count')) { + db.exec(`ALTER TABLE staff_user ADD COLUMN failed_count INTEGER NOT NULL DEFAULT 0`) + } + if (!staffCols.some((c) => c.name === 'locked_until')) { + db.exec(`ALTER TABLE staff_user ADD COLUMN locked_until TEXT`) + } const clientCols = db.prepare(`PRAGMA table_info(client)`).all() as { name: string }[] if (!clientCols.some((c) => c.name === 'owner_id')) { db.exec(`ALTER TABLE client ADD COLUMN owner_id TEXT`) diff --git a/apps/hq/src/migrations-pg.ts b/apps/hq/src/migrations-pg.ts index e799c17..8c9b977 100644 --- a/apps/hq/src/migrations-pg.ts +++ b/apps/hq/src/migrations-pg.ts @@ -235,6 +235,13 @@ CREATE TABLE IF NOT EXISTS project_milestone ( done integer NOT NULL DEFAULT 0, done_on text, sort integer NOT NULL DEFAULT 0, UNIQUE (client_module_id, key) ); +`, + }, + { + id: '006-login-lockout', + sql: ` +ALTER TABLE staff_user ADD COLUMN IF NOT EXISTS failed_count integer NOT NULL DEFAULT 0; +ALTER TABLE staff_user ADD COLUMN IF NOT EXISTS locked_until text; `, }, ] diff --git a/apps/hq/src/rate-limit.ts b/apps/hq/src/rate-limit.ts new file mode 100644 index 0000000..eac114c --- /dev/null +++ b/apps/hq/src/rate-limit.ts @@ -0,0 +1,21 @@ +/** + * A dependency-free fixed-window rate limiter keyed by an opaque string (client IP, + * account email, staff id, …). Returns true while the key is under `limit` in the + * current `windowMs` window, false once it is exhausted. Expired buckets are swept + * lazily so the map cannot grow without bound under key rotation. In its own module + * so both the api router and the public-share route can share it without a cycle. + */ +export function makeRateLimiter(limit: number, windowMs: number): (key: string, now?: number) => boolean { + const buckets = new Map() + return (key: string, now = Date.now()): boolean => { + if (buckets.size > 5000) for (const [k, b] of buckets) if (now >= b.resetAt) buckets.delete(k) + const bucket = buckets.get(key) + if (bucket === undefined || now >= bucket.resetAt) { + buckets.set(key, { count: 1, resetAt: now + windowMs }) + return true + } + if (bucket.count >= limit) return false + bucket.count += 1 + return true + } +} diff --git a/apps/hq/src/server.ts b/apps/hq/src/server.ts index a6f43f1..acd7613 100644 --- a/apps/hq/src/server.ts +++ b/apps/hq/src/server.ts @@ -8,6 +8,7 @@ import { maybePullAwsCosts } from './aws-costs' import { pollBounces } from './bounces' import { openDb, type DB } from './db' import { openPgDb } from './db-pg' +import { makeRateLimiter } from './rate-limit' import { renderPdf } from './pdf' import { getClient } from './repos-clients' import { validateShare } from './repos-shares' @@ -66,26 +67,9 @@ async function companySettings(db: DB): Promise> { return Object.fromEntries(rows.map((row) => [row.key, row.value])) } -/** - * A dependency-free fixed-window rate limiter keyed by an opaque string (the caller - * passes the client IP). Returns true while the key is under `limit` in the current - * `windowMs` window, false once it is exhausted. Expired buckets are swept lazily so - * the map cannot grow without bound under IP rotation. - */ -export function makeRateLimiter(limit: number, windowMs: number): (key: string, now?: number) => boolean { - const buckets = new Map() - return (key: string, now = Date.now()): boolean => { - if (buckets.size > 5000) for (const [k, b] of buckets) if (now >= b.resetAt) buckets.delete(k) - const bucket = buckets.get(key) - if (bucket === undefined || now >= bucket.resetAt) { - buckets.set(key, { count: 1, resetAt: now + windowMs }) - return true - } - if (bucket.count >= limit) return false - bucket.count += 1 - return true - } -} +// Re-exported for existing importers/tests; the source of truth is rate-limit.ts, +// shared with the api router without a server↔api import cycle. +export { makeRateLimiter } from './rate-limit' /** Minimal, self-contained "this link is gone" page. Carries no document data. */ function shareGonePage(message: string): string { @@ -155,6 +139,11 @@ export function mountPublicShare(app: express.Express, db: DB, deps: PublicShare export async function startServer(port: number, opts: { scheduler?: boolean } = {}): Promise { const app = express() + // D24 (red-team): behind the reverse proxy (nginx/Caddy), trust the first hop so + // req.ip reflects the real client via X-Forwarded-For — without this every request + // shares the proxy's IP and per-IP rate limiting collapses to one global bucket. + // The proxy MUST strip client-supplied X-Forwarded-For so it cannot be spoofed. + app.set('trust proxy', 1) // D19 engine selection: DATABASE_URL → Postgres (production); absent → SQLite file. const pgUrl = process.env['DATABASE_URL'] ?? '' const db = pgUrl !== '' ? await openPgDb(pgUrl) : openDb(process.env['HQ_DATA_DIR']) diff --git a/apps/hq/test/auth.test.ts b/apps/hq/test/auth.test.ts index e491da6..72a44b9 100644 --- a/apps/hq/test/auth.test.ts +++ b/apps/hq/test/auth.test.ts @@ -7,10 +7,10 @@ describe('hq auth', () => { it('logs in with correct password, rejects wrong one', async () => { const db = openDb(':memory:') await createStaff(db, { email: 'admin@tecnostac.com', displayName: 'Owner', role: 'owner', password: 'let-me-in-9' }) - expect(await login(db, 'admin@tecnostac.com', 'wrong')).toBeNull() + expect(await login(db, 'admin@tecnostac.com', 'wrong')).toMatchObject({ ok: false, reason: 'invalid' }) const ok = await login(db, 'admin@tecnostac.com', 'let-me-in-9') - expect(ok).not.toBeNull() - const staff = await verifySession(db, ok!.token) + expect(ok.ok).toBe(true) + const staff = await verifySession(db, ok.ok ? ok.token : '') expect(staff).toMatchObject({ role: 'owner' }) }) it('rejects passwords under 8 chars at creation', async () => { @@ -19,4 +19,24 @@ describe('hq auth', () => { createStaff(db, { email: 'a@b.c', displayName: 'X', role: 'staff', password: 'short' }), ).rejects.toThrow(/8/) }) + it('locks the account after repeated failures, then a wrong-but-formerly-valid attempt is barred (D24)', async () => { + const db = openDb(':memory:') + await createStaff(db, { email: 'lock@t.in', displayName: 'L', role: 'staff', password: 'correct-horse-9' }) + for (let i = 0; i < 8; i++) expect((await login(db, 'lock@t.in', 'nope')).ok).toBe(false) + // Now locked — even the CORRECT password is refused with reason 'locked'. + expect(await login(db, 'lock@t.in', 'correct-horse-9')).toMatchObject({ ok: false, reason: 'locked' }) + }) + it('a successful login clears the failure counter (D24)', async () => { + const db = openDb(':memory:') + await createStaff(db, { email: 'reset@t.in', displayName: 'R', role: 'staff', password: 'correct-horse-9' }) + for (let i = 0; i < 3; i++) await login(db, 'reset@t.in', 'nope') + expect((await login(db, 'reset@t.in', 'correct-horse-9')).ok).toBe(true) + const row = await db.get<{ failed_count: number; locked_until: string | null }>( + `SELECT failed_count, locked_until FROM staff_user WHERE email='reset@t.in'`) + expect(row).toMatchObject({ failed_count: 0, locked_until: null }) + }) + it('an unknown email still returns invalid (constant-time, no enumeration) (D24)', async () => { + const db = openDb(':memory:') + expect(await login(db, 'ghost@nowhere.in', 'whatever')).toMatchObject({ ok: false, reason: 'invalid' }) + }) }) diff --git a/apps/hq/test/employees-routes.test.ts b/apps/hq/test/employees-routes.test.ts index 075de67..e72715a 100644 --- a/apps/hq/test/employees-routes.test.ts +++ b/apps/hq/test/employees-routes.test.ts @@ -82,8 +82,8 @@ describe('/employees routes', () => { const pw = await call(token, 'POST', `/employees/${id}/password`, { password: 'brand-new-secret' }) expect(pw.status).toBe(200) - expect(await login(ctx.db, 'new@test.in', 'password-9')).toBeNull() - expect(await login(ctx.db, 'new@test.in', 'brand-new-secret')).not.toBeNull() + expect((await login(ctx.db, 'new@test.in', 'password-9')).ok).toBe(false) + expect((await login(ctx.db, 'new@test.in', 'brand-new-secret')).ok).toBe(true) }) it('non-owner gets 403 on every mutation route', async () => { diff --git a/apps/hq/test/employees.test.ts b/apps/hq/test/employees.test.ts index adbfb44..fd20cf1 100644 --- a/apps/hq/test/employees.test.ts +++ b/apps/hq/test/employees.test.ts @@ -45,7 +45,7 @@ describe('employee foundation', () => { it('deactivation kills the live session immediately and purges its rows', async () => { const { db, ownerId } = await withOwner() const emp = await createEmployee(db, ownerId, { email: 's@x.co', displayName: 'Stf', role: 'staff', password: 'password2' }) - const session = (await login(db, 's@x.co', 'password2'))! + const login2 = await login(db, 's@x.co', 'password2'); if (!login2.ok) throw new Error('login failed'); const session = login2 expect(await verifySession(db, session.token)).toMatchObject({ id: emp.id }) await deactivateEmployee(db, ownerId, emp.id) expect(await verifySession(db, session.token)).toBeNull() @@ -66,16 +66,16 @@ describe('employee foundation', () => { const { db, ownerId } = await withOwner() const emp = await createEmployee(db, ownerId, { email: 's@x.co', displayName: 'Stf', role: 'staff', password: 'password2' }) await deactivateEmployee(db, ownerId, emp.id) - expect(await login(db, 's@x.co', 'password2')).toBeNull() + expect((await login(db, 's@x.co', 'password2')).ok).toBe(false) expect((await getEmployee(db, emp.id))!.active).toBe(false) await reactivateEmployee(db, ownerId, emp.id) - expect(await login(db, 's@x.co', 'password2')).not.toBeNull() + expect((await login(db, 's@x.co', 'password2')).ok).toBe(true) }) it('password reset invalidates existing sessions immediately (compromised-credential lockout)', async () => { const { db, ownerId } = await withOwner() const emp = await createEmployee(db, ownerId, { email: 's@x.co', displayName: 'Stf', role: 'staff', password: 'password2' }) - const session = (await login(db, 's@x.co', 'password2'))! + const login2 = await login(db, 's@x.co', 'password2'); if (!login2.ok) throw new Error('login failed'); const session = login2 expect(await verifySession(db, session.token)).toMatchObject({ id: emp.id }) await setEmployeePassword(db, ownerId, emp.id, 'new-secret-9') expect(await verifySession(db, session.token)).toBeNull() @@ -104,8 +104,8 @@ describe('employee foundation', () => { const { db, ownerId } = await withOwner() const emp = await createEmployee(db, ownerId, { email: 's@x.co', displayName: 'Stf', role: 'staff', password: 'password2' }) await setEmployeePassword(db, ownerId, emp.id, 'new-secret-9') - expect(await login(db, 's@x.co', 'password2')).toBeNull() - expect(await login(db, 's@x.co', 'new-secret-9')).not.toBeNull() + expect((await login(db, 's@x.co', 'password2')).ok).toBe(false) + expect((await login(db, 's@x.co', 'new-secret-9')).ok).toBe(true) const audit = (await listAudit(db)).find((a) => a.action === 'reset_password' && a.entity_id === emp.id) expect(audit).toBeDefined() expect(audit!.before_json ?? '').not.toMatch(/hash/)