[FIX] base: avoid fail with wrong mimetype

[ryce-odoo]

Uploading a WEBP or SVG file disguised with a proper file extension (JPG, PNG) will cause a traceback because img.image is not populated when there is an empty source, SVG, or WEBP file uploaded as this code should not be reached with these file types.

The reason this occurs is because we check for the file extension when deciding to post process an image, but when we get to initializing the ImageProcess object, we then check the actual file structure to verify the type of file.

This is a workaround for the time being, but should not be a final solution in future versions.

Adding a null check on img.image in the _postprocess_contents method in order to avoid attempting to access the size of this image when it is null.

Raises a user error in order to trigger the catch and exit the code while logging the error and 'Post processing ignored:'.

opw-3672250

I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

[robodoo]

.

[JKE-be]

webp and svg are not resizable, so why would you like to add it in ir.config_parameter 'image_autoresize_extensions' ???
If you don't add it, you will just not enter into the if is_image_resizable and...'s condition...

Your issue here, is that you try to upload a svg that you hide under the extension jpeg to try to bypass some others rules...
But your commit message is wrong, your UserError too. Maybe we should handle this case (raise a Warning if we detect wrong mimetype e.g.) to have a better uX, but this commit don't reflect it.

[ryce-odoo]

webp and svg are not resizable, so why would you like to add it in ir.config_parameter 'image_autoresize_extensions' ??? If you don't add it, you will just not enter into the if is_image_resizable and...'s condition...

Your issue here, is that you try to upload a svg that you hide under the extension jpeg to try to bypass some others rules... But your commit message is wrong, your UserError too. Maybe we should handle this case (raise a Warning if we detect wrong mimetype e.g.) to have a better uX, but this commit don't reflect it.

Hey @JKE-be
The issue here is that a customer may not know that they have an SVG or WEBP that had its extension changed. I feel like this is the most efficient way to get around it because raising the error within this try block does not raise any error in the frontend and just exits at the except block saying "Post processing ignored". I feel like this is the best outcome considering some customers may not be tech savvy enough to understand file types.

[JKE-be]

so please explain it in the commit msg.

Your problem is not that you really upload a svg file. But that you upload a svg file renamed as .png.
Else you will not enter in this code.

[ryce-odoo]

Hey @JKE-be

Sorry for the late reply was out of office. I have updated the commit message. Please let me know if there is anything else I should change.

[JKE-be]

@robodoo override=ci/style (TRY301, to discuss if we keep or not this new rule (ruff mig), but don't rewrite this code now)

Did you check what introduce this regression in saas-16.4 ? What is the difference with saas-16.3 ?

[ryce-odoo]

Hey @JKE-be

Yeah it looks like the change that introduced this regression was adding this to the if condition:
"or (source[0:4] == b'RIFF' and source[8:15] == b'WEBPVP8')"

Looks like this change was meant to add compatibility for webp's and with further testing it looks like this issue has always existed with SVG's. However, I don't think anyone would try converting vectors to raster through the file extension like that.

Here's the commit in question:
d1292a9

[bso-odoo]

@ryce-odoo Can you elaborate on the scenario that causes the error (in the commit message) ?
In the opw, it is about an upload in the Documents app. The error also occurs when trying to upload such an image through the website builder inside an image field (e.g. a product image in eCommerce). But other uploads such as an image inside a website page do not cause the error.
Both failing uploads are solved with this PR. 👍

This looks good to me. Maybe we should find a way to make the "incoming" mimetype not trustworthy, and have _compute_mimetype not rely on the value if it was sent from specific routes, but that might not be a change for a stable version.

[ryce-odoo]

Hi @bso-odoo
I have updated the commit message. Let me know what you think.

[bso-odoo]

@ryce-odoo Nice, but what I meant was that it did not mention which upload feature you are talking about.
E.g.: state that, for example, the problem occurs when uploading such a file "in the Documents app".

[ryce-odoo]

Hey @bso-odoo
The issue on the original ticket was from the product documents page, but this happens anywhere where you upload an image to Odoo. It just depends on how much obfuscation is at play if you get this exact error message telling you where the issue is. For example, if you upload it to the chatter or documents app it will just give you a server error and if you upload it to the attachments, product documents, or website you will get the same traceback that tells you what the problem is. I don't know if it is necessary to clarify when this happens because this is an issue with base.

Changes implemented

[ryce-odoo]

Hey @bso-odoo @JKE-be

Can either of you guys give this a final check and approve for merge?

[JKE-be]

@robodoo delegate=bso-odoo

[JKE-be]

@robodoo override=ci/style

[ryce-odoo]

Hey @bso-odoo
I made that change and added it to the pot file.

[ryce-odoo]

Hey @bso-odoo
I resolved the scoping issue with _ and included a test. Can you check out the test for me? Should be enough to ensure that the functionality works.

[ryce-odoo]

Hey @bso-odoo
I changed the svg to be more legit and changed the commit title. Had to shorten it a bit to fit in the character limit.
Are we good now?

[bso-odoo]

@ryce-odoo Thanks for all the changes.

It seems runbot is not happy 😓 :

• For the upgrade issue, maybe rebasing your PR on the current saas-16.4 will solve it.
• For the styling TRY301 issue, I guess you'll have to directly log the error message and return - instead of relying on the catch to do it. Maybe you'll have a better idea that would allow for reusing the catch block's text...

[ryce-odoo]

Hey @bso-odoo
I have rebased and made the change to satisfy the style check. Everything seems to be in order now every check has passed.

[bso-odoo]

@ryce-odoo 😬 Since this is just a console-logged message after all, I de-translated it. Really sorry 🙏
I have kept the renaming of the unused _ because that is prone to generate bugs.

[ryce-odoo]

Hey @bso-odoo
Sounds good I was confused whether or not to keep the translation, but in the end even with the old version that didn't pass the style check, the message would only ever be logged in the console.

I think its ready to merge when you are.

[bso-odoo]

@robodoo r+

[ryce-odoo]

Thanks @bso-odoo !