fix: LDAP injection via unsanitized user input in DN and group filter construction (GHSA-7m6r-fhh7-r47c)#10154
Merged
mtrezza merged 2 commits intoparse-community:alphafrom Mar 9, 2026
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. |
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughAdds LDAP input sanitization to the authentication flow: introduces RFC-compliant escapeDN and escapeFilter helpers, validates authData.id type, applies escaping when building DNs and group filters, and adds tests covering escaping and injection scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthAdapter as Auth Adapter
participant Escaper as escapeDN / escapeFilter
participant LDAP as LDAP Server
Client->>AuthAdapter: submit authData (id, password)
AuthAdapter->>AuthAdapter: validate authData.id is string
AuthAdapter->>Escaper: escapeDN(authData.id) / escapeFilter(authData.id)
Escaper-->>AuthAdapter: escapedId
AuthAdapter->>LDAP: bind/search using escaped DN / escaped filter
LDAP-->>AuthAdapter: auth / search result
AuthAdapter-->>Client: authentication result (password removed from authData)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10154 +/- ##
==========================================
+ Coverage 92.16% 92.58% +0.41%
==========================================
Files 192 192
Lines 16181 16192 +11
Branches 183 183
==========================================
+ Hits 14913 14991 +78
+ Misses 1252 1189 -63
+ Partials 16 12 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Adapters/Auth/ldap.js`:
- Line 175: The current replacement at the const filter =
options.groupFilter.replace(/{{id}}/gi, escapeFilter(id)) only escapes for LDAP
filters and allows DN injection when the filter expects a DN (e.g.,
uniqueMember=uid={{id}},o=example). Change the replacement logic so that when
the groupFilter contains a DN-valued placeholder you first construct the full
bind DN for the user (using the same attribute/template the code uses to build
bind DNs), apply a DN-aware escape (escape DN special chars / use a DN-escaping
helper) and then insert that escaped DN into options.groupFilter; keep using
escapeFilter(id) for pure filter-valued placeholders. Update the code path
around the current const filter assignment (where options.groupFilter and
escapeFilter are referenced) and add a regression test that uses an id
containing commas, equals and leading/trailing spaces to ensure the substituted
DN cannot change the matched uniqueMember.
- Around line 121-125: The code calls escapeDN(authData.id) without validating
authData.id which can be missing or non-string and cause a TypeError; update the
logic around escapedId/userCn to first validate that authData && typeof
authData.id === 'string' (or else reject/throw a Parse.Error or return an error)
before calling escapeDN, and ensure any early-return/error uses the same error
type used elsewhere; modify the block around escapeDN, escapedId, and the userCn
construction (references: escapeDN, authData.id, escapedId, userCn, options.dn)
to perform the validation and handle malformed payloads gracefully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 446dbb3d-12c2-4692-b3c3-bff4e91069f3
📒 Files selected for processing (2)
spec/LdapAuth.spec.jssrc/Adapters/Auth/ldap.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/LdapAuth.spec.js (1)
96-128: Consider usingasync/awaitwithtry/finallyfor server cleanup.The tests use the
async donepattern, but if an assertion fails beforeserver.close(done), the server may not be properly closed, potentially causing test suite hangs or port conflicts.♻️ Suggested improvement for reliable cleanup
- it('should reject missing authData.id', async done => { - const server = await mockLdapServer(port, 'uid=testuser, o=example'); - const options = { - suffix: 'o=example', - url: `ldap://localhost:${port}`, - dn: 'uid={{id}}, o=example', - }; - try { - await ldap.validateAuthData({ password: 'secret' }, options); - fail('Should have rejected missing id'); - } catch (err) { - expect(err.message).toBe('LDAP: Wrong username or password'); - } - server.close(done); - }); + it('should reject missing authData.id', async () => { + const server = await mockLdapServer(port, 'uid=testuser, o=example'); + try { + const options = { + suffix: 'o=example', + url: `ldap://localhost:${port}`, + dn: 'uid={{id}}, o=example', + }; + await expectAsync( + ldap.validateAuthData({ password: 'secret' }, options) + ).toBeRejectedWithError('LDAP: Wrong username or password'); + } finally { + await new Promise(resolve => server.close(resolve)); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/LdapAuth.spec.js` around lines 96 - 128, Replace the current async tests that rely on `done` and call `server.close(done)` with an `async` test body that uses try/finally: await `ldap.validateAuthData(...)` inside a try and perform assertions in the try block, and always call `server.close()` in the finally block so the `mockLdapServer(port, ...)` started at the top of each test is reliably closed; reference the test titles ('should reject missing authData.id', 'should reject non-string authData.id'), the `mockLdapServer` variable, and the `ldap.validateAuthData` calls to locate where to wrap the logic in try/finally and remove the `done` callback usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Adapters/Auth/ldap.js`:
- Around line 179-180: The groupFilter replacement in searchForGroup using
escapeFilter(id) can still allow DN-structure based injection when filters are
DN-valued (e.g., "uniqueMember=uid={{id}},o=example"); detect when
options.groupFilter contains a DN-style attribute (look for patterns like
"attr=.*{{id}}" or presence of ","/"=" adjacent to the placeholder) and instead
of simple escapeFilter use strict DN handling: require and parse the incoming id
as a valid DN using the LDAP DN parser (e.g., parseDN/DistinguishedName
utilities) and then canonicalize/serialize it (or reject non-DNs) before
substituting, or reject filters that embed raw DNs and return an error; update
searchForGroup and any callers to validate/normalize DN-valued ids via parseDN
and adjust tests to include DN-normalization edge cases.
---
Nitpick comments:
In `@spec/LdapAuth.spec.js`:
- Around line 96-128: Replace the current async tests that rely on `done` and
call `server.close(done)` with an `async` test body that uses try/finally: await
`ldap.validateAuthData(...)` inside a try and perform assertions in the try
block, and always call `server.close()` in the finally block so the
`mockLdapServer(port, ...)` started at the top of each test is reliably closed;
reference the test titles ('should reject missing authData.id', 'should reject
non-string authData.id'), the `mockLdapServer` variable, and the
`ldap.validateAuthData` calls to locate where to wrap the logic in try/finally
and remove the `done` callback usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9acdbe2d-2e4c-448f-b173-3ae3f4250af4
📒 Files selected for processing (2)
spec/LdapAuth.spec.jssrc/Adapters/Auth/ldap.js
mtrezza
changed the title
parseplatformorg
pushed a commit
that referenced
this pull request
Mar 9, 2026
## [9.5.2-alpha.13](9.5.2-alpha.12...9.5.2-alpha.13) (2026-03-09) ### Bug Fixes * LDAP injection via unsanitized user input in DN and group filter construction ([GHSA-7m6r-fhh7-r47c](https://github.com/parse-community/parse-server/security/advisories/GHSA-7m6r-fhh7-r47c)) ([#10154](#10154)) ([5bbca7b](5bbca7b))
Contributor
|
🎉 This change has been released in version 9.5.2-alpha.13 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Issue
LDAP injection via unsanitized user input in DN and group filter construction (GHSA-7m6r-fhh7-r47c)
Tasks
Summary by CodeRabbit
Security Enhancements
Tests