From: Joas Schilling <coding@schilljs.com>
Date: Mon, 4 Dec 2023 10:06:47 +0000 (+0100)
Subject: fix(dav): Improve handling and logging of bulk upload failures
X-Git-Tag: v29.0.0beta1~709^2
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=e4460e3bffbe7518589602742c9ec177e954ee71;p=nextcloud-server.git

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 66e2a9efa2e..4d838d255eb 100644
--- 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 930e86c28b5..2541ea8f333 100644
--- 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 d131ad8e182..745a568b8ad 100644
--- 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);
 	}
 
 	/**