Webdav public dir writable behavior

.env

@@ -85,6 +85,11 @@ INVITE_FROM_ADDRESS=no-reply@example.org
 # USE ABSOLUTE PATHS for better predictability
 WEBDAV_TMP_DIR='/webdav/tmp'
 WEBDAV_PUBLIC_DIR='/webdav/public'
+# The public directory is readable by every authenticated user. By default only admins
+# (users flagged as such in the dashboard) can create, modify or delete files in it.
+# Set this to true to let every authenticated user write to it (shared drop folder),
+# which was the behaviour of Davis 5.4 and earlier.
+WEBDAV_PUBLIC_DIR_WRITABLE=false
 # By default, home directories are disabled totally (env var set to an empty string).
 # If needed, it is recommended to use a folder that is NOT a child of the public dir,
 # such as /webdav/homes for instance, so that users cannot access other users' homes.

README.md

@@ -209,16 +209,29 @@ BIRTHDAY_REMINDER_OFFSET=false
 ```shell
 WEBDAV_TMP_DIR=/webdav/tmp
 WEBDAV_PUBLIC_DIR=/webdav/public
+WEBDAV_PUBLIC_DIR_WRITABLE=false
 WEBDAV_HOMES_DIR=
 ```
 
+> [!NOTE]
+>
+> The public directory (served at `/dav/public`) is readable by every authenticated user. By default only users flagged as admins in the dashboard can create, modify or delete files in it; set `WEBDAV_PUBLIC_DIR_WRITABLE=true` to let every authenticated user write to it. The Diagnostics page of the dashboard shows which of the two applies.
+
+> [!IMPORTANT]
+>
+> Up to Davis 5.4 included, every authenticated user could write to the public directory. If you relied on that (a shared drop folder), set `WEBDAV_PUBLIC_DIR_WRITABLE=true` when upgrading, otherwise your users will get a `403` when saving files there.
+
+> [!NOTE]
+>
+> The directories must be absolute paths and must not live inside the web root (Davis refuses to start the DAV server otherwise). The tmp dir and the homes dir must not be inside the public dir either, and vice versa.
+
 > [!NOTE]
 >
 > In a docker setup, I recommend setting `WEBDAV_TMP_DIR` to `/tmp`.
 
 > [!NOTE]
 >
-> By default, home directories are disabled totally (the env var is set to an empty string). If needed, it is recommended to use a folder that is **NOT** a child of the public dir, such as `/webdav/homes` for instance, so that users cannot access other users' homes.
+> By default, home directories are disabled totally (the env var is set to an empty string). If needed, use a folder that is **NOT** a child of the public dir, such as `/webdav/homes` for instance, so that users cannot access other users' homes: Davis checks this and refuses to start the DAV server otherwise.
 
 **h. The log file path**
 

config/services.yaml

@@ -13,6 +13,9 @@ parameters:
     default_birthday_reminder_offset: "PT9H"
     caldav_enabled: "%env(bool:CALDAV_ENABLED)%"
     carddav_enabled: "%env(bool:CARDDAV_ENABLED)%"
+    # `bool:` must wrap `default:` so that the fallback string is cast too
+    webdav_public_dir_writable: "%env(bool:default:default_webdav_public_dir_writable:WEBDAV_PUBLIC_DIR_WRITABLE)%"
+    default_webdav_public_dir_writable: "false"
 
 services:
     # default configuration for services in *this* file
@@ -42,6 +45,7 @@ services:
             $calDAVEnabled: "%env(bool:CALDAV_ENABLED)%"
             $cardDAVEnabled: "%env(bool:CARDDAV_ENABLED)%"
             $webDAVEnabled: "%env(bool:WEBDAV_ENABLED)%"
+            $webdavPublicDirWritable: "%webdav_public_dir_writable%"
             $inviteAddress: "%env(INVITE_FROM_ADDRESS)%"
             $mailerDsn: "%env(MAILER_DSN)%"
 
@@ -83,6 +87,7 @@ services:
             $webdavPublicDir: "%env(resolve:WEBDAV_PUBLIC_DIR)%"
             $webdavHomesDir: "%env(resolve:WEBDAV_HOMES_DIR)%"
             $webdavTmpDir: "%env(resolve:WEBDAV_TMP_DIR)%"
+            $webdavPublicDirWritable: "%webdav_public_dir_writable%"
 
     App\Security\LoginFormAuthenticator:
         arguments:

docker/.env

@@ -50,6 +50,7 @@ LDAP_CERTIFICATE_CHECKING_STRATEGY=try
 # WebDAV settings
 WEBDAV_TMP_DIR=/webdav/tmp
 WEBDAV_PUBLIC_DIR=/webdav/public
+WEBDAV_PUBLIC_DIR_WRITABLE=false
 WEBDAV_HOMES_DIR=
 
 # Mail settings

src/Controller/DAVController.php

@@ -6,6 +6,7 @@
 use App\Entity\User;
 use App\Plugins\BirthdayCalendarPlugin;
 use App\Plugins\DavisIMipPlugin;
+use App\Plugins\DavisTemporaryFileFilterPlugin;
 use App\Plugins\PublicAwareDAVACLPlugin;
 use App\Services\BasicAuth;
 use App\Services\BirthdayService;
@@ -15,6 +16,7 @@
 use PDO;
 use Psr\Log\LoggerInterface;
 use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
+use Symfony\Component\Filesystem\Path;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpKernel\Profiler\Profiler;
@@ -92,6 +94,14 @@ class DAVController extends AbstractController
      */
     protected $webdavTmpDir;
 
+    /**
+     * Can every authenticated user write to the WebDAV public directory
+     * (otherwise only admins can, everybody can read).
+     *
+     * @var bool
+     */
+    protected $webdavPublicDirWritable;
+
     /**
      * @var EntityManagerInterface
      */
@@ -149,7 +159,7 @@ class DAVController extends AbstractController
      */
     protected $server;
 
-    public function __construct(MailerInterface $mailer, BasicAuth $basicAuthBackend, IMAPAuth $IMAPAuthBackend, LDAPAuth $LDAPAuthBackend, UrlGeneratorInterface $router, EntityManagerInterface $entityManager, LoggerInterface $logger, BirthdayService $birthdayService, string $publicDir, bool $calDAVEnabled = true, bool $cardDAVEnabled = true, bool $webDAVEnabled = false, bool $publicCalendarsEnabled = true, ?string $inviteAddress = null, ?string $authMethod = null, ?string $authRealm = null, ?string $webdavPublicDir = null, ?string $webdavHomesDir = null, ?string $webdavTmpDir = null)
+    public function __construct(MailerInterface $mailer, BasicAuth $basicAuthBackend, IMAPAuth $IMAPAuthBackend, LDAPAuth $LDAPAuthBackend, UrlGeneratorInterface $router, EntityManagerInterface $entityManager, LoggerInterface $logger, BirthdayService $birthdayService, string $publicDir, bool $calDAVEnabled = true, bool $cardDAVEnabled = true, bool $webDAVEnabled = false, bool $publicCalendarsEnabled = true, ?string $inviteAddress = null, ?string $authMethod = null, ?string $authRealm = null, ?string $webdavPublicDir = null, ?string $webdavHomesDir = null, ?string $webdavTmpDir = null, bool $webdavPublicDirWritable = false)
     {
         $this->publicDir = $publicDir;
 
@@ -162,6 +172,7 @@ public function __construct(MailerInterface $mailer, BasicAuth $basicAuthBackend
         $this->webdavPublicDir = $webdavPublicDir;
         $this->webdavHomesDir = $webdavHomesDir;
         $this->webdavTmpDir = $webdavTmpDir;
+        $this->webdavPublicDirWritable = $webdavPublicDirWritable;
 
         $this->em = $entityManager;
         $this->logger = $logger;
@@ -227,6 +238,7 @@ private function initServer(string $authMethod, string $authRealm = User::DEFAUL
         ];
 
         if ($this->webdavHomesDir) {
+            $this->assertWebdavDirectory($this->webdavHomesDir, 'WEBDAV_HOMES_DIR');
             $nodes[] = new \Sabre\DAVACL\FS\HomeCollection($principalBackend, $this->webdavHomesDir);
         }
 
@@ -239,7 +251,26 @@ private function initServer(string $authMethod, string $authRealm = User::DEFAUL
             $nodes[] = new \Sabre\CardDAV\AddressBookRoot($principalBackend, $carddavBackend);
         }
         if ($this->webDAVEnabled && $this->webdavTmpDir && $this->webdavPublicDir) {
-            $nodes[] = new \Sabre\DAV\FS\Directory($this->webdavPublicDir);
+            $this->assertWebdavDirectory($this->webdavTmpDir, 'WEBDAV_TMP_DIR');
+            $this->assertWebdavDirectory($this->webdavPublicDir, 'WEBDAV_PUBLIC_DIR');
+            // Temporary files and the locks database would be served as regular files if the tmp
+            // dir lived inside the public one, and users could browse other users' homes if the
+            // homes dir did.
+            $this->assertWebdavDirectoriesAreDisjoint($this->webdavPublicDir, 'WEBDAV_PUBLIC_DIR', $this->webdavTmpDir, 'WEBDAV_TMP_DIR');
+            if ($this->webdavHomesDir) {
+                $this->assertWebdavDirectoriesAreDisjoint($this->webdavPublicDir, 'WEBDAV_PUBLIC_DIR', $this->webdavHomesDir, 'WEBDAV_HOMES_DIR');
+            }
+
+            // Explicit ACL for the shared directory: every authenticated user can read it, and
+            // writing is reserved to admins (the ACL plugin grants them every privilege) unless
+            // WEBDAV_PUBLIC_DIR_WRITABLE opens it to everyone. Children inherit this ACL.
+            $publicDirAcl = [
+                ['principal' => '{DAV:}authenticated', 'privilege' => '{DAV:}read', 'protected' => true],
+            ];
+            if ($this->webdavPublicDirWritable) {
+                $publicDirAcl[] = ['principal' => '{DAV:}authenticated', 'privilege' => '{DAV:}write', 'protected' => true];
+            }
+            $nodes[] = new \Sabre\DAVACL\FS\Collection($this->webdavPublicDir, $publicDirAcl);
         }
 
         // The object tree needs in turn to be passed to the server class
@@ -296,13 +327,50 @@ private function initServer(string $authMethod, string $authRealm = User::DEFAUL
 
         // WebDAV plugins
         if ($this->webDAVEnabled && $this->webdavTmpDir && $this->webdavPublicDir) {
-            if (!is_dir($this->webdavTmpDir) || !is_dir($this->webdavPublicDir)) {
-                throw new \Exception('The WebDAV temp dir and/or public dir are not available. Make sure they are created with the correct permissions.');
-            }
             $lockBackend = new \Sabre\DAV\Locks\Backend\File($this->webdavTmpDir.'/locksdb');
             $this->server->addPlugin(new \Sabre\DAV\Locks\Plugin($lockBackend));
             $this->server->addPlugin(new \Sabre\DAV\Browser\GuessContentType());
-            $this->server->addPlugin(new \Sabre\DAV\TemporaryFileFilterPlugin($this->webdavTmpDir));
+            // Temporary files must obey the ACL of their directory (see the plugin for the why)
+            $this->server->addPlugin(new DavisTemporaryFileFilterPlugin($this->webdavTmpDir));
+        }
+    }
+
+    /**
+     * A WebDAV directory must exist, be given as an absolute path (a relative one would be
+     * resolved against the PHP process' working directory, which is not predictable) and
+     * must not live inside the web root, where the web server would serve its content
+     * directly and bypass every DAV permission check.
+     */
+    private function assertWebdavDirectory(string $dir, string $envVar): string
+    {
+        if (!Path::isAbsolute($dir)) {
+            throw new \RuntimeException(sprintf('%s must be an absolute path, "%s" given.', $envVar, $dir));
+        }
+
+        $realDir = realpath($dir);
+        if (false === $realDir || !is_dir($realDir)) {
+            throw new \RuntimeException(sprintf('%s points to "%s", which does not exist or is not a directory. Make sure it is created with the correct permissions.', $envVar, $dir));
+        }
+
+        $webRoot = realpath($this->publicDir);
+        if (false !== $webRoot && Path::isBasePath($webRoot, $realDir)) {
+            throw new \RuntimeException(sprintf('%s ("%s") must not be inside the web root ("%s"): the web server would serve these files without any permission check.', $envVar, $dir, $webRoot));
+        }
+
+        return $realDir;
+    }
+
+    /**
+     * Neither directory may be the other one or live inside it. Both must already have
+     * passed assertWebdavDirectory(), so they exist and realpath() resolves them.
+     */
+    private function assertWebdavDirectoriesAreDisjoint(string $dirA, string $envVarA, string $dirB, string $envVarB): void
+    {
+        $realA = realpath($dirA);
+        $realB = realpath($dirB);
+
+        if (Path::isBasePath($realA, $realB) || Path::isBasePath($realB, $realA)) {
+            throw new \RuntimeException(sprintf('%s ("%s") and %s ("%s") must be separate directories, one must not be inside the other.', $envVarA, $dirA, $envVarB, $dirB));
         }
     }
 
@@ -374,38 +442,38 @@ public function dav(Request $request, ?string $path, ?Profiler $profiler = null)
             return $response;
         }
 
-        // \Sabre\DAV\Server does not let us use a custom SAPI, and its behaviour
-        // is to directly output headers and content to php://output. Hence, we
-        // let the headers pass (we have not choice) and capture the output in a
-        // buffer.
-        // This allows us to use a Response, and not to break the events triggered
-        // by Symfony after the response is sent, like for instance the TERMINATE
-        // event from the Kernel, that is used to send emails...
-
+        // \Sabre\DAV\Server does not let us use a custom SAPI: it writes its status line and
+        // headers with header() and streams the body to php://output. We capture the output
+        // so that we can hand a proper Response back to Symfony (and keep its kernel events,
+        // like TERMINATE, working).
         ob_start(); // Does not capture headers!
         $this->server->start();
-
-        $output = ob_get_contents();
-        ob_end_clean();
-
-        // As previously said, headers are already _prepared_ by the server,
-        // so we can't modify them or remove them. But they are not _sent_ yet,
-        // so headers_sent() is false, and Symfony will add its own headers above it.
-        //
-        // The Content-type header is the problem, since Symfony will
-        // output `text/html` for everything since it doesn't know any better.
-        // Thus, we have to get the _real_ Content-type header already prepared,
-        // and force it in the Symfony Response.
-        //
-        // That's what we do here.
-        $response = new Response($output, http_response_code(), []);
-        foreach (headers_list() as $header) {
-            if ('content-type:' === strtolower(substr($header, 0, 13))) {
-                $headerArray = explode(':', $header);
-                $response->headers->set('Content-type', $headerArray[1]);
+        $output = ob_get_clean();
+
+        // Some plugins short-circuit a request by returning false from `beforeMethod` (the
+        // temporary file filter does, for .DS_Store and friends). sabre then never sends
+        // anything: status, headers and body only exist in its response object. So we always
+        // rebuild the Symfony response from that object, falling back to its body when
+        // nothing was streamed.
+        $sabreResponse = $this->server->httpResponse;
+        if ('' === $output) {
+            $body = $sabreResponse->getBody();
+            // A stream that sabre already sent has been closed (is_resource() is then false):
+            // only read bodies that were never streamed.
+            if (is_string($body) || (is_resource($body) && 'stream' === get_resource_type($body))) {
+                $output = $sabreResponse->getBodyAsString();
             }
         }
 
+        // Drop the headers sabre may already have queued with header(): Symfony re-sends the
+        // very same ones from the Response below, and would otherwise duplicate them.
+        header_remove();
+
+        $response = new Response($output, $sabreResponse->getStatus());
+        foreach ($sabreResponse->getHeaders() as $name => $values) {
+            $response->headers->set($name, $values);
+        }
+
         return $response;
     }
 }

src/Plugins/DavisTemporaryFileFilterPlugin.php

@@ -0,0 +1,59 @@
+<?php
+
+namespace App\Plugins;
+
+use Sabre\DAV\TemporaryFileFilterPlugin;
+use Sabre\DAVACL\Plugin as AclPlugin;
+use Sabre\HTTP\RequestInterface;
+use Sabre\HTTP\ResponseInterface;
+use Sabre\Uri;
+
+/**
+ * sabre/dav's TemporaryFileFilterPlugin intercepts the junk files desktop clients
+ * write next to real files (.DS_Store, Thumbs.db, ._*, *.swp, ...) and stores them
+ * outside the DAV tree. Because those files never exist in the tree, the ACL plugin
+ * never checks anything for them: without this subclass, anybody (authenticated or
+ * not) could store, read and delete such files under any path.
+ *
+ * We make temporary files obey the privileges of the directory they would live in,
+ * exactly like a real file would.
+ */
+final class DavisTemporaryFileFilterPlugin extends TemporaryFileFilterPlugin
+{
+    /**
+     * Same contract as the parent: false stops the request (the plugin answered it), null
+     * lets the regular handlers run.
+     *
+     * @return bool|null
+     */
+    public function beforeMethod(RequestInterface $request, ResponseInterface $response)
+    {
+        $path = $request->getPath();
+        if (false === $this->isTempFile($path)) {
+            return;
+        }
+
+        // This is a permission check: it must fail closed, never be skipped silently.
+        $acl = $this->server->getPlugin('acl');
+        if (!$acl instanceof AclPlugin) {
+            throw new \LogicException('The ACL plugin must be registered for '.self::class.' to work: temporary files would otherwise bypass every permission check.');
+        }
+
+        [$parent] = Uri\split($path);
+
+        // Temporary files are not nodes of the tree, so the finer-grained checks the ACL
+        // plugin does on real files (write-content on an existing file for PUT, for
+        // instance) cannot apply. We check the privilege on the parent directory that the
+        // matching operation on a real file would require.
+        $privilege = match ($request->getMethod()) {
+            'PUT' => '{DAV:}bind',
+            'DELETE' => '{DAV:}unbind',
+            default => '{DAV:}read',
+        };
+
+        // Throws NotAuthenticated (401) for anonymous users and NeedPrivileges (403) otherwise
+        $acl->checkPrivileges($parent ?? '', $privilege);
+
+        return parent::beforeMethod($request, $response);
+    }
+}

src/Services/Diagnostics.php

@@ -35,6 +35,7 @@ public function __construct(
         private bool $calDAVEnabled,
         private bool $cardDAVEnabled,
         private bool $webDAVEnabled,
+        private bool $webdavPublicDirWritable,
         private ?string $inviteAddress,
         private ?string $mailerDsn,
     ) {
@@ -66,10 +67,11 @@ public function buckets(): array
                 $this->mailer(),
                 $this->accountsWithoutEmail(),
             ],
-            'diagnostics.bucket.endpoints' => [
+            'diagnostics.bucket.endpoints' => array_values(array_filter([
                 $this->davEndpoint(),
                 $this->protocols(),
-            ],
+                $this->webdavPublicDir(),
+            ])),
         ];
 
         $result = [];
@@ -323,4 +325,21 @@ private function protocols(): array
 
         return $this->check(self::OK, 'diagnostics.protocols', implode(' · ', $enabled));
     }
+
+    /**
+     * Who can write to the shared WebDAV directory. A regular user getting 403 when saving a
+     * file there is the expected default, and this is the place that says so.
+     */
+    private function webdavPublicDir(): ?array
+    {
+        if (!$this->webDAVEnabled) {
+            return null;
+        }
+
+        if ($this->webdavPublicDirWritable) {
+            return $this->check(self::INFO, 'diagnostics.webdav_public_dir', 'diagnostics.webdav_public_dir.everyone', 'diagnostics.webdav_public_dir.everyone.hint');
+        }
+
+        return $this->check(self::INFO, 'diagnostics.webdav_public_dir', 'diagnostics.webdav_public_dir.admins', 'diagnostics.webdav_public_dir.admins.hint');
+    }
 }