Views: HTML is escaped with aggregation enabled

Thanks for creating this issue @ExTexan. I reproduced the error in 8.2.x:

It seems to only happen if I change the aggregation settings on the field as well (in my case I added SUM or AVG)

Included the export of the config in the yaml file attached.

Here's a fix and some tests. The tests only patch should fail once or twice and the patch will resolve this.

+++ b/core/modules/views/tests/src/Kernel/Handler/FieldKernelTest.php
@@ -168,6 +168,55 @@ public function testRewrite() {
+    $this->assertSubString($output, '<p>1</p>');
...
+    $this->assertSubString($output, '<p>1</p>');

One could use $this->assertSubString() here.

I'm having a look at this @DrupalConDublin.

I am having at look at this @DrupalCon Dublin

I did a manual review.
I've been able to reproduce the error.
The patch applied fixed the error.

regards,
slowflyer

@slowflyer hey how did you reproduce this, we are currently trying to reproduce this error on 8.1 8.2 and 8.3 and can't.

Discussed with @timplunkett, @dawehner, @cilefen and @xjm. We agreed that this is a major issue because the views render pipeline should not cause escaping and is core views functionality.

ok so I have managed to reproduce this, apologies.. I had not set the aggregation type to SUM. If i do, I can reproduce this. So I am now testing the patch again to see if it fixes the issue.

I have managed to reproduce the issue now and have tested the patch views_html_is_escaped-2714045-4.patch against 8.2 and it seems to fix the issue for all Aggregation type settings.

This looks good to me

@anouschka42 thanks for reviewing this issue. It'd be great to see before and after screenshots of your manual testing. Also given the security implications of:

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1222,6 +1222,9 @@ public function renderText($alter) {
+      $value_is_safe = TRUE;

I think we need a views subsystem maintainer to review this patch before committing.

I created a simple view with content type name & count of Nid. I enabled aggregation in order to get total number of nid for a content type. The patch at #4 worked for me . Please see before & after screenshots.

Manually tested against 8.2. I adjusted the content view to rewrite the title of an article with a h1 tag and enabled aggregation.

See screenshots. without patch

with patch #4 applied

So patch #4 worked for me. Thanks :)

Well lets at least add a test to make sure it is safe. So something like this?

@Lendude
That's a great expansion

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1222,6 +1222,9 @@ public function renderText($alter) {
       $value = $this->renderAltered($alter, $tokens);
+      // Altered text is run through Xss::filterAdmin.
+      // @see \Drupal\views\Plugin\views\PluginBase::viewsTokenReplace().
+      $value_is_safe = TRUE;

One thing I am wondering: Should renderAltered mark it as safe somehow? Note: \Drupal\views\Plugin\views\field\FieldPluginBase::tokenizeValue has another call to it. Is there a similar kind of bug in there as well?

@dawehner so maybe something like this? Alters the return value of renderAltered so bit of an API change.... not sure if that's worth it.

Yeah the impact of changing what renderAltered does is way bigger then this issue, so lets not go there. Reuploading #21, I think that is sufficient for this.

@dawehner the call in \Drupal\views\Plugin\views\field\FieldPluginBase::tokenizeValue is:
$value = strip_tags($this->renderAltered($fake_item, $tokens));
pretty sure we don't have to worry about any HTML tags after that :)

I'd RTBC this if it wasn't so close to my original patch. Need someone from the views subsystem to review this I guess. Thank you @Lendude for improving the tests!

I like the test coverage applied here.

Committed and pushed 7c43cce to 8.3.x and c30e795 to 8.2.x. Thanks!

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1222,6 +1222,9 @@ public function renderText($alter) {
       $value = $this->renderAltered($alter, $tokens);

I've checked the output of $this->renderAltered() and I can't see anyway that the output is not run through XSS filtering. Since the input text is coming from the admin interface I think this is an appropriate tactic for making safe and 'administer views' is restricted access. I've updated the comment to reflect this.

diff --git a/core/modules/views/src/Plugin/views/field/FieldPluginBase.php b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
index e876ae9..c8e4d7a 100644
--- a/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1222,8 +1222,11 @@ public function renderText($alter) {
     if (!empty($alter['alter_text']) && $alter['text'] !== '') {
       $tokens = $this->getRenderTokens($alter);
       $value = $this->renderAltered($alter, $tokens);
-      // Altered text is run through Xss::filterAdmin.
-      // @see \Drupal\views\Plugin\views\PluginBase::viewsTokenReplace().
+      // $alter['text'] is entered through the views admin UI and will be safe
+      // because the output of $this->renderAltered() is run through
+      // Xss::filterAdmin().
+      // @see \Drupal\views\Plugin\views\PluginBase::viewsTokenReplace()
+      // @see \Drupal\Component\Utility\Xss::filterAdmin()
       $value_is_safe = TRUE;
     }
 

diff --git a/core/modules/views/tests/src/Kernel/Handler/FieldKernelTest.php b/core/modules/views/tests/src/Kernel/Handler/FieldKernelTest.php
index f2e9156..be8a4f1 100644
--- a/core/modules/views/tests/src/Kernel/Handler/FieldKernelTest.php
+++ b/core/modules/views/tests/src/Kernel/Handler/FieldKernelTest.php
@@ -168,9 +168,9 @@ public function testRewrite() {
     $this->assertSubString($output, $random_text);
   }
 
-/**
- * Tests rewriting of the output with HTML.
- */
+  /**
+   * Tests rewriting of the output with HTML.
+   */
   public function testRewriteHtmlWithTokens() {
     /** @var \Drupal\Core\Render\RendererInterface $renderer */
     $renderer = \Drupal::service('renderer');

Fixed on commit.