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

Better check mime type of uploaded file #25351

Closed
wants to merge 2 commits into from
Closed

Better check mime type of uploaded file #25351

wants to merge 2 commits into from

Conversation

Quy
Copy link
Contributor

@Quy Quy commented Jun 27, 2019

Pull Request for Issue #16086

Credits go to @csthomas for PR #20968

Summary of Changes

A better way to check the MIME file type if the operating system is incorrectly configured.

If you still have a problem and your server is not running on Windows, test the following patch:

diff --git a/libraries/src/Helper/MediaHelper.php b/libraries/src/Helper/MediaHelper.php
index 60f7fab83d..99cb1d80e1 100644
--- a/libraries/src/Helper/MediaHelper.php
+++ b/libraries/src/Helper/MediaHelper.php
@@ -102,6 +102,12 @@ class MediaHelper
                                        $mime      = isset($imagesize['mime']) ? $imagesize['mime'] : false;
                                }
                        }
+
+                       // Last chance to get to know the type of mime
+                       if (!$mime && IS_UNIX && function_exists('exec') && function_exists('escapeshellarg'))
+                       {
+                               $mime = strtok(exec('file -bi ' . escapeshellarg($file)), ';');
+                       }
                }
                catch (\Exception $e)
                {

Testing Instructions

Try to upload pdf or image file.
Take a look at #16086

Please test if you still interested @goforitweb, @uglyeoin, @edthenet, @OrignlCin

Expected result

PDF file is recognized.

Actual result

#16086

Documentation Changes Required

No

@zero-24
Copy link
Contributor

zero-24 commented Jun 27, 2019

I'm not that deep into the issue we fixed back than in 3.7.1 but I would like to call @SniperSister @joomla/security to take a look here too. And I would like to understand why it worked and now it does not anymore? As the issue mention here should have been fixed by: #16091 lets be carefull here please. IIRC there was a reason we first checked with the image detectors and than with the the more generic ones.

@zero-24 zero-24 requested a review from SniperSister June 27, 2019 19:38
@ghost
Copy link

ghost commented Jul 15, 2019

there were several issues for this Pull Request, so i hope @goforitweb, @uglyeoin, @edthenet, @OrignlCin will test it.

@uglyeoin
Copy link
Contributor

@franz-wohlkoenig Thanks for tagging me. I'm happy to test.

@uglyeoin
Copy link
Contributor

I have not applied the patch.
I went to the media manager.
I uploaded a PDF.
It worked.
I went to an article, I clicked to upload an image, I uploaded a PDF. It worked. I couldn't see it until I went to the media manager but it worked.
Do I need different testing instructions? It all seems to work without the patch.

@ghost
Copy link

ghost commented Jul 20, 2019

Do I need different testing instructions? It all seems to work without the patch.

comment @Quy?

@brianteeman
Copy link
Contributor

A better way to check the MIME file type if the operating system is incorrectly configured.

If you did not have a problem uploading then other than confirming that you still dont have a problem uploading with this PR there is nothing else you can do.

What this PR really needs is tests from people who do have a problem uploading to see if this resolves that

@uglyeoin
Copy link
Contributor

@brianteeman I did previously have a problem. I wish I could remember which PDF in case it was specifically tied to one.

@zero-24
Copy link
Contributor

zero-24 commented Jul 20, 2019

Well in theory it should not be bound to an specific file but an specific server setup. But I personally would suggest to not merge this change without understanding what the actual issues is we are fixing here / how to actually reproduce the problem.

@uglyeoin
Copy link
Contributor

It is possible that either I or my host changed something. I don't know what if I did anything. If it reoccurs I will come back.

@coolcat-creations
Copy link
Contributor

I tried to upload STEP files (inside a ZIP) and this patch did not work unfortunately, I added stp,step,STP,STEP to the extension list and text/plain to the mime types. unfortunately I can't remove text/html from disallowes extenstions to test if thats the issue.

@brianteeman
Copy link
Contributor

If they are inside a zip then the problem is not the same as this one. Allowed extensions and mime types would apply to the zip and not to the contents of the zip

@coolcat-creations
Copy link
Contributor

@brianteeman thank you, I opened issue #26408 for it.

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

Successfully merging this pull request may close these issues.

7 participants