From 2dd02447b3e2dd2dd99879363fbe914d612604df Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Wed, 8 Nov 2023 16:13:40 +0100 Subject: [PATCH] fix: The WebDAV response element must only contain `propstat` OR `status` element(s) (#1488) Signed-off-by: Ferdinand Thiessen --- lib/DAV/Xml/Element/Response.php | 46 +++++++++----- tests/Sabre/DAV/Xml/Element/ResponseTest.php | 63 +++++++++++++++++++- tests/Sabre/DAVACL/PrincipalMatchTest.php | 8 +-- 3 files changed, 92 insertions(+), 25 deletions(-) diff --git a/lib/DAV/Xml/Element/Response.php b/lib/DAV/Xml/Element/Response.php index 79f06a09bf..df92914650 100644 --- 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: + * + * + * 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 index 6806af395b..0950acce61 100644 --- 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( +' + + + /uri + HTTP/1.1 200 OK + + +', $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( +' + + + /uri + + + my file + + HTTP/1.1 200 OK + + + +', $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 index fe29ad587e..b4515863d7 100644 --- a/tests/Sabre/DAVACL/PrincipalMatchTest.php +++ b/tests/Sabre/DAVACL/PrincipalMatchTest.php @@ -28,12 +28,8 @@ public function testPrincipalMatch() $expected = << - HTTP/1.1 200 OK /principals/user1 - - - HTTP/1.1 418 I'm a teapot - + HTTP/1.1 200 OK XML; @@ -63,7 +59,6 @@ public function testPrincipalMatchProp() $expected = << - HTTP/1.1 200 OK /principals/user1/ @@ -102,7 +97,6 @@ public function testPrincipalMatchPrincipalProperty() $expected = << - HTTP/1.1 200 OK /principals/user1/