From 4038aabdee2de5ce2d08aa63e3c8973894f6f83a Mon Sep 17 00:00:00 2001
From: Sybille Peters <sypets@gmx.de>
Date: Thu, 20 Apr 2023 15:26:54 +0200
Subject: [PATCH] [BUGFIX] Do not abort renameFile if filename sanitizable
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It is possible to rename a file with ResourseStorage::renameFile(). This
is also used under the hood, if a file is renamed in the file list.

RenameFile also performs a sanitation of the filename, in case the
filename was not yet sanitized (e.g. convert chars like space, quote,
ö, ê, ß or §).

But if the target filename is the same as the current filename, the
sanitation was not performed. This is now fixed so that the behaviour
is more consistent and sanitizing files with renameFile() is possible.

Resolves: #100683
Releases: main, 12.4
Change-Id: Ia35747f1381f4aac64af50c461e172c9f951cf83
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/79787
Tested-by: Benni Mack <benni@typo3.org>
Tested-by: core-ci <typo3@b13.com>
Reviewed-by: Benni Mack <benni@typo3.org>
---
 .../core/Classes/Resource/ResourceStorage.php |  6 +--
 .../Resource/ResourceStorageTest.php          | 38 +++++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/typo3/sysext/core/Classes/Resource/ResourceStorage.php b/typo3/sysext/core/Classes/Resource/ResourceStorage.php
index bdfcafa48295..86e9e85b781f 100644
--- a/typo3/sysext/core/Classes/Resource/ResourceStorage.php
+++ b/typo3/sysext/core/Classes/Resource/ResourceStorage.php
@@ -2008,11 +2008,11 @@ class ResourceStorage implements ResourceStorageInterface
      */
     public function renameFile($file, $targetFileName, $conflictMode = DuplicationBehavior::RENAME)
     {
-        // The name should be different from the current.
-        if ($file->getName() === $targetFileName) {
+        $sanitizedTargetFileName = $this->driver->sanitizeFileName($targetFileName);
+        // The new name should be different from the current.
+        if ($file->getName() === $sanitizedTargetFileName) {
             return $file;
         }
-        $sanitizedTargetFileName = $this->driver->sanitizeFileName($targetFileName);
         $this->assureFileRenamePermissions($file, $sanitizedTargetFileName);
         $this->eventDispatcher->dispatch(
             new BeforeFileRenamedEvent($file, $sanitizedTargetFileName)
diff --git a/typo3/sysext/core/Tests/Functional/Resource/ResourceStorageTest.php b/typo3/sysext/core/Tests/Functional/Resource/ResourceStorageTest.php
index 251b2c36337a..df3cc0d405e6 100644
--- a/typo3/sysext/core/Tests/Functional/Resource/ResourceStorageTest.php
+++ b/typo3/sysext/core/Tests/Functional/Resource/ResourceStorageTest.php
@@ -22,6 +22,7 @@ use TYPO3\CMS\Core\Resource\Driver\AbstractDriver;
 use TYPO3\CMS\Core\Resource\Driver\LocalDriver;
 use TYPO3\CMS\Core\Resource\File;
 use TYPO3\CMS\Core\Resource\Folder;
+use TYPO3\CMS\Core\Resource\Index\Indexer;
 use TYPO3\CMS\Core\Resource\ResourceFactory;
 use TYPO3\CMS\Core\Resource\ResourceStorage;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
@@ -110,6 +111,7 @@ final class ResourceStorageTest extends FunctionalTestCase
         $localDriver->method('getPermissions')->willReturn($permissionsFromDriver);
         $mockedResourceFactory = $this->createMock(ResourceFactory::class);
         $mockedFolder = $this->createMock(Folder::class);
+
         // Let all other checks pass
         $subject = $this->getMockBuilder(ResourceStorage::class)
             ->onlyMethods(['isWritable', 'isBrowsable', 'checkUserActionPermission', 'getResourceFactoryInstance'])
@@ -252,4 +254,40 @@ final class ResourceStorageTest extends FunctionalTestCase
         $subject->_set('driver', $mockedDriver);
         $subject->deleteFolder($folderMock);
     }
+
+    /**
+     * @test
+     */
+    public function renameFileWillCallRenameFileIfUnsanitizedAndNoChangeInTargetFilename(): void
+    {
+        $driverMock = $this->getMockBuilder(LocalDriver::class)
+            ->onlyMethods(['renameFile', 'sanitizeFileName'])
+            ->disableOriginalConstructor()
+            ->getMock();
+        $driverMock->method('sanitizeFileName')
+            ->willReturn('a_b.jpg');
+        $driverMock->expects(self::once())
+            ->method('renameFile')
+            ->with('/a b.jpg', 'a_b.jpg');
+        $indexerMock = $this->getMockBuilder(Indexer::class)
+            ->onlyMethods(['updateIndexEntry'])
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $subject = $this->getMockBuilder(ResourceStorage::class)
+            ->onlyMethods(['assureFileRenamePermissions', 'getIndexer'])
+            ->setConstructorArgs([$driverMock, ['name' => 'testing'], new NoopEventDispatcher()])
+            ->getMock();
+        $subject->method('getIndexer')
+            ->willReturn($indexerMock);
+        $file = new File(
+            [
+                'identifier' => '/a b.jpg',
+                'name' => 'a b.jpg',
+            ],
+            $subject,
+        );
+        $subject->renameFile($file, 'a b.jpg');
+    }
+
 }
-- 
GitLab