Bug #54357
closed
Inconsistent usage of filename sanitation
Added by Alexander Stehlik over 10 years ago.
Updated almost 6 years ago.
Category:
File Abstraction Layer (FAL)
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 the getDriver() in the ResourceStorage class to public and let the developers perform the sanitation manually or
- Add the
sanitizeFileName() check to AbstractDriver->fileExistsInFolder()
IMO the check for valid and secure filenames has to be part of a driver. Hence option 2 is the right one for me.
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 :(
- Target version deleted (
6.2.0)
- 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 :)
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
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.
- Status changed from Needs Feedback to New
Can I ask you if it is this still valid in recent TYPO3 releases like 6.2.12 or 7.2?
- Status changed from New to Needs Feedback
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.
- Status changed from Needs Feedback to New
- Status changed from New to Needs Feedback
I think it is fixed.
The method has been renamed though. It is called sanitizeFileName() now (see #68600).
- 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
Updated by 
Updated by 
Updated by 
Updated by 
Updated by 
Updated by 
Updated by 
Updated by