Logout CSRF + tests

config/packages/security.yaml

@@ -21,6 +21,8 @@ security:
             logout:
                 path: app_logout
                 target: dashboard
+                # Without this, any page the admin visits can log them out with a plain GET
+                enable_csrf: true
 
 
     access_control:

src/Controller/DAVController.php

@@ -187,6 +187,11 @@ public function home(): Response
 
     private function initServer(string $authMethod, string $authRealm = User::DEFAULT_AUTH_REALM)
     {
+        // Don't advertise the exact sabre/dav version: it appears in the `X-Sabre-Version`
+        // header, in the `<s:sabredav-version>` element of every error body and in the HTML
+        // browser, which only helps someone matching an installation against known advisories.
+        \Sabre\DAV\Server::$exposeVersion = false;
+
         // Get the PDO Connection of type PDO
         $pdo = $this->em->getConnection()->getNativeConnection();
 

src/Entity/User.php

@@ -20,9 +20,11 @@ class User
 
     /**
      * A username ends up in the principal URI (`principals/<username>`), 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.
+     * anything that would change that path's structure. Letters, digits and `_ . @ + ' -` are
+     * allowed: the punctuation is what shows up in mail-derived login names.
+     *
+     * Only checked in the `creation` validation group, so that an account whose name does not
+     * match — an LDAP or IMAP login, say — stays editable.
      */
     public const USERNAME_PATTERN = '/^[a-zA-Z0-9_.@+\'-]+$/';
 

src/Plugins/DavisIMipPlugin.php

@@ -230,10 +230,11 @@ public function schedule(ITip\Message $itip)
             ->replyTo(new Address($senderEmail, $mailSenderName))
             ->subject($subject);
 
+        // Keep holiday auto-replies from bouncing back at invitations.
+        $message->getHeaders()->addTextHeader('X-Auto-Response-Suppress', 'OOF, DR, RN, NRN, AutoReply');
+
         if (DAV\Server::$exposeVersion) {
-            $message->getHeaders()
-                    ->addTextHeader('X-Sabre-Version: ', DAV\Version::VERSION)
-                    ->addTextHeader('X-Auto-Response-Suppress', 'OOF, DR, RN, NRN, AutoReply');
+            $message->getHeaders()->addTextHeader('X-Sabre-Version', DAV\Version::VERSION);
         }
 
         // Now that we have everything, we can set the message body

templates/_partials/navigation.html.twig

@@ -22,7 +22,7 @@
             👤 {{ app.user.username }}
           </a>
           <div class="dropdown-menu" aria-labelledby="navUserMenu">
-            <a class="dropdown-item" href="{{ path('app_logout') }}">{{ "logout"|trans }}</a>
+            <a class="dropdown-item" href="{{ path('app_logout', {_csrf_token: csrf_token('logout')}) }}">{{ "logout"|trans }}</a>
           </div>
         </li>
         <li class="nav-item dropdown">

templates/security/login.html.twig

@@ -5,7 +5,7 @@
 
 {% if app.user %}
     <div class="mb-3">
-        {{ "login.already"|trans({username: app.user.username}) }}, <a href="{{ path('app_logout') }}">{{ "logout"|trans }}</a>
+        {{ "login.already"|trans({username: app.user.username}) }}, <a href="{{ path('app_logout', {_csrf_token: csrf_token('logout')}) }}">{{ "logout"|trans }}</a>
     </div>
 {% else %}
     <div class="row justify-content-md-center">

tests/Functional/AddressBookDavTest.php

@@ -44,8 +44,7 @@ private function addressBookFor(string $uri): array
     }
 
     /**
-     * Regression test for issue #275: a display name is optional in CardDAV, but the column
-     * was NOT NULL, so an MKCOL without {DAV:}displayname failed with a 500.
+     * A display name is optional in CardDAV: an MKCOL without {DAV:}displayname must succeed.
      */
     public function testAddressBookCanBeCreatedWithoutADisplayName(): void
     {

tests/Functional/CalendarSubscriptionTest.php

@@ -45,9 +45,8 @@ private function subscriptionFor(string $uri): array
     }
 
     /**
-     * Regression test: `calendarorder` had no default, and sabre only lists it in its INSERT
-     * when the client sent {http://apple.com/ns/ical/}calendar-order. Subscribing without one
-     * therefore failed with a NOT NULL violation.
+     * sabre only lists `calendarorder` in its INSERT when the client sent
+     * {http://apple.com/ns/ical/}calendar-order, so the column needs a default.
      */
     public function testSubscriptionCanBeCreatedWithoutACalendarOrder(): void
     {

tests/Functional/Controllers/AddressBookControllerTest.php

@@ -206,4 +206,15 @@ public function testAddressBookWithoutADisplayNameFallsBackToItsUri(): void
         $this->assertResponseIsSuccessful();
         $this->assertAnySelectorTextContains('h5', 'nameless-book');
     }
+
+    public function testAddressBookPagesAreNotReachableAnonymously(): void
+    {
+        $client = static::createClient();
+
+        foreach (['/addressbooks/1', '/addressbooks/1/new', '/addressbooks/1/edit/1'] as $url) {
+            $client->request('GET', $url);
+
+            $this->assertResponseRedirects('/login', null, $url.' must not be public');
+        }
+    }
 }

tests/Functional/Controllers/CalendarControllerTest.php

@@ -313,4 +313,15 @@ public function testCalendarNewIgnoresASubmittedOwner(): void
         $calendarRepository = static::getContainer()->get(CalendarInstanceRepository::class);
         $this->assertNull($calendarRepository->findOneBy(['uri' => 'hijack']));
     }
+
+    public function testCalendarPagesAreNotReachableAnonymously(): void
+    {
+        $client = static::createClient();
+
+        foreach (['/calendars/1', '/calendars/1/new', '/calendars/1/edit/1', '/calendars/1/shares/1'] as $url) {
+            $client->request('GET', $url);
+
+            $this->assertResponseRedirects('/login', null, $url.' must not be public');
+        }
+    }
 }

tests/Functional/Controllers/DashboardTest.php

@@ -2,6 +2,7 @@
 
 namespace App\Tests\Functional;
 
+use App\Security\AdminUser;
 use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
 
 class DashboardTest extends WebTestCase
@@ -91,4 +92,31 @@ public function testLoginCorrect(): void
         $this->assertSelectorTextContains('h3.environment', 'Configured environment');
         $this->assertSelectorExists('nav.navbar');
     }
+
+    /**
+     * Logging out is a state change: a bare `GET /logout` must not end the session.
+     */
+    public function testLogoutRequiresACsrfToken(): void
+    {
+        $client = static::createClient();
+        $client->loginUser(new AdminUser('admin', 'test'));
+
+        $client->request('GET', '/logout');
+        $this->assertResponseStatusCodeSame(403);
+
+        $client->request('GET', '/dashboard');
+        $this->assertResponseIsSuccessful('The session must survive a logout without a token');
+    }
+
+    public function testLogoutWorksFromTheMenuLink(): void
+    {
+        $client = static::createClient();
+        $client->loginUser(new AdminUser('admin', 'test'));
+
+        $crawler = $client->request('GET', '/dashboard');
+        $client->click($crawler->filter('a.dropdown-item')->selectLink('Logout')->link());
+
+        $client->request('GET', '/dashboard');
+        $this->assertResponseRedirects('/login');
+    }
 }

tests/Functional/Controllers/UserControllerTest.php

@@ -253,4 +253,19 @@ public function testUserCreationRejectsAUsernameThatBreaksThePrincipalUri(): voi
             static::getContainer()->get('doctrine.orm.entity_manager')->getRepository(User::class)->findOneByUsername('bad/user')
         );
     }
+
+    /**
+     * Every admin test authenticates first, so a missing `access_control` entry would go
+     * unnoticed — which is exactly how the `^/adressbooks` typo of #268 shipped.
+     */
+    public function testUserPagesAreNotReachableAnonymously(): void
+    {
+        $client = static::createClient();
+
+        foreach (['/users/', '/users/new', '/users/edit/1', '/users/delegates/1'] as $url) {
+            $client->request('GET', $url);
+
+            $this->assertResponseRedirects('/login', null, $url.' must not be public');
+        }
+    }
 }

tests/Functional/DavTest.php

@@ -129,8 +129,8 @@ public function testAnonymousCannotReadCalendarObject(): void
     }
 
     /**
-     * Regression test: the ACL plugin used to skip all privilege checks whenever
-     * `sabreAction=asset` was present in the query string, for any method and any path.
+     * `sabreAction=asset` is only meaningful for the browser plugin's own assets: it must not
+     * waive the privilege checks on an arbitrary path.
      */
     public function testAssetQueryParameterDoesNotBypassReadAcl(): void
     {
@@ -208,8 +208,8 @@ public function testWellKnownUrlsRedirectToTheDavEndpoint(): void
     }
 
     /**
-     * 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.
+     * The advertised methods depend on the node: MKCALENDAR only exists inside a calendar home,
+     * so OPTIONS has to answer for the path it was asked about rather than for the root.
      */
     public function testOptionsDescribesTheRequestedPath(): void
     {
@@ -233,4 +233,18 @@ public function testOptionsOnAnUnresolvablePathStillAnswers(): void
         $this->assertResponseIsSuccessful();
         $this->assertStringContainsString('PROPFIND', (string) $client->getResponse()->headers->get('Allow'));
     }
+
+    /**
+     * The exact sabre/dav version appeared in the `X-Sabre-Version` header, in every error
+     * body and in the HTML browser, which only helps match an install against known advisories.
+     */
+    public function testTheSabreVersionIsNotAdvertised(): void
+    {
+        $client = static::requestDavClient('GET', '/dav/');
+
+        $this->assertResponseStatusCodeSame(401);
+        $this->assertStringNotContainsString('sabredav-version', $client->getResponse()->getContent());
+        $this->assertFalse($client->getResponse()->headers->has('X-Sabre-Version'));
+        $this->assertFalse(\Sabre\DAV\Server::$exposeVersion, 'The DAV server must be built with version exposure off');
+    }
 }

tests/Functional/Service/LDAPAuthTest.php

@@ -63,8 +63,7 @@ public function testDomainComponentsAreAvailableInReverseOrder(): void
     }
 
     /**
-     * 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.
+     * A username carrying DN syntax must stay a value inside the DN, not add structure to it.
      */
     public function testAUsernameCannotInjectDnStructure(): void
     {

tests/Functional/SyncTokenTest.php

@@ -50,10 +50,8 @@ private function createAddressBookAtTokenEleven(): int
     }
 
     /**
-     * Regression test: sync tokens were stored as text, so `synctoken >= 9 AND synctoken < 11`
-     * was compared lexicographically ('10' sorts before '9'). A client syncing across a
-     * decimal-width boundary was told the collection had advanced but received no changes at
-     * all, silently losing contacts.
+     * Sync tokens are compared numerically, so a client syncing across a decimal-width
+     * boundary gets every change in the range. Compared as text, `'10' < '9'` would hide them.
      */
     public function testChangesAcrossADecimalBoundaryAreReported(): void
     {