Skip to content

Commit 71d4d82

Browse files
authored
fix: server crash when LDAP search filter is invalid (RocketChat#41373)
1 parent 3598e2d commit 71d4d82

4 files changed

Lines changed: 158 additions & 27 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@rocket.chat/meteor': patch
3+
---
4+
5+
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

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -702,26 +702,28 @@ export class LDAPEEManager extends LDAPManager {
702702
return new Promise((resolve, reject) => {
703703
let count = 0;
704704

705-
void ldap.searchAllUsers<IImportUser>({
706-
entryCallback: (entry: ldapjs.SearchEntry): IImportUser | undefined => {
707-
const data = ldap.extractLdapEntryData(entry);
708-
count++;
709-
710-
const userData = this.mapUserData(data);
711-
converter.addObjectToMemory(userData, { dn: data.dn, username: this.getLdapUsername(data) });
712-
return userData;
713-
},
714-
endCallback: (err: any): void => {
715-
if (err) {
716-
logger.error({ err });
717-
reject(err);
718-
return;
719-
}
720-
721-
logger.info({ msg: 'LDAP finished loading users. Users added to importer', count });
722-
resolve();
723-
},
724-
});
705+
ldap
706+
.searchAllUsers<IImportUser>({
707+
entryCallback: (entry: ldapjs.SearchEntry): IImportUser | undefined => {
708+
const data = ldap.extractLdapEntryData(entry);
709+
count++;
710+
711+
const userData = this.mapUserData(data);
712+
converter.addObjectToMemory(userData, { dn: data.dn, username: this.getLdapUsername(data) });
713+
return userData;
714+
},
715+
endCallback: (err: any): void => {
716+
if (err) {
717+
logger.error({ err });
718+
reject(err);
719+
return;
720+
}
721+
722+
logger.info({ msg: 'LDAP finished loading users. Users added to importer', count });
723+
resolve();
724+
},
725+
})
726+
.catch(reject);
725727
});
726728
}
727729

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
import { expect } from 'chai';
2+
import { beforeEach, describe, it } from 'mocha';
3+
import proxyquire from 'proxyquire';
4+
import sinon from 'sinon';
5+
6+
const loggerStub = { debug: sinon.stub(), error: sinon.stub(), info: sinon.stub(), warn: sinon.stub() };
7+
8+
const { LDAPConnection } = proxyquire.noCallThru().load('./Connection', {
9+
'../../../app/settings/server': { settings: { get: sinon.stub() } },
10+
'./getLDAPConditionalSetting': { getLDAPConditionalSetting: sinon.stub().returns('') },
11+
'./Logger': {
12+
logger: loggerStub,
13+
connLogger: loggerStub,
14+
bindLogger: loggerStub,
15+
searchLogger: loggerStub,
16+
authLogger: loggerStub,
17+
mapLogger: loggerStub,
18+
},
19+
});
20+
21+
describe('LDAPConnection', () => {
22+
describe('synchronous errors from client.search (e.g. invalid filters)', () => {
23+
const parseError = new Error('invalid attribute name');
24+
let connection: any;
25+
26+
beforeEach(() => {
27+
connection = new LDAPConnection();
28+
connection.options.userSearchField = 'uid';
29+
connection.client = { search: sinon.stub().throws(parseError) };
30+
});
31+
32+
it('should reject doCustomSearch with the error', async () => {
33+
const error = await connection
34+
.doCustomSearch('dc=test', { filter: '(&(=*))' }, () => undefined)
35+
.then(
36+
() => undefined,
37+
(e: unknown) => e,
38+
);
39+
expect(error).to.equal(parseError);
40+
});
41+
42+
it('should route the error to endCallback on paged searchAllUsers instead of leaking the throw', async () => {
43+
const endCallback = sinon.stub();
44+
await connection.searchAllUsers({ endCallback });
45+
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
46+
expect(endCallback.calledOnceWithExactly(parseError)).to.be.true;
47+
});
48+
49+
it('should route the error to endCallback on non-paged searchAllUsers instead of leaking the throw', async () => {
50+
connection.options.searchPageSize = 0;
51+
const endCallback = sinon.stub();
52+
await connection.searchAllUsers({ endCallback });
53+
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
54+
expect(endCallback.calledOnceWithExactly(parseError)).to.be.true;
55+
});
56+
});
57+
58+
describe('getUserFilter', () => {
59+
let connection: any;
60+
61+
beforeEach(() => {
62+
connection = new LDAPConnection();
63+
});
64+
65+
it('should compose the filter for a single search field', () => {
66+
connection.options.userSearchField = 'uid';
67+
expect(connection.getUserFilter('john')).to.equal('(&(uid=john))');
68+
});
69+
70+
it('should compose an OR filter for multiple search fields', () => {
71+
connection.options.userSearchField = 'uid,sAMAccountName';
72+
expect(connection.getUserFilter('john')).to.equal('(&(|(uid=john)(sAMAccountName=john)))');
73+
});
74+
75+
it('should trim whitespace and ignore empty segments (e.g. trailing commas)', () => {
76+
connection.options.userSearchField = ' uid , sAMAccountName ,';
77+
expect(connection.getUserFilter('john')).to.equal('(&(|(uid=john)(sAMAccountName=john)))');
78+
});
79+
80+
it('should include the user search filter when configured', () => {
81+
connection.options.userSearchField = 'uid';
82+
connection.options.userSearchFilter = '(objectclass=user)';
83+
expect(connection.getUserFilter('*')).to.equal('(&(objectclass=user)(uid=*))');
84+
});
85+
86+
it('should throw a configuration error when the search field is empty', () => {
87+
connection.options.userSearchField = '';
88+
expect(() => connection.getUserFilter('*')).to.throw('LDAP User Search Field is not configured');
89+
});
90+
91+
it('should throw a configuration error when the search field only has empty segments', () => {
92+
connection.options.userSearchField = ' , ,';
93+
expect(() => connection.getUserFilter('*')).to.throw('LDAP User Search Field is not configured');
94+
});
95+
96+
it('should reject searchAllUsers with the configuration error instead of composing an invalid filter', async () => {
97+
connection.options.userSearchField = '';
98+
connection.client = { search: sinon.stub() };
99+
const endCallback = sinon.stub();
100+
const error = await connection.searchAllUsers({ endCallback }).then(
101+
() => undefined,
102+
(e: unknown) => e,
103+
);
104+
expect(error).to.be.an('error').with.property('message', 'LDAP User Search Field is not configured');
105+
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
106+
expect(connection.client.search.called).to.be.false;
107+
});
108+
});
109+
});

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

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -349,7 +349,7 @@ export class LDAPConnection {
349349
let realEntries = 0;
350350

351351
return new Promise((resolve, reject) => {
352-
this.client.search(baseDN, searchOptions, (err, res: ldapjs.SearchCallbackResponse) => {
352+
this.clientSearch(baseDN, searchOptions, (err, res: ldapjs.SearchCallbackResponse) => {
353353
if (err) {
354354
searchLogger.error({ err });
355355
reject(err);
@@ -395,11 +395,18 @@ export class LDAPConnection {
395395

396396
this.addUserFilters(filter, username);
397397

398-
const usernameFilter = this.options.userSearchField.split(',').map((item) => `(${item}=${username})`);
398+
const fields = this.options.userSearchField
399+
.split(',')
400+
.map((field) => field.trim())
401+
.filter(Boolean);
399402

400-
if (usernameFilter.length === 0) {
401-
logger.error('LDAP_LDAP_User_Search_Field not defined');
402-
} else if (usernameFilter.length === 1) {
403+
if (!fields.length) {
404+
throw new Error('LDAP User Search Field is not configured');
405+
}
406+
407+
const usernameFilter = fields.map((field) => `(${field}=${username})`);
408+
409+
if (usernameFilter.length === 1) {
403410
filter.push(`${usernameFilter[0]}`);
404411
} else {
405412
filter.push(`(|${usernameFilter.join('')})`);
@@ -517,6 +524,14 @@ export class LDAPConnection {
517524
});
518525
}
519526

527+
private clientSearch(baseDN: string, searchOptions: ldapjs.SearchOptions, callback: ldapjs.SearchCallBack): void {
528+
try {
529+
this.client.search(baseDN, searchOptions, callback);
530+
} catch (err) {
531+
callback(err as ldapjs.Error, undefined as unknown as ldapjs.SearchCallbackResponse);
532+
}
533+
}
534+
520535
private async doAsyncSearch<T = ldapjs.SearchEntry>(
521536
baseDN: string,
522537
searchOptions: ldapjs.SearchOptions,
@@ -527,7 +542,7 @@ export class LDAPConnection {
527542

528543
searchLogger.debug({ msg: 'searchOptions', searchOptions, baseDN });
529544

530-
this.client.search(baseDN, searchOptions, (err: ldapjs.Error | null, res: ldapjs.SearchCallbackResponse): void => {
545+
this.clientSearch(baseDN, searchOptions, (err: ldapjs.Error | null, res: ldapjs.SearchCallbackResponse): void => {
531546
if (err) {
532547
searchLogger.error({ err });
533548
callback(err);
@@ -591,7 +606,7 @@ export class LDAPConnection {
591606

592607
searchLogger.debug({ msg: 'searchOptions', searchOptions, baseDN });
593608

594-
this.client.search(baseDN, searchOptions, (err: ldapjs.Error | null, res: ldapjs.SearchCallbackResponse): void => {
609+
this.clientSearch(baseDN, searchOptions, (err: ldapjs.Error | null, res: ldapjs.SearchCallbackResponse): void => {
595610
if (err) {
596611
searchLogger.error({ err });
597612
callback(err);

0 commit comments

Comments
 (0)