From f57a81763fce2cf7111e60facc04f9b59b41b86d Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 3 Aug 2026 11:11:01 -0400 Subject: [PATCH 1/4] fix(dav): resolve bulk-uploaded files by path after touch Avoid a nullable/ambiguous node: In particular, `getFirstNodeById()` doesn't guarantee which node it'll return. Signed-off-by: Josh --- apps/dav/lib/BulkUpload/BulkUploadPlugin.php | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/apps/dav/lib/BulkUpload/BulkUploadPlugin.php b/apps/dav/lib/BulkUpload/BulkUploadPlugin.php index 58e2275f1655a..6f0fa70ffb13e 100644 --- a/apps/dav/lib/BulkUpload/BulkUploadPlugin.php +++ b/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(), ]; From f24f604edb96b59388bf2001419a6e8d72b9d68a Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 3 Aug 2026 11:19:30 -0400 Subject: [PATCH 2/4] fix(dav): validate bulk upload part headers Signed-off-by: Josh --- apps/dav/lib/BulkUpload/MultipartRequestParser.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/apps/dav/lib/BulkUpload/MultipartRequestParser.php b/apps/dav/lib/BulkUpload/MultipartRequestParser.php index aa0f67e6b0e05..c54d24b95f4be 100644 --- a/apps/dav/lib/BulkUpload/MultipartRequestParser.php +++ b/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.'); From 51d5a68762e9d51742e88c85d0c69c52782b1295 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 3 Aug 2026 16:44:42 -0400 Subject: [PATCH 3/4] test(dav): cover more bulk upload part headers scenarios Signed-off-by: Josh --- .../unit/Files/MultipartRequestParserTest.php | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/apps/dav/tests/unit/Files/MultipartRequestParserTest.php b/apps/dav/tests/unit/Files/MultipartRequestParserTest.php index 78b1cc557e257..87d3c4e5fbd5c 100644 --- a/apps/dav/tests/unit/Files/MultipartRequestParserTest.php +++ b/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. */ From 845d39997ca7e89533701523af736ea3ffd96540 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 3 Aug 2026 16:47:14 -0400 Subject: [PATCH 4/4] test(dav): cover bulk upload node reload behavior Signed-off-by: Josh --- .../tests/unit/Files/BulkUploadPluginTest.php | 108 ++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 apps/dav/tests/unit/Files/BulkUploadPluginTest.php diff --git a/apps/dav/tests/unit/Files/BulkUploadPluginTest.php b/apps/dav/tests/unit/Files/BulkUploadPluginTest.php new file mode 100644 index 0000000000000..996a2832b20d7 --- /dev/null +++ b/apps/dav/tests/unit/Files/BulkUploadPluginTest.php @@ -0,0 +1,108 @@ +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; + } +}