mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-27 04:07:05 +00:00
🔒 fix: Harden admin OAuth refresh against user bans, tenant scope gaps, and cross-tenant migration
Post-identity-resolution ban check: the initial checkBan middleware fires before the
refresh token is exchanged and req.user is populated, so it can only evaluate IP bans.
After applyGoogleAdminRefresh/applyAdminRefresh resolves the user identity, we now
synthesize req.user and re-run checkBan against the resolved user's id before emitting
the JWT, so a user-level ban is enforced even from a fresh IP.
Domain allowlist now includes userId: the getAppConfig call in isEmailAllowedForUser
was passing only role, missing user and group-level allowedDomains overrides that the
initial OAuth callback's checkDomainAllowed enforces via userId. Both branches now
pass userId so buildPrincipals takes the full user+group+role resolution path. The
tenant branch is also inlined (replacing resolveAppConfigForUser) to accept userId,
wrapped in tenantStorage.run for correct Mongoose scoping and cache-key resolution.
Cross-tenant email-fallback migration: the Passport verify callback fires before
tenantContextMiddleware, so findUser({email}) is unscoped and can return a same-email
user from another tenant. Writing googleId onto that document permanently corrupts
the other tenant's account. Migration is now blocked for users with a tenantId;
single-tenant users are unaffected.
This commit is contained in:
parent
0f14dcce62
commit
bd158905b3
4 changed files with 78 additions and 3 deletions
|
|
@ -8,6 +8,7 @@ const {
|
|||
DEFAULT_SESSION_EXPIRY,
|
||||
SystemCapabilities,
|
||||
getTenantId,
|
||||
tenantStorage,
|
||||
} = require('@librechat/data-schemas');
|
||||
const {
|
||||
isEnabled,
|
||||
|
|
@ -22,7 +23,6 @@ const {
|
|||
AdminRefreshError,
|
||||
buildOpenIDRefreshParams,
|
||||
isEmailDomainAllowed,
|
||||
resolveAppConfigForUser,
|
||||
} = require('@librechat/api');
|
||||
const { loginController } = require('~/server/controllers/auth/LoginController');
|
||||
const { hasCapability, requireCapability } = require('~/server/middleware/roles/capabilities');
|
||||
|
|
@ -74,9 +74,12 @@ function resolveRequestOrigin(req) {
|
|||
async function isEmailAllowedForUser(user) {
|
||||
if (!user?.email) return false;
|
||||
try {
|
||||
const userId = user.id ?? user._id?.toString();
|
||||
const appConfig = user.tenantId
|
||||
? await resolveAppConfigForUser(getAppConfig, user)
|
||||
: await getAppConfig({ role: user.role ?? '' });
|
||||
? await tenantStorage.run({ tenantId: user.tenantId }, () =>
|
||||
getAppConfig({ role: user.role ?? '', userId, tenantId: user.tenantId }),
|
||||
)
|
||||
: await getAppConfig({ role: user.role ?? '', userId });
|
||||
return isEmailDomainAllowed(user.email, appConfig?.registration?.allowedDomains);
|
||||
} catch (err) {
|
||||
logger.warn(`[admin/oauth/refresh] domain allowlist check failed, denying: ${err?.message}`);
|
||||
|
|
@ -602,6 +605,9 @@ router.post(
|
|||
clientId: process.env.GOOGLE_CLIENT_ID,
|
||||
clientSecret: process.env.GOOGLE_CLIENT_SECRET,
|
||||
});
|
||||
req.user = { id: result.user._id };
|
||||
await middleware.checkBan(req, res, () => {});
|
||||
if (req.banned || res.headersSent) return;
|
||||
return res.json(result);
|
||||
} catch (err) {
|
||||
if (err instanceof AdminRefreshError) {
|
||||
|
|
@ -670,6 +676,9 @@ router.post(
|
|||
tenantId,
|
||||
},
|
||||
);
|
||||
req.user = { id: result.user._id };
|
||||
await middleware.checkBan(req, res, () => {});
|
||||
if (req.banned || res.headersSent) return;
|
||||
return res.json(result);
|
||||
} catch (err) {
|
||||
if (err instanceof AdminRefreshError) {
|
||||
|
|
|
|||
|
|
@ -22,6 +22,7 @@ jest.mock('@librechat/data-schemas', () => ({
|
|||
DEFAULT_SESSION_EXPIRY: 60000,
|
||||
SystemCapabilities: { ACCESS_ADMIN: 'ACCESS_ADMIN' },
|
||||
getTenantId: jest.fn(() => undefined),
|
||||
tenantStorage: { run: jest.fn((ctx, fn) => fn()) },
|
||||
}));
|
||||
|
||||
jest.mock('@librechat/api', () => {
|
||||
|
|
@ -411,4 +412,25 @@ describe('admin auth Google refresh route', () => {
|
|||
expect(applyGoogleAdminRefresh).not.toHaveBeenCalled();
|
||||
expect(applyAdminRefresh).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('re-runs checkBan with the resolved user identity and blocks a banned user', async () => {
|
||||
const middleware = require('~/server/middleware');
|
||||
let banCheckCalls = 0;
|
||||
middleware.checkBan.mockImplementation((req, res, next) => {
|
||||
banCheckCalls++;
|
||||
if (banCheckCalls >= 2 && req.user) {
|
||||
req.banned = true;
|
||||
return res.status(403).json({ message: 'banned' });
|
||||
}
|
||||
return next();
|
||||
});
|
||||
|
||||
const response = await request(app)
|
||||
.post('/api/admin/oauth/refresh')
|
||||
.send({ refresh_token: 'incoming-google-refresh', provider: 'google' });
|
||||
|
||||
expect(response.status).toBe(403);
|
||||
expect(middleware.checkBan).toHaveBeenCalledTimes(2);
|
||||
expect(middleware.checkBan.mock.calls[1][0].user).toEqual({ id: 'user-id' });
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -73,6 +73,14 @@ const socialLogin =
|
|||
return cb(error);
|
||||
}
|
||||
if (options.existingUsersOnly && id && !existingUser[providerKey]) {
|
||||
if (existingUser.tenantId) {
|
||||
logger.warn(
|
||||
`[${provider}Login] Admin migrate blocked for tenanted user ${email}: no tenant scope in OAuth callback`,
|
||||
);
|
||||
const tenantError = new Error(ErrorTypes.AUTH_FAILED);
|
||||
tenantError.code = ErrorTypes.AUTH_FAILED;
|
||||
return cb(tenantError);
|
||||
}
|
||||
await updateUser(existingUser._id, { [providerKey]: id });
|
||||
const verified = await findUser({ _id: existingUser._id, [providerKey]: id });
|
||||
if (!verified) {
|
||||
|
|
|
|||
|
|
@ -240,6 +240,42 @@ describe('socialLogin', () => {
|
|||
expect(callback).toHaveBeenCalledWith(null, existingUser);
|
||||
});
|
||||
|
||||
it('blocks migration via email fallback for a tenanted user (no tenant scope in OAuth callback)', async () => {
|
||||
const { updateUser } = require('~/models');
|
||||
const provider = 'google';
|
||||
const googleId = 'google-user-cross';
|
||||
const email = 'admin@tenantb.example.com';
|
||||
|
||||
const tenantedUser = {
|
||||
_id: 'tenant-b-user',
|
||||
email: email,
|
||||
provider: 'google',
|
||||
tenantId: 'tenant-b',
|
||||
};
|
||||
|
||||
findUser.mockResolvedValueOnce(null).mockResolvedValueOnce(tenantedUser);
|
||||
|
||||
const mockProfile = {
|
||||
id: googleId,
|
||||
emails: [{ value: email, verified: true }],
|
||||
photos: [{ value: 'https://example.com/avatar.png' }],
|
||||
name: { givenName: 'Admin', familyName: 'User' },
|
||||
};
|
||||
|
||||
const loginFn = socialLogin(provider, mockGetProfileDetails, { existingUsersOnly: true });
|
||||
const callback = jest.fn();
|
||||
|
||||
await loginFn(null, null, null, mockProfile, callback);
|
||||
|
||||
expect(updateUser).not.toHaveBeenCalled();
|
||||
expect(callback).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ code: ErrorTypes.AUTH_FAILED }),
|
||||
);
|
||||
expect(logger.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Admin migrate blocked for tenanted user'),
|
||||
);
|
||||
});
|
||||
|
||||
it('rejects the admin email fallback when stored provider id differs from the current sub', async () => {
|
||||
const provider = 'google';
|
||||
const googleId = 'google-user-new';
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue