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>

Author: solracsf

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;

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/');