Wrong mime type detection for CSV files

[mikehaertl]

I've used a file validator like this:

[['upload'], 'file', 'skipOnEmpty' => false, 'extensions' => 'csv'],

Even if i upload a valid CSV file, i always get the error "Only files with these extensions are allowed: csv.".

The reason is, that in FileValidator::validateExtension() the mime type is detected as "text/plain" instead of "text/csv". Thus FileHelper::getExtensionsByMimeType() doesn't return csv as valid extension.

[cebe]

You may want to turn off $checkExtensionByMimeType to have more predictable behavior when you set extensions and mimeTypes explicitly.

[Ragazzo]

yes , @cebe is right , also as a possible solution you can override FileHelper::$mimeMagicFile property to point to your valid mime file

[cebe]

Well, the magic file has the correct assignment for csv: https://github.com/yiisoft/yii2/blob/master/framework/helpers/mimeTypes.php#L147

[mikehaertl]

The problem is, that here tempName is used to validate the mimetype by extension. But tmpFile doesn't have an extension - at least not on my system.

https://github.com/yiisoft/yii2/blob/master/framework/validators/FileValidator.php#L315

[qiangxue]

Line 315 passes false as the last parameter to FileHelper::getMimeType(), which prevents checking MIME type by extension.

[mikehaertl]

Oh, right. So it seems that PHP's fileinfo detects the wrong mime type for me. I've tested with 2 simple CSV files:

name;email;city
abc;def;geh

and

name,email,city
abc,def,geh

In both cases I get text/plain as $result here:

https://github.com/yiisoft/yii2/blob/master/framework/helpers/BaseFileHelper.php#L147

[samdark]

It's the same for many plain text types. As I remember correctly, it fails to detect JSON as well.

[CriStaS]

Same problem uploading xls − helper detects it as application/vnd.ms-office instead of application/vnd.ms-excel because it can't parse excel from ms office format without extension.

Setting 'checkExtensionByMimeType' => false really helps, but this behavior doesn't seems to be transparent and native:

[['file'], 'file', 'skipOnEmpty' => false, 'extensions' => ['xls', 'xlsx'] ]

I set file check by extension, not mime type (using mimeTypes validator option), so I suppose validation to be performed on extension, not mime type, but validation still made by mime type, instead of extension.

I suppose that it will be helpful at least explain this behavior in 'Uploading Files' article, which now is absolutely wrong:

Keep in mind that only the file extension will be validated, but not the actual file content.

Better choice is to switch default behavior of extension validator to validate on extension. but not mime type and mention in docs possibility to perform check mime type by setting extensions list for those who doesn't know actual mime types of files to work with mimeTypes option, but even this mention must explain that there will be much wrong detections.

Thank you!

[Ragazzo]

I think it is better to correct docs rather than make security issues

[CriStaS]

This will be no security issue in this change, you still can perform check on mime type if you really want!
But now it is absolutely "untransparent" behavior.

We have to checks: extension and mime, so I select extension, but system checks on mime.

[Ragazzo]

if we set it ot false people that dont know about mime - type willget security issues, it is better to add new mime - types to Yii2 or you can simply point to your own mime - type file

[CriStaS]

But now, people, who doesn't know about mime types use

'extensions' => ['xls', 'xlsx']

and get only xlsx files accepted because system can't validate xls file correctly. So, admin checks validation rule − there is extension check, good; there is xls extension, good; hm.... he tries to upload xls and upload still fails, so admin starts digging into yii2 core code looking for explanation why... And gets mime check instead of asked extension check!!!
So, I'm talking about definitions. If you name validation by extension, please, accept xls if I have it in array. It is no security issue, it is my own choice to use extension check instead of mimeType one. It looks completely weird for me...

[Ragazzo]

it is security issue by default , that is why it is true, feel free to set it to false or as was said use your own mime file , or you can try to convince core developers to add custom mime - types to mime file

[CriStaS]

My goal is to make it work correctly. It works for me now with 'checkExtensionByMimeType' => false but it's very sad that I was forced to debug validator just to explain why it can't upload xls file.

I suppose changing default magic file will help too.

Biggest problem now is that documentation now contrary to real behavior.

[Ragazzo]

as i said you can submit PR for docs , that will help . It is better to clarify such things in docs rather than make any security vulnerabilities

[CthulhuDen]

To say this is a vulnerability is the same as to say we should always encode values read from AR for example. If the developer asks to conduct simple extension check why would we silently check some unrelated stuff behind his back?

[Ragazzo]

your comparison is incorrect , it would be correctly if we say why using prepared by default if developer can use it by himself, we should use only concatenation

[CriStaS]

@qiangxue, please, consider changing at least documentation.

But I think that it is absolutely wrong to perform check on mimeType and name it extension check by default.

Thank you!