SVG could be dangerous, especially coming from an external site. However, even not enabled by default, there should be a way for devs that know what they are doing to add SVG support. Here's a proposal:

  • Move the hardcoded list of extensions into a config, so that a site admin is able to add .svg to the list
  • Before saving the file content, dispatch a hook that allows 3rd-party to perform sanitization on the downloaded file. Allow the subscribers to cancel the file saving if they aren't able to sanitize the file.

Alternatively (more secure), depend on enshrined/svg-sanitize and do the sanitization by default.

I prefer the 2nd options as is more secure,

Issue fork imagecache_external-3390948

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea opened merge request !4

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Active » Needs review

This is ready for review and has tests

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. I've not been an active maintainer here since Drupal 6 so will defer to swentel for the next steps.

tim_dj’s picture

tim_dj
Location Netherlands
commented

This doesn't work out of the box. You need to add "image/svg+xml" to imagecache_external_allowed_mimetypes in imagecache_external.settings

I think this should be either added as default or there should be an UI to enable this.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

@tim_dj, indeed imagecache_external_allowed_mimetypes setting is not exposed in the admin form. But that is not an effect of this issue. I think you can open a follow-up to make this setting editable

tim_dj’s picture

tim_dj
Location Netherlands
commented

@claudiu.cristea then at the very least this should be documented because now it just doesn't work out of the box and I had to debug to find the cause

swentel’s picture

swentel commented
Status: Reviewed & tested by the community » Needs work

Cool! I agree however, that the mimetype should be added to the settings as well.

I see 2 options:

1. an update hook: there's an existing one, 'imagecache_external_update_8103' which does this for image/webp.

2. Add the mimetype as soon as you configure the svg settings in the UI.

I kind of prefer number 2: add when you configure tags, remove when svg tags are removed.

herved made their first commit to this issue’s fork.

herved opened merge request !11

herved’s picture

herved commented

It looks like something went wrong with the last merge in 3390948-svg (in , imagecache_external.module imagecache_sanitize_svg got removed).
I created a new 3390948-svg-2 branch, rebased on 3.0.x.

saidatom made their first commit to this issue’s fork.

swentel’s picture

swentel commented
Status: Needs work » Needs review

Going to review this over the weekend

swentel changed the visibility of the branch 3390948-svg to hidden.

swentel’s picture

swentel commented

Added a test that image/svg+xml is added or removed from the allowed mimetypes depending on the svg settings. Will merge in soon.

swentel’s picture

swentel commented
Status: Needs review » Fixed

Merged. Will release soon after merging two other issues.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

swentel’s picture

swentel commented
Status: Fixed » Closed (fixed)