Problem/Motivation

Discovered escaping in the render pipeline with views-view-field.html.twig templates. #2348729-42: Convert theme_views_view_field to twig

https://qa.drupal.org/pifr/test/1157318

TaxonomyFieldAllTermsTest failing
CustomBooleanTest failing.

Proposed resolution

Resolve failures in

  1. CustomBooleanTest

Remaining tasks

Add twig template for views-view-field.html.twig to a test theme.
Apply fixes.

Steps to reproduce

  1. Setup a content View with fields.
  2. Add a status/publisehd as a boolean field.
  3. Setup custom values for the boolean display with markup ('<p>Yay</p>' and '<em>Nay</em>'
  4. #59 contains a txt file diff that converts the field plugin to Boolean instead of BooleanFormatter thanks to (@damiankloip)
  5. The values should display and in HEAD they are escaped.

User interface changes

API changes

Data model changes

Follow-up to #2348729: Convert theme_views_view_field to twig

Files: 
CommentFileSizeAuthor
#62 interdiff.txt1.78 KBjoelpittet
#62 views_render_pipeline-2560553-62.patch6.05 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,166 pass(es). View
#59 help-status-for-test.txt636 bytesjoelpittet
#54 views_render_pipeline-2560553-54.patch6.03 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,983 pass(es). View
#54 interdiff.txt5.69 KBjoelpittet
#54 views_render_pipeline-2560553-54-tests-only.patch4.83 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,939 pass(es), 2 fail(s), and 0 exception(s). View
#51 views_render_pipeline-2560553-51-tests-only.patch3.03 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,783 pass(es). View
#51 views_render_pipeline-2560553-51.patch4.19 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,754 pass(es). View
#51 interdiff.txt4.52 KBjoelpittet
#48 views_render_pipeline-2560553-48.patch2.12 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,757 pass(es), 2 fail(s), and 0 exception(s). View
#37 2560553-37.patch9.37 KBiMiksu
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,683 pass(es), 4 fail(s), and 0 exception(s). View
#35 2560553-35.patch8.72 KBiMiksu
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,663 pass(es), 6 fail(s), and 0 exception(s). View
#30 views_render_pipeline-2560553-30.patch9.31 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,350 pass(es), 4 fail(s), and 0 exception(s). View
#25 interdiff-2560553-17-25.txt682 bytesmdrummond
#25 2560553-17-views-render-pipeline.patch10.57 KBmdrummond
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,336 pass(es), 2 fail(s), and 0 exception(s). View
#4 views_render_pipeline-2560553-4.patch6.86 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 108,299 pass(es), 6 fail(s), and 1 exception(s). View
#2 views_render_pipeline-2560553-2--test-only.patch2.81 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 108,226 pass(es), 1 fail(s), and 0 exception(s). View

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

Status: Active » Needs review
FileSize
2.81 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 108,226 pass(es), 1 fail(s), and 0 exception(s). View

These tests probably don't test everything but it should give this a kickstart. This should fail.

Status: Needs review » Needs work

The last submitted patch, 2: views_render_pipeline-2560553-2--test-only.patch, failed testing.

joelpittet’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.86 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 108,299 pass(es), 6 fail(s), and 1 exception(s). View
6.89 KB

Having a hard time writing these tests on something that doesn't exist:S

Tried to get all the broken tests covered in one shot... much easier with actual twig template. Needs heavy overhaul or refactor, even if this does what I want.

Status: Needs review » Needs work

The last submitted patch, 4: views_render_pipeline-2560553-4.patch, failed testing.

subhojit777’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
8.11 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 109,659 pass(es), 4 fail(s), and 1 exception(s). View
subhojit777’s picture

This will reduce the number of fails (somewhat..)

  1. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyFieldAllTermsTest.php
    @@ -43,6 +59,27 @@ public function testViewsHandlerAllTermsField() {
    +    $this->drupalGet('test_page_display_200');
    

    This was returning 404

  2. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyFieldAllTermsTest.php
    @@ -22,7 +22,23 @@ class TaxonomyFieldAllTermsTest extends TaxonomyTestBase {
    +  public static $testViews = array('taxonomy_all_terms_test', 'test_page_display');
    

    So I added `test_page_display`

+++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyFieldAllTermsTest.php
@@ -43,6 +59,27 @@ public function testViewsHandlerAllTermsField() {
+    // @todo Fix this.
+    $this->testViewsHandlerAllTermsField();

Can you please explain why `@todo` here. Everything starting with `test` itself is a test and is executed by the system. Why are you executing them explicitly.

joelpittet’s picture

Oh I put that @todo to remind myself this is probably a bad approach to writing tests (aka hijacking another test that was failing).

I just did that to get a quick test that could show the problem, likely needs to be refactored.

@subhojit777 becareful not the fix things outside the scope of this issue. There may be another issue out there trying to fix that problem explicitly and will have to needlessly reroll, also makes this patch a bit harder to review. Though your change is minor. I have to catch myself when I start fixing all the things too!

Status: Needs review » Needs work

The last submitted patch, 6: views_render_pipeline-2560553-6.patch, failed testing.

subhojit777’s picture

Status: Needs work » Needs review
FileSize
4.54 KB
12.64 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,189 pass(es), 4 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 10: views_render_pipeline-2560553-10.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
10.57 KB

Thanks @subhojit777. I'm removing the theme function conversion from the patch in #10.

Likely the tests I wrote in there aren't quite right, so needs work on that for sure. I swear this issue will start to make more sense when the test are better:)

Status: Needs review » Needs work

The last submitted patch, 12: views_render_pipeline-2560553-12.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

Don't we seriously need some profiling on this issue?

  1. +++ b/core/modules/views/src/Plugin/views/field/PrerenderList.php
    @@ -78,17 +79,23 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +        $separator = Xss::filterAdmin($this->options['separator']);
    ...
    +          '#template' => '{{ items | safe_join(separator) }}',
    

    Oh so you have to ensure that the $separator is safe even if you use a template? This somehow feels counter intuitive, to be honest.

  2. +++ b/core/modules/views/src/Plugin/views/field/PrerenderList.php
    @@ -78,17 +79,23 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +
    +      return \Drupal::service('renderer')->renderRoot($build);
    

    Don't you want a renderPlain here?

joelpittet’s picture

@dawehner re #14.1 I copied that from something else that does the same thing in HEAD for consistency and fixes the problem. We don't have to ensure the separator is safe and actually I may just remove that from the other place as well in this patch. Thanks for noticing.

#14.2 Yes, but but kinda hoping I can fix that without early rendering the values. (pretty sure this can be done, just need to sit with it for a bit)

Thanks for the review.

joelpittet’s picture

Status: Needs review » Needs work

Anybody feel free to improve the tests or resolve notes in #14

mdrummond’s picture

Status: Needs work » Needs review
FileSize
10.57 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,917 pass(es), 2 fail(s), and 0 exception(s). View
519 bytes

This fixes point 2 in in #14. I'm not clear on what the next step is for point 1. Just remove the XSS:filterAdmin()? Is that needed because it's used both in the template and the context below that? I'm not quite sure what's going on there.

Status: Needs review » Needs work

The last submitted patch, 17: 2560553-17-views-render-pipeline.patch, failed testing.

The last submitted patch, 17: 2560553-17-views-render-pipeline.patch, failed testing.

dawehner’s picture

  1. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyFieldAllTermsTest.php
    @@ -43,6 +59,27 @@ public function testViewsHandlerAllTermsField() {
     
    +  public function testViewsHandlerAllTermsFieldWithTemplate() {
    

    Its a bit odd that this kind of test coverage lives in a file related with taxonomy term fields and not some other test more specific test class

  2. +++ b/core/modules/views/src/Plugin/views/field/PrerenderList.php
    @@ -78,17 +79,23 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +        $separator = Xss::filterAdmin($this->options['separator']);
    +        $build = [
    +          '#type' => 'inline_template',
    +          '#template' => '{{ items | safe_join(separator) }}',
    +          '#context' => ['separator' => $separator, 'items' => $items],
    

    This is the same as Field.php does, so this is fine.

  3. +++ b/core/modules/views/src/Plugin/views/field/PrerenderList.php
    @@ -78,17 +79,23 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +
    +      return \Drupal::service('renderer')->renderPlain($build);
    

    Are you sure we need renderPlain() and cannot just use render()?

mdrummond’s picture

3. I used renderPlain because you suggested it. If we use straight up render, don't we need to define a render context? I don't fully understand how that works, but thought I spotted that when I was working on the page title block.

joelpittet’s picture

re #20.1 yes super weird. I'm just trying to reproduce and fix the bugs we found in the other issue, we can make these tests better by moving them into a better location too.
20.2 ok
20.3 renderPlain() seems appropriate.

joelpittet’s picture

Title: Views render pipeline is double escaping field taxonomy links » Views render pipeline is double escaping field taxonomy links and CustomBooleanTest
Manuel Garcia’s picture

+++ b/core/modules/views/src/Plugin/views/field/PrerenderList.php
@@ -78,17 +79,23 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+        $build = array(

Can we get this in short array syntax?

+++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-field.html.twig
@@ -0,0 +1,11 @@
+Use posts instead of twigs to protect your llamas from escaping the field.

+1

mdrummond’s picture

Status: Needs work » Needs review
FileSize
10.57 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,336 pass(es), 2 fail(s), and 0 exception(s). View
682 bytes

This adds the short array syntax. That's the only feedback I felt I could take action on right now.

Time to review again, and if it's looking good, move forward?

Status: Needs review » Needs work

The last submitted patch, 25: 2560553-17-views-render-pipeline.patch, failed testing.

joelpittet’s picture

Need to debug and sort out what's up with CustomBooleanTest.

The last submitted patch, 25: 2560553-17-views-render-pipeline.patch, failed testing.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia

Rerolling

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
9.31 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,350 pass(es), 4 fail(s), and 0 exception(s). View

Not entirely sure of this re-roll... pls have a look -=)

Status: Needs review » Needs work

The last submitted patch, 30: views_render_pipeline-2560553-30.patch, failed testing.

The last submitted patch, 30: views_render_pipeline-2560553-30.patch, failed testing.

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
joelpittet’s picture

Assigned: Unassigned » joelpittet

I'm going to tackle this some more

iMiksu’s picture

FileSize
8.72 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,663 pass(es), 6 fail(s), and 0 exception(s). View

The #30 patch was failing to apply against latest HEAD:

patching file core/modules/views/src/Plugin/views/field/FieldPluginBase.php
Hunk #1 FAILED at 1308.

Here's a reroll.

lauriii’s picture

Status: Needs work » Needs review
iMiksu’s picture

FileSize
9.37 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,683 pass(es), 4 fail(s), and 0 exception(s). View

Please ignore #35, it was missing one of the files. Here's a with the previously missing file.

joelpittet’s picture

Assigned: joelpittet » Unassigned

Don't need to assign myself, I'm working on it with @iMiksu, he's helping me find a better place for these tests and more consistent structure to follow other views + twig render pipeline tests.

The last submitted patch, 35: 2560553-35.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 37: 2560553-37.patch, failed testing.

The last submitted patch, 35: 2560553-35.patch, failed testing.

The last submitted patch, 37: 2560553-37.patch, failed testing.

iMiksu’s picture

Assigned: Unassigned » iMiksu
joelpittet’s picture

iMiksu’s picture

Assigned: iMiksu » Unassigned

I've been digging around different views tests and found out that these escaping issues needs a bit of testing planning which I think somebody more experienced should take a look (I'm also quite new to SafeMarkup). There is a test \Drupal\views\Tests\ViewsEscapingTest which could be abstracted.

I also think that this may be more child issue of #2297711: Fix HTML escaping due to Twig autoescape rather than #2348729: Convert theme_views_view_field to twig, should we relink this?

I'm gonna drop on this and leave this to somebody who knows lots of about testing and lots of about SafeMarkup and let myself move on to work on another issue :)

joelpittet’s picture

Issue summary: View changes

HandlerFieldRoleTest is not a problem

joelpittet’s picture

Title: Views render pipeline is double escaping field taxonomy links and CustomBooleanTest » Views render pipeline is escaping CustomBooleanTest
Issue summary: View changes
Status: Needs work » Needs review

Changing the scope of this issue to only the CustomBooleanTest because it's the only failing issue in the other issue now.

Also not "double escaping" just unexpected HTML single escaping.

joelpittet’s picture

FileSize
2.12 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,757 pass(es), 2 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 48: views_render_pipeline-2560553-48.patch, failed testing.

The last submitted patch, 48: views_render_pipeline-2560553-48.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
4.52 KB
4.19 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,754 pass(es). View
3.03 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,783 pass(es). View
iMiksu’s picture

Shouldn't the tests-only patch fail?

joelpittet’s picture

@iMiksu yes it should:)

joelpittet’s picture

FileSize
4.83 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,939 pass(es), 2 fail(s), and 0 exception(s). View
5.69 KB
6.03 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,983 pass(es). View

Was having troubles with the views preview using the theme.

Went with ViewsRenderPipelineSafeString to make this more discoverable.

The last submitted patch, 54: views_render_pipeline-2560553-54-tests-only.patch, failed testing.

The last submitted patch, 54: views_render_pipeline-2560553-54-tests-only.patch, failed testing.

joelpittet’s picture

Issue summary: View changes

joelpittet’s picture

Issue summary: View changes
FileSize
636 bytes

Found a way to test this manually.

Attached is patch make sure the boolean field uses Boolean class instead of BooleanFormatter.
core/modules/node/src/NodeViewsData.php

mr.baileys’s picture

  1. I manually tested the patch, ensuring that a) markup is not escaped (as expected) and b) markup is still admin filtered (as expected).
  2. Reviewed the code, and it looks sane to me, just 2 small issues (but neither should hold up this patch I think):
    1. +++ b/core/modules/views/src/Plugin/views/field/Boolean.php
      @@ -118,7 +119,8 @@ public function render(ResultRow $values) {
      +      $custom_value = $value ? UtilityXss::filterAdmin($this->options['type_custom_true']) : UtilityXss::filterAdmin($this->options['type_custom_false']);
      +      return ViewsRenderPipelineSafeString::create($custom_value);
      

      Nitpick: I would defer UtilityXss::filterAdmin() until the return statement (to avoid having it twice, since we are doing it for $custom_value regardless of value).

    2. +++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-field.html.twig
      @@ -0,0 +1,11 @@
      + * The reason for this template is to override the theme function provided by
      + * views.
      

      Should this explain *why* we want to override the theme function for this test (to force usage of twig auto-escaping on rendering the field values, so we can ensure there is no double-escaping going on), or is this not necessary?

mr.baileys’s picture

Issue tags: -Needs manual testing

Removing the manual testing tag since manual testing was done in #60.

joelpittet’s picture

FileSize
6.05 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,166 pass(es). View
1.78 KB

Thanks for the review I've updated those two points @mr.baileys. Slightly more generic because it can be used for things other than escaping and it's to test the template version of the same field template.

mr.baileys’s picture

Status: Needs review » Reviewed & tested by the community

Manual testing and review completed, all issues addressed, RTBC.

xjm’s picture

Issue tags: +SafeMarkup
joelpittet’s picture

Issue tags: +blocker

This is blocking a theme function conversion.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f26b065 and pushed to 8.0.x. Thanks!

  • alexpott committed f26b065 on 8.0.x
    Issue #2560553 by joelpittet, subhojit777, mdrummond, iMiksu, Manuel...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.