See here please: #38530

@ChristineWk that pull request is just to improve the message. It does NOT resolve the fact that something has changed and all svg uploads are now prevented

I have found the problem causing this and am working on a pull request

Can someone mark this a release blocker please @roland-d @fancyFranci @richard67

Yes, thanks Brian, that's why I didn't close the issue. Just pointed it out for additional information only.

ok this doesnt make much sense to me. If I understand the code from @bembelimen correctly in the MediaHelper then we are using the enshrined/sanitizer library to try and make the svg is safe and if the library fails to make it safe then we reject it

bad1, bad3, bad4 - these should be blocked from uploading and they currently are
good, good2, good3, good4, good5 - these should be uploaded and they currently are
example1, example2, example3, example4, example5, example6 - these should be uploaded and they are not

cleaned-example6.svg
As a further test I took example1 and example6 and passed them through the online demo of the sanitizer at https://svg.enshrined.co.uk/ and then tried to upload the sanitized result. They still failed.

I hope this information and the zip of images helps someone better than me to resolve this
svg.zip

The more I look at this the more confused I am. svgsanitize is designed to clean an upload svg but we appear to be using it as a validator only

Yes, the sanitizer is used to check the files, it is not used to clean them, even though it has that capability. The problem with cleaning files and use the cleaned version is that it may not have the intended results. For instance, some xlinks can be removed. Or scripts. Removed, it could break the SVG output.

The media manager can be used in the frontend so allowing SVG files that may have concerned issues should not be uploaded. However, if an administrator of the site knowingly puts a SVG on his/her site through FTP, for instance, nothing prevents him/her to. The admin is responsible for making sure the file is safe.

Now we need to educate the user so he/she knows why the file is harmful. Hence better wording on the messages we send and proper documentation.

No not at all. Use svgsanitize as designed. It will clean the files that need cleaning and reject the files that cannot be cleaned.

Telling a user that an svg file from a reputable source/application is harmful when it is not doesn't help anyone at all.

further tests with the svg files I supplied above and using the svg-scanner that is part of the package suggest that the scanner is crap. try it yourself and see how it will fail almost any svg that is not minified

Telling a user that an svg file from a reputable source/application is harmful when it is not doesn't help anyone at all.

100% spot on. Here are 2 images created via inkscape which is the FOSS eqv to Adobe illustrator and both will fail being uploaded.


I could supply several more.
The point being is I EXPECT these files to work. They were created with a well known program designed for this kind of work.

@N6REJ if you minify them then they will probably work

SVG security is HIGHLY dependant on the way the SVG is used. As an img src the sanitisation is done BY the browser, even unsanitised svgs are fine. Read the specs: https://svgwg.org/svg2-draft/conform.html#examples

As an img src the sanitisation is done BY the browser, even unsanitised svgs are fine

The thing is: can we safely assume that an uploaded SVG is always used in the img-src context? I don't think so.

No we can't but we can at least use the sanitize library as a sanitizer. Its clearly not great as a valaidator

we safely assume that an uploaded SVG is always used in the img-src context

No, the CMS doesn't have this flexibility and neither I posted the reference to imply this. I just wanted to point out that if SVGs are used as images (with src attribute pointing to the url of the image) there's no need for sanitisation. It's unrealistic to assume that this would be the only way people will use them as they can have a field and then embed the svg in the html, or in the css or...

No we can't but we can at least use the sanitize library as a sanitizer

Maybe it's easier to do the sanitisation on the client before even it reaches the server. The code from the SVGOM could easily imported and executed when the mime type === svg/... https://github.com/jakearchibald/svgomg/tree/main/src/js

Maybe it's easier to do the sanitisation on the client before even it reaches the server.

That's still telling someone that the svg from their reputable source is vulnerable just because we are using a library incorrectly

My point was that IF you can guarantee that svgs will only be used as image tags then there's no need for any sanitisation, proof is that the images from the svg.zip that @brianteeman posted above were uploaded without any changes on GitHub because the only way GH will ever use them is as img tags. In short this might give you another perspective...

SVG security is HIGHLY dependant on the way the SVG is used. As an img src the sanitisation is done BY the browser, even unsanitised svgs are fine. Read the specs: https://svgwg.org/svg2-draft/conform.html#examples

My understanding is that the img tag disables scripting (SVG files are 'run' in a browser sandbox), but the files are not sanitized. Therefore, anyone downloading the file from your site gets the original file.
Is that right @dgrammatiko?

The security issue occurs if a malicious svg could be uploaded (which is a low privilege task) it could then be accessed directly www.example.com/images/filename.svg . Image files are not prevented from being loaded directly.

Or an example from the post above https://user-images.githubusercontent.com/3889375/185746788-7c1c39c4-2680-448b-931b-f6011fe320b8.svg

@roland-d please explain your justification for removing the release blocker label.

@brianteeman yes that did work.

@N6REJ its because one of the rules in svgsanitize is to mark a space as an error. We could easily allow spaces just as we allow comments although I still prefer to use the library as it was intended

looks like if we'd just leave it alone and let it clean it all would be well

yes. but thats not we're doing

@N6REJ its because one of the rules in svgsanitize is to mark a space as an error. We could easily allow spaces just as we allow comments although I still prefer to use the library as it was intended

Not sure what you mean with "spaces" but spaces are not marked as invalid. An attribute (xml:space) is marked as invalid due to a bug: darylldoyle/svg-sanitizer#64

 
PS C:\laragon\www\j4\libraries\vendor\enshrined\svg-sanitize\src> php .\svg-scanner.php .\svg\example2.svg
{
    "totals": {
        "errors": 3
    },
    "files": {
        ".\\svg\\example2.svg": {
            "errors": 3,
            "messages": [
                {
                    "message": "Suspicious attribute 'space'",
                    "line": 3
                },
                {
                    "message": "Suspicious attribute 'space'",
                    "line": 8
                },
                {
                    "message": "Suspicious node 'svg'",
                    "line": -1
                }
            ]
        }
    }
}

ah I see what you mean now about xml:space and that it was just coincidence on the files it was reported. Perhaps until the library is updated you could whitelist it in the same way as you did the comments?

Found a few more bugs upstream. Please test #38545 as this removes a few more false positives that are preventing upload

@N6REJ its because one of the rules in svgsanitize is to mark a space as an error. We could easily allow spaces just as we allow comments although I still prefer to use the library as it was intended

Not sure what you mean with "spaces" but spaces are not marked as invalid. An attribute (xml:space) is marked as invalid due to a bug: darylldoyle/svg-sanitizer#64

@brianteeman said that not me.

My understanding is that the img tag disables scripting (SVG files are 'run' in a browser sandbox), but the files are not sanitized. Therefore, anyone downloading the file from your site gets the original file. Is that right @dgrammatiko?

@obuisard so, the SVG is never a threat or in anyway problematic for the server side, it's just a static file! For the client side ONLY if the SVG is embedded into the page there are potential problems. The images folder (or any folder that the media manager is managing) is supposed to be only for static assets (jpg, png, webp, avif, mp4, mov, pdf, etc) and the only way that these files are used for the CMS is ALWAYS as image, video, audio or object. There is no way that an SVG will ever be embedded in the page UNLESS a user or a developer does specifically that. In short the whole fear for SVGs is not justified. On top of that if SVGs are problematic because they might introduce XSS vulnerabilities then there are already documented such vulnerabilities in other parts of the software that got a status won't fix or not an issue. It seems to me that there are 2 rules...

@roland-d please explain your justification for removing the release blocker label.

There are several reasons I did it at the time:

• This issue is not blocking the 4.2.1 release
• There was no PR with a fix at the time
• SVGs can still be uploaded although I agree not all of them

Now following the discussion here, can't we make it an option that users can decide for themselves to allow SVG uploads? By default we do what we do now but if users want to open it up they can. They can upload SVGs Joomla considers bad anyway via FTP/Filemanager/SSH.

not having a pr is not a reason to remove a release blocker 🤦

as release lead I respectfully suggest that you do follow the discussion or there is no way yu can actually make a good faith decision.

not having a pr is not a reason to remove a release blocker 🤦

Fair enough, I will put the release blocker back

as release lead I respectfully suggest that you do follow the discussion or there is no way yu can actually make a good faith decision.

I am following the discussion, both here and on Glip

I actually ran into this issue yesterday trying to test the PR for media manager thumbnails.

At a bare minimum, the error text should be expanded. I thought Joomla was broken. Explaining that the SVG could be potentially harmful, and maybe linking to a place to minify it, would be extremely helpful.

Even better would be to offer the user the option to sanitize the SVG with a note it may behave in an unexpected way. That way it's an opt-in action, and if it breaks the user knows why.

As #38545 has now been merged there should be a big drop in failed svg

Even better would be to offer the user the option to sanitize the SVG with a note it may behave in an unexpected way. That way it's an opt-in action, and if it breaks the user knows why.

100% agree with that

Hello

I would point out that I can send svg files via FTP, then use them in the media manager (joomla 4.2.5).
Most of the time, it is impossible to download them from the media manager. The error message does not give much information.
Is this fixed for you?
Is this something that can be improved in a future version of Joomla?

Regards

This issue is not blocking any release, so I'm removing the release blocker label. Meanwhile a helpful fix was merged and workarounds via FTP exist. Of course I would still welcome an improvement like mentionend above.

@fancyFranci is this still valid? When people tested #39586 nobody mentioned that they were unable to upload SVG files

Don't forget, it's fixed in 4.3, not 4.2

Ok nice, I missed that PR. Then the issue could be closed indeed.

@obuisard what PR are you referring to?

My pr #38545 addressed some issues but that was for 4.2. Testing 4.3 with the same svg I have provided here already still shows multiple svg being rejected

There is still the outstanding problem as mentioned by @crystalenka

I actually ran into this issue yesterday trying to test the PR for media manager thumbnails.

At a bare minimum, the error text should be expanded. I thought Joomla was broken. Explaining that the SVG could be potentially harmful, and maybe linking to a place to minify it, would be extremely helpful.

Even better would be to offer the user the option to sanitize the SVG with a note it may behave in an unexpected way. That way it's an opt-in action, and if it breaks the user knows why.

The error text is still the unhelpful

Error
Unable to upload file.

@fancyFranci is this still valid? When people tested #39586 nobody mentioned that they were unable to upload SVG files

yes they did (#39586 (comment))

The error text is still the unhelpful

The fix is there #38536

yes they did (#39586 (comment))

Actually that was a problem with PHP7.x reporting the mime type differently and solved with the commit dfcf205

My point wasn't that my PR fixed the problem (yours did, I didn't;t claimed that) but asking if this is still valid. Also for the error reporting @Fedik has a PR #38536...

Brian @brianteeman I was thinking of the fix from Dimitris, as he explained in the previous comment.
However, it does not prevent SVG files from being denied drop into the Media Manager if the file is considered 'suspicious' by the sanitizer script we are using.
You have eased the rules (PR #38545) and the messaging is more user friendly (PR #38536), mime types are properly handled.
However, we will never totally 'fix' the SVG uploads because we don't use the sanitizer to actually 'sanitize' files.

@obuisard except that the messaging improvement has not been merged yet

However, we will never totally 'fix' the SVG uploads because we don't use the sanitizer to actually 'sanitize' files.

any why don't we?