Skip to content

Commit

Permalink
fix(trashbin): Truncate long filenames
Browse files Browse the repository at this point in the history 
Signed-off-by: Christopher Ng <chrng8@gmail.com>
  • Loading branch information
@Pytal @backportbot-nextcloud
Pytal authored and backportbot-nextcloud[bot] committed Jun 1, 2023
1 parent 08758c0 commit ad1052d
Show file tree
Hide file tree
Showing 6 changed files with 88 additions and 19 deletions.
2 changes: 1 addition & 1 deletion apps/files_trashbin/lib/Command/RestoreAllFiles.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ protected function restoreDeletedFiles(string $uid, OutputInterface $output): vo
$timestamp = $trashFile->getMtime();
$humanTime = $this->l10n->l('datetime', $timestamp);
$output->write("File <info>$filename</info> originally deleted at <info>$humanTime</info> ");
$file = $filename . '.d' . $timestamp;
$file = Trashbin::getTrashFilename($filename, $timestamp);
$location = Trashbin::getLocation($uid, $filename, (string) $timestamp);
if ($location === '.') {
$location = '';
Expand Down
6 changes: 4 additions & 2 deletions apps/files_trashbin/lib/Sabre/TrashFile.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,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());
}
}
4 changes: 3 additions & 1 deletion apps/files_trashbin/lib/Sabre/TrashFolder.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,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());
}
}
3 changes: 2 additions & 1 deletion apps/files_trashbin/lib/Trash/LegacyTrashBackend.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,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
);
Expand Down
47 changes: 33 additions & 14 deletions apps/files_trashbin/lib/Trashbin.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,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;
Expand Down Expand Up @@ -270,7 +270,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) {
Expand All @@ -286,7 +286,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);
}
}

Expand Down Expand Up @@ -351,7 +351,7 @@ public static function move2trash($file_path, $ownerOnly = false) {
\OC::$server->get(LoggerInterface::class)->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);

Expand Down Expand Up @@ -388,15 +388,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));
}
}
}
Expand Down Expand Up @@ -554,7 +554,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);
}
Expand Down Expand Up @@ -655,7 +655,7 @@ public static function delete($filename, $user, $timestamp = null) {
->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
$query->executeStatement();

$file = $filename . '.d' . $timestamp;
$file = static::getTrashFilename($filename, $timestamp);
} else {
$file = $filename;
}
Expand Down Expand Up @@ -698,8 +698,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);
Expand All @@ -722,7 +722,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);
Expand Down Expand Up @@ -1118,4 +1118,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;
}
}
45 changes: 45 additions & 0 deletions apps/files_trashbin/tests/StorageTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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');
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit ad1052d

Please sign in to comment.