Follow-up to #1825952: Turn on twig autoescape by default

Non-technical explanation

After #2264041: Add a test to ensure title callbacks are not vulnerable to XSS have proven that even battle hardened core developers can't write XSS free code we have introduced #1825952: Turn on twig autoescape by default to fix a torrent of security holes already present in core known and unknown and to avoid the most frequent kind of sechole(Security Hole) in the history of Drupal contrib. However, this has broken some places that were already securely written, resulting in broken layout and HTML tags shown to users. We need to find those places and update them to be compatible with the new method.

Problem/Motivation

#1825952: Turn on twig autoescape by default fixed escaping globally and caused HTML escaping on places where we explicitly set HTML in a variable. This was expected. The patch was an absolute must and avoiding / fixing all paths would've taken a lot of time and made an already big patch impossible to review and commit.

Instead we have opted to go ahead with this and let people find the broken pages. If people would've cared to review patches this could've been avoided but we know this is a no-go so instead we forced it.

@ti2m found escaped strings on several paths:

Another (and the last) update on the crawled urls. I enabled all modules on a fresh install and crawled the site as user 1. I only found two more urls with escaped strings (first two in the list below). But the general problem is, that e.g. node edit forms aren't covered at all as no node exists on a vanilla install. I could post a file with all covered urls, roughly 300, if anyone is interested.

The total list of urls with escaped HTML strings that I found:

  • /admin/config/regional/translate/settings
  • /admin/config/development/logging
  • /admin/modules/uninstall
  • /admin/config/regional/date-time/formats/add
  • /admin/config/regional/date-time/formats/manage/long
  • /admin/config/people/accounts/fields/user.user.user_picture
  • /admin/config/content/formats/manage/basic_html
  • /admin/structure/block
  • /admin/structure/block/list/seven
  • /admin/structure/views/nojs/rearrange-filter/frontpage/page_1
  • /admin/structure/menu/manage/footer
  • /filter/tips
  • All 'manage fields' pages (ref: Screenshot)
  • /admin/config/services/rest

Proposed resolution

If at all possible move all markup into a Twig template. If not then please read https://www.drupal.org/node/2311123 for alternative solutions. See aneek's patch at #2305831-35: Double escaping on /admin/modules/uninstall for an example.

Remaining tasks

Change the offending pieces to properly use Twig templates or use inline templates as described in https://www.drupal.org/node/2311123 .

Comments

xjm’s picture

Status: Postponed » Active
jbrown’s picture

jbrown’s picture

jhodgdon’s picture

node/1/translations - added issue today, just made it a child of this one: #2305799: HTML tags (raw) shown in Status column on Translations page

I noticed this on some Views UI pages today too, but hadn't filed issues, will hold off.

StuartJNCC’s picture

and here is another one: User info on "Search results" page - /search/node?keys=

webchick’s picture

The issue summary here could do with some instructions for people who want to help with this initiative. I'm seeing people attempt to use SafeMarkup::set() as HEAD does and then get slapped down for it.

jhodgdon’s picture

Yes. I'm also unclear on whether this issue is a "fix it all up in one fell swoop" or if we're supposed to file child issues.

benjy’s picture

The permissions page also has double escaped strings.

andyceo’s picture

One more: on the page with adding new field, autogenerated field name is double-escaped.

andypost’s picture

jhodgdon’s picture

So... These problems are all over the admin UI now. Can't we fix the root problem that caused them to appear? Or can someone ***** PLEASE ***** update the issue summary so we know what the real cause and correct fix is?

mradcliffe’s picture

I guess if you sneak it in as usage of StringTranslationTrait::t() it gets passed review?

$this->t() is used over 2000 times in core, which is pretty much SafeMarkup::set().

If the string needs to be translated anyway, then is it really so bad to use SafeMarkup::set() as that's what translation is going to do?

ivanjaros’s picture

vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

Issue summary: View changes
ianthomas_uk’s picture

Issue summary: View changes

I can't help with how to fix it, but I can help people understand why we shouldn't just revert the patch that caused a very visible problem. I've added a non-technical explanation to the issue summary.

chx’s picture

chx’s picture

Issue summary: View changes
LinL’s picture

LinL’s picture

larowlan’s picture

chx’s picture

Status: Postponed » Active

Go!

chx’s picture

Issue summary: View changes
chx’s picture

Issue summary: View changes
rpayanm’s picture

Another url with double escaped strings that I found was in the install process #2317281: Double escaping of install errors.

Sutharsan’s picture

pbz1912’s picture

aneek’s picture

aneek’s picture

Just curious, has something changed in last commits in Drupal 8.0.x? All the issues with Twig Autoescapes are seems to be fixed.
I checked with #2309215: HTML double-escaping in revision messages, #2309929: HTML double-escaping in field forms & #2319667: Simpletest Module Double escaped HTML in hook_requirements with the latest code checked out and all seems to be fixed automatically.

Can any one confirm on these?

Thanks!

andypost’s picture

FileSize
44.19 KB

Confirm that, that's very strange!!!

aneek’s picture

FileSize
31.47 KB

@andypost, so this means I'm not wrong. Something do happened in the last commits. Maybe the recent one. Strange!
Get the core committers, ha ha... :-)
Last Commit

aneek’s picture

@andypost, as you have seen also, a fresh install brakes all of them.
Double escaped string

Rade’s picture

mgifford’s picture

hass’s picture

Tried SafeMarkup::set(), but it's not working with conditionals.

Cottser’s picture

I just want to mention here that it looks like this ongoing issue will fix a lot of these cases:

#2324371: Fix common HTML escaped render #key values due to Twig autoescape

So please test with the patch there before creating new child issues. Thanks!

znerol’s picture

Cottser’s picture

webchick’s picture

Do we need a fresh run through all of these to verify them now that #2324371: Fix common HTML escaped render #key values due to Twig autoescape was committed?

chx’s picture

Yes.

webchick’s picture

Status: Active » Postponed (maintainer needs more info)

Okie doke.

joelpittet’s picture

Title: [meta] Fix double-escaping due to Twig autoescape » [meta] Fix HTML escaping due to Twig autoescape
Issue summary: View changes
xjm’s picture

Issue tags: +SafeMarkup
joelpittet’s picture

Status: Postponed (maintainer needs more info) » Active
joelpittet’s picture

To clarify, I un-postponed so people don't think that this is a postponed issue that needs fixing elsewhere. This issue "Needs issue summary update" as the active task and it's meta tasks need to be verified and closed if they are no longer valid.

webchick’s picture

I actually wonder if we should just close this out and open up individual issues for things we find?

Zekvyrin’s picture

@webchick: I worked in a few child issues and, for me, it was very helpful that we had this one as a parent issue, as I could easily access proposed solutions and check other similar issues' solutions as a reference.

webchick’s picture

Fair enough. The first step is go through all of the issues mentioned in the issue summary and sidebar and see if they are still issues anymore.

joelpittet’s picture

I've went through the child issues and they are all still valid in the context of not being fixed by that render #key values commit.

I was pouring over whether they are the 'correct' solution or not. We are trying to avoid SafeMarkup::set(), some people just side step the issue by using #markup which in some cases is the same but different way of doing SafeMarkup::set().

'#inline_template' also should be discouraged, but it's many times the only viable solution when you are dealing with table cell content with new tags. (#type table we are manipulating the data to fit the table markup, instead of marking up the data in X list format in a specific template. ) This pattern seems to be very consistent, which is good, but also a PITA for both manipulating markup and SafeMarkup::set.

penyaskito’s picture

Cheet’s picture

Added #2409477: Various ! placeholders throughout core are now showing escaped HTML (especially examples) (caused by the recent changed introduced by #2399037: (change notice, etc.) String::format() marks a resulting string as safe even when passed an unsafe passthrough argument).

So looking into this issue I noticed that simply changing the "!url" to "@url" totally mangles any GET request that you can make.

For example:
"admin/config/media/image-styles/add?a=b&b=c"

becomes:
"admin/config/media/image-styles/add%3Fa%3Db%26b%3Dc"

It looks like the string is marked as "unsafe" and escaped. Perhaps there should be a way to mark variables as urls or some special use case for urls specifically. However for now, setting the escape character to "@" in a t() string is NOT the way to go.

Adam Clarey’s picture

I'm not sure how this is supposed to work as templates are nested, some templates are going to be outputting the rendered markup from child templates.

I've created a custom theme and every place I want to output markup from a variable it is always being escaped.

For a specific example, I have a view and without any custom templates it renders fine. However when I copy views-view-fields.html.twig to my custom theme without changing it at all, the view content is then displayed as entirely escaped markup.

From this thread I'm guessing its because field.content is the pre-rendered markup for each field.

Can someone point out how you are supposed to output markup in twig templates please? Maybe using my specific example as a use-case.

Thanks

Cottser’s picture

@Adam Clarey the nesting is not the issue, the issue is that by default views_view_fields is rendered as a theme function, not a Twig template:

#2348747: Convert theme_views_view_fields() to 100% Twig: remove the theme function
#2363423: views-view-fields.html.twig gets escaped

Adam Clarey’s picture

So am I to assume this is a bug that is a bug that is being fixed? The only thing I can gleam from the long issue thread is that if I want to use a template in my own theme, I need to add '|raw' to the end of the field so it will output the raw markup instead of the escaped markup.

Obviously this isn't ideal

Cottser’s picture

@Adam Clarey if you can please test the latest patch from #2363423: views-view-fields.html.twig gets escaped and comment on that issue with your results that would be super helpful!

You can ignore the first issue, I probably shouldn't have included that link (or not first anyway), that's just how we discovered the issue.

Adam Clarey’s picture

The patch didn't help i'm afraid.

Putting the template views-view-fields.html.twig in a custom theme only outputs the sanitized markup.

To output as actual markup the twig variables require '|raw' adding to theme, eg field.content|raw

Cottser’s picture

Please, #2363423: views-view-fields.html.twig gets escaped is the issue for these comments. If you can post there the steps you used to test that would be super helpful! Thanks.

cilefen’s picture

joelpittet’s picture

Title: [meta] Fix HTML escaping due to Twig autoescape » Fix HTML escaping due to Twig autoescape
Category: Task » Plan
cilefen’s picture

cilefen’s picture

DYdave’s picture

Got the same problem as mentioned in #59.
Related with #2610236: Views - "No Results Behavior" for individual fields escapes HTML.
Running on 8.0.6.
When using Views Custom text field, whatever you put in the text field, whether it resolves to empty or not, the returned value is not going to be empty, unless the text field is left completely empty (kind of useless and defeats the purpose of the field).
See the very good inline comment in Custom.php line 67:

<?php
// Return the text, so the code never thinks the value is empty.
?>

So whether your pattern in the text field evaluates to empty or not, it doesn't matter, returned value is not going to be empty.

When setting up a No results text, the value then ends up in the wrong case, in my opinion, again, with a great inline comment in FieldPluginBase.php line 1308:

<?php
      // If the string is not already marked safe, it is still OK to return it
      // because it will be sanitized by Twig.
?>

ok, so is it going to be the recommended practice for such field customization or is this perhaps a buggy limit case?
 
I would be very happy to provide whatever additional information or screenshots, but the test case is exactly the same as #2610236-8: Views - "No Results Behavior" for individual fields escapes HTML with the Custom text field.
 
Of course, overriding the views field theme with {{ output|raw -}} worked fine, as mentioned in #59 and at #2584655-13: Views field "Global > Custom Text" incorrectly escapes HTML.
Let me know if there's anything I can do to help on this.
Cheers!

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DamienMcKenna’s picture

This is causing some small problems for Metatag: #2649592: Write tests to cover apostrophe handling in meta tags

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.

pwolanin’s picture