From e4460e3bffbe7518589602742c9ec177e954ee71 Mon Sep 17 00:00:00 2001
From: Joas Schilling <coding@schilljs.com>
Date: Mon, 4 Dec 2023 11:06:47 +0100
Subject: [PATCH] fix(dav): Improve handling and logging of bulk upload
 failures

Signed-off-by: Joas Schilling <coding@schilljs.com>
---
 apps/dav/lib/BulkUpload/BulkUploadPlugin.php        |  2 +-
 apps/dav/lib/BulkUpload/MultipartRequestParser.php  | 13 +++++++++++--
 .../tests/unit/Files/MultipartRequestParserTest.php | 12 ++++++++++--
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/apps/dav/lib/BulkUpload/BulkUploadPlugin.php b/apps/dav/lib/BulkUpload/BulkUploadPlugin.php
index 66e2a9efa2e7b..4d838d255eb08 100644
--- a/apps/dav/lib/BulkUpload/BulkUploadPlugin.php
+++ b/apps/dav/lib/BulkUpload/BulkUploadPlugin.php
@@ -65,7 +65,7 @@ public function httpPost(RequestInterface $request, ResponseInterface $response)
 			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 930e86c28b582..2541ea8f33337 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 @@ private function parseBoundaryFromHeaders(string $contentType): string {
 		$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 @@ private function readPartHeaders(): array {
 				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 d131ad8e182aa..745a568b8ad2c 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 @@ private function getMultipartParser(array $parts, array $headers = [], string $b
 			->method('getBody')
 			->willReturn($stream);
 
-		return new MultipartRequestParser($request);
+		return new MultipartRequestParser($request, $this->logger);
 	}
 
 
@@ -90,7 +98,7 @@ public function testBodyTypeValidation(): void {
 			->willReturn($bodyStream);
 
 		$this->expectExceptionMessage('Body should be of type resource');
-		new MultipartRequestParser($request);
+		new MultipartRequestParser($request, $this->logger);
 	}
 
 	/**