mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-03 22:32:42 +00:00
🔒 fix: Apply brutal-review hardening to Google admin refresh
Tighten the Google OAuth refresh flow against all outstanding code review findings: enforce JWT aud claim verification against the configured clientId (ISSUER_MISMATCH on mismatch), reject ambiguous googleId matches (limit:2 in findUsers, USER_ID_MISMATCH when multiple rows match), scope the authInfo refresh-token carrier to the Google provider only, add TOCTOU re-read defense after the admin googleId migration write in socialLogin, deduplicate canAccessAdmin/mintToken closures via buildAdminRefreshClosures shared by both OpenID and Google refresh paths, document rotation semantics on AdminExchangeResponse.refreshToken, standardise all log prefixes to [admin/oauth/refresh], and expand test coverage for all new paths.
This commit is contained in:
Dustin Healy
parent
0e55d8a1df
commit
fcdb66bb6b
7 changed files with 235 additions and 72 deletions
|
|
@ -41,46 +41,6 @@ const middleware = require('~/server/middleware');
|
|||
|
||||
const requireAdminAccess = requireCapability(SystemCapabilities.ACCESS_ADMIN);
|
||||
|
||||
function buildGoogleAdminRefreshDeps(sessionExpiry) {
|
||||
return {
|
||||
findUsers,
|
||||
getUserById,
|
||||
canAccessAdmin: async (user) => {
|
||||
try {
|
||||
return await hasCapability(
|
||||
{
|
||||
id: user.id ?? user._id?.toString(),
|
||||
role: user.role ?? '',
|
||||
tenantId: user.tenantId,
|
||||
},
|
||||
SystemCapabilities.ACCESS_ADMIN,
|
||||
);
|
||||
} catch (err) {
|
||||
logger.warn(`[admin/oauth/refresh] capability check failed, denying: ${err?.message}`);
|
||||
return false;
|
||||
}
|
||||
},
|
||||
isEmailAllowed: async (user) => {
|
||||
if (!user?.email) return false;
|
||||
try {
|
||||
const appConfig = user.tenantId
|
||||
? await resolveAppConfigForUser(getAppConfig, user)
|
||||
: await getAppConfig({ baseOnly: true });
|
||||
return isEmailDomainAllowed(user.email, appConfig?.registration?.allowedDomains);
|
||||
} catch (err) {
|
||||
logger.warn(
|
||||
`[admin/oauth/refresh] domain allowlist check failed, denying: ${err?.message}`,
|
||||
);
|
||||
return false;
|
||||
}
|
||||
},
|
||||
mintToken: async (user) => ({
|
||||
token: await generateToken(user, sessionExpiry),
|
||||
expiresAt: Date.now() + sessionExpiry,
|
||||
}),
|
||||
};
|
||||
}
|
||||
|
||||
const setBalanceConfig = createSetBalanceConfig({
|
||||
getAppConfig,
|
||||
findBalanceByUser,
|
||||
|
|
@ -111,6 +71,52 @@ function resolveRequestOrigin(req) {
|
|||
}
|
||||
}
|
||||
|
||||
function buildAdminRefreshClosures(sessionExpiry) {
|
||||
return {
|
||||
canAccessAdmin: async (user) => {
|
||||
try {
|
||||
return await hasCapability(
|
||||
{
|
||||
id: user.id ?? user._id?.toString(),
|
||||
role: user.role ?? '',
|
||||
tenantId: user.tenantId,
|
||||
},
|
||||
SystemCapabilities.ACCESS_ADMIN,
|
||||
);
|
||||
} catch (err) {
|
||||
logger.warn(`[admin/oauth/refresh] capability check failed, denying: ${err?.message}`);
|
||||
return false;
|
||||
}
|
||||
},
|
||||
mintToken: async (user) => ({
|
||||
token: await generateToken(user, sessionExpiry),
|
||||
expiresAt: Date.now() + sessionExpiry,
|
||||
}),
|
||||
};
|
||||
}
|
||||
|
||||
function buildGoogleAdminRefreshDeps(sessionExpiry) {
|
||||
return {
|
||||
findUsers,
|
||||
getUserById,
|
||||
...buildAdminRefreshClosures(sessionExpiry),
|
||||
isEmailAllowed: async (user) => {
|
||||
if (!user?.email) return false;
|
||||
try {
|
||||
const appConfig = user.tenantId
|
||||
? await resolveAppConfigForUser(getAppConfig, user)
|
||||
: await getAppConfig({ baseOnly: true });
|
||||
return isEmailDomainAllowed(user.email, appConfig?.registration?.allowedDomains);
|
||||
} catch (err) {
|
||||
logger.warn(
|
||||
`[admin/oauth/refresh] domain allowlist check failed, denying: ${err?.message}`,
|
||||
);
|
||||
return false;
|
||||
}
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
router.post(
|
||||
'/login/local',
|
||||
middleware.logHeaders,
|
||||
|
|
@ -654,27 +660,7 @@ router.post(
|
|||
{
|
||||
findUsers,
|
||||
getUserById,
|
||||
canAccessAdmin: async (user) => {
|
||||
try {
|
||||
return await hasCapability(
|
||||
{
|
||||
id: user.id ?? user._id?.toString(),
|
||||
role: user.role ?? '',
|
||||
tenantId: user.tenantId,
|
||||
},
|
||||
SystemCapabilities.ACCESS_ADMIN,
|
||||
);
|
||||
} catch (err) {
|
||||
logger.warn(
|
||||
`[admin/oauth/refresh] capability check failed, denying: ${err?.message}`,
|
||||
);
|
||||
return false;
|
||||
}
|
||||
},
|
||||
mintToken: async (user) => ({
|
||||
token: await generateToken(user, sessionExpiry),
|
||||
expiresAt: Date.now() + sessionExpiry,
|
||||
}),
|
||||
...buildAdminRefreshClosures(sessionExpiry),
|
||||
},
|
||||
{
|
||||
userId: normalizedUserId,
|
||||
|
|
|
|||
|
|
@ -296,7 +296,15 @@ describe('admin auth Google refresh route', () => {
|
|||
expect(response.body).toEqual({
|
||||
token: 'admin-jwt',
|
||||
refreshToken: 'rotated-refresh',
|
||||
user: expect.objectContaining({ provider: 'google' }),
|
||||
user: expect.objectContaining({
|
||||
_id: 'user-id',
|
||||
id: 'user-id',
|
||||
email: 'admin@example.com',
|
||||
name: 'Admin',
|
||||
username: 'admin',
|
||||
role: 'ADMIN',
|
||||
provider: 'google',
|
||||
}),
|
||||
expiresAt: 1234567890,
|
||||
});
|
||||
expect(applyGoogleAdminRefresh).toHaveBeenCalledWith(
|
||||
|
|
@ -316,6 +324,45 @@ describe('admin auth Google refresh route', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('forwards the tenant id from getTenantId() to the helper', async () => {
|
||||
const { getTenantId } = require('@librechat/data-schemas');
|
||||
getTenantId.mockReturnValueOnce('tenant-x');
|
||||
|
||||
const response = await request(app)
|
||||
.post('/api/admin/oauth/refresh')
|
||||
.send({ refresh_token: 'incoming-google-refresh', provider: 'google' });
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(applyGoogleAdminRefresh).toHaveBeenCalledWith(
|
||||
expect.any(Object),
|
||||
expect.objectContaining({ tenantId: 'tenant-x' }),
|
||||
);
|
||||
});
|
||||
|
||||
it('canAccessAdmin closure calls hasCapability with the normalized user id', async () => {
|
||||
const { hasCapability } = require('~/server/middleware/roles/capabilities');
|
||||
let capturedDeps;
|
||||
applyGoogleAdminRefresh.mockImplementationOnce(async (deps) => {
|
||||
capturedDeps = deps;
|
||||
return {
|
||||
token: 'jwt',
|
||||
refreshToken: 'r',
|
||||
user: { id: 'u', _id: 'u', email: 'e@e.com', name: '', username: '', role: 'ADMIN' },
|
||||
expiresAt: 0,
|
||||
};
|
||||
});
|
||||
|
||||
await request(app)
|
||||
.post('/api/admin/oauth/refresh')
|
||||
.send({ refresh_token: 'google-refresh', provider: 'google' });
|
||||
|
||||
await capturedDeps.canAccessAdmin({ id: 'user-1', role: 'ADMIN', tenantId: 'tenant-a' });
|
||||
expect(hasCapability).toHaveBeenCalledWith(
|
||||
{ id: 'user-1', role: 'ADMIN', tenantId: 'tenant-a' },
|
||||
'ACCESS_ADMIN',
|
||||
);
|
||||
});
|
||||
|
||||
it('does not require OPENID_REUSE_TOKENS for the google provider', async () => {
|
||||
isEnabled.mockReturnValue(false);
|
||||
|
||||
|
|
|
|||
|
|
@ -56,7 +56,7 @@ const socialLogin =
|
|||
}
|
||||
|
||||
const passResult = (user) =>
|
||||
refreshToken ? cb(null, user, { refreshToken }) : cb(null, user);
|
||||
refreshToken && provider === 'google' ? cb(null, user, { refreshToken }) : cb(null, user);
|
||||
|
||||
if (existingUser?.provider === provider) {
|
||||
if (
|
||||
|
|
@ -74,6 +74,15 @@ const socialLogin =
|
|||
}
|
||||
if (options.existingUsersOnly && id && !existingUser[providerKey]) {
|
||||
await updateUser(existingUser._id, { [providerKey]: id });
|
||||
const verified = await findUser({ _id: existingUser._id, [providerKey]: id });
|
||||
if (!verified) {
|
||||
logger.warn(
|
||||
`[${provider}Login] Admin migrate superseded by concurrent write, denying: ${email}`,
|
||||
);
|
||||
const concurrentError = new Error(ErrorTypes.AUTH_FAILED);
|
||||
concurrentError.code = ErrorTypes.AUTH_FAILED;
|
||||
return cb(concurrentError);
|
||||
}
|
||||
existingUser[providerKey] = id;
|
||||
}
|
||||
await handleExistingUser(existingUser, avatarUrl, appConfig, email);
|
||||
|
|
|
|||
|
|
@ -177,6 +177,37 @@ describe('socialLogin', () => {
|
|||
expect(callback).toHaveBeenCalledWith(null, existingUser);
|
||||
});
|
||||
|
||||
it('does not migrate the provider id on the chat path (only admin path migrates)', async () => {
|
||||
const { updateUser } = require('~/models');
|
||||
const provider = 'google';
|
||||
const googleId = 'google-user-chat';
|
||||
const email = 'chat@example.com';
|
||||
|
||||
const existingUser = {
|
||||
_id: 'chatUser',
|
||||
email: email,
|
||||
provider: 'google',
|
||||
};
|
||||
|
||||
findUser.mockResolvedValueOnce(null).mockResolvedValueOnce(existingUser);
|
||||
|
||||
const mockProfile = {
|
||||
id: googleId,
|
||||
emails: [{ value: email, verified: true }],
|
||||
photos: [{ value: 'https://example.com/avatar.png' }],
|
||||
name: { givenName: 'Chat', familyName: 'User' },
|
||||
};
|
||||
|
||||
const loginFn = socialLogin(provider, mockGetProfileDetails);
|
||||
const callback = jest.fn();
|
||||
|
||||
await loginFn(null, null, null, mockProfile, callback);
|
||||
|
||||
expect(updateUser).not.toHaveBeenCalled();
|
||||
expect(handleExistingUser).toHaveBeenCalled();
|
||||
expect(callback).toHaveBeenCalledWith(null, existingUser);
|
||||
});
|
||||
|
||||
it('migrates the missing provider id when finding by email fallback (admin path)', async () => {
|
||||
const { updateUser } = require('~/models');
|
||||
const provider = 'google';
|
||||
|
|
@ -427,6 +458,36 @@ describe('socialLogin', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('does not forward the refresh token as authInfo for non-google providers', async () => {
|
||||
const provider = 'github';
|
||||
const githubId = 'gh-user-123';
|
||||
const email = 'user@example.com';
|
||||
|
||||
const existingUser = {
|
||||
_id: 'ghUser',
|
||||
email,
|
||||
provider: 'github',
|
||||
githubId,
|
||||
};
|
||||
|
||||
findUser.mockResolvedValue(existingUser);
|
||||
|
||||
const mockProfile = {
|
||||
id: githubId,
|
||||
emails: [{ value: email, verified: true }],
|
||||
photos: [{ value: 'https://example.com/avatar.png' }],
|
||||
name: { givenName: 'GitHub', familyName: 'User' },
|
||||
};
|
||||
|
||||
const loginFn = socialLogin(provider, mockGetProfileDetails);
|
||||
const callback = jest.fn();
|
||||
|
||||
await loginFn(null, 'github-refresh-token', null, mockProfile, callback);
|
||||
|
||||
expect(callback).toHaveBeenCalledWith(null, existingUser);
|
||||
expect(callback).not.toHaveBeenCalledWith(null, existingUser, expect.anything());
|
||||
});
|
||||
|
||||
it('passes the IdP refresh token through as authInfo when present', async () => {
|
||||
const provider = 'google';
|
||||
const googleId = 'google-with-refresh';
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue