LDAP: Fixed handling of invalid group valuesHEAD development

Updated LDAP group handling to properly handle empty group values by checking the explode's count property instead of performing a general array count. Also updated logic with DN validation/filtering before LDAP calls are made. For #6088
author: Dan Brown 2026-08-31 11:49:14 +0100
committer: Dan Brown 2026-08-31 11:58:33 +0100
commit: 18f8469a1c72f8cc8497e9372635e6dea5028071 (patch)
tree: 9d4284b899d876887c19128a5410cf0e85b6a123
parent: 7539933d48c824b641a744c5b22d63a48e6d8cc3 (diff)
download: bookstack-development.tar.gz
bookstack-development.tar.zst
bookstack-development.zip
3 files changed, 71 insertions, 16 deletions
diff --git a/app/Access/Ldap.php b/app/Access/Ldap.php
index d14f68821..54d2af72d 100644
--- a/app/Access/Ldap.php
+++ b/app/Access/Ldap.php
@@ -48,7 +48,7 @@ class Ldap
     }
 
     /**
-     * Search LDAP tree using the provided filter.
+     * Search the LDAP tree using the provided filter.
      *
      * @param resource|\LDAP\Connection   $ldapConnection
      *
@@ -95,7 +95,7 @@ class Ldap
     }
 
     /**
-     * Bind to LDAP directory.
+     * Bind to the LDAP directory.
      *
      * @param resource|\LDAP\Connection $ldapConnection
      */
diff --git a/app/Access/LdapService.php b/app/Access/LdapService.php
index 0f456efc2..648262508 100644
--- a/app/Access/LdapService.php
+++ b/app/Access/LdapService.php
@@ -346,13 +346,15 @@ class LdapService
         }
 
         $userGroups = $this->extractGroupsFromSearchResponseEntry($user);
-        $allGroups = $this->getGroupsRecursive($userGroups, []);
+        $filteredGroups = $this->filterGroups($userGroups);
+        $allGroups = $this->getGroupsRecursive($filteredGroups, []);
         $formattedGroups = $this->extractGroupNamesFromLdapGroupDns($allGroups);
 
         if ($this->config['dump_user_groups']) {
             throw new JsonDebugException([
                 'details_from_ldap'            => $user,
                 'parsed_direct_user_groups'    => $userGroups,
+                'parsed_filtered_user_groups'  => $filteredGroups,
                 'parsed_recursive_user_groups' => $allGroups,
                 'parsed_resulting_group_names' => $formattedGroups,
             ]);
@@ -367,7 +369,7 @@ class LdapService
 
         foreach ($groupDNs as $groupDN) {
             $exploded = $this->ldap->explodeDn($groupDN, 1);
-            if ($exploded !== false && count($exploded) > 0) {
+            if ($exploded !== false && $exploded['count'] > 0) {
                 $names[] = $exploded[0];
             }
         }
@@ -390,7 +392,8 @@ class LdapService
             }
 
             $parentGroups = $this->getParentsOfGroup($groupDN);
-            $groupsToAdd = array_merge($groupsToAdd, $parentGroups);
+            $parentGroupsFiltered = $this->filterGroups($parentGroups);
+            $groupsToAdd = array_merge($groupsToAdd, $parentGroupsFiltered);
             $checked[] = $groupDN;
         }
 
@@ -404,6 +407,20 @@ class LdapService
     }
 
     /**
+     * @param string[] $groupDNs
+     * @return string[]
+     */
+    protected function filterGroups(array $groupDNs): array
+    {
+        $filtered = array_filter($groupDNs, function (string $groupDN) {
+            $exploded = $this->ldap->explodeDn($groupDN, 1);
+            return $exploded !== false && $exploded['count'] > 0;
+        });
+
+        return array_values($filtered);
+    }
+
+    /**
      * @throws LdapException
      */
     protected function getParentsOfGroup(string $groupDN): array
diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php
index 5adbe11a1..9bf15ae2b 100644
--- a/tests/Auth/LdapTest.php
+++ b/tests/Auth/LdapTest.php
@@ -43,7 +43,7 @@ class LdapTest extends TestCase
         $this->mockUser = User::factory()->make();
     }
 
-    protected function runFailedAuthLogin()
+    protected function runFailedAuthLogin(): void
     {
         $this->commonLdapMocks(1, 1, 1, 1, 1);
         $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1)
@@ -51,14 +51,14 @@ class LdapTest extends TestCase
         $this->post('/login', ['username' => 'timmyjenkins', 'password' => 'cattreedog']);
     }
 
-    protected function mockEscapes($times = 1)
+    protected function mockEscapes($times = 1): void
     {
         $this->mockLdap->shouldReceive('escape')->times($times)->andReturnUsing(function ($val) {
             return ldap_escape($val);
         });
     }
 
-    protected function mockExplodes($times = 1)
+    protected function mockExplodes($times = 1): void
     {
         $this->mockLdap->shouldReceive('explodeDn')->times($times)->andReturnUsing(function ($dn, $withAttrib) {
             return ldap_explode_dn($dn, $withAttrib);
@@ -76,7 +76,7 @@ class LdapTest extends TestCase
     /**
      * Set LDAP method mocks for things we commonly call without altering.
      */
-    protected function commonLdapMocks(int $connects = 1, int $versions = 1, int $options = 2, int $binds = 4, int $escapes = 2, int $explodes = 0, int $groups = 0)
+    protected function commonLdapMocks(int $connects = 1, int $versions = 1, int $options = 2, int $binds = 4, int $escapes = 2, int $explodes = 0, int $groups = 0): void
     {
         $this->mockLdap->shouldReceive('connect')->times($connects)->andReturn($this->resourceId);
         $this->mockLdap->shouldReceive('setVersion')->times($versions);
@@ -364,7 +364,7 @@ class LdapTest extends TestCase
             'services.ldap.remove_from_groups' => false,
         ]);
 
-        $this->commonLdapMocks(1, 1, 4, 5, 2, 2, 2);
+        $this->commonLdapMocks(1, 1, 4, 5, 2, 4, 2);
         $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
@@ -409,7 +409,7 @@ class LdapTest extends TestCase
             'services.ldap.remove_from_groups' => true,
         ]);
 
-        $this->commonLdapMocks(1, 1, 3, 4, 2, 1, 1);
+        $this->commonLdapMocks(1, 1, 3, 4, 2, 2, 1);
         $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
@@ -451,7 +451,7 @@ class LdapTest extends TestCase
             'dn'       => 'dc=test,' . config('services.ldap.base_dn'),
             'mail'     => [$this->mockUser->email],
         ]];
-        $this->commonLdapMocks(1, 1, 4, 5, 2, 2, 0);
+        $this->commonLdapMocks(1, 1, 4, 5, 2, 4, 0);
         $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn($userResp, ['count' => 1,
@@ -523,7 +523,7 @@ class LdapTest extends TestCase
                       ],
         ];
 
-        $this->commonLdapMocks(1, 1, 3, 4, 2, 1);
+        $this->commonLdapMocks(1, 1, 3, 4, 2, 2);
 
         $escapedName = ldap_escape($this->mockUser->name);
         $this->mockLdap->shouldReceive('searchAndGetEntries')->twice()
@@ -559,7 +559,7 @@ class LdapTest extends TestCase
             'services.ldap.remove_from_groups' => true,
         ]);
 
-        $this->commonLdapMocks(1, 1, 3, 4, 2, 1, 1);
+        $this->commonLdapMocks(1, 1, 3, 4, 2, 2, 1);
         $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
@@ -600,7 +600,7 @@ class LdapTest extends TestCase
             'services.ldap.remove_from_groups' => true,
         ]);
 
-        $this->commonLdapMocks(1, 1, 4, 5, 2, 2, 2);
+        $this->commonLdapMocks(1, 1, 4, 5, 2, 4, 2);
         $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
             ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
             ->andReturn(['count' => 1, 0 => [
@@ -628,6 +628,44 @@ class LdapTest extends TestCase
         ]);
     }
 
+    public function test_login_group_mapping_gracefully_handles_utf8_and_invalid_group_dns()
+    {
+        $invalidRole = Role::factory()->create(['display_name' => 'LdapTester']);
+        $invalidRole2 = Role::factory()->create(['display_name' => 'beans']);
+        $roleToReceive2 = Role::factory()->create(['display_name' => 'ldapper']);
+        $this->mockUser->forceFill(['external_auth_id' => $this->mockUser->name])->save();
+
+        app('config')->set([
+            'services.ldap.user_to_groups'     => true,
+            'services.ldap.group_attribute'    => 'memberOf',
+            'services.ldap.remove_from_groups' => true,
+        ]);
+
+        $this->commonLdapMocks(1, 1, 4, 5, 2, 6, 2);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2)
+            ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
+            ->andReturn(['count' => 1, 0 => [
+                'uid'      => [$this->mockUser->name],
+                'cn'       => [$this->mockUser->name],
+                'dn'       => 'dc=test' . config('services.ldap.base_dn'),
+                'mail'     => [$this->mockUser->email],
+                'memberof' => [
+                    'count' => 4,
+                    0       => 'cn=ldaptëstër,ou=groups,dc=ëxamplë,dc=com',
+                    1       => 'cn=ldapper,ou=groups,dc=example,dc=com',
+                    2       => 'bëans',
+                    3       => '',
+                ],
+            ]]);
+
+        $this->mockUserLogin()->assertRedirect('/');
+
+        $user = User::query()->where('email', $this->mockUser->email)->first();
+        $this->assertDatabaseMissing('role_user', ['user_id' => $user->id, 'role_id' => $invalidRole->id]);
+        $this->assertDatabaseMissing('role_user', ['user_id' => $user->id, 'role_id' => $invalidRole2->id]);
+        $this->assertDatabaseHas('role_user', ['user_id' => $user->id, 'role_id' => $roleToReceive2->id]);
+    }
+
     public function test_login_uses_specified_display_name_attribute()
     {
         app('config')->set([
@@ -867,7 +905,7 @@ class LdapTest extends TestCase
             'services.ldap.remove_from_groups' => true,
         ]);
 
-        $this->commonLdapMocks(1, 1, 6, 8, 4, 2, 2);
+        $this->commonLdapMocks(1, 1, 6, 8, 4, 4, 2);
         $this->mockLdap->shouldReceive('searchAndGetEntries')
             ->times(4)
             ->andReturn(['count' => 1, 0 => [
author	Dan Brown	2026-08-31 11:49:14 +0100
committer	Dan Brown	2026-08-31 11:58:33 +0100
commit	18f8469a1c72f8cc8497e9372635e6dea5028071 (patch)
tree	9d4284b899d876887c19128a5410cf0e85b6a123
parent	7539933d48c824b641a744c5b22d63a48e6d8cc3 (diff)
download	bookstack-development.tar.gz bookstack-development.tar.zst bookstack-development.zip