From 1ccac58403ed35ce91ca417136288171457277ff Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 11 Aug 2025 18:49:34 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20fix:=20Provider=20Validation=20f?= =?UTF-8?q?or=20Social,=20OpenID,=20SAML,=20and=20LDAP=20Logins=20(#8999)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: social login provider crossover * feat: Enhance OpenID login handling and add tests for provider validation * refactor: authentication error handling to use ErrorTypes.AUTH_FAILED enum * refactor: update authentication error handling in LDAP and SAML strategies to use ErrorTypes.AUTH_FAILED enum * ci: Add validation for login with existing email and different provider in SAML strategy chore: Add logging for existing users with different providers in LDAP, SAML, and Social Login strategies --- api/server/controllers/ErrorController.js | 46 ------- api/server/index.js | 7 +- api/server/index.spec.js | 2 +- api/server/routes/oauth.js | 13 +- api/strategies/ldapStrategy.js | 12 +- api/strategies/openidStrategy.js | 10 +- api/strategies/openidStrategy.spec.js | 45 +++++-- api/strategies/samlStrategy.js | 10 ++ api/strategies/samlStrategy.spec.js | 24 +++- api/strategies/socialLogin.js | 19 ++- client/src/components/Auth/Login.tsx | 19 ++- client/src/locales/en/translation.json | 1 + .../api/src/middleware/error.spec.ts | 115 +++++++++++------- packages/api/src/middleware/error.ts | 83 +++++++++++++ packages/api/src/middleware/index.ts | 1 + packages/api/src/types/error.ts | 27 ++++ packages/api/src/types/index.ts | 1 + packages/data-provider/src/config.ts | 4 + 18 files changed, 314 insertions(+), 125 deletions(-) delete mode 100644 api/server/controllers/ErrorController.js rename api/server/controllers/ErrorController.spec.js => packages/api/src/middleware/error.spec.ts (65%) create mode 100644 packages/api/src/middleware/error.ts create mode 100644 packages/api/src/types/error.ts diff --git a/api/server/controllers/ErrorController.js b/api/server/controllers/ErrorController.js deleted file mode 100644 index 6907e63d9c..0000000000 --- a/api/server/controllers/ErrorController.js +++ /dev/null @@ -1,46 +0,0 @@ -const { logger } = require('~/config'); - -//handle duplicates -const handleDuplicateKeyError = (err, res) => { - logger.error('Duplicate key error:', err.keyValue); - const field = `${JSON.stringify(Object.keys(err.keyValue))}`; - const code = 409; - res - .status(code) - .send({ messages: `An document with that ${field} already exists.`, fields: field }); -}; - -//handle validation errors -const handleValidationError = (err, res) => { - logger.error('Validation error:', err.errors); - let errors = Object.values(err.errors).map((el) => el.message); - let fields = `${JSON.stringify(Object.values(err.errors).map((el) => el.path))}`; - let code = 400; - if (errors.length > 1) { - errors = errors.join(' '); - res.status(code).send({ messages: `${JSON.stringify(errors)}`, fields: fields }); - } else { - res.status(code).send({ messages: `${JSON.stringify(errors)}`, fields: fields }); - } -}; - -module.exports = (err, _req, res, _next) => { - try { - if (err.name === 'ValidationError') { - return handleValidationError(err, res); - } - if (err.code && err.code == 11000) { - return handleDuplicateKeyError(err, res); - } - // Special handling for errors like SyntaxError - if (err.statusCode && err.body) { - return res.status(err.statusCode).send(err.body); - } - - logger.error('ErrorController => error', err); - return res.status(500).send('An unknown error occurred.'); - } catch (err) { - logger.error('ErrorController => processing error', err); - return res.status(500).send('Processing error in ErrorController.'); - } -}; diff --git a/api/server/index.js b/api/server/index.js index a792c70a67..1fe419d0f2 100644 --- a/api/server/index.js +++ b/api/server/index.js @@ -8,14 +8,12 @@ const express = require('express'); const passport = require('passport'); const compression = require('compression'); const cookieParser = require('cookie-parser'); -const { isEnabled } = require('@librechat/api'); const { logger } = require('@librechat/data-schemas'); const mongoSanitize = require('express-mongo-sanitize'); +const { isEnabled, ErrorController } = require('@librechat/api'); const { connectDb, indexSync } = require('~/db'); - const validateImageRequest = require('./middleware/validateImageRequest'); const { jwtLogin, ldapLogin, passportLogin } = require('~/strategies'); -const errorController = require('./controllers/ErrorController'); const initializeMCPs = require('./services/initializeMCPs'); const configureSocialLogins = require('./socialLogins'); const AppService = require('./services/AppService'); @@ -120,8 +118,7 @@ const startServer = async () => { app.use('/api/tags', routes.tags); app.use('/api/mcp', routes.mcp); - // Add the error controller one more time after all routes - app.use(errorController); + app.use(ErrorController); app.use((req, res) => { res.set({ diff --git a/api/server/index.spec.js b/api/server/index.spec.js index 43ad57108f..79ca1621ac 100644 --- a/api/server/index.spec.js +++ b/api/server/index.spec.js @@ -92,7 +92,7 @@ async function healthCheckPoll(app, retries = 0) { if (response.status === 200) { return; // App is healthy } - } catch (error) { + } catch { // Ignore connection errors during polling } diff --git a/api/server/routes/oauth.js b/api/server/routes/oauth.js index afc4a05b75..5f3c0c1de8 100644 --- a/api/server/routes/oauth.js +++ b/api/server/routes/oauth.js @@ -1,7 +1,10 @@ // file deepcode ignore NoRateLimitingForLogin: Rate limiting is handled by the `loginLimiter` middleware const express = require('express'); const passport = require('passport'); +const { isEnabled } = require('@librechat/api'); const { randomState } = require('openid-client'); +const { logger } = require('@librechat/data-schemas'); +const { ErrorTypes } = require('librechat-data-provider'); const { checkBan, logHeaders, @@ -10,8 +13,6 @@ const { checkDomainAllowed, } = require('~/server/middleware'); const { setAuthTokens, setOpenIDAuthTokens } = require('~/server/services/AuthService'); -const { isEnabled } = require('~/server/utils'); -const { logger } = require('~/config'); const router = express.Router(); @@ -46,13 +47,13 @@ const oauthHandler = async (req, res) => { }; router.get('/error', (req, res) => { - // A single error message is pushed by passport when authentication fails. + /** A single error message is pushed by passport when authentication fails. */ + const errorMessage = req.session?.messages?.pop() || 'Unknown error'; logger.error('Error in OAuth authentication:', { - message: req.session?.messages?.pop() || 'Unknown error', + message: errorMessage, }); - // Redirect to login page with auth_failed parameter to prevent infinite redirect loops - res.redirect(`${domains.client}/login?redirect=false`); + res.redirect(`${domains.client}/login?redirect=false&error=${ErrorTypes.AUTH_FAILED}`); }); /** diff --git a/api/strategies/ldapStrategy.js b/api/strategies/ldapStrategy.js index 434534c264..a9c00fa596 100644 --- a/api/strategies/ldapStrategy.js +++ b/api/strategies/ldapStrategy.js @@ -1,10 +1,10 @@ const fs = require('fs'); +const { isEnabled } = require('@librechat/api'); const LdapStrategy = require('passport-ldapauth'); -const { SystemRoles } = require('librechat-data-provider'); const { logger } = require('@librechat/data-schemas'); +const { SystemRoles, ErrorTypes } = require('librechat-data-provider'); const { createUser, findUser, updateUser, countUsers } = require('~/models'); const { getBalanceConfig } = require('~/server/services/Config'); -const { isEnabled } = require('~/server/utils'); const { LDAP_URL, @@ -90,6 +90,14 @@ const ldapLogin = new LdapStrategy(ldapOptions, async (userinfo, done) => { (LDAP_ID && userinfo[LDAP_ID]) || userinfo.uid || userinfo.sAMAccountName || userinfo.mail; let user = await findUser({ ldapId }); + if (user && user.provider !== 'ldap') { + logger.info( + `[ldapStrategy] User ${user.email} already exists with provider ${user.provider}`, + ); + return done(null, false, { + message: ErrorTypes.AUTH_FAILED, + }); + } const fullNameAttributes = LDAP_FULL_NAME && LDAP_FULL_NAME.split(','); const fullName = diff --git a/api/strategies/openidStrategy.js b/api/strategies/openidStrategy.js index f2151f3dfd..276ba725cf 100644 --- a/api/strategies/openidStrategy.js +++ b/api/strategies/openidStrategy.js @@ -3,9 +3,9 @@ const fetch = require('node-fetch'); const passport = require('passport'); const client = require('openid-client'); const jwtDecode = require('jsonwebtoken/decode'); -const { CacheKeys } = require('librechat-data-provider'); const { HttpsProxyAgent } = require('https-proxy-agent'); const { hashToken, logger } = require('@librechat/data-schemas'); +const { CacheKeys, ErrorTypes } = require('librechat-data-provider'); const { Strategy: OpenIDStrategy } = require('openid-client/passport'); const { isEnabled, safeStringify, logHeaders } = require('@librechat/api'); const { getStrategyFunctions } = require('~/server/services/Files/strategies'); @@ -320,6 +320,14 @@ async function setupOpenId() { } for openidId: ${claims.sub}`, ); } + if (user != null && user.provider !== 'openid') { + logger.info( + `[openidStrategy] Attempted OpenID login by user ${user.email}, was registered with "${user.provider}" provider`, + ); + return done(null, false, { + message: ErrorTypes.AUTH_FAILED, + }); + } const userinfo = { ...claims, ...(await getUserInfo(openidConfig, tokenset.access_token, claims.sub)), diff --git a/api/strategies/openidStrategy.spec.js b/api/strategies/openidStrategy.spec.js index b423b7994c..550ef18147 100644 --- a/api/strategies/openidStrategy.spec.js +++ b/api/strategies/openidStrategy.spec.js @@ -1,7 +1,8 @@ const fetch = require('node-fetch'); const jwtDecode = require('jsonwebtoken/decode'); -const { setupOpenId } = require('./openidStrategy'); +const { ErrorTypes } = require('librechat-data-provider'); const { findUser, createUser, updateUser } = require('~/models'); +const { setupOpenId } = require('./openidStrategy'); // --- Mocks --- jest.mock('node-fetch'); @@ -50,7 +51,7 @@ jest.mock('openid-client', () => { issuer: 'https://fake-issuer.com', // Add any other properties needed by the implementation }), - fetchUserInfo: jest.fn().mockImplementation((config, accessToken, sub) => { + fetchUserInfo: jest.fn().mockImplementation(() => { // Only return additional properties, but don't override any claims return Promise.resolve({}); }), @@ -261,17 +262,20 @@ describe('setupOpenId', () => { }); it('should update an existing user on login', async () => { - // Arrange – simulate that a user already exists + // Arrange – simulate that a user already exists with openid provider const existingUser = { _id: 'existingUserId', - provider: 'local', + provider: 'openid', email: tokenset.claims().email, openidId: '', username: '', name: '', }; findUser.mockImplementation(async (query) => { - if (query.openidId === tokenset.claims().sub || query.email === tokenset.claims().email) { + if ( + query.openidId === tokenset.claims().sub || + (query.email === tokenset.claims().email && query.provider === 'openid') + ) { return existingUser; } return null; @@ -294,12 +298,38 @@ describe('setupOpenId', () => { ); }); + it('should block login when email exists with different provider', async () => { + // Arrange – simulate that a user exists with same email but different provider + const existingUser = { + _id: 'existingUserId', + provider: 'google', + email: tokenset.claims().email, + googleId: 'some-google-id', + username: 'existinguser', + name: 'Existing User', + }; + findUser.mockImplementation(async (query) => { + if (query.email === tokenset.claims().email && !query.provider) { + return existingUser; + } + return null; + }); + + // Act + const result = await validate(tokenset); + + // Assert – verify that the strategy rejects login + expect(result.user).toBe(false); + expect(result.details.message).toBe(ErrorTypes.AUTH_FAILED); + expect(createUser).not.toHaveBeenCalled(); + expect(updateUser).not.toHaveBeenCalled(); + }); + it('should enforce the required role and reject login if missing', async () => { // Arrange – simulate a token without the required role. jwtDecode.mockReturnValue({ roles: ['SomeOtherRole'], }); - const userinfo = tokenset.claims(); // Act const { user, details } = await validate(tokenset); @@ -310,9 +340,6 @@ describe('setupOpenId', () => { }); it('should attempt to download and save the avatar if picture is provided', async () => { - // Arrange – ensure userinfo contains a picture URL - const userinfo = tokenset.claims(); - // Act const { user } = await validate(tokenset); diff --git a/api/strategies/samlStrategy.js b/api/strategies/samlStrategy.js index 376434f733..e00918ff43 100644 --- a/api/strategies/samlStrategy.js +++ b/api/strategies/samlStrategy.js @@ -2,6 +2,7 @@ const fs = require('fs'); const path = require('path'); const fetch = require('node-fetch'); const passport = require('passport'); +const { ErrorTypes } = require('librechat-data-provider'); const { hashToken, logger } = require('@librechat/data-schemas'); const { Strategy: SamlStrategy } = require('@node-saml/passport-saml'); const { getStrategyFunctions } = require('~/server/services/Files/strategies'); @@ -203,6 +204,15 @@ async function setupSaml() { ); } + if (user && user.provider !== 'saml') { + logger.info( + `[samlStrategy] User ${user.email} already exists with provider ${user.provider}`, + ); + return done(null, false, { + message: ErrorTypes.AUTH_FAILED, + }); + } + const fullName = getFullName(profile); const username = convertToUsername( diff --git a/api/strategies/samlStrategy.spec.js b/api/strategies/samlStrategy.spec.js index fc8329a31a..d6880655d0 100644 --- a/api/strategies/samlStrategy.spec.js +++ b/api/strategies/samlStrategy.spec.js @@ -378,11 +378,11 @@ u7wlOSk+oFzDIO/UILIA }); it('should update an existing user on login', async () => { - // Set up findUser to return an existing user + // Set up findUser to return an existing user with saml provider const { findUser } = require('~/models'); const existingUser = { _id: 'existing-user-id', - provider: 'local', + provider: 'saml', email: baseProfile.email, samlId: '', username: 'oldusername', @@ -400,6 +400,26 @@ u7wlOSk+oFzDIO/UILIA expect(user.email).toBe(baseProfile.email); }); + it('should block login when email exists with different provider', async () => { + // Set up findUser to return a user with different provider + const { findUser } = require('~/models'); + const existingUser = { + _id: 'existing-user-id', + provider: 'google', + email: baseProfile.email, + googleId: 'some-google-id', + username: 'existinguser', + name: 'Existing User', + }; + findUser.mockResolvedValue(existingUser); + + const profile = { ...baseProfile }; + const result = await validate(profile); + + expect(result.user).toBe(false); + expect(result.details.message).toBe(require('librechat-data-provider').ErrorTypes.AUTH_FAILED); + }); + it('should attempt to download and save the avatar if picture is provided', async () => { const profile = { ...baseProfile }; diff --git a/api/strategies/socialLogin.js b/api/strategies/socialLogin.js index 4f9462316a..79ba5b8e9e 100644 --- a/api/strategies/socialLogin.js +++ b/api/strategies/socialLogin.js @@ -1,6 +1,7 @@ +const { isEnabled } = require('@librechat/api'); const { logger } = require('@librechat/data-schemas'); +const { ErrorTypes } = require('librechat-data-provider'); const { createSocialUser, handleExistingUser } = require('./process'); -const { isEnabled } = require('~/server/utils'); const { findUser } = require('~/models'); const socialLogin = @@ -11,12 +12,20 @@ const socialLogin = profile, }); - const oldUser = await findUser({ email: email.trim() }); + const existingUser = await findUser({ email: email.trim() }); const ALLOW_SOCIAL_REGISTRATION = isEnabled(process.env.ALLOW_SOCIAL_REGISTRATION); - if (oldUser) { - await handleExistingUser(oldUser, avatarUrl); - return cb(null, oldUser); + if (existingUser?.provider === provider) { + await handleExistingUser(existingUser, avatarUrl); + return cb(null, existingUser); + } else if (existingUser) { + logger.info( + `[${provider}Login] User ${email} already exists with provider ${existingUser.provider}`, + ); + const error = new Error(ErrorTypes.AUTH_FAILED); + error.code = ErrorTypes.AUTH_FAILED; + error.provider = existingUser.provider; + return cb(error); } if (ALLOW_SOCIAL_REGISTRATION) { diff --git a/client/src/components/Auth/Login.tsx b/client/src/components/Auth/Login.tsx index 39406e300f..d9ebd41cc1 100644 --- a/client/src/components/Auth/Login.tsx +++ b/client/src/components/Auth/Login.tsx @@ -1,6 +1,7 @@ -import { useOutletContext, useSearchParams } from 'react-router-dom'; import { useEffect, useState } from 'react'; -import { OpenIDIcon } from '@librechat/client'; +import { ErrorTypes } from 'librechat-data-provider'; +import { OpenIDIcon, useToastContext } from '@librechat/client'; +import { useOutletContext, useSearchParams } from 'react-router-dom'; import type { TLoginLayoutContext } from '~/common'; import { ErrorMessage } from '~/components/Auth/ErrorMessage'; import SocialButton from '~/components/Auth/SocialButton'; @@ -11,6 +12,7 @@ import LoginForm from './LoginForm'; function Login() { const localize = useLocalize(); + const { showToast } = useToastContext(); const { error, setError, login } = useAuthContext(); const { startupConfig } = useOutletContext(); @@ -21,6 +23,19 @@ function Login() { // Persist the disable flag locally so that once detected, auto-redirect stays disabled. const [isAutoRedirectDisabled, setIsAutoRedirectDisabled] = useState(disableAutoRedirect); + useEffect(() => { + const oauthError = searchParams?.get('error'); + if (oauthError && oauthError === ErrorTypes.AUTH_FAILED) { + showToast({ + message: localize('com_auth_error_oauth_failed'), + status: 'error', + }); + const newParams = new URLSearchParams(searchParams); + newParams.delete('error'); + setSearchParams(newParams, { replace: true }); + } + }, [searchParams, setSearchParams, showToast, localize]); + // Once the disable flag is detected, update local state and remove the parameter from the URL. useEffect(() => { if (disableAutoRedirect) { diff --git a/client/src/locales/en/translation.json b/client/src/locales/en/translation.json index efcc58b82e..16bc764e53 100644 --- a/client/src/locales/en/translation.json +++ b/client/src/locales/en/translation.json @@ -107,6 +107,7 @@ "com_auth_error_login_rl": "Too many login attempts in a short amount of time. Please try again later.", "com_auth_error_login_server": "There was an internal server error. Please wait a few moments and try again.", "com_auth_error_login_unverified": "Your account has not been verified. Please check your email for a verification link.", + "com_auth_error_oauth_failed": "Authentication failed. Please check your login method and try again.", "com_auth_facebook_login": "Continue with Facebook", "com_auth_full_name": "Full name", "com_auth_github_login": "Continue with Github", diff --git a/api/server/controllers/ErrorController.spec.js b/packages/api/src/middleware/error.spec.ts similarity index 65% rename from api/server/controllers/ErrorController.spec.js rename to packages/api/src/middleware/error.spec.ts index c46315a5e5..d49e22b343 100644 --- a/api/server/controllers/ErrorController.spec.js +++ b/packages/api/src/middleware/error.spec.ts @@ -1,36 +1,43 @@ -const errorController = require('./ErrorController'); -const { logger } = require('~/config'); +import { logger } from '@librechat/data-schemas'; +import { ErrorController } from './error'; +import type { Request, Response } from 'express'; +import type { ValidationError, MongoServerError, CustomError } from '~/types'; // Mock the logger -jest.mock('~/config', () => ({ +jest.mock('@librechat/data-schemas', () => ({ + ...jest.requireActual('@librechat/data-schemas'), logger: { error: jest.fn(), + warn: jest.fn(), }, })); describe('ErrorController', () => { - let mockReq, mockRes, mockNext; + let mockReq: Request; + let mockRes: Response; beforeEach(() => { - mockReq = {}; + mockReq = { + originalUrl: '', + } as Request; mockRes = { status: jest.fn().mockReturnThis(), send: jest.fn(), - }; - mockNext = jest.fn(); - logger.error.mockClear(); + } as unknown as Response; + (logger.error as jest.Mock).mockClear(); }); describe('ValidationError handling', () => { it('should handle ValidationError with single error', () => { const validationError = { name: 'ValidationError', + message: 'Validation error', errors: { email: { message: 'Email is required', path: 'email' }, }, - }; + } as ValidationError; - errorController(validationError, mockReq, mockRes, mockNext); + ErrorController(validationError, mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(400); expect(mockRes.send).toHaveBeenCalledWith({ @@ -43,13 +50,14 @@ describe('ErrorController', () => { it('should handle ValidationError with multiple errors', () => { const validationError = { name: 'ValidationError', + message: 'Validation error', errors: { email: { message: 'Email is required', path: 'email' }, password: { message: 'Password is required', path: 'password' }, }, - }; + } as ValidationError; - errorController(validationError, mockReq, mockRes, mockNext); + ErrorController(validationError, mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(400); expect(mockRes.send).toHaveBeenCalledWith({ @@ -63,9 +71,9 @@ describe('ErrorController', () => { const validationError = { name: 'ValidationError', errors: {}, - }; + } as ValidationError; - errorController(validationError, mockReq, mockRes, mockNext); + ErrorController(validationError, mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(400); expect(mockRes.send).toHaveBeenCalledWith({ @@ -78,43 +86,59 @@ describe('ErrorController', () => { describe('Duplicate key error handling', () => { it('should handle duplicate key error (code 11000)', () => { const duplicateKeyError = { + name: 'MongoServerError', + message: 'Duplicate key error', code: 11000, keyValue: { email: 'test@example.com' }, - }; + errmsg: + 'E11000 duplicate key error collection: test.users index: email_1 dup key: { email: "test@example.com" }', + } as MongoServerError; - errorController(duplicateKeyError, mockReq, mockRes, mockNext); + ErrorController(duplicateKeyError, mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(409); expect(mockRes.send).toHaveBeenCalledWith({ messages: 'An document with that ["email"] already exists.', fields: '["email"]', }); - expect(logger.error).toHaveBeenCalledWith('Duplicate key error:', duplicateKeyError.keyValue); + expect(logger.warn).toHaveBeenCalledWith( + 'Duplicate key error: E11000 duplicate key error collection: test.users index: email_1 dup key: { email: "test@example.com" }', + ); }); it('should handle duplicate key error with multiple fields', () => { const duplicateKeyError = { + name: 'MongoServerError', + message: 'Duplicate key error', code: 11000, keyValue: { email: 'test@example.com', username: 'testuser' }, - }; + errmsg: + 'E11000 duplicate key error collection: test.users index: email_1 dup key: { email: "test@example.com" }', + } as MongoServerError; - errorController(duplicateKeyError, mockReq, mockRes, mockNext); + ErrorController(duplicateKeyError, mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(409); expect(mockRes.send).toHaveBeenCalledWith({ messages: 'An document with that ["email","username"] already exists.', fields: '["email","username"]', }); - expect(logger.error).toHaveBeenCalledWith('Duplicate key error:', duplicateKeyError.keyValue); + expect(logger.warn).toHaveBeenCalledWith( + 'Duplicate key error: E11000 duplicate key error collection: test.users index: email_1 dup key: { email: "test@example.com" }', + ); }); it('should handle error with code 11000 as string', () => { const duplicateKeyError = { - code: '11000', + name: 'MongoServerError', + message: 'Duplicate key error', + code: 11000, keyValue: { email: 'test@example.com' }, - }; + errmsg: + 'E11000 duplicate key error collection: test.users index: email_1 dup key: { email: "test@example.com" }', + } as MongoServerError; - errorController(duplicateKeyError, mockReq, mockRes, mockNext); + ErrorController(duplicateKeyError, mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(409); expect(mockRes.send).toHaveBeenCalledWith({ @@ -129,9 +153,9 @@ describe('ErrorController', () => { const syntaxError = { statusCode: 400, body: 'Invalid JSON syntax', - }; + } as CustomError; - errorController(syntaxError, mockReq, mockRes, mockNext); + ErrorController(syntaxError, mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(400); expect(mockRes.send).toHaveBeenCalledWith('Invalid JSON syntax'); @@ -141,9 +165,9 @@ describe('ErrorController', () => { const customError = { statusCode: 422, body: { error: 'Unprocessable entity' }, - }; + } as CustomError; - errorController(customError, mockReq, mockRes, mockNext); + ErrorController(customError, mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(422); expect(mockRes.send).toHaveBeenCalledWith({ error: 'Unprocessable entity' }); @@ -152,9 +176,9 @@ describe('ErrorController', () => { it('should handle error with statusCode but no body', () => { const partialError = { statusCode: 400, - }; + } as CustomError; - errorController(partialError, mockReq, mockRes, mockNext); + ErrorController(partialError, mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(500); expect(mockRes.send).toHaveBeenCalledWith('An unknown error occurred.'); @@ -163,9 +187,9 @@ describe('ErrorController', () => { it('should handle error with body but no statusCode', () => { const partialError = { body: 'Some error message', - }; + } as CustomError; - errorController(partialError, mockReq, mockRes, mockNext); + ErrorController(partialError, mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(500); expect(mockRes.send).toHaveBeenCalledWith('An unknown error occurred.'); @@ -176,7 +200,7 @@ describe('ErrorController', () => { it('should handle unknown errors', () => { const unknownError = new Error('Some unknown error'); - errorController(unknownError, mockReq, mockRes, mockNext); + ErrorController(unknownError, mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(500); expect(mockRes.send).toHaveBeenCalledWith('An unknown error occurred.'); @@ -187,32 +211,31 @@ describe('ErrorController', () => { const mongoError = { code: 11100, message: 'Some MongoDB error', - }; + } as MongoServerError; - errorController(mongoError, mockReq, mockRes, mockNext); + ErrorController(mongoError, mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(500); expect(mockRes.send).toHaveBeenCalledWith('An unknown error occurred.'); expect(logger.error).toHaveBeenCalledWith('ErrorController => error', mongoError); }); - it('should handle null/undefined errors', () => { - errorController(null, mockReq, mockRes, mockNext); + it('should handle generic errors', () => { + const genericError = new Error('Test error'); + + ErrorController(genericError, mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(500); - expect(mockRes.send).toHaveBeenCalledWith('Processing error in ErrorController.'); - expect(logger.error).toHaveBeenCalledWith( - 'ErrorController => processing error', - expect.any(Error), - ); + expect(mockRes.send).toHaveBeenCalledWith('An unknown error occurred.'); + expect(logger.error).toHaveBeenCalledWith('ErrorController => error', genericError); }); }); describe('Catch block handling', () => { beforeEach(() => { // Restore logger mock to normal behavior for these tests - logger.error.mockRestore(); - logger.error = jest.fn(); + (logger.error as jest.Mock).mockRestore(); + (logger.error as jest.Mock) = jest.fn(); }); it('should handle errors when logger.error throws', () => { @@ -220,10 +243,10 @@ describe('ErrorController', () => { const freshMockRes = { status: jest.fn().mockReturnThis(), send: jest.fn(), - }; + } as unknown as Response; // Mock logger to throw on the first call, succeed on the second - logger.error + (logger.error as jest.Mock) .mockImplementationOnce(() => { throw new Error('Logger error'); }) @@ -231,7 +254,7 @@ describe('ErrorController', () => { const testError = new Error('Test error'); - errorController(testError, mockReq, freshMockRes, mockNext); + ErrorController(testError, mockReq, freshMockRes); expect(freshMockRes.status).toHaveBeenCalledWith(500); expect(freshMockRes.send).toHaveBeenCalledWith('Processing error in ErrorController.'); diff --git a/packages/api/src/middleware/error.ts b/packages/api/src/middleware/error.ts new file mode 100644 index 0000000000..0d17ff5048 --- /dev/null +++ b/packages/api/src/middleware/error.ts @@ -0,0 +1,83 @@ +import { logger } from '@librechat/data-schemas'; +import { ErrorTypes } from 'librechat-data-provider'; +import type { NextFunction, Request, Response } from 'express'; +import type { MongoServerError, ValidationError, CustomError } from '~/types'; + +const handleDuplicateKeyError = (err: MongoServerError, res: Response) => { + logger.warn('Duplicate key error: ' + (err.errmsg || err.message)); + const field = err.keyValue ? `${JSON.stringify(Object.keys(err.keyValue))}` : 'unknown'; + const code = 409; + res + .status(code) + .send({ messages: `An document with that ${field} already exists.`, fields: field }); +}; + +const handleValidationError = (err: ValidationError, res: Response) => { + logger.error('Validation error:', err.errors); + const errorMessages = Object.values(err.errors).map((el) => el.message); + const fields = `${JSON.stringify(Object.values(err.errors).map((el) => el.path))}`; + const code = 400; + const messages = + errorMessages.length > 1 + ? `${JSON.stringify(errorMessages.join(' '))}` + : `${JSON.stringify(errorMessages)}`; + + res.status(code).send({ messages, fields }); +}; + +/** Type guard for ValidationError */ +function isValidationError(err: unknown): err is ValidationError { + return err !== null && typeof err === 'object' && 'name' in err && err.name === 'ValidationError'; +} + +/** Type guard for MongoServerError (duplicate key) */ +function isMongoServerError(err: unknown): err is MongoServerError { + return err !== null && typeof err === 'object' && 'code' in err && err.code === 11000; +} + +/** Type guard for CustomError with statusCode and body */ +function isCustomError(err: unknown): err is CustomError { + return err !== null && typeof err === 'object' && 'statusCode' in err && 'body' in err; +} + +export const ErrorController = ( + err: Error | CustomError, + req: Request, + res: Response, + next: NextFunction, +): Response | void => { + try { + if (!err) { + return next(); + } + const error = err as CustomError; + + if ( + (error.message === ErrorTypes.AUTH_FAILED || error.code === ErrorTypes.AUTH_FAILED) && + req.originalUrl && + req.originalUrl.includes('/oauth/') && + req.originalUrl.includes('/callback') + ) { + const domain = process.env.DOMAIN_CLIENT || 'http://localhost:3080'; + return res.redirect(`${domain}/login?redirect=false&error=${ErrorTypes.AUTH_FAILED}`); + } + + if (isValidationError(error)) { + return handleValidationError(error, res); + } + + if (isMongoServerError(error)) { + return handleDuplicateKeyError(error, res); + } + + if (isCustomError(error) && error.statusCode && error.body) { + return res.status(error.statusCode).send(error.body); + } + + logger.error('ErrorController => error', err); + return res.status(500).send('An unknown error occurred.'); + } catch (processingError) { + logger.error('ErrorController => processing error', processingError); + return res.status(500).send('Processing error in ErrorController.'); + } +}; diff --git a/packages/api/src/middleware/index.ts b/packages/api/src/middleware/index.ts index 176e8bc9ac..1d6d7c1f01 100644 --- a/packages/api/src/middleware/index.ts +++ b/packages/api/src/middleware/index.ts @@ -1 +1,2 @@ export * from './access'; +export * from './error'; diff --git a/packages/api/src/types/error.ts b/packages/api/src/types/error.ts new file mode 100644 index 0000000000..edb53f0c6c --- /dev/null +++ b/packages/api/src/types/error.ts @@ -0,0 +1,27 @@ +import type { Error as MongooseError } from 'mongoose'; + +/** MongoDB duplicate key error interface */ +export interface MongoServerError extends Error { + code: number; + keyValue?: Record; + errmsg?: string; +} + +/** Mongoose validation error interface */ +export interface ValidationError extends MongooseError { + name: 'ValidationError'; + errors: Record< + string, + { + message: string; + path?: string; + } + >; +} + +/** Custom error with status code and body */ +export interface CustomError extends Error { + statusCode?: number; + body?: unknown; + code?: string | number; +} diff --git a/packages/api/src/types/index.ts b/packages/api/src/types/index.ts index 75ee614ff1..ba5eada387 100644 --- a/packages/api/src/types/index.ts +++ b/packages/api/src/types/index.ts @@ -1,5 +1,6 @@ export * from './azure'; export * from './events'; +export * from './error'; export * from './google'; export * from './mistral'; export * from './openai'; diff --git a/packages/data-provider/src/config.ts b/packages/data-provider/src/config.ts index e6e57277d7..590e88a8ba 100644 --- a/packages/data-provider/src/config.ts +++ b/packages/data-provider/src/config.ts @@ -1359,6 +1359,10 @@ export enum ErrorTypes { * Endpoint models not loaded */ ENDPOINT_MODELS_NOT_LOADED = 'endpoint_models_not_loaded', + /** + * Generic Authentication failure + */ + AUTH_FAILED = 'auth_failed', } /**