-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
Improve avatar upload experience #3181
Conversation
Fixes #3055 - On the frontend, accept only image types as a hint to the OS file picker - On the backend, use `getimagesize` as an additional validation guard against non-image files with image extensions. This isn't necessary for security, but results in less confusing error mesages.
[ci skip] [skip ci]
src/User/AvatarValidator.php
Outdated
if (! in_array($guessedExtension, $allowedTypes)) { | ||
$stream = $file->getStream(); | ||
$tmpFilename = $stream->getMetadata('uri'); | ||
$notImage = ! getimagesize($tmpFilename); | ||
|
||
if ($notImage || ! in_array($guessedExtension, $allowedTypes)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getimagesize
doesn't seem to be a reliable way of checking if the file is an image (https://www.php.net/manual/en/function.getimagesize.php)
I'm thinking it might be better to catch errors when creation the intervention image object, logging the error in the logs, and returning a better response to the end user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use that logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't get rid of the mimes check though 🤔 ? the try catch should act as a second check.
Fixes #3055
getimagesize
as an additional validation guard against non-image files with image extensions. This isn't necessary for security, but results in less confusing error mesages.