🛂 fix: Reject OpenID Email Fallback When Stored openidId Mismatches Token Sub (#12312)

* 🔐 fix: Reject OpenID email fallback when stored openidId mismatches token sub

When `findOpenIDUser` falls back to email lookup after the primary
`openidId`/`idOnTheSource` query fails, it now rejects any user whose
stored `openidId` differs from the incoming JWT subject claim. This
closes an account-takeover vector where a valid IdP JWT containing a
victim's email but a different `sub` could authenticate as the victim
when OPENID_REUSE_TOKENS is enabled.

The migration path (user has no `openidId` yet) is unaffected.

* test: Validate openidId mismatch guard in email fallback path

Update `findOpenIDUser` unit tests to assert that email-based lookups
returning a user with a different `openidId` are rejected with
AUTH_FAILED. Add matching integration test in `openIdJwtStrategy.spec`
exercising the full verify callback with the real `findOpenIDUser`.

* 🔐 fix: Remove redundant `openidId` truthiness check from mismatch guard

The `&& openidId` middle term in the guard condition caused it to be
bypassed when the incoming token `sub` was empty or undefined. Since
the JS callers can pass `payload?.sub` (which may be undefined), this
created a path where the guard never fired and the email fallback
returned the victim's account. Removing the term ensures the guard
rejects whenever the stored openidId differs from the incoming value,
regardless of whether the incoming value is falsy.

* test: Cover falsy openidId bypass and openidStrategy mismatch rejection

Add regression test for the guard bypass when `openidId` is an empty
string and the email lookup finds a user with a stored openidId.

Add integration test in openidStrategy.spec.js exercising the
mismatch rejection through the full processOpenIDAuth callback,
ensuring both OIDC paths (JWT reuse and standard callback) are
covered.

Restore intent-documenting comment on the no-provider fixture.

api/strategies/openIdJwtStrategy.spec.js

@@ -271,6 +271,32 @@ describe('openIdJwtStrategy – OPENID_EMAIL_CLAIM', () => {
     expect(user).toBe(false);
   });
 
+  it('should reject login when email fallback finds user with mismatched openidId', async () => {
+    const emailMatchWithDifferentSub = {
+      _id: 'user-id-2',
+      provider: 'openid',
+      openidId: 'different-sub',
+      email: payload.email,
+      role: SystemRoles.USER,
+    };
+
+    findUser.mockImplementation(async (query) => {
+      if (query.$or) {
+        return null;
+      }
+      if (query.email === payload.email) {
+        return emailMatchWithDifferentSub;
+      }
+      return null;
+    });
+
+    const req = { headers: { authorization: 'Bearer tok' }, session: {} };
+    const { user, info } = await invokeVerify(req, payload);
+
+    expect(user).toBe(false);
+    expect(info).toEqual({ message: 'auth_failed' });
+  });
+
   it('should trim whitespace from OPENID_EMAIL_CLAIM', async () => {
     process.env.OPENID_EMAIL_CLAIM = '  upn  ';
     findUser.mockResolvedValue(null);

api/strategies/openidStrategy.spec.js

@@ -356,6 +356,33 @@ describe('setupOpenId', () => {
     expect(updateUser).not.toHaveBeenCalled();
   });
 
+  it('should block login when email fallback finds user with mismatched openidId', async () => {
+    const existingUser = {
+      _id: 'existingUserId',
+      provider: 'openid',
+      openidId: 'different-sub-claim',
+      email: tokenset.claims().email,
+      username: 'existinguser',
+      name: 'Existing User',
+    };
+    findUser.mockImplementation(async (query) => {
+      if (query.$or) {
+        return null;
+      }
+      if (query.email === tokenset.claims().email) {
+        return existingUser;
+      }
+      return null;
+    });
+
+    const result = await validate(tokenset);
+
+    expect(result.user).toBe(false);
+    expect(result.details.message).toBe(ErrorTypes.AUTH_FAILED);
+    expect(createUser).not.toHaveBeenCalled();
+    expect(updateUser).not.toHaveBeenCalled();
+  });
+
   it('should enforce the required role and reject login if missing', async () => {
     // Arrange – simulate a token without the required role.
     jwtDecode.mockReturnValue({

packages/api/src/auth/openid.spec.ts

@@ -107,18 +107,18 @@ describe('findOpenIDUser', () => {
   });
 
   describe('Email-based searches', () => {
-    it('should find user by email when primary conditions fail', async () => {
+    it('should find user by email when primary conditions fail and openidId matches', async () => {
       const mockUser: IUser = {
         _id: 'user123',
         provider: 'openid',
-        openidId: 'openid_456',
+        openidId: 'openid_123',
         email: 'user@example.com',
         username: 'testuser',
       } as IUser;
 
       mockFindUser
-        .mockResolvedValueOnce(null) // Primary condition fails
-        .mockResolvedValueOnce(mockUser); // Email search succeeds
+        .mockResolvedValueOnce(null)
+        .mockResolvedValueOnce(mockUser);
 
       const result = await findOpenIDUser({
         openidId: 'openid_123',
@@ -202,7 +202,7 @@ describe('findOpenIDUser', () => {
       });
     });
 
-    it('should allow login when user has openid provider', async () => {
+    it('should reject email fallback when existing openidId does not match token sub', async () => {
       const mockUser: IUser = {
         _id: 'user123',
         provider: 'openid',
@@ -212,8 +212,34 @@ describe('findOpenIDUser', () => {
       } as IUser;
 
       mockFindUser
-        .mockResolvedValueOnce(null) // Primary condition fails
-        .mockResolvedValueOnce(mockUser); // Email search finds user with openid provider
+        .mockResolvedValueOnce(null)
+        .mockResolvedValueOnce(mockUser);
+
+      const result = await findOpenIDUser({
+        openidId: 'openid_123',
+        findUser: mockFindUser,
+        email: 'user@example.com',
+      });
+
+      expect(result).toEqual({
+        user: null,
+        error: ErrorTypes.AUTH_FAILED,
+        migration: false,
+      });
+    });
+
+    it('should allow email fallback when existing openidId matches token sub', async () => {
+      const mockUser: IUser = {
+        _id: 'user123',
+        provider: 'openid',
+        openidId: 'openid_123',
+        email: 'user@example.com',
+        username: 'testuser',
+      } as IUser;
+
+      mockFindUser
+        .mockResolvedValueOnce(null)
+        .mockResolvedValueOnce(mockUser);
 
       const result = await findOpenIDUser({
         openidId: 'openid_123',
@@ -259,7 +285,7 @@ describe('findOpenIDUser', () => {
       });
     });
 
-    it('should not migrate user who already has openidId', async () => {
+    it('should reject when user already has a different openidId', async () => {
       const mockUser: IUser = {
         _id: 'user123',
         provider: 'openid',
@@ -269,8 +295,8 @@ describe('findOpenIDUser', () => {
       } as IUser;
 
       mockFindUser
-        .mockResolvedValueOnce(null) // Primary condition fails
-        .mockResolvedValueOnce(mockUser); // Email search finds user with existing openidId
+        .mockResolvedValueOnce(null)
+        .mockResolvedValueOnce(mockUser);
 
       const result = await findOpenIDUser({
         openidId: 'openid_123',
@@ -279,24 +305,24 @@ describe('findOpenIDUser', () => {
       });
 
       expect(result).toEqual({
-        user: mockUser,
-        error: null,
+        user: null,
+        error: ErrorTypes.AUTH_FAILED,
         migration: false,
       });
     });
 
-    it('should handle user with no provider but existing openidId', async () => {
+    it('should reject when user has no provider but a different openidId', async () => {
       const mockUser: IUser = {
         _id: 'user123',
         openidId: 'existing_openid',
         email: 'user@example.com',
         username: 'testuser',
-        // No provider field
+        // No provider field — tests a different branch than openid-provider mismatch
       } as IUser;
 
       mockFindUser
-        .mockResolvedValueOnce(null) // Primary condition fails
-        .mockResolvedValueOnce(mockUser); // Email search finds user
+        .mockResolvedValueOnce(null)
+        .mockResolvedValueOnce(mockUser);
 
       const result = await findOpenIDUser({
         openidId: 'openid_123',
@@ -305,8 +331,8 @@ describe('findOpenIDUser', () => {
       });
 
       expect(result).toEqual({
-        user: mockUser,
-        error: null,
+        user: null,
+        error: ErrorTypes.AUTH_FAILED,
         migration: false,
       });
     });
@@ -398,14 +424,14 @@ describe('findOpenIDUser', () => {
       const mockUser: IUser = {
         _id: 'user123',
         provider: 'openid',
-        openidId: 'openid_456',
+        openidId: 'openid_123',
         email: 'user@example.com',
         username: 'testuser',
       } as IUser;
 
       mockFindUser
-        .mockResolvedValueOnce(null) // Primary condition fails
-        .mockResolvedValueOnce(mockUser); // Email search succeeds
+        .mockResolvedValueOnce(null)
+        .mockResolvedValueOnce(mockUser);
 
       const result = await findOpenIDUser({
         openidId: 'openid_123',
@@ -413,7 +439,6 @@ describe('findOpenIDUser', () => {
         email: 'User@Example.COM',
       });
 
-      /** Email is passed as-is; findUser implementation handles normalization */
       expect(mockFindUser).toHaveBeenNthCalledWith(2, { email: 'User@Example.COM' });
       expect(result).toEqual({
         user: mockUser,
@@ -432,5 +457,31 @@ describe('findOpenIDUser', () => {
         }),
       ).rejects.toThrow('Database error');
     });
+
+    it('should reject email fallback when openidId is empty and user has a stored openidId', async () => {
+      const mockUser: IUser = {
+        _id: 'user123',
+        provider: 'openid',
+        openidId: 'existing-real-id',
+        email: 'user@example.com',
+        username: 'testuser',
+      } as IUser;
+
+      mockFindUser.mockResolvedValueOnce(mockUser);
+
+      const result = await findOpenIDUser({
+        openidId: '',
+        findUser: mockFindUser,
+        email: 'user@example.com',
+      });
+
+      expect(mockFindUser).toHaveBeenCalledTimes(1);
+      expect(mockFindUser).toHaveBeenCalledWith({ email: 'user@example.com' });
+      expect(result).toEqual({
+        user: null,
+        error: ErrorTypes.AUTH_FAILED,
+        migration: false,
+      });
+    });
   });
 });

packages/api/src/auth/openid.ts

@@ -47,7 +47,13 @@ export async function findOpenIDUser({
       return { user: null, error: ErrorTypes.AUTH_FAILED, migration: false };
     }
 
-    // If user found by email but doesn't have openidId, prepare for migration
+    if (user?.openidId && user.openidId !== openidId) {
+      logger.warn(
+        `[${strategyName}] Rejected email fallback for ${user.email}: stored openidId does not match token sub`,
+      );
+      return { user: null, error: ErrorTypes.AUTH_FAILED, migration: false };
+    }
+
     if (user && !user.openidId) {
       logger.info(
         `[${strategyName}] Preparing user ${user.email} for migration to OpenID with sub: ${openidId}`,