Bug #54357
closed
Inconsistent usage of filename sanitation
0%
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:
- Make the
sanitizeFilename()method publicly available (by changing thegetDriver()in theResourceStorageclass to public and let the developers perform the sanitation manually or - Add the
sanitizeFileName()check toAbstractDriver->fileExistsInFolder()
Updated by Markus Klein almost 11 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.
Updated by Alexander Stehlik almost 11 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 :(
Updated by
Updated by Frans Saris over 10 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 :)
Updated by Alexander Stehlik over 10 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
Updated by Marc Bastian Heinrichs over 10 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.
Updated by Alexander Opitz about 10 years ago
- Status changed from Needs Feedback to New
Updated by Riccardo De Contardi over 9 years ago
Can I ask you if it is this still valid in recent TYPO3 releases like 6.2.12 or 7.2?
Updated by Riccardo De Contardi over 9 years ago
- Status changed from New to Needs Feedback
Updated by Alexander Stehlik over 9 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.
Updated by Riccardo De Contardi over 9 years ago
- Status changed from Needs Feedback to New
Updated by Frans Saris over 9 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.
Updated by Benni Mack over 6 years ago
- Status changed from New to Needs Feedback
So this is fixed now?
Updated by Alexander Stehlik over 6 years ago
I think it is fixed.
The method has been renamed though. It is called sanitizeFileName() now (see #68600).
Updated by Riccardo De Contardi over 6 years 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.