From f0b25ca1d11a2cfacae7edd5bed1c53503659d9d Mon Sep 17 00:00:00 2001 From: mostafa Date: Sun, 23 Aug 2026 16:56:47 +0330 Subject: [PATCH] fix(dav): only derive the write size from a PUT Content-Length put() is not only reached by PUT. The chunked upload assembly step is a MOVE or COPY of /.file: Directory::moveInto() and ::copyInto() only short-circuit File and Directory sources at storage level, so a FutureFile falls through to Tree::copyNode() -> createFile() -> put(), where the data is the AssemblyStream of the uploaded chunks and the request itself carries no body at all. For those requests the Content-Length says nothing about the stream being written. Clients that send "Content-Length: 0" there, Safari among them, made File::put() hand a 0 to IWriteStreamStorage::writeStream(). Storages that only measure the stream when they are given no size, ObjectStoreStorage among them, then wrote an empty file while the request still answered 201, so the upload looked successful and the file was silently empty. The size comparison further down already restricts itself to PUT for exactly this reason. Apply the same restriction when deriving the expected size, which covers both the MOVE and the COPY variant. Genuine empty PUT uploads are unaffected: the method is still PUT, so a Content-Length of 0 is passed through as before. Signed-off-by: mostafa Co-Authored-By: Claude Opus 5 --- apps/dav/lib/Connector/Sabre/File.php | 20 ++++- .../tests/unit/Connector/Sabre/FileTest.php | 73 +++++++++++++++++++ 2 files changed, 91 insertions(+), 2 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index ae68770d9a90c..6c099904ed2cb 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -208,8 +208,24 @@ public function put($data) { } } - $lengthHeader = $this->request->getHeader('content-length'); - $expected = $lengthHeader !== '' ? (int)$lengthHeader : null; + // Only the Content-Length of a PUT body describes the data that is about to be + // written. put() is also reached by the chunked upload assembly step, a MOVE or + // COPY of /.file, where the data is the AssemblyStream of the chunks + // and the request itself carries no body at all - Directory::moveInto() and + // ::copyInto() only short-circuit File and Directory sources, so a FutureFile + // falls through to Tree::copyNode() -> createFile() -> put(). Clients that send + // "Content-Length: 0" on that request (Safari, for instance) would otherwise + // hand a 0 down to writeStream(), and backends which only determine the length + // themselves when none was given - such as ObjectStoreStorage - then store an + // empty file while still reporting success. The size comparison further down is + // already restricted to PUT for the same reason. + $expected = null; + if ($this->request->getMethod() === 'PUT') { + $lengthHeader = $this->request->getHeader('content-length'); + if ($lengthHeader !== '') { + $expected = (int)$lengthHeader; + } + } if ($partStorage->instanceOfStorage(IWriteStreamStorage::class)) { $isEOF = false; diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 361359593dd2a..dfa244ab35736 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -215,6 +215,79 @@ function ($path) use ($storage) { $this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files'); } + public static function expectedSizeProvider(): array { + return [ + 'PUT with a length passes it through' => ['PUT', ['CONTENT_LENGTH' => '9'], 9], + 'PUT of an empty body still expects zero' => ['PUT', ['CONTENT_LENGTH' => '0'], 0], + 'PUT without the header expects nothing' => ['PUT', [], null], + // A MOVE is the chunked upload assembly step and carries no body, so whatever + // Content-Length it happens to send must not be taken as the size of the + // assembled stream. Safari sends 0 here, which used to be written verbatim. + 'MOVE ignores a zero length' => ['MOVE', ['CONTENT_LENGTH' => '0'], null], + 'MOVE ignores a non-zero length' => ['MOVE', ['CONTENT_LENGTH' => '9'], null], + // COPY reaches put() the same way: Directory::copyInto() only short-circuits + // File and Directory sources at storage level, so a FutureFile falls through + // to Tree::copyNode() -> createFile() -> put(). + 'COPY ignores a zero length' => ['COPY', ['CONTENT_LENGTH' => '0'], null], + 'COPY ignores a non-zero length' => ['COPY', ['CONTENT_LENGTH' => '9'], null], + ]; + } + + /** + * The expected size handed to IWriteStreamStorage::writeStream() may only come from + * the Content-Length of a PUT body. Passing it on for other methods makes storages + * that only measure the stream when given no size - ObjectStoreStorage among them - + * write a truncated or empty file. + */ + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'expectedSizeProvider')] + public function testPutExpectedSizeOnlyComesFromPutContentLength(string $method, array $server, ?int $expectedSize): void { + $storage = $this->getMockBuilder(Local::class) + ->onlyMethods(['writeStream']) + ->setConstructorArgs([['datadir' => Server::get(ITempManager::class)->getTemporaryFolder()]]) + ->getMock(); + Filesystem::mount($storage, [], $this->user . '/'); + + /** @var View&MockObject $view */ + $view = $this->getMockBuilder(View::class) + ->onlyMethods(['getRelativePath', 'resolvePath']) + ->getMock(); + $view->expects($this->atLeastOnce()) + ->method('resolvePath') + ->willReturnCallback(fn ($path) => [$storage, $path]); + $view->expects($this->any()) + ->method('getRelativePath') + ->willReturnArgument(0); + + $receivedSize = false; + $storage->expects($this->once()) + ->method('writeStream') + ->willReturnCallback(function (string $path, $stream, ?int $size = null) use (&$receivedSize): int { + $receivedSize = $size; + return (int)stream_copy_to_stream($stream, fopen('php://temp', 'r+')); + }); + + $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ + 'permissions' => Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, + ], null); + + $request = new Request([ + 'server' => $server, + 'method' => $method, + ], $this->requestId, $this->config, null); + + $file = new File($view, $info, null, $request); + + try { + $file->put($this->getStream('test data')); + } catch (\Exception $e) { + // Whatever happens after the write - size checks, renaming the part file - is + // not what this test is about. + } + + $this->assertSame($expectedSize, $receivedSize); + } + /** * Simulate putting a file to the given path. *