From 11ab5f6ee5e5ae955a65bedf00f68a9c2a2d2ecc Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 19 Mar 2026 16:42:57 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=82=20fix:=20Reject=20OpenID=20Email?= =?UTF-8?q?=20Fallback=20When=20Stored=20`openidId`=20Mismatches=20Token?= =?UTF-8?q?=20Sub=20(#12312)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🔐 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 | 26 +++++++ api/strategies/openidStrategy.spec.js | 27 +++++++ packages/api/src/auth/openid.spec.ts | 95 ++++++++++++++++++------ packages/api/src/auth/openid.ts | 8 +- 4 files changed, 133 insertions(+), 23 deletions(-) diff --git a/api/strategies/openIdJwtStrategy.spec.js b/api/strategies/openIdJwtStrategy.spec.js index 79af848046..fd710f1ebd 100644 --- a/api/strategies/openIdJwtStrategy.spec.js +++ b/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); diff --git a/api/strategies/openidStrategy.spec.js b/api/strategies/openidStrategy.spec.js index 16fa548a59..4436fab672 100644 --- a/api/strategies/openidStrategy.spec.js +++ b/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({ diff --git a/packages/api/src/auth/openid.spec.ts b/packages/api/src/auth/openid.spec.ts index 7349508ce1..0761a24e85 100644 --- a/packages/api/src/auth/openid.spec.ts +++ b/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, + }); + }); }); }); diff --git a/packages/api/src/auth/openid.ts b/packages/api/src/auth/openid.ts index a7079ccd16..12ff48b2a9 100644 --- a/packages/api/src/auth/openid.ts +++ b/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}`,