CFileValidator could validate uploaded file by its MIME-type. #1008

Closed
wants to merge 10 commits into
from

Conversation

4 participants
Contributor

resurtm commented Jul 22, 2012

No description provided.

Member

klimov-paul commented Jul 26, 2012

This enhancement has no sense: mime type of the uploaded file is determined by the web server using the mime type tables. When server determines the mime type of the file, it checks the file extension – nothing more. Server is unable to check, if the file content matches some mime type or not.
So mime type is just an alias of the file extension.
As far as “CFileValidator” can validate file extension, it does not need to validate mime type.

Contributor

resurtm commented Jul 26, 2012

@klimov-paul,

As far as “CFileValidator” can validate file extension, it does not need to validate mime type.

I do not agree. In some cases MIME type checking might be very useful in terms of application security. How would your application behave if i'll try to upload *.xls file named as *.gif where only image files were expected (e.g. user profile image)? :)

This enhancement has no sense: mime type of the uploaded file is determined by the web server using the mime type tables. When server determines the mime type of the file, it checks the file extension – nothing more. Server is unable to check, if the file content matches some mime type or not.

Ok. I've pushed one new commit. Checking of MIME type inside CFileValidator now occurs through the CFileHelper::getMimeType().

CFileHelper::getMimeType() does the real MIME type checking by obtaining and analyzing file's content (it internally uses finfo_open/finfo_file and deprecated mime_content_type as a fallback way — both methods does the real MIME type checking).

I've tested this new commit by renaming GIF file to the test.xls and then tried to upload it with form with following validation rule:

<?php

array('test','file','mimeTypes'=>'application/vnd.ms-excel, application/msexcel, application/x-msexcel, application/x-ms-excel, application/vnd.ms-excel, application/x-excel, application/x-dos_ms_excel, application/xls, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet'),

Of course validation in this case failed, while uploading the real *.xls file (saved from MS Excel) passes the validation normally.

Thank you for your remark. :)

Member

klimov-paul commented Jul 26, 2012

Alright "finfo_open" can make a trick.
However you should place a warning into the doc comments, which tells developer mime type check is unreliable if "file info" package is not install at your server.
Solution can work only for php > 5.3.0, PECL fileinfo >= 0.1.0 and appropriate set of "magic files".

Contributor

resurtm commented Jul 26, 2012

@klimov-paul Take a look into CFileHelper::getMimeType() method's source code (which is used in my code). There's already exists fallback mode for the case, when fileinfo not installed or PHP version is less than 5.3.

Member

klimov-paul commented Jul 26, 2012

The "fallback" inside "CFileHelper::getMimeType()" will cause an implicit errors in your case.
You can rely the method "CFileHelper::getMimeType()" will return real mime type only if function "finfo_open" is available. Otherwise it will cause the situation I have described initially.

Contributor

resurtm commented Jul 26, 2012

@klimov-paul Ah, yes, sorry, now i see. I've mistakenly thought, that mime_content_type() is not the part of the fileinfo extension and thought, that it's available regardless of the fileinfo availability. :)

I'll think more for robust solution.

Contributor

resurtm commented Jul 26, 2012

Solution can work only for php > 5.3.0, PECL fileinfo >= 0.1.0 and appropriate set of "magic files".

Btw, PHP version is not the thing to be checked, because mime_content_type() works good for PHP < 5.3. The only criteria is fileinfo availability.

resurtm added some commits Jul 28, 2012

@resurtm resurtm Merge branch 'master' of git://github.com/yiisoft/yii into CFileValid…
…ator-mimeTypes
26486a7
@resurtm resurtm Some refinements to the CFileValidator's ability to check MIME type o…
…f uploaded file. Added note that fileinfo extension should be installed. Exception will be thrown if $mimeTypes specified by user. Disabled detection of MIME type by file name extension (fake detection which is unnecessary in this part of code).
9e9ab8d
@resurtm resurtm Removed unnecessary check for NULL. eeed9d4
Contributor

resurtm commented Jul 28, 2012

@mdomba @samdark I'm done with refinements and fileinfo checking.

samdark was assigned Jul 28, 2012

Owner

samdark commented Jul 28, 2012

Looks fine to me.

Would it be more precise to use if(function_exists('finfo_open')) ?

Owner

resurtm replied Jul 28, 2012

if(function_exists('finfo_open'))

This check fits only for PHP >= 5.3.0 because finfo_open() does not exists in PHP < 5.3.0.

In case PHP version is less than 5.3.0 mime_content_type() is used inside CFileHelper::getMimeType. mime_content_type() is also part of the fileinfo extension, but deprecated since 5.3.0 release.

Check a bit how finfo_file is used in the getMimeType.

That method does not return "null" but false if any error happens... so in that case even if the user has that extension loaded, but finfo_file returns false (any possible error while processing the file for mime type)... getMimeType would resolve by using other "less secure" methods

Because of this and becasuse we want security here... maybe it would be better to use directly here the finfo* methods instead of just checking the extension and using getMymeType.

Contributor

resurtm commented Jul 28, 2012

Closing this PR for rework (this is littered a bit with useless commits and comments). I will open new one very soon.

resurtm closed this Jul 28, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment