[mediabundle] Do not allow any and all filetypes to be uploaded to the cms. #2680
Conversation
|
same issue with symfonyinsight complaining about movemedia not being called movemediaaction |
src/Kunstmaan/MediaBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
src/Kunstmaan/MediaBundle/Validator/Constraints/IsExtensionAllowed.php
Outdated
Show resolved
Hide resolved
src/Kunstmaan/MediaBundle/Validator/Constraints/IsExtensionAllowedValidator.php
Outdated
Show resolved
Hide resolved
|
@acrobat indentation restored and symfony way used for validator error id |
src/Kunstmaan/MediaBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
|
|
||
| $errors = $this->validator->validate($file, new Image(['detectCorrupted' => true])); | ||
|
|
||
| return 0 === $errors->count(); |
There was a problem hiding this comment.
I don't like the fact that when I upload a corrected file, the ui shows "The extension of this file is not allowed on this server." as the error. In my case a .gif file is actually allowed, but the file is corrupted.
Maybe we need to discus this case
There was a problem hiding this comment.
@acrobat zou het voor jouw ok zijn als we de translation gewoon wat meer inclusive maken? Bv
"Your file has been rejected by the server because either the extension is not allowed or the file is corrupt"
?
There was a problem hiding this comment.
Imo we shouldn't group exception messages as it's not clear to the user what the exact problem is and how they can resolve it.
| $mimeType = $this->mimeTypes->guessMimeType($file->getRealPath()); | ||
| $extensions = $this->mimeTypes->getExtensions($mimeType); | ||
| $extension = array_shift($extensions); | ||
| if (!in_array($extension, $this->allowedExtensions, true)) { |
There was a problem hiding this comment.
The code itself looks ok, but it yields unexpected results. Forexample if I only allow .txt files to be uploaded and I upload a .json file, this checks allows it.
This is because $this->mimeTypes->guessMimeType() for a json files returns text/plain, which results in "txt", "text","conf","def","list","log","in","asc" as extensions from $this->mimeTypes->getExtensions and so is .txt picked. And the upload is allowed. Although there is technically no bug in this code, the result is unexpected.
So we should think about a solution so that allowed extensions list is actually the only extensions allowed
There was a problem hiding this comment.
Ik heb een soort van fallback gemaakt waarbij we kijken dat indien de locale fileinfo text/plain teruggeeft en het een uploadedfile is we fallbacken naar de clientmimetype. Dat is natuurlijk niet 100% foolproof want is spoofable maar at least kunnen we toch iets meer onderscheid maken tussen al die files die anders als "text/plain" gewoon zomaar toegelaten worden.
There was a problem hiding this comment.
I'm not sure about this update as symfony indicates (and you) this value can't be trusted. https://github.com/symfony/symfony/blob/0baa58d4e4bb006c4ae68f75833b586bd3cb6e6f/src/Symfony/Component/HttpFoundation/File/UploadedFile.php#L128-L129
Would it be a solution to move away from the idea of listing extensions and have the user provide an allowed list of mimetypes? Like you would do with the File constraint?
There was a problem hiding this comment.
The thing is that its just a server thing. because of the way the upload works the uploaded file will be put in the temp folder as "9d0329d30" or something like that without an extension. Because files like json,xml etc are not executable nor have a header in the file to specify what file type they are this effectively makes them just a plaintext file. The good thing is that because of that they are not a threat to the server as they wouldn't be able to be executed. Therefore i think its more a matter of cleanliness to do this optional extra check for files we have already verified are not a threat to the system to deny them from being uploaded if we have not allowed that extension.
Using mimetypes would give us the exact same "problem" because someone might be confused as to why a file with mimetype "text/json" is allowed on the server if he hasn't put it in his allowed configuration.
But yeah tldr: files that get detected by fileinfo on the server as plaintext are no threat and therefore its just a matter of cleanliness to choose to allow/block them from being uploaded which we can do using the clientinfo.
Co-authored-by: Jeroen Thora <jeroenthora@gmail.com>
…add more extensions
…owedValidator.php Co-authored-by: Jeroen Thora <jeroenthora@gmail.com>
Co-authored-by: Jeroen Thora <jeroenthora@gmail.com>
| $mimeType = $this->mimeTypes->guessMimeType($file->getRealPath()); | ||
| $extensions = $this->mimeTypes->getExtensions($mimeType); | ||
| $extension = array_shift($extensions); | ||
| if (!in_array($extension, $this->allowedExtensions, true)) { |
There was a problem hiding this comment.
I'm not sure about this update as symfony indicates (and you) this value can't be trusted. https://github.com/symfony/symfony/blob/0baa58d4e4bb006c4ae68f75833b586bd3cb6e6f/src/Symfony/Component/HttpFoundation/File/UploadedFile.php#L128-L129
Would it be a solution to move away from the idea of listing extensions and have the user provide an allowed list of mimetypes? Like you would do with the File constraint?
|
|
||
| $errors = $this->validator->validate($file, new Image(['detectCorrupted' => true])); | ||
|
|
||
| return 0 === $errors->count(); |
There was a problem hiding this comment.
Imo we shouldn't group exception messages as it's not clear to the user what the exact problem is and how they can resolve it.
|
@Numkil I was doing some fixes in the mediabundle and came across this config. Can't this existing feature be used implement the required behaviour? |
Security testing advised us to remove the ability to upload any and all sorts of media to the cms that could then be used for nefarious purposed through the media/uploads folder.