humhub / humhub

HumHub is an Open Source Enterprise Social Network. Easy to install, intuitive to use and extendable with countless freely available modules.
https://www.humhub.com
Other
6.32k stars 1.66k forks source link

File Validator - CSV detection broken (Ending Line Break) #7205

Open LoSaMe opened 1 month ago

LoSaMe commented 1 month ago

What steps will reproduce the problem?

  1. Add pdf,pptx,ppt,docx,doc,xlsx,xls,csv,zip,png,jpg to the allowed file extensions
  2. Try to upload a csv file

What is the expected result?

Upload the file without error

What do you get instead?

An error message that the extension is not allowed:

Einige Dateien konten nicht hochgeladen werden: Mappe1.csv:
Es sind nur Dateien mit folgenden Dateierweiterungen erlaubt: pdf, pptx, ppt, docx, doc, xlsx, xls, csv, zip, png, jpg.

Additional info

If I do not use the allowed file extensions (allow all), the upload is successful

Q A
HumHub version 1.15.4
PHP version 7.4.33
Operating system Debian 11.10
luke- commented 1 month ago

It is also checked whether the content is really a CSV file. Something is probably going wrong here.

You can disable it via configuration file: https://community.humhub.com/s/installation-and-setup/wiki/80/configuration-examples#file-validator%3A-turn-off-mimetime-to-extension-checking

marc-farre commented 1 month ago

Tested with HumHub 1.16.2: image

Upload works on my instance: image

LoSaMe commented 1 month ago

Thanks for the fast reply.

@luke- It works if i disable the check. But i guess this poses security risks? For example, this would be a CSV example that is rejected:

Stunde,Montag,Dienstag,Mittwoch,Donnerstag,Freitag
1,Mathematik,Deutsch,Englisch,Erdkunde,Politik
2,Sport,Deutsch,Englisch,Sport,Geschichte

@marc-farre Is 1.16.2 already released? We also have an instance in version 1.16.1 where the problem also occurs.

Edit: The problem is not limited to csv files

marc-farre commented 1 month ago

@LoSaMe No, 1.16.2 is not yet released, but it makes no difference for your issue with 1.16.1.

I've created a test CSV file with your content with UTF-8 char set: test.csv

Upload works for me. Maybe you could send your file, so I can test?

LoSaMe commented 1 month ago

@marc-farre In my case, the test.csv file is also rejected. Here is my file with the same content: test2.csv

marc-farre commented 1 month ago

@LoSaMe Thanks for providing this example.

@luke- This is the issue:

When the CSV file contains an ending blank line, upload works: image

When it doesn't, upload throws an error: image

image

luke- commented 1 month ago

@marc-farre Thats interesting. Good to know. Probably thats against the CSV spec.

marc-farre commented 1 month ago

@luke- it's seems to be recommended but not mandatory:

"The last record in the file may or may not have an ending line break.": https://datatracker.ietf.org/doc/html/rfc4180

"Though it is RECOMMENDED, the last record in a file is not required to have a ending line break.": https://github.com/parsecsv/csv-spec/blob/master/csv-spec.md#csv-format-specification

luke- commented 1 month ago

Hmm, I don't know how exactly Yii2 determines the MimeType, but it is either worth opening an issue there or with the lib used.

marc-farre commented 1 month ago

When no ending line break is present in the CSV file, FileHelper::getMimeType returns text/plain instead of text/csv here: https://github.com/humhub/humhub/blob/ee6eb65425fc34f2f5d158430b9c30d99e4b1e24/protected/humhub/modules/file/validators/FileValidator.php#L109-L109

I've found an old closed issue about this: https://github.com/yiisoft/yii2/issues/6148 There is no mention about the ending line break.

@luke- Do you want me to open an issue? Should I investigate Yii2 code and try to send a PR with a fix?

luke- commented 1 month ago

@marc-farre I don't think the issue is that critical at the moment.

@LoSaMe If you want, you can also create an issue in the Yii2 repo for this.