Logout CSRF + tests (#290)

Author: tchapi

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');
+        }
+    }
 }