Fix common HTML escaped render #key values due to Twig autoescape

I think this is the solution to #2273925: Ensure #markup is XSS escaped in Renderer::doRender() as well: add a #safe_markup which can be used to add anything and filter_xss #markup as well.

+1 for the five keys in the OP

+ 1 for this issue fixing. :)

@joelpittet,
After applying your patch one thing caught my eye. There is some dots in the manage field pages. See the attached screenshot.
I had a little discussion with @chx in IRC and he as of that discussion, '#description', '#field_prefix', '#field_suffix', '#prefix', '#suffix' elements should expect string always. But seeing at the error in Drupal\locale\Tests\LocaleExportTest file as strlen() expects parameter 1 to be string, array given, there may be any places present where these attributes have array values instead string.

If I am able to backtrace the error then I'll let you know.

Although we could whitelist button for these uses cases, https://html5sec.org/ says that button w/ form attribute and one of onformchange or formaction attributes can be used to sniff/snoop on form-submissions via XSS. So I think we shouldn't do that.

So I think the option is fix this mess and use a for-real theme hook, this concatenation feels puke.
And a button in #description seems wrong - for https://html5sec.org/ reasons alone.

OK, OK but what are those arrays?

The arrays are from modules/locale/src/Form/ImportForm.php::BuildForm() (the #description key).

    $validators = array(
      'file_validate_extensions' => array('po'),
      'file_validate_size' => array(file_upload_max_size()),
    );
    $form['file'] = array(
      '#type' => 'file',
      '#title' => $this->t('Translation file'),
      '#description' => array(
        '#theme' => 'file_upload_help',
        '#description' => $this->t('A Gettext Portable Object file.'),
        '#upload_validators' => $validators,
      ),
      '#size' => 50,
      '#upload_validators' => $validators,
      '#attributes' => array('class' => array('file-import-input')),
    );

I found this using grep. I think this is the only array, since TwigTransTest is also loading the locale import form.

So vote 1 for ignoring arrays, i.e. wrap the code in a check that it's scalar first.

That is absolutely horrible. But yes, I vote so too.

@joelpittet & @chx,
By checking with is_scalar and is_null (since is_scalar don't check for null values) won't solve the purpose for those markups which uses #theme to style their output. Since we can't use theme_*() any more #2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system we need to find a proper way to make the theme implementation to supply string mainly for markup elements.
So do we have to call drupal_render() again?

$build = array(
  '#theme' => 'file_upload_help',
  '#description' => $this->t('A Gettext Portable Object file.'),
  '#upload_validators' => $validators,
);
$form['file'] = array(
  '#type' => 'file',
  '#title' => $this->t('Translation file'),
  '#description' => drupal_render($build),
  '#size' => 50,
  '#upload_validators' => $validators,
  '#attributes' => array('class' => array('file-import-input')),
);

Regarding cross site scripting, is it possible to check the values inside any attributes of a HTML tag? Like,

<form id="test"></form><button form="test" formaction="javascript:alert(1)">X</button>

If any malicious attribute value is found then strip it off? So the above becomes,
<form id="test"></form><button form="test" formaction="">X</button>
OR
<form id="test"></form><button form="test">X</button>

It's a big list of my queries. :-)
Please let me know your thoughts.
Thanks!

If you use a render array then that's secure. Solving #markup is perhaps not in scope for this, so I just vote in a !is_array check for the sake of simplicity.

> If any malicious attribute value is found then strip it off

Yes, Xss::filterAdmin removes any attribute with malicious values.

@chx, okay so I'll change the code in modules/locale/src/Form/ImportForm.php::BuildForm() and check if any test cases fail. Also, in the patch file I'll change and allow "button" tag in Xss::filterAdmin().

Just to be sure I will also check with the string <form id="test"></form><button form="test" formaction="javascript:alert(1)">X</button> for any possible XSS attack.

Will let you know about my observations.
Thanks!

@chx & @joelpittet,

Here is another patch with checks on array values and also changed the #theme declaration in modules/locale/src/Form/ImportForm.php::BuildForm(). This one is not finished though and I've checked with some tests and it failed. Seems that we have a long way to go. Making it for review so you all can have a look where it failed.

Please review and provide comments.

Thanks @joelpittet.
Settings arrays to null is what I thought at that moment. May be someone can come up with a better idea. Also, I thought of one thing. Instead of using $elements[$key] = Xss::filterAdmin($elements[$key]); we could use $elements[$key] = Xss::filter($string, $adminExtendedTags);. Where, $adminExtendedTags will consist of the default Xss::$adminTags variable and added with some more HTML tags like button.

Then I will check for XSS as per the site https://html5sec.org/.

I'll check these tomorrow. In the mean time if you have any suggestions please let me know.

Happy to work on this issue. :-)

Guys,
I think #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render fixes the issues with the keys mentioned in the issue. So do we need this issue anymore? and also #2309929: HTML double-escaping in field forms is fixed I checked with the last committed code.

Do we have tests for these to make sure? Edit: just a few new asserts I hope.

@Wim Leers any idea why #markup and #description by-passes autoescape now? is that secure?

It does not works anymore after reinstall

That's fine, I guess, although #prefix and #suffix still need to be escaped in drupal_render.

Let's drunkroll!

Go bot, go!!

I tested the patch provided by lauriii on comment #42 and it worked fine for me

reviewing

@laurii thanks so much for fixing this! I wonder though, is the field ui and rdf fixes belong to this issue ...?

@chx: I guess we have to fix them to make tests pass

I'm not surre if this belongs to this issue, but on the install screen there is one message double escaped. I don't know, where this issue is coming from, since only one message is double escaped not all of them.

#2317281: Double escaping of install errors

@killerpoke that belongs to another issue as @laurii points out. Please remove the embedding of such a huge image it makes an already long issue too long. Thanks.

As much as I love the idea and the plan laid out in the issue summary, unfortunately the patch has some issues:

a) #prefix, #suffix can still be changed in #pre_render, so the check should be done before its used (e.g. directly above the concating of that strings.
b) #description and possibly (?) #field_prefix, _suffix are Form API properties, so should also be run through filterXssAdmin shortly before usage.

