fix(files_trashbin): normalize paths before restoring and deleting

apps/files_trashbin/lib/Trash/LegacyTrashBackend.php

@@ -22,6 +22,17 @@
 use OCP\IUser;
 use OCP\IUserManager;
 
+/**
+ * Default backend provided by files_trashbin.
+ *
+ * This backend is registered for the generic IStorage interface. Specialized
+ * backends can be registered for specific storage types and take precedence
+ * when handling those storages.
+ *
+ * The "legacy" designation refers to the files_trashbin storage layout and
+ * static Trashbin API that this class wraps and exposes through the
+ * ITrashBackend interface, not to this backend being unused or deprecated.
+ */
 class LegacyTrashBackend implements ITrashBackend {
 	/** @var array<string, string> */
 	private array $deletedFiles = [];
@@ -78,12 +89,14 @@ public function listTrashFolder(ITrashItem $folder): array {
 
 	#[\Override]
 	public function restoreItem(ITrashItem $item) {
-		Trashbin::restore($item->getTrashPath(), $item->getName(), $item->isRootItem() ? $item->getDeletedTime() : null);
+		Trashbin::restore(ltrim($item->getTrashPath(), '/'), $item->getName(), $item->isRootItem() ? $item->getDeletedTime() : null);
 	}
 
 	#[\Override]
 	public function removeItem(ITrashItem $item) {
 		$user = $item->getUser();
+		$trashPath = ltrim($item->getTrashPath(), '/');
+
 		if ($item->isRootItem()) {
 			$path = substr($item->getTrashPath(), 0, -strlen('.d' . $item->getDeletedTime()));
 			Trashbin::delete($path, $user->getUID(), $item->getDeletedTime());

apps/files_trashbin/lib/Trashbin.php

@@ -515,14 +515,17 @@ private static function copy(View $view, $source, $target) {
 	}
 
 	/**
-	 * Restore a file or folder from trash bin
+	 * Restore a file or folder from the trash bin.
 	 *
-	 * @param string $file path to the deleted file/folder relative to "files_trashbin/files/",
-	 *                     including the timestamp suffix ".d12345678"
-	 * @param string $filename name of the file/folder
-	 * @param int $timestamp time when the file/folder was deleted
+	 * @param string $file Stored trash path relative to "files_trashbin/files/",
+	 *                     including the ".d<timestamp>" suffix for a root item.
+	 *                     The path must not start with "/".
+	 * @param string $filename Original filename or folder name, without the
+	 *                         ".d<timestamp>" suffix.
+	 * @param int|null $timestamp Deletion timestamp for a root trash item;
+	 *                            null when restoring an item below a root trash item.
 	 *
-	 * @return bool true on success, false otherwise
+	 * @return bool Whether the item was restored successfully.
 	 */
 	public static function restore($file, $filename, $timestamp) {
 		$user = OC_User::getUser();
@@ -732,13 +735,17 @@ protected static function emitTrashbinPostDelete($path) {
 	}
 
 	/**
-	 * delete file from trash bin permanently
+	 * Delete a file or folder permanently from the trash bin.
 	 *
-	 * @param string $filename path to the file
-	 * @param string $user
-	 * @param int $timestamp of deletion time
+	 * @param string $filename Path relative to "files_trashbin/files/".
+	 *                         The path must not start with "/".
+	 *                         If $timestamp is provided, this is the original
+	 *                         filename/path without the ".d<timestamp>" suffix.
+	 *                         If $timestamp is null, this is the stored trash path.
+	 * @param string $user User owning the trash bin.
+	 * @param int|null $timestamp Deletion timestamp for a root trash item.
 	 *
-	 * @return int|float size of deleted files
+	 * @return int|float Size of deleted files.
 	 */
 	public static function delete($filename, $user, $timestamp = null) {
 		$userRoot = \OC::$server->getUserFolder($user)->getParent();

apps/files_trashbin/tests/TrashbinTest.php

@@ -20,6 +20,7 @@
 use OCA\Files_Trashbin\Expiration;
 use OCA\Files_Trashbin\Helper;
 use OCA\Files_Trashbin\Storage;
+use OCA\Files_Trashbin\Trash\ITrashManager;
 use OCA\Files_Trashbin\Trashbin;
 use OCP\App\IAppManager;
 use OCP\AppFramework\Utility\ITimeFactory;
@@ -47,6 +48,8 @@ class TrashbinTest extends \Test\TestCase {
 	private static $rememberRetentionObligation;
 	private static bool $trashBinStatus;
 	private View $rootView;
+	private array $trashDeleteHookParams = [];
+	private array $trashRestoreHookParams = [];
 
 	public static function setUpBeforeClass(): void {
 		parent::setUpBeforeClass();
@@ -349,6 +352,183 @@ public function testExpireOldFilesUtilLimitsAreMet(): void {
 		$this->assertSame('file1.txt', $element['name']);
 	}
 
+	private function getTrashRow(string $filename, int $timestamp): array|false {
+		$connection = Server::get(IDBConnection::class);
+		$query = $connection->getQueryBuilder();
+
+		return $query
+			->select('id', 'timestamp')
+			->from('files_trash')
+			->where(
+				$query->expr()->eq(
+					'user',
+					$query->createNamedParameter(self::TEST_TRASHBIN_USER1),
+				)
+			)
+			->andWhere(
+				$query->expr()->eq(
+					'id',
+					$query->createNamedParameter($filename),
+				)
+			)
+			->andWhere(
+				$query->expr()->eq(
+					'timestamp',
+					$query->createNamedParameter($timestamp),
+				)
+			)
+			->executeQuery()
+			->fetchAssociative();
+	}
+
+	public function captureTrashDeleteHook(array $params): void {
+		$this->trashDeleteHookParams[] = $params;
+	}
+
+	public function captureTrashRestoreHook(array $params): void {
+		$this->trashRestoreHookParams[] = $params;
+	}
+
+	/**
+	 * Permanent deletion through the trash manager removes the physical trash
+	 * item and its files_trash metadata row.
+	 */
+	public function testRemoveItemRemovesMetadataAndEmitsCanonicalPath(): void {
+		$userManager = Server::get(IUserManager::class);
+		$user = $userManager->get(self::TEST_TRASHBIN_USER1);
+		$this->assertNotNull($user);
+
+		$userFolder = Server::get(IRootFolder::class)
+			->getUserFolder(self::TEST_TRASHBIN_USER1);
+
+		$file = $userFolder->newFile('file1.txt');
+		$file->putContent('foo');
+		$file->delete();
+
+		$filesInTrash = Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1);
+		$this->assertCount(1, $filesInTrash);
+
+		/** @var FileInfo $trashedFile */
+		$trashedFile = $filesInTrash[0];
+		$timestamp = $trashedFile->getMtime();
+
+		$this->assertNotFalse(
+			$this->getTrashRow('file1.txt', $timestamp),
+		);
+
+		$trashManager = Server::get(ITrashManager::class);
+		$trashItem = $trashManager->getTrashRootItem($user, 'file1.txt');
+
+		$this->assertNotNull($trashItem);
+		$this->assertSame(
+			'/file1.txt.d' . $timestamp,
+			$trashItem->getTrashPath(),
+		);
+
+		$this->trashDeleteHookParams = [];
+		\OC_Hook::connect(
+			'\OCP\Trashbin',
+			'delete',
+			$this,
+			'captureTrashDeleteHook',
+		);
+
+		$trashManager->removeItem($trashItem);
+
+		$this->assertFalse(
+			$this->rootView->file_exists(
+				$this->trashRoot1 . '/files/file1.txt.d' . $timestamp,
+			),
+		);
+
+		$this->assertFalse(
+			$this->getTrashRow('file1.txt', $timestamp),
+		);
+
+		$this->assertCount(1, $this->trashDeleteHookParams);
+		$this->assertSame(
+			'/files_trashbin/files/file1.txt.d' . $timestamp,
+			$this->trashDeleteHookParams[0]['path'],
+		);
+		$this->assertStringNotContainsString(
+			'//',
+			$this->trashDeleteHookParams[0]['path'],
+		);
+	}
+
+	/**
+	 * Restore through the trash manager accepts the ITrashItem path representation
+	 * and emits a canonical trash path.
+	 */
+	public function testRestoreItemEmitsCanonicalPathAndRemovesMetadata(): void {
+		$userManager = Server::get(IUserManager::class);
+		$user = $userManager->get(self::TEST_TRASHBIN_USER1);
+		$this->assertNotNull($user);
+
+		$userFolder = Server::get(IRootFolder::class)
+			->getUserFolder(self::TEST_TRASHBIN_USER1);
+
+		$file = $userFolder->newFile('file1.txt');
+		$file->putContent('foo');
+		$file->delete();
+
+		$filesInTrash = Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1);
+		$this->assertCount(1, $filesInTrash);
+
+		/** @var FileInfo $trashedFile */
+		$trashedFile = $filesInTrash[0];
+		$timestamp = $trashedFile->getMtime();
+
+		$this->assertNotFalse(
+			$this->getTrashRow('file1.txt', $timestamp),
+		);
+
+		$trashManager = Server::get(ITrashManager::class);
+		$trashItem = $trashManager->getTrashRootItem($user, 'file1.txt');
+
+		$this->assertNotNull($trashItem);
+		$this->assertSame(
+			'/file1.txt.d' . $timestamp,
+			$trashItem->getTrashPath(),
+		);
+
+		$this->trashRestoreHookParams = [];
+		\OC_Hook::connect(
+			'\OCA\Files_Trashbin\Trashbin',
+			'post_restore',
+			$this,
+			'captureTrashRestoreHook',
+		);
+
+		$trashManager->restoreItem($trashItem);
+
+		$this->assertTrue($userFolder->nodeExists('file1.txt'));
+		$this->assertSame(
+			'foo',
+			$userFolder->get('file1.txt')->getContent(),
+		);
+
+		$this->assertFalse(
+			$this->rootView->file_exists(
+				$this->trashRoot1 . '/files/file1.txt.d' . $timestamp,
+			),
+		);
+
+		$this->assertFalse(
+			$this->getTrashRow('file1.txt', $timestamp),
+		);
+
+		$this->assertCount(1, $this->trashRestoreHookParams);
+		$this->assertSame(
+			'/file1.txt.d' . $timestamp,
+			$this->trashRestoreHookParams[0]['trashPath'],
+		);
+		$this->assertStringNotContainsString(
+			'//',
+			$this->trashRestoreHookParams[0]['trashPath'],
+		);
+	}
+
 	/**
 	 * Test restoring a file
 	 */