fix: server crash when LDAP search filter is invalid

[KevLehman]

Proposed changes (including videos or screenshots)

ldapjs parses the search filter synchronously inside client.search and throws before any callback runs — e.g. Error: invalid attribute name when the User Search Field is empty, since getUserFilter then composes (&(objectclass=user)(=*)). That throw escaped doAsyncSearch/doPagedSearch as an UnhandledPromiseRejection and terminated the whole server process (exit code 1) during LDAP sync or login.

Three layered fixes:

1. Route synchronous client.search throws to the callback error path (Connection.ts): a small clientSearch wrapper (try/catch, mirroring the pattern already used in bindDN) used by all three call sites (doCustomSearch, doAsyncSearch, doPagedSearch). Covers any malformed filter, not just the empty-field case.
2. Reject an empty User Search Field with a clear configuration error (getUserFilter): the existing guard was dead code — ''.split(',') returns [''], so the length === 0 branch was unreachable and an empty field composed the invalid filter (=*). Segments are now trimmed and empty ones dropped (also covers trailing commas like sAMAccountName,), and a fully empty field throws LDAP User Search Field is not configured before ldapjs is ever called.
3. Stop discarding the searchAllUsers promise in the sync job (ee/.../Manager.ts importNewUsers): it was void-ed, so any rejection not routed through endCallback became an unhandled rejection; it now chains to the surrounding promise's reject and lands in sync()'s existing try/catch.

Unit tests (Connection.spec.ts, new) pin the behavior: sync-throw routing (verified to fail with the leaked throw when the try/catch is removed) and getUserFilter composition/trimming/validation.

Issue(s)

https://rocketchat.atlassian.net/browse/CORE-2407

Steps to test or reproduce

1. Admin → Users → LDAP: Server Type = Active Directory, set Host/Port/Base DN and authentication.
2. Leave User Search Field (LDAP_AD_User_Search_Field) empty.
3. Enable LDAP and trigger Sync Now (POST /api/v1/ldap.syncNow).

Before: server logs UnhandledPromiseRejection: Error: invalid attribute name and exits. After: sync fails with a logged LDAP User Search Field is not configured error and the server keeps running.

Non-AD equivalent: any non-AD server type with LDAP_User_Search_Field empty or containing a trailing comma.

Further comments

An e2e test isn't feasible here: CI has no LDAP server, so ldap.syncNow/ldap.testSearch fail at connect/bind before ever reaching the filter parse. The unit tests stub the ldapjs client at the exact throw point instead.

All getUserFilter callers (searchByUsername login/testSearch paths, searchAllUsers sync path) already wrap in try/catch or now propagate to one, so the new throw fails only the operation, never the process.

Summary by CodeRabbit

• Bug Fixes
  • Fixed LDAP login and user synchronization crashes caused by invalid or empty user search field settings.
  • LDAP operations now fail gracefully with a logged error instead of terminating the server.
  • Improved handling of LDAP search errors across paged and non-paged searches.
  • LDAP search field configuration is now validated more reliably, including comma-separated fields and optional filters.

[dionisio-bot]

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

[changeset-bot]

🦋 Changeset detected

Latest commit: 9b0873c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@rocket.chat/meteor	Patch
@rocket.chat/core-typings	Patch
@rocket.chat/rest-typings	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9402ff56-3592-4041-9328-8cdd6d82e0f8

📥 Commits

Reviewing files that changed from the base of the PR and between b634242 and 9b0873c.

📒 Files selected for processing (4)

• .changeset/ldap-sync-crash-invalid-filter.md
• apps/meteor/ee/server/lib/ldap/Manager.ts
• apps/meteor/server/lib/ldap/Connection.spec.ts
• apps/meteor/server/lib/ldap/Connection.ts

📜 Recent review details

⏰ Context from checks skipped due to timeout. (3)

• GitHub Check: cubic · AI code reviewer
• GitHub Check: Hacktron Security Check
• GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (1/4)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation

Files:

• apps/meteor/server/lib/ldap/Connection.spec.ts
• apps/meteor/ee/server/lib/ldap/Manager.ts
• apps/meteor/server/lib/ldap/Connection.ts

**/*.spec.ts

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use .spec.ts extension for test files (e.g., login.spec.ts)

Files:

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

🧠 Learnings (6)

📚 Learning: 2026-03-16T21:50:37.589Z

Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.

Applied to files:

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

📚 Learning: 2026-02-24T19:22:48.358Z

Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.

Applied to files:

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

📚 Learning: 2026-02-26T19:25:44.063Z

Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.

Applied to files:

• apps/meteor/server/lib/ldap/Connection.spec.ts
• apps/meteor/ee/server/lib/ldap/Manager.ts
• apps/meteor/server/lib/ldap/Connection.ts

📚 Learning: 2026-02-26T19:25:44.063Z

Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.

Applied to files:

• apps/meteor/server/lib/ldap/Connection.spec.ts
• apps/meteor/ee/server/lib/ldap/Manager.ts
• apps/meteor/server/lib/ldap/Connection.ts

📚 Learning: 2026-03-06T18:10:15.268Z

Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.

Applied to files:

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

📚 Learning: 2026-05-06T12:21:44.083Z

Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.

Applied to files:

• apps/meteor/server/lib/ldap/Connection.spec.ts
• apps/meteor/ee/server/lib/ldap/Manager.ts
• apps/meteor/server/lib/ldap/Connection.ts

🔇 Additional comments (4)

apps/meteor/server/lib/ldap/Connection.ts (1)

352-352: LGTM!

Also applies to: 398-409, 527-534, 545-545, 609-609

apps/meteor/server/lib/ldap/Connection.spec.ts (1)

1-110: LGTM!

apps/meteor/ee/server/lib/ldap/Manager.ts (1)

705-726: LGTM!

.changeset/ldap-sync-crash-invalid-filter.md (1)

1-6: LGTM!

---

Walkthrough

LDAP search calls now handle synchronous client errors, invalid user-search-field configuration is rejected explicitly, and user import propagates search failures instead of leaving them unhandled. Unit tests cover search errors, filter composition, validation, and import-facing rejection behavior.

Changes

LDAP error handling

Layer / File(s)	Summary
LDAP search safety and filter validation apps/meteor/server/lib/ldap/Connection.ts, apps/meteor/server/lib/ldap/Connection.spec.ts	LDAP searches route through a synchronous-error wrapper, while user search fields are trimmed, empty entries are removed, and invalid configurations throw explicit errors. Tests cover paged and non-paged searches, filter composition, and rejected invalid configuration.
Import failure propagation and release metadata apps/meteor/ee/server/lib/ldap/Manager.ts, .changeset/ldap-sync-crash-invalid-filter.md	importNewUsers propagates searchAllUsers promise rejections, and the changeset records the patch release and graceful LDAP failure behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: type: bug, area: authentication

Suggested reviewers: sampaiodiego, pierre-lehnen-rc

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main fix: preventing server crashes caused by invalid LDAP search filters.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)

• CORE-2407: Request failed with status code 401

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 80.51948% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.24%. Comparing base (b634242) to head (9b0873c).
⚠️ Report is 7 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #41373      +/-   ##
===========================================
- Coverage    68.27%   68.24%   -0.03%     
===========================================
  Files         3968     3969       +1     
  Lines       154740   154824      +84     
  Branches     27847    27798      -49     
===========================================
+ Hits        105652   105665      +13     
- Misses       44294    44416     +122     
+ Partials      4794     4743      -51     

Flag	Coverage Δ
e2e	59.22% <ø> (-0.16%)	⬇️
e2e-api	44.06% <54.54%> (-0.31%)	⬇️
unit	70.42% <80.51%> (-0.25%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cubic-dev-ai]

No issues found across 4 files

_{Re-trigger cubic}