From 5865aafe06fb505c96ab2702a1cb5607b044fe12 Mon Sep 17 00:00:00 2001 From: Thomas Joise Date: Fri, 17 Jul 2026 02:15:57 +0530 Subject: [PATCH] fix(employees): purge sessions on password reset; reject empty patches and duplicate emails cleanly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the Phase 1-2 employee slice: - setEmployeePassword now deletes the employee's sessions in the same transaction — resetting a suspected-compromised password must not leave the attacker's existing 14-day bearer token valid (deactivateEmployee already did this; the reset path is the natural lockout action). - updateEmployee throws 'Nothing to update' when the patch resolves to zero recognized fields, instead of returning 200 and writing a no-op before==after audit row; PATCH /employees/:id with {} or wrongly-typed fields now 400s. - createEmployee pre-checks UNIQUE(email) inside the transaction and throws 'Email already in use' instead of surfacing the raw engine-specific 'UNIQUE constraint failed: staff_user.email' text (would differ on Postgres, the locked production engine — D15). Co-Authored-By: Claude Fable 5 --- apps/hq/src/repos-employees.ts | 19 +++++++++++++----- apps/hq/test/employees-routes.test.ts | 14 ++++++++++++++ apps/hq/test/employees.test.ts | 28 +++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/apps/hq/src/repos-employees.ts b/apps/hq/src/repos-employees.ts index 4f7bc9e..282c3e7 100644 --- a/apps/hq/src/repos-employees.ts +++ b/apps/hq/src/repos-employees.ts @@ -64,6 +64,11 @@ export function createEmployee(db: DB, userId: string, input: { const id = uuidv7() const { salt, hash } = hashPin(input.password) // generic scrypt; PIN digit policy not applied return db.transaction(() => { + // Pre-check the UNIQUE(email) so the caller gets a friendly, engine-neutral + // message instead of raw driver text (SQLite and Postgres word it differently). + if (db.prepare(`SELECT 1 FROM staff_user WHERE email=?`).get(email) !== undefined) { + throw new Error('Email already in use') + } db.prepare( `INSERT INTO staff_user (id, email, display_name, role, pw_salt, pw_hash) VALUES (?, ?, ?, ?, ?, ?)`, ).run(id, email, input.displayName, input.role, salt, hash) @@ -92,17 +97,20 @@ export function updateEmployee(db: DB, userId: string, id: string, patch: { sets.push('display_name=?'); args.push(patch.displayName) } if (patch.role !== undefined) { sets.push('role=?'); args.push(patch.role) } - if (sets.length > 0) { - args.push(id) - db.prepare(`UPDATE staff_user SET ${sets.join(', ')} WHERE id=?`).run(...args) - } + // An empty patch is a caller error — succeeding silently would write a + // no-op audit row and tell the caller a change landed when nothing did. + if (sets.length === 0) throw new Error('Nothing to update') + args.push(id) + db.prepare(`UPDATE staff_user SET ${sets.join(', ')} WHERE id=?`).run(...args) const after = getEmployee(db, id)! writeAudit(db, userId, 'update', 'staff_user', id, before, after) return after })() } -/** Reset an employee's password. Audits the action; never logs the hash. */ +/** Reset an employee's password. Audits the action; never logs the hash. + * Existing sessions are purged in the same transaction — a reset is the natural + * "lock out the old credential" action, so a stolen token must not outlive it. */ export function setEmployeePassword(db: DB, userId: string, id: string, password: string): void { if (password.length < 8) throw new Error('Password must be at least 8 characters') const { salt, hash } = hashPin(password) @@ -110,6 +118,7 @@ export function setEmployeePassword(db: DB, userId: string, id: string, password const before = getEmployee(db, id) if (!before) throw new Error('Employee not found') db.prepare(`UPDATE staff_user SET pw_salt=?, pw_hash=? WHERE id=?`).run(salt, hash, id) + db.prepare(`DELETE FROM session WHERE staff_id=?`).run(id) writeAudit(db, userId, 'reset_password', 'staff_user', id) })() } diff --git a/apps/hq/test/employees-routes.test.ts b/apps/hq/test/employees-routes.test.ts index b0cae6c..d8e676b 100644 --- a/apps/hq/test/employees-routes.test.ts +++ b/apps/hq/test/employees-routes.test.ts @@ -121,6 +121,20 @@ describe('/employees routes', () => { expect((await call(token, 'POST', '/employees/nope/password', { password: 'password-9' })).status).toBe(404) }) + it('PATCH with no recognized fields is a 400, not a silent 200 no-op', async () => { + const token = await tokenOf('owner@test.in', 'owner-password') + const created = await call(token, 'POST', '/employees', { + email: 'noop@test.in', displayName: 'Noop', role: 'staff', password: 'password-9', + }) + const id = created.json.employee.id as string + expect((await call(token, 'PATCH', `/employees/${id}`, {})).status).toBe(400) + // wrongly-typed field is dropped by the route → resolves to an empty patch → 400 + expect((await call(token, 'PATCH', `/employees/${id}`, { role: 123 })).status).toBe(400) + // and the employee is untouched + const after = await call(token, 'GET', '/employees') + expect(after.json.employees.find((e: { id: string }) => e.id === id)).toMatchObject({ displayName: 'Noop', role: 'staff' }) + }) + it('mutations act as the signed-in user (audit userId = actor)', async () => { const token = await tokenOf('owner@test.in', 'owner-password') const created = await call(token, 'POST', '/employees', { diff --git a/apps/hq/test/employees.test.ts b/apps/hq/test/employees.test.ts index fb5547d..29c4835 100644 --- a/apps/hq/test/employees.test.ts +++ b/apps/hq/test/employees.test.ts @@ -72,6 +72,34 @@ describe('employee foundation', () => { expect(login(db, 's@x.co', 'password2')).not.toBeNull() }) + it('password reset invalidates existing sessions immediately (compromised-credential lockout)', () => { + const { db, ownerId } = withOwner() + const emp = createEmployee(db, ownerId, { email: 's@x.co', displayName: 'Stf', role: 'staff', password: 'password2' }) + const session = login(db, 's@x.co', 'password2')! + expect(verifySession(db, session.token)).toMatchObject({ id: emp.id }) + setEmployeePassword(db, ownerId, emp.id, 'new-secret-9') + expect(verifySession(db, session.token)).toBeNull() + const rows = db.prepare(`SELECT COUNT(*) AS n FROM session WHERE staff_id=?`).get(emp.id) as { n: number } + expect(rows.n).toBe(0) + }) + + it('rejects a duplicate email with a friendly, engine-neutral message', () => { + const { db, ownerId } = withOwner() + createEmployee(db, ownerId, { email: 'dup@x.co', displayName: 'A', role: 'staff', password: 'password1' }) + expect(() => + createEmployee(db, ownerId, { email: 'Dup@X.co', displayName: 'B', role: 'staff', password: 'password2' }), + ).toThrow(/already in use/i) // not the raw 'UNIQUE constraint failed: …' SQLite text + }) + + it('rejects an empty patch instead of writing a no-op audit row', () => { + const { db, ownerId } = withOwner() + const emp = createEmployee(db, ownerId, { email: 's@x.co', displayName: 'Stf', role: 'staff', password: 'password2' }) + const auditBefore = listAudit(db).filter((a) => a.entity === 'staff_user' && a.entity_id === emp.id).length + expect(() => updateEmployee(db, ownerId, emp.id, {})).toThrow(/nothing to update/i) + const auditAfter = listAudit(db).filter((a) => a.entity === 'staff_user' && a.entity_id === emp.id).length + expect(auditAfter).toBe(auditBefore) + }) + it('password reset works and is audited without the hash', () => { const { db, ownerId } = withOwner() const emp = createEmployee(db, ownerId, { email: 's@x.co', displayName: 'Stf', role: 'staff', password: 'password2' })