fix: The WebDAV response element must only contain propstat OR `sta…

…tus` element(s) (#1488) Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
sabre-io · Nov 8, 2023 · 2dd0244 · 2dd0244
1 parent a74464d
commit 2dd0244
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 25 deletions.
diff --git a/lib/DAV/Xml/Element/Response.php b/lib/DAV/Xml/Element/Response.php
@@ -40,7 +40,7 @@ class Response implements Element
      *
      * This is currently only used in WebDAV-Sync
      *
-     * @var string
+     * @var string|null
      */
     protected $httpStatus;
 
@@ -112,13 +112,21 @@ public function getResponseProperties()
      */
     public function xmlSerialize(Writer $writer)
     {
-        if ($status = $this->getHTTPStatus()) {
-            $writer->writeElement('{DAV:}status', 'HTTP/1.1 '.$status.' '.\Sabre\HTTP\Response::$statusCodes[$status]);
-        }
+        /*
+         * Accordingly to the RFC the element looks like:
+         * <!ELEMENT response (href, ((href*, status)|(propstat+)), error?, responsedescription? , location?) >
+         *
+         * So the response
+         *   - MUST contain a href and
+         *   - EITHER a status and additional href(s)
+         *     OR one or more propstat(s)
+         */
         $writer->writeElement('{DAV:}href', $writer->contextUri.\Sabre\HTTP\encodePath($this->getHref()));
 
         $empty = true;
+        $httpStatus = $this->getHTTPStatus();
 
+        // Add propstat elements
         foreach ($this->getResponseProperties() as $status => $properties) {
             // Skipping empty lists
             if (!$properties || (!is_int($status) && !ctype_digit($status))) {
@@ -130,19 +138,25 @@ public function xmlSerialize(Writer $writer)
             $writer->writeElement('{DAV:}status', 'HTTP/1.1 '.$status.' '.\Sabre\HTTP\Response::$statusCodes[$status]);
             $writer->endElement(); // {DAV:}propstat
         }
+
+        // The WebDAV spec only allows the status element on responses _without_ a propstat
         if ($empty) {
-            /*
-             * The WebDAV spec _requires_ at least one DAV:propstat to appear for
-             * every DAV:response. In some circumstances however, there are no
-             * properties to encode.
-             *
-             * In those cases we MUST specify at least one DAV:propstat anyway, with
-             * no properties.
-             */
-            $writer->writeElement('{DAV:}propstat', [
-                '{DAV:}prop' => [],
-                '{DAV:}status' => 'HTTP/1.1 418 '.\Sabre\HTTP\Response::$statusCodes[418],
-            ]);
+            if (null !== $httpStatus) {
+                $writer->writeElement('{DAV:}status', 'HTTP/1.1 '.$httpStatus.' '.\Sabre\HTTP\Response::$statusCodes[$httpStatus]);
+            } else {
+                /*
+                * The WebDAV spec _requires_ at least one DAV:propstat to appear for
+                * every DAV:response if there is no status.
+                * In some circumstances however, there are no properties to encode.
+                *
+                * In those cases we MUST specify at least one DAV:propstat anyway, with
+                * no properties.
+                */
+                $writer->writeElement('{DAV:}propstat', [
+                    '{DAV:}prop' => [],
+                    '{DAV:}status' => 'HTTP/1.1 418 '.\Sabre\HTTP\Response::$statusCodes[418],
+                ]);
+            }
         }
     }
 

diff --git a/tests/Sabre/DAV/Xml/Element/ResponseTest.php b/tests/Sabre/DAV/Xml/Element/ResponseTest.php
@@ -241,8 +241,8 @@ public function testSerializeUrlencoding()
      * @depends testSerialize
      *
      * The WebDAV spec _requires_ at least one DAV:propstat to appear for
-     * every DAV:response. In some circumstances however, there are no
-     * properties to encode.
+     * every DAV:response if there is no status.
+     * In some circumstances however, there are no properties to encode.
      *
      * In those cases we MUST specify at least one DAV:propstat anyway, with
      * no properties.
@@ -268,6 +268,65 @@ public function testSerializeNoProperties()
 ', $xml);
     }
 
+    /**
+     * @depends testSerialize
+     *
+     * The WebDAV spec _requires_ at least one DAV:propstat _OR_ a status to appear for
+     * every DAV:response.
+     * So if there are no properties but a status, the response should contain that status.
+     */
+    public function testSerializeNoPropertiesButStatus()
+    {
+        $innerProps = [];
+
+        $property = new Response('uri', $innerProps, 200);
+        $xml = $this->write(['{DAV:}root' => ['{DAV:}response' => $property]]);
+
+        self::assertXmlStringEqualsXmlString(
+'<?xml version="1.0"?>
+<d:root xmlns:d="DAV:">
+  <d:response>
+      <d:href>/uri</d:href>
+      <d:status>HTTP/1.1 200 OK</d:status>
+  </d:response>
+</d:root>
+', $xml);
+    }
+
+    /**
+     * @depends testSerialize
+     *
+     * The WebDAV standard only allow EITHER propstat(s) OR a status within the response.
+     * Make sure that if there are propstat(s), no status element is added.
+     */
+    public function testSerializePropertiesAndStatus()
+    {
+        $innerProps = [
+            200 => [
+                '{DAV:}displayname' => 'my file',
+            ],
+        ];
+
+        $property = new Response('uri', $innerProps, 200);
+
+        $xml = $this->write(['{DAV:}root' => ['{DAV:}response' => $property]]);
+
+        self::assertXmlStringEqualsXmlString(
+'<?xml version="1.0"?>
+<d:root xmlns:d="DAV:">
+  <d:response>
+    <d:href>/uri</d:href>
+    <d:propstat>
+      <d:prop>
+        <d:displayname>my file</d:displayname>
+      </d:prop>
+      <d:status>HTTP/1.1 200 OK</d:status>
+    </d:propstat>
+  </d:response>
+</d:root>
+', $xml);
+    }
+
     /**
      * In the case of {DAV:}prop, a deserializer should never get called, if
      * the property element is empty.

diff --git a/tests/Sabre/DAVACL/PrincipalMatchTest.php b/tests/Sabre/DAVACL/PrincipalMatchTest.php
@@ -28,12 +28,8 @@ public function testPrincipalMatch()
         $expected = <<<XML
 <?xml version="1.0"?>
 <d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
-    <d:status>HTTP/1.1 200 OK</d:status>
     <d:href>/principals/user1</d:href>
-    <d:propstat>
-        <d:prop/>
-        <d:status>HTTP/1.1 418 I'm a teapot</d:status>
-    </d:propstat>
+    <d:status>HTTP/1.1 200 OK</d:status>
 </d:multistatus>
 XML;
 
@@ -63,7 +59,6 @@ public function testPrincipalMatchProp()
         $expected = <<<XML
 <?xml version="1.0"?>
 <d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
-    <d:status>HTTP/1.1 200 OK</d:status>
     <d:href>/principals/user1/</d:href>
     <d:propstat>
         <d:prop>
@@ -102,7 +97,6 @@ public function testPrincipalMatchPrincipalProperty()
         $expected = <<<XML
 <?xml version="1.0"?>
 <d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
-    <d:status>HTTP/1.1 200 OK</d:status>
     <d:href>/principals/user1/</d:href>
     <d:propstat>
         <d:prop>