Updated after #86.

Problem/Motivation

Changing the allowed file extensions on an image field doesn't update in the edit form. Other changes to the field are reflected though.

Steps to reproduce -
1. Create an Image Field
2. Set the allowed file extensions to also include svg, etc.
3. Or modify the allowed file extensions after creating the field to include svg, etc.

The current patch tries to solve the fact that the list as defined by a site builder is reduced based on a hard-coded list instead of the list of extensions supported by the actual toolkit.

Proposed resolution

It should also check when defining an image field, not only when showing an image widget (and allow site builders to shoot themselves in the foot).

  • When defining a new image field, the default value for 'Allowed file extensions' should be the list of supported extensions.
  • When saving field settings, the entered list should be validated against the list of supported extensions.
  • If there are no unsupported extensions: save as is
  • If there are unsupported extensions, we have the following options (TBD):
    • Warn but accept the reduced set.
    • Fail with a form validation error.
  • If an existing image field is edited: no changes, ie take the currently saved list as value.
  • If an existing image field is saved: same validation as above.
  • If an image widget is displayed we have 2 options that should align with the options above:
    1. Take the list as is. Do not make any changes to the list at this point.
    2. Take the list and intersect with the actual list of supported extensions.
  • If an image with an unsupported but allowed extension is uploaded:
    • Fail (aligns with the fail option above).
    • Fail if dimensions are specified (never logical).
    • Fail if dimensions are specified and getimagesize() (does not require a toolkit) indicates that the dimensions fall outside the allowed dimensions. If getimagesize() fails to produce dimensions: TBD.

I am in favor of the warning option, not the hard fail as most error handling on the image widget side will still be necessary for when toolkit configuration has changed (more specifically the list of supported extensions).

If we go for the warning, the warning should indicate which extensions are unsupported and what the consequences are:

  • Image styles will not be applied to unsupported extensions.
  • Resizing due to min/max resolution will not be possible and thus image uploads might fail.
  • [future request] Auto rotating on upload will not be possible, so an upload might fail. To determine if we actually need to rotate, we would not necessarily need an image toolkit, just the EXIF PHP extension.

Remaining tasks

Decide on a path forward, quickly. So we can get rolling on a direction again. This issue has stalled out at comment #86, which bodes poorly for adopting any solution.

User interface changes

API changes

Data model changes

Original Summary

Changing the allowed file extensions on an image field doesn't update in the edit form. Other changes to the field are reflected though.

Steps to reproduce -
1. Create an Image Field
2. Set the allowed file extensions
3. Modify the allowed file extensions after creating the field.

Files: 

Comments

guybedford’s picture

Title: File Types on Image Field not updating » Allow additional Image Field extensions
Category: bug » feature

Turns out this is a duplicate of http://drupal.org/node/515152 in D6.

Tiff and other file types are not allowed for Image Fields.

yched’s picture

Component: field system » file.module

recategorize

BarisW’s picture

Version: 7.0-rc4 » 7.x-dev
Component: file.module » image.module
Category: feature » bug

guybedford: why has this been changed to a feature request? I'd say this is still a bug. If you use other Image libraries (eg GraphicsMagick) it is possible to convert TIFF files and other formats.

Currently you are allowed to change the file types but you don't get a warning/error if certain filetypes are not allowed. This is a Drupal WTF that causes a lot of confusion. So we need to add a message if one tries to change the file extensions on images, or we need to let the user change the extensions to whatever he finds useful.

BarisW’s picture

And for clarification some screenshots:

When entering file extensions in the field definition interface, you are allowed to add any file extension.

But when creating a node, it filters out all extensions except jpg/png/gif. HUH?

rfulcher’s picture

What is the status on this issue? I am having the same problem. I have people who want to upload tif files and they can't.

webchick’s picture

Closed #1081060: Image field silently disallows non-web extensions; proclaim it loudly instead as a duplicate, which has a good analysis of the problem from tegneged:

I investigated the image.field.inc module and found at line 307

// If not using custom extension validation, ensure this is an image.
$supported_extensions = array('png', 'gif', 'jpg', 'jpeg');
$extensions = isset($elements[$delta]['#upload_validators']['file_validate_extensions'][0]) ? $elements[$delta]['#upload_validators']['file_validate_extensions'][0] : implode(' ', $supported_extensions);
$extensions = array_intersect(explode(' ', $extensions), $supported_extensions);
$elements[$delta]['#upload_validators']['file_validate_extensions'][0] = implode(' ', $extensions);

If i am reading this right the array_intersec will remove any extension not in the supported_extensions array from the allowed extensions.

