Bug #54357

Inconsistent usage of filename sanitation

Added by Alexander Stehlik over 5 years ago. Updated about 1 year ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
File Abstraction Layer (FAL)
Target version:
-
Start date:
2013-12-12
Due date:
% Done:

0%

TYPO3 Version:
6.2
PHP Version:
5.4
Tags:
Complexity:
Is Regression:
No
Sprint Focus:

Description

How would you solve this:

Before I create a file I check if the file exists in the folder where I want to create it:

\TYPO3\CMS\Core\Resource\Folder->hasFile()
\TYPO3\CMS\Core\Resource\ResourceStorage->hasFileInFolder()
\TYPO3\CMS\Core\Resource\Driver\AbstractDriver->fileExistsInFolder()

To add a file I use:

\TYPO3\CMS\Core\Resource\Folder->addFile()
\TYPO3\CMS\Core\Resource\ResourceStorage->addFile()
\TYPO3\CMS\Core\Resource\Driver\AbstractDriver->addFile()

The problem here is, that AbstractDriver->hasFile() will not call AbstractDriver->sanitizeFileName() before checking if the file exists, but add AbstractDriver->addFile() will do.

This will lead to the problem that addFile() can fail because the sanitized filename exists in the folder but not the file that was checked in the first place.

So basically I see two options:

  1. Make the sanitizeFilename() method publicly available (by changing the getDriver() in the ResourceStorage class to public and let the developers perform the sanitation manually or
  2. Add the sanitizeFileName() check to AbstractDriver->fileExistsInFolder()

Related issues

Related to TYPO3 Core - Bug #55299: conflictMode rename is not working when uploading file with umlaut Closed 2014-01-24
Related to TYPO3 Core - Task #68600: Change ResourceStorage::processUploadedFilename to sanitizeFileName Closed 2015-07-28

History

#1 Updated by Markus Klein over 5 years ago

IMO the check for valid and secure filenames has to be part of a driver. Hence option 2 is the right one for me.

#2 Updated by Alexander Stehlik over 5 years ago

Yes, this was my first thought, too.

But we currently have only a sanitation for filenames, this means we could only do the check for the fileExistsInFolder() method but not for the fileExists() method. This would be inconsistent again :(

#3 Updated by Steffen Ritter over 5 years ago

  • Target version deleted (6.2.0)

#4 Updated by Frans Saris over 5 years ago

  • Status changed from New to Needs Feedback

Hi Alexander,

IMO you should handle this with a exception driven approach.

try {
    Folder->addFile($newFile);
} catch(ExistingTargetFileNameException $exception) {
    // show message to user
}

And with #55299 resolved this is posible in all cases.

Has file is IMO only to check if a file exist that you want to use. Not to check if a file name is not in use. It's only a small difference :)

#5 Updated by Alexander Stehlik over 5 years ago

Hi Franks,

thank you for your feedback. I guess you are right. You could just catch the Exception :)

Issue can be closed as far as I'm concerned.

Cheers,
Alex

#6 Updated by Marc Bastian Heinrichs over 5 years ago

Running into the same problem with folders. Assuming following code:

$folderName = PathUtility::dirname(ltrim($fileRecord['identifier'], '/'));
if (!$storage->hasFolder($folderName)) {
    try {
        $importFolder = $storage->createFolder($folderName);
    } catch (Exception $e) {
        $this->error('Error: Folder could not be created for file "' . $fileRecord['identifier'] . '" with storage uid "' . $fileRecord['storage'] . '"');
        continue;
    }
} else {
    $importFolder = $storage->getFolder($folderName);
}

if $folderName is a name that changes by sanitizing on creation, the driver will try to create it over and over again, also if it is already there.
One point is, that the driver should also check if the sanitized folder is already there before creation.

The point is, that it would be nice to avoid the creation call by having public access to the sanitizeFileName of the driver, e.g. by addding sanitizeFileName() also to the storage. This is also a must to have a replacement for BasicFileUtility::cleanFileName.

#7 Updated by Alexander Opitz almost 5 years ago

  • Status changed from Needs Feedback to New

#8 Updated by Riccardo De Contardi over 4 years ago

Can I ask you if it is this still valid in recent TYPO3 releases like 6.2.12 or 7.2?

#9 Updated by Riccardo De Contardi over 4 years ago

  • Status changed from New to Needs Feedback

#10 Updated by Alexander Stehlik about 4 years ago

I just looked into this and did not find any possibility to use FAL methods to sanitize a filename before using it in the current 6.2.14 release.

You can / must use BasicFileUtility::cleanFileName() which is not nice because it does not now anything about the FALS driver beeing used and its capabilities concerning file naming.

This could be solved quite easily by putting a public sanitizeFilename() method in the ResourceStorage class.

#11 Updated by Riccardo De Contardi about 4 years ago

  • Status changed from Needs Feedback to New

#12 Updated by Frans Saris about 4 years ago

In current master we now got a method that sanitizes a filename and also respects possible preAddFile signal.

See https://review.typo3.org/#/c/40310/12/typo3/sysext/core/Classes/Resource/ResourceStorage.php

Sadly this can not be back ported as it changes the signal behaviour.

#13 Updated by Benni Mack over 1 year ago

  • Status changed from New to Needs Feedback

So this is fixed now?

#14 Updated by Alexander Stehlik about 1 year ago

I think it is fixed.

The method has been renamed though. It is called sanitizeFileName() now (see #68600).

#15 Updated by Riccardo De Contardi about 1 year ago

  • Status changed from Needs Feedback to Closed

Thank you for your feedback; I close this issue. If you think that this is the wrong decision or experience the issue again please reopen it or open a new issue with a reference to this one.

Thank you.

Also available in: Atom PDF