Site name from frontpage view is double-escaped on frontpage and in preview

+++ b/core/includes/theme.inc
@@ -1254,6 +1254,10 @@ function template_preprocess_html(&$variables) {
+    // Do an early render if the title is a render array.
+    $variables['page']['#title'] = (string) \Drupal::service('renderer')->render($variables['page']['#title']);

From #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter -- what was the reason for the string cast?

Is this a proper fix for the preview?

#2/ #3: Can we remove the cast?

Isn't the problem that the site name is already being escaped in system.tokens.inc

$site_name = $config->get('name');
$replacements[$original] = $sanitize ? SafeMarkup::checkPlain($site_name) : $site_name;

Then the escaped site name gets replaced in Welcome to [site:name] and that string gets replaced later on again because it's never been marked as safe.

For now I wrote some test coverage already to show the issue.

This patch points out some of the places where we try to touch the title... By no means final as it removes a whole bunch of XSS filtering (which it probably shouldn't), so I gues this will make a couple of tests fail as well.

As to the string cast and the early render of the welcome title, I suppose it was so we can get the head title in case it has tokens? Should a site title actually have markup anyway?

@olli yes, I suspect that cast should be gone because it marks the string as unsafe, so Twig escapes it again when it had already been filtered during the render call.

Maybe we could even somehow do away with the whole early render of the title as Twig can also deal with render arrays, just need to figure out what to do with the head title.

The views preview double escaping seems to be a separate problem, we try to filter the title multiple times before it arrives into the Twig template in ViewUI::renderPreview()

Here's #5 + #6. Manually tested it by setting site name to <script>alert('xss');</script> with front page title set to Welcome to <em>[site:name]</em> and it seems to work.

While this solves the double escaping on the frontpage view and the preview I wonder if this is the right fix? Just to echo @dawehner in #2531430: Page's <title> double escaped, in both cases the title has already been already escaped by the time we put it into the markup array, should it really be?

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -716,7 +716,11 @@ public function renderPreview($display_id, $args = array()) {
+                  '#markup' => $executable->getTitle(),

$executable->getTitle() has already been escaped here.

+++ b/core/includes/theme.inc
@@ -1237,7 +1237,7 @@ function template_preprocess_html(&$variables) {
+    $variables['page']['#title'] = \Drupal::service('renderer')->render($variables['page']['#title']);

$variables['page']['#title'] has already been escaped here.

This could also use a test for the double escaped title at admin/structure/views/view/frontpage/preview/page_1.

Reverted a change from #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter:

-        '#title' => SafeMarkup::xssFilter($this->view->getTitle(), Xss::getAdminTagList()),
+        '#title' => ['#markup' => $this->view->getTitle(), '#allowed_tags' => Xss::getHtmlTagList()],

Isn't the default still allowing admin tags so we can remove #allowed_tags?

#14

$executable->getTitle() has already been escaped here.

I don't see that. The replacement of [site:name] is escaped, but in #12 that <em>..</em> was not.

Did another test by setting title in views ui to Welcome to <del>[site:name]</del> <script>alert('xss2');</script> with site name <script>alert('xss');</script>. Now the getTitle() method returns a string Welcome to <del>&lt;script&gt;alert(&#039;xss&#039;);&lt;/script&gt;</del> alert('xss2');: <script>alert('xss');</script> is escaped, <script>alert('xss2');</script> is filtered, <del>[site:name]</del> is not escaped or filtered. So the title I see on front page, preview info area and preview title section looks like this: "Welcome to <script>alert('xss');</script> alert('xss2');".

#14

This could also use a test for the double escaped title at admin/structure/views/view/frontpage/preview/page_1.

I agree, and maybe use admin tags in that test.

EDIT: Replaced <big> with <del>.

In HEAD my manual test in #15 gives:
Front page:
Welcome to &lt;script&gt;alert(&#039;xss&#039;);&lt;/script&gt; alert('xss2');
Preview info:
Welcome to <del>&lt;script&gt;alert(&#039;xss&#039;);&lt;/script&gt;</del> alert('xss2');
Preview title section:
Welcome to <script>alert('xss');</script> alert('xss2');

@olli by escaped I meant filtered, I was testing with ampersands and the filtering escapes the & to & :(

So if my welcome message is Welcome to [site:name] &, that final ampersand will be set to & already, because we've already filtered the string before we send the markup array into the rendering system, which I don't know is necessary.

And as to the token replacement, why do we sanitize that, can't we just do that on output? Ie:

+++ b/core/modules/views/src/Plugin/views/area/Title.php
@@ -52,8 +52,7 @@ public function preRender(array $results) {
-      $value = $this->globalTokenReplace($this->options['title']);
-      $this->view->setTitle($this->sanitizeValue($value, 'xss_admin'));
+      $this->view->setTitle($this->globalTokenReplace($this->options['title'], ['sanitize' => FALSE]));

f

And as to the token replacement, why do we sanitize that, can't we just do that on output? Ie:

No, I don't think so. The default (sanitize=TRUE) is for rendering HTML so here we let system_tokens() know it needs to escape plain text data.

Yes the site name is clearly plain text so it should be escaped (on output), not filtered.

But then should the page title here be escaped or filtered, i.e. do we consider it plain text or HTML? In #2555931: Add #plain_text to escape text in render arrays there was a similar discussion, and it should actually probably be escaped / considered plain text as well as it's in line with what we do with node titles?

So then we could just have $this->view->getTitle() output the unsanitized title (including processed tokens) as '#plain_text', as opposed to as '#markup' -- and let the escaping happen there?

#19 Do you mean we'd drop support for html so something like Revisions for <em>%1</em> would not be possible in views title?

Do we do this in core? I've seen markup in titles on some drupal 7 sites (with sometimes unnecessary or double escaping elsewhere) but I'm unclear if we ever "officially" supported this?

Also, your use case seems clear and ability to use filters on all titles would be a nice to have, but do we need this on a Welcome message or a site name as well?

I know that a lot of admin pages for example uses <em> tags in there, so I think allowing HTML for <h1> is needed.
For <title> I think stripping all tags is the right approach.

OK so maybe we allow a limited set of tags in <h1> titles, ie <em> and <bdi>, and strip all tags for <title>?

OK so maybe we allow a limited set of tags in
titles, ie and , and strip all tags for ?

Well this is the hard question, what should people be able to use in titles? I strong maybe also something which would make sense?

1. +++ b/core/modules/views/src/Plugin/views/display/Page.php
   @@ -181,7 +181,7 @@ public function execute() {
   -        '#title' => ['#markup' => $this->view->getTitle(), '#allowed_tags' => Xss::getHtmlTagList()],
   +        '#title' => ['#markup' => $this->view->getTitle()],
   

   Any chance we could just pass the string to #title without the render array? That will filter out tags. or maybe #plain_text is better suited here.

2. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -716,7 +716,11 @@ public function renderPreview($display_id, $args = array()) {
   -              Xss::filterAdmin($executable->getTitle()),
   +              array(
   +                'data' => array(
   +                  '#markup' => $executable->getTitle(),
   +                ),
   +              ),
   

   same here, just plain text?

Looks like another issue solved this. Here's a screenshot using the steps to reproduce from the issue summary.