fix: server crash when LDAP search filter is invalid

.changeset/ldap-sync-crash-invalid-filter.md

@@ -0,0 +1,5 @@
+---
+'@rocket.chat/meteor': patch
+---
+
+Fixes the server crashing during LDAP login or sync when the configured search settings produce an invalid LDAP filter (for example, an empty User Search Field). The operation now fails gracefully with a logged error instead of terminating the process.

apps/meteor/ee/server/lib/ldap/Manager.ts

@@ -702,26 +702,28 @@ export class LDAPEEManager extends LDAPManager {
 		return new Promise((resolve, reject) => {
 			let count = 0;
 
-			void ldap.searchAllUsers<IImportUser>({
-				entryCallback: (entry: ldapjs.SearchEntry): IImportUser | undefined => {
-					const data = ldap.extractLdapEntryData(entry);
-					count++;
-
-					const userData = this.mapUserData(data);
-					converter.addObjectToMemory(userData, { dn: data.dn, username: this.getLdapUsername(data) });
-					return userData;
-				},
-				endCallback: (err: any): void => {
-					if (err) {
-						logger.error({ err });
-						reject(err);
-						return;
-					}
-
-					logger.info({ msg: 'LDAP finished loading users. Users added to importer', count });
-					resolve();
-				},
-			});
+			ldap
+				.searchAllUsers<IImportUser>({
+					entryCallback: (entry: ldapjs.SearchEntry): IImportUser | undefined => {
+						const data = ldap.extractLdapEntryData(entry);
+						count++;
+
+						const userData = this.mapUserData(data);
+						converter.addObjectToMemory(userData, { dn: data.dn, username: this.getLdapUsername(data) });
+						return userData;
+					},
+					endCallback: (err: any): void => {
+						if (err) {
+							logger.error({ err });
+							reject(err);
+							return;
+						}
+
+						logger.info({ msg: 'LDAP finished loading users. Users added to importer', count });
+						resolve();
+					},
+				})
+				.catch(reject);
 		});
 	}
 

apps/meteor/server/lib/ldap/Connection.spec.ts

@@ -0,0 +1,109 @@
+import { expect } from 'chai';
+import { beforeEach, describe, it } from 'mocha';
+import proxyquire from 'proxyquire';
+import sinon from 'sinon';
+
+const loggerStub = { debug: sinon.stub(), error: sinon.stub(), info: sinon.stub(), warn: sinon.stub() };
+
+const { LDAPConnection } = proxyquire.noCallThru().load('./Connection', {
+	'../../../app/settings/server': { settings: { get: sinon.stub() } },
+	'./getLDAPConditionalSetting': { getLDAPConditionalSetting: sinon.stub().returns('') },
+	'./Logger': {
+		logger: loggerStub,
+		connLogger: loggerStub,
+		bindLogger: loggerStub,
+		searchLogger: loggerStub,
+		authLogger: loggerStub,
+		mapLogger: loggerStub,
+	},
+});
+
+describe('LDAPConnection', () => {
+	describe('synchronous errors from client.search (e.g. invalid filters)', () => {
+		const parseError = new Error('invalid attribute name');
+		let connection: any;
+
+		beforeEach(() => {
+			connection = new LDAPConnection();
+			connection.options.userSearchField = 'uid';
+			connection.client = { search: sinon.stub().throws(parseError) };
+		});
+
+		it('should reject doCustomSearch with the error', async () => {
+			const error = await connection
+				.doCustomSearch('dc=test', { filter: '(&(=*))' }, () => undefined)
+				.then(
+					() => undefined,
+					(e: unknown) => e,
+				);
+			expect(error).to.equal(parseError);
+		});
+
+		it('should route the error to endCallback on paged searchAllUsers instead of leaking the throw', async () => {
+			const endCallback = sinon.stub();
+			await connection.searchAllUsers({ endCallback });
+			// eslint-disable-next-line @typescript-eslint/no-unused-expressions
+			expect(endCallback.calledOnceWithExactly(parseError)).to.be.true;
+		});
+
+		it('should route the error to endCallback on non-paged searchAllUsers instead of leaking the throw', async () => {
+			connection.options.searchPageSize = 0;
+			const endCallback = sinon.stub();
+			await connection.searchAllUsers({ endCallback });
+			// eslint-disable-next-line @typescript-eslint/no-unused-expressions
+			expect(endCallback.calledOnceWithExactly(parseError)).to.be.true;
+		});
+	});
+
+	describe('getUserFilter', () => {
+		let connection: any;
+
+		beforeEach(() => {
+			connection = new LDAPConnection();
+		});
+
+		it('should compose the filter for a single search field', () => {
+			connection.options.userSearchField = 'uid';
+			expect(connection.getUserFilter('john')).to.equal('(&(uid=john))');
+		});
+
+		it('should compose an OR filter for multiple search fields', () => {
+			connection.options.userSearchField = 'uid,sAMAccountName';
+			expect(connection.getUserFilter('john')).to.equal('(&(|(uid=john)(sAMAccountName=john)))');
+		});
+
+		it('should trim whitespace and ignore empty segments (e.g. trailing commas)', () => {
+			connection.options.userSearchField = ' uid , sAMAccountName ,';
+			expect(connection.getUserFilter('john')).to.equal('(&(|(uid=john)(sAMAccountName=john)))');
+		});
+
+		it('should include the user search filter when configured', () => {
+			connection.options.userSearchField = 'uid';
+			connection.options.userSearchFilter = '(objectclass=user)';
+			expect(connection.getUserFilter('*')).to.equal('(&(objectclass=user)(uid=*))');
+		});
+
+		it('should throw a configuration error when the search field is empty', () => {
+			connection.options.userSearchField = '';
+			expect(() => connection.getUserFilter('*')).to.throw('LDAP User Search Field is not configured');
+		});
+
+		it('should throw a configuration error when the search field only has empty segments', () => {
+			connection.options.userSearchField = ' , ,';
+			expect(() => connection.getUserFilter('*')).to.throw('LDAP User Search Field is not configured');
+		});
+
+		it('should reject searchAllUsers with the configuration error instead of composing an invalid filter', async () => {
+			connection.options.userSearchField = '';
+			connection.client = { search: sinon.stub() };
+			const endCallback = sinon.stub();
+			const error = await connection.searchAllUsers({ endCallback }).then(
+				() => undefined,
+				(e: unknown) => e,
+			);
+			expect(error).to.be.an('error').with.property('message', 'LDAP User Search Field is not configured');
+			// eslint-disable-next-line @typescript-eslint/no-unused-expressions
+			expect(connection.client.search.called).to.be.false;
+		});
+	});
+});

apps/meteor/server/lib/ldap/Connection.ts

@@ -349,7 +349,7 @@ export class LDAPConnection {
 		let realEntries = 0;
 
 		return new Promise((resolve, reject) => {
-			this.client.search(baseDN, searchOptions, (err, res: ldapjs.SearchCallbackResponse) => {
+			this.clientSearch(baseDN, searchOptions, (err, res: ldapjs.SearchCallbackResponse) => {
 				if (err) {
 					searchLogger.error({ err });
 					reject(err);
@@ -395,11 +395,18 @@ export class LDAPConnection {
 
 		this.addUserFilters(filter, username);
 
-		const usernameFilter = this.options.userSearchField.split(',').map((item) => `(${item}=${username})`);
+		const fields = this.options.userSearchField
+			.split(',')
+			.map((field) => field.trim())
+			.filter(Boolean);
 
-		if (usernameFilter.length === 0) {
-			logger.error('LDAP_LDAP_User_Search_Field not defined');
-		} else if (usernameFilter.length === 1) {
+		if (!fields.length) {
+			throw new Error('LDAP User Search Field is not configured');
+		}
+
+		const usernameFilter = fields.map((field) => `(${field}=${username})`);
+
+		if (usernameFilter.length === 1) {
 			filter.push(`${usernameFilter[0]}`);
 		} else {
 			filter.push(`(|${usernameFilter.join('')})`);
@@ -517,6 +524,14 @@ export class LDAPConnection {
 		});
 	}
 
+	private clientSearch(baseDN: string, searchOptions: ldapjs.SearchOptions, callback: ldapjs.SearchCallBack): void {
+		try {
+			this.client.search(baseDN, searchOptions, callback);
+		} catch (err) {
+			callback(err as ldapjs.Error, undefined as unknown as ldapjs.SearchCallbackResponse);
+		}
+	}
+
 	private async doAsyncSearch<T = ldapjs.SearchEntry>(
 		baseDN: string,
 		searchOptions: ldapjs.SearchOptions,
@@ -527,7 +542,7 @@ export class LDAPConnection {
 
 		searchLogger.debug({ msg: 'searchOptions', searchOptions, baseDN });
 
-		this.client.search(baseDN, searchOptions, (err: ldapjs.Error | null, res: ldapjs.SearchCallbackResponse): void => {
+		this.clientSearch(baseDN, searchOptions, (err: ldapjs.Error | null, res: ldapjs.SearchCallbackResponse): void => {
 			if (err) {
 				searchLogger.error({ err });
 				callback(err);
@@ -591,7 +606,7 @@ export class LDAPConnection {
 
 		searchLogger.debug({ msg: 'searchOptions', searchOptions, baseDN });
 
-		this.client.search(baseDN, searchOptions, (err: ldapjs.Error | null, res: ldapjs.SearchCallbackResponse): void => {
+		this.clientSearch(baseDN, searchOptions, (err: ldapjs.Error | null, res: ldapjs.SearchCallbackResponse): void => {
 			if (err) {
 				searchLogger.error({ err });
 				callback(err);