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

[4.x] Uniform image validation #9284

Closed
wants to merge 4 commits into from
Closed

[4.x] Uniform image validation #9284

wants to merge 4 commits into from

Conversation

royduin
Copy link
Contributor

@royduin royduin commented Jan 9, 2024

Within #7666 the image validation is moved to the new ImageValidator but that's not used yet on the asset itself. We encountered some issues with this in the assets browser and end users uploading images where the extension and mime type do not match. For example a JPG saved as .png will result in an exception:

Image [testomonial.png] does not actually appear to be a valid image.

With a broken thumb in the asset browser:
image

By using the same logic here will fix this and will show a svg placholder thumb as there is something wrong with the image:
image

Another approach is to remove the mime type check as images with a extension mismatching the mimetype can still be processed and thumb generation works. But maybe that could lead to other problems later? Can imagine not all browsers like images with a mismatching mime.

@royduin royduin changed the title Uniform image validation [4.x] Uniform image validation Jan 9, 2024
@royduin
Copy link
Contributor Author

royduin commented Jan 9, 2024

Seems like there are some issues with Github currently going on: https://www.githubstatus.com/incidents/pxg3dz4yg7lp, actions should run normally when that's fixed.

@jasonvarga
Copy link
Member

I don't understand why the tests don't run on this PR. Weird.

@jasonvarga
Copy link
Member

Oh yes they continue indefinitely because this code introduces an infinite loop.

@jasonvarga
Copy link
Member

Closing as this causes the infinite loop.

  1. Checking mime type causes generating of meta data
  2. Generating meta data requires checking if its an image
  3. Checking if its an image (with your code) will check the mime type
  4. Go to step 1.

In the previous code, step 3 will check the file extension and not the mime type, and continue on its way.

Other than the infinite loop, this makes isImage's logic different from the similar methods: isSvg, isVideo, isMedia, etc.

If the only problem you're encountering is that jpgs saved with other extensions aren't generated, I'm fine with that as it is the security measure kicking in. Generally you should be saving with the correct file extension.

@jasonvarga jasonvarga closed this Jan 9, 2024
@royduin royduin deleted the patch-1 branch January 10, 2024 08:59
@royduin
Copy link
Contributor Author

royduin commented Jan 10, 2024

Follow-up: #9290 + maybe we should validate this on upload so we can prevent users from uploading incorrect files.

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.

2 participants