Responsive Image not working in rewritten Views field/area due to XSS filtering

I am moving this to Views.

Thanks @handkerchief for the issue report.

@alexpott, @dawehner, @tim.plunkett, and I discussed this issue and agreed to defer triaging it on a more thorough understanding of the bug. A great next step would be for a contributor to confirm @handkerchief's report and document the exact steps to reproduce it, probably including some screenshots to illustrate the problem. This could be a good novice task.

Once we have steps documented for the scenarios that work and do not work (with screenshots), it could also be helpful to look in the page source to see if this is more a theme layer bug or a Views bug (i.e. are the media queries output but just not working?).

I can confirm.

Steps to recreate:

1. Enable Responsive Images module and create presets/breakpoints/etc.
2. Add an Image field and have it display as a Responsive Image.
3. Create a view to show: fields
4. Add your image field and have the formatter be Responsive Image
5. Add a Global: Custom Text
6. Add your image field using the replacement text (ex: {{ field_slideshow_image }} )

I'll attach screenshots and bug show this to XJM.

PS - Classes added to markup to make it easier to see the bug in the HTML.

Found a way to fix this by adding picture & source to the $adminTags variable on core/lib/Drupal/Component/Utility/Xss.php. Attaching patch and screenshot showing new markup with patch.

Nice, thanks @nmillin!

So #5 demonstrates that this is indeed related to XSS filtering. (@nmillin and I discussed that the change/hack above was just to confirm the issue; it's not actually a correct fix to allow these tags across the board because it opens security holes.)

I think this is a situation where the best fix might actually be to work around this and not use Views rewriting to render the field.

Maybe this would be a situation to use the rendered entity plugin for a global field? Make an entity display that is just that field, and then use that area handler on the view. In the center column of the Views admin UI, header, footer, no results behavior etc. Click add and search for "rendered entity":

Hmmmm... That wouldn't help my use case of adding custom markup. I would still need to use TWIG templates to do what I want. Screenshot:

Is it worth checking if/what issues picture & source elements would have for XSS? Googling around, I found https://html5sec.org/#142, but that is more for image tags which already have filters in Drupal. I tried some onload XSS attack vectors and they were stripped by Drupal.

Well, hypothetically speaking, I would create my custom display mode for my entity type/bundle, and provide a Twig template in my module or theme for the configured entity display that included the desired markup around the field. But I have not actually tried to do this so there may be flaws in this suggestion. :)

I should note that the decision we made in D8 to automatically Xss::filterAdmin() various things is not without flaws, and rewritten Views fields might be an example of that. But ideally as much markup as possible ends up in actual Twig template files and thus avoids the unwanted filtering issue.

What security issues does adding picture and source to $adminTags add?

It looks like $adminTags hasn't been updated in 3+ years (https://github.com/drupal/drupal/commits/8.2.x/core/lib/Drupal/Component...). Should $adminTags be reviewed to see if there are other HTML5 elements besides picture and source that should be included or is this list taken from a XSS security website? I'm confused where the list came from and I couldn't find anything searching online. Am I missing something simple here?

Your custom display mode in #10 is sort of what I ended up doing. I show: content and then use 3 twig template files to add some custom divs. Totally possible to do, but overkill for what I was trying to do. Related to that, I read https://www.drupal.org/node/2665694 that you linked to. As a site builder I'm cringing inside. Views won't be as useful to me in the future I guess.

(What part of "with feature parity for site builders" continues to worry people?) Anyway. Updating our XSS filtering APIs needs care, but I do agree it's worth posting a separate issue to consider whether there are additional tags that need to be allowed (like picture). Edit: I'd have thought there would be an issue for that already somewhere.

I think we should keep this issue scoped as is to sort out whether Views' XSS filtering here makes sense as it is currently implemented, or if there's a simpler way to accomplish this. Three template files does sound like a lot to display one little element.

Maybe the theme team can offer some insight here.

I looked for an existing issue, but I couldn't find one. I created https://www.drupal.org/node/2776667 to review the $adminTags variable. Please let me know if there is anything else I can do to advance this. Thanks XJM!

I found https://www.drupal.org/node/732992 which is where the $adminTags array came from (same HTML elements). Adding as a related issue since it is an old issue and was hard for me to find.

+1, I can confirm this behavior

I think it would be really essential for responsive images to work in views as expected. I am rewriting fields in views for various use cases on a regular base. Sometimes you really have to use the field display instead of content using view modes to have the individual fields for filtering or sorting purposes. Speaking of two core modules, I think they should work together well. I was rather surprised, that this behavior happened, after finishing many many views including the corresponding styles, trying to implement the responsive images as basically last instance. Now I have bit of a problem … Any news on this?

I totally agreed with @toni4i. There is certain requirement to use responsive images with views fields rewrite or custom text options. Upto some extend I can try manage the views by showing direct image field display but in case of Entity Reference fields where we want the image to link to the referred entity node in that case there is no solution other than doing something via views results alterations.

There is definite need to have solution for this problem. Is there any update on this ?

I created https://www.drupal.org/node/2776667 to fix the root issue (adding picture and source to $adminTags so they are filtered out), but there hasn't been any movement on that issue.

I'm not sure how to get core contributors to review that issue. #frustrating

Until there is a solution, I'm patching my sites (https://www.drupal.org/node/2776667#comment-11460577). This patch allows for picture and source html elements to be displayed and not stripped out.

@nmillin Thanks very much for the quick response!

I have tried applying that patch and that allows the &
tags now in views rewrite options. But now the problem is the media attribute for the tag which is not coming fine (see below)

Current: <source srcset="/sites/default/files/styles/640x320/public/2016-11/15483349-illustration-of-little-boy-Stock-Vector-boy-cartoon-student.jpg?itok=pTok5Eyn 1x" media=" 960px)" type="image/jpeg">

Required:
<source srcset="/sites/default/files/styles/themen_highlights_tablet_960x480_/public/2016-11/15483349-illustration-of-little-boy-Stock-Vector-boy-cartoon-student.jpg?itok=HOZeBMCP 1x" media="all and (min-width: 560px) and (max-width: 959px)" type="image/jpeg">

Due to which Responsive image switching is not working. Do you have any idea on this ? Any help would be much appreciated.

I can confirm the problem described in #23 … Picture and Source are now visible, but the media tag unfortunately does not get printed correctly – only the last value and a colon will be included.

<picture>
    <source srcset="/sites/default/files/styles/crop_4_3_mobile/public/2016-09/Meeting.jpg?itok=9CJWX_Bg 1x" media=" 45em)" type="image/jpeg">
    <source srcset="/sites/default/files/styles/crop_4_3_tablet/public/2016-09/Meeting.jpg?itok=xBmUUm43 1x" media=" 60em)" type="image/jpeg">
    <source srcset="/sites/default/files/styles/crop_4_3_wide/public/2016-09/Meeting.jpg?itok=12S6qH8p 1x" media=" 120em)" type="image/jpeg">
    <source srcset="/sites/default/files/styles/crop_4_3_wide/public/2016-09/Meeting.jpg?itok=12S6qH8p 1x" media=" 120.063em)" type="image/jpeg">
    <img srcset="/sites/default/files/styles/crop_4_3_wide/public/2016-09/Meeting.jpg?itok=12S6qH8p" alt="" typeof="Image">
</picture>

ran into the same issue with "Select multiple image styles and use the sizes attribute." ant a views overwrite were the sizes attribute gets filtert... by XXS filters... and only the part after the last colon gets output...

I can confirm #23 in 8.3.5. This issue is still not fixed

The patch in #5 works, but the output is still broken as stated in #23.
This issue was first reported 2 years ago for Drupal 8.1 and still isn't fixed in Drupal 8.4. :-(

Rewriting still not working in 8.5. Any chance this can get sorted?

F

I'm actually not able to reproduce this in 8.6, not sure if something was changed?

I just tested this on a fresh install of Drupal 8.6.0 and I can confirm it is still happening.

If I create a view of content type article and add the field image using the Responsive Image formatter, the results are correct. If I rewrite the field and include the image field results in the rewrite, the <picture> tag are stripped out and all is left is the <img> tag.

F

I've done a bit of further testing and <picture> and <source> are being filtered out because of the core Xss.php filtering. The patch above no longer works as the code in that file in Drupal 8.6.0 is slightly different, so the patch fails. I've no idea how to create a patch, but 'picture' and 'source' needs to be added to the $adminTags array in that file.

As a side note, are there any XSS security issues if Picture and Source are allowed?

F

This issue still stands but I just tried the suggestion by @xjm at #10 and it certainly works!

Could probably give this a try for anyone who wants to just make it work temporarily. I guess this doesn't count as an actual fix though.

We were able to get the source and picture tags back by not rewriting the image in the view at all. It took some re-jiggering of the view and its css, but it worked. Seems odd that source and picture are stripped this way.

Hi All,

comment #38 is a work around which leads to lots of rework and complexity specially if the views are complicated as was in our case, patch in #37 is working prefectly fine for 8.6.6 version.

#37 would be nice - but the paths

diff --git a/public_html/core/lib/Drupal/Component/Utility/Xss.php b/public_html/core/lib/Drupal/Component/Utility/Xss.php

should be

diff --git a/core/lib/Drupal/Component/Utility/Xss.php b/core/lib/Drupal/Component/Utility/Xss.php

tia

Greetings,
I've faced the same problem. i've solved this by keeping my view and used it with a twig template.

First I've created the different images i needed for my picture HTML tag and output them as URL with a specific image style, i my case two images. Then I've used the twig template "views-view-fields--YouR-VieW-SySteM-NAmE" and added the following code in the twig template:

        <picture>
          <source srcset="{{ fields.field_Your_ImAge_FiELd_NamE_1.content }}" media="(min-width: 992px)">
          <source srcset="{{ fields.field_Your_ImAge_FiELd_NamE_2.content }}" media="(min-width: 768px)">
          <source srcset="{{ fields.field_Your_ImAge_FiELd_NamE_1.content }}" media="(min-width: 576px)">
          <img class="card-img mb-2" src="{{ fields.field_Your_ImAge_FiELd_NamE_1.content }}">
        </picture>

Note 1: I had the problem that twig commentary was in the SRC attribute of those tag thus the images were not displayed in the browser. Strangely. But the URL was correctly generated, so you will have to deactivate twig debug to see the image.

Note 2: To know the name of your fields just hover the field link in the Field area of your view, and check the URL in the bottom left of your browser.

Note 3: This code is only a small portion of the code I've used in the twig template. I've also used an other view template to output what i wanted, in my case it was a bootstrap 4 card.

Hi,

I've fixed the paths mentioned in #40 and submitted the patch in the related issue.

I'm currently using that patch as well and I have the replacement patterns working with the <picture> element.

@dsdeiz Thank you for #37 patch, and also so much thanks to @postovan-dumitru for his #42 comment and related issue. These solutions works fine for 8.7.4 version.

I agree with #38, if possible just re-gig the view to not rewrite the output of the image.

Otherwise I also look forward to this patch being rolled out.

Same problem with excluding the field and rewritting it to a custom one.

This is another major one ony my adventure updating a medium D7 site to D8. If you are working with views a lot and theme them for usage with for example bootstrap, you rewrite a lot.
Templating and printing single fields is not a real option here when using lots of views.
#38 works as a quick fix. Thank you for that patch. To apply it you need to remove that "public_html" that should not be there.

I'm using Blazy to display the images in my site. Maybe it could be added to core?

This could be useful as a workaround to someone else.

As per the comment by @attisan on #37 patch has path issue and it's not working with 8.8.x. So I've updated the patch.
Here is the updated patch

Ok i can definitely confirm that a workaround is to chose a relation to the media in views and then use the according image field with that relation field. I its also better to do that because the responsive image formatter is avaliable there.
That seem to work nicely it seems so far.
It does not, my bad.
Patch #49 works here.

Patch #49 works for me too, thanks.
In which drupal version will this be implemented ?

In drupal 8.9.1 and 8.9.2 this patch does not work anymore due to changes in
web/core/lib/Drupal/Component/Utility/Xss.php

Was able to change the file manually to get the same result.

            $skip_protocol_filtering = substr($attribute_name, 0, 5) === 'data-' || in_array($attribute_name, [
              'title',
              'alt',
              'rel',
              'property',
              'media'
            ]);

Patch from #49 work good with Drupal 9.1.0. Thanks!

#49 also worked for me in 8.9.13.

I agree my lord :)
I have no idea if that can be implemented as i do no know about the security issues or why that was implemented. Maybe some core dev will comment on this.
I just know i have to patch every single install because i use views rewrites extensively.

Same problem if we use responsive image style with type "Select multiple image styles and use the sizes attribute." and sizes attribute with media queries in it. Then Xss removes all up to and including last colon in sizes atttibute. I modified the #49 adding 'sizes' to the skipped attributes list.

Fix codestyle warning "A comma should follow the last multiline array item" for #60

Looking at #2776667: Review/update $adminTags variable for new html elements to be whitelisted and the proposed fix here, this seems like a duplicate as it now stands.

Per @xjm in #14

I think we should keep this issue scoped as is to sort out whether Views' XSS filtering here makes sense as it is currently implemented, or if there's a simpler way to accomplish this.

So I'd say we either focus on a different path than updating the allowed tags here, or we conclude 'Updating the allowed tags is the only correct fix here', close this, and try to land #2776667: Review/update $adminTags variable for new html elements to be whitelisted.

Thank you @Lendude for pointing to this issue. Yes indeed, as it is now this is a duplicate.

As part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)". This issue was marked "Postponed (maintainer needs more info)" a year ago.

Like @Lendude and @Eugene Bocharov mentioned, this is duplicate of #2776667: Review/update $adminTags variable for new html elements to be whitelisted. I have confirmed that picture and source are included in the patch (as well as noscript) for the duplicate issue.

Marking this as duplicate and updating other issue to request issue credit be added for who worked on this issue.

Thanks!

It solves the responsive image problem for Drupal core: 9.5.10.