fix(dav): Improve handling and logging of bulk upload failures

Signed-off-by: Joas Schilling <coding@schilljs.com>

diff --git a/apps/dav/lib/BulkUpload/BulkUploadPlugin.php b/apps/dav/lib/BulkUpload/BulkUploadPlugin.php
index 66e2a9efa2e7b6392b15d0528936a805a21d7b1b..4d838d255eb08e7ade4679f02693edd07e1f191b 100644 (file)
--- a/apps/dav/lib/BulkUpload/BulkUploadPlugin.php
+++ b/apps/dav/lib/BulkUpload/BulkUploadPlugin.php
@@ -65,7 +65,7 @@ class BulkUploadPlugin extends ServerPlugin {
                        return true;
                }
 
-               $multiPartParser = new MultipartRequestParser($request);
+               $multiPartParser = new MultipartRequestParser($request, $this->logger);
                $writtenFiles = [];
 
                while (!$multiPartParser->isAtLastBoundary()) {

diff --git a/apps/dav/lib/BulkUpload/MultipartRequestParser.php b/apps/dav/lib/BulkUpload/MultipartRequestParser.php
index 930e86c28b58273998a715d4f30f4d387789e63b..2541ea8f333378b26d84f203cd01aae8f9c47257 100644 (file)
--- a/apps/dav/lib/BulkUpload/MultipartRequestParser.php
+++ b/apps/dav/lib/BulkUpload/MultipartRequestParser.php
@@ -23,6 +23,7 @@
 namespace OCA\DAV\BulkUpload;
 
 use OCP\AppFramework\Http;
+use Psr\Log\LoggerInterface;
 use Sabre\DAV\Exception;
 use Sabre\DAV\Exception\BadRequest;
 use Sabre\DAV\Exception\LengthRequired;
@@ -42,7 +43,10 @@ class MultipartRequestParser {
        /**
         * @throws BadRequest
         */
-       public function __construct(RequestInterface $request) {
+       public function __construct(
+               RequestInterface $request,
+               protected LoggerInterface $logger,
+       ) {
                $stream = $request->getBody();
                $contentType = $request->getHeader('Content-Type');
 
@@ -78,7 +82,7 @@ class MultipartRequestParser {
                $boundaryValue = trim($boundaryValue);
 
                // Remove potential quotes around boundary value.
-               if (substr($boundaryValue, 0, 1) == '"' && substr($boundaryValue, -1) == '"') {
+               if (substr($boundaryValue, 0, 1) === '"' && substr($boundaryValue, -1) === '"') {
                        $boundaryValue = substr($boundaryValue, 1, -1);
                }
 
@@ -179,6 +183,11 @@ class MultipartRequestParser {
                                throw new Exception('An error occurred while reading headers of a part');
                        }
 
+                       if (!str_contains($line, ':')) {
+                               $this->logger->error('Header missing ":" on bulk request: ' . json_encode($line));
+                               throw new Exception('An error occurred while reading headers of a part', Http::STATUS_BAD_REQUEST);
+                       }
+
                        try {
                                [$key, $value] = explode(':', $line, 2);
                                $headers[strtolower(trim($key))] = trim($value);

diff --git a/apps/dav/tests/unit/Files/MultipartRequestParserTest.php b/apps/dav/tests/unit/Files/MultipartRequestParserTest.php
index d131ad8e182aa78561b2eae127776b6fd43debc4..745a568b8ad2c6d23649d75100c00a6ff55491b5 100644 (file)
--- a/apps/dav/tests/unit/Files/MultipartRequestParserTest.php
+++ b/apps/dav/tests/unit/Files/MultipartRequestParserTest.php
@@ -23,9 +23,17 @@
 namespace OCA\DAV\Tests\unit\DAV;
 
 use \OCA\DAV\BulkUpload\MultipartRequestParser;
+use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
 class MultipartRequestParserTest extends TestCase {
+
+       protected LoggerInterface $logger;
+
+       protected function setUp(): void {
+               $this->logger = $this->createMock(LoggerInterface::class);
+       }
+
        private function getValidBodyObject() {
                return [
                        [
@@ -73,7 +81,7 @@ class MultipartRequestParserTest extends TestCase {
                        ->method('getBody')
                        ->willReturn($stream);
 
-               return new MultipartRequestParser($request);
+               return new MultipartRequestParser($request, $this->logger);
        }
 
 
@@ -90,7 +98,7 @@ class MultipartRequestParserTest extends TestCase {
                        ->willReturn($bodyStream);
 
                $this->expectExceptionMessage('Body should be of type resource');
-               new MultipartRequestParser($request);
+               new MultipartRequestParser($request, $this->logger);
        }
 
        /**