From 21f6255eafe1efe8222d5817c3dddefe6693ef31 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 14 Jul 2026 12:13:16 -0600 Subject: [PATCH 1/4] fix: server crash when LDAP search filter is invalid ldapjs parses the search filter synchronously and throws before the callback runs (e.g. 'invalid attribute name' when the User Search Field is empty, producing a filter like '(&(=*))'). The throw escaped doAsyncSearch/doPagedSearch as an unhandled rejection and terminated the process during LDAP sync. Route synchronous client.search throws to the existing callback error path via a small clientSearch wrapper used by all three call sites. --- .changeset/ldap-sync-crash-invalid-filter.md | 5 ++ .../meteor/server/lib/ldap/Connection.spec.ts | 56 +++++++++++++++++++ apps/meteor/server/lib/ldap/Connection.ts | 16 +++++- 3 files changed, 74 insertions(+), 3 deletions(-) create mode 100644 .changeset/ldap-sync-crash-invalid-filter.md create mode 100644 apps/meteor/server/lib/ldap/Connection.spec.ts diff --git a/.changeset/ldap-sync-crash-invalid-filter.md b/.changeset/ldap-sync-crash-invalid-filter.md new file mode 100644 index 0000000000000..cecdbba85e822 --- /dev/null +++ b/.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. diff --git a/apps/meteor/server/lib/ldap/Connection.spec.ts b/apps/meteor/server/lib/ldap/Connection.spec.ts new file mode 100644 index 0000000000000..ab0b31fde94de --- /dev/null +++ b/apps/meteor/server/lib/ldap/Connection.spec.ts @@ -0,0 +1,56 @@ +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.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; + }); + }); +}); diff --git a/apps/meteor/server/lib/ldap/Connection.ts b/apps/meteor/server/lib/ldap/Connection.ts index 900633159e704..229f0cd5871fc 100644 --- a/apps/meteor/server/lib/ldap/Connection.ts +++ b/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); @@ -517,6 +517,16 @@ export class LDAPConnection { }); } + // ldapjs parses the filter synchronously and throws on invalid filters (e.g. an empty search field); + // route those to the callback so they don't escape as unhandled rejections and crash the process. + 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( baseDN: string, searchOptions: ldapjs.SearchOptions, @@ -527,7 +537,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 +601,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); From ce93170512280792ef6510d88a89f51617311526 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 14 Jul 2026 12:24:12 -0600 Subject: [PATCH 2/4] fix: propagate searchAllUsers promise rejections in LDAP import importNewUsers discarded the searchAllUsers promise with void, so any rejection not routed through endCallback became an unhandled rejection. Chain it to the surrounding promise's reject instead. --- apps/meteor/ee/server/lib/ldap/Manager.ts | 42 ++++++++++++----------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/apps/meteor/ee/server/lib/ldap/Manager.ts b/apps/meteor/ee/server/lib/ldap/Manager.ts index c78eba154c57b..2dba4d7e41adf 100644 --- a/apps/meteor/ee/server/lib/ldap/Manager.ts +++ b/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({ - 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({ + 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); }); } From 4ce0b569fae99113157fb4221956f6809068de81 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 14 Jul 2026 12:30:09 -0600 Subject: [PATCH 3/4] fix: reject empty LDAP user search field with a clear error getUserFilter's empty-field guard was dead code: ''.split(',') returns [''], so the length === 0 branch was unreachable and an empty field composed the invalid filter '(=*)'. Trim segments, drop empty ones (also covers trailing commas), and throw a clear configuration error instead of handing a malformed filter to ldapjs. --- .../meteor/server/lib/ldap/Connection.spec.ts | 53 +++++++++++++++++++ apps/meteor/server/lib/ldap/Connection.ts | 15 ++++-- 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/apps/meteor/server/lib/ldap/Connection.spec.ts b/apps/meteor/server/lib/ldap/Connection.spec.ts index ab0b31fde94de..a3ef1c9f60021 100644 --- a/apps/meteor/server/lib/ldap/Connection.spec.ts +++ b/apps/meteor/server/lib/ldap/Connection.spec.ts @@ -25,6 +25,7 @@ describe('LDAPConnection', () => { beforeEach(() => { connection = new LDAPConnection(); + connection.options.userSearchField = 'uid'; connection.client = { search: sinon.stub().throws(parseError) }; }); @@ -53,4 +54,56 @@ describe('LDAPConnection', () => { 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; + }); + }); }); diff --git a/apps/meteor/server/lib/ldap/Connection.ts b/apps/meteor/server/lib/ldap/Connection.ts index 229f0cd5871fc..5ea13e64d90c2 100644 --- a/apps/meteor/server/lib/ldap/Connection.ts +++ b/apps/meteor/server/lib/ldap/Connection.ts @@ -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('')})`); From 9b0873cd09e9c4c24cec39d8f0ef4f8641e81147 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 14 Jul 2026 12:38:40 -0600 Subject: [PATCH 4/4] chore: remove redundant comment on clientSearch --- apps/meteor/server/lib/ldap/Connection.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/apps/meteor/server/lib/ldap/Connection.ts b/apps/meteor/server/lib/ldap/Connection.ts index 5ea13e64d90c2..1c3aba67ef5cd 100644 --- a/apps/meteor/server/lib/ldap/Connection.ts +++ b/apps/meteor/server/lib/ldap/Connection.ts @@ -524,8 +524,6 @@ export class LDAPConnection { }); } - // ldapjs parses the filter synchronously and throws on invalid filters (e.g. an empty search field); - // route those to the callback so they don't escape as unhandled rejections and crash the process. private clientSearch(baseDN: string, searchOptions: ldapjs.SearchOptions, callback: ldapjs.SearchCallBack): void { try { this.client.search(baseDN, searchOptions, callback);