Add a test to ensure title callbacks are not vulnerable to XSS

Would it be okay to move to filter_xss_admin() instead?

I had a quick look at this and adding the below in hook_preprocess_page() goes straight to the page.

$variables['title'] = '<script>alert("xss");</script>';

Since this is the recommended way now drupal_set_title() has gone, I think a strip_tags by default would be preferred somewhere?

Unfortunately I'm not sure where it would go, drupal_prepare_page() is too early for preprocess functions and DefaultHtmlFragmentRenderer::render() doesn't feel right either.

I could be wrong but my thinking was that the change notice indicates that in D7 it was like:

<?php
// This would be escaped
drupal_set_title($my_title);
?>

So when people are converting their modules to D8, they'd come along and change it to

<?php
// This won't be escaped.
$vars['title'] = $my_title;
?>

And they'd inadvertently introduce a possible security issue.

I've seen contrib modules doing stuff like that when overriding the title, for example: http://drupalcode.org/project/h1_title_override.git/blob/HEAD:/h1_title_...

The change notice doesn't specifically mention preprocess but I wouldn't have thought that would stop some people attempting it this way when they're trying to override one off node/page titles.

I'd like to see title to be run through a strip tags, seems like a sensible default to me.

So maybe this?

I do not think this is the right place ;

Well, we can also do that directly in the template.

trying to trace through the flow in D7 (omg) I this it is always check_plain() by default, though there is the option to bypass.

I'm not clear why the patch in #11 would resolve the problem of an unsafe #title?

So, in this issue Tim argued that the return of the title resolver (and hence the title callback) is expected be safe and we added documentation that effect to the title resolver: #2267763: Page titles with %placeholders are broken when used as breadcrumbs

However, re-reading the arguments here I am now thinking that's not viable given the example here. Can we put the xss filter into the title resolver?

Actually - I take that back. the comment added to that interface is right - it's the title resolver implementation that's missing the step of sanitizing the title that comes from the title callback.

So, this should catch more, but doesn't really help with preprocess. Maybe we need to save the original title and filter it again if it changed?

@chx - ok, though we are then twice filtering anything that comes out of the title resolver.

It feels like a good part of the problem here is that in 7 we had the parameter for drupal_set_title() to we could know when to pass-through the value but it was sanitized by default.

If PHP had support for "tainted" strings this might be an easier problem to solve. Ot possible with TWIG auto-escape if that's enabled.

In the mean time, perhaps the title resolver or other case that wasn't a string passed though could wrap it in a simple object instance (possibly with just __toString()) and we can then treat all non-object titles as tainted and needing to be sanitized?

I don't see that Drupal 7 actually protects against an unsafe value being set during the page preprocess step. I see this as the process step:

function template_process_page(&$variables) {
  if (!isset($variables['breadcrumb'])) {
    // Build the breadcrumb last, so as to increase the chance of being able to
    // re-use the cache of an already rendered menu containing the active link
    // for the current page.
    // @see menu_tree_page_data()
    $variables['breadcrumb'] = theme('breadcrumb', array('breadcrumb' => drupal_get_breadcrumb()));
  }
  if (!isset($variables['title'])) {
    $variables['title'] = drupal_get_title();
  }
...
}

So we are only getting a safe value if nothing was set by a theme or module before.

however, dawehner points out that most code would use drupal_set_title() rather than setting the variable, so it was easy to do the right thing.

chx - yes, though we'd need to figure out how to not double-escape titles such as those already run through t() with placeholders

I think my patch in #24 at least addresses the current breadcumb hole and others from custom titles. Do you want me to pull that into a different issue? add a test?

For breadcrumbs I filled #2314599: Use title/url instead of l() for building breadcrumb

Reroll.

@chx
Is still issue actually still needed? Just curious.

Here is one. Not sure whether it is a bug that in case you hard-code the HTML inside the routing system, you can put everything in there. This is basically the same problem as t('

RTBC, as the route is not user supplied data, this should be fine.

Thanks for the test.

Committed 4300214 and pushed to 8.0.x. Thanks!