[FIX] tools: properly guess image/svg+xml #68211

qsm-odoo · 2021-03-22T16:37:37Z

One of the two "magic" library we allow to use to guess to mimetype of
data is sometimes returning "image/svg" instead of the wanted
"image/svg+xml".

robodoo · 2021-03-22T16:38:00Z

Pull request status dashboard.

qsm-odoo · 2021-03-22T16:39:41Z

Not sure who to ping for this kind of code review... maybe you @xmo-odoo? What do you think about this quick fix?

odoo/tools/mimetypes.py

mart-e

One of the two library [...] is somtimes [...]

This feels very uncertain :D When? Why? Do you have a bug report to link to?
If a library is returning a wrong result, we should report this upstream, and only if we can not backport it (e.g. fix included in a release with breaking changes or unmaintained), we should monkey patch it.
I can see this kind code hanging around in 6 years with nobody remembering why it was put there in the first place.

Regarding the patch itself, if I am not mistaken, the python-magic package does not identify the mimetype itself, it loads the libmagic library installed on the system.

So

isn't the python-magic provided by Debian has the same issue?
isn't the issue in one of these system library instead?

qsm-odoo · 2021-03-23T09:39:20Z

This feels very uncertain :D When? Why?

Not uncertain at all... when? When I use a SVG file (= sometimes). Why? I don't know, I did not write the lib, the image/svg mimetype simply is not a valid mimetype that I know of. We could make this patch even in ir.attachment to not allow to save the "image/svg" mimetype and close our eyes on the guess method if we were careless 😉

Do you have a bug report to link to?

No, otherwise I would have 😉 It is just a bug we detected on our own. Why did not we see it before? Because we never guess the mimetype of svg file using those libraries as we mostly guess the mimetype checking the filename, see _compute_mimetype.

isn't the python-magic provided by Debian has the same issue?

@JKE-be is this what you tested yesterday? I remember the conflict between the two python-magic libraries but did not know about them using a random magic package as explained by @mart-e 🤷 Anyways, you know better than me what to do, you found the fix in 5minutes in the first place 😉

mart-e · 2021-03-23T09:53:38Z

So if you can reproduce it on your computer, can you check which implementation/version of magic is installed on your computer? Because I tried localy and it worked well for me with

$ pacman -Qo /usr/lib/libmagic.so
/usr/lib/libmagic.so is owned by file 5.39-1
$ file --mime-type /tmp/test.svg
/tmp/test.svg: image/svg+xml

(and same with invoking magic.from_buffer(data, mime=True))

xmo-odoo · 2021-03-23T09:53:58Z

@mart-e searching for "image/svg" libmagic seems to have a fair number of hits, but most are duplicates of mdn/kuma#6125, which the MDN folks apparently didn't bother investigating, they just monkepatched the incorrect mimetype: mdn/kuma#6144

And from the comment on mdn/kuma#6144 it might not even be a reliable misdetection:

The magic.Magic() will, for unknown reasons, sometimes think an SVG image's mime type is image/svg

mart-e · 2021-03-23T10:00:35Z

maybe file/file@bc86bfe (changed at file/file@1a08bb5 ) ?

xmo-odoo · 2021-03-23T10:01:43Z

Also wrt "two libraries" it kinda looks like the debian one has been deprecated / removed / ...? starting from buster the package uses the description of "pypi magic" and links to its github instead of the old magic from file(1). Though if the issue is in libmagic I expect that will be buggy either way.

(/cc @d-fence)

JKE-be · 2021-03-23T10:01:50Z

You are right @mart-e It happens in both cases.
Yesterday when I try to reproduce with debian package I install py2 package instead of py3 probably; my bad.

But the bug is really not "uncertain", install debian python3-magic or with pip3 and try to upload any svg and you will see that it returns the wrong 'image/svg' mimetype instead of "image/svg+xml"

Until now, we don't see the error because most of us don't upload svg, or just don't have the "unrequired" magic package.
So we fallback on our own guess_mimetype which one return the right 'image/svg+xml'

Let me know if we need to rewrite the PR to fix both import (debian or pip) or if you have better plan (like remove dependencies to magic?)...

JKE-be · 2021-03-23T10:05:37Z

ps: add xml on top of the svg will works too...

<?xml version="1.0" encoding="UTF-8"?>
or/and
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">

Maybe it is the simple solution

JKE-be · 2021-03-23T10:15:49Z

After more discussion, we continue to think that we should never use image/svg and apply something similar to this PR like
KUMA that you linked previously

JKE-be · 2021-03-23T10:23:13Z

Although it is not required, an SVG file may start with an XML declaration, indicating the XML version used and the character encoding. The following explicitly declares the default (version 1.0) XML syntax in a file with a UTF-8 Unicode character set:

The XML declaration is only required if you are using a non-Unicode character encoding (technically, anything other than UTF-8 or UTF-16). We always use UTF-8, so we don’t often use an XML declaration. If used, the declaration should always appear on the first line of the file.

You may also include an SGML DOCTYPE declaration, but it is no longer recommended for SVG. The DOCTYPE points to the document type definition of the SVG file format, and is used by some validators and code editing tools. However, some of these validation tools will not recognize perfectly valid XML content from non-SVG namespaces.

src: from the O'Reilly Media book by Amelia Bellamy-Royds, Kurt Cagle, and Dudley Storey.

mart-e · 2021-03-23T10:45:02Z

But the bug is really not "uncertain", install debian python3-magic or with pip3 and try to upload any svg and you will see that it returns the wrong 'image/svg' mimetype instead of "image/svg+xml"

>>> ms.file("/tmp/icon.svg")
'text/plain'

My debian is too old :D

After more discussion, we continue to think that we should never use image/svg

I see your point, especially if we can see that file misidentified it during 3 years according to the commits I gave above.
On the server side we should be good as always using startswith for comparison

odoo/odoo/fields.py

Lines 1997 to 2000 in a7110b6

    
           # Full mimetype detection 
        
           if (guess_mimetype(decoded_value).startswith('image/svg') and 
        
                   not record.env.is_system()): 
        
               raise UserError(_("Only admins can upload SVG files."))

But it's not always the case client side

odoo/addons/web_editor/static/src/js/wysiwyg/widgets/media.js

Line 882 in a7110b6

if (response.headers.get('content-type') === 'image/svg+xml') {

Anyway, this patch should also be applied on the Debian version, and with a good comment.

Now, is it bad enough to change this in 12.0?

JKE-be · 2021-03-23T10:51:58Z

Now, is it bad enough to change this in 12.0?

Imo yes, since it doesn't work in v12 and we add svg support in v12: https://github.com/odoo/odoo//1be50fdeafcd2b94f7b2f47d6e5bc4b6ebaceadd

We always follow the same policy: fix bug in first version supported where code is still the same and fwd port will not have conflict. I don't see any reason to don't follow it now.

xmo-odoo · 2021-03-23T12:52:53Z

@qsm-odoo could you explain the problematic use-case though? As in, how you stumbled upon this issue.

d-fence · 2021-03-23T12:58:56Z

Here are my 2 cents:
Only certain type of svg files are impacted. The files that starts with <svg.
Look at the magic file code for svg in version 5.35 which is the Debian Buster version.

Now fixed in master but not yet in Debian Buster (fixed with magic 5.39 in Debian Bullseye)

You can easily test that by creating a file with only <svg at its beginning ... and use file on it.

JKE-be · 2021-03-23T13:01:38Z

Hello @xmo-odoo

Have magic installed on your computer
Install website / create a new one... in master

It will load https://github.com/odoo/odoo/blob/master/addons/website/static/src/img/website_logo.svg?short_path=991f026 as attachment (the default website logo)

which one only contains svg without (optional ?) xml version and doctype.

At this point, you have no error, but the mimetype in db ir.attachment is image/svg

Now go to your website, and you will see a "placeholder broken" image on Chrome, or nothing on Firefox.
Check your network and you see that the svg is loaded with the wrong mimetype ofc.

update in DB or apply patch and create a new website, now that logo have the right mimetype, you can see you logo appears on your website

JKE-be · 2021-03-23T13:03:37Z

@d-fence , yes it was the other solution proposed here #68211 (comment)

But since browser cannot display image/svg, idea was to fix for all future svg too

d-fence · 2021-03-23T13:09:14Z

IMHO the present fix makes sense and to answer @mart-e , I think it's needed in 12.0 as the Debian versions were not fixed at the time of 12.0 release ... with a good comment that explains why.

Without this 'XML declaration', if you have magic installed, the mime type is image/svg instead of image/svg+xml and browser don't render it. This commit is related to odoo#68211 meanwhile a more generic solution (if needed).

xmo-odoo · 2021-03-23T14:13:57Z

Did you mean, change the api of guess_mimetypes in stable ?

Not sure what you're responding to, but the version I'm suggesting doesn't change the API of guess_mimetypes.

JKE-be · 2021-03-23T14:18:07Z

guess_mimetypes take a 3rd params _guesser now. What is considered as one change of api for Odoo no ?
My first version was a partial to avoid this new params, but code became more complexe imo.

xmo-odoo · 2021-03-23T14:31:23Z

guess_mimetypes take a 3rd params _guesser now. What is considered as one change of api for Odoo no ?

That's just an optimisation (so the guesser is looked up as a local instead of a global), the way guess_mimetypes is called doesn't change in any way, nor is this parameter supposed to be externally supplied (hence the _ prefix). In fact it probably can't be as the "reference" version (the one if no libmagic is installed) was not changed.

Without this 'XML declaration', if you have magic installed, the mime type is image/svg instead of image/svg+xml and browser don't render it. This commit is related to #68211 meanwhile a more generic solution (if needed). closes #68239 Signed-off-by: Quentin Smetz (qsm) <qsm@odoo.com>

odony · 2021-09-20T12:02:28Z

odoo/tools/mimetypes.py

        ms = magic.open(magic.MAGIC_MIME_TYPE)
        ms.load()
-        guess_mimetype = lambda bin_data, default=None: ms.buffer(bin_data)
+        _guesser = ms.buffer
+    def guess_mimetypes(bin_data, default=None):


Suggested change

def guess_mimetypes(bin_data, default=None):

def guess_mimetype(bin_data, default=None):

Magic return image/svg instead of image/svg+xml if file starts with <svg

JKE-be · 2021-09-20T16:04:08Z

@robodoo r+

For tracking, a svg with doctype svg and padding is considered as svg by libmagic, but is it not an issue since a browser cannot display it and consider that <?xml should be the first chars from the line.

robodoo · 2021-09-20T16:04:29Z

@JKE-be, you may want to rebuild or fix this PR as it has failed CI.

Magic return image/svg instead of image/svg+xml if file starts with <svg closes #68211 Signed-off-by: Jérémy Kersten (jke) <jke@openerp.com>

fw-bot · 2021-09-24T05:53:03Z

This pull request has forward-port PRs awaiting action (not merged or closed): #76876

fw-bot · 2021-09-25T05:53:29Z

This pull request has forward-port PRs awaiting action (not merged or closed): #77188, #77193

fw-bot · 2021-09-26T05:53:42Z

This pull request has forward-port PRs awaiting action (not merged or closed): #77188, #77193

fw-bot · 2021-09-27T05:53:20Z

This pull request has forward-port PRs awaiting action (not merged or closed): #77188, #77193

fw-bot · 2021-09-28T05:53:12Z

This pull request has forward-port PRs awaiting action (not merged or closed): #77188

fw-bot · 2021-09-29T05:53:05Z

This pull request has forward-port PRs awaiting action (not merged or closed): #77188

qsm-odoo self-assigned this Mar 22, 2021

qsm-odoo requested review from a team as code owners March 22, 2021 16:37

seb-odoo reviewed Mar 22, 2021

View reviewed changes

odoo/tools/mimetypes.py Outdated Show resolved Hide resolved

C3POdoo added the RD research & development, internal work label Mar 22, 2021

qsm-odoo force-pushed the 12.0-fix-magic-lib-svg-qsm branch from 00a687b to c983a67 Compare March 22, 2021 17:50

mart-e reviewed Mar 23, 2021

View reviewed changes

JKE-be closed this Mar 23, 2021

JKE-be reopened this Mar 23, 2021

qsm-odoo mentioned this pull request Aug 18, 2021

[IMP] website: add support for svg logo in configurator #74956

Closed

JKE-be force-pushed the 12.0-fix-magic-lib-svg-qsm branch from 6e6e947 to e37dcb0 Compare September 16, 2021 10:44

C3POdoo requested review from a team September 16, 2021 10:46

JKE-be changed the title ~~[FIX] tools: properly guess image/svg+xml mimetype with first magic lib~~ [FIX] tools: properly guess image/svg+xml Sep 20, 2021

odony reviewed Sep 20, 2021

View reviewed changes

[FIX] tools: properly guess image/svg+xml mimetype with magic lib

6dc907e

Magic return image/svg instead of image/svg+xml if file starts with <svg

JKE-be force-pushed the 12.0-fix-magic-lib-svg-qsm branch from e37dcb0 to 6dc907e Compare September 20, 2021 13:21

robodoo pushed a commit that referenced this pull request Sep 20, 2021

[FIX] tools: properly guess image/svg+xml mimetype with magic lib

542cbe8

Magic return image/svg instead of image/svg+xml if file starts with <svg closes #68211 Signed-off-by: Jérémy Kersten (jke) <jke@openerp.com>

fw-bot mentioned this pull request Sep 20, 2021

[FW][FIX] tools: properly guess image/svg+xml #76868

Closed

robodoo closed this Sep 20, 2021

robodoo temporarily deployed to merge September 20, 2021 16:49 Inactive

This was referenced Sep 20, 2021

[FW][FIX] tools: properly guess image/svg+xml #76873

Closed

[FW][FIX] tools: properly guess image/svg+xml #76876

Closed

This was referenced Sep 24, 2021

[FW][FIX] tools: properly guess image/svg+xml #77136

Closed

[FW][FIX] tools: properly guess image/svg+xml #77188

Closed

[FW][FIX] tools: properly guess image/svg+xml #77193

Closed

qsm-odoo deleted the 12.0-fix-magic-lib-svg-qsm branch September 27, 2021 07:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] tools: properly guess image/svg+xml #68211

[FIX] tools: properly guess image/svg+xml #68211

qsm-odoo commented Mar 22, 2021

robodoo commented Mar 22, 2021

qsm-odoo commented Mar 22, 2021

mart-e left a comment •

edited

qsm-odoo commented Mar 23, 2021

mart-e commented Mar 23, 2021

xmo-odoo commented Mar 23, 2021 •

edited

mart-e commented Mar 23, 2021 •

edited

xmo-odoo commented Mar 23, 2021 •

edited

JKE-be commented Mar 23, 2021 •

edited

JKE-be commented Mar 23, 2021 •

edited

JKE-be commented Mar 23, 2021

JKE-be commented Mar 23, 2021

mart-e commented Mar 23, 2021

JKE-be commented Mar 23, 2021 •

edited

xmo-odoo commented Mar 23, 2021

d-fence commented Mar 23, 2021 •

edited

JKE-be commented Mar 23, 2021

JKE-be commented Mar 23, 2021 •

edited

d-fence commented Mar 23, 2021

xmo-odoo commented Mar 23, 2021 •

edited

JKE-be commented Mar 23, 2021

xmo-odoo commented Mar 23, 2021

odony Sep 20, 2021

JKE-be commented Sep 20, 2021

robodoo commented Sep 20, 2021

fw-bot commented Sep 24, 2021

fw-bot commented Sep 25, 2021

fw-bot commented Sep 26, 2021

fw-bot commented Sep 27, 2021

fw-bot commented Sep 28, 2021

fw-bot commented Sep 29, 2021

	def guess_mimetypes(bin_data, default=None):
	def guess_mimetype(bin_data, default=None):

[FIX] tools: properly guess image/svg+xml #68211

[FIX] tools: properly guess image/svg+xml #68211

Conversation

qsm-odoo commented Mar 22, 2021

robodoo commented Mar 22, 2021

qsm-odoo commented Mar 22, 2021

mart-e left a comment • edited

Choose a reason for hiding this comment

qsm-odoo commented Mar 23, 2021

mart-e commented Mar 23, 2021

xmo-odoo commented Mar 23, 2021 • edited

mart-e commented Mar 23, 2021 • edited

xmo-odoo commented Mar 23, 2021 • edited

JKE-be commented Mar 23, 2021 • edited

JKE-be commented Mar 23, 2021 • edited

JKE-be commented Mar 23, 2021

JKE-be commented Mar 23, 2021

mart-e commented Mar 23, 2021

JKE-be commented Mar 23, 2021 • edited

xmo-odoo commented Mar 23, 2021

d-fence commented Mar 23, 2021 • edited

JKE-be commented Mar 23, 2021

JKE-be commented Mar 23, 2021 • edited

d-fence commented Mar 23, 2021

xmo-odoo commented Mar 23, 2021 • edited

JKE-be commented Mar 23, 2021

xmo-odoo commented Mar 23, 2021

odony Sep 20, 2021

Choose a reason for hiding this comment

JKE-be commented Sep 20, 2021

robodoo commented Sep 20, 2021

fw-bot commented Sep 24, 2021

fw-bot commented Sep 25, 2021

fw-bot commented Sep 26, 2021

fw-bot commented Sep 27, 2021

fw-bot commented Sep 28, 2021

fw-bot commented Sep 29, 2021

mart-e left a comment •

edited

xmo-odoo commented Mar 23, 2021 •

edited

mart-e commented Mar 23, 2021 •

edited

xmo-odoo commented Mar 23, 2021 •

edited

JKE-be commented Mar 23, 2021 •

edited

JKE-be commented Mar 23, 2021 •

edited

JKE-be commented Mar 23, 2021 •

edited

d-fence commented Mar 23, 2021 •

edited

JKE-be commented Mar 23, 2021 •

edited

xmo-odoo commented Mar 23, 2021 •

edited