Page MenuHomePhabricator

Remove SVG checks
Closed, ResolvedPublic

Description

All grade A browsers in MediaWiki support SVG (T135554).
Despite this Multimedia Viewer checks for SVG support before booting.
This can safely be removed.

Details

Related Gerrit Patches:
mediawiki/extensions/MultimediaViewer : masterRemove SVG checks
mediawiki/extensions/MultimediaViewer : masterRevert "Remove SVG checks" - Do not allow in-page SVG rendering

Event Timeline

Restricted Application added a project: Multimedia. · View Herald TranscriptMay 17 2019, 5:50 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 511053 had a related patch set uploaded (by Simon04; owner: Simon04):
[mediawiki/extensions/MultimediaViewer@master] Remove SVG checks

https://gerrit.wikimedia.org/r/511053

Change 511053 merged by jenkins-bot:
[mediawiki/extensions/MultimediaViewer@master] Remove SVG checks

https://gerrit.wikimedia.org/r/511053

Krinkle added a comment.EditedMay 18 2019, 7:45 PM

This has been considered before in other places as well. It is not about browser support. This is about performance, rendering correctness and security.

  • Performance: SVGs can be very large documents not intended for client-side rendering. Viewing the original is not the same as viewing a large thumbnail. Mediaviewer is meant to display large thumbnails, not originals.
  • Rendering correctness: Browsers vary widely in how they render SVGs. They may have varying levels of support for SVG features, have additional fonts installed etc. The rendering of SVGs on Commons very tightly depends and assumes a specific set of fonts, character sets, widths of glyphs in those fonts in relation to other aspects of the design. This should not be rendered client-side in a browser.
  • Security: We have not done a recent audit of our whitelisted SVG syntax for the purposes of rendering on trusted origins.

SVGs are effectively full HTML documents that can contain JavaScript and more, these are uploaded by users, and this commit is rendering them on primary domains under *.wikipedia.org which is a direct no-go and the primary reason for direct revert.

We currently render these SVGs only from upload.wikimedia.org, which is explicitly untrusted, and has stricter CSP rules set as well.

The MediaWiki software is responsible for safely deciding when an original can be used. The thumbnail API will use the original in limited cases, such as JPEGs, when the requested thumbnail size is larger than the original and the original file type is considered safe (such as JPEG). This logic does not belong in MultimediaViewer, and must not be bypassed. The MediaWiki thumbnail API will never serve the original SVG no matter which thumbnail size is requested, and it behaves that way for a reason.

Change 511091 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/MultimediaViewer@master] Revert "Remove SVG checks" - Do not allow in-page SVG rendering

https://gerrit.wikimedia.org/r/511091

Hm. This sounds like a misunderstanding to me. The removed code literally said isBrowserSupported, as well as "MuyltimediaViewer uses SVG icons extensively and is unusable without them." As far as I can tell there is no code that expects to get .svg files from Commons. The removed code was exclusively about the .svg icons that are part of the MultiMediaViewer codebase. Performance, rendering correctness, and security apply to these as well, but should be fine, because these files got individual review.

Change 511091 abandoned by Krinkle:
Revert "Remove SVG checks" - Do not allow in-page SVG rendering

Reason:
Misunderstanding

https://gerrit.wikimedia.org/r/511091

Thanks. Misunderstood the patch and jumped the gun.

simon04 closed this task as Resolved.May 19 2019, 10:05 AM