fix(api): treat a session whose user no longer exists as logged out

.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.

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();
+  });
+});

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,
+    });
+  });
+});

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);
     })