Remove use of SafeMarkup::checkPlain() from adminSummary() and adminLabel() in views plugins

So Drupal\views_ui\Tests\XssTest in HEAD is hilarious - it is testing for double escaping :)

Removed a few Html::escape() - the whole point here is to rely on Twig to autoescape.

Needed a reroll.

@stefan.r can I get some steps to reproduce - thanks.

Adding a test for double escaping. With the steps to reproduce we can add to this.

Quick note: EntityRow.php no longer needs the SafeMarkup use statement.

1. +++ b/core/modules/views_ui/src/Tests/XssTest.php
   @@ -29,8 +29,22 @@ public function testViewsUi() {
   -    $this->assertRaw('[title] == <marquee>test</marquee>', 'Token label is properly escaped.');
   -    $this->assertRaw('[title_1] == <script>alert("XSS")</script>', 'Token label is properly escaped.');
   +    $this->assertEscaped('[title] == <marquee>test</marquee>', 'Token label is properly escaped.');
   +    $this->assertEscaped('[title_1] == <script>alert("XSS")</script>', 'Token label is properly escaped.');
   

   This is so much more readable!

2. +++ b/core/modules/views_ui/src/Tests/XssTest.php
   @@ -29,8 +29,22 @@ public function testViewsUi() {
   +  public function testNoDoubleEscaping() {
   +    $this->drupalGet('admin/structure/views');
   +    $this->assertNoEscaped('<');
   +
   +    $this->drupalGet('admin/structure/views/view/sa_contrib_2013_035');
   +    $this->assertNoEscaped('<');
   +
   +    $this->drupalGet('admin/structure/views/nojs/handler/sa_contrib_2013_035/page_1/header/area');
   +    $this->assertNoEscaped('<');
   

   Nice test!

Bugs that occur in HEAD and patch found whilst testing:

• #2567475: Views add field list label is being filter not escaped
• I've tested all the other plugins are we have double escaping bugs because of t()'d stuff stored in views caches - we need to do #2539110: TranslationWrapper should be used instead of t() in derivative defintions or #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list

Afaics this patch does make anything and definitely will make those problems easier to solve.

While trying to reproduce this I found an actual alert(), which is rare consider we escape things like gangbusters.

1. Steps, add a plain text single value "field_alert" with <script>alert('Title');</script> in every field possible on article.
2. Go to the admin content view and add that new field as a filter.
3. As soon as you click the checkbox, boom right in the kisser

Will try to figure out where that is coming from.

re: #15 it's the field label value, it's on click, and this patch doesn't change it (bug in head), needs a follow-up.

Follow-up added #2567673: Escaped markup with $.text() injected into a tag will cause XSS for #15

Address pre 1 of #13.

Rerolled as #2492839: Views replacement token bc layer allows for Twig template injection via arguments landed

@joelpittet I think this is covered by #2564283: Remove use of SafeMarkup::checkPlain() from adminSummary() and adminLabel() in views plugins

Well, then nevermind my last comment.

RTBC++ I'm in a recursive comment! in a recursive comment!

The double escaping mentioned in #10 looks to be mostly there before this patch. Going to need to track that down and add some double escaping tests for that. @stefan.r could you hook us up with your steps to reproduce that?

@joelpittet
Yeah I saw similar things as part of #2506479: Replace !placeholder with @placeholder for non URLs in t() in Views, except for t() output that is used as an attribute value

Oh wasn't following that one yet, thanks for the heads up

Committed/pushed to 8.0.x, thanks!