fix: remove just uploaded files in case of error - also for storages which do not support part files (#40892)

Author: DeepDiver1975

apps/dav/lib/Connector/Sabre/File.php

@@ -187,9 +187,7 @@ public function put($data) {
 			try {
 				$this->changeLock(ILockingProvider::LOCK_EXCLUSIVE);
 			} catch (LockedException $e) {
-				if ($usePartFile) {
-					$partStorage->unlink($internalPartPath);
-				}
+				$this->cleanFailedUpload($partStorage, $internalPartPath);
 				throw new FileLocked($e->getMessage(), $e->getCode(), $e);
 			}
 
@@ -234,9 +232,7 @@ public function put($data) {
 				}
 			}
 		} catch (\Exception $e) {
-			if ($usePartFile) {
-				$partStorage->unlink($internalPartPath);
-			}
+			$this->cleanFailedUpload($partStorage, $internalPartPath);
 			$this->convertToSabreException($e);
 		}
 
@@ -255,9 +251,7 @@ public function put($data) {
 			try {
 				$this->changeLock(ILockingProvider::LOCK_EXCLUSIVE);
 			} catch (LockedException $e) {
-				if ($usePartFile) {
-					$partStorage->unlink($internalPartPath);
-				}
+				$this->cleanFailedUpload($partStorage, $internalPartPath);
 				throw new FileLocked($e->getMessage(), $e->getCode(), $e);
 			}
 
@@ -633,9 +627,7 @@ private function createFileChunked($data) {
 				}
 				return $etag;
 			} catch (\Exception $e) {
-				if ($partFile !== null) {
-					$targetStorage->unlink($targetInternalPath);
-				}
+				$this->cleanFailedUpload($targetStorage, $targetInternalPath);
 				$this->convertToSabreException($e);
 			}
 		}
@@ -767,4 +759,23 @@ protected function header($string) {
 	public function getNode() {
 		return \OC::$server->getRootFolder()->get($this->getFileInfo()->getPath());
 	}
+
+	private function cleanFailedUpload(Storage $partStorage, $internalPartPath): void {
+		if ($partStorage->file_exists($internalPartPath)) {
+			try {
+				# broken/uncompleted uploaded files shall not go into trash-bin
+				if (class_exists(\OCA\Files_Trashbin\Storage::class)) {
+					\OCA\Files_Trashbin\Storage::$disableTrash = true;
+				}
+				# delete file from storage
+				$partStorage->unlink($internalPartPath);
+				# delete file from cache
+				$partStorage->getCache()->remove($internalPartPath);
+			} finally {
+				if (class_exists(\OCA\Files_Trashbin\Storage::class)) {
+					\OCA\Files_Trashbin\Storage::$disableTrash = false;
+				}
+			}
+		}
+	}
 }

apps/dav/tests/unit/Upload/FailedUploadTest.php

@@ -0,0 +1,73 @@
+<?php
+/**
+ * @author Thomas Müller <thomas.mueller@tmit.eu>
+ *
+ * @copyright Copyright (c) 2023, ownCloud GmbH
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+namespace OCA\DAV\Tests\unit\Upload;
+
+use OC\Files\FileInfo;
+use OC\Files\View;
+use OCA\DAV\Connector\Sabre\File;
+use OCP\Lock\ILockingProvider;
+use Sabre\DAV\Exception\BadRequest;
+use Test\TestCase;
+use Test\Traits\UserTrait;
+
+/**
+ * @group DB
+ */
+class FailedUploadTest extends TestCase {
+	use UserTrait;
+
+	public function test(): void {
+		# init
+		$user = $this->createUser('user');
+		$folder = \OC::$server->getUserFolder($user->getUID());
+
+		# fake request
+		$_SERVER['CONTENT_LENGTH'] = 12;
+		$_SERVER['REQUEST_METHOD'] = 'PUT';
+		unset($_SERVER['HTTP_OC_CHUNKED']);
+
+		# perform the request
+		$path = '/test.txt';
+		$info = new FileInfo("user/files/$path", null, null, [], null);
+		$view = new View();
+		$file = new File($view, $info);
+		$file->acquireLock(ILockingProvider::LOCK_SHARED);
+		$stream = fopen('data://text/plain,' . '123456', 'rb');
+		try {
+			$file->put($stream);
+		} catch (BadRequest $e) {
+			self::assertEquals('expected filesize 12 got 6', $e->getMessage());
+		}
+
+		# assert file does not exist
+		self::assertFalse($folder->nodeExists($path));
+		# ensure folder can ge listed
+		$children = $folder->getDirectoryListing();
+		self::assertCount(0, $children);
+
+		# assert there is no file on disk
+		$internalPath = $folder->getInternalPath();
+		self::assertFalse($folder->getStorage()->file_exists($internalPath.'/test.txt'));
+
+		# assert file is not in cache
+		self::assertFalse($folder->getStorage()->getCache()->inCache($internalPath.'/test.txt'));
+	}
+}

apps/files/js/file-upload.js

@@ -1296,8 +1296,9 @@ OC.Uploader.prototype = _.extend({
 						var message = '';
 						if (upload) {
 							var response = upload.getResponse();
-							message = response.message;
+							message = t('files', 'Failed to upload the file "{fileName}": {error}', {fileName: upload.getFileName(), error: response.message});
 						}
+
 						OC.Notification.show(message || data.errorThrown, {type: 'error'});
 					}
 

apps/files_trashbin/lib/Storage.php

@@ -39,9 +39,12 @@ class Storage extends Wrapper {
 	/**
 	 * Disable trash logic
 	 *
+	 * NOTE: this public for a very specific purpose to handle broken uploads.
+	 * Don't touch this property unless you know what this is doing! :dancers:
+	 *
 	 * @var bool
 	 */
-	private static $disableTrash = false;
+	public static $disableTrash = false;
 
 	/** @var  IUserManager */
 	private $userManager;

changelog/unreleased/40892

@@ -0,0 +1,5 @@
+Bugfix: Cleanup storage and database after failed files uploads
+
+Storage and database is cleaned up of any remaining items in case a files upload fails.
+
+https://github.com/owncloud/core/pull/40892