FriendsOfFlarum / upload

The file upload extension with insane intelligence for your Flarum forum.
https://discuss.flarum.org/d/4154
MIT License
177 stars 96 forks source link

Allowing XLSX extension #99

Closed flowli closed 4 years ago

flowli commented 7 years ago

Hi Daniel,

thanks for providing this great extension! There is just one issue I have.

To allow .xlsx files I added mime type pattern "^application\/vnd\..*" to be accepted with adapter "local". But when I try to upload an xlsx file, I says "Uploading files of this type is not allowed.".

In UploadHandler.php:87 I did throw new ValidationException( [ 'upload' => print_r($upload, true) ] ); to see file details and it says

[originalName:Symfony\Component\HttpFoundation\File\UploadedFile:private] => Test.xlsx [mimeType:Symfony\Component\HttpFoundation\File\UploadedFile:private] => application/vnd.openxmlformats-officedocument.spreadsheetml.sheet

while throw new ValidationException( [ 'upload' => $upload->getMimeType() ] ); gives me "application/octet-stream".

It would be great to understand how to allow xlsx files.

Many thanks! Florian

luceos commented 7 years ago

According to the source:

Symfony\Component\HttpFoundation\File\UploadedFile reads the mime type provided by the client, whereas the getMimeType() method attempts to analyse the actual file using the Symfony\Component\HttpFoundation\File\MimeType\ExtensionGuesser class, preventing abuse. The drawback of that method is that it might fallback on octet-stream instead.

I hate to introduce a security concern into this package. So you should probably whitelist octet-stream and listen to the fired event to do some manual validation..

@clarkwinkelmann do you see another solution here?

clarkwinkelmann commented 7 years ago

Yep that's tricky. If the ExtensionGuesser is not able to guess that mime type there's no way out... You have to bypass that test (whitelist the octet-steam) for that file to be accepted.

An option could maybe be to allow two levels of check in the extension "stupid mime check" (client mime type) and "real mime check" (via the ExtensionGuesser). That way you don't have to choose between all or nothing when dealing with tricky files.

luceos commented 7 years ago

@clarkwinkelmann are you suggesting to allow admins to configure the intensity of mime checking?

clarkwinkelmann commented 7 years ago

@luceos are you suggesting to allow admins to configure the intensity of mime checking?

Exactly, just like you can select a level of check for your Discord server users, maybe allow different levels of security for files (and name the weak options with stupid names so users don't use them if they can avoid them)

flowli commented 7 years ago

@luceos Thank you for the idea to whitelist and "listen to the fired event to do some manual validation". Could you give me a tiny snippet of code how to do it?

luceos commented 7 years ago

You could apply the same logic you would write an extension and Listen to events there. Instead of listening to a core event you would listen to the Adapter\Identified:

https://github.com/flagrow/upload/blob/master/src/Commands/UploadHandler.php#L83

clarkwinkelmann commented 4 years ago

Closing this issue because of inactivity. It seems like the consensus was to not implement any change into Upload anyway. Feel free to open a new issue if there's anything we can do at the extension level.