Task #68600

Change ResourceStorage::processUploadedFilename to sanitizeFileName

Added by Frans Saris about 4 years ago. Updated almost 2 years ago.

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

100%

TYPO3 Version:
7
PHP Version:
Tags:
Complexity:
Sprint Focus:
Stabilization Sprint

Description

The method is not only for processing filenames of uploaded files but for all addFile actions. ResourceStorage::sanitizeFileName() better fits functionality and makes API more self explaining.


Related issues

Related to TYPO3 Core - Feature #67545: Add AJAX method to check if file exists Closed 2015-06-17
Related to TYPO3 Core - Bug #54357: Inconsistent usage of filename sanitation Closed 2013-12-12
Precedes Resize images automatically - Bug #68699: Upload signal changed again in 7.4-dev Closed 2015-08-03

Associated revisions

Revision 84218a9a (diff)
Added by Frans Saris about 4 years ago

[TASK] Rename ResourceStorage::processUploadedFilename to sanitizeFileName

Feature #67545 introduced ResourceStorage::processUploadedFilename and
changed the existing PreFileAddSignal. The name of the newly introduced
method doesn't really fit its functionality and using the same signal
for
two different purposes is also not correct.

This change renames the method
processUploadedFilename() to sanitizeFileName()
and introduces the SanitizeFileNameSignal.
The changes made to PreFileAddSignal are reverted.

Resolves: #68600
Related: #67545
Releases: master
Change-Id: Iabeffe5b8b3fe85a4f49b9341a45b2a026dd241c
Reviewed-on: http://review.typo3.org/42116
Reviewed-by: Nicole Cordes <>
Tested-by: Nicole Cordes <>
Reviewed-by: Mathias Schreiber <>
Tested-by: Mathias Schreiber <>
Reviewed-by: Frans Saris <>
Tested-by: Frans Saris <>

History

#1 Updated by Andreas Wolf about 4 years ago

I think this whole method is a bit problematic:

1. The order of operations is wrong (I’m currently on correcting this, see below)
2. It emits a signal that is only used when the file is actually about to be added (PreFileAddSignal).
3. The folder check is unnecessary IMHO as the $targetFolder cannot be passed as NULL
4. Considering the original reason this method was added, I don’t see why a simple call to hasFileInFolder() did not suffice.

As for the signal, an API user might rely on the file only being emitted in addFile()—that implies that the file will actually be added moments later. If the file is not added at all in the current (AJAX) request, then things might break. There is documentation on that, but I’d rather have a dedicated signal than abusing a totally unrelated signal.

Concerning the order of operations: The first thing to do is let others alter the filename (via the signal), then the driver should tell if the filename is allowed (or correct it – sanitizeFileName()) and only then we must check the permissions.

#2 Updated by Frans Saris about 4 years ago

Andreas Wolf wrote:

4. Considering the original reason this method was added, I don’t see why a simple call to hasFileInFolder() did not suffice.

The whole issue is that hasFileInFolder() doesn't sanitize the input and also the signal can change the file name. So to check if a file already exists, f.i. when uploading a new file, you first need to sanitize the filename just like the normal addFile() procedure would do. And this involves the driver->sanitize() and the PreAddFile signal.

As for the signal, an API user might rely on the file only being emitted in addFile()—that implies that the file will actually be added moments later. If the file is not added at all in the current (AJAX) request, then things might break. There is documentation on that, but I’d rather have a dedicated signal than abusing a totally unrelated signal.

Maybe a new signal would indeed make sense, but current signal is currently documented in the code with:

// We do not care whether the file exists yet because $targetFileName may be changed by an
// external slot and only then we should check how to proceed according to $conflictMode

That's why we went for adjusting the current signal.

So your proposal is to go for a SanitizeFileName Signal?

#3 Updated by Frans Saris about 4 years ago

I'm not convinced that order is wrong. IMO sanitizeFileName() should be called before the signal. Else the Signal gets the wrong filename.

Of Course the Signal could return an file that is not save. But that's the responsibility of the developer. When the filename is really passed to the driver to be created/added the driver sanitizes the filename one last time before creating the file and returns the filename.

Also the signal has a reference to the $driver and is able to call $driver->sanitizeFileName().

So the order of operations is correct.

#4 Updated by Benni Mack about 4 years ago

  • Category changed from 1599 to File Abstraction Layer (FAL)
  • Assignee deleted (Benni Mack)

#5 Updated by Frans Saris about 4 years ago

  • Status changed from New to In Progress
  • Assignee set to Frans Saris

#6 Updated by Gerrit Code Review about 4 years ago

  • Status changed from In Progress to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/42116

#7 Updated by Gerrit Code Review about 4 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/42116

#8 Updated by Gerrit Code Review about 4 years ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/42116

#9 Updated by Gerrit Code Review about 4 years ago

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/42116

#10 Updated by Gerrit Code Review about 4 years ago

Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/42116

#11 Updated by Gerrit Code Review about 4 years ago

Patch set 6 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/42116

#12 Updated by Gerrit Code Review about 4 years ago

Patch set 7 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/42116

#13 Updated by Gerrit Code Review about 4 years ago

Patch set 8 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/42116

#14 Updated by Frans Saris about 4 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

#15 Updated by Riccardo De Contardi almost 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF