Image preview not working with SVG

[germain-gg]

Steps to reproduce

1. Go to a room
2. Upload an SVG
3. Click on that SVG

What happened?

The lightbox opens up but the image is not shown anywhere

What did you expect?

For the image to be displayed, centered in the lightbox

Operating system

macOs

Browser information

Chrome / Firefox

URL for webapp

develop.element.io

[HarHarLinks]

For me the preview (thumbnails which are pngs generated from svgs) are working, but not the lightbox (full resolution actual svg).
An explanation is offered in #15094 (comment).
This is contrary to #10833 (might be outdated).

[G2G2G2G]

Converting it to png for the lightbox is perfectly acceptable, we use a lot of svgs internally and the thumbnails are fine as pngs, people understand what it is. Privacy is understandable, nobody needs vector scaling in an image preview in lightbox.. (preferably when saving the image as it does the svg though, obviously the download button does)

[alexanderstephan]

Hey everyone, I would like to work on this issue (in the context of a lab course at my university) sometime in the near future in order to improve the whole situation regarding svgs.

I just wanted to give a quick summary of what problems I could find as of today:

1. From what I can tell the preview thumbnail for svgs does not work at the moment and is unrelated to encryption. I tested stable and development in Firefox and Chromium each.

2. I also noticed that sometimes the thumbnail is empty (mx_no-image-placeholder) and other times instead a download badge for the file is displayed. Right now, I am not sure when each of these is triggered. I also used the same files.
   

3. The lightbox viewer for svgs still works for unencrypted rooms, but not for encrypted. Sometimes it has a strange scaling behavior where the svg gets cut off like this
   

If some of these just mentioned points don't match your current experience, please let me know.

[G2G2G2G]

@alexanderstephan there's some cross site scripting vulnerability in them (see that post above mine), but the thumbnails have worked for me for at least a year (and my entire office can view them fine too)

I think of SVG does indeed have that vulnerability it should just be treated as either:

• a regular file like your first screenshot and not try to thumbnail at all
• a png image, convert it to png for both thumbnail and preview

[HarHarLinks]

Ideally the SVG could be cleared of potentially dangerous things, disallow <script> tags and any field containing an URI. But I am not an expert in how (else) SVG XSS might be done and what dangers could remain. As much as I'd like SVG supported, I do support the "better safe than sorry" approach as necessary.

[alexanderstephan]

@G2G2G2G Interesting, I asked a lot of people to try it as well, but it did work for none of them yet. But that fits in with my believe that behavior is not consistent right now.

I am also wondering why the Lightbox worked for unencrypted Rooms when the vulnerability should be identical. Converting to png would a possible solution, but it might not be ideal, especially for larger images combined with larger screens.

Also, thumbnails should not oppose a thread as far I can tell, since the svg is contained within an img tag. So no sanitizations should be needed in this case.

However, we could just use something like DOMPurify to solve the problem in general. I think this was mentioned in a similiar issue before.

[G2G2G2G]

@alexanderstephan lmao that's hilarious, the lightbox DOES work with unencrypted (non e2e to be 100% clear) rooms.

I've tested this since you mentioned it, you can post an svg into a non e2e room and forward it to another room, any type, and lightbox still works there. Massive oversight if their goal truly was to stop all SVGs for some XSS attack @t3chguy (since he posted that in the other thread, might be good to know that the filtering out doesn't actually work)

I like your DOMpurify idea. I do not understand the base64 styled attacks etc but from what I read it unfortunately isn't as easy as just deleting all of the "http/s ://" etc..

In inkscape, I use the optimized SVG saving, which uses this to strip out everything:
https://github.com/scour-project/scour
(though not something we may want since it does change the data inside of an SVG more than just "purifying it" should/would)

[alexanderstephan]

@G2G2G2G Nice idea with scour! But I agree, I think we should not alter the data (if not needed for security or to fix malformed code) and also DOMPurify is a project mainly dedicated to enhance security. From what I've seen scour is rather for optimizing performance. So I would prefer DOMPurify for those reasons.

I finally had some time again to do some more testing. I actually could reproduce the behavior regarding forwarding.

Update: Yep, in order to safely display SVG files within e2e encrypted rooms, it should always get sanitized, or the complete way blob urls are used, must be changed, I think.

I just wondered at first whether files would be encrypted in general after being forwarded to e2e rooms, but that does not seem to be the case. It makes sense to me since security is compromised when sending the data for the first time without encryption. I am preparing a PR that proposes a fix for the lightbox issue, it should be ready soon! :)

[alexanderstephan]

@G2G2G2G I played around with using DOMPurify and came up with a Draft PR. DOMPurify actually should be secure by default, even for base64 style attacks. I think the name is a bit misleading, as it actually does more then purifying.

It has a very tight security policy and a mailing list, and tries to adress every possible security incident that has occured for SVGs. But I still need to figure out some things, as now the downloaded file gets also sanitized.

[G2G2G2G]

I read that comment in your previous post edit, it doesn't really make sense to me that if it has issues with e2e they shouldn't also be blocked with regular rooms. So yea I agree with running your dompurify draft in all rooms for svgs.
It's not like svgs are all that popular or anything, not gonna explode someone's old device.. So IMO it's the right road you're going down with "it should always get sanitized"
Especially when dendrite / element want to go to full decentralization and run the server within the client too, it'll be more important

[HarHarLinks]

I have just tried this again, and it now displays both thumbnail and full resolution (presumably the actual svg) in the lightbox for me.

An issue seems to be that in contrast to pixel based images, element/electron/chromium doesn't seem to have a proper sense of scale for vector images: the image is displayed bigger than my viewport is, and can't be zoomed.

[alexanderstephan]

Very interesting, but also works for me now (non e2e of course).

If you look at the source code of the svg, does at have a specified height and width? If not, what happens if you add it? From my experience this happens when the size can't be inferred from the file. In this case the svg could be limited to a percentage of the windows width e.g., I think.

[G2G2G2G]

svgs aren't like regular images (unfortunately) in browsers, their scale can be all out of whack depending on viewbox and their internal sizes..
setting a static height: and width: would fix it, using javascript to get the screen size or whatever
this is why all the icon bloat packages like font awesome embed them into fonts instead of using layered single svg files with the <g> tag (like <g id="item1">imgdata</g> & <img src="coolfile.svg#item1) which is superior in every way except sizing/positioning/working with them.

[HarHarLinks]

non e2e of course

correct, limited to non-e2ee, I seem to have missed this was pointed out earlier.

If you look at the source code of the svg, does at have a specified height and width?

The <svg> tag has x="0px" y="0px" viewBox="0 0 550 400" and a lot of the sub-elements have width and height. I added width="550px" height="400px", now the thumbnail is bigger and the lightbox uses the added dimensions. @G2G2G2G seems to be correct.

[G2G2G2G]

Here's a good explanation https://www.geeksforgeeks.org/svg-viewbox-attribute/ the sizes inside of the svg are only relevant to the svg itself.. (but with nothing set the svg sometimes takes the entire space available because some viewboxes are massive)
all browsers seem to interpret svgs the same, and just make them as big as they can inside their parent

<svg viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg">
  <!--
  with relative unit such as percentage, the visual size
  of the square looks unchanged regardless of the viewBox
  -->
  <rect x="0" y="0" width="100%" height="100%" fill="blue"/>

  <!--
  with a large viewBox the circle looks small
  as it is using user units for the r attribute:
  4 resolved against 100 as set in the viewBox
  -->
  <circle cx="50%" cy="50%" r="4" fill="white"/>
</svg>

(a simple svg from mozilla's site, added fill="blue" so it's easier to see on darkmode... save as whatever.svg)

The HTML inside of element currently looks like this:

<div class="mx_ImageView_image_wrapper">
<img src="url" alt="test.svg" class="mx_ImageView_image mx_ImageView_image_animating" draggable="true" style="cursor: default; transform: translateX(0px) translateY(0px) scale(1) rotate(0deg);"
>
</div>

<div class="mx_ImageView_image_wrapper">
<img src="url" alt="test.svg" class="mx_ImageView_image mx_ImageView_image_animating" draggable="true" style="cursor: default; transform: translateX(0px) translateY(0px) scale(1) rotate(0deg);" 
height="400px"> 
</div>

This will fix it, simply adding the height.. or the width.. or both, adding only 1 of them keeps the aspect ratio correct, I'd say height is probably the best, but if it's super wide then it might go off screen, say if it was 100px height and 100000px width lol

I guess getting the dimensions of the svg with javascript and then your element screen size with javascript and calculating a good view size would be the best thing so you don't mess up the aspect ratio etc
https://stackoverflow.com/questions/18147915/get-width-height-of-svg-element

(this is the only way to prevent people from simply making 200byte svgs that take up people's entire screens and annoy them)

[HarHarLinks]

I just came across an event like this in a public room

"content": {
    "body": "unifiedpush_broad_overview.svg",
    "info": {
      "h": 1587,
      "mimetype": "image/svg+xml",
      "size": 28044,
      "w": 1123,
      "xyz.amorgan.blurhash": "TKI5MT%}56-RtRSh00ng-o?]--xV"
    },
    "msgtype": "m.image",
    "url": "mxc://librepush.net/QagMrijoMYyceCArSDpyUPfo"
  }

that is: svg, no thumb, yes blurhash. it displays in the timeline just the blurhash. svg works in the lightbox. timeline either fails to render the svg, or does not even try, but it really should.

---

at the same time this image uses black text on a transparent background. this is really illegible, especially with dark mode element. there should be a toggle to add (custom?) background color behind svgs at least in the lightbox.

[alexanderstephan]

Using @G2G2G2G s approach to specify the height works just fine, but the real problem is calculating the blur hash for the preview. I can't seem to get a proper blur hash as calculated in ./workers/blurhash.worker.ts. I even set the imageData width and height manually or the image property itself does not work. In the end there is a hash, but it's just black e.g. something like U009jvfQfQfQfQfQfQfQfQfQfQfQfQfQfQfQ. You can see the blur hash value when viewing the message source. https://blurha.sh/ can preview it for example.

To solve this, my idea was to manually set the width and height attribute just for thumbnail svgs, so the blur hash gets calculated properly (note that the hash is calculated in the thumbnail method). Afterwards, the thumbnail info can get deleted again, as we already got the proper blur hash and can then we use the original SVG again for the preview (before checking that the size is let's say < 2MB). This is quite ugly, but the best I can come up with. What do you think of this solution?

Alternatively, one could probably modify the blur hash algorithm itself, but I would like to avoid it. Another alternative would be to not display this kind of SVG again like it was before, just sending it as a file that can be downloaded.

[G2G2G2G]

such a headache, I get some of it when working with svgs on web stuff... I see why so many places embed into font glyphs instead of the svg groups/layers for web images/icons
yea I think your solution is good, even if you just defaulted to it being a file download link I wouldn't blame you lmao

That blurhash doesn't care about aspect ratio at least on their site, it smashes it to a static size and hashes it.. imo good enough for the outcome cuz it looks bad regardless, so idk if it's even worth bothering calculating the thumbnail sizes instead of just 1 static size or whatever.

[t3chguy]

Another thing to consider is that blurhash doesn't support transparency which a lot of SVGs rely on - #18054

[turt2live]

Closing in favour of #15094 which has security background to it.