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

MediaHelper: proper check if file is an image (#42105) #43345

Merged
merged 4 commits into from
May 17, 2024

Conversation

coderiekelt
Copy link
Contributor

Pull Request for Issue #42105.

Summary of Changes

  • Changed MediaHelper.php to properly check if the uploaded file is actually an image, instead of assuming true.

Testing Instructions

Upload a PDF file to the media library, this should work properly. Uploading images will also still work fine in accordance with previous behavior.

Actual result BEFORE applying this Pull Request

Uploading a PDF fails, providing an unhelpful error to the user. When checking the request in within the browser the message is along the lines of "This is not a valid image".

Expected result AFTER applying this Pull Request

Uploading a PDF works fine, uploading images still follows the previous behavior.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@brianteeman
Copy link
Contributor

unable to test as I cannot replicate the reported error

@coderiekelt
Copy link
Contributor Author

coderiekelt commented Apr 24, 2024

After some digging it appears that this error occurs if the exif extension is not loaded.

A simple test script to get the MIME type from a blank PDF file yielded this result:

getimagesize: unknown (false)
exif_imagetype: application/octet-stream

Meaning this problem will not occur if the exif extension is loaded, since it will return application/octet-stream. (Which is properly detected further down in the function.)

If exif is not loaded it will fail, since the MediaHelper will assume that the file is always an image and just gives up after trying getimagesize() due to the way the way the if statement within this function is structured.

@brianteeman
Copy link
Contributor

i can confirm that this issue occurs when exif is not available.

@brianteeman
Copy link
Contributor

Some history #16086

Which should have been fixed with #16091

@richard67
Copy link
Member

richard67 commented May 5, 2024

Some history #16086

Which should have been fixed with #16091

@brianteeman Yes, but that code has been modified meanwhile. In PR #16091 it was checked if the file name extension is in the allowed list for images, $params->get('image_extensions'), and if that was the case, the static::getMimeType method was called with value true for the $isImage parameter.
That was right, and that's why it worked with that PR.

But later the code was changed so it uses the list of allowed file name extensions for uploads, which can also be e.g. a video or a PDF file:
params->get('restrict_uploads_extensions', 'bmp,gif,jpg,jpeg,png,webp,ico,mp3,m4a,mp4a,ogg,mp4,mp4v,mpeg,mov,odg,odp,ods,odt,pdf,png,ppt,txt,xcf,xls,csv')

The static::getMimeType method then checks with EXIF only when that parameter has value true, but that would fail for a video or PDF file:

if ($isImage && \function_exists('exif_imagetype')) {
$mime = image_type_to_mime_type(exif_imagetype($file));
} elseif ($isImage && \function_exists('getimagesize')) {
$imagesize = getimagesize($file);
$mime = $imagesize['mime'] ?? false;

So to me the change made by this PR here seems to be right. It replaces the hard-coded true value by the result of the static::isImage call, which does nothing else than checking the file name extension again for valid values for images.

@richard67 richard67 added the bug label May 5, 2024
@Quy
Copy link
Contributor

Quy commented May 16, 2024

I have tested this item ✅ successfully on fde2fe8


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43345.

@Fedik
Copy link
Member

Fedik commented May 16, 2024

Uploading a PDF fails, providing an unhelpful error to the user

The fix for unhelpful message is there #38536

@Fedik
Copy link
Member

Fedik commented May 16, 2024

Hm, the getMimeType() should not care about isImage, it should read the file mime for any file, regardless its type.
But that another issue.

As for now the PR is okay.

@Fedik
Copy link
Member

Fedik commented May 16, 2024

I have tested this item ✅ successfully on fde2fe8


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43345.

@Fedik Fedik removed the bug label May 16, 2024
@Fedik
Copy link
Member

Fedik commented May 16, 2024

r2c


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43345.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 16, 2024
@Fedik Fedik added the bug label May 16, 2024
@MacJoom MacJoom self-assigned this May 16, 2024
@MacJoom MacJoom merged commit 2964ee6 into joomla:4.4-dev May 17, 2024
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 17, 2024
@MacJoom
Copy link
Contributor

MacJoom commented May 17, 2024

Thank you!

@Quy Quy added this to the Joomla! 4.4.5 milestone May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants