fix(dav): keep part files when overwriting on interrupted uploads

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 {
@@ -248,7 +273,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)',
@@ -407,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;

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);
 				}

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");
+		}
 	}
 }

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;
@@ -21,14 +22,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 +533,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 +1090,51 @@ 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/');