mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-07-01 20:01:35 +00:00
🔁 fix: Harden Google admin refresh against bot review findings
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.
This commit is contained in:
parent
d40c51616e
commit
1dddf97c4a
6 changed files with 212 additions and 21 deletions
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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';
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue