Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.3] Media Manager SVG File #38068

Closed
Tracked by #13
ChristineWk opened this issue Jun 15, 2022 · 28 comments
Closed
Tracked by #13

[4.3] Media Manager SVG File #38068

ChristineWk opened this issue Jun 15, 2022 · 28 comments

Comments

@ChristineWk
Copy link

Steps to reproduce the issue

Upload svg file as logo in cassiopeia.
A message appears: An error has occurred.

Expected result

Solution: Under Media you have to allow svg in several places
file types; image/svg+xml
Valid image file extensions: svg
Allowed extensions: svg

Actual result

If you do not enter all types (see above). A message appears: An error has occurred.

Additional comments

Feature Rquest: Wouldn't it make sense to offer such information directly where the problem can occur?
Eg by displaying a small question mark icon and behind it the information that you first have to set e.g. svg "on" at this point or that.
Thank you.

@chmst
Copy link
Contributor

chmst commented Jun 15, 2022

There is indeed some need for information and documentation. Maybe we can add information for cassiopeia?
@drmenzelit

@brianteeman
Copy link
Contributor

The problem is that there is a bug and the correct error message is not being displayed.

@ChristineWk
Copy link
Author

ChristineWk commented Jun 15, 2022

The problem is that there is a bug and the correct error message is not being displayed.

would a PR be possible?

@N6REJ
Copy link
Contributor

N6REJ commented Jul 15, 2022

I don't know if this is related or not, but I just tried to upload this image and it was refused even though I have everything set correctly
collabnet-versionone-logo
using tiny I was able to user <img src=> to include it in the article however.
image

@obuisard
Copy link
Contributor

Indeed, there is a problem with SVGs here. Even when the allowed types and mime types are set correctly in the media manager's global configuration (like stated in the previous comment by @N6REJ), it is impossible to add SVGs to the Media manager (via uploads or drag-and-drop). Once the files are added to the images folder via FTP, for instance, the files are visible (without a preview, which is expected for now) and selectable in the article manager or Cassiopeia.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38068.

@N6REJ
Copy link
Contributor

N6REJ commented Aug 16, 2022

@Quy can you tackle this?

@N6REJ N6REJ added bug and removed Feature labels Aug 16, 2022
@ChristineWk ChristineWk changed the title Media Manager SVG File (Feature Request) Media Manager SVG File Aug 17, 2022
@ChristineWk ChristineWk changed the title Media Manager SVG File [4.3] Media Manager SVG File Aug 17, 2022
@ChristineWk
Copy link
Author

@obuisard: Thanks for your intervention

@brianteeman
Copy link
Contributor

@obuisard that is not my experience as shown in this video https://www.youtube.com/watch?v=U7hHYpuNqsg&ab_channel=LearnJoomla4

@obuisard
Copy link
Contributor

Thank you, Brian @brianteeman. It confirms that it used to work under Joomla 4.0.4. But my tests under Joomla 4.1.5 and 4.2.0 are failing.

@obuisard
Copy link
Contributor

obuisard commented Aug 17, 2022

I am doing more testing on this.
My findings so far:

In Joomla 4.0.4, SVG files can be uploaded EXCEPT if the files start with an XML comment. Once the comment has been removed or moved inside the svg tag, it can be uploaded. This kind of comment is usually added by generators of SVG files.

In Joomla 4.1.5, no file can be uploaded unless they are 'simple' XML files (containing the svg tag and the optional xml declaration). The ones containing additional namespaces or DOCTYPE cannot be uploaded.

So, I see regression.

@brianteeman
Copy link
Contributor

Sounds like a issue with the security SVG filter

@obuisard
Copy link
Contributor

obuisard commented Aug 17, 2022

We need to document what is a clean SVG and why some SVG files may be denied upload. SVG files need to be sanitized to avoid all kinds of security issues (cross-site scripting, HTML injections, denial of service - basically any possible attack related to XML documents). Some users may not be aware of it.
And the error message is not helping.

@brianteeman
Copy link
Contributor

Its more than that. The svg files used to test the filters when they were added do not work either

@brianteeman
Copy link
Contributor

you need to roll back to when it worked and then see what has changed. I have my suspicions but its too late and I had too many beer to try

@obuisard
Copy link
Contributor

Yes @brianteeman, thank you!

@obuisard
Copy link
Contributor

obuisard commented Aug 18, 2022

The major difference I see is that in 4.0 the content of the files was scanned for specific html/xml tags. In more recent versions, a sanitizer is used instead (from vendor 'enshrined'). It's the same one used in Nicholas K. Dionysopoulos's plugin for Joomla 3 (https://github.com/nikosdion/joomlasvg).

The new sanitizer checks the SVG files and returns a clean version or false (the file could not be parsed).
(this sanitizer covers more potential issues than the parser used to in Joomla 4.0, hence the restricted number of files allowed now).

The sanitizer reports what issues have been addressed during sanitization. We use it in 4.2, for instance, to see if there are issues with the file.

Based on my review of the MediaHelper code in 4.2,

if ($isValid === false || count($svgErrors))

should be

if ($isValid === false)

However, we do need to use the cleaned file if we do so.

So the question here is:
should we parse the file and invalidate it if there are issues (like it does work now) or should we allow the cleaned version of the file? This may have been discussed already. If we keep it as it is, we should report the issues to the user, for better feedback on why the file was not uploaded.

Another issue involves the error messages from the MediaHelper never showing because all is caught is JLIB_MEDIA_ERROR_UPLOAD_INPUT when returning from the LocalAdapter's canUpload function.

@N6REJ
Copy link
Contributor

N6REJ commented Aug 18, 2022

Incase it helps here's 2 inkscape svg files. One is in "save as inkscape svg" and the other is "save as svg"
Bearsampp-logo-128x128
Bearsampp-logo-inkscape

@obuisard
Copy link
Contributor

I am going to write a PR that fixes the messaging that appears when the upload fails.
Another PR should be created to give the option to the administrator to sanitize SVG files rather than just scanning them for issues.

@Fedik
Copy link
Member

Fedik commented Aug 19, 2022

I have added better messenging, Please test #38536

@level420
Copy link

For joomla 4.2.4 I was not able to upload any svg image even with all settings for the media manager

Allowed extensions: svg
Legal Image Extensions: svg
Legal MIME Types: image/svg+xml

Any progress on this issue?

@Orgoth
Copy link

Orgoth commented Jul 11, 2023

For joomla 4.3.2 it is the same.

Allowed extensions: svg
Legal Image Extensions: svg
Legal MIME Types: image/svg+xml

Had to upload the files via sftp, the media manager blocks any attempt.
Message by the Manager: File can not be uploaded. "Datei kann nicht hochgeladen werden."


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38068.

@obuisard
Copy link
Contributor

For joomla 4.3.2 it is the same.

Allowed extensions: svg Legal Image Extensions: svg Legal MIME Types: image/svg+xml

Had to upload the files via sftp, the media manager blocks any attempt. Message by the Manager: File can not be uploaded. "Datei kann nicht hochgeladen werden."

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38068.

This can still happen. SVG files are checked upon upload and if there is anything in the file that is considered harmful, the file will be rejected. You need to make sure the SVG file has been sanitized before use.

@Orgoth
Copy link

Orgoth commented Jul 11, 2023

For joomla 4.3.2 it is the same.
Allowed extensions: svg Legal Image Extensions: svg Legal MIME Types: image/svg+xml
Had to upload the files via sftp, the media manager blocks any attempt. Message by the Manager: File can not be uploaded. "Datei kann nicht hochgeladen werden."
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38068.

This can still happen. SVG files are checked upon upload and if there is anything in the file that is considered harmful, the file will be rejected. You need to make sure the SVG file has been sanitized before use.

I have create the svg with inkscape, I have assumed, it would already be sanitized.

@obuisard
Copy link
Contributor

I have create the svg with inkscape, I have assumed, it would already be sanitized.

Actually, no.
If you saved it via Inkscape, chances are the file is full of namespace tags added by the editor, unless you have saved your file as 'plain SVG'.

@Orgoth
Copy link

Orgoth commented Jul 11, 2023

I tested a SVGOMG optimized file and it worked.
Thank you very much for this hint.
It would be good if this could be described better.
Since just the message alone is not helpful at all.

Instead of this message: "File cannot be uploaded."
Would be better to add the addition that the user should check if the file is sanitized.

Some hint would be really helpful, otherwise there will be a lot more users like me who stumble across this problem and then open completely unnecessary new tickets.
It's even worse with pagebuilders like Yootheme or SPPB, where there's no feedback at all, just an empty popup.
Which suggests that there is a problem with the notification not being communicated correctly to the outside world.

Regardless, thanks again and a thumbs up from me as well for the answers.

@Quy
Copy link
Contributor

Quy commented Jul 11, 2023

Instead of this message: "File cannot be uploaded." Would be better to add the addition that the user should check if the file is sanitized.

Please test PR #38536.

@obuisard
Copy link
Contributor

obuisard commented Jul 11, 2023

Instead of this message: "File cannot be uploaded." Would be better to add the addition that the user should check if the file is sanitized.

Please test PR #38536.

Yes, please test, it will help getting it included in the 4.3.4 release next month. Thank you!
If you need help on how to test, please refer to https://docs.joomla.org/Bug_Tracking_Process

@alikon
Copy link
Contributor

alikon commented Jul 11, 2023

closing as we have pr #38536

@alikon alikon closed this as completed Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests