From 1dddf97c4a7191d357383ecfb31f1eeaa9eade7e Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Thu, 18 Jun 2026 11:50:52 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=81=20fix:=20Harden=20Google=20admin?= =?UTF-8?q?=20refresh=20against=20bot=20review=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five validated findings from the initial bot pass: socialLogin.js: mirror the OpenID migrate-or-reject pattern on the email fallback. When an existing user is found by email and the stored provider id is empty, persist the refreshed sub so the refresh path can later bind to it. When the stored id is present and differs, reject as AUTH_FAILED to prevent identity-swap, matching the existing OpenID behavior in packages/api/src/auth/openid.ts. oauth.js: scope the non-OpenID admin refresh-token forwarding to provider === 'google'. The previous else branch would have forwarded a Discord refresh token (passport-discord supplies one) into the admin exchange payload even though /api/admin/oauth/refresh only accepts openid or google, leaving the admin client with a token it could not refresh. admin/auth.js (refreshGoogleAdminSession): drop id_token from the mandatory-fields check. Google's OAuth refresh response is documented to include id_token only conditionally, so the previous mandatory check broke refresh whenever Google omitted it. Decode id_token when present (fast path); when absent, call Google's userinfo endpoint with the access token to read sub. Wrap tokenResponse.json() in try/catch and return IDP_INCOMPLETE on parse failure instead of a generic 500. Tighten access_token to a typeof string check. admin/auth.js (refreshGoogleAdminSession): reuse serializeUserForExchange for the response user so the Google refresh shape matches /oauth/exchange and the OpenID branch exactly (full _id, id, email, name, username, role, avatar, provider, openidId). The previous Google-specific subset dropped fields the admin client relies on for later provider-specific refreshes and disambiguation. Tests cover each fix: socialLogin's migration and rejection cases, the oauth.js Discord-gating case, the userinfo fallback path on missing id_token, CLAIMS_INCOMPLETE when both id_token and userinfo are absent, IDP_INCOMPLETE on a non-JSON token body, and the full response shape on the happy path. --- api/server/controllers/auth/oauth.js | 2 +- api/server/controllers/auth/oauth.spec.js | 22 +++++ api/server/routes/admin/auth.js | 63 +++++++++++--- api/server/routes/admin/auth.refresh.test.js | 90 +++++++++++++++++++- api/strategies/socialLogin.js | 14 ++- api/strategies/socialLogin.test.js | 42 ++++++++- 6 files changed, 212 insertions(+), 21 deletions(-) diff --git a/api/server/controllers/auth/oauth.js b/api/server/controllers/auth/oauth.js index 2f265ba227..4d40a2f04c 100644 --- a/api/server/controllers/auth/oauth.js +++ b/api/server/controllers/auth/oauth.js @@ -49,7 +49,7 @@ function createOAuthHandler(redirectUri = domains.client) { refreshToken = req.user.tokenset?.refresh_token || req.user.federatedTokens?.refresh_token; } - } else { + } else if (req.user.provider === 'google') { refreshToken = req.authInfo?.refreshToken; } const expiresAt = Date.now() + sessionExpiry; diff --git a/api/server/controllers/auth/oauth.spec.js b/api/server/controllers/auth/oauth.spec.js index d579e7524e..d9984a8dd5 100644 --- a/api/server/controllers/auth/oauth.spec.js +++ b/api/server/controllers/auth/oauth.spec.js @@ -191,4 +191,26 @@ describe('createOAuthHandler', () => { expect.any(Number), ); }); + + it('does not forward refresh tokens for admin providers other than google or openid', async () => { + const handler = createOAuthHandler('http://admin.example.com/auth/discord/callback'); + const req = buildReq({ + user: { _id: 'user-9', email: 'd@example.com', provider: 'discord' }, + authInfo: { refreshToken: 'discord-refresh-token' }, + }); + const res = buildRes(); + const next = jest.fn(); + + await handler(req, res, next); + + expect(mockGenerateAdminExchangeCode).toHaveBeenCalledWith( + {}, + req.user, + 'jwt-token', + undefined, + 'http://admin.example.com', + 'pkce-challenge', + expect.any(Number), + ); + }); }); diff --git a/api/server/routes/admin/auth.js b/api/server/routes/admin/auth.js index a8b6775ce6..02da392795 100644 --- a/api/server/routes/admin/auth.js +++ b/api/server/routes/admin/auth.js @@ -20,6 +20,7 @@ const { applyAdminRefresh, AdminRefreshError, buildOpenIDRefreshParams, + serializeUserForExchange, } = require('@librechat/api'); const { loginController } = require('~/server/controllers/auth/LoginController'); const { hasCapability, requireCapability } = require('~/server/middleware/roles/capabilities'); @@ -37,6 +38,7 @@ const { getOpenIdConfig } = require('~/strategies'); const middleware = require('~/server/middleware'); const GOOGLE_TOKEN_ENDPOINT = 'https://oauth2.googleapis.com/token'; +const GOOGLE_USERINFO_ENDPOINT = 'https://openidconnect.googleapis.com/v1/userinfo'; const requireAdminAccess = requireCapability(SystemCapabilities.ACCESS_ADMIN); @@ -51,6 +53,28 @@ function decodeJwtPayload(token) { } } +async function resolveGoogleSubFromUserinfo(accessToken) { + try { + const response = await fetch(GOOGLE_USERINFO_ENDPOINT, { + headers: { Authorization: `Bearer ${accessToken}` }, + }); + if (!response.ok) { + logger.warn('[admin/oauth/refresh] Google userinfo fallback returned non-OK', { + status: response.status, + }); + return undefined; + } + const body = await response.json().catch(() => undefined); + return typeof body?.sub === 'string' ? body.sub : undefined; + } catch (err) { + logger.warn('[admin/oauth/refresh] Google userinfo fallback failed', { + name: err?.name, + message: err?.message, + }); + return undefined; + } +} + async function refreshGoogleAdminSession({ refreshToken, userId, tenantId, sessionExpiry }) { if (!process.env.GOOGLE_CLIENT_ID || !process.env.GOOGLE_CLIENT_SECRET) { throw new AdminRefreshError( @@ -87,25 +111,42 @@ async function refreshGoogleAdminSession({ refreshToken, userId, tenantId, sessi throw new AdminRefreshError('REFRESH_FAILED', 401, 'Refresh failed'); } - const tokenset = await tokenResponse.json(); - if (!tokenset.access_token || !tokenset.id_token) { + let tokenset; + try { + tokenset = await tokenResponse.json(); + } catch (err) { + logger.warn('[admin/oauth/refresh] Google returned non-JSON body', { + name: err?.name, + message: err?.message, + }); + throw new AdminRefreshError('IDP_INCOMPLETE', 502, 'Google returned a non-JSON token response'); + } + if (typeof tokenset?.access_token !== 'string') { throw new AdminRefreshError( 'IDP_INCOMPLETE', 502, - 'Google returned a tokenset missing access_token or id_token', + 'Google returned a tokenset missing access_token', ); } - const claims = decodeJwtPayload(tokenset.id_token); - if (!claims?.sub) { + let googleId; + if (typeof tokenset.id_token === 'string') { + const claims = decodeJwtPayload(tokenset.id_token); + if (typeof claims?.sub === 'string') { + googleId = claims.sub; + } + } + if (!googleId) { + googleId = await resolveGoogleSubFromUserinfo(tokenset.access_token); + } + if (!googleId) { throw new AdminRefreshError( 'CLAIMS_INCOMPLETE', 502, - 'Google id_token has no readable claims or no sub', + 'Could not resolve google sub from refresh response', ); } - const googleId = claims.sub; const SAFE_USER_PROJECTION = '-password -__v -totpSecret -backupCodes'; let user; @@ -162,17 +203,11 @@ async function refreshGoogleAdminSession({ refreshToken, userId, tenantId, sessi const token = await generateToken(user, sessionExpiry); const expiresAt = Date.now() + sessionExpiry; - const responseUser = { - id: user.id ?? user._id?.toString(), - email: user.email, - name: user.name, - role: user.role, - }; return { token, refreshToken: tokenset.refresh_token ?? refreshToken, - user: responseUser, + user: serializeUserForExchange(user), expiresAt, }; } diff --git a/api/server/routes/admin/auth.refresh.test.js b/api/server/routes/admin/auth.refresh.test.js index 292789dcc7..e15225ccaf 100644 --- a/api/server/routes/admin/auth.refresh.test.js +++ b/api/server/routes/admin/auth.refresh.test.js @@ -54,6 +54,20 @@ jest.mock('@librechat/api', () => { } return params; }), + serializeUserForExchange: jest.fn((user) => { + const userId = String(user._id); + return { + _id: userId, + id: userId, + email: user.email, + name: user.name ?? '', + username: user.username ?? '', + role: user.role ?? 'USER', + avatar: user.avatar, + provider: user.provider, + openidId: user.openidId, + }; + }), }; }); @@ -291,11 +305,13 @@ describe('admin auth Google refresh route', () => { findUsers.mockResolvedValue([ { - _id: { toString: () => 'user-id' }, + _id: 'user-id', id: 'user-id', email: 'admin@example.com', name: 'Admin', + username: 'admin', role: 'ADMIN', + provider: 'google', googleId: 'google-admin-id', }, ]); @@ -331,10 +347,13 @@ describe('admin auth Google refresh route', () => { token: 'admin-jwt', refreshToken: 'incoming-google-refresh', user: { + _id: 'user-id', id: 'user-id', email: 'admin@example.com', name: 'Admin', + username: 'admin', role: 'ADMIN', + provider: 'google', }, expiresAt: expect.any(Number), }); @@ -388,12 +407,12 @@ describe('admin auth Google refresh route', () => { expect(response.body.error_code).toBe('REFRESH_FAILED'); }); - it('returns IDP_INCOMPLETE when Google omits access_token or id_token', async () => { + it('returns IDP_INCOMPLETE when Google omits access_token', async () => { global.fetch = jest.fn(() => Promise.resolve({ ok: true, status: 200, - json: () => Promise.resolve({ access_token: 'only-access' }), + json: () => Promise.resolve({ id_token: validIdToken() }), }), ); @@ -405,6 +424,71 @@ describe('admin auth Google refresh route', () => { expect(response.body.error_code).toBe('IDP_INCOMPLETE'); }); + it('returns IDP_INCOMPLETE when Google returns a non-JSON token body', async () => { + global.fetch = jest.fn(() => + Promise.resolve({ + ok: true, + status: 200, + json: () => Promise.reject(new SyntaxError('Unexpected token < in JSON at position 0')), + }), + ); + + const response = await request(app) + .post('/api/admin/oauth/refresh') + .send({ refresh_token: 'incoming-google-refresh', provider: 'google' }); + + expect(response.status).toBe(502); + expect(response.body.error_code).toBe('IDP_INCOMPLETE'); + }); + + it('falls back to Google userinfo when the refresh response omits id_token', async () => { + const tokenCall = jest.fn(() => + Promise.resolve({ + ok: true, + status: 200, + json: () => Promise.resolve({ access_token: 'google-new-access' }), + }), + ); + const userinfoCall = jest.fn(() => + Promise.resolve({ + ok: true, + status: 200, + json: () => Promise.resolve({ sub: 'google-admin-id' }), + }), + ); + global.fetch = jest.fn((url) => + url === 'https://openidconnect.googleapis.com/v1/userinfo' ? userinfoCall() : tokenCall(), + ); + + const response = await request(app) + .post('/api/admin/oauth/refresh') + .send({ refresh_token: 'incoming-google-refresh', provider: 'google' }); + + expect(response.status).toBe(200); + expect(response.body.user._id).toBe('user-id'); + expect(userinfoCall).toHaveBeenCalled(); + }); + + it('returns CLAIMS_INCOMPLETE when id_token is absent and userinfo also fails', async () => { + global.fetch = jest.fn((url) => { + if (url === 'https://openidconnect.googleapis.com/v1/userinfo') { + return Promise.resolve({ ok: false, status: 401, json: () => Promise.resolve({}) }); + } + return Promise.resolve({ + ok: true, + status: 200, + json: () => Promise.resolve({ access_token: 'google-new-access' }), + }); + }); + + const response = await request(app) + .post('/api/admin/oauth/refresh') + .send({ refresh_token: 'incoming-google-refresh', provider: 'google' }); + + expect(response.status).toBe(502); + expect(response.body.error_code).toBe('CLAIMS_INCOMPLETE'); + }); + it('returns CLAIMS_INCOMPLETE when Google id_token has no sub', async () => { const header = Buffer.from(JSON.stringify({ alg: 'RS256' })).toString('base64url'); const payload = Buffer.from(JSON.stringify({})).toString('base64url'); diff --git a/api/strategies/socialLogin.js b/api/strategies/socialLogin.js index 9eac62fb9a..dd8241e72a 100644 --- a/api/strategies/socialLogin.js +++ b/api/strategies/socialLogin.js @@ -3,7 +3,7 @@ const { ErrorTypes } = require('librechat-data-provider'); const { isEnabled, isEmailDomainAllowed, resolveAppConfigForUser } = require('@librechat/api'); const { createSocialUser, handleExistingUser } = require('./process'); const { getAppConfig } = require('~/server/services/Config'); -const { findUser } = require('~/models'); +const { findUser, updateUser } = require('~/models'); const socialLogin = (provider, getProfileDetails, options = {}) => @@ -59,6 +59,18 @@ const socialLogin = refreshToken ? cb(null, user, { refreshToken }) : cb(null, user); if (existingUser?.provider === provider) { + if (id && existingUser[providerKey] && existingUser[providerKey] !== id) { + logger.warn( + `[${provider}Login] Rejected email fallback for ${email}: stored ${providerKey} does not match`, + ); + const error = new Error(ErrorTypes.AUTH_FAILED); + error.code = ErrorTypes.AUTH_FAILED; + return cb(error); + } + if (id && !existingUser[providerKey]) { + await updateUser(existingUser._id, { [providerKey]: id }); + existingUser[providerKey] = id; + } await handleExistingUser(existingUser, avatarUrl, appConfig, email); return passResult(existingUser); } else if (existingUser) { diff --git a/api/strategies/socialLogin.test.js b/api/strategies/socialLogin.test.js index a24f13e511..d5c96c6f3f 100644 --- a/api/strategies/socialLogin.test.js +++ b/api/strategies/socialLogin.test.js @@ -35,6 +35,7 @@ jest.mock('@librechat/api', () => ({ jest.mock('~/models', () => ({ findUser: jest.fn(), + updateUser: jest.fn(), })); jest.mock('~/server/services/Config', () => ({ @@ -138,7 +139,8 @@ describe('socialLogin', () => { expect(callback).toHaveBeenCalledWith(null, existingUser); }); - it('should fallback to finding user by email if not found by provider ID', async () => { + it('migrates the missing provider id when finding by email fallback', async () => { + const { updateUser } = require('~/models'); const provider = 'google'; const googleId = 'google-user-789'; const email = 'user@example.com'; @@ -147,7 +149,6 @@ describe('socialLogin', () => { _id: 'user789', email: email, provider: 'google', - googleId: 'old-google-id', }; findUser.mockResolvedValueOnce(null).mockResolvedValueOnce(existingUser); @@ -172,10 +173,47 @@ describe('socialLogin', () => { `[${provider}Login] User found by email: ${email} but not by ${provider}Id`, ); + expect(updateUser).toHaveBeenCalledWith('user789', { googleId }); + expect(existingUser.googleId).toBe(googleId); expect(handleExistingUser).toHaveBeenCalled(); expect(callback).toHaveBeenCalledWith(null, existingUser); }); + it('rejects the email fallback when the stored provider id differs from the current sub', async () => { + const provider = 'google'; + const googleId = 'google-user-new'; + const email = 'user@example.com'; + + const existingUser = { + _id: 'user789', + email: email, + provider: 'google', + googleId: 'google-user-old', + }; + + findUser.mockResolvedValueOnce(null).mockResolvedValueOnce(existingUser); + + const mockProfile = { + id: googleId, + emails: [{ value: email, verified: true }], + photos: [{ value: 'https://example.com/avatar.png' }], + name: { givenName: 'Bob', familyName: 'Johnson' }, + }; + + const loginFn = socialLogin(provider, mockGetProfileDetails); + const callback = jest.fn(); + + await loginFn(null, null, null, mockProfile, callback); + + expect(logger.warn).toHaveBeenCalledWith( + `[${provider}Login] Rejected email fallback for ${email}: stored ${provider}Id does not match`, + ); + expect(handleExistingUser).not.toHaveBeenCalled(); + expect(callback).toHaveBeenCalledWith( + expect.objectContaining({ code: ErrorTypes.AUTH_FAILED }), + ); + }); + it('should create new user if not found by provider ID or email', async () => { const provider = 'google'; const googleId = 'google-new-user';