Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stable25] fix(trashbin): Truncate long filenames #38576

Merged
merged 1 commit into from
Jun 5, 2023
Merged

[stable25] fix(trashbin): Truncate long filenames #38576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/files_trashbin/lib/Command/RestoreAllFiles.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,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 @@ -278,7 +278,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 @@ -294,7 +294,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 @@ -358,7 +358,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 @@ -395,15 +395,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 @@ -561,7 +561,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 @@ -662,7 +662,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 @@ -705,8 +705,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 @@ -729,7 +729,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 @@ -1125,4 +1125,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,

Check failure

Code scanning / Psalm

InvalidScalarArgument

Argument 3 of substr_replace expects array<array-key, int>|int, float|int provided
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Psalm error seems legit, this should be an int. How come it did not complain on original PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case maxLength is always 250 so the result is always int and can be ignored

But the alert likely didn't show initially because it exceeded the limit, see https://github.com/nextcloud/server/security/code-scanning/tools/Psalm/status/configurations/actions-FZTWS5DIOVRC653POJVWM3DPO5ZS643UMF2GSYZNMNXWIZJNMFXGC3DZONUXGLTZNVWA/ce2b36315853290650a327094ccdb926d691e375530fc54471e6d340970bbb9c

$length - $maxLength
);
}
return $trashFilename;
}
}
45 changes: 45 additions & 0 deletions apps/files_trashbin/tests/StorageTest.php
View file
Open in desktop
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