fix(dav): bulk upload part validation and path-based file reload

apps/dav/lib/BulkUpload/BulkUploadPlugin.php

@@ -60,6 +60,8 @@ public function httpPost(RequestInterface $request, ResponseInterface $response)
 			}
 
 			try {
+				$path = $headers['x-file-path'];
+
 				// TODO: Remove 'x-file-mtime' when the desktop client no longer use it.
 				if (isset($headers['x-file-mtime'])) {
 					$mtime = MtimeSanitizer::sanitizeMtime($headers['x-file-mtime']);
@@ -69,19 +71,20 @@ public function httpPost(RequestInterface $request, ResponseInterface $response)
 					$mtime = null;
 				}
 
-				$node = $this->userFolder->newFile($headers['x-file-path'], $content);
+				$node = $this->userFolder->newFile($path, $content);
 				$node->touch($mtime);
-				$node = $this->userFolder->getFirstNodeById($node->getId());
-
-				$writtenFiles[$headers['x-file-path']] = [
+				// re-fetch to obtain updated metadata
+				$node = $this->userFolder->get($path);
+
+				$writtenFiles[$path] = [
 					'error' => false,
 					'etag' => $node->getETag(),
 					'fileid' => DavUtil::getDavFileId($node->getId()),
 					'permissions' => DavUtil::getDavPermissions($node, $node->getParent()),
 				];
 			} catch (\Exception $e) {
-				$this->logger->error($e->getMessage(), ['path' => $headers['x-file-path']]);
-				$writtenFiles[$headers['x-file-path']] = [
+				$this->logger->error($e->getMessage(), ['path' => $path ?? null]);
+				$writtenFiles[$path ?? ''] = [
 					'error' => true,
 					'message' => $e->getMessage(),
 				];

apps/dav/lib/BulkUpload/MultipartRequestParser.php

@@ -193,6 +193,14 @@ private function readPartHeaders(): array {
 			throw new LengthRequired('The Content-Length header must not be null.');
 		}
 
+		if (!ctype_digit($headers['content-length'])) {
+			throw new BadRequest('Content-Length must be a non-negative integer.');
+		}
+
+		if (!isset($headers['x-file-path']) || $headers['x-file-path'] === '') {
+			throw new BadRequest('The X-File-Path header must not be null or empty.');
+		}
+
 		// TODO: Drop $md5 condition when the latest desktop client that uses it is no longer supported.
 		if (!isset($headers['x-file-md5']) && !isset($headers['oc-checksum'])) {
 			throw new BadRequest('The hash headers must not be null.');

apps/dav/tests/unit/Files/BulkUploadPluginTest.php

@@ -0,0 +1,108 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2021 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-only
+ */
+
+namespace OCA\DAV\Tests\unit\Files;
+
+use OCA\DAV\BulkUpload\BulkUploadPlugin;
+use OCP\AppFramework\Http;
+use OCP\Files\File;
+use OCP\Files\Folder;
+use OCP\Files\NotFoundException;
+use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
+use Sabre\HTTP\RequestInterface;
+use Sabre\HTTP\ResponseInterface;
+use Test\TestCase;
+
+class BulkUploadPluginTest extends TestCase {
+	private Folder&MockObject $userFolder;
+	private LoggerInterface&MockObject $logger;
+	private BulkUploadPlugin $plugin;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->userFolder = $this->createMock(Folder::class);
+		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->plugin = new BulkUploadPlugin($this->userFolder, $this->logger);
+	}
+
+	public function testPathLookupFailureIsReportedForPart(): void {
+		$request = $this->createBulkRequest('/coucou.txt', "Coucou\n");
+		$response = $this->createMock(ResponseInterface::class);
+
+		/** @var File&MockObject $createdFile */
+		$createdFile = $this->createMock(File::class);
+		$createdFile->expects(self::once())
+			->method('touch')
+			->with(null);
+
+		$this->userFolder->expects(self::once())
+			->method('newFile')
+			->with('/coucou.txt', "Coucou\n")
+			->willReturn($createdFile);
+
+		// The fix must reload the exact upload path, rather than resolving an
+		// arbitrary accessible node using the file ID.
+		$this->userFolder->expects(self::once())
+			->method('get')
+			->with('/coucou.txt')
+			->willThrowException(new NotFoundException('Uploaded file could not be reloaded'));
+
+		$this->userFolder->expects(self::never())
+			->method('getFirstNodeById');
+
+		$this->logger->expects(self::once())
+			->method('error')
+			->with(
+				'Uploaded file could not be reloaded',
+				['path' => '/coucou.txt'],
+			);
+
+		$response->expects(self::once())
+			->method('setStatus')
+			->with(Http::STATUS_OK);
+
+		$response->expects(self::once())
+			->method('setBody')
+			->with(json_encode([
+				'/coucou.txt' => [
+					'error' => true,
+					'message' => 'Uploaded file could not be reloaded',
+				],
+			], JSON_THROW_ON_ERROR));
+
+		self::assertFalse($this->plugin->httpPost($request, $response));
+	}
+
+	private function createBulkRequest(string $path, string $content): RequestInterface {
+		$boundary = 'bulk-upload-test-boundary';
+		$body = '--' . $boundary . "\r\n"
+			. 'X-File-Path: ' . $path . "\r\n"
+			. 'X-File-MD5: ' . md5($content) . "\r\n"
+			. 'Content-Length: ' . strlen($content) . "\r\n"
+			. "\r\n"
+			. $content . "\r\n"
+			. '--' . $boundary . "--\r\n";
+
+		$stream = fopen('php://temp', 'r+');
+		fwrite($stream, $body);
+		rewind($stream);
+
+		/** @var RequestInterface&MockObject $request */
+		$request = $this->createMock(RequestInterface::class);
+		$request->method('getPath')->willReturn('bulk');
+		$request->method('getHeader')
+			->with('Content-Type')
+			->willReturn('multipart/related; boundary=' . $boundary);
+		$request->method('getBody')->willReturn($stream);
+
+		return $request;
+	}
+}

apps/dav/tests/unit/Files/MultipartRequestParserTest.php

@@ -197,6 +197,51 @@ public function testNullContentLength(): void {
 		$multipartParser->parseNextPart();
 	}
 
+	public function testMissingFilePath(): void {
+		$bodyObject = self::getValidBodyObject();
+		unset($bodyObject['0']['headers']['X-File-Path']);
+
+		$multipartParser = $this->getMultipartParser($bodyObject);
+
+		$this->expectExceptionMessage('The X-File-Path header must not be null or empty.');
+		$multipartParser->parseNextPart();
+	}
+
+	#[\PHPUnit\Framework\Attributes\DataProvider('invalidContentLengthProvider')]
+	public function testInvalidContentLength(string $contentLength): void {
+		$bodyObject = self::getValidBodyObject();
+		$bodyObject['0']['headers']['Content-Length'] = $contentLength;
+
+		$multipartParser = $this->getMultipartParser($bodyObject);
+
+		$this->expectExceptionMessage('Content-Length must be a non-negative integer.');
+		$multipartParser->parseNextPart();
+	}
+
+	public static function invalidContentLengthProvider(): array {
+		return [
+			'non-numeric' => ['not-a-number'],
+			'negative' => ['-1'],
+			'decimal' => ['1.5'],
+			'empty' => [''],
+		];
+	}
+
+	public function testZeroContentLength(): void {
+		$bodyObject = self::getValidBodyObject();
+		$bodyObject['0']['headers']['Content-Length'] = 0;
+		$bodyObject['0']['headers']['X-File-MD5'] = md5('');
+		unset($bodyObject['0']['headers']['OC-Checksum']);
+		$bodyObject['0']['content'] = '';
+
+		$multipartParser = $this->getMultipartParser($bodyObject);
+
+		[$headers, $content] = $multipartParser->parseNextPart();
+
+		$this->assertSame('0', $headers['content-length']);
+		$this->assertSame('', $content);
+}
+
 	/**
 	 * Test with a lower Content-Length.
 	 */