Merge pull request #42001 from nextcloud/bugfix/noid/improve-logging-of-bulk-failures

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

Author: nickvergessen

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()) {

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);

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);
 	}
 
 	/**