From bd158905b311cf5731a18676eafdb0d9b0219e2a Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 22 Jun 2026 10:42:20 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20fix:=20Harden=20admin=20OAuth=20?= =?UTF-8?q?refresh=20against=20user=20bans,=20tenant=20scope=20gaps,=20and?= =?UTF-8?q?=20cross-tenant=20migration?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- api/server/routes/admin/auth.js | 15 ++++++-- api/server/routes/admin/auth.refresh.test.js | 22 ++++++++++++ api/strategies/socialLogin.js | 8 +++++ api/strategies/socialLogin.test.js | 36 ++++++++++++++++++++ 4 files changed, 78 insertions(+), 3 deletions(-) diff --git a/api/server/routes/admin/auth.js b/api/server/routes/admin/auth.js index b8a69f8a67..e78ac95113 100644 --- a/api/server/routes/admin/auth.js +++ b/api/server/routes/admin/auth.js @@ -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) { diff --git a/api/server/routes/admin/auth.refresh.test.js b/api/server/routes/admin/auth.refresh.test.js index 72eaede651..b669eef963 100644 --- a/api/server/routes/admin/auth.refresh.test.js +++ b/api/server/routes/admin/auth.refresh.test.js @@ -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' }); + }); }); diff --git a/api/strategies/socialLogin.js b/api/strategies/socialLogin.js index 459e69e47d..da751d0c1c 100644 --- a/api/strategies/socialLogin.js +++ b/api/strategies/socialLogin.js @@ -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) { diff --git a/api/strategies/socialLogin.test.js b/api/strategies/socialLogin.test.js index 0ea950cc80..01b8c6f210 100644 --- a/api/strategies/socialLogin.test.js +++ b/api/strategies/socialLogin.test.js @@ -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';