Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FileValidator validateExtension not working correctly on PHP 8.1 #19307

Closed
Akdmeh opened this issue Mar 16, 2022 · 13 comments
Closed

FileValidator validateExtension not working correctly on PHP 8.1 #19307

Akdmeh opened this issue Mar 16, 2022 · 13 comments
Milestone

Comments

@Akdmeh
Copy link

Akdmeh commented Mar 16, 2022

What steps will reproduce the problem?

Trying to validate image on model, f.e. picture.jpg
['image', 'image', 'extensions'=>['jpg', 'jpeg', 'png', 'gif']],
I think, the problem is in BaseFileHelper:getMimeType.
Let's see:
Inside FileValidator (which ImageValidator extends) we're trying to get file extension by mime-type via code:
$mimeType = FileHelper::getMimeType($file->tempName, null, false);
As you see, we send tempName (in my case it have temp path like /tmp/phpn1MNFs, no extension at all!).
But then the magic is going inside FileHelper (and BaseFileHelper).
First, this fallback won't work well:
if (!extension_loaded('fileinfo')) { if ($checkExtension) { return static::getMimeTypeByExtension($file, $magicFile); }
We send /tmp/phpn1MNFs into $file variable, so no extension available which leads always to null and uncorrect result.
Next, if fileinfo available, new lines added recently on 2.0.45:
if (PHP_VERSION_ID >= 80100) { return static::getMimeTypeByExtension($file, $magicFile); }
Again, $file does not have any extension so it will always return null!
That's the reason of this bug.
As I can understand, before PHP 8.1 code validated via fileinfo (on next lines of this method), but new code disables fileinfo checks for PHP 8.1 and run getMimeTypeByExtension instead, which cannot work well with temp upload file without extension.
But it works well before (on Yii 2.0.44 & PHP 8.1) in my case.

What is the expected result?

Successfull validation

What do you get instead?

Validation failed

Possible ways to fix

Send not tmpName but original file name (possible vulnerabilities included, so this way needs deeper inspection).
OR
Fix code in method BaseFileHelper::getMimeType to work with fileinfo & PHP 8.1 correctly.
Honestly, I don't see a reason to add:
if (PHP_VERSION_ID >= 80100) { ... } fix, because whole next code works well in my case, so maybe I'm missing some other behaviour in other parts of code.
I temporarily fixed error by setting checkExtensionByMimeType inside validator to false, but Yii 2.0.45 on PHP 8.1 will break existing file extension checks.

Additional info

Q A
Yii version 2.0.45
PHP version 8.1
Operating system Rocky Linux
@WinterSilence
Copy link
Contributor

WinterSilence commented Mar 16, 2022

$mimeType = FileHelper::getMimeType($file->tempName, null, false);
As you see, we send tempName (in my case it have temp path like /tmp/phpn1MNFs, no extension at all!).

getMimeType($file, $magicFile = null, $checkExtension = true) i.e. file extension not checking

@Akdmeh
Copy link
Author

Akdmeh commented Mar 16, 2022

Let's dive into source deeper:
`
public static function getMimeType($file, $magicFile = null, $checkExtension = true)
{
if ($magicFile !== null) {
$magicFile = Yii::getAlias($magicFile);
}
//Fallback if fileinfo absent (this condition skipped in my case)
if (!extension_loaded('fileinfo')) {
if ($checkExtension) {
return static::getMimeTypeByExtension($file, $magicFile);
}

        throw new InvalidConfigException('The fileinfo PHP extension is not installed.');
    }

//This branch will be executed on PHP 8.1, ignoring $checkExtension and starts to check extension (which is absent!). Added on 2.0.45 and breaks code.
if (PHP_VERSION_ID >= 80100) {
return static::getMimeTypeByExtension($file, $magicFile);
}
//"Traditional" file extension check, worked well before 2.0.45 but for some reason skipped in 8.1 (maybe it hasn't work in some other code or breaks tests)
$info = finfo_open(FILEINFO_MIME_TYPE, $magicFile);

    if ($info) {
        $result = finfo_file($info, $file);
        finfo_close($info);

        if ($result !== false) {
            return $result;
        }
    }

    return $checkExtension ? static::getMimeTypeByExtension($file, $magicFile) : null;
}`

@WinterSilence
Copy link
Contributor

@samdark

//This branch will be executed on PHP 8.1, ignoring $checkExtension and starts to check extension (which is absent!). Added on 2.0.45 and breaks code.

your diversion? (:

@WinterSilence
Copy link
Contributor

@Akdmeh

finfo_file(): The finfo parameter expects an finfo instance now; previously, a resource was expected.

@Akdmeh
Copy link
Author

Akdmeh commented Mar 16, 2022

Yes, I saw this change but don't work with fileinfo ext before so don't know how to fix it :-(
And as I said, existing code without patch for 8.1 works well (but maybe fail in another places)

@WinterSilence
Copy link
Contributor

@Akdmeh where code if (PHP_VERSION_ID >= 80100) {?

@Akdmeh
Copy link
Author

Akdmeh commented Mar 16, 2022

helpers/BaseFileHelper.php line 165 (in 2.0.45)

@WinterSilence
Copy link
Contributor

@Akdmeh i can't find this in current master branch

@Akdmeh
Copy link
Author

Akdmeh commented Mar 16, 2022

Maybe, this commit deleting this buggy lines and it will fix this issue in next release (2.0.46)?
7b8c29d
So, it will be fixed as I can understand

@WinterSilence
Copy link
Contributor

@Akdmeh can you load master branch and check your code?

@michalwoz
Copy link
Contributor

We worked around this issue(temporary) by adding 'checkExtensionByMimeType' => false, to validation rules.

@Akdmeh
Copy link
Author

Akdmeh commented Mar 21, 2022

Yes, 'checkExtensionByMimeType' => false fixed the issue (but it needs to rewrite existing code) and I sure the current master branch should fix the issue. But I cannot check it (I'm sitting in some place with horrible internet and cannot update composer 😭). I'll try again but I think it will be ok and issue can be closed after next Yii2 release

@samdark
Copy link
Member

samdark commented Mar 30, 2022

Yes, it's fixed in current master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants