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

SVG support #13499

Closed
wants to merge 4 commits into from
Closed

SVG support #13499

wants to merge 4 commits into from

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Jan 7, 2017

Pull Request for Issue # .

Summary of Changes

This PR enables svg uploads and introduces a sanitiser for svg files
Sanitizer source: https://github.com/darylldoyle/svg-sanitizer
It is missing some db needed updates, will do them if it gets approved

  • This SHOULD NOT interfere with the work currently done in the new media manager group
  • The security strike team needs to approve this! so calling @Kubik-Rubik

Testing Instructions

  • Apply patch
  • Edit media configuration and replace the contents of Legal Extensions (File Types) with
bmp,csv,doc,gif,ico,jpg,jpeg,odg,odp,ods,odt,pdf,png,ppt,swf,txt,xcf,xls,svg,BMP,CSV,DOC,GIF,ICO,JPG,JPEG,ODG,ODP,ODS,ODT,PDF,PNG,PPT,SWF,TXT,XCF,XLS,SVG

Try to upload an svg file

Preview

screen shot 2017-01-07 at 14 59 41

Documentation Changes Required

NOTES

  • This PR was created only as a way for the security strike team to evaluate the usage of the SVG sanitisation
  • The SVG support is limiting svgs to certain level, as many features (e.g. scripts) will be removed
  • This PR was not meant to override the work done in the new media manager group, it's more like a bridge to the security team for an evaluation of a sanitisation library that could possible be used by the new media group

I hope that this clears up my intentions here (speed up the process by involving more people)


This change is Reviewable

@Bakual
Copy link
Contributor

Bakual commented Jan 7, 2017

What are those changes in the autoloading files? Those look wrong to me.

@wilsonge
Copy link
Contributor

wilsonge commented Jan 7, 2017

Dimitris is on an older version of composer :) It's regressing the autoload files back from composer 1.3.x to 1.2.x

@dgrammatiko
Copy link
Contributor Author

I sea 😜, will update that in a bit...

@dgrammatiko
Copy link
Contributor Author

Also Joomla is not supporting uploading of webp images...
About webp: https://developers.google.com/speed/webp/

@uglyeoin
Copy link
Contributor

I have tested this item ✅ successfully on e4fd085

The SVG uploaded, but I was unable to select it as my intro image or as a normal image, rendering it pointless. I need further instructions to test whether the sanitisation worked.


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

@dgrammatiko
Copy link
Contributor Author

@uglyeoin one easy way to test the uploaded file is by pointing your browser to the path of the svg e.g. /images/test.svg Then you can inspect the uploaded file with the browser's development tools:
screen shot 2017-01-10 at 17 09 57

@anibalsanchez
Copy link
Contributor

I have tested this item ✅ successfully on e4fd085

Test OK


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

@ghazal
Copy link
Contributor

ghazal commented Jan 17, 2017

I have tested this item ✅ successfully on e4fd085

@uglyeoin I guess this is not the purpose of this patch. As the title says, it only implements a way to make safe use of svg format.
Check this also : #4674


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

@uglyeoin
Copy link
Contributor

@ghazal I guess so. I have tested it successfully, but I have not tested the security side of things. I guess I need an insecure SVG in order to do so. I assume the other tests took this into account?

@uglyeoin
Copy link
Contributor

@dgt41 perhaps you could supply an SVG for people to test with?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants