[#29728] - Allow sanitized SVG uploads in Joomla 3 if you explicitly enable it
- Closed
- 22 Oct 2020
- Medium
- Build: staging
- # 29728
- Diff
- nikosdion:feature/svg-sanitiation
- Success Hound Smells good to me. Woof! Details
- Success Download Prebuilt packages are available for download. Details
- Success continuous-integration/drone/pr Build is passing Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Failure continuous-integration/appveyor/pr AppVeyor build failed Details
- Pending JTracker/HumanTestResults Human Test Results: 2 Successful 1 Failed. Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #28599 .
Summary of Changes
If and only if a Super User goes into the Media Manager's Options page and explicitly modifies the two to three necessary and separate options....
....YOU GET SVG SUPPORT IN JOOMLA 3, AT LONG LAST!
Testing Instructions
Part 1. Make sure SVG support is NOT enabled by default.
We are going to validate that by merely installing this PR, without making any changes, you still get the frustrating and subpar SVG experience offered by Joomla 1.0 to 3.9 inclusive.
- Install this PR using the Patch Tester.
- Go to Media Manager and try to upload the Joomla SVG logo file.
Expected result: Joomla yells at you that the file type is not supported.
- Be mean and upload the file manually into the
imagesdirectory using means external to Joomla, e.g. SFTP. - Go to Media Manager
Expected result: You see an SVG file type icon, not a preview of the logo. Whomp, whomp.
- Ignoring your past failures go edit an article. Any article.
- Try to select an intro or fulltext image.
Expected result: You cannot, in fact, select the SVG file.
- Pretend you are a desperate user and try to insert an image into the article text using the Image editor button (in the bottom row of the TInyMCE editor).
Expected result: nope, you still cannot select the SVG file.
Part 2. Persuade Joomla to use SVGs
- Go to Content, Media
- Click on Options in the toolbar.
- In the Legal Extensions (File Types) field add
,svg,SVGThis is used for uploading - In the Legal Image Extensions (File Types) field add
,svgThis is used for display and selecting images - If you have Check MIME Types set to Yes (that's the dfault) also find the Legal MIME Types field and add
,image/svg+xmlThis is also used for uploading.
Repeat the previous tests.
Expected result: you can upload AND select SVG files.
Commentary
The MediaHelper is rigged to always sanitize the uploaded SVG file. If you try to upload malformed XML or an XML document that does not look like a valid SVG with a .svg extension you will get an error that the file doesn't look like valid SVG. If you try to upload an SVG with JavaScript and / or event attributes they will be stripped away. If you want to use SVG animations either use the SVG animation features or upload the SVG outside of Joomla. Do note, however, that Joomla always renders images in <img> tags which has the side-effect that JavaScript is disabled on your SVG. If you really, REALLY want to use JavaScript in your SVG you will need to both upload it outside of Joomla AND display it using an <object> tag. Joomla is decidedly erring on the side of extreme caution here.
If you try selecting an SVG file for an article intro or full text image you will be disappointed. The image is inserted in an <img> tag but nothing is shown. You need to add some CSS to force the minimum size of the image element. The way Protostar is constructed vectors appear zero sized. I'm not touching that because you really need to know what you're doing to enable SVG support which means that you should very darned well know how to do some basic CSS or view template overriding.
The legal image extensions option is a bit of a lie. To ensure that backwards compatibility with previous Joomla 3 versions (which used a hardcoded list of image extensions, ignoring this setting for all intents and purposes) doesn't evaporate we forcibly add the extensions jpg, jpeg, png, gif, bmp, ico, xcf, odg to whatever you put in here. Please bear in mind that neither XCF nor ODG are supported natively by any browser. I have no idea why they are there other than "that's the way we found it in Joomla 1.0 and carried it blindly along for the past 15 years". I think that about 20 years ago Netscape Navigator in Linux might have supported them natively. Anyway, misguided or not, I kept the default so nobody cries foul at breaking b/c :)
The image extensions really only tell Media Manager which files to display with a preview which is also how it decides which files are allowed to be picked for insertion, either in an article or in any extension that renders a media picker field. That is a long-winded way of saying that we magically add SVG support to every third party extension that uses the native media picker controls in Joomla.
Finally, if you are using JCE please bear in mind that it comes with a system plugin which overrides the Media Manager and the media picker dialog. If you try using this PR in conjunction with JCE you will not see SVG support and you'll be complaining. Don't. JCE documents how to disable this feature.
Post-scriptum: if this gets merged in Joomla 3 I will come up with a new PR for Joomla 4 which adds the sanitization. Joomla 4 already allows the Super User to allow SVGs to be used but does not sanitize uploaded files, making such a proposition very risky.
Documentation Changes Required
Obviously we need to tell people how to enable SVG support. For example.
If you want to enable support for SVG files in Joomla 3.10 you need to do the following:
- Go to Content, Media
- Click on Options in the toolbar.
- In the Legal Extensions (File Types) field add
,svg,SVGThis is used for uploading - In the Legal Image Extensions (File Types) field add
,svgThis is used for display and selecting images - If you have Check MIME Types set to Yes (that's the dfault) also find the Legal MIME Types field and add
,image/svg+xmlThis is also used for uploading.
You can now upload and use SVG files. Bear in mind that SVG files are vector graphics and do not carry an intrinsic width and height. You may have to explicitly define the dimensions when inserting an SVG file.
Votes
| Status | New | ⇒ | Pending |
joomla-cms-bot
- 
| Category | ⇒ | Administration com_media Front End com_banners External Library Composer Change Libraries Repository Unit Tests |
| Labels |
Added:
?
?
?
|
||
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-06-21 16:36:48 |
| Closed_By | ⇒ | nikosdion | |
| Labels |
Added:
?
Removed: ? |
||
@nikosdion Maybe there was some missunderstanding? I haven't asked you to rewrite the library or find another one. I only tried to find out if there is a way to make this PR happen in J3.
Update: Ah, I think it was not related to my comments.
If it was me who could decide then I'd drop support for PHP 5.4 ... but it is not me. (sad)
@nikosdion one persons comment is not everyone
Add a PHP version check to public function sanitizeSVG($tempName) in MedaiHelper and interrupt the upload with a clear message: "SVG upload not supported with PHP < 5.4."
plus add a post-installation message telling the same story.
All with a ????? from my side because I didn't check the whole pr codes.
Be brave & leave PHP 5.3 behind in this case after 6 years EOL! ;-)
| Status | Closed | ⇒ | New |
| Closed_Date | 2020-06-21 16:36:48 | ⇒ | |
| Closed_By | nikosdion | ⇒ | |
| Labels |
Added:
?
Removed: ? |
||
| Status | New | ⇒ | Pending |
We are looking into this with the goal not to waste more of your time @nikosdion and to make the library 5.3 compatible. As far as I have seen it with looking into it just for a short time it is the array syntax and that it is a quick fix. I already made this fix but want to test it before I make a PR, so let's work together and make it happen. Seems to me not a long way to go.
@rdeutz If we patch the library inside the vendor directory we are one well-meaning composer update of pulling a new version of it which is not compatible with PHP 5.4. Therefore going that route is functionally equivalent to doing nothing, lest the project really considers PHP 5.3 support to just be perfunctory rather than meaningful.
The other option is to pull this library into a separate directory in libraries just like it's done with idna_convert, lessc etc. In this case we CAN "core hack" the library to make it compatible with PHP 5.3 However, this means that we'd also need to be able to test it on PHP 5.3. I don't have a PHP 5.3 environment and even if I tried to set up a Vagrant box with Ubuntu 16.04, the last LTS to support it, I'd fail the 16.04 update sources have been removed following its recent end of life.
If you have a PHP 5.3 environment please do move the library into the libraries folder and modify it to support PHP 5.3. Make a PR to my branch and I'll merge it. I just don't want to do code changes I can't test. Fair enough?
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29728.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29728.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29728.
Maybe a way to speed up the decision
I would also be happy to receive information on the current status.
My Opinion: PHP 5.3 is not important to me. It has not been supported since 2014!
Because Joomla 3 will be used for even longer and SVG is popular, I think Re.Later's proposal is great.
@ReLater Add a PHP version check to
public function sanitizeSVG($tempName)inMedaiHelperand interrupt the upload with a clear message: "SVG upload not supported with PHP < 5.4."
If this is not wanted and no solution can be found for the support of PHP 5.3 in the near future, this should be communicated. Because this also blocks to find solution for Joomla 4
On Joomla! 3.10 after importing the media parameters for the svg and loading the example file with FTP-connection, I tried to insert (in a new article) the image and I'm still not present in the image selection pop-up.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29728.
On Joomla! 3.10 after importing the media parameters for the svg and loading the example file with FTP-connection, I tried to insert (in a new article) the image and I'm still not present in the image selection pop-up.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29728.
I tested with v3.9.21 and it works.
With v3.10.0-alpha1, when I try to apply patch with Test Patcher I'm getting this error: The file marked for modification does not exist: composer.json
If this is not wanted and no solution can be found for the support of PHP 5.3 in the near future, this should be communicated. Because this also blocks to find solution for Joomla 4
This doesn't block anything for 4.0. This PR can be redone on 4.0 (or 4.1 since 4.0 no longer accepts new features although neither does 3.x but this PR is still being considered for staging so
Always on J! 3.10, I did some tests.
With the imports of SVG readings I don't see the file in the media component anymore, before (when I posted the first time) I could see it even if I could not open it...
in @nikosdion defense, our patchtester doesn't work in 5.3.29 either!
pt does work in 5.4.45
I have tested this item
Fails mime type comparison.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29728.
@nikosdion it's failing the compare due to "+xml" being stripped..
This is with php 7.4.4 and exif extension active
upon request I did a bunch more testing and with php 5.3.29, & the following extensions...
turned on I get the following error.
with exif off i get
with php 5.4.45 & exif OFF i get the same error, with exif ON it works as expected.
with php 7.4.4 & exif ON I get
this is due to the missing "+xml" as I previously stated.
with exif off I get
FWIW my personal opinion here (bear in mind just an opinion and @HLeithner / @zero-24 makes the final decision for 3.x) is that we add this to 3.10 but continue disabling the ability to upload SVG's on 5.3 - if we have a way to explicitly verify their safe (we do in 5.4+) then we can accept them - else by default they are insecure.
| Status | Ready to Commit | ⇒ | Pending |
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-10-22 13:58:06 |
| Closed_By | ⇒ | nikosdion |
You are asking me to support PHP 5.3 as a requirement. Three problems.
First, there is currently no actively maintained LTS Linux release with this ancient version of PHP. So I can’t plausibly test with it.
Second, you ask me to rewrite the library or find another library. There is no other library. SVG sanitization started being an active subject of discussion after PHP 5.3 went EOL in August 2014.
Third, the notion that I’d blindly rewrite a critical security library for a PHP version I can’t even run is idiotic. If you were stupid enough to accept it the JSST would reject it for good reason.
I’m done wasting my time.