Views field rewrite for "Content: Path" incorrectly escapes HTML

+++ b/core/modules/node/src/Plugin/views/field/Path.php
@@ -68,7 +70,8 @@ public function query() {
-    return \Drupal::url('entity.node.canonical', ['node' => $nid], ['absolute' => $this->options['absolute']]);
+    $url = \Drupal::url('entity.node.canonical', ['node' => $nid], ['absolute' => $this->options['absolute']]);
+    return ViewsRenderPipelineMarkup::create(Xss::filterAdmin($url));
   }

IMHO the right thing to do would be to return a simple render eoement with just ['#markup']
ViewsRenderPipelineMarkup is really more about internals of views.

@dawehner,

++, patch changed according to above comment. and tested manually.

We need automated tests, sorry

+++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_path_plugin.yml
@@ -0,0 +1,183 @@
+uuid: fccbe0a6-be65-4789-8100-dacc4ddd3f31

Removed UUID from the added test view.

Since my change was very minor I'm going to RTBC this. The code change looks good and relies on Twig's auto-escaping -- always a good thing.

Thank you, @cilefen, for the tests!

Hiding test-only patches...

+1 for this fix. We applied similar strategies in other areas.

I was confused why this works reading the patch. What's getting escaped is not the canonical URL for the node, but the altered text - so I could not figure out why making the URL #markup would affect anything.

Reading through, it looks like it will be this hunk in FieldPluginBase::renderText():

    if ($value_is_safe) {
      $value = ViewsRenderPipelineMarkup::create($value);
    }

So the problem is that the 'safeness' of the rewritten text is based on the 'safeness' of the original text.

To me that looks like a much deeper issue with SafeMarkup and field rewrites, and at a minimum we should look at a follow-up + comments here explaining why it works like this.

From #17:

I was confused why this works reading the patch. What's getting escaped is not the canonical URL for the node, but the altered text - so I could not figure out why making the URL #markup would affect anything.

At core/modules/views/src/Plugin/views/field/FieldPluginBase.php:1140 the render() method is called and, if a render array is returned it's converted to a Render\Markup object. When FieldPluginBase::renderText is later called, $value_is_safe is (correctly) set to true as viewsTokenReplace() will return an XSS admin filtered string.

If Path::render() returns just a URL string, then $value_is_safe ends up as false. Perhaps incorrectly? We could mark the return from Path::render() as safe, but I thought we were trying to get away from that...

Though this does raise a point: renderText() does an awful lot of string replacements and whatnot. All of these need to be (at a minimum) Xss::filterAdmin()'ed and by packing them all in one long method with a comment at the top seems like a security issue waiting to happen.

But that cleanup should happen in a followup issue...

Possibly related issue.

Filed this followup: #2760143: Refactor views rewrite to be a) sane and b) secure

+++ b/core/modules/node/src/Tests/Views/PathPluginTest.php
@@ -0,0 +1,83 @@
+/**
+ * @file
+ * Contains \Drupal\node\Tests\Views\RowPluginTest.
+ */

Nitpick on commit: Remove the @file header.

Committed and pushed 8fb4440373a148a04f631090fc924ba74d44362e to 8.2.x and 61301b5 to 8.1.x. Thanks!

And yes views rewriting and safeness is hard and complex.

Fixes on commit:

diff --git a/core/modules/node/src/Tests/Views/PathPluginTest.php b/core/modules/node/src/Tests/Views/PathPluginTest.php
index e48cb87..1ee6272 100644
--- a/core/modules/node/src/Tests/Views/PathPluginTest.php
+++ b/core/modules/node/src/Tests/Views/PathPluginTest.php
@@ -1,14 +1,8 @@
 <?php
 
-/**
- * @file
- * Contains \Drupal\node\Tests\Views\RowPluginTest.
- */
-
 namespace Drupal\node\Tests\Views;
 
 use Drupal\views\Views;
-use Drupal\node\Entity\Node;
 
 /**
  * Tests the node row plugin.