Thanks for the hard work @all!

This is awesome!

Okay, so here is the point:

#description is used somewhere, where it is automatically escaped.

#description is a FAPI element, which has nothing to do with drupal_render, so we need to fix it:

a) near where #description is actually output, to ensure no code can change it afterwards again and break it again
b) somewhere within Form API - as that is where the property is defined and documented

Is that more clear now?

Thanks for working on this very important issue!

Now we set markup as safe when we've XSS filtered them. Had to fix some elements in render tests since Xss::filterAdmin removes all unknown elements such as <foo>. I added filter to doBuildForm() which should be pretty close to printing.

1. +++ b/core/includes/common.inc
   @@ -2777,6 +2778,22 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
   +  $markup_keys = array(
   +    '#description',
   +    '#field_prefix',
   +    '#field_suffix',
   

   Remove those three here and move down to below (commented there, where #prefix / #suffix is used.

2. +++ b/core/includes/common.inc
   @@ -2777,6 +2778,22 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
   +    if (!empty($elements[$key]) && is_scalar($elements[$key]) && !SafeMarkup::isSafe($elements[$key])) {
   +      $elements[$key] = SafeMarkup::set(Xss::filterAdmin($elements[$key]));
   +    }
   

   Using SafeMarkup::set is wrong here. Xss::filterAdmin already returns safe markup ...

3. +++ b/core/includes/common.inc
   @@ -2896,6 +2913,7 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
      // #cache is disabled, #cache is enabled, there is a cache hit or miss.
      $prefix = isset($elements['#prefix']) ? $elements['#prefix'] : '';
      $suffix = isset($elements['#suffix']) ? $elements['#suffix'] : '';
   

   Here is where we should add the code, but I think I would prefer UX like:

   // Performance check - critical path
   if (!empty($elements[$key])) {
   $elements[$key] = SafeMarkup::checkAdminXss($elements[$key]);
   }

   That would be way better UX in my opinion and also then do the remaining if checks etc.

4. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -679,6 +681,20 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
   +      '#prefix',
   +      '#suffix',
   

   Remove those two and use the nicer UX function on SafeMarkup class as its a common function / we might people to use with their own admin FAPI properties.

5. +++ b/core/modules/system/src/Tests/Common/RenderTest.php
   @@ -869,11 +869,11 @@ function testDrupalRenderChildElementRenderCachePlaceholder() {
   -        '#prefix' => '<foo>',
   -        '#suffix' => '</foo>'
   +        '#prefix' => '<pre>',
   +        '#suffix' => '</pre>'
   

   This change looks good to me.

Fixed most of the points I guess. Tests are still most likely failing, just making sure that more stuff isnt breaking.

Thanks for working on this but look

   $suffix = isset($elements['#suffix']) ? $elements['#suffix'] : '';
      $elements[$key] = SafeMarkup::checkAdminXss($elements[$key]);
   $elements['#markup'] = $prefix . $elements['#children'] . $suffix;

You are running checkAdminXss after #prefix / #suffix is already used. Which also means that #prefix / #suffix is not tested :)

You could do:

   $prefix = !empty($elements['#prefix']) ? SafeMarkup::checkAdminXss($elements['#prefix']) : '';
   $suffix = !empty($elements['#suffix']) ? SafeMarkup::checkAdminXss($elements['#suffix']) : '';

and be done with it.

Dont know what I was thinking..

1. +++ b/core/includes/common.inc
   @@ -20,6 +20,7 @@
    use Drupal\Component\Utility\UrlHelper;
   +use Drupal\Component\Utility\Xss;
    use Drupal\Core\Cache\Cache;
   

   This declaration is no longer used.

2. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -137,6 +137,22 @@ public static function escape($string) {
   +  public static function checkAdminXss($string) {
   +    return static::isSafe($string) ? $string : Xss::filterAdmin($string);
   +  }
   

   This however needs to use:

   use XSS?

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -10,8 +10,10 @@
    use Drupal\Component\Utility\String;
    use Drupal\Component\Utility\UrlHelper;
   +use Drupal\Component\Utility\Xss;
    use Drupal\Core\Access\CsrfTokenGenerator;
    use Drupal\Core\DependencyInjection\ClassResolverInterface;
   

   This XSS can be removed now.

If that is the case, we are still doing it wrong. We should then probably do it in preprocess. The latest possible point before it's rendered that is always executed.

The test is failing continuously due to a JavaScript test in /core/modules/system/src/Tests/Common/JavaScriptTest.php. This file contains a test case testBrowserConditionalComments where Conditional IE based script is tested. With the patch no applied the test generates,

<!--[if lte IE 8]>
<script src="http://drupal8.local:8083/core/misc/collapse.js?0"></script>
<![endif]-->

<!--[if !IE]><!-->
<script>
<!--//--><![CDATA[//><!--
jQuery(function () { });
//--><!]]>
</script>
<!--<![endif]-->

But with the patch applied the test generates,

<script src="http://drupal8.local:8083/core/misc/collapse.js?0"></script>

<script>
<!--//--><![CDATA[//><!--
jQuery(function () { });
//--><!]]>
</script>

So it's clear that in common.inc SafeMarkup::checkAdminXss($elements['#prefix']) and SafeMarkup::checkAdminXss($elements['#suffix']) are stripping the tags. But this feature of conditional JS or CSS add is a feature that is necessary.
We can mitigate this by checking why <!-- & <! is not getting passed via Xss::filter() method.

Any thoughts?

Nope, this is correct.

Whatever is setting this script tag, need to run it via SafeMarkup::set(), because this is a case of:

Yes, I want to output script tags and _yes_ I know what I am doing.

The autoescape is exactly for cases like this to prevent them - so it needs to be specifically marked.

Ah, I re-read the comment and it seems we even allow certain script tags, but we don't allow comments ...

Interesting ...

I overall think its an edge case, where calling SafeMarkup::set is actually okay, but I will need to take a look at the test itself.

So there is a work-around using #inline_template and even using JS escaping strategy if needed.

I think that test would be a good precedent to show how to use #inline_template.

@Fabianx,
So you are saying that we need to change the test case?

function testBrowserConditionalComments() {
    $default_query_string = $this->container->get('state')->get('system.css_js_query_string') ?: '0';

    $attached['#attached']['library'][] = 'core/drupal';
    $attached['#attached']['js']['core/misc/collapse.js'] = array(
      'browsers' => array('IE' => 'lte IE 8', '!IE' => FALSE),
    );
    $attached['#attached']['js'][] = array(
      'type' => 'inline',
      'data' => 'jQuery(function () { });',
      'browsers' => array('IE' => FALSE),
    );
    $this->render($attached);
    $javascript = drupal_get_js();

    $expected_1 = "<!--[if lte IE 8]>\n" . '<script src="' . file_create_url('core/misc/collapse.js') . '?' . $default_query_string . '"></script>' . "\n<![endif]-->";
    $expected_2 = "<!--[if !IE]><!-->\n" . '<script>' . "\n<!--//--><![CDATA[//><!--\n" . 'jQuery(function () { });' . "\n//--><!]]>\n" . '</script>' . "\n<!--<![endif]-->";

    $this->assertTrue(strpos($javascript, $expected_1) > 0, 'Rendered JavaScript within downlevel-hidden conditional comments.');
    $this->assertTrue(strpos($javascript, $expected_2) > 0, 'Rendered JavaScript within downlevel-revealed conditional comments.');
  }

As per the code, $attached['#attached']['js'] is attaching theJavaScript and then the render() function calls drupal_render() where the tags are getting stripped off due to the application of SafeMarkup::checkAdminXss()

I do agree with you @Fabianx as to use inline_template.

Also, what do you think if we move this bit of code,

// Filtering keys which are expected to contain HTML.
    $markup_keys = array(
      '#description',
      '#field_prefix',
      '#field_suffix',
    );
    foreach ($markup_keys as $key) {
      if (!empty($element[$key]) && is_scalar($element[$key])) {
        $element[$key] = SafeMarkup::checkAdminXss($element[$key]);
      }
    }

in $element['#after_build']. Because in doBuildForm() method if we apply this to each element then there is (I think) no chance any one can alter it afterwards. So the logic is this #after_build callback will be defined after all modules defines there #after_build functions.

I created an interdiff that should fix the problem, it is attached.

@Fabianx, thanks. Trying once again.

#71 We don't need to fix the test, but the #pre_render.

We can do two things:

- Keep the suffix / prefix approach and use SafeMarkup::set()

OR

Do something like:

if (X) {
$new_element = array(
  '#type' => 'inline_template',
  '#inline_template' => "\n<!--[if {{ expression }}]>\n{{ element }}<![endif]-->\n",
  '#context' => array(
    'expression' => $expression,
    'element' => $element,
  ),
);
} else {
  $new_element = array(...);
}

return $new_element;

Regarding the location where the form is changed:

Yes, this should be in #after_build or even afterwards.

Latest possible point before those properties are output.

@Fabianx, I applied your interdiff and "JAVASCRIPTTEST" ran fine. Just changing the location of the SafeMarkup::checkAdminXss to #after_build.

Uploading one patch. Needs manual and automated review both.

I'll check this one tonight. Again failed. :-(

Maybe lets try that in template_preprocess_form_element() or whatever is called for all FAPI elements again?

If its not possible in #after_build might need to do it in doBuild() - which should be fine, too.

@Fabianx,
Yes I think this time it "may" pass. Yup its possible in #after_build. Just the function needed to be a static method rather than a private or protected one. Uploading a patch with your interdiff added.

Needs automated and manual review both.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -791,7 +792,8 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
-
+    // Add after_build attribute to each element just to check safe markup.
+    $element['#after_build'][] = array(get_class($this), 'formSafeCheck');

You will need to change that, too:

FormBuilder::formSafeCheck

@Fabianx, yes you are right. I keep forgetting it's OOP now. Anyway, I'll add self::formSafeCheck

Created a new one with @Fabianx's inputs. Guys please review it.

Thanks!

Thanks much for moving this forward rapidly!

Very interesting approach. At first I thought I don't like it because of #after_build but since you want to fire some code just after #after_build it actually does make sense :) However,

    $element['#after_build'][] = array(get_class($this), 'self::formSafeCheck');
     // The #after_build flag allows any piece of a form to be altered
     // after normal input parsing has been completed.
     if (isset($element['#after_build']) && !isset($element['#after_build_done'])) {

Thinking aloud about this code, it seems to me that -- whether realistic or not but I suspect yes -- there's a chance of recursion/infinite loop here based on the presence of #after_build_done. After this code the isset($element['#after_build']) is always true. All put together I suggest:

     // The #after_build flag allows any piece of a form to be altered
     // after normal input parsing has been completed.
     if (!isset($element['#after_build_done'])) {
      $element['#after_build'][] = array(get_class($this), 'self::formSafeCheck');

this change. Also, array(get_class($this), 'self::formSafeCheck'); I am surprised this even works. o_O Behold this: http://3v4l.org/CfFch vs http://3v4l.org/k6KTB Very nice PHPWTF, thanks. http://www.phpwtf.org/variable-function-calls written here.

The proper syntax is array($this, 'formSafeCheck'); simply, please use it.

@chx,
Thanks for your feedback. One thing though as per comment http://php.net/manual/en/function.call-user-func-array.php#93744 in PHP (5.3 >=) we can use call_user_func_array('parent::func', $args);

What are your thoughts on this?

Look http://php.net/manual/en/language.types.callable.php here

A method of an instantiated object is passed as an array containing an object at index 0 and the method name at index 1.

vs

Static class methods can also be passed without instantiating an object of that class by passing the class name instead of an object at index 0. As of PHP 5.2.3, it is also possible to pass 'ClassName::methodName'.

but we have a very fine instantiated object: $this and formSafeCheck is not static.

Should we just call this statically after invoking all other after builds?

Hm, looking at that again, I think the best place is to do invoke the method right before returning the $element at the end of the function.

Because this is usually before its rendered.

@Fabianx, just before the return we can call this section. But what do you think to introduce a new attribute kind of "#secure_build" for each field element and also for the form itself. Idea is the same as "#after_build". So other modules can assign their own callback methods to implement security based on their different needs? - Just a thought.

But it this is not desired then instead of calling in "#after_build" we can also have the code block as you said just before the return.

I don't really think we need to solve the #secure_build in this issue, after all there is methods how to mark the #description keys, etc. secure.

This is mainly for the most used properties, a convenience thing.

@Fabianx, I do agree. As I said before it's just a thought.

I do like @chx's idea to place the #after_build in a condition. But then again the idea is, there should be at least one #after_build and it's callback is self::formSafeCheck.
So even if we add the isset check, for each element the code will still go to the logic where

if (isset($element['#after_build']) && !isset($element['#after_build_done'])) {

is written. So if there is any recursive infinite loop how does this condition based logic prevents it?
I'm a bit confused here. :-(

Then @Fabianx, with your thought, placing the check in just before the return $element; section will solve the problem surely but don't we have any other way(s)?

There is no point in adding an #after_build that always runs, or is there?

And no, putting it before the return is the right way, lets not overcomplicate this, please.

And we can run the filterXSS again and again and probably should do so.

This is for cases where we rebuild the form.

@Fabianx, So lets put this bit of code just before the return statement. So do you think its a good idea to keep

// Filtering keys which are expected to contain HTML.
    $markup_keys = array(
      '#description',
      '#field_prefix',
      '#field_suffix',
    );
    foreach ($markup_keys as $key) {
      if (!empty($element[$key]) && is_scalar($element[$key])) {
        $element[$key] = SafeMarkup::checkAdminXss($element[$key]);
      }
    }

this bit of code in a method or just as is? Also if we make this as method what if to define a protected variable to store $markup_keys instead of local variables and use it as $this->markup_keys?

It is definitely cleaner to keep this is in a protected method.

I don't think we need to move the keys to a variable for now as its unlikely someone will override this class.

@Fabianx, yup very less chance of overriding the class methods or variables. So lets keep it simple and will use the method that I created "formSafeCheck". I'll post a patch tomorrow for review.

Thanks!

New patch to review, based on @Fabianx suggestions.

I can confirm that #97 patch fixes the issues I have found in Google Analytics module. As I'm not deep enough in this patch I stay away from setting it to RTBC.

Just a thought, shouldn't #title not be part of the list?

@hass #2348851: Regression: Allow HTML tags inside detail summary has a use case for title but it's so rare vs the title being so commonly used it's better not to slow this whole thing down even further. Do you know of any other cases?

What I tried to use from time to time SUP and SUB in title. I also have an open bug in views for table header about this.

I agree with #101, we definitely need to call SafeMarkup::checkAdminXss() on the prefix and suffix in rdf module before calling SafeMarkup::set() - similar to how we solved it in the #pre_render case for the scripts.

@joelpittet & @Fabianx, I haven't checked that part very minutely before. But I think I've to check that part too. I'll have an update on this soon.

Thanks for the feedbacks :-)

Just a quick note. I tried to use <sup>1</sup> in a #title and this works well. There is nothing to do here about #title.

Addressed comments by @joelpittet & @Fabianx.

Thanks!

-      $variables['content']['comment_body']['#prefix'] = '';
+      $prefix = '';
+    }
+    else {
+      $prefix = SafeMarkup::checkAdminXss($variables['content']['comment_body']['#prefix']);
     }

I hate to play pedantic code style, but this can be written easier as:

$prefix='';
if (!empty(...)) {
  $prefix = SafeMarkup::checkAdminXss($variables['content']['comment_body']['#prefix']);
}

Could we please fix this?

Besides that, this is RTBC. Thanks!

@Fabianx, indeed this can be fixed. I tried to change the minimal of the codes in the module. However, this can easily be done. :-)

Thanks!

New patch.

RTBC if tests pass. Thanks!

I think we can remove the SafeMarkup::set() calls from editor.admin.inc and (maybe) Drupal\Core\Render\Element\MachineName

Done.

Opsie.

Re-reviewed: Back to RTBC - provided tests still pass.

These SafeMarkup:sets cannot be removed since they are not going through the form apis sanitation.

Well then we need to think on how the patch is architected because those properties are supposed to be safe from double escape?

@chx,
Just to get an idea,
Isn't that was the initial idea of this issue to check whether other modules or core sends proper data in #prefix, #suffix etc. before sending it to twig and to remove the double escape. In that case why the modules need to send data wrapped in SafeMarkup::set() method? What if the system checks for the data and then decides to send it via SafeMarkup::set() or checkAdminXss()?

@lauriii, as you said,

These SafeMarkup:sets cannot be removed since they are not going through the form apis sanitation.

Can't we just let the modules/core forms send what ever data (ideal case they will sanitize the data) possible and check in our form or theme layer?

#129 You are right.

It was me who derailed this issue and pushed to move that to Form API.

We will need to track the execution stack from drupal_render($element) down to the actual twig template, to see where we can be inject if not at FAPI level.

Edit:

It might be that the place to add this is before the actual _theme() call in drupal_render(), but it might also be there is something else the Form API elements have in common.

@Fabianx, then I think we need to re-structure this one and have to work from scratch. Rather than taking the FAPI approach I'd suggest to work this on the theme layer. What if to work on #post_render callback?

The last level of code execution on theme level - Any suggestions? @chx, @lauriii, @alexpott, @joelpittet any thoughts?

Basic changes that are expected,
At the end execution phase of form render or theme render, need to check the values in the #key elements are safe or not. If not safe then use XSS check else if safe then use SafeMarkup.

Please feel free to modify the implementation logic mentioned above. And any ideas or thoughts are welcome.

EDIT #1
The issue #2273925: Ensure #markup is XSS escaped in Renderer::doRender() is also needs some attention (I think). I came across this while going via drupal_render() function.

EDIT #2
There are few more findings about drupal_render() & _theme()functions.
@Fabianx, I tried to drill down drupal_render() function and the following points can be assumed,

1. Each form element will have #theme_wrappers.
2. #render_children is only to be used internally and only set via _theme() function so no other module will add it in FAPI.

Being said that if we can implement the sanitization of form elements in _theme() function just before,

// Generate the output using either a function or a template.
  $output = '';

where it is decided where the theme get created, via functions or via twig template then, it may solve one section of this issue. But if #theme_wrappers are supplied by any modules this may fail as to prevent infinite loop drupal_render() takes some measures.

if (isset($elements['#theme_wrappers']) && !isset($elements['#render_children'])) {

So guys, please share your thoughts and correct me if I am in a wrong direction.

Thanks!

A patch based on my thoughts in comment #132. Probably it will fail but I want to be sure that which sections fail.

Future plans-
After fixing failing sections and addressing review comments if this one is feasible enough then will add test cases that will validate this one.

It seems "#post_render_cache" form attribute has some issues with this patch. :-)
Any ideas please..

Based on the discussion with @chx and @Fabianx, this patch is based on patch #116.

Review review :-)

@chx, thanks for the fix :-)

So any reviews guys? Or can we make this RTBC?

Now we are almost back at the original patch, but 100 comments further, we are finally back to RTBC :).

But now we can at least say:

We tried all possible locations and this one is perfect.

Great job! Thanks for all that contributed so far!

1. +++ b/core/includes/common.inc
   @@ -2848,6 +2848,19 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
   +  // Check the elements for insecure HTML and pass through sanitization.
   +  if (isset($elements)) {
   +    $markup_keys = array(
   +      '#description',
   +      '#field_prefix',
   +      '#field_suffix',
   +    );
   +    foreach ($markup_keys as $key) {
   +      if (!empty($elements[$key]) && is_scalar($elements[$key])) {
   +        $elements[$key] = SafeMarkup::checkAdminXss($elements[$key]);
   +      }
   +    }
   +  }
   

   It'd be great to have more explicit testing of this in RenderTest - but that can be a followup.

2. +++ b/core/includes/common.inc
   @@ -2929,8 +2942,9 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
   +  $prefix = isset($elements['#prefix']) ? SafeMarkup::checkAdminXss($elements['#prefix']) : '';
   
   +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
   @@ -139,7 +139,8 @@ public static function preRenderConditionalComments($element) {
   -      $expression = $browsers['IE'];
   +      // Need to filter at least for admin usage.
   +      $expression = SafeMarkup::checkAdminXss($browsers['IE']);
   

   I don't get why we need to do this.

Leaving at rtbc to get an answer to the second question.

@Fabianx, now as all are in place do we need the following?

+++ b/core/modules/rdf/rdf.module
@@ -498,14 +498,15 @@ function rdf_preprocess_comment(&$variables) {
   }
   // Adds RDF metadata markup above comment body.
   if (!empty($variables['rdf_metadata_attributes'])) {
-    if (!isset($variables['content']['comment_body']['#prefix'])) {
-      $variables['content']['comment_body']['#prefix'] = '';
+    $prefix = '';
+    if (!empty($variables['content']['comment_body']['#prefix'])) {
+      $prefix = SafeMarkup::checkAdminXss($variables['content']['comment_body']['#prefix']);
     }
     $rdf_metadata = array(
       '#theme' => 'rdf_metadata',
       '#metadata' => $variables['rdf_metadata_attributes'],
     );
-    $variables['content']['comment_body']['#prefix'] = drupal_render($rdf_metadata) . $variables['content']['comment_body']['#prefix'];
+    $variables['content']['comment_body']['#prefix'] = SafeMarkup::set(drupal_render($rdf_metadata) . $prefix);
   }
 }

• $prefix = SafeMarkup::checkAdminXss($variables['content']['comment_body']['#prefix']);
  

  Do we still need to check the #prefix here? Clearly the prefix will go via drupal_render and gets checked.

• $variables['content']['comment_body']['#prefix'] = SafeMarkup::set(drupal_render($rdf_metadata) . $prefix);
  

  Is it really necessary now to mark the content as safe here?

#145: Here is my reasoning:

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -139,7 +139,8 @@ public static function preRenderConditionalComments($element) {
-      $expression = $browsers['IE'];
+      // Need to filter at least for admin usage.
+      $expression = SafeMarkup::checkAdminXss($browsers['IE']);

My reasoning, was that anyone could do something strange like setting the expression to some user input data, which clearly is an edge case.

On the other hand there is the "rule" to never put something in SafeMarkup::set that you are not 100% sure is safe.

Therefore would like to keep the check for consistency, but maybe improve the comment?

#147: I just _love_ that solution. Very clever usage of drupal_render() here and one less SafeMarkup::set(). Win!

#147 looks awesome.

It'd be good to get a better comment for the $browsers['IE'] case. I think somewhere we agreed to document every unexpected usage of SafeMarkup::set() really well so contrib does not copy unnecessarily - we should have the same rule for SafeMarkup::checkAdminXss().

Improved the comment in HtmlTag::preRenderConditionalComments.

The comment looks good to me. I hope it passes Alex' review as well.

Lets get this in!

1. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -137,6 +137,22 @@ public static function escape($string) {
   +   *  Applies a very permissive XSS/HTML filter for admin-only use.
   

   Extra space in front.

2. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -137,6 +137,22 @@ public static function escape($string) {
   +   * @param $string
   

   @param string $string

3. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -137,6 +137,22 @@ public static function escape($string) {
   +   *  @see \Drupal\Component\Utility\Xss\filterAdmin
   

   Indented too far, and it should be @see \Drupal\Component\Utility\Xss::filterAdmin()

4. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -799,7 +800,6 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
        }
   -
        // The #after_build flag allows any piece of a form to be altered
   

   Please don't remove this for no reason.

5. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
   @@ -147,19 +148,23 @@ public static function preRenderConditionalComments($element) {
   +    // Ensure what we are dealing with is safe.
   +    // This would be done later anyway in drupal_render().
   ...
   +    // Now calling SafeMarkup::set is safe, because we ensured the
   +    // data coming in was at least admin escaped.
   

   These comments need a little finessing, they don't read perfectly in English. Also, they should be rewrapped to 80 characters.

1. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -137,6 +137,22 @@ public static function escape($string) {
   +   *   SafeString::set, it won't be escaped again.
   

   Is "SafeString" supposed to be this class? If so, it can be self::set(), otherwise it needs to be fully qualified, and still needs ()

2. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -137,6 +137,22 @@ public static function escape($string) {
   +   * @see \Drupal\Component\Utility\Xss\filterAdmin
   

   you missed the :: and the () here.

Interdiffs++

[++]Commit to core :-)

Thanks @Tim Plunkett for a great review. I did go through the patch again very carefully, but could not find further occurrences of what Tim described.

Therefore back to RTBC.

Committed 56af688 and pushed to 8.0.x. Thanks!

Reclassifying as a bug since this will solve a load.

diff --git a/core/lib/Drupal/Core/Form/FormBuilder.php b/core/lib/Drupal/Core/Form/FormBuilder.php
index 3522fd2..dd2bcd8 100644
--- a/core/lib/Drupal/Core/Form/FormBuilder.php
+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -10,7 +10,6 @@
 use Drupal\Component\Utility\Crypt;
 use Drupal\Component\Utility\Html;
 use Drupal\Component\Utility\NestedArray;
-use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Component\Utility\String;
 use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\Access\CsrfTokenGenerator;

Fixed on commit.

Found some inconsistency in the code markup. Please see below.

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -139,27 +139,33 @@ public static function preRenderConditionalComments($element) {
+      $element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]>\n" . $prefix);

I believe the conditional comment markup in this line and the following lines is incorrect and inconsistent. They should always begin with <!-- and end with -->

Can the issue be re-opened to fix this.

Please file a followup and link here. Thanks!

Follow up to fix conditional comment expressions in HtmlTag.php has been raised.

#2364357: Markup in HtmlTag.php incorrect.

Issue closed, everything is as it should be.

Great work everyone! Happy to see this in.

Edit: Moving discussion to follow-up issue.