fix(auth): sanitise username before LDAP DN and filter insertion by deacon-mp · Pull Request #3334 · apache/caldera

deacon-mp · 2026-03-16T03:56:54Z

Summary

_ldap_login in app/service/login_handlers/default.py inserted the raw, unsanitised username directly into an LDAP Distinguished Name (DN) string via str.format(), allowing injection of DN metacharacters (e.g. commas, equals signs) to manipulate the bind DN
_ldap_get_group inserted the same unsanitised username into an LDAP search filter string (%s=%s), allowing injection of filter metacharacters (e.g. *, )(, null bytes)
Both paths are addressed by importing and applying the appropriate ldap3 escaping helpers (already available via the existing ldap3 dependency — no new dependencies required)

Changes

app/service/login_handlers/default.py:
- Import escape_rdn_value from ldap3.utils.dn (for DN component sanitisation)
- Import escape_filter_chars from ldap3.utils.conv (for search filter sanitisation)
- Apply escape_rdn_value(username) before inserting into the DN format string
- Apply escape_filter_chars(username) before inserting into the LDAP search filter
tests/services/test_ldap_injection.py: new tests asserting that injection metacharacters are properly escaped and that both helpers are importable from the handler module

Test plan

pytest tests/services/test_ldap_injection.py passes
Existing login handler and auth service tests pass
Manual verification: usernames containing ,, =, *, )(, and null bytes do not alter DN or filter structure

The `_ldap_login` method inserted the raw username directly into the LDAP distinguished-name string via str.format(), and `_ldap_get_group` inserted it unsanitised into a search-filter string. Both paths were vulnerable to LDAP injection. Fix by importing `escape_rdn_value` from `ldap3.utils.dn` and `escape_filter_chars` from `ldap3.utils.conv` (both already ship with the ldap3 dependency) and applying them before the respective string-building steps. Add tests/services/test_ldap_injection.py to assert that injection metacharacters are escaped and that the helpers are reachable from the handler module.

Copilot

Pull request overview

Hardens LDAP authentication by escaping user-controlled usernames before inserting them into LDAP DNs and LDAP search filters to prevent DN/filter injection.

Changes:

Escape username via ldap3.utils.dn.escape_rdn_value before DN formatting in _ldap_login
Escape username via ldap3.utils.conv.escape_filter_chars before building LDAP search filters in _ldap_get_group
Add pytest coverage for escaping helpers and import availability

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
app/service/login_handlers/default.py	Escapes username for DN and LDAP filter construction in login/group lookup flows
tests/services/test_ldap_injection.py	Adds tests around escaping behavior and verifies helper imports from the handler module

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    def test_escapes_comma(self):
+        result = escape_rdn_value("admin,dc=evil,dc=com")
+        assert "," not in result or result.startswith("\\")


+class TestDefaultLoginHandlerImports:
+    """Verify that the login handler imports the sanitisation helpers."""
+
+    def test_escape_rdn_value_imported(self):
+        from app.service.login_handlers.default import escape_rdn_value as _erv
+        assert callable(_erv)
+
+    def test_escape_filter_chars_imported(self):
+        from app.service.login_handlers.default import escape_filter_chars as _efc
+        assert callable(_efc)


- Fix test_escapes_comma assertion to check for backslash-escaped comma (\\, or hex \\2c) rather than an ambiguous startswith check - Add TestLdapLoginEscapesUsername integration tests that monkeypatch ldap3.Connection and assert injected commas/filter chars are escaped in the bind DN and search filter constructed by _ldap_login/_ldap_get_group

Copilot

Pull request overview

Hardens LDAP authentication logic against DN and filter injection by escaping usernames before insertion into bind DN strings and LDAP search filters.

Changes:

Escape username for DN/RDN contexts via ldap3.utils.dn.escape_rdn_value in _ldap_login.
Escape username for LDAP filter contexts via ldap3.utils.conv.escape_filter_chars in _ldap_get_group.
Add regression tests covering common injection metacharacters and handler behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
app/service/login_handlers/default.py	Escapes username before building bind DN and LDAP search filter to prevent injection.
tests/services/test_ldap_injection.py	Adds tests for escaping helpers and verifies handler methods don’t transmit unescaped injection strings.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+DefaultLoginHandler before they are inserted into DN strings or
+LDAP search filters."""
+import pytest
+from unittest.mock import AsyncMock, MagicMock, patch


+class TestDefaultLoginHandlerImports:
+    """Verify that the login handler imports the sanitisation helpers."""
+
+    def test_escape_rdn_value_imported(self):
+        from app.service.login_handlers.default import escape_rdn_value as _erv
+        assert callable(_erv)
+
+    def test_escape_filter_chars_imported(self):
+        from app.service.login_handlers.default import escape_filter_chars as _efc
+        assert callable(_efc)
+
+


+        username_part = bind_dn.split(",dc=example")[0]
+        assert "admin,dc=evil" not in username_part, (
+            f"Unescaped injection username found in bind DN: {bind_dn!r}"
+        )


+        mock_conn.entries = [mock_entry]
+
+        def fake_search(dn, search_filter, attributes=None):
+            captured_filters.append(search_filter)


+    def test_escapes_null_byte(self):
+        result = escape_rdn_value("admin\x00")
+        assert "\x00" not in result


The null byte tests only verified that literal NUL was absent but did not confirm it was properly escaped. Now assert the expected \00 hex escape sequence is present in the output.

Copilot

Pull request overview

This PR hardens LDAP authentication against DN and filter injection by escaping usernames before inserting them into bind DNs and LDAP search filters.

Changes:

Escape username for DN construction in _ldap_login using ldap3.utils.dn.escape_rdn_value
Escape username for LDAP filters in _ldap_get_group using ldap3.utils.conv.escape_filter_chars
Add tests covering escaping behavior and verifying helper imports

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
app/service/login_handlers/default.py	Escapes username before DN and filter interpolation to prevent LDAP injection
tests/services/test_ldap_injection.py	Adds tests to validate escaping behavior for DN and filters

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+DefaultLoginHandler before they are inserted into DN strings or
+LDAP search filters."""
+import pytest
+from unittest.mock import AsyncMock, MagicMock, patch


+    def test_escapes_equals(self):
+        result = escape_rdn_value("user=injected")
+        # ldap3 escapes the = so it cannot break the DN structure
+        assert result != "user=injected"


+import pytest
+from unittest.mock import AsyncMock, MagicMock, patch


+class TestDefaultLoginHandlerImports:
+    """Verify that the login handler imports the sanitisation helpers."""
+
+    def test_escape_rdn_value_imported(self):
+        from app.service.login_handlers.default import escape_rdn_value as _erv
+        assert callable(_erv)
+
+    def test_escape_filter_chars_imported(self):
+        from app.service.login_handlers.default import escape_filter_chars as _efc
+        assert callable(_efc)
+
+


deacon-mp requested a review from Copilot March 16, 2026 03:57

Copilot AI reviewed Mar 16, 2026

View reviewed changes

Copilot started reviewing on behalf of deacon-mp March 16, 2026 04:25 View session

deacon-mp requested a review from Copilot March 16, 2026 04:33

Copilot AI reviewed Mar 16, 2026

View reviewed changes

Copilot started reviewing on behalf of deacon-mp March 16, 2026 04:52 View session

Add assertions for expected NUL escape representation in LDAP tests

a503205

The null byte tests only verified that literal NUL was absent but did not confirm it was properly escaped. Now assert the expected \00 hex escape sequence is present in the output.

deacon-mp requested a review from Copilot March 16, 2026 13:56

Copilot AI reviewed Mar 16, 2026

View reviewed changes

Copilot started reviewing on behalf of deacon-mp March 16, 2026 14:21 View session

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(auth): sanitise username before LDAP DN and filter insertion#3334

fix(auth): sanitise username before LDAP DN and filter insertion#3334
deacon-mp wants to merge 3 commits into
masterfrom
fix/ldap-dn-injection

deacon-mp commented Mar 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		import pytest
		from unittest.mock import AsyncMock, MagicMock, patch

Conversation

deacon-mp commented Mar 16, 2026

Summary

Changes

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants