From be36f975b4b6b456c7eacf766404ac45085d9f65 Mon Sep 17 00:00:00 2001 From: Christopher Ng Date: Wed, 31 May 2023 19:06:42 -0700 Subject: [PATCH] fix(trashbin): Truncate long filenames Signed-off-by: Christopher Ng --- apps/dav/lib/CardDAV/SystemAddressbook.php | 2 +- apps/files_trashbin/lib/Sabre/TrashFile.php | 6 ++- apps/files_trashbin/lib/Sabre/TrashFolder.php | 4 +- .../lib/Trash/LegacyTrashBackend.php | 3 +- apps/files_trashbin/lib/Trashbin.php | 47 +++++++++++++------ apps/files_trashbin/tests/StorageTest.php | 45 ++++++++++++++++++ 6 files changed, 88 insertions(+), 19 deletions(-) diff --git a/apps/dav/lib/CardDAV/SystemAddressbook.php b/apps/dav/lib/CardDAV/SystemAddressbook.php index d088031ae9c17..fbd3c17830bb0 100644 --- a/apps/dav/lib/CardDAV/SystemAddressbook.php +++ b/apps/dav/lib/CardDAV/SystemAddressbook.php @@ -52,7 +52,7 @@ public function getChildren() { } public function getACL() { - return array_filter(parent::getACL(), function($acl) { + return array_filter(parent::getACL(), function ($acl) { if (in_array($acl['privilege'], ['{DAV:}write', '{DAV:}all'], true)) { return false; } diff --git a/apps/files_trashbin/lib/Sabre/TrashFile.php b/apps/files_trashbin/lib/Sabre/TrashFile.php index fb5d8bf1fc372..e1e6c01077519 100644 --- a/apps/files_trashbin/lib/Sabre/TrashFile.php +++ b/apps/files_trashbin/lib/Sabre/TrashFile.php @@ -27,12 +27,14 @@ namespace OCA\Files_Trashbin\Sabre; +use OCA\Files_Trashbin\Trashbin; + class TrashFile extends AbstractTrashFile { public function get() { - return $this->data->getStorage()->fopen($this->data->getInternalPath() . '.d' . $this->getLastModified(), 'rb'); + return $this->data->getStorage()->fopen(Trashbin::getTrashFilename($this->data->getInternalPath(), $this->getLastModified()), 'rb'); } public function getName(): string { - return $this->data->getName() . '.d' . $this->getLastModified(); + return Trashbin::getTrashFilename($this->data->getName(), $this->getLastModified()); } } diff --git a/apps/files_trashbin/lib/Sabre/TrashFolder.php b/apps/files_trashbin/lib/Sabre/TrashFolder.php index ae654b7daedb7..c66aca5ebcd09 100644 --- a/apps/files_trashbin/lib/Sabre/TrashFolder.php +++ b/apps/files_trashbin/lib/Sabre/TrashFolder.php @@ -27,8 +27,10 @@ namespace OCA\Files_Trashbin\Sabre; +use OCA\Files_Trashbin\Trashbin; + class TrashFolder extends AbstractTrashFolder { public function getName(): string { - return $this->data->getName() . '.d' . $this->getLastModified(); + return Trashbin::getTrashFilename($this->data->getName(), $this->getLastModified()); } } diff --git a/apps/files_trashbin/lib/Trash/LegacyTrashBackend.php b/apps/files_trashbin/lib/Trash/LegacyTrashBackend.php index 68ee8c1c517b9..29ccedde385dc 100644 --- a/apps/files_trashbin/lib/Trash/LegacyTrashBackend.php +++ b/apps/files_trashbin/lib/Trash/LegacyTrashBackend.php @@ -59,11 +59,12 @@ private function mapTrashItems(array $items, IUser $user, ITrashItem $parent = n if (!$originalLocation) { $originalLocation = $file->getName(); } + $trashFilename = Trashbin::getTrashFilename($file->getName(), $file->getMtime()); return new TrashItem( $this, $originalLocation, $file->getMTime(), - $parentTrashPath . '/' . $file->getName() . ($isRoot ? '.d' . $file->getMtime() : ''), + $parentTrashPath . '/' . ($isRoot ? $trashFilename : $file->getName()), $file, $user ); diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index 91f3cec21ee78..9f5d4d68f04b7 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -208,8 +208,8 @@ private static function copyFilesToUser($sourcePath, $owner, $targetPath, $user, $view = new View('/'); - $target = $user . '/files_trashbin/files/' . $targetFilename . '.d' . $timestamp; - $source = $owner . '/files_trashbin/files/' . $sourceFilename . '.d' . $timestamp; + $target = $user . '/files_trashbin/files/' . static::getTrashFilename($targetFilename, $timestamp); + $source = $owner . '/files_trashbin/files/' . static::getTrashFilename($sourceFilename, $timestamp); $free = $view->free_space($target); $isUnknownOrUnlimitedFreeSpace = $free < 0; $isEnoughFreeSpaceLeft = $view->filesize($source) < $free; @@ -276,7 +276,7 @@ public static function move2trash($file_path, $ownerOnly = false) { $lockingProvider = \OC::$server->getLockingProvider(); // disable proxy to prevent recursive calls - $trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp; + $trashPath = '/files_trashbin/files/' . static::getTrashFilename($filename, $timestamp); $gotLock = false; while (!$gotLock) { @@ -292,7 +292,7 @@ public static function move2trash($file_path, $ownerOnly = false) { $timestamp = $timestamp + 1; - $trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp; + $trashPath = '/files_trashbin/files/' . static::getTrashFilename($filename, $timestamp); } } @@ -357,7 +357,7 @@ public static function move2trash($file_path, $ownerOnly = false) { \OC::$server->getLogger()->error('trash bin database couldn\'t be updated', ['app' => 'files_trashbin']); } \OCP\Util::emitHook('\OCA\Files_Trashbin\Trashbin', 'post_moveToTrash', ['filePath' => Filesystem::normalizePath($file_path), - 'trashPath' => Filesystem::normalizePath($filename . '.d' . $timestamp)]); + 'trashPath' => Filesystem::normalizePath(static::getTrashFilename($filename, $timestamp))]); self::retainVersions($filename, $owner, $ownerPath, $timestamp); @@ -394,15 +394,15 @@ private static function retainVersions($filename, $owner, $ownerPath, $timestamp if ($rootView->is_dir($owner . '/files_versions/' . $ownerPath)) { if ($owner !== $user) { - self::copy_recursive($owner . '/files_versions/' . $ownerPath, $owner . '/files_trashbin/versions/' . basename($ownerPath) . '.d' . $timestamp, $rootView); + self::copy_recursive($owner . '/files_versions/' . $ownerPath, $owner . '/files_trashbin/versions/' . static::getTrashFilename(basename($ownerPath), $timestamp), $rootView); } - self::move($rootView, $owner . '/files_versions/' . $ownerPath, $user . '/files_trashbin/versions/' . $filename . '.d' . $timestamp); + self::move($rootView, $owner . '/files_versions/' . $ownerPath, $user . '/files_trashbin/versions/' . static::getTrashFilename($filename, $timestamp)); } elseif ($versions = \OCA\Files_Versions\Storage::getVersions($owner, $ownerPath)) { foreach ($versions as $v) { if ($owner !== $user) { - self::copy($rootView, $owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $owner . '/files_trashbin/versions/' . $v['name'] . '.v' . $v['version'] . '.d' . $timestamp); + self::copy($rootView, $owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $owner . '/files_trashbin/versions/' . static::getTrashFilename($v['name'] . '.v' . $v['version'], $timestamp)); } - self::move($rootView, $owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $user . '/files_trashbin/versions/' . $filename . '.v' . $v['version'] . '.d' . $timestamp); + self::move($rootView, $owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $user . '/files_trashbin/versions/' . static::getTrashFilename($filename . '.v' . $v['version'], $timestamp)); } } } @@ -560,7 +560,7 @@ private static function restoreVersions(View $view, $file, $filename, $uniqueFil } elseif ($versions = self::getVersionsFromTrash($versionedFile, $timestamp, $user)) { foreach ($versions as $v) { if ($timestamp) { - $rootView->rename($user . '/files_trashbin/versions/' . $versionedFile . '.v' . $v . '.d' . $timestamp, $owner . '/files_versions/' . $ownerPath . '.v' . $v); + $rootView->rename($user . '/files_trashbin/versions/' . static::getTrashFilename($versionedFile . '.v' . $v, $timestamp), $owner . '/files_versions/' . $ownerPath . '.v' . $v); } else { $rootView->rename($user . '/files_trashbin/versions/' . $versionedFile . '.v' . $v, $owner . '/files_versions/' . $ownerPath . '.v' . $v); } @@ -661,7 +661,7 @@ public static function delete($filename, $user, $timestamp = null) { ->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp))); $query->execute(); - $file = $filename . '.d' . $timestamp; + $file = static::getTrashFilename($filename, $timestamp); } else { $file = $filename; } @@ -704,8 +704,8 @@ private static function deleteVersions(View $view, $file, $filename, $timestamp, } elseif ($versions = self::getVersionsFromTrash($filename, $timestamp, $user)) { foreach ($versions as $v) { if ($timestamp) { - $size += $view->filesize('/files_trashbin/versions/' . $filename . '.v' . $v . '.d' . $timestamp); - $view->unlink('/files_trashbin/versions/' . $filename . '.v' . $v . '.d' . $timestamp); + $size += $view->filesize('/files_trashbin/versions/' . static::getTrashFilename($filename . '.v' . $v, $timestamp)); + $view->unlink('/files_trashbin/versions/' . static::getTrashFilename($filename . '.v' . $v, $timestamp)); } else { $size += $view->filesize('/files_trashbin/versions/' . $filename . '.v' . $v); $view->unlink('/files_trashbin/versions/' . $filename . '.v' . $v); @@ -728,7 +728,7 @@ public static function file_exists($filename, $timestamp = null) { $view = new View('/' . $user); if ($timestamp) { - $filename = $filename . '.d' . $timestamp; + $filename = static::getTrashFilename($filename, $timestamp); } $target = Filesystem::normalizePath('files_trashbin/files/' . $filename); @@ -1126,4 +1126,23 @@ public static function isEmpty($user) { public static function preview_icon($path) { return \OC::$server->getURLGenerator()->linkToRoute('core_ajax_trashbin_preview', ['x' => 32, 'y' => 32, 'file' => $path]); } + + /** + * Return the filename used in the trash bin + */ + public static function getTrashFilename(string $filename, int $timestamp): string { + $trashFilename = $filename . '.d' . $timestamp; + $length = strlen($trashFilename); + // oc_filecache `name` column has a limit of 250 chars + $maxLength = 250; + if ($length > $maxLength) { + $trashFilename = substr_replace( + $trashFilename, + '', + $maxLength / 2, + $length - $maxLength + ); + } + return $trashFilename; + } } diff --git a/apps/files_trashbin/tests/StorageTest.php b/apps/files_trashbin/tests/StorageTest.php index d32f4c32f3759..e5f2ecf2a837a 100644 --- a/apps/files_trashbin/tests/StorageTest.php +++ b/apps/files_trashbin/tests/StorageTest.php @@ -88,6 +88,11 @@ class StorageTest extends \Test\TestCase { */ private $userView; + // 239 chars so appended timestamp of 12 chars will exceed max length of 250 chars + private const LONG_FILENAME = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.txt'; + // 250 chars + private const MAX_FILENAME = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.txt'; + protected function setUp(): void { parent::setUp(); @@ -107,6 +112,8 @@ protected function setUp(): void { $this->rootView = new \OC\Files\View('/'); $this->userView = new \OC\Files\View('/' . $this->user . '/files/'); $this->userView->file_put_contents('test.txt', 'foo'); + $this->userView->file_put_contents(static::LONG_FILENAME, 'foo'); + $this->userView->file_put_contents(static::MAX_FILENAME, 'foo'); $this->userView->mkdir('folder'); $this->userView->file_put_contents('folder/inside.txt', 'bar'); @@ -162,6 +169,44 @@ public function testSingleStorageDeleteFolder() { $this->assertEquals('inside.txt', $name); } + /** + * Test that deleting a file with a long filename puts it into the trashbin. + */ + public function testSingleStorageDeleteLongFilename() { + $truncatedFilename = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.txt'; + + $this->assertTrue($this->userView->file_exists(static::LONG_FILENAME)); + $this->userView->unlink(static::LONG_FILENAME); + [$storage,] = $this->userView->resolvePath(static::LONG_FILENAME); + $storage->getScanner()->scan(''); // make sure we check the storage + $this->assertFalse($this->userView->getFileInfo(static::LONG_FILENAME)); + + // check if file is in trashbin + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/'); + $this->assertEquals(1, count($results)); + $name = $results[0]->getName(); + $this->assertEquals($truncatedFilename, substr($name, 0, strrpos($name, '.'))); + } + + /** + * Test that deleting a file with the max filename length puts it into the trashbin. + */ + public function testSingleStorageDeleteMaxLengthFilename() { + $truncatedFilename = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.txt'; + + $this->assertTrue($this->userView->file_exists(static::MAX_FILENAME)); + $this->userView->unlink(static::MAX_FILENAME); + [$storage,] = $this->userView->resolvePath(static::MAX_FILENAME); + $storage->getScanner()->scan(''); // make sure we check the storage + $this->assertFalse($this->userView->getFileInfo(static::MAX_FILENAME)); + + // check if file is in trashbin + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/'); + $this->assertEquals(1, count($results)); + $name = $results[0]->getName(); + $this->assertEquals($truncatedFilename, substr($name, 0, strrpos($name, '.'))); + } + /** * Test that deleting a file from another mounted storage properly * lands in the trashbin. This is a cross-storage situation because