From fba20bfa4d3768ab639c4438beaa6f23bf10e222 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:05:12 +0100 Subject: [PATCH 1/5] fix(dav): detect a chunk that is shorter on storage than the cache claims AssemblyStream compares what it read from each chunk against the size that chunk reports, but it asks for that size again while streaming. Sabre\File::get() repairs a filecache entry that disagrees with the storage and refreshes the node on the way past, so a chunk that was short on storage ends up compared against its own repaired size, matches, and the assembled file is silently truncated. Snapshot the sizes when the stream is opened and compare against those. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- apps/dav/lib/Upload/AssemblyStream.php | 23 +++++--- .../tests/unit/Upload/AssemblyStreamTest.php | 53 +++++++++++++++++++ 2 files changed, 69 insertions(+), 7 deletions(-) diff --git a/apps/dav/lib/Upload/AssemblyStream.php b/apps/dav/lib/Upload/AssemblyStream.php index 3761e777b49db..7ec2ae263e5d6 100644 --- a/apps/dav/lib/Upload/AssemblyStream.php +++ b/apps/dav/lib/Upload/AssemblyStream.php @@ -27,6 +27,15 @@ class AssemblyStream implements \Icewind\Streams\File { /** @var IFile[] */ private $nodes; + /** + * Node sizes as of stream_open: reading a node can change the size it reports, + * because Sabre\File::get() repairs a filecache entry that disagrees with the + * storage, which would otherwise hide a short chunk. + * + * @var int[] + */ + private array $nodeSizes = []; + /** @var int */ private $pos = 0; @@ -58,9 +67,10 @@ public function stream_open($path, $mode, $options, &$opened_path) { return strnatcmp($a->getName(), $b->getName()); }); $this->nodes = array_values($nodes); - $this->size = array_reduce($this->nodes, function ($size, IFile $file) { - return $size + $file->getSize(); - }, 0); + $this->nodeSizes = array_map(function (IFile $file) { + return $file->getSize(); + }, $this->nodes); + $this->size = array_sum($this->nodeSizes); return true; } @@ -92,12 +102,11 @@ public function stream_seek($offset, $whence = SEEK_SET) { if (!isset($this->nodes[$nodeIndex + 1])) { break; } - $node = $this->nodes[$nodeIndex]; - if ($nodeStart + $node->getSize() > $offset) { + if ($nodeStart + $this->nodeSizes[$nodeIndex] > $offset) { break; } + $nodeStart += $this->nodeSizes[$nodeIndex]; $nodeIndex++; - $nodeStart += $node->getSize(); } $stream = $this->getStream($this->nodes[$nodeIndex]); @@ -147,7 +156,7 @@ public function stream_read($count) { if (feof($this->currentStream)) { fclose($this->currentStream); - $currentNodeSize = $this->nodes[$this->currentNode]->getSize(); + $currentNodeSize = $this->nodeSizes[$this->currentNode]; if ($this->currentNodeRead < $currentNodeSize) { throw new \Exception('Stream from assembly node shorter than expected, got ' . $this->currentNodeRead . ' bytes, expected ' . $currentNodeSize); } diff --git a/apps/dav/tests/unit/Upload/AssemblyStreamTest.php b/apps/dav/tests/unit/Upload/AssemblyStreamTest.php index bd41c3f8962cb..93aecb06d03f6 100644 --- a/apps/dav/tests/unit/Upload/AssemblyStreamTest.php +++ b/apps/dav/tests/unit/Upload/AssemblyStreamTest.php @@ -63,6 +63,27 @@ public function testSeek(string $expected, array $nodeData): void { $this->assertEquals(substr($expected, $offset), $content); } + /** + * Reading a node can change the size it reports: Sabre\File::get() notices a + * filecache entry that disagrees with the storage, fixes it and refreshes the + * node. The short-chunk guard must keep comparing against the size the + * assembly was opened with, otherwise a chunk that is short on disk is + * accepted and the assembled file is silently truncated. + */ + public function testShortNodeIsDetectedWhenItsSizeIsRefreshedWhileReading(): void { + $nodes = [ + $this->buildNode('0', '12345'), + $this->buildShrinkingNode('1', 'ab', 5), + $this->buildNode('2', '67890'), + ]; + + $stream = AssemblyStream::wrap($nodes); + + $this->expectException(\Exception::class); + $this->expectExceptionMessage('Stream from assembly node shorter than expected'); + stream_get_contents($stream); + } + public static function providesNodes(): array { $data8k = self::makeData(8192); $dataLess8k = self::makeData(8191); @@ -164,4 +185,36 @@ private function buildNode(string $name, string $data) { return $node; } + + /** + * A node that reports the size held in the filecache until it is read, and + * its real - smaller - size afterwards, the way Sabre\File::get() behaves + * when it repairs a stale cache entry. + */ + private function buildShrinkingNode(string $name, string $data, int $cachedSize) { + $node = $this->getMockBuilder(File::class) + ->onlyMethods(['getName', 'get', 'getSize']) + ->getMock(); + + $fetched = false; + + $node->expects($this->any()) + ->method('getName') + ->willReturn($name); + + $node->expects($this->any()) + ->method('get') + ->willReturnCallback(function () use ($data, &$fetched) { + $fetched = true; + return $data; + }); + + $node->expects($this->any()) + ->method('getSize') + ->willReturnCallback(function () use ($data, $cachedSize, &$fetched) { + return $fetched ? strlen($data) : $cachedSize; + }); + + return $node; + } } From 87cee99c2e370b78adfe436f2fefe9d1f12453ef Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:05:23 +0100 Subject: [PATCH 2/5] fix(files): close both streams when an encrypted write fails Encryption::writeStream() only closed the source and target streams when the copy returned. If the source throws part way through, both were left open and the encryption stream was closed during engine shutdown instead, where writing back into the storage layer is no longer safe and takes the process down. Close them in a finally, the way Common::writeStream() already does. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- .../Files/Storage/Wrapper/Encryption.php | 22 +++++++---- .../Files/Storage/Wrapper/EncryptionTest.php | 38 +++++++++++++++++++ 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/lib/private/Files/Storage/Wrapper/Encryption.php b/lib/private/Files/Storage/Wrapper/Encryption.php index 4a1fa9135a46d..eaa6df265a93b 100644 --- a/lib/private/Files/Storage/Wrapper/Encryption.php +++ b/lib/private/Files/Storage/Wrapper/Encryption.php @@ -936,15 +936,21 @@ public function writeStream(string $path, $stream, ?int $size = null): int { if ($target === false) { throw new GenericFileException("Failed to open $path for writing"); } - $count = stream_copy_to_stream($stream, $target); - if ($count === false) { - $result = false; - $count = 0; - } else { - $result = true; + try { + $count = stream_copy_to_stream($stream, $target); + if ($count === false) { + $result = false; + $count = 0; + } else { + $result = true; + } + } finally { + // close the streams like Common::writeStream() does, also when the source + // fails: an encryption stream left open is only closed during engine + // shutdown, where its write-back into the storage layer crashes + fclose($stream); + fclose($target); } - fclose($stream); - fclose($target); // object store, stores the size after write and doesn't update this during scan // manually store the unencrypted size diff --git a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php index 5f7e157d67cf0..95fc5b7fb2b38 100644 --- a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php +++ b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php @@ -9,6 +9,7 @@ namespace Test\Files\Storage\Wrapper; use Exception; +use Icewind\Streams\CallbackWrapper; use OC\Encryption\Exceptions\ModuleDoesNotExistsException; use OC\Encryption\File; use OC\Encryption\Util; @@ -1026,4 +1027,41 @@ public static function dataTestShouldEncrypt(): array { [false, true, true, true, false], ]; } + + /** + * A source stream that fails part way through must still leave both streams + * closed: an encryption stream that stays open is only closed during engine + * shutdown, where writing back into the storage layer is no longer safe. + */ + public function testWriteStreamClosesStreamsWhenTheSourceFails(): void { + $target = fopen('php://temp', 'w+'); + + /** @var Encryption&MockObject $storage */ + $storage = $this->getMockBuilder(Encryption::class) + ->disableOriginalConstructor() + ->onlyMethods(['fopen']) + ->getMock(); + $storage->expects($this->once()) + ->method('fopen') + ->willReturn($target); + + $source = fopen('php://temp', 'r+'); + fwrite($source, 'some data'); + rewind($source); + $failing = CallbackWrapper::wrap($source, function ($count): void { + throw new Exception('source stream failed'); + }); + + $thrown = null; + try { + $storage->writeStream('foo.txt', $failing); + } catch (\Throwable $e) { + $thrown = $e; + } + + $this->assertNotNull($thrown, 'Expected the source failure to propagate'); + $this->assertEquals('source stream failed', $thrown->getMessage()); + $this->assertFalse(is_resource($failing), 'source stream was closed'); + $this->assertFalse(is_resource($target), 'target stream was closed'); + } } From 48d1b20c1d40a412169805ac6148d409a3d3cc6a Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:05:28 +0100 Subject: [PATCH 3/5] fix(dav): verify the assembled file against OC-Total-Length after the move The chunks are checked against the length the client declared before the move, but nothing checks what actually landed. An assembly cut short leaves the destination truncated while the response still reports the upload as a success. Compare the destination against the declared length once the move is done. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- apps/dav/lib/Upload/ChunkingPlugin.php | 38 ++++++++++-- .../tests/unit/Upload/ChunkingPluginTest.php | 62 ++++++++++++++++++- 2 files changed, 94 insertions(+), 6 deletions(-) diff --git a/apps/dav/lib/Upload/ChunkingPlugin.php b/apps/dav/lib/Upload/ChunkingPlugin.php index 0f71f3437f0fb..87da47344f2b2 100644 --- a/apps/dav/lib/Upload/ChunkingPlugin.php +++ b/apps/dav/lib/Upload/ChunkingPlugin.php @@ -13,6 +13,7 @@ use OCP\AppFramework\Http; use Sabre\DAV\Exception\BadRequest; use Sabre\DAV\Exception\NotFound; +use Sabre\DAV\IFile; use Sabre\DAV\INode; use Sabre\DAV\Server; use Sabre\DAV\ServerPlugin; @@ -57,8 +58,8 @@ public function beforeMove($sourcePath, $destination) { // If the destination does not exist yet it's not a directory either ;) } - $this->verifySize(); - return $this->performMove($sourcePath, $destination); + $expectedSize = $this->verifySize(); + return $this->performMove($sourcePath, $destination, $expectedSize); } /** @@ -70,9 +71,10 @@ public function beforeMove($sourcePath, $destination) { * * @param string $path source path * @param string $destination destination path + * @param int|float|null $expectedSize size the assembled file should have * @return bool|void false to stop handling, void to skip this handler */ - public function performMove($path, $destination) { + public function performMove($path, $destination, $expectedSize = null) { $fileExists = $this->server->tree->nodeExists($destination); // do a move manually, skipping Sabre's default "delete" for existing nodes try { @@ -85,6 +87,8 @@ public function performMove($path, $destination) { throw $e; } + $this->verifyAssembledSize($destination, $expectedSize); + // trigger all default events (copied from CorePlugin::move) $this->server->emit('afterMove', [$path, $destination]); $this->server->emit('afterUnbind', [$path]); @@ -98,12 +102,13 @@ public function performMove($path, $destination) { } /** + * @return int|float|null the expected assembled size, null if the client did not declare one * @throws BadRequest */ private function verifySize() { $expectedSize = $this->server->httpRequest->getHeader('OC-Total-Length'); if ($expectedSize === null) { - return; + return null; } $actualSize = $this->sourceNode->getSize(); @@ -112,5 +117,30 @@ private function verifySize() { if ((string)$expectedSize !== (string)$actualSize) { throw new BadRequest("Chunks on server do not sum up to $expectedSize but to $actualSize bytes"); } + + return $actualSize; + } + + /** + * An assembly cut short leaves the destination truncated while the response + * still reports success, so check what actually landed. + * + * @param int|float|null $expectedSize + * @throws BadRequest + */ + private function verifyAssembledSize(string $destination, $expectedSize): void { + if ($expectedSize === null) { + return; + } + + $destinationNode = $this->server->tree->getNodeForPath($destination); + if (!$destinationNode instanceof IFile) { + return; + } + + $actualSize = $destinationNode->getSize(); + if ((string)$expectedSize !== (string)$actualSize) { + throw new BadRequest("Assembled file has $actualSize bytes but $expectedSize bytes were expected"); + } } } diff --git a/apps/dav/tests/unit/Upload/ChunkingPluginTest.php b/apps/dav/tests/unit/Upload/ChunkingPluginTest.php index 62f2d5e9c98c1..d55d932bd6336 100644 --- a/apps/dav/tests/unit/Upload/ChunkingPluginTest.php +++ b/apps/dav/tests/unit/Upload/ChunkingPluginTest.php @@ -14,6 +14,7 @@ use OCA\DAV\Upload\FutureFile; use PHPUnit\Framework\MockObject\MockObject; use Sabre\DAV\Exception\NotFound; +use Sabre\DAV\IFile; use Sabre\HTTP\RequestInterface; use Sabre\HTTP\ResponseInterface; use Test\TestCase; @@ -83,8 +84,9 @@ public function testBeforeMoveFutureFileSkipNonExisting(): void { $calls = [ ['source', $sourceNode], ['target', new NotFound()], + ['target', $this->buildAssembledNode(4)], ]; - $this->tree->expects($this->exactly(2)) + $this->tree->expects($this->exactly(3)) ->method('getNodeForPath') ->willReturnCallback(function (string $path) use (&$calls) { $expected = array_shift($calls); @@ -121,8 +123,9 @@ public function testBeforeMoveFutureFileMoveIt(): void { $calls = [ ['source', $sourceNode], ['target', new NotFound()], + ['target', $this->buildAssembledNode(4)], ]; - $this->tree->expects($this->exactly(2)) + $this->tree->expects($this->exactly(3)) ->method('getNodeForPath') ->willReturnCallback(function (string $path) use (&$calls) { $expected = array_shift($calls); @@ -186,4 +189,59 @@ public function testBeforeMoveSizeIsWrong(): void { $this->assertFalse($this->plugin->beforeMove('source', 'target')); } + + /** + * The chunks summed up to the expected size, but the assembly that streamed + * them into the destination was cut short. What actually landed has to be + * checked, otherwise the truncated file is reported back as a success. + */ + public function testBeforeMoveAssembledFileIsTruncated(): void { + $this->expectException(\Sabre\DAV\Exception\BadRequest::class); + $this->expectExceptionMessage('Assembled file has 3 bytes but 4 bytes were expected'); + + $sourceNode = $this->createMock(FutureFile::class); + $sourceNode->expects($this->once()) + ->method('getSize') + ->willReturn(4); + + $calls = [ + ['source', $sourceNode], + ['target', new NotFound()], + ['target', $this->buildAssembledNode(3)], + ]; + $this->tree->expects($this->exactly(3)) + ->method('getNodeForPath') + ->willReturnCallback(function (string $path) use (&$calls) { + $expected = array_shift($calls); + $this->assertSame($expected[0], $path); + if ($expected[1] instanceof \Throwable) { + throw $expected[1]; + } + return $expected[1]; + }); + + $this->tree->expects($this->any()) + ->method('nodeExists') + ->with('target') + ->willReturn(false); + $this->tree->expects($this->once()) + ->method('move') + ->with('source', 'target'); + $this->response->expects($this->never()) + ->method('setStatus'); + $this->request->expects($this->once()) + ->method('getHeader') + ->with('OC-Total-Length') + ->willReturn('4'); + + $this->plugin->beforeMove('source', 'target'); + } + + private function buildAssembledNode(int $size): IFile&MockObject { + $node = $this->createMock(IFile::class); + $node->expects($this->once()) + ->method('getSize') + ->willReturn($size); + return $node; + } } From 92788863e67f61b8a0bea4eae3101492f61fa3c1 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:06:30 +0100 Subject: [PATCH 4/5] fix(dav): fail an upload whose storage write failed without a content-length A failed write was only reported when the request declared a content-length to check against. The MOVE that assembles a chunked upload never declares one, so on object storage a write that the backend rejected was answered with 204 while the previous object was still in place - a lost update the client cannot see. A write that failed is a failure whether or not a size was declared. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- apps/dav/lib/Connector/Sabre/File.php | 7 ++- .../tests/unit/Connector/Sabre/FileTest.php | 59 +++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index ae68770d9a90c..c4028568fdced 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -248,7 +248,12 @@ public function put($data) { } fclose($target); } - if ($result === false && $expected !== null) { + if ($result === false) { + if ($expected === null) { + // nothing to report the size against, e.g. the MOVE that assembles + // a chunked upload - but the write still failed + throw new Exception($this->l10n->t('Could not write file contents')); + } throw new Exception( $this->l10n->t( 'Error while copying file to target location (copied: %1$s, expected filesize: %2$s)', diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 361359593dd2a..2ba9430dcd55e 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -21,14 +21,17 @@ use OCA\DAV\Connector\Sabre\File; use OCP\Constants; use OCP\Encryption\Exceptions\GenericEncryptionException; +use OCP\Files\Cache\IUpdater; use OCP\Files\EntityTooLargeException; use OCP\Files\FileInfo; use OCP\Files\ForbiddenException; +use OCP\Files\GenericFileException; use OCP\Files\InvalidContentException; use OCP\Files\InvalidPathException; use OCP\Files\LockNotAcquiredException; use OCP\Files\NotPermittedException; use OCP\Files\Storage\IStorage; +use OCP\Files\Storage\IWriteStreamStorage; use OCP\Files\StorageNotAvailableException; use OCP\IConfig; use OCP\IRequestId; @@ -529,6 +532,61 @@ public function testSimplePutFailsSizeCheck(): void { $this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files'); } + /** + * A storage write that fails must not be reported as success just because the + * request carried no content-length to check against - the MOVE that assembles + * a chunked upload never does, so this used to answer 204 while the storage + * still held the previous content. + */ + public function testPutFailsWhenStorageWriteFailsWithoutContentLength(): void { + $storage = $this->createMock(IWriteStreamStorage::class); + $storage->method('getId')->willReturn('object::user:' . $this->user); + // object stores write straight to the final path + $storage->method('needsPartFile')->willReturn(false); + $storage->method('instanceOfStorage') + ->willReturnCallback(fn (string $class): bool => $class === IWriteStreamStorage::class); + $storage->method('writeStream') + ->willThrowException(new GenericFileException('Error while writing stream to object store')); + // let the bookkeeping that follows a successful write run, so that a + // swallowed failure shows up as "no exception" rather than as a side effect + $storage->method('getUpdater')->willReturn($this->createMock(IUpdater::class)); + + $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ + 'permissions' => Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FILE, + ], null); + + /** @var View&MockObject */ + $view = $this->getMockBuilder(View::class) + ->onlyMethods(['resolvePath', 'getRelativePath', 'file_exists', 'putFileInfo', 'getFileInfo']) + ->getMock(); + $view->expects($this->any()) + ->method('resolvePath') + ->willReturn([$storage, 'files/test.txt']); + $view->expects($this->any()) + ->method('getRelativePath') + ->willReturnArgument(0); + $view->expects($this->any()) + ->method('file_exists') + ->willReturn(true); + $view->expects($this->any()) + ->method('putFileInfo') + ->willReturn(true); + $view->expects($this->any()) + ->method('getFileInfo') + ->willReturn($info); + + // the assembly MOVE of a chunked upload sends no content-length + $request = new Request([ + 'method' => 'MOVE', + ], $this->requestId, $this->config, null); + + $file = new File($view, $info, null, $request); + + $this->expectException(\Sabre\DAV\Exception::class); + $file->put($this->getStream('irrelevant')); + } + /** * Test exception during final rename in simple upload mode */ @@ -1031,6 +1089,7 @@ public function testSimplePutNoCreatePermissions(): void { $this->assertEquals('new content', $view->file_get_contents('root/file.txt')); } + public function testPutLockExpired(): void { $view = new View('/' . $this->user . '/files/'); From 4e2ef40926d903b0c28563ffb1ea86db592736e8 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:06:46 +0100 Subject: [PATCH 5/5] fix(dav): keep part files when overwriting so an interrupted upload cannot destroy the target The guard deciding whether a DAV write goes through a part file asked View::isCreatable() about the part file path itself. isCreatable() answers whether something can be created *inside* a path, so for a file that does not exist yet it is always false and the guard collapsed to "skip the part file whenever the target is updatable" - that is, for every overwrite. An upload interrupted during assembly then truncated the user's existing file in place, while oc_filecache kept asserting the previous size and etag, so no client had any reason to re-fetch until a later occ files:scan turned the divergence into a download of the empty file. Ask isCreatable() about the directory that will hold the part file instead, and keep the part file out of the two cases where it cannot stand in for the target: - Part file names were always hashed, although the commit introducing the hashing only meant to do so for names too long to fit. Encryption resolves a part file's key by stripping the .ocTransferId suffix, which only leads back to the target while the real name is kept, so a hashed name left an encrypted overwrite undecryptable. Hash only when the name would overflow the filesystem limit, and keep writing directly to the target when it must be. - A single file share maps the target and nothing else, so a part file named beside it lands on a different storage - the recipient's own, under their quota - rather than next to the file being written. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- apps/dav/lib/Connector/Sabre/File.php | 49 +++- .../tests/unit/Connector/Sabre/FileTest.php | 45 +++ .../RequestTest/ChunkedUploadAssemblyTest.php | 270 ++++++++++++++++++ ...yptionUserKeyChunkedUploadAssemblyTest.php | 40 +++ 4 files changed, 395 insertions(+), 9 deletions(-) create mode 100644 apps/dav/tests/unit/Connector/Sabre/RequestTest/ChunkedUploadAssemblyTest.php create mode 100644 apps/dav/tests/unit/Connector/Sabre/RequestTest/EncryptionUserKeyChunkedUploadAssemblyTest.php diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index c4028568fdced..d37bc234a6b6d 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -52,6 +52,11 @@ use Sabre\DAV\IFile; class File extends Node implements IFile { + /** Longest name common filesystems (ext/xfs) accept */ + private const MAX_FILENAME_LENGTH = 255; + /** '.ocTransferId' + a rand() value + '.part' */ + private const PART_FILE_SUFFIX_MAX_LENGTH = 28; + protected IRequest $request; protected IL10N $l10n; @@ -135,28 +140,48 @@ public function put($data) { if ($needsPartFile) { $transferId = \rand(); + $partFileBasePath = $this->getPartFileBasePath($this->path); // mark file as partial while uploading (ignored by the scanner) - $partFilePath = $this->getPartFileBasePath($this->path) . '.ocTransferId' . $transferId . '.part'; + $partFilePath = $partFileBasePath . '.ocTransferId' . $transferId . '.part'; - if (!$view->isCreatable($partFilePath) && $view->isUpdatable($this->path)) { + // isCreatable() asks whether something can be created *inside* a path, so + // it has to be given the directory that will hold the part file + if (!$view->isCreatable(dirname($partFilePath)) && $view->isUpdatable($this->path)) { $needsPartFile = false; } - } - if (!$needsPartFile) { - // upload file directly as the final path - $partFilePath = $this->path; - if ($view && !$this->emitPreHooks($exists)) { - throw new Exception($this->l10n->t('Could not write to final file, canceled by hook')); + // a hashed part file name cannot reuse the target's encryption key, so + // renaming it over an existing file would leave undecryptable content + if ($exists && $partFileBasePath !== $this->path) { + $needsPartFile = false; } + } // the part file and target file might be on a different storage in case of a single file storage (e.g. single file share) - [$partStorage, $internalPartPath] = $this->fileView->resolvePath($partFilePath); + [$partStorage, $internalPartPath] = $this->fileView->resolvePath($needsPartFile ? $partFilePath : $this->path); [$storage, $internalPath] = $this->fileView->resolvePath($this->path); if ($partStorage === null || $storage === null) { throw new ServiceUnavailable($this->l10n->t('Failed to get storage for file')); } + + // a single file share maps the target and nothing else, so the part file + // would land beside it on a different storage - the recipient's own, with + // their quota - instead of next to the file being written + if ($needsPartFile && $partStorage->getId() !== $storage->getId()) { + $needsPartFile = false; + $partStorage = $storage; + $internalPartPath = $internalPath; + } + + if (!$needsPartFile) { + // upload file directly as the final path + $partFilePath = $this->path; + + if ($view && !$this->emitPreHooks($exists)) { + throw new Exception($this->l10n->t('Could not write to final file, canceled by hook')); + } + } try { if (!$needsPartFile) { try { @@ -412,6 +437,12 @@ private function getPartFileBasePath($path) { $partFileInStorage = Server::get(IConfig::class)->getSystemValue('part_file_in_storage', true); if ($partFileInStorage) { $filename = basename($path); + // only hash when the name would otherwise overflow the filesystem limit: + // encryption resolves the part file's key by stripping the suffix, which + // only leads back to the target while the real name is kept + if (strlen($filename) + self::PART_FILE_SUFFIX_MAX_LENGTH <= self::MAX_FILENAME_LENGTH) { + return $path; + } // hash does not need to be secure but fast and semi unique $hashedFilename = hash('xxh128', $filename); return substr($path, 0, strlen($path) - strlen($filename)) . $hashedFilename; diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 2ba9430dcd55e..b8a67dce85f66 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -9,6 +9,7 @@ namespace OCA\DAV\Tests\unit\Connector\Sabre; +use Icewind\Streams\CallbackWrapper; use OC\AppFramework\Http\Request; use OC\Files\Filesystem; use OC\Files\Storage\Local; @@ -1089,6 +1090,50 @@ public function testSimplePutNoCreatePermissions(): void { $this->assertEquals('new content', $view->file_get_contents('root/file.txt')); } + /** + * An upload that is interrupted while overwriting an existing file must not + * destroy what is already there: the data goes into a part file first and is + * only renamed over the target once it is complete. + */ + public function testPutOverwriteInterruptedKeepsOriginal(): void { + $view = new View('/' . $this->user . '/files'); + $view->file_put_contents('interrupted.txt', 'original content'); + + [$targetStorage] = $view->resolvePath('interrupted.txt'); + if (!$targetStorage->needsPartFile()) { + // object stores write straight to the final path, so there is no part + // file to protect the previous content - nothing to assert here + $this->markTestSkipped('Storage does not use part files'); + } + + $file = new File($view, $view->getFileInfo('interrupted.txt')); + + $read = 0; + $data = CallbackWrapper::wrap($this->getStream('new content'), function ($count) use (&$read): void { + $read += $count; + if ($read > 3) { + throw new \RuntimeException('connection lost mid upload'); + } + }); + + // beforeMethod locks + $view->lockFile('interrupted.txt', ILockingProvider::LOCK_SHARED); + try { + $file->put($data); + $this->fail('Expected the interrupted upload to fail'); + } catch (\Sabre\DAV\Exception $e) { + // expected + } finally { + // afterMethod unlocks + $view->unlockFile('interrupted.txt', ILockingProvider::LOCK_SHARED); + } + + // read straight from the storage: a failed write must not have touched it, + // whatever the view still holds a lock on + [$storage, $internalPath] = $view->resolvePath('interrupted.txt'); + $this->assertEquals('original content', $storage->file_get_contents($internalPath)); + $this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files'); + } public function testPutLockExpired(): void { $view = new View('/' . $this->user . '/files/'); diff --git a/apps/dav/tests/unit/Connector/Sabre/RequestTest/ChunkedUploadAssemblyTest.php b/apps/dav/tests/unit/Connector/Sabre/RequestTest/ChunkedUploadAssemblyTest.php new file mode 100644 index 0000000000000..fe3c7c1612566 --- /dev/null +++ b/apps/dav/tests/unit/Connector/Sabre/RequestTest/ChunkedUploadAssemblyTest.php @@ -0,0 +1,270 @@ +//.file` against real storage: the request goes + * through the real UploadHome/UploadFolder tree, ChunkingPlugin, FutureFile and + * AssemblyStream into Directory::createFile()/File::put(). + * + * The encryption subclasses run the identical scenarios on encrypted storage. + */ +#[\PHPUnit\Framework\Attributes\Group(name: 'DB')] +class ChunkedUploadAssemblyTest extends RequestTestCase { + private const CHUNKS = [ + '00001' => [300000, 'a'], + '00002' => [170000, 'b'], + ]; + + protected function getSabreServer(View $view, $user, $password, ExceptionPlugin $exceptionPlugin) { + $authBackend = new Auth($user, $password); + $authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend); + + $server = new Server(); + $server->setBaseUri('/'); + $server->addPlugin($authPlugin); + $server->addPlugin(new LockPlugin()); + $server->addPlugin(new ChunkingV2Plugin(\OCP\Server::get(ICacheFactory::class))); + $server->addPlugin(new ChunkingPlugin()); + $server->addPlugin($exceptionPlugin); + + // like ServerFactory, build the tree after authentication set up the + // session and the filesystem + $treeBuilt = false; + $server->on('beforeMethod:*', function () use ($server, $view, $user, &$treeBuilt): void { + if ($treeBuilt) { + return; + } + $treeBuilt = true; + $filesDir = new Directory($view, $view->getFileInfo('')); + $uploads = $this->buildUploadsCollection($user); + $server->tree = new CachingTree(new SimpleCollection('root', [$filesDir, $uploads])); + }, 30); + + return $server; + } + + /** + * The `uploads` branch of the tree, resolving to the real UploadHome the + * production RootCollection would return for this principal. + */ + private function buildUploadsCollection(string $user): ICollection { + return new class($user) implements ICollection { + private ?UploadHome $home = null; + + public function __construct( + private string $user, + ) { + } + + private function home(): UploadHome { + if ($this->home === null) { + $this->home = new UploadHome( + ['uri' => 'principals/' . $this->user], + \OCP\Server::get(CleanupService::class), + \OCP\Server::get(IRootFolder::class), + \OCP\Server::get(IUserSession::class), + \OCP\Server::get(\OCP\Share\IManager::class), + ); + } + return $this->home; + } + + public function createFile($name, $data = null) { + throw new Forbidden(); + } + + public function createDirectory($name) { + throw new Forbidden(); + } + + public function getChild($name) { + if ($name === $this->user) { + return $this->home(); + } + throw new NotFound(); + } + + public function getChildren() { + return [$this->home()]; + } + + public function childExists($name) { + return $name === $this->user; + } + + public function delete() { + throw new Forbidden(); + } + + public function getName() { + return 'uploads'; + } + + public function setName($name) { + throw new Forbidden(); + } + + public function getLastModified() { + return 0; + } + }; + } + + private function expectedContent(): string { + $content = ''; + foreach (self::CHUNKS as [$size, $char]) { + $content .= str_repeat($char, $size); + } + return $content; + } + + /** + * MKCOL the upload session and PUT the chunks, like a sync client does. + */ + private function uploadChunks(View $view, string $user, string $transfer): void { + $response = $this->request($view, $user, 'pass', 'MKCOL', '/uploads/' . $user . '/' . $transfer); + $this->assertEquals(Http::STATUS_CREATED, $response->getStatus()); + + foreach (self::CHUNKS as $name => [$size, $char]) { + $response = $this->request( + $view, + $user, + 'pass', + 'PUT', + '/uploads/' . $user . '/' . $transfer . '/' . $name, + str_repeat($char, $size) + ); + $this->assertEquals(Http::STATUS_CREATED, $response->getStatus()); + } + } + + private function moveToDestination(View $view, string $user, string $transfer, string $target) { + return $this->request( + $view, + $user, + 'pass', + 'MOVE', + '/uploads/' . $user . '/' . $transfer . '/.file', + null, + [ + 'Destination' => '/files/' . $target, + 'OC-Total-Length' => (string)strlen($this->expectedContent()), + ] + ); + } + + private function listPartFiles(View $view): array { + [$storage, $internalPath] = $view->resolvePath(''); + $files = []; + foreach (scandir($storage->getLocalFile($internalPath)) as $file) { + if (str_ends_with($file, '.part')) { + $files[] = $file; + } + } + return $files; + } + + public function testAssembleChunkedUpload(): void { + $user = self::getUniqueID(); + $view = $this->setupUser($user, 'pass'); + + $this->uploadChunks($view, $user, 'chunking-42'); + $response = $this->moveToDestination($view, $user, 'chunking-42', 'target.txt'); + + $this->assertEquals(Http::STATUS_CREATED, $response->getStatus()); + $this->assertEquals($this->expectedContent(), $view->file_get_contents('target.txt')); + $this->assertEquals(strlen($this->expectedContent()), $view->getFileInfo('target.txt')->getSize()); + $this->assertEmpty($this->listPartFiles($view), 'No stray part files'); + } + + public function testAssembleChunkedUploadOverwrite(): void { + $user = self::getUniqueID(); + $view = $this->setupUser($user, 'pass'); + + $view->file_put_contents('target.txt', 'the original content'); + + $this->uploadChunks($view, $user, 'chunking-42'); + $response = $this->moveToDestination($view, $user, 'chunking-42', 'target.txt'); + + $this->assertEquals(Http::STATUS_NO_CONTENT, $response->getStatus()); + $this->assertEquals($this->expectedContent(), $view->file_get_contents('target.txt')); + $this->assertEquals(strlen($this->expectedContent()), $view->getFileInfo('target.txt')->getSize()); + $this->assertEmpty($this->listPartFiles($view), 'No stray part files'); + } + + /** + * An assembly whose chunk is shorter on storage than the filecache claims + * must fail loudly and must not touch the existing destination file or its + * catalog entry. + */ + public function testFailedAssemblyKeepsOriginal(): void { + $user = self::getUniqueID(); + $view = $this->setupUser($user, 'pass'); + + $view->file_put_contents('target.txt', 'the original content'); + $originalEtag = $view->getFileInfo('target.txt')->getEtag(); + + $this->uploadChunks($view, $user, 'chunking-42'); + + // truncate a chunk on storage behind the filecache's back, as an upload + // interrupted between write and cache update leaves it + // resolve down to the raw storage: on encrypted storage getLocalFile() + // would hand out a decrypted temporary copy, not the stored file + $uploadsView = new View('/' . $user . '/uploads'); + [$chunkStorage, $chunkInternalPath] = $uploadsView->resolvePath('chunking-42/00002'); + while ($chunkStorage instanceof Wrapper) { + $chunkStorage = $chunkStorage->getWrapperStorage(); + } + $this->assertInstanceOf(Local::class, $chunkStorage); + $chunkFile = $chunkStorage->getSourcePath($chunkInternalPath); + $handle = fopen($chunkFile, 'r+'); + // truncate on a block boundary so that an encrypted chunk still decrypts + // cleanly, just short - matching a partially flushed write + ftruncate($handle, 8192 * 11); + fclose($handle); + + try { + $status = $this->moveToDestination($view, $user, 'chunking-42', 'target.txt')->getStatus(); + } catch (DavException $e) { + // the failure made it out of the request as an exception instead + $status = $e->getHTTPCode(); + } + // assert outside the try: catching \Exception here would also swallow + // PHPUnit's own assertion failures + $this->assertGreaterThanOrEqual(400, $status, 'A truncated assembly must not report success'); + + $this->assertEquals('the original content', $view->file_get_contents('target.txt')); + $this->assertEquals(strlen('the original content'), $view->getFileInfo('target.txt')->getSize()); + $this->assertEquals($originalEtag, $view->getFileInfo('target.txt')->getEtag()); + $this->assertEmpty($this->listPartFiles($view), 'No stray part files'); + } +} diff --git a/apps/dav/tests/unit/Connector/Sabre/RequestTest/EncryptionUserKeyChunkedUploadAssemblyTest.php b/apps/dav/tests/unit/Connector/Sabre/RequestTest/EncryptionUserKeyChunkedUploadAssemblyTest.php new file mode 100644 index 0000000000000..d2d519630f4bd --- /dev/null +++ b/apps/dav/tests/unit/Connector/Sabre/RequestTest/EncryptionUserKeyChunkedUploadAssemblyTest.php @@ -0,0 +1,40 @@ +createUser($name, $password); + $tmpFolder = Server::get(ITempManager::class)->getTemporaryFolder(); + $this->registerMount($name, '\OC\Files\Storage\Local', '/' . $name, ['datadir' => $tmpFolder]); + // we use per-user keys + Server::get(IAppConfig::class)->setValueBool('encryption', 'useMasterKey', false); + $this->setupForUser($name, $password); + $this->loginWithEncryption($name); + return new View('/' . $name . '/files'); + } +}