That's indeed what's going on.

Morbus at #1081060-2: Image field silently disallows non-web extensions; proclaim it loudly instead notes:

I am of the mind that the /intent/ behind the "Image" field type is "one that should be displayed in a browser" versus the more generic "any of the 150+ available image formats" - the image widgets also support this assertion by offering rendering via the <img> tag. TIF files, though common graphic formats, are not "common" file formats intended for the web or display within a browser. You're more than welcome to create a File field, for example, label it "Image", and set the extensions you'd like and there'd be no particular problem.

I tend to agree with this analysis, since image fields come with things like thumbnail generation, inline display, and so on rather than just "a field to put images." A quick gander through http://en.wikipedia.org/wiki/Comparison_of_web_browsers#Image_format_sup... seems to back up the assertion that support for extensions other than JPG, PNG, and GIF are only spottily supported.

Nevertheless, it seems odd that we would hard-code such a whitelist and not allow users to shoot themselves in the foot if they wanted to. Perhaps that array should be swapped out with a variable that could be overridden in settings.php?

Or, if nothing else, we should make the description of that field more clear and set the field to #disabled = TRUE so people don't think they can change it.

webchick’s picture

Version: 7.x-dev » 8.x-dev

Also, bugs are fixed in Drupal 8 first, then backported.

rfulcher’s picture

Webchick,

Thanks very much for the analysis of the situation. I am ok with what you explained, it seems very logical. My suggestion of the situation is to put something in the documentation of the image field so that people know that it is for web image formats not any image format. If I knew that from the get go I would have just converted the images and went on.

Again thinks a lot for looking into this and replying. It is much appreciated.

:)

magnusk’s picture

