From fff05b0b725fcd0f0f17861bd0d297df96aafdfa Mon Sep 17 00:00:00 2001 From: tchapi Date: Thu, 10 Sep 2026 21:27:32 +0200 Subject: [PATCH 1/5] chore --- README.md | 21 ++++----------------- docker/configurations/Caddyfile | 4 ---- docker/configurations/nginx.conf | 3 --- public/.htaccess | 4 ---- src/Controller/DAVController.php | 25 ++++++++++++++++++++++++- 5 files changed, 28 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 8b9b718b..33a05c43 100644 --- a/README.md +++ b/README.md @@ -438,24 +438,11 @@ More examples and information [here](https://symfony.com/doc/current/setup/web_s Web-based protocols like CalDAV and CardDAV can be found using a discovery service. Some clients require that you implement a path prefix to point to the correct location for your service. See [here](https://en.wikipedia.org/wiki/List_of_/.well-known/_services_offered_by_webservers) for more info. -If you use Apache as your webserver, you can enable the redirections with: +Davis answers `/.well-known/caldav` and `/.well-known/carddav` itself and redirects them to its DAV endpoint, so **no web server configuration is needed**. Because the redirect is built from the application's own base path, it also works when Davis is installed in a sub-directory (`https://example.org/davis/`), which a hard-coded `/dav/` rewrite does not. -```apache -RewriteEngine On -RewriteRule ^\.well-known/carddav /dav/ [R=301,L] -RewriteRule ^\.well-known/caldav /dav/ [R=301,L] -``` - -Make sure that `mod_rewrite` is enabled on your installation beforehand. - -If you use Nginx, you can add this to your configuration: - -```nginx -location / { - rewrite ^/.well-known/carddav /dav/ redirect; - rewrite ^/.well-known/caldav /dav/ redirect; -} -``` +> [!NOTE] +> +> If your web server still rewrites these two paths itself (earlier versions of this README suggested doing so), you can remove those rules: they take precedence over Davis and will send clients to the wrong place on a sub-directory installation. # 🐳 Dockerized installation diff --git a/docker/configurations/Caddyfile b/docker/configurations/Caddyfile index abb618f6..eb109e92 100644 --- a/docker/configurations/Caddyfile +++ b/docker/configurations/Caddyfile @@ -3,10 +3,6 @@ } :9000 { - # Redirect .well-known - redir /.well-known/caldav /dav/ - redir /.well-known/carddav /dav/ - root * /var/www/davis/public php_fastcgi unix//var/run/php-fpm/php-fpm.sock { # Preserve the original X-Forwarded-Proto from upstream, as it might be HTTPS diff --git a/docker/configurations/nginx.conf b/docker/configurations/nginx.conf index bfd4ef6e..89291f8f 100644 --- a/docker/configurations/nginx.conf +++ b/docker/configurations/nginx.conf @@ -14,9 +14,6 @@ server { root /var/www/davis/public/; index index.php; - rewrite ^/.well-known/caldav /dav/ redirect; - rewrite ^/.well-known/carddav /dav/ redirect; - charset utf-8; # Security headers (add `Strict-Transport-Security` once TLS is terminated in front of nginx) diff --git a/public/.htaccess b/public/.htaccess index 86559844..2776637c 100644 --- a/public/.htaccess +++ b/public/.htaccess @@ -20,10 +20,6 @@ DirectoryIndex index.php RewriteEngine On - # Add .well-known redirections - RewriteRule ^\.well-known/carddav /dav/ [R=301,L] - RewriteRule ^\.well-known/caldav /dav/ [R=301,L] - # Determine the RewriteBase automatically and set it as environment variable. # If you are using Apache aliases to do mass virtual hosting or installed the # project in a subdirectory, the base path will be prepended to allow proper diff --git a/src/Controller/DAVController.php b/src/Controller/DAVController.php index abb1e5d0..ecdc0890 100644 --- a/src/Controller/DAVController.php +++ b/src/Controller/DAVController.php @@ -312,6 +312,20 @@ private function initExceptionListener() }); } + /** + * Service discovery (RFC 6764). + * + * This lives in the application rather than in each web server's configuration so that + * the redirect is built from the real base path: a hard-coded `/dav/` sends clients to + * the wrong place whenever Davis is installed under a sub-directory. + */ + #[Route('/.well-known/caldav', name: 'well_known_caldav')] + #[Route('/.well-known/carddav', name: 'well_known_carddav')] + public function wellKnown(): Response + { + return $this->redirectToRoute('dav', ['path' => ''], Response::HTTP_MOVED_PERMANENTLY); + } + #[Route('/dav/{path}', name: 'dav', requirements: ['path' => '.*'])] public function dav(Request $request, ?string $path, ?Profiler $profiler = null) { @@ -327,7 +341,16 @@ public function dav(Request $request, ?string $path, ?Profiler $profiler = null) // Adapted from CorePlugin's httpOptions() // https://github.com/sabre-io/dav/blob/master/lib/DAV/CorePlugin.php#L210 - $methods = $this->server->getAllowedMethods(''); + // + // The methods depend on the node being asked about: MKCALENDAR, for instance, is + // only offered inside a calendar home. Answering for the root instead of the + // requested path told every client the same, incomplete story. + try { + $methods = $this->server->getAllowedMethods($path ?? ''); + } catch (\Throwable $e) { + // An unresolvable path should still get a usable answer + $methods = $this->server->getAllowedMethods(''); + } $response->headers->set('Allow', strtoupper(implode(', ', $methods))); $features = ['1', '3', 'extended-mkcol']; From 8c40fe4dac27c6bd55e6b474c62e0251fe47c2b4 Mon Sep 17 00:00:00 2001 From: tchapi Date: Thu, 10 Sep 2026 21:29:43 +0200 Subject: [PATCH 2/5] tests --- tests/Functional/DavTest.php | 39 ++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/Functional/DavTest.php b/tests/Functional/DavTest.php index cb43e828..981e9f10 100644 --- a/tests/Functional/DavTest.php +++ b/tests/Functional/DavTest.php @@ -194,4 +194,43 @@ public function testBrowserAssetsAreServedAnonymouslyFromRoot(): void $client->getResponse()->getContent() ); } + + public function testWellKnownUrlsRedirectToTheDavEndpoint(): void + { + $client = static::createClient(); + + foreach (['/.well-known/caldav', '/.well-known/carddav'] as $wellKnown) { + $client->request('GET', $wellKnown); + + $this->assertResponseStatusCodeSame(301, $wellKnown.' should redirect'); + $this->assertResponseRedirects('/dav/'); + } + } + + /** + * OPTIONS used to answer for the server root whatever was asked, so it never advertised + * the methods that only exist deeper in the tree, MKCALENDAR being the obvious one. + */ + public function testOptionsDescribesTheRequestedPath(): void + { + $client = static::createClient(); + + static::requestDav($client, 'OPTIONS', '/dav/'); + $this->assertResponseIsSuccessful(); + $this->assertStringNotContainsString('MKCALENDAR', (string) $client->getResponse()->headers->get('Allow')); + + static::requestDav($client, 'OPTIONS', '/dav/calendars/test_user/new-calendar'); + $this->assertResponseIsSuccessful(); + $this->assertStringContainsString('MKCALENDAR', (string) $client->getResponse()->headers->get('Allow')); + } + + public function testOptionsOnAnUnresolvablePathStillAnswers(): void + { + $client = static::createClient(); + + static::requestDav($client, 'OPTIONS', '/dav/calendars/nope/nope/nope'); + + $this->assertResponseIsSuccessful(); + $this->assertStringContainsString('PROPFIND', (string) $client->getResponse()->headers->get('Allow')); + } } From e7a06afadec0ed294d3d2f426a9baecb0dbbac97 Mon Sep 17 00:00:00 2001 From: tchapi Date: Thu, 10 Sep 2026 21:49:32 +0200 Subject: [PATCH 3/5] username --- src/Controller/Api/ApiController.php | 3 +- src/Entity/User.php | 10 +++ src/Form/UserType.php | 6 ++ src/Services/AbstractAuth.php | 14 ++++ src/Services/IMAPAuth.php | 18 +++-- src/Services/LDAPAuth.php | 69 ++++++++++------ src/Services/Utils.php | 12 +++ .../Controllers/UserControllerTest.php | 25 ++++++ tests/Functional/Service/AuthBackendTest.php | 27 +++++++ tests/Functional/Service/LDAPAuthTest.php | 77 ++++++++++++++++++ tests/Functional/Service/UtilsTest.php | 80 +++++++++++++++++++ translations/validators.de.xlf | 4 + translations/validators.en.xlf | 4 + translations/validators.fr.xlf | 4 + 14 files changed, 318 insertions(+), 35 deletions(-) create mode 100644 tests/Functional/Service/LDAPAuthTest.php create mode 100644 tests/Functional/Service/UtilsTest.php diff --git a/src/Controller/Api/ApiController.php b/src/Controller/Api/ApiController.php index 9932540a..59908b4c 100644 --- a/src/Controller/Api/ApiController.php +++ b/src/Controller/Api/ApiController.php @@ -7,6 +7,7 @@ use App\Entity\CalendarSubscription; use App\Entity\Principal; use App\Entity\User; +use App\Services\Utils; use Doctrine\Persistence\ManagerRegistry; use Sabre\DAV\Sharing\Plugin as SharingPlugin; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; @@ -26,7 +27,7 @@ class ApiController extends AbstractController */ private function validateUsername(string $username): bool { - return !empty($username) && is_string($username) && !preg_match('/[^a-zA-Z0-9_.@-]/', $username); + return Utils::isValidUsername($username); } /** diff --git a/src/Entity/User.php b/src/Entity/User.php index a1219608..0a12e509 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -18,8 +18,18 @@ class User #[ORM\Column(type: 'integer')] private $id; + /** + * A username ends up in the principal URI (`principals/`), so it must not carry + * anything that would change that path's structure. Letters, digits and `_ . @ + ' -` are allowed: + * the punctuation is what shows up in mail-derived login names. Enforced when a user is created; existing + * accounts are left alone so that an odd username created before this rule stays editable. + */ + public const USERNAME_PATTERN = '/^[a-zA-Z0-9_.@+\'-]+$/'; + #[ORM\Column(type: 'string', length: 255, unique: true)] #[Assert\NotBlank] + #[Assert\Length(max: 255, groups: ['creation'])] + #[Assert\Regex(pattern: self::USERNAME_PATTERN, message: 'form.username.invalid', groups: ['creation'])] private $username; #[ORM\Column(name: 'digesta1', type: 'string', length: 255)] diff --git a/src/Form/UserType.php b/src/Form/UserType.php index 2b76a523..e43cbcb2 100644 --- a/src/Form/UserType.php +++ b/src/Form/UserType.php @@ -11,6 +11,7 @@ use Symfony\Component\Form\Extension\Core\Type\SubmitType; use Symfony\Component\Form\Extension\Core\Type\TextType; use Symfony\Component\Form\FormBuilderInterface; +use Symfony\Component\Form\FormInterface; use Symfony\Component\OptionsResolver\OptionsResolver; class UserType extends AbstractType @@ -55,6 +56,11 @@ public function configureOptions(OptionsResolver $resolver): void $resolver->setDefaults([ 'new' => false, 'data_class' => User::class, + // The username rule only applies to new accounts: the field is disabled when editing, + // and an account created before the rule (or by LDAP/IMAP) must stay editable. + 'validation_groups' => static fn (FormInterface $form): array => $form->getConfig()->getOption('new') + ? ['Default', 'creation'] + : ['Default'], ]); } } diff --git a/src/Services/AbstractAuth.php b/src/Services/AbstractAuth.php index 9d57bc7b..4742cd0e 100644 --- a/src/Services/AbstractAuth.php +++ b/src/Services/AbstractAuth.php @@ -12,6 +12,13 @@ * and an empty password means an *unauthenticated bind* for LDAP servers, which Active * Directory (and OpenLDAP with `allow bind_anon_cred`) answers with success, i.e. it * would log the caller in as any user. + * + * It also rejects usernames that would not survive being put in a principal URI. sabre + * derives the principal from the login name (`principals/`), so a name containing + * a slash would address a different, possibly existing, node: `alice/calendar-proxy-write` + * is exactly the URI Davis uses for alice's delegation proxy. Only structural characters are + * refused here, not the stricter set required when creating an account, so that an unusual + * but working username keeps authenticating. */ abstract class AbstractAuth extends AbstractBasic { @@ -25,6 +32,13 @@ final protected function validateUserPass($username, $password): bool return false; } + // Anything that would change the shape of `principals/`: + // [/\\] a forward or back slash, which would add a path segment + // [\x00-\x20\x7f] any control character, plus space (0x20) and DEL (0x7f) + if (1 === preg_match('~[/\\\\]|[\\x00-\\x20\\x7f]~', $username)) { + return false; + } + return $this->checkCredentials($username, $password); } diff --git a/src/Services/IMAPAuth.php b/src/Services/IMAPAuth.php index bed48d33..51cf4e7d 100644 --- a/src/Services/IMAPAuth.php +++ b/src/Services/IMAPAuth.php @@ -130,15 +130,17 @@ protected function imapOpen(string $username, string $password): bool $user = $this->doctrine->getRepository(User::class)->findOneBy(['username' => $username]); if (!$user) { - // We only have a username, so we use it for displayname and email - $this->utils->createPasswordlessUserWithDefaultObjects($username, $username, $username); - - $em = $this->doctrine->getManager(); - try { - $em->flush(); - } catch (\Exception $e) { - error_log('IMAP Error (flush): '.$e->getMessage()); + // We only have a username, so we use it for displayname and email + $this->utils->createPasswordlessUserWithDefaultObjects($username, $username, $username); + $this->doctrine->getManager()->flush(); + } catch (\Throwable $e) { + // Letting the login through without a principal would leave the account + // authenticated but unusable: no calendar home, so clients fall back to the + // server root and every write is refused. + error_log('IMAP Error (could not create the user "'.$username.'"): '.$e->getMessage()); + + return false; } } } diff --git a/src/Services/LDAPAuth.php b/src/Services/LDAPAuth.php index c0e9d7ab..4381f83c 100644 --- a/src/Services/LDAPAuth.php +++ b/src/Services/LDAPAuth.php @@ -82,6 +82,39 @@ public function __construct(ManagerRegistry $doctrine, Utils $utils, string $LDA $this->utils = $utils; } + /** + * Builds the bind DN for a username by filling the placeholders of LDAP_DN_PATTERN. + * + * Every substituted value is escaped for a DN context: without that, a username such as + * `someone,ou=admins` would not be a value inside the DN but extra structure, changing + * which entry we bind against. + */ + protected function buildDn(string $username): string + { + $escape = static fn (string $value): string => ldap_escape($value, '', LDAP_ESCAPE_DN); + + // Extract user and domain from username (in the form user@domain.org) + $user_parts = explode('@', $username, 2); + + $ldap_user = $user_parts[0]; + $ldap_domain = $user_parts[1] ?? ''; + + // Replace common placeholders + $dn = str_replace( + ['%u', '%U', '%d'], + [$escape($username), $escape($ldap_user), $escape($ldap_domain)], + $this->LDAPDnPattern + ); + + // Replace domain parts + $domain_split = array_reverse(explode('.', $ldap_domain)); + for ($i = 1; $i <= count($domain_split) and $i <= 9; ++$i) { + $dn = str_replace('%'.$i, $escape($domain_split[$i - 1]), $dn); + } + + return $dn; + } + /** * Connects to an LDAP server and tries to authenticate. * @@ -140,25 +173,7 @@ protected function ldapOpen($username, $password) return false; } - // Extract user and domain from username (in the form user@domain.org) - $user_parts = explode('@', $username, 2); - - $ldap_user = $user_parts[0]; - - if (count($user_parts) > 1) { - $ldap_domain = $user_parts[1]; - } else { - $ldap_domain = ''; - } - - // Replace common placeholders - $dn = str_replace(['%u', '%U', '%d'], [$username, $ldap_user, $ldap_domain], $this->LDAPDnPattern); - - // Replace domain parts - $domain_split = array_reverse(explode('.', $ldap_domain)); - for ($i = 1; $i <= count($domain_split) and $i <= 9; ++$i) { - $dn = str_replace('%'.$i, $domain_split[$i - 1], $dn); - } + $dn = $this->buildDn($username); $success = false; try { @@ -200,14 +215,16 @@ protected function ldapOpen($username, $password) } } - $this->utils->createPasswordlessUserWithDefaultObjects($username, $displayName, $email); - - $em = $this->doctrine->getManager(); - try { - $em->flush(); - } catch (\Exception $e) { - error_log('LDAP Error (flush): '.$e->getMessage()); + $this->utils->createPasswordlessUserWithDefaultObjects($username, $displayName, $email); + $this->doctrine->getManager()->flush(); + } catch (\Throwable $e) { + // Letting the login through without a principal would leave the account + // authenticated but unusable: no calendar home, so clients fall back to the + // server root and every write is refused. + error_log('LDAP Error (could not create the user "'.$username.'"): '.$e->getMessage()); + + $success = false; } } } diff --git a/src/Services/Utils.php b/src/Services/Utils.php index 1354bd50..38aca4bd 100644 --- a/src/Services/Utils.php +++ b/src/Services/Utils.php @@ -49,8 +49,20 @@ public function hashPassword(string $username, string $password): string return md5($username.':'.$this->authRealm.':'.$password); } + /** + * A username is only acceptable if it can be used verbatim in a principal URI. + */ + public static function isValidUsername(?string $username): bool + { + return null !== $username && '' !== $username && 1 === preg_match(User::USERNAME_PATTERN, $username); + } + public function createPasswordlessUserWithDefaultObjects(string $username, string $displayName, string $email) { + if (!self::isValidUsername($username)) { + throw new \InvalidArgumentException(sprintf('Refusing to create the user "%s": a username may only contain letters, digits and the characters _ . @ + \' -', $username)); + } + $user = new User(); $user->setUsername($username); diff --git a/tests/Functional/Controllers/UserControllerTest.php b/tests/Functional/Controllers/UserControllerTest.php index 53b2b749..344837bd 100644 --- a/tests/Functional/Controllers/UserControllerTest.php +++ b/tests/Functional/Controllers/UserControllerTest.php @@ -228,4 +228,29 @@ public function testDelegateRemoveThroughAnotherUsersProxyIs404(): void $this->postAdmin($client, '/users/delegates/'.$userId.'/remove/'.$foreignProxy->getId().'/'.$delegate->getId()); $this->assertResponseStatusCodeSame(404); } + + public function testUserCreationRejectsAUsernameThatBreaksThePrincipalUri(): void + { + $user = new AdminUser('admin', 'test'); + + $client = static::createClient(); + $client->loginUser($user); + + $crawler = $client->request('GET', '/users/new'); + $form = $crawler->selectButton('user_save')->form(); + + $client->submit($form, [ + 'user[username]' => 'bad/user', + 'user[displayName]' => 'Bad User', + 'user[email]' => 'bad@example.org', + 'user[password][first]' => 'secret', + 'user[password][second]' => 'secret', + ]); + + // The form is re-rendered rather than redirecting, and nothing is created + $this->assertResponseIsSuccessful(); + $this->assertNull( + static::getContainer()->get('doctrine.orm.entity_manager')->getRepository(User::class)->findOneByUsername('bad/user') + ); + } } diff --git a/tests/Functional/Service/AuthBackendTest.php b/tests/Functional/Service/AuthBackendTest.php index dc1b70b9..da0fe0f4 100644 --- a/tests/Functional/Service/AuthBackendTest.php +++ b/tests/Functional/Service/AuthBackendTest.php @@ -103,4 +103,31 @@ public function testBasicAuthStillAcceptsValidCredentials(): void [$ok] = self::check($backend, 'test_user:wrong'); $this->assertFalse($ok); } + + /** + * A username becomes the principal URI (`principals/`), so one containing a + * slash would address a different node — `alice/calendar-proxy-write` is exactly the URI + * Davis uses for alice's delegation proxy. + */ + public function testUsernamesThatWouldBreakThePrincipalUriAreRejected(): void + { + foreach (['alice/calendar-proxy-write', 'alice\\bob', 'alice bob', "alice\tbob", "alice\nbob"] as $username) { + $backend = self::acceptAllBackend(); + + [$ok] = self::check($backend, $username.':password'); + + $this->assertFalse($ok, sprintf('%s must not authenticate', var_export($username, true))); + $this->assertSame([], $backend->seen, 'The backend must not even be consulted'); + } + } + + public function testAnUnusualButStructurallySoundUsernameStillAuthenticates(): void + { + $backend = self::acceptAllBackend(); + + [$ok, $principal] = self::check($backend, 'first.last+tag@example.org:password'); + + $this->assertTrue($ok); + $this->assertSame('principals/first.last+tag@example.org', $principal); + } } diff --git a/tests/Functional/Service/LDAPAuthTest.php b/tests/Functional/Service/LDAPAuthTest.php new file mode 100644 index 00000000..2f06d185 --- /dev/null +++ b/tests/Functional/Service/LDAPAuthTest.php @@ -0,0 +1,77 @@ +get(ManagerRegistry::class), + $container->get(Utils::class), + 'ldap://127.0.0.1', + $pattern, + 'mail', + false, + 'try' + ); + + // buildDn is protected; no setAccessible() needed since PHP 8.1 + return (new \ReflectionMethod($backend, 'buildDn'))->invoke($backend, $username); + } + + public function testPlaceholdersAreFilledIn(): void + { + $this->assertSame( + 'uid=alice,ou=users,dc=example,dc=com', + $this->buildDn('uid=%u,ou=users,dc=example,dc=com', 'alice') + ); + } + + public function testUserAndDomainPartsAreSplitOnTheAtSign(): void + { + $this->assertSame( + 'uid=alice,dc=example.org', + $this->buildDn('uid=%U,dc=%d', 'alice@example.org') + ); + } + + public function testDomainComponentsAreAvailableInReverseOrder(): void + { + $this->assertSame( + 'uid=alice,dc=example,dc=org', + $this->buildDn('uid=%U,dc=%2,dc=%1', 'alice@example.org') + ); + } + + /** + * Regression test: the username was interpolated into the DN pattern verbatim, so a name + * carrying DN syntax added structure to the DN instead of being a value inside it. + */ + public function testAUsernameCannotInjectDnStructure(): void + { + $evil = 'alice,ou=admins'; + + $dn = $this->buildDn('uid=%u,ou=users,dc=example,dc=com', $evil); + + $this->assertSame('uid='.ldap_escape($evil, '', LDAP_ESCAPE_DN).',ou=users,dc=example,dc=com', $dn); + $this->assertStringNotContainsString('uid=alice,ou=admins,', $dn, 'The comma must not stay structural'); + } + + public function testTheDomainPartIsEscapedToo(): void + { + $dn = $this->buildDn('uid=%U,dc=%d', 'alice@example.org,ou=admins'); + + $this->assertStringNotContainsString('dc=example.org,ou=admins', $dn); + } +} diff --git a/tests/Functional/Service/UtilsTest.php b/tests/Functional/Service/UtilsTest.php new file mode 100644 index 00000000..78e3c760 --- /dev/null +++ b/tests/Functional/Service/UtilsTest.php @@ -0,0 +1,80 @@ +em = static::getContainer()->get(EntityManagerInterface::class); + $this->utils = static::getContainer()->get(Utils::class); + + $this->em->getConnection()->beginTransaction(); + } + + protected function tearDown(): void + { + $this->em->getConnection()->rollBack(); + parent::tearDown(); + } + + public static function usernameProvider(): iterable + { + yield ['alice', true]; + yield ['first.last@example.org', true]; + yield ['a_b-c.d', true]; + yield ["o'brien@example.org", true]; + yield ['', false]; + yield [null, false]; + yield ['bad/user', false]; + yield ['alice bob', false]; + // plus-addressing is common in mail-derived usernames and is URI-safe in a path segment + yield ['alice+tag@example.org', true]; + yield ['éric', false]; + } + + /** + * @dataProvider usernameProvider + */ + public function testIsValidUsername(?string $username, bool $expected): void + { + $this->assertSame($expected, Utils::isValidUsername($username)); + } + + /** + * Auto-created accounts (IMAP/LDAP) go through this too: provisioning a principal from a + * username that cannot live in a principal URI would leave the account authenticated but + * unusable. + */ + public function testCreatingAUserWithAnUnusableUsernameIsRefused(): void + { + $this->expectException(\InvalidArgumentException::class); + + try { + $this->utils->createPasswordlessUserWithDefaultObjects('bad/user', 'Bad', 'bad@example.org'); + } finally { + $this->em->clear(); + $this->assertNull($this->em->getRepository(User::class)->findOneByUsername('bad/user')); + } + } + + public function testCreatingAUserWithAValidUsernameWorks(): void + { + $this->utils->createPasswordlessUserWithDefaultObjects('new.user@example.org', 'New User', 'new@example.org'); + $this->em->flush(); + + $this->assertNotNull($this->em->getRepository(User::class)->findOneByUsername('new.user@example.org')); + } +} diff --git a/translations/validators.de.xlf b/translations/validators.de.xlf index b7c3f5bf..a7a9a21c 100644 --- a/translations/validators.de.xlf +++ b/translations/validators.de.xlf @@ -385,6 +385,10 @@ form.uri.unique Diese URI wird bereits für diesen Auftraggeber verwendet. Bitte wählen Sie einen anderen. + + form.username.invalid + Ein Benutzername darf nur Buchstaben, Ziffern und die Zeichen "_", ".", "@", "+", "'" und "-" enthalten. + diff --git a/translations/validators.en.xlf b/translations/validators.en.xlf index 37881680..8c9ffc2b 100644 --- a/translations/validators.en.xlf +++ b/translations/validators.en.xlf @@ -385,6 +385,10 @@ form.uri.unique This URI is already used with this principal. Please choose another one. + + form.username.invalid + A username may only contain letters, digits and the characters "_", ".", "@", "+", "'" and "-". + diff --git a/translations/validators.fr.xlf b/translations/validators.fr.xlf index f6ad0fa3..e5a903f9 100644 --- a/translations/validators.fr.xlf +++ b/translations/validators.fr.xlf @@ -558,6 +558,10 @@ form.uri.unique Cette URI est déjà utilisée avec ce principal. Veuillez en choisir une autre. + + form.username.invalid + Un nom d'utilisateur ne peut contenir que des lettres, des chiffres et les caractères "_", ".", "@", "+", "'" et "-". + \ No newline at end of file From 7b681fe3472d2e9be9977a8db6a1e1b94c245887 Mon Sep 17 00:00:00 2001 From: tchapi Date: Thu, 10 Sep 2026 22:20:05 +0200 Subject: [PATCH 4/5] ldap --- src/Services/AbstractAuth.php | 50 ++++++++++++++-- src/Services/LDAPAuth.php | 42 ++++++++++++++ tests/Functional/Service/AuthBackendTest.php | 61 ++++++++++++++++++++ 3 files changed, 149 insertions(+), 4 deletions(-) diff --git a/src/Services/AbstractAuth.php b/src/Services/AbstractAuth.php index 4742cd0e..0f4e38e2 100644 --- a/src/Services/AbstractAuth.php +++ b/src/Services/AbstractAuth.php @@ -3,6 +3,8 @@ namespace App\Services; use Sabre\DAV\Auth\Backend\AbstractBasic; +use Sabre\HTTP\RequestInterface; +use Sabre\HTTP\ResponseInterface; /** * Common base for the HTTP Basic authentication backends (internal, IMAP, LDAP). @@ -22,20 +24,24 @@ */ abstract class AbstractAuth extends AbstractBasic { + /** + * The username as the backend spells it, when that differs from what the client sent. + */ + private ?string $canonicalUsername = null; + /** * @param string $username * @param string $password */ final protected function validateUserPass($username, $password): bool { + $this->canonicalUsername = null; + if (!is_string($username) || !is_string($password) || '' === $username || '' === $password) { return false; } - // Anything that would change the shape of `principals/`: - // [/\\] a forward or back slash, which would add a path segment - // [\x00-\x20\x7f] any control character, plus space (0x20) and DEL (0x7f) - if (1 === preg_match('~[/\\\\]|[\\x00-\\x20\\x7f]~', $username)) { + if (self::breaksPrincipalUri($username)) { return false; } @@ -46,4 +52,40 @@ final protected function validateUserPass($username, $password): bool * Validates a non-empty username and password against the backend. */ abstract protected function checkCredentials(string $username, string $password): bool; + + /** + * Backends call this when the directory spells the username differently from what the + * client sent — LDAP matches `ALICE` against `uid=alice` quite happily. The principal is + * then built from that spelling instead, so the login, the account and the principal URI + * cannot drift apart and produce a second, empty account. + */ + protected function setCanonicalUsername(string $username): void + { + // It ends up in a principal URI like any other username + if ('' !== $username && !self::breaksPrincipalUri($username)) { + $this->canonicalUsername = $username; + } + } + + /** + * @return array{0: bool, 1: string} + */ + public function check(RequestInterface $request, ResponseInterface $response) + { + $result = parent::check($request, $response); + + if (true === $result[0] && null !== $this->canonicalUsername) { + return [true, $this->principalPrefix.$this->canonicalUsername]; + } + + return $result; + } + + private static function breaksPrincipalUri(string $username): bool + { + // Anything that would change the shape of `principals/`: + // [/\\] a forward or back slash, which would add a path segment + // [\x00-\x20\x7f] any control character, plus space (0x20) and DEL (0x7f) + return 1 === preg_match('~[/\\\\]|[\\x00-\\x20\\x7f]~', $username); + } } diff --git a/src/Services/LDAPAuth.php b/src/Services/LDAPAuth.php index 4381f83c..628d0c2d 100644 --- a/src/Services/LDAPAuth.php +++ b/src/Services/LDAPAuth.php @@ -82,6 +82,34 @@ public function __construct(ManagerRegistry $doctrine, Utils $utils, string $LDA $this->utils = $utils; } + /** + * Returns the username as the directory spells it, or null when it cannot be determined. + */ + private function canonicalUsernameFor($ldap, string $dn): ?string + { + try { + $read = ldap_read($ldap, $dn, '(objectclass=*)', ['dn']); + } catch (\Exception $e) { + $read = false; + } + + if (false === $read) { + return null; + } + + $entries = ldap_get_entries($ldap, $read); + $matchedDn = $entries[0]['dn'] ?? null; + + if (!is_string($matchedDn) || '' === $matchedDn) { + return null; + } + + // With the second argument set, only the values are returned, not the attribute names + $rdns = ldap_explode_dn($matchedDn, 1); + + return (false !== $rdns && isset($rdns[0]) && '' !== $rdns[0]) ? $rdns[0] : null; + } + /** * Builds the bind DN for a username by filling the placeholders of LDAP_DN_PATTERN. * @@ -185,6 +213,20 @@ protected function ldapOpen($username, $password) error_log('LDAP Error (ldap_bind to '.$this->LDAPAuthUrl.'): '.ldap_error($ldap).' ('.ldap_errno($ldap).')'); } + if ($success) { + // Directories match names case-insensitively, so `ALICE` binds against `uid=alice` + // just as well as `alice` does. Take the spelling the server actually matched: + // read the entry back and use the value of the first RDN of the DN it returns. + // Deriving it from the DN rather than from a fixed attribute keeps this working + // whatever LDAP_DN_PATTERN is built on (uid, cn, sAMAccountName, mail...). + $canonical = $this->canonicalUsernameFor($ldap, $dn); + + if (null !== $canonical) { + $this->setCanonicalUsername($canonical); + $username = $canonical; + } + } + if ($success && $this->autoCreate) { $user = $this->doctrine->getRepository(User::class)->findOneBy(['username' => $username]); diff --git a/tests/Functional/Service/AuthBackendTest.php b/tests/Functional/Service/AuthBackendTest.php index da0fe0f4..79e3c736 100644 --- a/tests/Functional/Service/AuthBackendTest.php +++ b/tests/Functional/Service/AuthBackendTest.php @@ -130,4 +130,65 @@ public function testAnUnusualButStructurallySoundUsernameStillAuthenticates(): v $this->assertTrue($ok); $this->assertSame('principals/first.last+tag@example.org', $principal); } + + /** + * A backend may report the username as the directory spells it; the principal must then be + * built from that, otherwise a case-variant login lands on a principal that does not exist. + */ + public function testACanonicalUsernameBecomesThePrincipal(): void + { + $backend = new class extends AbstractAuth { + protected function checkCredentials(string $username, string $password): bool + { + $this->setCanonicalUsername(strtolower($username)); + + return true; + } + }; + + [$ok, $principal] = self::check($backend, 'ALICE:password'); + + $this->assertTrue($ok); + $this->assertSame('principals/alice', $principal); + } + + public function testACanonicalUsernameThatWouldBreakThePrincipalUriIsIgnored(): void + { + $backend = new class extends AbstractAuth { + protected function checkCredentials(string $username, string $password): bool + { + $this->setCanonicalUsername('some/other/path'); + + return true; + } + }; + + [$ok, $principal] = self::check($backend, 'alice:password'); + + $this->assertTrue($ok); + $this->assertSame('principals/alice', $principal, 'It must fall back to the name the client sent'); + } + + public function testTheCanonicalUsernameDoesNotLeakBetweenAttempts(): void + { + $backend = new class extends AbstractAuth { + public bool $canonicalise = true; + + protected function checkCredentials(string $username, string $password): bool + { + if ($this->canonicalise) { + $this->setCanonicalUsername('canonical'); + } + + return true; + } + }; + + [, $first] = self::check($backend, 'ALICE:password'); + $this->assertSame('principals/canonical', $first); + + $backend->canonicalise = false; + [, $second] = self::check($backend, 'BOB:password'); + $this->assertSame('principals/BOB', $second, 'A later attempt must not reuse the previous canonical name'); + } } From 80b3fd5e9a8d72913e0bc5d2c315bd90b6cf9adf Mon Sep 17 00:00:00 2001 From: tchapi Date: Thu, 10 Sep 2026 22:26:11 +0200 Subject: [PATCH 5/5] ci --- .github/workflows/ci.yml | 8 +++++--- tests/Functional/Service/LDAPAuthTest.php | 8 ++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ccbaa75f..6a2a3c7c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -112,13 +112,15 @@ jobs: - name: Checkout uses: actions/checkout@v4 - - name: Install MySQL / GD / ZIP PHP extensions + - name: Install MySQL / GD / ZIP / LDAP PHP extensions run: | - apk add $PHPIZE_DEPS icu-libs icu-dev libpng-dev libzip-dev + apk add $PHPIZE_DEPS icu-libs icu-dev libpng-dev libzip-dev openldap-dev docker-php-ext-configure intl docker-php-ext-configure gd docker-php-ext-configure zip - docker-php-ext-install pdo pdo_mysql intl gd zip + # ext-ldap is optional for Davis (only AUTH_METHOD=LDAP needs it) but the LDAP tests + # skip themselves without it, and we would rather run them + docker-php-ext-install pdo pdo_mysql intl gd zip ldap - name: Install Composer run: wget -qO - https://getcomposer.org/installer | php -- --install-dir=/usr/local/bin --filename=composer --quiet diff --git a/tests/Functional/Service/LDAPAuthTest.php b/tests/Functional/Service/LDAPAuthTest.php index 2f06d185..bd213c23 100644 --- a/tests/Functional/Service/LDAPAuthTest.php +++ b/tests/Functional/Service/LDAPAuthTest.php @@ -11,6 +11,14 @@ class LDAPAuthTest extends KernelTestCase { + protected function setUp(): void + { + // ext-ldap is optional for Davis: it is only needed with AUTH_METHOD=LDAP + if (!function_exists('ldap_escape')) { + $this->markTestSkipped('The LDAP extension is not loaded'); + } + } + private function buildDn(string $pattern, string $username): string { self::bootKernel();