Skip to content

fix: the ldap manager passes a username variable lab... in Manager.ts #40400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
orbisai0security wants to merge 1 commit into
base: develop
Choose a base branch
from
+1 −1
Open

fix: the ldap manager passes a username variable lab... in Manager.ts #40400

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/meteor/server/lib/ldap/Manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export class LDAPManager {
}

public static async testSearch(username: string): Promise<void> {
const escapedUsername = ldapEscape.filter`${username}`;
const escapedUsername = ldapEscape.filter`${username}`.replace(/[/+<>;=]/g, (c) => `\\${c.charCodeAt(0).toString(16).padStart(2, '0')}`);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Fix only patches testSearch; the actual login paths (findUser, findAuthenticatedUser) remain unmodified.

The additional .replace() escaping is applied exclusively at line 118 (testSearch), which is a diagnostic utility method. The two production-critical paths — findUser (line 220) and findAuthenticatedUser (line 254) — still use only ldapEscape.filter${username}`` with no additional escaping. If the intention is to harden against LDAP injection via the username field, both of those methods must receive the same treatment; otherwise the fix provides zero protection on any actual authentication flow.

🔧 Apply the same escaping to all three call sites 
 private static async findUser(ldap: LDAPConnection, username: string, password: string): Promise<ILDAPEntry | undefined> {
-    const escapedUsername = ldapEscape.filter`${username}`;
+    const escapedUsername = ldapEscape.filter`${username}`.replace(/[/+<>;=]/g, (c) => `\\${c.charCodeAt(0).toString(16).padStart(2, '0')}`);
 private static async findAuthenticatedUser(ldap: LDAPConnection, username: string): Promise<ILDAPEntry | undefined> {
-    const escapedUsername = ldapEscape.filter`${username}`;
+    const escapedUsername = ldapEscape.filter`${username}`.replace(/[/+<>;=]/g, (c) => `\\${c.charCodeAt(0).toString(16).padStart(2, '0')}`);

Alternatively, extract to a shared helper to avoid triplicated logic:

+  private static escapeUsername(username: string): string {
+    return ldapEscape.filter`${username}`.replace(/[/+<>;=]/g, (c) => `\\${c.charCodeAt(0).toString(16).padStart(2, '0')}`);
+  }
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/meteor/server/lib/ldap/Manager.ts` at line 118, The testSearch call
applies extra .replace() escaping but the production authentication paths do
not; update findUser and findAuthenticatedUser to use the same escapedUsername
transformation as testSearch (or consolidate into a shared helper like
escapeLdapUsername used by testSearch, findUser, and findAuthenticatedUser) so
all three call sites perform ldapEscape.filter`${username}` followed by
.replace(/[/+<>;=]/g, c => `\\${c.charCodeAt(0).toString(16).padStart(2,'0')}`)
before building LDAP queries.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

What characters does the npm package ldap-escape version 2.0.6 escape when using the filter tagged template function?

💡 Result:

The npm package ldap-escape version 2.0.6, when using the filter tagged template function (ldapEscape.filter), escapes the following characters for LDAP search filters: * (to \2A), ( (to \28), ) (to \29), \ (to \5C), and NUL (null byte, to \00) [1][2][3][4]. This is explicitly stated in the package's README and npm registry documentation, which apply to version 2.0.6 (published April 13, 2022) [2]. These follow Active Directory LDAP filter escaping rules per RFC 4515 standards [5].

Citations:

🏁 Script executed:

# Find the Manager.ts file and examine the context around line 118
find . -name "Manager.ts" -path "*/ldap/*" | head -5

Repository: RocketChat/Rocket.Chat

Length of output: 151

🏁 Script executed:

# Also search for Connection.ts to understand getUserFilter usage
find . -name "Connection.ts" -path "*/ldap/*" | head -5

Repository: RocketChat/Rocket.Chat

Length of output: 110

🏁 Script executed:

# Read Manager.ts around line 118 to see context
sed -n '100,140n' ./apps/meteor/server/lib/ldap/Manager.ts | cat -n

Repository: RocketChat/Rocket.Chat

Length of output: 48

🏁 Script executed:

# Read Connection.ts to find getUserFilter and how it uses the username parameter
rg -n "getUserFilter" ./apps/meteor/server/lib/ldap/Connection.ts -A 10 -B 2

Repository: RocketChat/Rocket.Chat

Length of output: 1524

🏁 Script executed:

# Get the full Manager.ts file and see line 118 and surrounding context
wc -l ./apps/meteor/server/lib/ldap/Manager.ts

Repository: RocketChat/Rocket.Chat

Length of output: 111

🏁 Script executed:

# Read Manager.ts to understand the escaping at line 118
head -150 ./apps/meteor/server/lib/ldap/Manager.ts | tail -60 | cat -n

Repository: RocketChat/Rocket.Chat

Length of output: 2056

🏁 Script executed:

# Search for findUser and findAuthenticatedUser methods in Manager.ts
rg -n "findUser|findAuthenticatedUser" ./apps/meteor/server/lib/ldap/Manager.ts -A 8 -B 2

Repository: RocketChat/Rocket.Chat

Length of output: 1488

Remove the extra .replace() call or apply consistent escaping across all methods.

The additional escaping in testSearch targets RFC 4514 DN special characters (/, +, <, >, ;, =), not RFC 4515 filter special characters. ldapEscape.filter already handles all RFC 4515 mandatory escapes (*, (, ), \, NUL); the extra .replace() provides no security benefit in the filter assertion value context where the username is embedded as (userField=${username}).

More critically, this creates an inconsistency: testSearch applies double-encoding while findUser and findAuthenticatedUser use only the standard ldapEscape.filter. A username containing any of these characters (common in some AD environments) will fail in testSearch with "User not found" but succeed in the unpatched methods, causing confusing behavior.

Either remove the extra .replace() to align with findUser and findAuthenticatedUser, or apply the same escaping consistently across all three methods.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/meteor/server/lib/ldap/Manager.ts` at line 118, testSearch is
double-escaping the username by chaining .replace() onto ldapEscape.filter which
targets RFC4515 filters while the .replace() targets RFC4514 DN chars, creating
inconsistent behavior with findUser and findAuthenticatedUser; remove the extra
.replace() so escapedUsername is produced solely via
ldapEscape.filter`${username}` (and verify testSearch, findUser, and
findAuthenticatedUser all use the same ldapEscape.filter usage for consistency).

Copy link
Copy Markdown

@anupamme anupamme May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orbisai0security can you address code review comments?

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

const ldap = new LDAPConnection();

try {
Expand Down