I just encountered the same issue. A client requested the addition of the TIFF image format (http://en.wikipedia.org/wiki/Tagged_Image_File_Format) to the upload widget. I changed the field configuration and could not understand why the node edit form would not support the tif and tiff extensions. This is a seriously misleading field configuration option.

If TIFF cannot be supported (they're commonly images obtained from scanners?) then the field configuration interface should indicate as much and prevent entering extensions that will be ignored. Further, for the sake of maintenance and configuration control, the supported extensions should probably not be managed as a hard-coded string in image_field_widget_form(). Which could open up control of the accepted extensions via each image toolkit's admin configuration.

As a practical exploration, if I force the widget to accept TIFF files I see that the thumbnail cannot be generated with the default GD image toolkit. The thumbnail is fine if I install and use the ImageMagick toolkit instead (http://drupal.org/project/imagemagick).

josegaert’s picture

There is a lot of talk about Drupal 8 and the html5 initiative.
And html5 includes svg as a major part of its specifications.
Shouldn’t Drupal prepare for html5 and the svg image format by providing support for it in the image module?
True, in the big bad world of the browsers, http://en.wikipedia.org/wiki/Comparison_of_web_browsers#cite_note-FFSVG-171, there is only minor support for it. But it is the buzz of the day, and they are all trying hard to improve support. Well, the major browsers.

For me, I'm going from one pitfall to another trying to write a module that supports svg and also plays well with the image UI.
I think that implementing svg in Image module shouldn't be that difficult. The nice thing about svg is that it doesn't need an image toolkit to transform it. Every transformation can be done easily in php by wrapping one into another with some extra attributes and maybe some extra options.

leschekfm’s picture

Hi,

Nevertheless, it seems odd that we would hard-code such a whitelist and not allow users to shoot themselves in the foot if they wanted to. Perhaps that array should be swapped out with a variable that could be overridden in settings.php?

I think this would be a good way to go. I have made a patch that removes the restriction to the extensions in the supported_extensions array and adds a warning that unsupported extensions may not be properly displayed. The patch is against d7 for now. If this is the desired behavior I would create another one for d8.

So please give feedback if this would be ok :)

leschekfm’s picture

Assigned: Unassigned » leschekfm
Status: Active » Postponed

As I have noticed that #1664844: Convert image toolkits into plugins is being worked on, I would suggest that these plugins define which file types they can handle (I've read this proposal somewhere before). Nonetheless I think it would make sense to add an option to disable the restriction to the supported extensions. This option could be integrated in the image-toolkit form (admin/config/media/image-toolkit).

I think this should be postponed until the above mentioned issue is resolved.

claudiu.cristea’s picture

Status: Postponed » Active

Now, new image types can be added by implementing/extending toolkit plugins. Drupal is shipped with the GD toolkit, handling JPEG, PNG and GIF image formats. A contrib/custom module can easily extend this toolkit with other image formats. Is this enough for your case?

errev’s picture

Issue summary: View changes

What about Drupal 7.
Does it work for D7?

brandy.brown’s picture

the patch works well for me thank you

DamienMcKenna’s picture

Assigned: leschekfm » Unassigned

Don't forget to remove your name from the "Assigned" field after you finish working on a patch.

Dave Reid’s picture

Assigned: Unassigned » Dave Reid
Status: Active » Needs review
Issue tags: +Media Initiative, +sprint
FileSize
0 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

We definitely should not hard-code this list of extensions. We should be using the list of supported extensions from the Image toolkit instead.

Dave Reid’s picture

FileSize
4.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,338 pass(es). View

The last submitted patch, 17: 1014816-image-widget-toolkit-supported-extensions.patch, failed testing.

Dave Reid’s picture

FileSize
5.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,331 pass(es). View

Improved inline docs to help clarify what is going on here.

mondrake’s picture

#20:

+++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
@@ -347,7 +347,16 @@ public static function isAvailable() {
+      $extension = Unicode::strtolower(image_type_to_extension($image_type, FALSE));
+      $extensions[] = $extension;
+      // Add some known similar extensions.
+      // @todo Figure out how to handle this better.
+      if ($extension === 'jpeg') {
+        $extensions[] = 'jpg';
+      }
+      elseif ($extension === 'tiff') {
+        $extensions[] = 'tif';
+      }
     }

1. There is #2477381: GDToolkit::getSupportedExtensions returns incomplete list aiming at improving the list of extensions actually supported by GDToolkit. Maybe this can be referenced in the @todo.
2. TIFF images are not currently supported by the GD toolkit (see ::supportedTypes), so you'll never get an $extension === 'tiff' at the moment. The toolkit needs to be enhanced if we want that, but I'd say that would be a separate issue.

In general I think the issue title is out of sync now, we are no longer adding additional file extensions, but rather removing the hardcoded ones with the ones provided by the default toolkit.

Dave Reid’s picture

Issue tags: +D8Media
Dave Reid’s picture

Issue tags: -sprint
Dave Reid’s picture

Title: Allow additional Image Field extensions » Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only)
Dave Reid’s picture

FileSize
5.47 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,712 pass(es). View
1.03 KB
mgifford’s picture

@Dave Reid any feedback on #21?

I installed the patch in #25.

I assumed that this would allow me to upload a SVG file to an article (after allowing the svg extension) but it didn't.

The issue summary should be updated to include what the patch is trying to accomplish. .

darol100’s picture

1+ for been implemented.

mondrake’s picture

Issue summary: View changes
FileSize
4.13 KB
12.47 KB

@mgifford re #26:

I assumed that this would allow me to upload a SVG file to an article (after allowing the svg extension) but it didn't.

That's right but here you need to have a toolkit that does support the SVG format. Core's GD toolkit does not. I tested #25 with ImageMagick 8.x-1.0-alpha1, configured to support SVG, and it works nice, see below:

Before selecting the image (see 'Allowed types')

After uploading the image (see the filename)

(caveat: you may need to add a convert effect to the image styles used for the image widget/formatter and convert to PNG or JPEG to see the derivative picture)

mondrake’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

I think this needs tests to make sure that we only upload files whose extension is within the intersect of the extensions declared by the field settings and those provided by the toolkit.

Also, the content of the interdiff in #25 is not in the patch :(

m1n0’s picture

Rerolled patch from #25, now containing the code from interdiff.

m1n0’s picture

Status: Needs work » Needs review

The last submitted patch, 11: drupal-allow_unsupported_image_extensions-1014816-11.patch, failed testing.

mondrake’s picture

Added tests. I think we're pretty close to RTBC now?

claudiu.cristea’s picture

Nits.

  1. +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
    @@ -47,7 +47,7 @@ public function __construct($plugin_id, $plugin_definition, FieldDefinitionInter
    -    return new static($plugin_id, $plugin_definition,$configuration['field_definition'], $configuration['settings'], $configuration['third_party_settings'], $container->get('element_info'));
    +    return new static($plugin_id, $plugin_definition, $configuration['field_definition'], $configuration['settings'], $configuration['third_party_settings'], $container->get('element_info'));
    

    Unrelated. Remove it from the patch.

  2. +++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
    @@ -27,6 +31,36 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, array $third_party_settings, ElementInfoManagerInterface $element_info, ImageFactory $image_factory) {
    

    This is not inheriting anymore the docblock :)

  3. +++ b/core/modules/image/src/Tests/ImageFieldWidgetTest.php
    @@ -30,6 +32,22 @@ public function testWidgetElement() {
    +    $field_config ->setSetting('file_extensions', 'png gif jpg jpeg tiff')->save();
    ...
    +    $field_config ->setSetting('file_extensions', 'png jpe tiff')->save();
    

    Remove space before "->".

mondrake’s picture

Dave Reid’s picture

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community
mondrake’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

In the patch we added 'jpg' and 'jpe' extensions to ::getSupportedExtensions, but have not adjusted ::extensionToImageType accordingly - this leads to failure in 'create_new' and 'convert' GD toolkit operations.

Also - for the image factory - do we go for constructor injection change (will force the patch to go to 8.1.x) like in #35, or shall we better introduce a getter method so that this can still go to 8.0.x?

mondrake’s picture

#38:

have not adjusted ::extensionToImageType accordingly

Fixed and added tests.

better introduce a getter method

Done here.

Also, removed 'Needs screenshots' tag as the ones in #28 are OK IMO

mondrake’s picture

Status: Needs work » Needs review
jwilson3’s picture

Allowing SVG upload to a website is a security risk because an SVG file can contain javascript. One mitigating factor may be that <script> tags seem to be ignored in external SVGs attached inside an <embed> <object> or <img> tag. I *think* the SVG has to be added directly into the DOM for the javascript to be executable, in which case, it would be up to other Drupal filtering mechanisms to strip the JS before display.

Can someone test this by creating and uploading SVGs containing the following tests, to be sure?

  1. SVG w/ script tags:
    <svg xmlns="http://www.w3.org/2000/svg"><script>alert('Danger');</script></svg>
    
  2. SVG w/ onload attribute in sub-element
    <svg xmlns="http://www.w3.org/2000/svg"><g onload="javascript:alert(1)"></g></svg>
    
  3. SVG w/ onload attribute in SVG element
    <svg onload="javascript:alert(1)" xmlns="http://www.w3.org/2000/svg"></svg>
    
  4. SVG w/ js in xlink
    <svg xmlns="http://www.w3.org/2000/svg"> <a xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="javascript:alert(1)"><rect width="1000" height="1000" fill="white"/></a> </svg>
    

There are more tests, some related to specific browsers, see the full list here:
Source: https://html5sec.org/#svg

jwilson3’s picture

Issue tags: +security
mondrake’s picture

Issue summary: View changes
FileSize
22.63 KB

#41, thanks @jwilson3.

This could not be tested with core only, as PHP GD2 (and therefore the GD toolkit) does not support vector image formats like SVG.

I tested this with the following environment:

  • latest D8.0.x HEAD
  • the patch in #39 applied
  • the patch in #2377747-73: Incorrect node create validation error when an invalid image is attached to a field applied - that introduces toolkit-based image validation instead of the extension-based only validation in HEAD currently
  • ImageMagick module 8.x-1.0-alpha1, with
    • SVG image format enabled in configuration (see README.txt) of that module
    • command debugging enabled in configuration of that module
    • use 'identify' for image format identification
  • ImageMagick 6.9.1-0 Q16 x86_64 2015-03-22 binaries installed

In that configuration, any attempt to upload any of the SVG files in #41 in an image field fails with the following message:

and in the watchdog we have the following failure from ImageMagick detected:

input:

User notice: ImageMagick command:
/opt/ImageMagick/bin/identify -format 'format:%m|width:%w|height:%h|exif_orientation:%[EXIF:Orientation]
' '/private/var/tmp/phpcwn59F'

which result in error:

User notice: ImageMagick error:
identify: no decode delegate for this image format `' @ error/constitute.c/ReadImage/501.

In other words - for the files indicated ImageMagick was unable to identify the files as images, and therefore upload was prevented (again, this requires #2377747-73: Incorrect node create validation error when an invalid image is attached to a field).

claudiu.cristea’s picture

Status: Needs review » Needs work
+++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
@@ -273,4 +286,17 @@ public static function validateRequiredFields($element, FormStateInterface $form
+  /**
+   * Returns the image factory service.
+   *
+   * @return \Drupal\Core\Image\ImageFactory
+   *   The image factory service.
+   */
+  protected function getImageFactory() {

Well, I don't think you can change the constructor signature neither in 8.0.x, neither in 8.1.x because in the meantime somebody might have extended the class in a module and that would break his site. IMO it's good to go with a getter.

Anyway, let's add a @deprecated tag with a message like "@deprecated in Drupal 8.0.x, will be removed before Drupal 9.0.0.

Also, I would mark this method as @internal (that is not yet documented, see https://www.drupal.org/node/2550249).

I would add also "@todo Replace this method with proper service injection in 9.0.x."

mondrake’s picture

#44 done, thanks for feedback.

claudiu.cristea’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
    @@ -273,4 +286,23 @@ public static function validateRequiredFields($element, FormStateInterface $form
    +   * @deprecated in Drupal 8.0.x, will be removed before Drupal 9.0.0.
    ...
    +   * @return \Drupal\Core\Image\ImageFactory
    ...
    +   * @internal
    ...
    +   * @todo Replace this method with proper service injection in 9.0.x.
    

    See https://www.drupal.org/coding-standards/docs#order for how to order the sections. @return, @deprecated, @todo is the correct order. @internal is not documented yet, let's leave it the last.

  2. +++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
    @@ -273,4 +286,23 @@ public static function validateRequiredFields($element, FormStateInterface $form
    +    if (!$this->imageFactory) {
    

    I wonder that this doesn't popup a notice when the protected property is not initialised. I would use !isset().

mondrake’s picture

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
@@ -273,4 +286,23 @@ public static function validateRequiredFields($element, FormStateInterface $form
+   * @deprecated in Drupal 8.0.x, will be removed before Drupal 9.0.0.

This is the ultimate sophistication: Introducing an already deprecated method :-)

Good to go, from my point of view.

mondrake’s picture

Issue tags: -security

Summarizing here a private conversation with the Security Team about #41, after having received clearance to make it public.

  1. this issue is about *Image* fields, and #43 demonstrates how toolkit-based validation may prevent uploading malicious files if these fail to be identified as images by the toolkit
  2. there is no check whatsoever on upload of SVG files in *File* fields, if they are set to accept 'svg' extensions: by design for these fields the validation is based on extensions only.
  3. So yes, allowing SVG uploads is a possibly self-inflicted security risk, as long as field and toolkit settings are changed from default behaviour.
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
@@ -273,4 +286,23 @@ public static function validateRequiredFields($element, FormStateInterface $form
+   * @deprecated in Drupal 8.0.x, will be removed before Drupal 9.0.0.

Let's not deprecate until we actually have service injection. We shouldn't deprecate until we actually can.

Also what about upgrade path issues? Also what happens if you change the toolkit and some image files are no longer supported - in this instance I don;t think you should be allowed to change the toolkit...

mondrake’s picture

Status: Needs work » Needs review
FileSize
7.5 KB
673 bytes

#50, removed the deprecation.

what happens if you change the toolkit and some image files are no longer supported

  1. it will no longer be possible to upload files with unsupported extensions in the Image fields
  2. nothing will happen to derivatives that have already been generated, but new derivatives for files with unsupported extensions (which also applies after an image style gets flushed) will no longer be generated if the toolkit ::isValid method return FALSE. This will result in a broken image icon and the Alt text being presented.
  3. Image and ImageToolkit are not connected to managed files, so contrib may have other effects if they process images that are not part of those files.

So... I am not sure it makes sense to 'lock' the toolkit if with the change some formats will become unsupported. Such lock would only work for managed files AFAICS, but as pointed above there could be other files that get into the equation. Also note, ImageMagick toolkit allows to define its supported extensions via configuration, so even within the same toolkit we might have the situation of extension previously supported that do not get support any more.

Also what about upgrade path issues?

We're not touching any config here, just intersecting the extensions supported by the toolkit with those defined in the field definition, that are two independent lists. Maybe I am missing something?

jwilson3’s picture

@Mondrake re comment #43:

files indicated ImageMagick was unable to identify the files as images, and therefore upload was prevented

So were you able to identify definitively that ImageMagick specifically blocking these because of the JS present or could it have been because the SVGs (as written in #41 didn't have the "full" format (with doctype and complete svg signature)? fwiw, most browsers understand the svg files as I've written them in #41, so i assume imagemagick has more strict rules on what it accepts, that may or may not have anything to do with the fact the svg has some embedded JS, thus you may need to specify a more compliant SVG file than the ones in #41 to test this.

Presuming many site builders may not know of the security risks inherent with SVG, this seems like a pretty glaring "self-inflicted" security hole to be acceptable to allow without any further discussion.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

I see @alexpott questions from #50 were answered and the deprecation was removed. Also, regarding #52, i see the response clear: By default, .svg files are not in the allowed extensions. So, the upload will fail. Note that this risk exists also for other kind of files if the site admin would allow such uploads.

Moving back to RTBC.

jwilson3’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
53.23 KB

Note that this risk exists also for other kind of files if the site admin would allow such uploads.

Not true.

For regular file fields, the Drupal 8 default extensions list only contains txt. If you manually configure the field to accept files with extension js, then the file extension is changed automatically to ".js.txt". See attached screenshot (taken from Simplytest.me instance running Drupal 8.0.2).

This same security check needs to be done for SVG. Expecting the image library to do this checking would be akin to allowing JS files be uploaded to the server and then let jQuery test to see if the code is safe or not. (Okay, that's a poor analogy, I admit).

jwilson3’s picture

A little research: According to the HTML5 spec, SVGs added to the page using the <img> tag cannot not execute embedded scripts, but scripts can be run when using the <object> or <iframe> tags.

Does this make it safe enough then to not have to change file extension? (The assumption being Drupal image fields will always use the <img tag)?

mondrake’s picture

Status: Needs work » Needs review
Issue tags: +needs security review

NR seems to me a more appropriate status, given input in #55.

#54

For regular file fields, the Drupal 8 default extensions list only contains txt. If you manually configure the field to accept files with extension js, then the file extension is changed automatically to ".js.txt". See attached screenshot (taken from Simplytest.me instance running Drupal 8.0.2).

This same security check needs to be done for SVG.

For regular *file* fields, it's out of scope of this issue.
For *image* fields, it seems to me that, based on comment in #55, we need consensus that it is not necessary.

Tagging 'needs security review', I do not know if it's the right thing to do. #49 reflects latest input from Security Team.

mondrake’s picture

mondrake’s picture

andypost’s picture

Status: Needs review » Needs work

The last submitted patch, 57: 1014816-57.patch, failed testing.

mondrake’s picture

Version: 8.1.x-dev » 8.2.x-dev
mondrake’s picture

Status: Needs work » Needs review
FileSize
7.47 KB

Rerolled.

Saphyel’s picture

the patch applies well, but when you try to upload a svg file you still see:

The specified file Tux.svg could not be uploaded.
Only files with the following extensions are allowed: png gif jpg jpeg.

And is still showing in the field even after edit the field:
Allowed types: png gif jpg jpeg.

Saphyel’s picture

Assigned: Dave Reid » Unassigned
Status: Needs review » Needs work
mondrake’s picture

@Saphyel please see #28 and #43, but also be aware about security concerns about allowing upload of SVG files (here in #41 and #49, but more recently in #2718305: Mitigation measures for ImageTragick (CVE-2016–3714/3715/3716/3717/3718)).

Saphyel’s picture

@mondrake anyway I added other formats like tiff and others random texts and is still only allowing the same 4 formats...

But I'm more concern about SVG to be honest, I'm gonna read all your links, thanks!

mondrake’s picture

I added other formats like tiff and others random texts and is still only allowing the same 4 formats...

that's because the default GD toolkit supports only those formats, at least at the moment. it does not matter what the PHP GD library supports.

The last submitted patch, 62: 1014816-62.patch, failed testing.

mondrake’s picture

Gábor Hojtsy’s picture

Issue tags: -Media Initiative

Remove duplicate tag.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs review

This issue still needs a security review according to the comments and issue tags.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

robpowell’s picture

Can someone summarize what the patch does and how to test it? Currently, on simplytest.me, it does not accept .svg - error "The specified file blog.svg could not be uploaded. Only files with the following extensions are allowed: png gif jpg jpeg." Is this expected?

Also, does this patch contend at all with modules/imagemagick?

joelpittet’s picture

SVG is xml, it can't be acted upon in the same way other images can be (crop, scale, etc) which would be what the image toolkits would support. So yes that is expected. edit: Apparently there is some support for this in imagemagik... sorry for the noise.

What do you mean by "contend"?

robpowell’s picture

I was just looking to see how I could help test if the patch is doing what is supposed to. If you could help me by clarifying what is the patch supposed to do, that would be great :). I was hoping that using this patch with imagemagick would allow me to upload an svg. I meant contention, would enabling imagemagik cause any negative effect to the results of this patch.

joelpittet’s picture

@robpowell, just a guess but you'd need imagemagick to test out with SVG because GD doesn't support SVG and I'd expect that would be available in the https://www.drupal.org/project/imagemagick module.

So this patch + imagemagick + core 8.3.x should provide that test, wanna give that a try?

mondrake’s picture

see comments #28 and #43

joel_guesclin’s picture

I don't know if this helps or confuses the issue, but I wonder whether it is actually appropriate to use the same field type for matrix and vector images (ie png, jpg, etc on one side, and svg on the other).
I say this for three reasons:
First, a lot of the things you can do with matrix graphics in ImageMagick (resizing them, scaling them, etc) are meaningless for vector graphics (all you do is specify the size)
Second, vector graphics are generally not used for the same things: vector graphics are great for icons, but a no-no for photos for example.
Third, one of the cool things you can do with SVG is actually to include the content directly in the HTML using the <svg> tag, which then allows you to do things like change the colour directly in your CSS - and this means not using the <img> tag at all. This seems to me so different from what is done with the Image type field that I wonder if it should not be handled by something completely different
Just my penny's worth...

mondrake’s picture

I feel the discussion here is getting too much biased on "SVG yes" vs "SVG no".

There are other image formats - WEBP, TIFF for example - that could be uploaded in an Image field if a toolkit supports them, and that cannot be now given the hardcoded restriction that this patch removes.

For example, even if #2340699: Let GDToolkit support WEBP image format (for PHP 7.1+) was solved, it still would not be possible to upload WEBP images in an Image field with the default GD toolkit, because of the hardcoded

$supported_extensions = array('png', 'gif', 'jpg', 'jpeg');

joel_guesclin’s picture

@mondrake - do you think that support for PDF belongs in the Image field type or is this something that should be handled by File Entity?
I don't know anything about TIFF, but my point was more that vector graphics are a very different kettle of fish to matrix graphics - but I confess my knowledge of both is rather sketchy and I will happily bow to the experts on this.
The one thing about SVG that is really different is the ability to refer to it in the CSS files, so for example the same icon can have different colors depending on where in the site it's used, just by changing the CSS

Lukas von Blarer’s picture

As mondrake stated, this is rather about supporting any format that the image toolkit can handle. TIFF, PDF, WEBP and SVG being candidates for that. And it is very important that we get support for this.

The security concerns SVG introduces are valid, but currently I create file fields and upload SVGs through that and render them as well. I don't understand how this different from simply supporting SVG in an image field. Could we strip out stuff like <script> tags if we render an SVG inline or through an image style? How big are the security issues if SVGs are rendered using a <img> tag? As far as I am aware this is not a security issue. jwilson3 came to the same conclusion in #55.

#79: SVGs are images and they should be supported by image field. Icons are definitely not the only use case for SVGs. An example: I recently created a website for an architecture firm. The want to display plans for their buildings. the old ones are JPEGs or PNGs and the new ones are SVGs. There is no point in creating two different fields for this.

The rendering of the SVGs is a more interesting topic. What I did was using a preprocessor which either renders a responsive image if it is a bitmap or a <img> tag if it is a SVG. Should we solve this in the field formatter or in the image style? and how does this affect responsive images? This is going to be difficult to solve... In most cases I would like image styles not to apply anything to SVGs. But for example converting them to a different file format is also a very valid use case which would need an image style to be applied. Any thoughts on this?

fietserwin’s picture

RE #82: If you want to solve this in an image style you probably will need an effect like the aspect switcher effect (D7, currently being ported to D8) that decided what to do based on aspect ratio (landscape or portrait). this new effect would be a "format switcher"or something like that. Not unfeasible, but not currently existing either.

If you want to solve it with an image formatter, you can intercept before the target image (the variable that will be used to render the src attribute) is changed to that of a derivative image url. Your formatter should extend the current image formatter and decide what to do based on format, e.g. vector graphics: just render the image itself, otherwise pass it on to the standard image formatter that can apply an image style for bitmapped images.

Lukas von Blarer’s picture

Having to decide what the image style should do based on the format make them too complex. This could live in contrib. But having a default behavior that solves this in the image formatter sound like a reasonable approach.

fietserwin’s picture

Issue tags: -needs security review

Regarding the needs security review tag.

A quick search on the internet reveals a lot of possible problems with the svg format. WP also doesn't support it out of the box. Using the svg described in https://bjornjohansen.no/svg-in-wordpress, I tested:
FF 49:

  • Direct load of svg: alert
  • svg as img: no alert
  • svg as iframe src: alert
  • svg as object data: alert

Chrome 53:

  • Direct load of svg: alert
  • svg as img: no alert
  • svg as object data: alert

So, apparently, no direct XSS attacks possible when we restrict ourselves to img tags, but other risks, see e.g. http://security.stackexchange.com/questions/11384/exploits-or-other-secu..., might still be possible, even with img tags.

However, once an image is on the server, it can be referenced via other constructs as well, thus saying that because this is an image field and thus rendering it is restricted to img tags cannot be assured. Example: anonymous people that may upload images and write comments with site internal links only. IMO this means that svg fiiles should be renamed like we currently already do for some other extensions in file_save_upload() : ... preg_match('/\.(php|pl|py|cgi|asp|js)(\.|$)/i', $file->getFilename()) ...

But this should not be part of this issue as it is more of an (already existing) issue with file fields in general and not, or at least less, for image fields.

Therefore removing the "needs security review".

fietserwin’s picture

Regarding the "Needs issue summary update" tag.

TLDR: Also check when defining an image field, not only when showing an image widget (and allow site builders to shoot themselves in the foot).

Why was this issue posted in the first place and what are we trying to "solve" currently?

The issue started with "I am defining a list of allowed extensions for an image field and without any message or warning the list that users see is a subset of it.". The curent patch tries to solve the fact that the list as defined by a site builder is reduced based on a hardcoded list instead of the list of extensions supported by the actual toolkit.

IMO we are tackling this problem in a wrong manner. I would like to propose the following functionality:

  • When defining a new image field, the default value for 'Allowed file extensions' should be the list of supported extensions.
  • When saving field settings, the entered list should be validated against the list of supported extensions.
  • If there are no unsupported extensions: save as is
  • If there are unsupported extensions, we have the following options (TBD):
    • Warn but accept the reduced set.
    • Fail with a form validation error.
  • If an existing image field is edited: no changes, ie take the currently saved list as value.
  • If an existing image field is saved: same validation as above.
  • If an image widget is displayed we have 2 options that should align with the options above:
    • Take the list as is. Do not make any changes to the list at this point.
    • Take the list and intersect with the actual list of supported extensions.
  • If an image with an unsupported but allowed extension is uploaded:
    • Fail (aligns with the fail option above).
    • Fail if dimensions are specified (never logical).
    • Fail if dimensions are specified and getimagesize() (does not require a toolkit) indicates that the dimensions fall outside the allowed dimensions. If getimagesize() fails to produce dimensions: TBD.

I am in favor of the warning option, not the hard fail as most error handling on the image widget side will still be necessary for when toolkit configuration has changed (more specifically the list of supported extensions).

If we go for the warning, the warning should indicate which extensions are unsupported and what the consequences are:

  • Image styles will not be applied to unsupported extensions.
  • Resizing due to min/max resolution will not be possible and thus image uploads might fail.
  • [future request] Auto rotating on upload will not be possible, so an upload might fail. To determine if we actually need to rotate, we would not necessarily need an image toolkit, just the EXIF PHP extension.

This proposal (assuming the warning option):

  • Leaves some freedom to site builders so they can decide for themselves to accept the consequences or change the list after all.
  • Toolkits do not have to be changed for this. They already cater for the case of unsupported extensions, though error handling higher up might have to be changed.
  • Details about handling edge cases on upload and validation may need to be refined and/or decided upon.
  • If the list of unsupported extensions changes after an image field has been defined (change of toolkit, change in (configuration of) toolkit to drop support for an extension) we will need the same error handling as already proposed above (mostly those options above that are in line with the warning case).

If we can get some agreement on this proposal we can update the issue summary and rework/extend the patch.

heddn’s picture

Issue summary: View changes
Status: Needs review » Needs work
heddn’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Updated the IS. Let's get some reviews on the direction aspects of this issue and some votes on its direction.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

blue_waters’s picture

Not sure how generally helpful this will be, but having just discovered this issue (via https://www.drupal.org/node/2541480) and needing to upload over 100 inline SVGs across several posts for a particular content type, I've created a workaround module here...

https://github.com/58bits/insert_svg

The code is mainly a copy of https://www.drupal.org/project/editor_file, updated to insert an img element with its src attributed set to the uploaded SVG file instead.

After saving a post with an inserted SVG, the editor will return to the built-in image widget, where additional formatting and a caption can be added.

Another option was to use the Paragraphs (https://www.drupal.org/project/paragraphs) module with the newly released SVG file formatter (https://www.drupal.org/project/svg_formatter), and a File field in a paragraph type - but in the end we needed inline text support.

sahaj’s picture

@blue_waters thanks for your solution, I will give it a try on D8. But I still would be grateful to see a fix for D7 (should I open a new issue for that?)

mondrake’s picture

Just a re-roll of #69 that no longer applies.

mondrake’s picture

shmel210’s picture

This module svg_image_field creates own field type widget and formatter, supports preview and alt attribute.

I am planning to add validation of svg image with unittest.

Please take a look on it, maybe worth to add it to core or left it as is.