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.

Sign up for GitHub

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

Jump to bottom

[FIX] base: ir_qweb_field:image: handle webp mimetype #161931

Closed
wants to merge 1 commit into from
Closed

[FIX] base: ir_qweb_field:image: handle webp mimetype #161931

wants to merge 1 commit into from

Conversation

kebeclibre
Copy link
Contributor

See discussions on #85494. TLDR: webp image format needs to be supported, but we should avoid going through the Pillow library as it is largely unsafe for that format. jpg attachment are created in JS at upload time.
Wkhtmltopdf doesn't support webp, so, in reports, we should display one of those jpg copies This work is handled by ir.qweb: _get_converted_image_data_uri which is used as:

<img src="image_data_uri(some_b64value)" />

The mentionned PR did not however adapt the ir.qweb.field.image that, when passed the option qweb_img_raw_data should return a base64 url such as data:[mimetype],base64,[datas]. usage:

<span t-field="object.image_field" t-options-qweb_img_raw_data="1" />

Hence, before this commit, there was a crash as we tried to pass that value to PIL.

After this commit, there is no crash, and the image displays correctly as JPG in the PDF

opw-3859423

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

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

@robodoo
Copy link
Contributor

robodoo commented Apr 15, 2024 •
edited

Pull request status dashboard.

@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Apr 15, 2024
@kebeclibre kebeclibre marked this pull request as ready for review April 17, 2024 14:18
@kebeclibre
Copy link
Contributor Author

@bso-odoo Hello, can i get a small review here ? cheers

@C3POdoo C3POdoo requested review from a team, xmo-odoo and HydrionBurst and removed request for a team April 17, 2024 14:21
@bso-odoo
Copy link
Contributor

@kebeclibre I am not very familiar with Studio and reports, so I might not have tried the correct scenario.
Here is what I did using your branch:

  • Go to Invoicing > Studio 🛠️ > Reports > Invoices
  • Put the cursor above the "Due Date"
  • Type "/image" to insert an image
  • Upload a webp
  • Save
  • Click on 🖨️ Print Preview

=> The image does not show up in the generated PDF.

I was indeed not aware of this qweb_img_raw_data option.
If this PR solves a different scenario, could you please describe it ? 🙏

@bso-odoo
Copy link
Contributor

@kebeclibre Ok, got it, I had to use /field instead. 🤦‍♂️ That one works indeed. But maybe you want to also address the /image in this PR ?

bso-odoo
bso-odoo reviewed Apr 18, 2024
View reviewed changes
Copy link
Contributor

@bso-odoo bso-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the commit message:

  • mentionned => mentioned
  • aren't the lines supposed to be truncated at 72 columns (when possible) ?

addons/web/tests/test_ir_qweb.py Outdated Show resolved Hide resolved
odoo/addons/base/models/ir_qweb_fields.py Outdated Show resolved Hide resolved
@kebeclibre
Copy link
Contributor Author

@kebeclibre Ok, got it, I had to use /field instead. 🤦‍♂️ That one works indeed. But maybe you want to also address the /image in this PR ?

I don't think it is in the same scope. For the /image there is still the possibility to manually wrap the src in image_data_uri. Besides, I'm not really sure if every use case for /image can be treated the same way.

Other wise, changes done @bso-odoo

@odoo/rd-security Can i get an override here ? There is one impacted line that triggers the security CI but the it is just a variable name.

@xmo-odoo
Copy link
Collaborator

There is one impacted line that triggers the security CI but the it is just a variable name.

It's not "just a variable name":

fp – A filename (string), os.PathLike object or a file object.

So Image.open has filesystem access (though because it expects an image it's likely harder to exfilter data) ;)

Here it's safe because it's given an in-memory file-like payload, so there's no way for a user to poke around (except at the image decoders).

xmo-odoo
xmo-odoo requested changes Apr 22, 2024
View reviewed changes
Copy link
Collaborator

@xmo-odoo xmo-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robodoo override=ci/security

odoo/addons/base/models/ir_qweb_fields.py Outdated Show resolved Hide resolved
odoo/addons/base/models/ir_qweb_fields.py Show resolved Hide resolved
odoo/addons/base/models/ir_qweb_fields.py Outdated Show resolved Hide resolved
odoo/addons/base/models/ir_qweb_fields.py Outdated Show resolved Hide resolved
@kebeclibre
[FIX] base: ir_qweb_field:image: handle webp mimetype
efe2227 
See discussions on odoo#85494.
TLDR: webp image format needs to be supported, but we should avoid
going through the Pillow library as it is largely unsafe for that format.
jpg attachment are created in JS at upload time.
Wkhtmltopdf doesn't support webp, so, in reports, we should display one of those jpg copies.
This work is handled by `ir.qweb: _get_converted_image_data_uri` which is used as:
```xml
<img src="image_data_uri(some_b64value)" />
```

The mentioned PR did not however adapt the ir.qweb.field.image that, when passed the option `qweb_img_raw_data`
should return a base64 url such as `data:[mimetype],base64,[datas]`.
usage:
```xml
<span t-field="object.image_field" t-options-widget="'image'" t-options-qweb_img_raw_data="1" />
```

Hence, before this commit, there was a crash as we tried to pass that value to PIL.

After this commit, there is no crash, and the image displays correctly as JPG in the PDF

opw-3859423
@kebeclibre
Copy link
Contributor Author

@xmo-odoo changes done

@bso-odoo bso-odoo changed the title WIP [FIX] base: ir_qweb_field:image: handle webp mimetype [FIX] base: ir_qweb_field:image: handle webp mimetype Apr 23, 2024
@bso-odoo
Copy link
Contributor

@robodoo r+

@robodoo
Copy link
Contributor

robodoo commented Apr 23, 2024

I'm sorry, @bso-odoo. I'm afraid I can't do that.

@rdeodoo
Copy link
Contributor

rdeodoo commented Apr 23, 2024

@robodoo delegate=@bso-odoo

@bso-odoo
Copy link
Contributor

@robodoo r+

robodoo pushed a commit that referenced this pull request Apr 23, 2024
@kebeclibre
[FIX] base: ir_qweb_field:image: handle webp mimetype
9056a4b 
See discussions on #85494.
TLDR: webp image format needs to be supported, but we should avoid
going through the Pillow library as it is largely unsafe for that format.
jpg attachment are created in JS at upload time.
Wkhtmltopdf doesn't support webp, so, in reports, we should display one of those jpg copies.
This work is handled by `ir.qweb: _get_converted_image_data_uri` which is used as:
```xml
<img src="image_data_uri(some_b64value)" />
```

The mentioned PR did not however adapt the ir.qweb.field.image that, when passed the option `qweb_img_raw_data`
should return a base64 url such as `data:[mimetype],base64,[datas]`.
usage:
```xml
<span t-field="object.image_field" t-options-widget="'image'" t-options-qweb_img_raw_data="1" />
```

Hence, before this commit, there was a crash as we tried to pass that value to PIL.

After this commit, there is no crash, and the image displays correctly as JPG in the PDF

opw-3859423

closes #161931

Signed-off-by: Benoit Socias (bso) <bso@odoo.com>
@fw-bot fw-bot mentioned this pull request Apr 23, 2024
Closed
@robodoo robodoo closed this Apr 23, 2024
This was referenced Apr 23, 2024
Closed
Closed
Closed
@fw-bot
Copy link
Contributor

fw-bot commented Apr 27, 2024

@kebeclibre @bso-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

2 similar comments
@fw-bot
Copy link
Contributor

fw-bot commented Apr 28, 2024

@kebeclibre @bso-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

@fw-bot
Copy link
Contributor

fw-bot commented Apr 29, 2024

@kebeclibre @bso-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

@fw-bot fw-bot deleted the saas-16.4-img-webp-lpe branch May 7, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bso-odoo bso-odoo bso-odoo left review comments

@xmo-odoo xmo-odoo xmo-odoo approved these changes

@HydrionBurst HydrionBurst Awaiting requested review from HydrionBurst HydrionBurst was automatically assigned from odoo/rd-framework-py

Assignees
No one assigned
Labels
OE the report is linked to a support ticket (opw-...)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@kebeclibre @robodoo @bso-odoo @xmo-odoo @rdeodoo @fw-bot @C3POdoo