diff --git a/apps/dav/lib/BulkUpload/MultipartRequestParser.php b/apps/dav/lib/BulkUpload/MultipartRequestParser.php index 107c67ea691..96a90f82cde 100644 --- a/apps/dav/lib/BulkUpload/MultipartRequestParser.php +++ b/apps/dav/lib/BulkUpload/MultipartRequestParser.php @@ -134,7 +134,10 @@ class MultipartRequestParser { $headers = $this->readPartHeaders(); - $content = $this->readPartContent((int)$headers['content-length'], $headers['x-file-md5']); + $length = (int)$headers['content-length']; + + $this->validateHash($length, $headers['x-file-md5'] ?? '', $headers['oc-checksum'] ?? ''); + $content = $this->readPartContent($length); return [$headers, $content]; } @@ -184,8 +187,9 @@ class MultipartRequestParser { throw new LengthRequired('The Content-Length header must not be null.'); } - if (!isset($headers['x-file-md5'])) { - throw new BadRequest('The X-File-MD5 header must not be null.'); + // 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.'); } return $headers; @@ -197,13 +201,7 @@ class MultipartRequestParser { * @throws Exception * @throws BadRequest */ - private function readPartContent(int $length, string $md5): string { - $computedMd5 = $this->computeMd5Hash($length); - - if ($md5 !== $computedMd5) { - throw new BadRequest('Computed md5 hash is incorrect.'); - } - + private function readPartContent(int $length): string { if ($length === 0) { $content = ''; } else { @@ -225,12 +223,25 @@ class MultipartRequestParser { } /** - * Compute the MD5 hash of the next x bytes. + * Compute the MD5 or checksum hash of the next x bytes. + * TODO: Drop $md5 argument when the latest desktop client that uses it is no longer supported. */ - private function computeMd5Hash(int $length): string { - $context = hash_init('md5'); + private function validateHash(int $length, string $fileMd5Header, string $checksumHeader): void { + if ($checksumHeader !== '') { + [$algorithm, $hash] = explode(':', $checksumHeader, 2); + } elseif ($fileMd5Header !== '') { + $algorithm = 'md5'; + $hash = $fileMd5Header; + } else { + throw new BadRequest('No hash provided.'); + } + + $context = hash_init($algorithm); hash_update_stream($context, $this->stream, $length); fseek($this->stream, -$length, SEEK_CUR); - return hash_final($context); + $computedHash = hash_final($context); + if ($hash !== $computedHash) { + throw new BadRequest("Computed $algorithm hash is incorrect ($computedHash)."); + } } } diff --git a/apps/dav/tests/unit/Files/MultipartRequestParserTest.php b/apps/dav/tests/unit/Files/MultipartRequestParserTest.php index e6325ab8ad1..40880bdca9c 100644 --- a/apps/dav/tests/unit/Files/MultipartRequestParserTest.php +++ b/apps/dav/tests/unit/Files/MultipartRequestParserTest.php @@ -7,12 +7,14 @@ namespace OCA\DAV\Tests\unit\DAV; use OCA\DAV\BulkUpload\MultipartRequestParser; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; +use Sabre\HTTP\RequestInterface; use Test\TestCase; class MultipartRequestParserTest extends TestCase { - protected LoggerInterface $logger; + protected LoggerInterface&MockObject $logger; protected function setUp(): void { $this->logger = $this->createMock(LoggerInterface::class); @@ -24,6 +26,7 @@ class MultipartRequestParserTest extends TestCase { 'headers' => [ 'Content-Length' => 7, 'X-File-MD5' => '4f2377b4d911f7ec46325fe603c3af03', + 'OC-Checksum' => 'md5:4f2377b4d911f7ec46325fe603c3af03', 'X-File-Path' => '/coucou.txt' ], 'content' => "Coucou\n" @@ -32,7 +35,8 @@ class MultipartRequestParserTest extends TestCase { } private function getMultipartParser(array $parts, array $headers = [], string $boundary = 'boundary_azertyuiop'): MultipartRequestParser { - $request = $this->getMockBuilder('Sabre\HTTP\RequestInterface') + /** @var RequestInterface&MockObject $request */ + $request = $this->getMockBuilder(RequestInterface::class) ->disableOriginalConstructor() ->getMock(); @@ -74,7 +78,8 @@ class MultipartRequestParserTest extends TestCase { */ public function testBodyTypeValidation(): void { $bodyStream = 'I am not a stream, but pretend to be'; - $request = $this->getMockBuilder('Sabre\HTTP\RequestInterface') + /** @var RequestInterface&MockObject $request */ + $request = $this->getMockBuilder(RequestInterface::class) ->disableOriginalConstructor() ->getMock(); $request->expects($this->any()) @@ -88,15 +93,39 @@ class MultipartRequestParserTest extends TestCase { /** * Test with valid request. * - valid boundary - * - valid md5 hash + * - valid hash * - valid content-length * - valid file content * - valid file path */ public function testValidRequest(): void { - $multipartParser = $this->getMultipartParser( - $this->getValidBodyObject() - ); + $bodyObject = $this->getValidBodyObject(); + unset($bodyObject['0']['headers']['X-File-MD5']); + + $multipartParser = $this->getMultipartParser($bodyObject); + + [$headers, $content] = $multipartParser->parseNextPart(); + + $this->assertSame((int)$headers['content-length'], 7, 'Content-Length header should be the same as provided.'); + $this->assertSame($headers['oc-checksum'], 'md5:4f2377b4d911f7ec46325fe603c3af03', 'OC-Checksum header should be the same as provided.'); + $this->assertSame($headers['x-file-path'], '/coucou.txt', 'X-File-Path header should be the same as provided.'); + + $this->assertSame($content, "Coucou\n", 'Content should be the same'); + } + + /** + * Test with valid request. + * - valid boundary + * - valid md5 hash + * - valid content-length + * - valid file content + * - valid file path + */ + public function testValidRequestWithMd5(): void { + $bodyObject = $this->getValidBodyObject(); + unset($bodyObject['0']['headers']['OC-Checksum']); + + $multipartParser = $this->getMultipartParser($bodyObject); [$headers, $content] = $multipartParser->parseNextPart(); @@ -107,31 +136,48 @@ class MultipartRequestParserTest extends TestCase { $this->assertSame($content, "Coucou\n", 'Content should be the same'); } + /** + * Test with invalid hash. + */ + public function testInvalidHash(): void { + $bodyObject = $this->getValidBodyObject(); + $bodyObject['0']['headers']['OC-Checksum'] = 'md5:f2377b4d911f7ec46325fe603c3af03'; + unset($bodyObject['0']['headers']['X-File-MD5']); + $multipartParser = $this->getMultipartParser( + $bodyObject + ); + + $this->expectExceptionMessage('Computed md5 hash is incorrect (4f2377b4d911f7ec46325fe603c3af03).'); + $multipartParser->parseNextPart(); + } + /** * Test with invalid md5 hash. */ public function testInvalidMd5Hash(): void { $bodyObject = $this->getValidBodyObject(); + unset($bodyObject['0']['headers']['OC-Checksum']); $bodyObject['0']['headers']['X-File-MD5'] = 'f2377b4d911f7ec46325fe603c3af03'; $multipartParser = $this->getMultipartParser( $bodyObject ); - $this->expectExceptionMessage('Computed md5 hash is incorrect.'); + $this->expectExceptionMessage('Computed md5 hash is incorrect (4f2377b4d911f7ec46325fe603c3af03).'); $multipartParser->parseNextPart(); } /** - * Test with a null md5 hash. + * Test with a null hash headers. */ - public function testNullMd5Hash(): void { + public function testNullHash(): void { $bodyObject = $this->getValidBodyObject(); + unset($bodyObject['0']['headers']['OC-Checksum']); unset($bodyObject['0']['headers']['X-File-MD5']); $multipartParser = $this->getMultipartParser( $bodyObject ); - $this->expectExceptionMessage('The X-File-MD5 header must not be null.'); + $this->expectExceptionMessage('The hash headers must not be null.'); $multipartParser->parseNextPart(); } @@ -159,7 +205,7 @@ class MultipartRequestParserTest extends TestCase { $bodyObject ); - $this->expectExceptionMessage('Computed md5 hash is incorrect.'); + $this->expectExceptionMessage('Computed md5 hash is incorrect (41060d3ddfdf63e68fc2bf196f652ee9).'); $multipartParser->parseNextPart(); } @@ -173,7 +219,7 @@ class MultipartRequestParserTest extends TestCase { $bodyObject ); - $this->expectExceptionMessage('Computed md5 hash is incorrect.'); + $this->expectExceptionMessage('Computed md5 hash is incorrect (0161002bbee6a744f18741b8a914e413).'); $multipartParser->parseNextPart(); }