Andreas/LibreChat
1
0
Fork 0
mirror of https://github.com/danny-avila/LibreChat.git synced 2025-12-16 16:30:15 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions 6

🔒 fix: Provider Validation for Social, OpenID, SAML, and LDAP Logins (#8999)

Browse source 
* 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
This commit is contained in:
Danny Avila 2025-08-11 18:49:34 -04:00
parent 04d74a7e07
commit 1ccac58403
No known key found for this signature in database
GPG key ID: BF31EEB2C5CA0956
18 changed files with 314 additions and 125 deletions
Download patch file Download diff file Expand all files Collapse all files

46
api/server/controllers/ErrorController.js
View file

@ -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.');
}
};

241
api/server/controllers/ErrorController.spec.js
View file

@ -1,241 +0,0 @@
const errorController = require('./ErrorController');
const { logger } = require('~/config');
// Mock the logger
jest.mock('~/config', () => ({
logger: {
error: jest.fn(),
},
}));
describe('ErrorController', () => {
let mockReq, mockRes, mockNext;
beforeEach(() => {
mockReq = {};
mockRes = {
status: jest.fn().mockReturnThis(),
send: jest.fn(),
};
mockNext = jest.fn();
logger.error.mockClear();
});
describe('ValidationError handling', () => {
it('should handle ValidationError with single error', () => {
const validationError = {
name: 'ValidationError',
errors: {
email: { message: 'Email is required', path: 'email' },
},
};
errorController(validationError, mockReq, mockRes, mockNext);
expect(mockRes.status).toHaveBeenCalledWith(400);
expect(mockRes.send).toHaveBeenCalledWith({
messages: '["Email is required"]',
fields: '["email"]',
});
expect(logger.error).toHaveBeenCalledWith('Validation error:', validationError.errors);
});
it('should handle ValidationError with multiple errors', () => {
const validationError = {
name: 'ValidationError',
errors: {
email: { message: 'Email is required', path: 'email' },
password: { message: 'Password is required', path: 'password' },
},
};
errorController(validationError, mockReq, mockRes, mockNext);
expect(mockRes.status).toHaveBeenCalledWith(400);
expect(mockRes.send).toHaveBeenCalledWith({
messages: '"Email is required Password is required"',
fields: '["email","password"]',
});
expect(logger.error).toHaveBeenCalledWith('Validation error:', validationError.errors);
});
it('should handle ValidationError with empty errors object', () => {
const validationError = {
name: 'ValidationError',
errors: {},
};
errorController(validationError, mockReq, mockRes, mockNext);
expect(mockRes.status).toHaveBeenCalledWith(400);
expect(mockRes.send).toHaveBeenCalledWith({
messages: '[]',
fields: '[]',
});
});
});
describe('Duplicate key error handling', () => {
it('should handle duplicate key error (code 11000)', () => {
const duplicateKeyError = {
code: 11000,
keyValue: { email: 'test@example.com' },
};
errorController(duplicateKeyError, mockReq, mockRes, mockNext);
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);
});
it('should handle duplicate key error with multiple fields', () => {
const duplicateKeyError = {
code: 11000,
keyValue: { email: 'test@example.com', username: 'testuser' },
};
errorController(duplicateKeyError, mockReq, mockRes, mockNext);
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);
});
it('should handle error with code 11000 as string', () => {
const duplicateKeyError = {
code: '11000',
keyValue: { email: 'test@example.com' },
};
errorController(duplicateKeyError, mockReq, mockRes, mockNext);
expect(mockRes.status).toHaveBeenCalledWith(409);
expect(mockRes.send).toHaveBeenCalledWith({
messages: 'An document with that ["email"] already exists.',
fields: '["email"]',
});
});
});
describe('SyntaxError handling', () => {
it('should handle errors with statusCode and body', () => {
const syntaxError = {
statusCode: 400,
body: 'Invalid JSON syntax',
};
errorController(syntaxError, mockReq, mockRes, mockNext);
expect(mockRes.status).toHaveBeenCalledWith(400);
expect(mockRes.send).toHaveBeenCalledWith('Invalid JSON syntax');
});
it('should handle errors with different statusCode and body', () => {
const customError = {
statusCode: 422,
body: { error: 'Unprocessable entity' },
};
errorController(customError, mockReq, mockRes, mockNext);
expect(mockRes.status).toHaveBeenCalledWith(422);
expect(mockRes.send).toHaveBeenCalledWith({ error: 'Unprocessable entity' });
});
it('should handle error with statusCode but no body', () => {
const partialError = {
statusCode: 400,
};
errorController(partialError, mockReq, mockRes, mockNext);
expect(mockRes.status).toHaveBeenCalledWith(500);
expect(mockRes.send).toHaveBeenCalledWith('An unknown error occurred.');
});
it('should handle error with body but no statusCode', () => {
const partialError = {
body: 'Some error message',
};
errorController(partialError, mockReq, mockRes, mockNext);
expect(mockRes.status).toHaveBeenCalledWith(500);
expect(mockRes.send).toHaveBeenCalledWith('An unknown error occurred.');
});
});
describe('Unknown error handling', () => {
it('should handle unknown errors', () => {
const unknownError = new Error('Some unknown error');
errorController(unknownError, mockReq, mockRes, mockNext);
expect(mockRes.status).toHaveBeenCalledWith(500);
expect(mockRes.send).toHaveBeenCalledWith('An unknown error occurred.');
expect(logger.error).toHaveBeenCalledWith('ErrorController => error', unknownError);
});
it('should handle errors with code other than 11000', () => {
const mongoError = {
code: 11100,
message: 'Some MongoDB error',
};
errorController(mongoError, mockReq, mockRes, mockNext);
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);
expect(mockRes.status).toHaveBeenCalledWith(500);
expect(mockRes.send).toHaveBeenCalledWith('Processing error in ErrorController.');
expect(logger.error).toHaveBeenCalledWith(
'ErrorController => processing error',
expect.any(Error),
);
});
});
describe('Catch block handling', () => {
beforeEach(() => {
// Restore logger mock to normal behavior for these tests
logger.error.mockRestore();
logger.error = jest.fn();
});
it('should handle errors when logger.error throws', () => {
// Create fresh mocks for this test
const freshMockRes = {
status: jest.fn().mockReturnThis(),
send: jest.fn(),
};
// Mock logger to throw on the first call, succeed on the second
logger.error
.mockImplementationOnce(() => {
throw new Error('Logger error');
})
.mockImplementation(() => {});
const testError = new Error('Test error');
errorController(testError, mockReq, freshMockRes, mockNext);
expect(freshMockRes.status).toHaveBeenCalledWith(500);
expect(freshMockRes.send).toHaveBeenCalledWith('Processing error in ErrorController.');
expect(logger.error).toHaveBeenCalledTimes(2);
});
});
});

7
api/server/index.js
View file

@ -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({

2
api/server/index.spec.js
View file

@ -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
}

13
api/server/routes/oauth.js
View file

@ -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}`);
});
/**

12
api/strategies/ldapStrategy.js
View file

@ -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 =

10
api/strategies/openidStrategy.js
View file

@ -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)),

45
api/strategies/openidStrategy.spec.js
View file

@ -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);

10
api/strategies/samlStrategy.js
View file

@ -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(

24
api/strategies/samlStrategy.spec.js
View file

@ -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 };

19
api/strategies/socialLogin.js
View file

@ -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) {