views-view-fields.html.twig gets escaped

Thanks for the report @stefan.korn!

Here's what we were doing on #2348747: Convert theme_views_view_fields() to 100% Twig: remove the theme function, taken out so we can at least fix this one.
I've tested it briefly, stuff is still escaped, let's see what the bot thinks.

Oops, forgot one on the theme function.

Looks good, thanks @Manuel Garcia. Can we get some tests please to show things are getting properly escaped and not double escaped?

Edit: Importantly, we should have a test-only patch that fails without the patch here.

So forgive me if I'm mistaken, but if we don't remove the theme function (See comment 77 on #2348747: Convert theme_views_view_fields() to 100% Twig: remove the theme function), this patch then needs to be reworked, so that the sanitization takes place on the theme function instead. That way when you are not using the twig file, stuff still gets sanitized. Does that sound right?

Yes that sounds right, two potential code paths: theme function and template.

OK I've been doing some testing on this one, and in order for us to fix this bug we have to move the markup generation part out of template_preprocess_views_view_fields, so stuff like:

$object->wrapper_prefix = '<' . $object->inline_html;
        $object->wrapper_prefix .= $attributes;
        $object->wrapper_prefix .= '>';
        $object->wrapper_suffix = '</' . $object->inline_html . '>';

has to be moved into the theme function, and also into the template file.

Otherwise, doing the sanitizing on the theme function will result in escaped html being printed.

How should we do this, on this issue, a new one, or on #2348747: Convert theme_views_view_fields() to 100% Twig: remove the theme function ?

If it's necessary to fix the escaping then this issue seems appropriate :)

Some progress on this.

• Salvaged template_preprocess_views_view_fields refactoring from #2348747: Convert theme_views_view_fields() to 100% Twig: remove the theme function
• Moved markup generation into theme function.
• Salvaged views-view-fields.html.twig part from #2348747: Convert theme_views_view_fields() to 100% Twig: remove the theme function
• Moved the Saniziting into the theme function (except content, see @todo)

I'm not sure how to handle sanitizing the content... before the patch, it was done on render() using sanitizeValue(). But now from the theme function not sure how to go about it. Any pointers?

Thanks @Manuel Garcia, I don't have a thought yet for how to fix the theme function not escaping, but here's a small point.

+++ b/core/modules/views/templates/views-view-fields.html.twig
@@ -25,17 +27,24 @@
-<!--
-THIS FILE IS NOT USED AND IS HERE AS A STARTING POINT FOR CUSTOMIZATION ONLY.
-See http://api.drupal.org/api/function/theme_views_view_fields/8 for details.
-After copying this file to your theme's folder and customizing it, remove this
-HTML comment.
--->

This comment should be kept.

Actually Drupal\views\Plugin\views\HandlerBase::sanitizeValue() is just a fancy wrapper for Xss and SafeMarkup functions, and in this case I think it's just sending the output to SafeMarkup::checkPlain().

Good catch Cottser on the comment, restored it now in this patch.

I've done some testing by copying over the twig template into bartik, and if we keep the sanitization on FieldPluginBase::render, the field.content does NOT get double escaped - looks like we can keep it there. So this patch restores that change. Could use with double checking, I suppose.

This should get rid of all those exceptions. Not sure what's going on with the failing test though.

More on fixing exceptions..

1. +++ b/core/modules/views/views.theme.inc
   @@ -219,12 +198,33 @@ function theme_views_view_fields($variables) {
   +    $wrapper_element = SafeMarkup::checkPlain($field->wrapper_element);
   +    if ($wrapper_element) {
   +      $output .= '<' . $wrapper_element . $field->wrapper_attributes . '>';
   +      if (isset($field->label_element) && !empty($field->label_element)) {
   +        $label_element = SafeMarkup::checkPlain($field->label_element);
   +        $output .= '<' . $label_element . $field->label_attributes . '>';
   +      }
   +      $output .= SafeMarkup::checkPlain($field->label);
   +      if (isset($field->label_suffix) && isset($field->label_suffix)) {
   ...
   +    if ($element_type) {
   +      $output .= '</' . $element_type . '>';
   +    }
   +    if ($wrapper_element) {
   +      $output .= '</' . $wrapper_element . '>';
   +    }
      }
   

   Should we use a template/inline template here instead?

2. +++ b/core/modules/views/views.theme.inc
   @@ -219,12 +198,33 @@ function theme_views_view_fields($variables) {
   +    // @todo this is not sanitized in render, cant use sanitizeValue from here
   

   If we add a todo, let's create an issue for that.

Thanks @penyaskito for the review!
We've already discussed it on IRC, but I'll answer here as well for people reading the issue.

1. See #2348747: Convert theme_views_view_fields() to 100% Twig: remove the theme function (not getting in because of perf)
2. Good catch, that's actually not necessary any more, this patch now removes that (see #14).

Looks good to me, but still needs the test.

Oh, this is a nice improvement. This will also help us with #2502089: Remove SafeMarkup::set() in template_preprocess_views_view_table().

+++ b/core/modules/views/views.theme.inc
@@ -219,12 +198,32 @@ function theme_views_view_fields($variables) {
+    $wrapper_element = SafeMarkup::checkPlain($field->wrapper_element);
...
+        $label_element = SafeMarkup::checkPlain($field->label_element);
+        $output .= '<' . $label_element . $field->label_attributes . '>';
...
+    $element_type = SafeMarkup::checkPlain($field->element_type);
+    if ($element_type) {
+      $output .= '<' . $element_type . $field->element_attributes . '>';
+    }

I think we should not just be checkPlain()ing the tag -- we should be validating it against an allowed and valid tags list. Maybe the best thing to do would be to instead compose the whole markup string without any early sanitization and then run it through SafeMarkup::checkAdminXss().

Also, is there any possibility of converting $output to a render array? Then we could either put it in #markup (which would make it be XSS-filtered automatically) or use more explicit render array structure.

As for converting $output to a render array, if so it would need to be immediately rendered so probably not much point that I can see. Theme functions must return strings.

@Cottser, thanks! So I'd go with the first suggestion then: assemble the HTML string, then checkAdminXss() on the whole thing. And if someone wants to use tags that are not allowed in the admin list, they can do that in code rather than through the UI.

Mmm, just noticed that the template also uses the elements right there. What's the relationship between $output and the template? How can we ensure the template is fully sanitized for XSS? Maybe we should check against the admin tag list? checkPlain() is a wash if we're putting angle brackets around it.

@xjm yep that's a good point. Anything output in the template that isn't SafeMarkup (or similar with the new things we are going to introduce like SafeString) will be escaped, but script escaped == script.

This may be a "if you want to shoot yourself in the foot" case as long as Views only provides safe options for wrappers and doesn't provide a text input so you can enter whatever you want. Not sure.

Hi!
I'm newy at D8. So perhaps my comment is wrong. I'm reading twig documentation and found there are 2 filters to avoid double escaped strings:

{% autoescape 'html' %}
    {{ var }}
    {{ var|raw }}      {# var do not escapes #}
    {{ var|escape }}   {# var do not escapes a second time #}
{% endautoescape %}

Thus the sample you wrote should be wroten as

{% autoescape false %}
{% for field in fields %}
  {{ field.separator|escape }}


  {{ field.wrapper_prefix }}
    {{ field.label_html|escape }}
    {{ field.content|escape }}
  {{ field.wrapper_suffix|escape }}
{% endfor %}


{% endautoescape %}

Or you can set the filter "|escape" only where you know you'l have a double-escaped string.

Hope it helps.

Oh! Sorry i was reading on double-escape and came here from a link. And my comment out of branche's target

The patch from #20 is still fine. @prajaankit in the future please leave a comment when uploading a new patch explaining what you're doing.

I'm going to see if I can rough in a test here.

It's a start, anyway.

Beta eval added.

This is such a nice clean up! And it has tests. I'd like to see some manual testing on this.

Steps:

1. Apply the patch.
2. Copy the template to your theme's templates folder and flush cache.
3. Make a view with fields (or use an existing one).
4. Compare the markup before and after without the patch version of the theme_function and twig template.
5. Compare the markup before the template move and after the patch

I'll pick this up tomorrow if nobody snags it before then.

I coped the template to Classy and check a simple View before and after. Screenshots attached.

I realized something. Is it appropriate that the "THIS FILE IS NOT USED AND IS HERE AS A STARTING POINT FOR CUSTOMIZATION ONLY." text is there as an html comment and not a Twig comment? That makes it visible in the markup.

@davidhernandez re "THIS FILE IS NOT USED AND IS HERE AS A STARTING POINT FOR CUSTOMIZATION ONLY."

That's a bit of legacy porting, probably should be Twig but if we can get that twig template in, it shouldn't matter. Feel free to open up a follow-up if you want to remove it quickly. I'll RTBC it right away if you ping me.

Thanks for covering off #36.4 @davidhernandez

I guess that is actually 5, since I moved the template. I didn't check before and after without moving the template.

@davidhernandez so that is "with the patch" test in #38?

That is before and after the patch, but with the template moved to Classy in both cases. I did not try things as-is, leaving the template alone, which I assume is what you want for testing the changes to the theme function?

Thanks for the manual testing @davidhernandez!

I did some too, the scenario was to take the existing admin content, convert from table to unformatted list and join the last 3 columns inline with a |.

Reason I did the testing this way is to make sure the changes that were outside the template didn't break the existing theme function in some way.

Before Patch

With theme function

With twig template

After Patch

With theme function

With twig template

Turns out there is a missing | between the first and second inline item after this patch is applied.

You can see this in the screenshots after Status: Published|

The good thing is that they are consistent between the output of the theme and twig template after the patch is applied., the bad thing is we lost the separator:S

Likely need to add that to the tests.

To be clear, you are talking about losing the pipe, |, between "Published" and "Updated"?

Yes

@joel, what do you use to get those markers on the screenshot?

Skitch! https://evernote.com/skitch/

I rerolled the patch to be a little more current.

There were conflicts, however all I really needed to do was clean the 3-4 lines of code that were causing the issue. They were found in views-view-fields.html.twig at around line 34

I rerolled the patch to be a little more current.

There were conflicts, however all I really needed to do was clean the 3-4 lines of code that were causing the issue. They were found in views-view-fields.html.twig at around line 34

EDIT: i messed this one up. disregard

Reroll without the messup. My apologies.

Interdiff fixing twig comment close

Ok so something was wrong with my install in my tests I think because I've had @dorficus and I redo a manual test without being able to reproduce that issue. Also I believe @davidhernandez also couldn't get that to show up like that either.

Here's the new manual testing for #53

1. +++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
   @@ -0,0 +1,61 @@
   +    \Drupal::service('theme_handler')->install(array('views_test_theme'));
   

   Don't we need a test with and without this test theme installed?

2. +++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
   @@ -0,0 +1,61 @@
   +    $this->assertNoRaw('</');
   

   $this->assertNoEscaped('<') is easier on the eye.

3. +++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-fields.html.twig
   @@ -0,0 +1,48 @@
   +{#
   +/**
   + * @file
   + * Theme override to display all the fields in a views row.
   + *
   

   I think this test twig file should have a comment stating that it exists because by default the one provided by views is not used.

4. +++ b/core/modules/views/views.theme.inc
   @@ -219,12 +198,32 @@ function theme_views_view_fields($variables) {
   +    $wrapper_element = SafeMarkup::checkPlain($field->wrapper_element);
   +    if ($wrapper_element) {
   +      $output .= '<' . $wrapper_element . $field->wrapper_attributes . '>';
   +      if (isset($field->label_element) && !empty($field->label_element)) {
   +        $label_element = SafeMarkup::checkPlain($field->label_element);
   +        $output .= '<' . $label_element . $field->label_attributes . '>';
   +      }
   +      $output .= SafeMarkup::checkPlain($field->label);
   +      if (isset($field->label_suffix) && isset($field->label_suffix)) {
   +        $output .= $field->label_suffix;
   +      }
   +      if (isset($label_element)) {
   +        $output .= '</' . $label_element . '>';
   +      }
   +    }
   +    $element_type = SafeMarkup::checkPlain($field->element_type);
   +    if ($element_type) {
   +      $output .= '<' . $element_type . $field->element_attributes . '>';
   +    }
        $output .= $field->content;
   -
   -    $output .= $field->wrapper_suffix;
   +    if ($element_type) {
   +      $output .= '</' . $element_type . '>';
   +    }
   +    if ($wrapper_element) {
   +      $output .= '</' . $wrapper_element . '>';
   +    }
   

   The calls to SafeMarkup::checkPlain() should just be htmlspecialchars() as their safeness is immediately lost through string concatenation.

+++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
@@ -0,0 +1,61 @@
+    $this->assertEqual($this->config('system.theme')->get('default'), 'views_test_theme');

IMHO this line is pointless. We should no test that config API works as expected

+++ b/core/modules/views/templates/views-view-fields.html.twig
--- /dev/null
+++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-fields.html.twig

+++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-fields.html.twig
+++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-fields.html.twig
@@ -0,0 +1,48 @@

Given that this is functional identical to views-view-fields.html.twig i'm curious whether you could just use a twig include here

There's an issue that has some theme functions on the chopping block so that test without a theme install was not added. I did change the `<` to `<` and added the commenting as requested. I also changed the SafeMarkup::checkPlain methods to htmlspecialchars() function.

EDIT: I didn't see your request before I pushed this patch. I'll take care of those others.

dorficus, can you post an interdiff?

Here is the interdiff between #53 and #57.

@davidhernandez I apologize for forgetting the interdiff. Thank you @manuelgarcia for posting it.

Changed to include, removed redundant test, added test text to make sure we're using right template.

Had a no-newline error due to new IDE. Sorry.

No interdiff due to just one file with one line added to end.

Added a test without the theme installed

This looks great! Only nitpicks:)

1. +++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
   @@ -46,6 +46,14 @@ protected function setUp() {
      public function testViewsViewFieldsEscaping() {
   +
   

   Don't need this extra space after the function start.

2. +++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
   @@ -46,6 +46,14 @@ protected function setUp() {
   +    // Assert that there are no escaped '<'s
   ...
   +
   +    // Install theme to test with template system
   

   Periods at the end of sentences.

3. +++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
   @@ -62,5 +70,4 @@ public function testViewsViewFieldsEscaping() {
      }
   -
    }
   

   We actually want this space between the function and the ending class for coding standards.

1. +++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
   @@ -0,0 +1,73 @@
   +  public static $testViews = array('test_page_display');
   +
   +
   +  /**
   

   One new line here can be removed.

2. +++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-fields.html.twig
   @@ -0,0 +1,10 @@
   + * This exists because by default the one provided by Views is not used.
   

   The reason for this template is to override the theme function provided by views. And nit: don't need to capitalize views.

3. +++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-fields.html.twig
   @@ -0,0 +1,10 @@
   +{% include '@views/views-view-fields.html.twig' %}
   +May the force be with you!
   

   Wicked.

Handling the code issues

Glance over this one and let me know if I missed anything.

Changed a comment that I missed in the previous one.

Somehow I lost some words. I found them and they're back under control.

Thanks for fixing all the nits!

This looks great to me and the tests look great and if the views template changes then we don't have to duplicate them to this copied template.

htmlspecialchars() needs to be replaced with Html::escape()

Working on #72.

Addressing #72.

Oops, Html::escape() only takes one string as an argument...

+++ b/core/modules/views/views.theme.inc
@@ -219,12 +198,32 @@ function theme_views_view_fields($variables) {
+        $output .= $field->label_suffix;

Is it okay to not escape $field->label_suffix?

Html::escape() replacements look good

+++ b/core/modules/views/src/Tests/ViewsEscapingTest.php
@@ -0,0 +1,72 @@
+    // Assert that there are no escaped '<'s characters.
...
+    // Assert that there are no escaped '<'s characters.

superfluous s

@dawehner++

+++ b/core/modules/views/views.theme.inc
@@ -219,12 +198,32 @@ function theme_views_view_fields($variables) {
+      if (isset($field->label_suffix) && isset($field->label_suffix)) {
+        $output .= $field->label_suffix;
+      }

In fact what is label_suffix? It does not seem to ever exist.

Ah it is introduced by the patch.

+++ b/core/modules/views/views.theme.inc
@@ -149,26 +136,24 @@ function template_preprocess_views_view_fields(&$variables) {
         if ($object->handler->options['element_label_colon']) {

Why not just pass the boolean into the template? Setting the suffix outside means we need to escape. Whereas if the twig template and theme_views_view_fields handled it it'd be obvious it was safe.

+++ b/core/modules/views/views.theme.inc
@@ -219,12 +198,32 @@ function theme_views_view_fields($variables) {
+      if (isset($field->label_suffix) && isset($field->label_suffix)) {

I'm not seeing things am I? The isset is written twice. Was the second one suppose to be a !empty?

1. +++ b/core/modules/views/templates/views-view-fields.html.twig
   @@ -17,7 +17,8 @@
   + *   - has_label_colon: A boolean indicating whether to display a colon after
   

   For twig coding standards we don't use types https://www.drupal.org/node/1823416#datatypes

   Though I think we should update that rule for booleans because we do that type frequently. @Cottser thoughts?

2. +++ b/core/modules/views/templates/views-view-fields.html.twig
   @@ -40,9 +41,9 @@
   +      <{{ field.label_element }}{{ field.label_attributes }}>{{ field.label }}{% if field.has_label_colon %} :{% endif %}</{{ field.label_element }}>
   

   Minor clean-up but this could be nicer written as

   {{ field.has_label_colon ? ': ' }}

   But also the space is on the wrong side of the colon.

+++ b/core/modules/views/views.theme.inc
@@ -90,6 +90,9 @@
+      // Set up default value of the flag that indicates whether to display a
+      // colon after the label.
+      $object->has_label_colon = FALSE;

@@ -206,8 +209,8 @@
+      if (isset($field->has_label_colon) && $field->has_label_colon) {

Do you need the isset() now? I think not.

@joelpittet/#84: yeah the special naming, not sure. I thought we had written up those conventions somewhere but maybe not.

+++ b/core/modules/views/templates/views-view-fields.html.twig
@@ -11,13 +11,16 @@
+ *   - has_label_colon: A boolean indicating whether to display a colon after
+ *     the label.

Well, if has to be themer friendly, how about:

"Whether or not to display a colon after the label."

@Manuel Garcia it's not really about being "themer friendly" it's mostly because Twig treats arrays like objects in our their keys are accessed, from what I recall.

That could be a better change but I'd rather not hold up this blocker on a comment that isn't clearly documented.

Committed e76a96f and pushed to 8.0.x. Thanks!

Nice work

I tried this patch with Beta 14 and I get this error

"Fatal error: Call to undefined method Drupal\Component\Utility\Html::escape() in /core/modules/views/views.theme.inc on line 205"

Any ideas on what might be going on?

@sgalindo this will not work with beta14, you'll need to use HEAD instead

@sgalindo2388 if you want to play with it that method was added in #2550945: Add Html::escape() But you'd be better off with HEAD like @stefan.r suggested.

@stefan.r @joelpittet Thank you for the suggestions. using HEAD worked.