From 97e1e950ad1c05ae2f680ec04ade24f44c055f9d Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 1 Sep 2026 02:02:21 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=B2=20fix:=20Draw=20OpenID=20Refresh?= =?UTF-8?q?=20Lookups=20Into=20System=20Context=20(#15451)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: scope refresh user lookups to system context * fix: preserve refresh system context * fix: bind refresh recovery to user tenant --- api/server/controllers/AuthController.js | 280 ++++++++++-------- api/server/controllers/AuthController.spec.js | 54 +++- 2 files changed, 210 insertions(+), 124 deletions(-) diff --git a/api/server/controllers/AuthController.js b/api/server/controllers/AuthController.js index 10cd6766a3..47f5faf5b7 100644 --- a/api/server/controllers/AuthController.js +++ b/api/server/controllers/AuthController.js @@ -1,7 +1,7 @@ const cookies = require('cookie'); const jwt = require('jsonwebtoken'); const crypto = require('node:crypto'); -const { logger } = require('@librechat/data-schemas'); +const { logger, runAsSystem, tenantStorage } = require('@librechat/data-schemas'); const { math, isEnabled, @@ -72,6 +72,14 @@ const sanitizeUserForAuthResponse = (user) => { return safeUser; }; +const runInUserTenant = (user, fn) => + user.tenantId + ? tenantStorage.run( + { tenantId: user.tenantId, userId: user._id.toString() }, + async () => await fn(), + ) + : runAsSystem(fn); + const getValidOpenIDReuseUserId = (parsedCookies, refreshToken) => { const openidUserId = parsedCookies.openid_user_id; if (!openidUserId || !process.env.JWT_REFRESH_SECRET) { @@ -345,7 +353,9 @@ const refreshController = async (req, res) => { context: 'refreshController', }, async (sendAuthorized) => { - const user = await getUserById(reuseUserId, AUTH_REFRESH_USER_PROJECTION); + const user = await runAsSystem(async () => + getUserById(reuseUserId, AUTH_REFRESH_USER_PROJECTION), + ); if (!user || !isReusableOpenIDSessionIdentity(reuseSessionTokens, user)) { return undefined; } @@ -378,99 +388,116 @@ const refreshController = async (req, res) => { const refreshUserId = req.session?.openidTokens?.appUserId ?? getValidOpenIDReuseUserId(parsedCookies); - const refreshUser = refreshUserId - ? await getUserById(refreshUserId, AUTH_REFRESH_USER_PROJECTION) - : null; + if (!refreshUserId) { + return res.status(403).send('Invalid OpenID refresh token'); + } + + const refreshUser = await runAsSystem(async () => + getUserById(refreshUserId, AUTH_REFRESH_USER_PROJECTION), + ); if (!refreshUser) { return res.status(403).send('Invalid OpenID refresh token'); } - let successfulRefreshToken = refreshToken; - let refreshResult; - try { - refreshResult = await refreshOpenIDUser({ - req, - res, - user: refreshUser, - refreshToken, - browserRefreshToken: parsedCookies.refreshToken, - strategyName: 'refreshController', - deferPublication: true, - }); - } catch (error) { - if (!fallbackRefreshToken || !isInvalidGrantError(error)) { - throw error; + return await runInUserTenant(refreshUser, async () => { + let successfulRefreshToken = refreshToken; + let refreshResult; + try { + refreshResult = await refreshOpenIDUser({ + req, + res, + user: refreshUser, + refreshToken, + browserRefreshToken: parsedCookies.refreshToken, + strategyName: 'refreshController', + deferPublication: true, + }); + } catch (error) { + if (!fallbackRefreshToken || !isInvalidGrantError(error)) { + throw error; + } + logger.info( + '[refreshController] Session refresh token was rejected; retrying the distinct browser token', + ); + successfulRefreshToken = fallbackRefreshToken; + refreshResult = await refreshOpenIDUser({ + req, + res, + user: refreshUser, + refreshToken: fallbackRefreshToken, + browserRefreshToken: parsedCookies.refreshToken, + strategyName: 'refreshController (browser fallback)', + deferPublication: true, + }); } - logger.info( - '[refreshController] Session refresh token was rejected; retrying the distinct browser token', - ); - successfulRefreshToken = fallbackRefreshToken; - refreshResult = await refreshOpenIDUser({ + const { tokenset, claims, openidIssuer, user, error, migration } = refreshResult; + + if (error || !user) { + logger.warn( + `[refreshController] Redirecting to /login: error=${error ?? 'null'}, user=${user ? 'exists' : 'null'}`, + ); + return res.status(401).redirect('/login'); + } + + if (user._id.toString() !== refreshUser._id.toString()) { + logger.warn( + '[refreshController] Refreshed identity resolved a different user; refusing token issuance', + { + refreshUserId: refreshUser._id.toString(), + resolvedUserId: user._id.toString(), + }, + ); + return res.status(401).redirect('/login'); + } + + // Handle migration: update user with openidId if found by email without openidId + // Also handle case where user has mismatched openidId (e.g., after database switch) + if (migration || user.openidId !== claims.sub) { + const reason = migration ? 'migration' : 'openidId mismatch'; + await updateUser(user._id.toString(), { + provider: 'openid', + openidId: claims.sub, + ...(openidIssuer ? { openidIssuer } : {}), + }); + logger.info( + `[refreshController] Updated user ${user.email} openidId (${reason}): ${user.openidId ?? 'null'} -> ${claims.sub}`, + ); + } + + if ( + successfulRefreshToken !== refreshToken && + req.session?.openidTokens?.refreshToken === refreshToken + ) { + delete req.session.openidTokens; + } + + const token = await sendOpenIDAuthResponse({ + tokenset, + user, + existingRefreshToken: successfulRefreshToken, + openidSubject: claims?.sub, + openidIssuer, + predecessorIdentity: { + userId: refreshUser._id.toString(), + tenantId: refreshUser.tenantId, + openidIssuer: refreshUser.openidIssuer, + }, + rejectedRefreshTokens: successfulRefreshToken === refreshToken ? [] : [refreshToken], req, res, - user: refreshUser, - refreshToken: fallbackRefreshToken, - browserRefreshToken: parsedCookies.refreshToken, - strategyName: 'refreshController (browser fallback)', - deferPublication: true, }); - } - const { tokenset, claims, openidIssuer, user, error, migration } = refreshResult; - - if (error || !user) { - logger.warn( - `[refreshController] Redirecting to /login: error=${error ?? 'null'}, user=${user ? 'exists' : 'null'}`, + return await withOpenIDResponseDelivery( + { + res, + openidTokens: req.session?.openidTokens, + context: 'refreshController', + }, + (sendAuthorized) => + sendAuthorized(() => + res.status(200).send({ token, user: sanitizeUserForAuthResponse(user) }), + ), ); - return res.status(401).redirect('/login'); - } - - // Handle migration: update user with openidId if found by email without openidId - // Also handle case where user has mismatched openidId (e.g., after database switch) - if (migration || user.openidId !== claims.sub) { - const reason = migration ? 'migration' : 'openidId mismatch'; - await updateUser(user._id.toString(), { - provider: 'openid', - openidId: claims.sub, - ...(openidIssuer ? { openidIssuer } : {}), - }); - logger.info( - `[refreshController] Updated user ${user.email} openidId (${reason}): ${user.openidId ?? 'null'} -> ${claims.sub}`, - ); - } - - if ( - successfulRefreshToken !== refreshToken && - req.session?.openidTokens?.refreshToken === refreshToken - ) { - delete req.session.openidTokens; - } - - const token = await sendOpenIDAuthResponse({ - tokenset, - user, - existingRefreshToken: successfulRefreshToken, - openidSubject: claims?.sub, - openidIssuer, - predecessorIdentity: { - userId: refreshUser._id.toString(), - tenantId: refreshUser.tenantId, - openidIssuer: refreshUser.openidIssuer, - }, - rejectedRefreshTokens: successfulRefreshToken === refreshToken ? [] : [refreshToken], - req, - res, }); - return await withOpenIDResponseDelivery( - { - res, - openidTokens: req.session?.openidTokens, - context: 'refreshController', - }, - (sendAuthorized) => - sendAuthorized(() => - res.status(200).send({ token, user: sanitizeUserForAuthResponse(user) }), - ), - ); } catch (error) { if (isOpenIDRefreshOwnershipError(error)) { clearOpenIDAuthTokens( @@ -494,53 +521,60 @@ const refreshController = async (req, res) => { const userId = getValidOpenIDReuseUserId(parsedCookies, bridgeSourceToken); if (userId) { try { - const bridgeUser = await getUserById(userId, AUTH_REFRESH_USER_PROJECTION); + const bridgeUser = await runAsSystem(async () => + getUserById(userId, AUTH_REFRESH_USER_PROJECTION), + ); if (!bridgeUser) { return res.status(403).send('Invalid OpenID refresh token'); } - const bridgedRefreshToken = await getRefreshTokenBridge({ - oldRefreshToken: bridgeSourceToken, - userId, - tenantId: bridgeUser.tenantId, - openidIssuer: bridgeUser.openidIssuer, - }); + const bridgeResponse = await runInUserTenant(bridgeUser, async () => { + const bridgedRefreshToken = await getRefreshTokenBridge({ + oldRefreshToken: bridgeSourceToken, + userId, + tenantId: bridgeUser.tenantId, + openidIssuer: bridgeUser.openidIssuer, + }); - if (bridgedRefreshToken) { - logger.info( - '[refreshController] Recovered via refresh-token bridge after invalid_grant', - { - userId, - }, - ); - - try { - const { appAuthToken } = await recoverOpenIDRefreshBridge({ - req, - res, - refreshToken: bridgeSourceToken, - bridgedRefreshToken, - bridgeUser, - }); - - return await withOpenIDResponseDelivery( + if (bridgedRefreshToken) { + logger.info( + '[refreshController] Recovered via refresh-token bridge after invalid_grant', { - res, - openidTokens: req.session?.openidTokens, - context: 'refreshController', + userId, }, - (sendAuthorized) => - sendAuthorized(() => - res.status(200).send({ - token: appAuthToken, - user: sanitizeUserForAuthResponse(bridgeUser), - }), - ), ); - } catch (retryError) { - logger.error('[refreshController] Bridge recovery retry failed', retryError); - // Fall through to generic error response + + try { + const { appAuthToken } = await recoverOpenIDRefreshBridge({ + req, + res, + refreshToken: bridgeSourceToken, + bridgedRefreshToken, + bridgeUser, + }); + + return await withOpenIDResponseDelivery( + { + res, + openidTokens: req.session?.openidTokens, + context: 'refreshController', + }, + (sendAuthorized) => + sendAuthorized(() => + res.status(200).send({ + token: appAuthToken, + user: sanitizeUserForAuthResponse(bridgeUser), + }), + ), + ); + } catch (retryError) { + logger.error('[refreshController] Bridge recovery retry failed', retryError); + // Fall through to generic error response + } } + }); + if (bridgeResponse !== undefined) { + return bridgeResponse; } } catch (bridgeError) { logger.warn('[refreshController] Refresh-token bridge lookup failed', { diff --git a/api/server/controllers/AuthController.spec.js b/api/server/controllers/AuthController.spec.js index b9fd676bae..d597e82edc 100644 --- a/api/server/controllers/AuthController.spec.js +++ b/api/server/controllers/AuthController.spec.js @@ -1,5 +1,26 @@ +let mockActiveTenantId; +const mockRunAsSystem = jest.fn(async (fn) => { + const previousTenantId = mockActiveTenantId; + mockActiveTenantId = '__SYSTEM__'; + try { + return await fn(); + } finally { + mockActiveTenantId = previousTenantId; + } +}); +const mockTenantStorageRun = jest.fn(async (context, fn) => { + const previousTenantId = mockActiveTenantId; + mockActiveTenantId = context.tenantId; + try { + return await fn(); + } finally { + mockActiveTenantId = previousTenantId; + } +}); jest.mock('@librechat/data-schemas', () => ({ logger: { error: jest.fn(), debug: jest.fn(), warn: jest.fn(), info: jest.fn() }, + runAsSystem: (fn) => mockRunAsSystem(fn), + tenantStorage: { run: (context, fn) => mockTenantStorageRun(context, fn) }, })); jest.mock('~/server/services/GraphTokenService', () => ({ getGraphApiToken: jest.fn(), @@ -506,6 +527,7 @@ describe('refreshController – OpenID path', () => { beforeEach(() => { jest.clearAllMocks(); + mockActiveTenantId = undefined; delete process.env.OPENID_SCOPE; delete process.env.OPENID_REFRESH_AUDIENCE; process.env.JWT_REFRESH_SECRET = 'test-refresh-secret'; @@ -900,6 +922,7 @@ describe('refreshController – OpenID path', () => { 'user-db-id', '-password -__v -totpSecret -backupCodes -federatedTokens', ); + expect(mockRunAsSystem).toHaveBeenCalledTimes(1); expect(setCloudFrontAuthCookies).toHaveBeenCalledWith(req, res, user); expect(res.status).toHaveBeenCalledWith(200); expect(res.send).toHaveBeenCalledWith({ @@ -1049,10 +1072,15 @@ describe('refreshController – OpenID path', () => { openidIssuer: baseClaims.iss, }, }; + findOpenIDUser.mockImplementationOnce(async () => { + expect(mockActiveTenantId).toBe('tenant-1'); + return { user: { ...defaultUser }, error: null, migration: false }; + }); await refreshController(req, res); expect(getUserById).toHaveBeenCalled(); + expect(mockRunAsSystem).toHaveBeenCalledTimes(1); expect(setCloudFrontAuthCookies).not.toHaveBeenCalled(); expectOpenIDRefreshGrant(); }); @@ -1389,6 +1417,27 @@ describe('refreshController – OpenID path', () => { expect(res.redirect).toHaveBeenCalledWith('/login'); }); + it('rejects a refreshed identity that resolves to a different user', async () => { + findOpenIDUser.mockResolvedValue({ + user: { ...defaultUser, _id: 'different-user-id' }, + error: null, + migration: false, + }); + + await refreshController(req, res); + + expect(setOpenIDAuthTokens).not.toHaveBeenCalled(); + expect(logger.warn).toHaveBeenCalledWith( + '[refreshController] Refreshed identity resolved a different user; refusing token issuance', + { + refreshUserId: 'user-db-id', + resolvedUserId: 'different-user-id', + }, + ); + expect(res.status).toHaveBeenCalledWith(401); + expect(res.redirect).toHaveBeenCalledWith('/login'); + }); + it('should preserve invalid OpenID refresh token behavior', async () => { openIdClient.refreshTokenGrant.mockRejectedValue(new Error('invalid_grant')); @@ -1446,7 +1495,10 @@ describe('refreshController – OpenID path', () => { tenantId: 'tenant-1', openidIssuer: 'https://issuer.example.com', }); - getRefreshTokenBridge.mockResolvedValue('bridged-refresh'); + getRefreshTokenBridge.mockImplementationOnce(async () => { + expect(mockActiveTenantId).toBe('tenant-1'); + return 'bridged-refresh'; + }); const nonRotatingTokenset = { ...mockTokenset }; delete nonRotatingTokenset.refresh_token; openIdClient.refreshTokenGrant