Address SA-CONTRIB-2013-035 for views in D8

Rerolled that it can be committed.

Rerolled so that it will apply - file locations have changed since the last patch.

Not too sure how this should be done now:

 function theme_views_ui_view_info($variables) {
   $output = '';
-  $output .= '<h3 class="views-ui-view-title">' . $variables['title'] . "</h3>\n";
+  $output .= '<h3 class="views-ui-view-title">' . check_plain($variables['title']) . "</h3>\n";
   $output .= '<div class="views-ui-view-displays">' . $variables['displays'] . "</div>\n";
   return $output;
 }

Do we use the striptags function in twig. I've done <h3 class="views-ui-view-title views-table-filter-text-source">{{ title | striptags }}</h3> for now, but not sure if that's correct!

#4: drupal-1948418-4.patch queued for re-testing.

Instead of check_plain() we should better use String::checkPlain().

Thanks for the review, dawehner.

I've changed the patch to use String::checkPlain() and I've used it in template_preprocess_views_ui_view_info() to sanitise the title variable there, instead of in the twig template.

Forward-porting an SA sounds critical.

I wonder whether we actually still needs due the fact that auto-espacing landed in core.

From #14 I suspect @dawehner might be correct, but don't want the "Needs roll" tags to slow up the review process, on an already slow moving critical.

Fixed up 2 things.

Reroll of #18.

--- b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -989,7 +989,7 @@
       if (!empty($handler->options['relationship']) && !empty($relationships[$handler->options['relationship']])) {
         $options[$id] = '(' . $relationships[$handler->options['relationship']] . ') ' . $options[$id];
       }
-      $options[$id] = String::checkPlain($label);
+      $options[$id] = String::checkPlain($options[$id]);
     }
     return $options;
   }

I am not sure how this was supposed to work.

Tagging with D8 upgrade path, see issue summary of #2341575: [meta] Provide a beta to beta/rc upgrade path for why.

Patch needs a reroll, changing the status to needs work and adding the tag Needs reroll

Patch rerolled,

Thanks a ton @cilefen for mentoring me on this reroll.

Removing needs reroll tag.

So we need to:

1. Determine whether this is still reproducible in D8 HEAD.
2. If so, write an automated test that fails against HEAD and add it to the patch: https://drupal.org/contributor-tasks/write-tests
3. If not, still write a passing automated test that demonstrates that it's not a problem.

In #3, we've discussed elsewhere that we should actually be removing early checkPlain() calls, so my recommendation would be to add only the test in that case.

Build a view in standard profile that exposes the vulnerabilities. But D8 is atm not vulnerable because some of this is nuked by twig autoescape and also HandlerBase::adminLabel contains a call to String::checkPlain.

The test proves that Views UI is not vulnerable to SA-CONTRIB-2013-035. But it shows another bug, we double escape the admin labels in views ui.

The exported view was not valid because the Views Wizard added fields without plugin_id to the view.

+++ b/core/modules/views_ui/tests/modules/views_ui_test/config/install/views.view.sa_contrib_2013_035.yml
@@ -0,0 +1,215 @@
+      title: '<marquee>VIEWS TITLE</marquee>'
+  page_1:

More marquee!

The follow up is filled, so let's get this one in.

+++ b/core/modules/views_ui/src/Tests/XssTest.php
@@ -0,0 +1,34 @@
+    $this->assertRaw('<script>alert("foo");</script>, <marquee>test</marquee>', 'The view tag is properly escaped.');
...
+    $this->assertRaw('<marquee>test</marquee>', 'Field admin label is properly escaped.');
...
+    $this->assertRaw('[title] == <marquee>test</marquee>', 'Token label is properly escaped.');
+    $this->assertRaw('[title_1] == <script>alert("XSS")</script>', 'Token label is properly escaped.');

Have you considered to check for String::checkPlain('actual-readable-string') instead?

Committed 6efeaf7 and pushed to 8.0.x. Thanks!

@webflo, you missed something #2473907: Tests not being run by testbot due to missing summary line