From c597337bff40918d2c8618a664619befa9b4dd24 Mon Sep 17 00:00:00 2001 From: Josh Date: Thu, 6 Aug 2026 10:06:33 -0400 Subject: [PATCH 1/4] fix(files_trashbin): clarify Trashbin path handling contracts Signed-off-by: Josh --- apps/files_trashbin/lib/Trashbin.php | 29 +++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index 41301c84f41d6..2b31d2ccce4c4 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/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" suffix for a root item. + * The path must not start with "/". + * @param string $filename Original filename or folder name, without the + * ".d" 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" 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(); From 27b5788e312963ed011533c3b52269febdcb5bae Mon Sep 17 00:00:00 2001 From: Josh Date: Thu, 6 Aug 2026 10:10:17 -0400 Subject: [PATCH 2/4] fix(files_trashbin): normalize trash paths before restore and delete Signed-off-by: Josh --- apps/files_trashbin/lib/Trash/LegacyTrashBackend.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/files_trashbin/lib/Trash/LegacyTrashBackend.php b/apps/files_trashbin/lib/Trash/LegacyTrashBackend.php index da6d5f213f17c..d86a75a5b02ae 100644 --- a/apps/files_trashbin/lib/Trash/LegacyTrashBackend.php +++ b/apps/files_trashbin/lib/Trash/LegacyTrashBackend.php @@ -78,12 +78,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()); From 3b81c23f8d67afbb2de3a9c9d96e42c05c37af25 Mon Sep 17 00:00:00 2001 From: Josh Date: Thu, 6 Aug 2026 10:46:10 -0400 Subject: [PATCH 3/4] docs(files_trashbin): document and explain LegacyTrashBackend In particular the "legacy" part is a bit confusing (and can be misconstrued) until realizing it's a essentially wrapper... Signed-off-by: Josh --- apps/files_trashbin/lib/Trash/LegacyTrashBackend.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/apps/files_trashbin/lib/Trash/LegacyTrashBackend.php b/apps/files_trashbin/lib/Trash/LegacyTrashBackend.php index d86a75a5b02ae..399c65323fe1a 100644 --- a/apps/files_trashbin/lib/Trash/LegacyTrashBackend.php +++ b/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 */ private array $deletedFiles = []; From c492032b5ee772b0d01527fd97bb0c770cd3c17e Mon Sep 17 00:00:00 2001 From: Josh Date: Thu, 6 Aug 2026 11:14:15 -0400 Subject: [PATCH 4/4] test(files_trashbin): cover canonical paths for trash restore and deletion Signed-off-by: Josh --- apps/files_trashbin/tests/TrashbinTest.php | 180 +++++++++++++++++++++ 1 file changed, 180 insertions(+) diff --git a/apps/files_trashbin/tests/TrashbinTest.php b/apps/files_trashbin/tests/TrashbinTest.php index b6bb552517773..60008ac3863dd 100644 --- a/apps/files_trashbin/tests/TrashbinTest.php +++ b/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 */