From f31a2c526ea31f6ff7ae2c5fa2d5b63ec0defa25 Mon Sep 17 00:00:00 2001 From: Prince Rawat <63495981+rawatprince@users.noreply.github.com> Date: Tue, 11 Aug 2026 20:31:58 +0530 Subject: [PATCH] fix(api): treat a session whose user no longer exists as logged out --- .../deleted-user-session-unauthenticated.md | 15 +++ .../src/utils/__tests__/passport.int.test.ts | 94 +++++++++++++++++++ .../api/src/utils/__tests__/passport.test.ts | 54 +++++++++++ packages/api/src/utils/passport.ts | 7 +- 4 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 .changeset/deleted-user-session-unauthenticated.md create mode 100644 packages/api/src/utils/__tests__/passport.int.test.ts create mode 100644 packages/api/src/utils/__tests__/passport.test.ts diff --git a/.changeset/deleted-user-session-unauthenticated.md b/.changeset/deleted-user-session-unauthenticated.md new file mode 100644 index 0000000000..50355f8727 --- /dev/null +++ b/.changeset/deleted-user-session-unauthenticated.md @@ -0,0 +1,15 @@ +--- +'@hyperdx/api': patch +--- + +Treat a session whose user no longer exists as logged out instead of failing the +request. Deleting a team member left that person's browser holding a session +cookie pointing at a user document that was gone, and `deserializeUser` reported +the missing user as an error rather than as an unauthenticated session. Because +`passport.session()` runs ahead of every router, each request carrying the +cookie came back `500 Something went wrong :(` regardless of path or method, +including public routes such as `POST /team/setup/:token` and `GET /logout`, so +a removed person could neither accept a fresh invite nor clear their own +session. The stale id is now dropped from the session and the request continues +unauthenticated, so protected routes answer 401 and the browser is sent back to +the login page. diff --git a/packages/api/src/utils/__tests__/passport.int.test.ts b/packages/api/src/utils/__tests__/passport.int.test.ts new file mode 100644 index 0000000000..0954f53338 --- /dev/null +++ b/packages/api/src/utils/__tests__/passport.int.test.ts @@ -0,0 +1,94 @@ +import { getAgent, getLoggedInAgent, getServer } from '@/fixtures'; +import TeamInvite from '@/models/teamInvite'; +import User from '@/models/user'; + +const MEMBER_EMAIL = 'removed-member@example.com'; +// Repeats the password from `MOCK_USER` in fixtures.ts, which is not exported. +// It has to satisfy the same complexity rules the setup route enforces. +const MEMBER_PASSWORD = 'TacoCat!2#4X'; + +describe('passport session deserialization', () => { + const server = getServer(); + + beforeAll(async () => { + await server.start(); + }); + + afterEach(async () => { + await server.clearDBs(); + }); + + afterAll(async () => { + await server.stop(); + }); + + const findInvite = async () => { + const invite = await TeamInvite.findOne({ email: MEMBER_EMAIL }); + if (invite == null) { + throw new Error('TeamInvite not found'); + } + return invite; + }; + + // Registers the admin, invites MEMBER_EMAIL and accepts the invite on a + // second agent, so `memberAgent` ends up holding that member's session + // cookie the way a real browser would. + const setUpTeamWithMember = async () => { + const { agent: adminAgent } = await getLoggedInAgent(server); + + await adminAgent + .post('/team/invitation') + .send({ email: MEMBER_EMAIL, name: 'Removed Member' }) + .expect(200); + + const memberAgent = getAgent(server); + await memberAgent + .post(`/team/setup/${(await findInvite()).token}`) + .send({ password: MEMBER_PASSWORD }) + .expect(303); + + const member = await User.findOne({ email: MEMBER_EMAIL }); + if (member == null) { + throw new Error('member not found'); + } + + return { adminAgent, memberAgent, member }; + }; + + it('answers 401 once the session user has been deleted', async () => { + const { adminAgent, memberAgent, member } = await setUpTeamWithMember(); + + await memberAgent.get('/team').expect(200); + + await adminAgent.delete(`/team/member/${member._id}`).expect(200); + + // The member's browser still holds its connect.sid cookie. Before this + // fix, deserializeUser answered `done(new Error('User not found'))`, so + // every request carrying that cookie came back 500 + // {"message":"Something went wrong :("}. + await memberAgent.get('/team').expect(401); + }); + + it('lets a deleted user accept a fresh invite with their stale cookie', async () => { + const { adminAgent, memberAgent, member } = await setUpTeamWithMember(); + + await adminAgent.delete(`/team/member/${member._id}`).expect(200); + + // Re-invite the same person. Their browser still has the cookie from the + // deleted account, and the invite is accepted on a public route that never + // required authentication in the first place. + await adminAgent + .post('/team/invitation') + .send({ email: MEMBER_EMAIL, name: 'Removed Member' }) + .expect(200); + + const invite = await findInvite(); + await memberAgent + .post(`/team/setup/${invite.token}`) + .send({ password: MEMBER_PASSWORD }) + .expect(303); + + expect(await User.findOne({ email: MEMBER_EMAIL })).not.toBeNull(); + expect(await TeamInvite.findById(invite._id)).toBeNull(); + }); +}); diff --git a/packages/api/src/utils/__tests__/passport.test.ts b/packages/api/src/utils/__tests__/passport.test.ts new file mode 100644 index 0000000000..93c6e5f10f --- /dev/null +++ b/packages/api/src/utils/__tests__/passport.test.ts @@ -0,0 +1,54 @@ +import passport from '@/utils/passport'; + +const mockFindUserById = jest.fn(); + +jest.mock('@/controllers/user', () => ({ + findUserById: (id: string) => mockFindUserById(id), +})); + +// The two-argument form of `deserializeUser` is passport's own private +// implementation: it walks the same registered deserializer chain that +// `passport.session()` drives on every request. The session strategy also +// passes the request along, which is immaterial here because the deserializer +// registered above takes only `(id, done)`. +const deserializeSession = (id: string) => + new Promise<{ err: unknown; user: unknown }>(resolve => { + passport.deserializeUser(id, (err, user) => resolve({ err, user })); + }); + +describe('utils/passport', () => { + afterEach(() => { + mockFindUserById.mockReset(); + }); + + it('deserializes the session user', async () => { + const user = { _id: 'user-id', email: 'user@example.com' }; + mockFindUserById.mockResolvedValue(user); + + expect(await deserializeSession('user-id')).toEqual({ err: null, user }); + }); + + it('treats a deleted session user as unauthenticated, not as an error', async () => { + mockFindUserById.mockResolvedValue(null); + + // `user: false` is what passport makes of an explicit `done(null, false)`: + // it clears the stale id from the session and leaves the request + // unauthenticated. An error here fails every request for as long as the + // browser keeps the cookie, and so does a bare `done(null)`, which + // passport reports as "Failed to deserialize user out of session". + expect(await deserializeSession('deleted-user-id')).toEqual({ + err: null, + user: false, + }); + }); + + it('still surfaces lookup failures', async () => { + const err = new Error('mongo is down'); + mockFindUserById.mockRejectedValue(err); + + expect(await deserializeSession('user-id')).toEqual({ + err, + user: undefined, + }); + }); +}); diff --git a/packages/api/src/utils/passport.ts b/packages/api/src/utils/passport.ts index ab91ed8985..99b89243e3 100644 --- a/packages/api/src/utils/passport.ts +++ b/packages/api/src/utils/passport.ts @@ -15,7 +15,12 @@ passport.deserializeUser(function (id: string, done) { findUserById(id) .then(user => { if (user == null) { - return done(new Error('User not found')); + // The session outlived the user, e.g. the account was deleted. That is + // not a server fault, and reporting it as one turned every request + // still carrying the cookie into a 500, public routes included, so the + // browser could not even reach the login page. `false` is passport's + // "no such user" signal: the request continues unauthenticated. + return done(null, false); } done(null, user as UserDocument); })