From f226db0fb7f03096d2a832304ad9614e944e4bf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tao=20Bojl=C3=A9n?= Date: Tue, 16 Jun 2026 09:21:56 +0100 Subject: [PATCH] (auth) Invalidate sessions on password change (#778) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Security] Invalidate sessions on password change (GHSA-g5xq-67g7-36r2) Resetting a password did not clear the user's PG-stored sessions (connect-pg-simple, 30-day maxAge), and passport.deserializeUser looks users up by id only. A phished/attacker session therefore survived a password reset for up to 30 days, even after the legitimate user took the expected recovery action. Add deleteSessionsForUser(), which removes the user's rows from public.session (matched on passport's `sess -> 'passport' ->> 'user'`), and call it from both password-change paths: - resetPasswordForToken: deletes all of the user's sessions. - UserApi.changePassword: deletes all *other* sessions, preserving the caller's own (via the request's sessionID) so they aren't logged out mid-action. No DB migration; deserializeUser is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) * Drop advisory ID from code comments Keep the explanatory comments; remove the GHSA reference so it isn't exposed in source while the advisory is unpublished. Co-Authored-By: Claude Opus 4.8 (1M context) * Drop file-path reference from sessionPersistence comment Co-Authored-By: Claude Opus 4.8 (1M context) * Make password-change + session invalidation atomic Wrap the password update and session purge in a single transaction (via makeKyselyTransactionWithRetry) in both resetPasswordForToken and UserApi.changePassword. Previously these were separate commits, so a failure after the password update left the user's sessions alive — the exact persistence the fix is meant to prevent. Also narrow the test's eslint-disable for the unused mock constructor args from a block disable to per-line comments. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- server/graphql/datasources/UserApi.test.ts | 110 ++++++++++++++++++ server/graphql/datasources/UserApi.ts | 33 ++++-- server/graphql/modules/user.ts | 6 +- .../services/userManagementService/index.ts | 1 + .../sessionPersistence.test.ts | 44 +++++++ .../sessionPersistence.ts | 28 +++++ .../userManagementService.test.ts | 56 ++++++++- .../userManagementService.ts | 29 +++-- 8 files changed, 284 insertions(+), 23 deletions(-) create mode 100644 server/graphql/datasources/UserApi.test.ts create mode 100644 server/services/userManagementService/sessionPersistence.test.ts create mode 100644 server/services/userManagementService/sessionPersistence.ts diff --git a/server/graphql/datasources/UserApi.test.ts b/server/graphql/datasources/UserApi.test.ts new file mode 100644 index 0000000..4d02ccb --- /dev/null +++ b/server/graphql/datasources/UserApi.test.ts @@ -0,0 +1,110 @@ +import { type Kysely } from 'kysely'; + +import { hashPassword } from '../../services/userManagementService/index.js'; +import { makeTestWithFixture } from '../../test/utils.js'; +import UserAPI from './UserApi.js'; +import { type GraphQLUserParent } from './userKyselyPersistence.js'; + +// changePassword only touches the injected Kysely instance (for the user +// update + session deletion); the other constructor deps are unused here. +function makeMockKyselyPg() { + const updateExecuteTakeFirst = jest.fn(); + const updateBuilder = { + set: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + returning: jest.fn().mockReturnThis(), + executeTakeFirst: updateExecuteTakeFirst, + }; + + const deleteWhere = jest.fn(); + const deleteBuilder = { where: deleteWhere, execute: jest.fn() }; + deleteWhere.mockReturnValue(deleteBuilder); + deleteBuilder.execute.mockResolvedValue([]); + + const updateTable = jest.fn().mockReturnValue(updateBuilder); + const deleteFrom = jest.fn().mockReturnValue(deleteBuilder); + + // changePassword runs inside makeKyselyTransactionWithRetry, which calls + // `kysely.transaction().execute(cb)`. Run the callback against this same mock. + const transaction = jest.fn().mockReturnValue({ + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- test trx stub + execute: (cb: (trx: any) => unknown) => cb(kyselyPg), + }); + + + const kyselyPg = { + updateTable, + deleteFrom, + transaction, + } as unknown as Kysely; + return { kyselyPg, updateExecuteTakeFirst, deleteFrom, deleteWhere }; +} + +describe('UserAPI', () => { + describe('#changePassword', () => { + const testWithFixtures = makeTestWithFixture(() => ({})); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + testWithFixtures( + 'invalidates the user other sessions but preserves the caller session', + async () => { + const currentPassword = 'current-password'; + const userId = 'user-123'; + const currentSid = 'sid-abc'; + const passwordHash = await hashPassword(currentPassword); + + const { kyselyPg, updateExecuteTakeFirst, deleteFrom, deleteWhere } = + makeMockKyselyPg(); + + // kyselyUserUpdate succeeds (returns a non-undefined row). + updateExecuteTakeFirst.mockResolvedValue({ + id: userId, + email: 'test@example.com', + password: 'new-hash', + first_name: 'Test', + last_name: 'User', + org_id: 'org-456', + role: 'ADMIN', + approved_by_admin: true, + rejected_by_admin: false, + login_methods: ['password'], + permissions: [], + created_at: new Date(), + updated_at: new Date(), + }); + + // Remaining constructor deps are unused by changePassword. + const sut = new UserAPI( + kyselyPg, + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- unused mock dep + {} as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- unused mock dep + {} as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- unused mock dep + {} as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- unused mock dep + {} as any, + ); + + const user = { + id: userId, + loginMethods: ['password'], + password: passwordHash, + } as unknown as GraphQLUserParent; + + await sut.changePassword( + user, + { currentPassword, newPassword: 'new-password' }, + currentSid, + ); + + // The caller's own session is preserved; all others are invalidated. + expect(deleteFrom).toHaveBeenCalledWith('public.session'); + expect(deleteWhere).toHaveBeenCalledWith('sid', '!=', currentSid); + }, + ); + }); +}); diff --git a/server/graphql/datasources/UserApi.ts b/server/graphql/datasources/UserApi.ts index 38329dd..3bd00b7 100644 --- a/server/graphql/datasources/UserApi.ts +++ b/server/graphql/datasources/UserApi.ts @@ -2,8 +2,10 @@ import { type Exception } from '@opentelemetry/api'; import { uid } from 'uid'; import { inject, type Dependencies } from '../../iocContainer/index.js'; +import { type CombinedPg } from '../../services/combinedDbTypes.js'; import { type LoginMethod } from '../../services/coreAppTables.js'; import { + deleteSessionsForUser, hashPassword, passwordMatchesHash, } from '../../services/userManagementService/index.js'; @@ -12,6 +14,7 @@ import { makeNotFoundError, makeUnauthorizedError, } from '../../utils/errors.js'; +import { makeKyselyTransactionWithRetry } from '../../utils/kyselyTransactionWithRetry.js'; import { safePick } from '../../utils/misc.js'; import { WEEK_MS } from '../../utils/time.js'; import { @@ -233,6 +236,9 @@ class UserAPI { async changePassword( user: GraphQLUserParent, params: { currentPassword: string; newPassword: string }, + // The caller's current session id, preserved so the user isn't logged out + // of the session they're changing their password from. + currentSid?: string, ) { const { currentPassword, newPassword } = params; @@ -262,15 +268,24 @@ class UserAPI { } const hashedNewPassword = await hashPassword(newPassword); - const updated = await kyselyUserUpdate(this.kyselyPg, user.id, { - password: hashedNewPassword, - }); - if (updated == null) { - // Row went missing between load and update (e.g. concurrent delete). - throw makeNotFoundError(`User ${user.id} not found`, { - shouldErrorSpan: true, - }); - } + // Update the password and invalidate the user's other sessions atomically: + // if the session purge failed independently, a phished/attacker session + // could outlive the password change. Keep the caller's own session. + await makeKyselyTransactionWithRetry(this.kyselyPg)( + async (trx) => { + const updated = await kyselyUserUpdate(trx, user.id, { + password: hashedNewPassword, + }); + if (updated == null) { + // Row went missing between load and update (e.g. concurrent delete). + throw makeNotFoundError(`User ${user.id} not found`, { + shouldErrorSpan: true, + }); + } + + await deleteSessionsForUser(trx, user.id, { exceptSid: currentSid }); + }, + ); return { __typename: 'ChangePasswordSuccessResponse' as const, diff --git a/server/graphql/modules/user.ts b/server/graphql/modules/user.ts index 2927291..45d9feb 100644 --- a/server/graphql/modules/user.ts +++ b/server/graphql/modules/user.ts @@ -213,7 +213,11 @@ const Mutation: GQLMutationResolvers = { if (user == null) { throw unauthenticatedError('Authenticated user required'); } - return context.dataSources.userAPI.changePassword(user, params.input); + return context.dataSources.userAPI.changePassword( + user, + params.input, + context.req.sessionID, + ); }, async deleteUser(_, params, context) { const user = context.getUser(); diff --git a/server/services/userManagementService/index.ts b/server/services/userManagementService/index.ts index b3ab252..f2373e2 100644 --- a/server/services/userManagementService/index.ts +++ b/server/services/userManagementService/index.ts @@ -4,6 +4,7 @@ export { type UserManagementService, } from './userManagementService.js'; export { hashPassword, passwordMatchesHash } from './utils.js'; +export { deleteSessionsForUser } from './sessionPersistence.js'; export { Invoker, UserPermission, diff --git a/server/services/userManagementService/sessionPersistence.test.ts b/server/services/userManagementService/sessionPersistence.test.ts new file mode 100644 index 0000000..8a0c464 --- /dev/null +++ b/server/services/userManagementService/sessionPersistence.test.ts @@ -0,0 +1,44 @@ +import { type Kysely } from 'kysely'; + +import { deleteSessionsForUser } from './sessionPersistence.js'; + +// Build a mock Kysely whose `deleteFrom(...)` returns a chainable +// `{ where, execute }`, so we can assert the issued query shape. +function makeMockDb() { + const execute = jest.fn().mockResolvedValue([]); + const where = jest.fn(); + const builder = { where, execute }; + where.mockReturnValue(builder); + const deleteFrom = jest.fn().mockReturnValue(builder); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const db = { deleteFrom } as unknown as Kysely; + return { db, deleteFrom, where, execute }; +} + +describe('deleteSessionsForUser', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('deletes session rows matching the user id', async () => { + const { db, deleteFrom, where, execute } = makeMockDb(); + + await deleteSessionsForUser(db, 'user-123'); + + expect(deleteFrom).toHaveBeenCalledWith('public.session'); + // Matches on the passport user id; no `sid` exclusion when none requested. + expect(where).toHaveBeenCalledTimes(1); + expect(where).toHaveBeenCalledWith(expect.anything(), '=', 'user-123'); + expect(execute).toHaveBeenCalledTimes(1); + }); + + it('preserves the caller session when exceptSid is given', async () => { + const { db, where, execute } = makeMockDb(); + + await deleteSessionsForUser(db, 'user-123', { exceptSid: 'sid-abc' }); + + expect(where).toHaveBeenCalledWith(expect.anything(), '=', 'user-123'); + expect(where).toHaveBeenCalledWith('sid', '!=', 'sid-abc'); + expect(execute).toHaveBeenCalledTimes(1); + }); +}); diff --git a/server/services/userManagementService/sessionPersistence.ts b/server/services/userManagementService/sessionPersistence.ts new file mode 100644 index 0000000..c83820d --- /dev/null +++ b/server/services/userManagementService/sessionPersistence.ts @@ -0,0 +1,28 @@ +import { sql, type Kysely } from 'kysely'; + +/** + * Delete connect-pg-simple session rows for a user, to force re-authentication + * after a password change. Passport stores the user id at + * `sess -> 'passport' ->> 'user'` (see `passport.serializeUser` in + * `server/api.ts`, which serializes `user.id`). + * + * Pass `exceptSid` to keep one session alive — e.g. the caller's own session on + * a self-service password change, so they aren't logged out mid-request. + */ +export async function deleteSessionsForUser( + // `Kysely` because the session table is managed by connect-pg-simple and + // is intentionally absent from our typed schema; injected instances are + // themselves `Kysely`. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + db: Kysely, + userId: string, + opts: { exceptSid?: string } = {}, +): Promise { + let query = db + .deleteFrom('public.session') + .where(sql`sess -> 'passport' ->> 'user'`, '=', userId); + if (opts.exceptSid != null) { + query = query.where('sid', '!=', opts.exceptSid); + } + await query.execute(); +} diff --git a/server/services/userManagementService/userManagementService.test.ts b/server/services/userManagementService/userManagementService.test.ts index 6ddcfcf..24e9087 100644 --- a/server/services/userManagementService/userManagementService.test.ts +++ b/server/services/userManagementService/userManagementService.test.ts @@ -1,14 +1,16 @@ import { type Kysely } from 'kysely'; import { makeTestWithFixture } from '../../test/utils.js'; -import UserManagementService from './userManagementService.js'; import type { UserManagementPg } from './index.js'; +import UserManagementService from './userManagementService.js'; // Mock dependencies const mockDb = { selectFrom: jest.fn(), insertInto: jest.fn(), + updateTable: jest.fn(), deleteFrom: jest.fn(), + transaction: jest.fn(), } as unknown as Kysely; const mockSendEmail = jest.fn(); @@ -204,4 +206,56 @@ describe('UserManagementService', () => { }, ); }); + + describe('#resetPasswordForToken', () => { + testWithFixtures( + 'invalidates all sessions for the user after resetting the password', + async ({ sut }) => { + const userId = 'user-123'; + + // Valid, non-expired reset token. + const mockSelect = { + selectAll: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + executeTakeFirst: jest.fn().mockResolvedValue({ + hashed_token: 'hashed', + user_id: userId, + org_id: 'org-456', + created_at: new Date(), + }), + }; + + const mockUpdate = { + set: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + execute: jest.fn().mockResolvedValue([]), + }; + + const mockDelete = { + where: jest.fn().mockReturnThis(), + execute: jest.fn().mockResolvedValue([]), + }; + + (mockDb.selectFrom as jest.Mock).mockReturnValue(mockSelect); + (mockDb.updateTable as jest.Mock).mockReturnValue(mockUpdate); + (mockDb.deleteFrom as jest.Mock).mockReturnValue(mockDelete); + // Steps 2-4 run inside makeKyselyTransactionWithRetry, which calls + // `pgQuery.transaction().execute(cb)`. Run the callback against mockDb. + (mockDb.transaction as jest.Mock).mockReturnValue({ + execute: (cb: (trx: typeof mockDb) => unknown) => cb(mockDb), + }); + + await sut.resetPasswordForToken({ + token: 'plaintext-token', + newPassword: 'new-password', + }); + + // The password row is updated... + expect(mockDb.updateTable).toHaveBeenCalledWith('public.users'); + // ...and the user's sessions are deleted so a phished session can't + // outlive the reset. + expect(mockDb.deleteFrom).toHaveBeenCalledWith('public.session'); + }, + ); + }); }); diff --git a/server/services/userManagementService/userManagementService.ts b/server/services/userManagementService/userManagementService.ts index f02d2bf..b04b6e2 100644 --- a/server/services/userManagementService/userManagementService.ts +++ b/server/services/userManagementService/userManagementService.ts @@ -7,6 +7,7 @@ import { makeNotFoundError, makeUnauthorizedError, } from '../../utils/errors.js'; +import { makeKyselyTransactionWithRetry } from '../../utils/kyselyTransactionWithRetry.js'; import { asyncRandomBytes } from '../../utils/misc.js'; import { HOUR_MS } from '../../utils/time.js'; import { CoopEmailAddress } from '../sendEmailService/sendEmailService.js'; @@ -17,6 +18,7 @@ import { type Invoker, type UserRole, } from './permissioning.js'; +import { deleteSessionsForUser } from './sessionPersistence.js'; import { hashPassword } from './utils.js'; class UserManagementService { @@ -457,18 +459,21 @@ class UserManagementService { return; } - // Step 2: reset password for that token's user - await this.pgQuery - .updateTable('public.users') - .set({ password: await hashPassword(newPassword) }) - .where('id', '=', fetchedToken.user_id) - .execute(); - - // Step 3: Delete all tokens for the user - await this.pgQuery - .deleteFrom('user_management_service.password_reset_tokens') - .where('user_id', '=', fetchedToken.user_id) - .execute(); + const hashedPassword = await hashPassword(newPassword); + // Atomic: a committed password change must not leave the user's sessions + // or reset tokens alive. + await makeKyselyTransactionWithRetry(this.pgQuery)(async (trx) => { + await trx + .updateTable('public.users') + .set({ password: hashedPassword }) + .where('id', '=', fetchedToken.user_id) + .execute(); + await deleteSessionsForUser(trx, fetchedToken.user_id); + await trx + .deleteFrom('user_management_service.password_reset_tokens') + .where('user_id', '=', fetchedToken.user_id) + .execute(); + }); } async getUsersForOrg(orgId: string) { -- 2.51.2