Follow-up to #2297711: Fix HTML escaping due to Twig autoescape

Problem/Motivation

The DX for fixing HTML escaped values from the render API is currently a pain.

We don't want to open up security holes but we don't want to also make it a pain to put #descriptions on fields with HTML tags in them for example. So the mid point between those two problems seems to be Xss::filterAdmin() on certain render array keys.

Example issue for reference: @see #2309929: HTML double-escaping in field forms

Proposed resolution

@larowlan and @chx came up with a great idea to deal with the DX and safe markup work necessary for a good chunk of what's left HTML escaped through keys in the Render API.

Proposing the following keys be run through Xss::filterAdmin() in the render API:

  • #description
  • #field_prefix
  • #field_suffix
  • #prefix
  • #suffix

Remaining tasks

Decide which keys should be XSS filtered.

User interface changes

n/a

API changes

Certain keys will be automatically run through Xss::filterAdmin().

Files: 
CommentFileSizeAuthor
#155 interdiff-153-155.txt2.33 KBjoelpittet
#155 fix_common_html_escaped-2324371-155.patch16.95 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,783 pass(es). View
#155 interdiff-153.txt3.09 KBjoelpittet
#153 2324371-153.patch15.66 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,510 pass(es). View
#150 2324371_150.patch15.42 KBwebflo
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,715 pass(es). View
#150 2324371_150.interdiff.txt569 byteswebflo

Comments

chx’s picture

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.

larowlan’s picture

+1 for the five keys in the OP

joelpittet’s picture

Status: Active » Needs review
FileSize
4.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,418 pass(es), 19 fail(s), and 84 exception(s). View
1.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,210 pass(es), 19 fail(s), and 84 exception(s). View
3.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,101 pass(es), 26 fail(s), and 0 exception(s). View

@chx not sure exactly where to put that for #markup but here's a quick patch to show it works. Grabbed the tests from #2309929-27: HTML double-escaping in field forms And applied to the keys.

The last submitted patch, 3: fix_common_html_escaped-2324371-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2309929-28-testonly-fail.patch, failed testing.

The last submitted patch, 3: fix_common_html_escaped-2324371-3-just-change.patch, failed testing.

aneek’s picture

+ 1 for this issue fixing. :)

aneek’s picture

FileSize
21.42 KB

@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.

Dots

joelpittet’s picture

I've my doubts now whether this is viable.

Those bullets are there, but get's filtered out of the allowed tags and that's why they show up.
Seven's local task override has this:

function seven_menu_local_tasks(&$variables) {
...
    $variables['primary']['#prefix'] = '

' . t('Primary tabs') . '

'; $variables['primary']['#prefix'] .= ''; $variables['primary']['#prefix'] .= '•••'; $variables['primary']['#prefix'] .= '
    '; $variables['primary']['#suffix'] = '
'; $variables['primary']['#suffix'] .= ''; $output .= drupal_render($variables['primary']); ... $variables['secondary']['#prefix'] = '

' . t('Secondary tabs') . '

'; $variables['secondary']['#prefix'] .= ''; $variables['secondary']['#prefix'] .= '
    '; $variables['secondary']['#suffix'] = '
'; $variables['secondary']['#suffix'] .= ''; ... }

Which is ridiculous I know, and maybe because we haven't converted those to twig yet that this may be an outlier... BUT I can see people wanting to put a button tag, and maybe other filtered out tags in Xss::AdminFilter() into a prefix/description/suffix etc. Also if those keys are on other hook_theme() implementations they will be filtered automatically and be potentially a bug WTF(especially #description).

Hmm, I'm at a bit of a loss here, help! I kind of just want to SafeMarkup::set() all the #description values that are passing through markup... though not sure if that is viable either.

larowlan’s picture

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.

joelpittet’s picture

@larowlan thanks, yeah now that I think about it, it's strange to but a button in description. More thinking the other ones, like putting a button in #field_suffix may be a totally reasonable thing?

Here's where we are actually cleaning up that mess for theme_menu_blah hooks #1898478: menu.inc - Convert theme_ functions to Twig

chx’s picture

OK, OK but what are those arrays?

herom’s picture

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.

larowlan’s picture

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

chx’s picture

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

aneek’s picture

@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!

chx’s picture

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.

aneek’s picture

@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!

aneek’s picture

FileSize
1.86 KB
6.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,229 pass(es), 19 fail(s), and 0 exception(s). View

@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.

aneek’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 19: fix_common_html_escaped-2324371-19.patch, failed testing.

joelpittet’s picture

+++ b/core/includes/common.inc
@@ -3126,6 +3127,23 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
+      $elements[$key] = NULL;

This would nullify arrays that are passed in. Is this what we want to do? Likely, but just checking...

Anyways that fixes the exceptions:) Thanks @aneek!

I will dig deeper later in the week when I get some time, though anybody can feel free to jump in!

aneek’s picture

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. :-)

aneek’s picture

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.

chx’s picture

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

andypost’s picture

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review

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

andypost’s picture

Assigned: Wim Leers » Unassigned
Status: Needs review » Needs work

It does not works anymore after reinstall

joelpittet’s picture

Status: Needs work » Needs review
FileSize
890 bytes
18.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,914 pass(es), 6 fail(s), and 0 exception(s). View

Here is a different approach... not sure if it's going to fly but the #description and field_prefix/suffix are FormElement keys so I delegated the responsibility of filtering them to that class. Unfortunately I can't think of a good way to add pre_render without NestedArray::mergeDeep, so I manually added it to where it needed to be or unioned it off of FormElement.

joelpittet’s picture

FileSize
18.24 KB

wrong interdiff.

Status: Needs review » Needs work

The last submitted patch, 28: fix_common_html_escaped-2324371-28.patch, failed testing.

chx’s picture

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

joelpittet’s picture

FileSize
6.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,110 pass(es), 18 fail(s), and 0 exception(s). View

Ok I went back to the global fix, not totally happy with that, but my idea of using the FormElement preRender is rife with merging issues or lots of duplicate code (error prone).

I did change up the logic a bit so that if it's scalar, it will do this, and if not it will leave it alone. If you have a renderable array or an object with __toString(), you have likely dealt with the issue already. May need to make that isSafe() or something...

Bloating up drupal_render feels wrong.

joelpittet’s picture

Status: Needs work » Needs review

My guess is more fails this time. But also I moved the code up before preRender so the JS errors may be fixed.

Status: Needs review » Needs work

The last submitted patch, 32: fix_common_html_escaped-2324371-32.patch, failed testing.

joelpittet’s picture

Yup, JavaScriptTest is fixed. Though some of the tests are looking for prefix <foo> which isn't safe.

If I'm generating XML, this may not be ideal. Hmmm.

joelpittet’s picture

FTR, I don't think #prefix/#suffix and their ilk should exist in the first place. The promote bad habits of putting markup in PHP.

I'm wondering if really this blanket approach is hurting us in the long run?

larowlan’s picture

Issue tags: +Needs reroll
rteijeiro’s picture

Assigned: Unassigned » rteijeiro

Let's drunkroll!

rteijeiro’s picture

Assigned: rteijeiro » Unassigned
Status: Needs work » Needs review
FileSize
4.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,751 pass(es), 18 fail(s), and 0 exception(s). View

Go bot, go!!

Status: Needs review » Needs work

The last submitted patch, 39: fix_common_html_escaped-2324371-39.patch, failed testing.

lauriii’s picture

Assigned: Unassigned » lauriii
Issue tags: -Needs reroll
lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs work » Needs review
FileSize
3.94 KB
6.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,669 pass(es). View
alqahtani’s picture

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

lauriii’s picture

FileSize
5.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,799 pass(es). View
563 bytes
marcus7777’s picture

Issue tags: +Amsterdam2014

reviewing

chx’s picture

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

lauriii’s picture

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

killerpoke’s picture

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.

lauriii’s picture

chx’s picture

@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.

Fabianx’s picture

Status: Needs review » Needs work

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!

lauriii’s picture

Assigned: Unassigned » lauriii
Fabianx’s picture

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!

lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs work » Needs review
FileSize
8.59 KB
10.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,896 pass(es), 2 fail(s), and 0 exception(s). View

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.

Fabianx’s picture

  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.

Status: Needs review » Needs work

The last submitted patch, 54: fix_common_html_escaped-2324371-54.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
2.26 KB
11.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,951 pass(es), 2 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 57: fix_common_html_escaped-2324371-57.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
11.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,919 pass(es). View
1.5 KB
chx’s picture

Status: Needs review » Needs work

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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
10.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,024 pass(es), 2 fail(s), and 0 exception(s). View

Dont know what I was thinking..

Status: Needs review » Needs work

The last submitted patch, 61: fix_common_html_escaped-2324371-61.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review
  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.

joelpittet’s picture

Thanks everyone and especially @lauriii for helping out, this is going in a much better direction now with all your help.
Glad the form stuff has been moved to it's domain and not poluting drupal_render().

There are a lot of links in the #description key especially but as long as they translate them or mark them as safe a head of time this should be ok.

The DX issue that would be most confusing is if a contrib module updates a description by appending more info, they'd have to re set the concatenated string in total as safe and not just their own addition unfortunately.

Fabianx’s picture

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.

lauriii’s picture

FileSize
1.29 KB
10.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,069 pass(es), 2 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 66: fix_common_html_escaped-2324371-66.patch, failed testing.

aneek’s picture

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?

Fabianx’s picture

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.

Fabianx’s picture

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.

aneek’s picture

@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 = "";
    $expected_2 = "\n" . '' . "\n\n" . '' . "\n";

    $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.

Fabianx’s picture

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

aneek’s picture

@Fabianx, thanks. Trying once again.

Fabianx’s picture

#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\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.

aneek’s picture

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

aneek’s picture

FileSize
4.34 KB
13.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,060 pass(es), 10 fail(s), and 0 exception(s). View

Uploading one patch. Needs manual and automated review both.

aneek’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 76: fix_common_html_escaped-2324371-76.patch, failed testing.

aneek’s picture

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

Fabianx’s picture

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.

aneek’s picture

FileSize
985 bytes
13.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,069 pass(es). View

@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.

aneek’s picture

Status: Needs work » Needs review
Fabianx’s picture

+++ 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

aneek’s picture

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

aneek’s picture

FileSize
1.22 KB
13.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,069 pass(es). View

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

Thanks!

chx’s picture

Status: Needs review » Needs work

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.

aneek’s picture

@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?

chx’s picture

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.

Fabianx’s picture

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.

aneek’s picture

@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.

Fabianx’s picture

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.

aneek’s picture

@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)?

Fabianx’s picture

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.

aneek’s picture

@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?

Fabianx’s picture

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.

aneek’s picture

@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!

aneek’s picture

FileSize
1.73 KB
13.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,184 pass(es). View

New patch to review, based on @Fabianx suggestions.

aneek’s picture

Status: Needs work » Needs review
hass’s picture

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.

hass’s picture

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

joelpittet’s picture

+++ b/core/modules/rdf/rdf.module
@@ -505,7 +505,7 @@ function rdf_preprocess_comment(&$variables) {
-    $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) . $variables['content']['comment_body']['#prefix']);

This is the only line I could find myself that seemed a bit on the iffy side of things.

I'm guessing that this is done so that the prefix doesn't get checkAdminXss filtered?

Kind of depends on what is inside there I guess...

@hass re #100 have you seen any issues arise that #title was commonly getting escaped tags in it? Usually I'd expect, for UI consistency, that field doesn't get tag much. Also, if there are any <em> tags they are likely coming from t(). I could be wrong though...

Otherwise nice work and thank you for helping make this much better than what I'd started, I was very skeptical but if someone can let me know on the SafeMarkup::set() what is up there. I'd be fine with RTBCing this puppy.

chx’s picture

@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?

hass’s picture

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.

Fabianx’s picture

Status: Needs review » Needs work

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.

aneek’s picture

@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 :-)

hass’s picture

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.

aneek’s picture

FileSize
1 KB
13.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,395 pass(es). View

Addressed comments by @joelpittet & @Fabianx.

Thanks!

aneek’s picture

Status: Needs work » Needs review
Fabianx’s picture

-      $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!

aneek’s picture

@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!

aneek’s picture

FileSize
675 bytes
13.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,415 pass(es). View

New patch.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC if tests pass. Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

chx’s picture

Status: Needs work » Needs review
FileSize
2.81 KB
16.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,442 pass(es), 1 fail(s), and 0 exception(s). View

Done.

chx’s picture

FileSize
3.74 KB
17.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,461 pass(es), 5 fail(s), and 0 exception(s). View
chx’s picture

FileSize
17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2324371_116.patch. Unable to apply patch. See the log in the details link for more information. View
3.35 KB

Opsie.

Fabianx’s picture

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

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

The last submitted patch, 114: 2324371_114.patch, failed testing.

The last submitted patch, 115: 2324371_115.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 116: 2324371_116.patch, failed testing.

lauriii queued 116: 2324371_116.patch for re-testing.

The last submitted patch, 116: 2324371_116.patch, failed testing.

Fabianx’s picture

Issue tags: +Needs reroll
lauriii’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
17.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,470 pass(es), 5 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 125: 2324371_125.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
16.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,479 pass(es). View
2.03 KB

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

chx’s picture

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

aneek’s picture

@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?

Fabianx’s picture

#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.

aneek’s picture

@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!

aneek’s picture

Status: Needs review » Needs work
aneek’s picture

FileSize
4.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,514 pass(es), 15 fail(s), and 0 exception(s). View

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.

aneek’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 133: fix_common_html_escaped-2324371-133.patch, failed testing.

aneek’s picture

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

aneek’s picture

FileSize
2.04 KB
16.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,551 pass(es), 11 fail(s), and 0 exception(s). View

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

Review review :-)

aneek’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 137: fix_common_html_escaped-2324371-137.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
17.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,545 pass(es). View
2.18 KB
aneek’s picture

@chx, thanks for the fix :-)

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

chx’s picture

FileSize
2.99 KB
15.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,661 pass(es). View
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

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!

joelpittet’s picture

Can't say we didn't try on this one.
RTBC+1

alexpott’s picture

  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.

aneek’s picture

@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?

chx’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
15.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,537 pass(es). View
1.02 KB
Fabianx’s picture

#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!

alexpott’s picture

#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().

webflo’s picture

FileSize
569 bytes
15.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,715 pass(es). View

Improved the comment in HtmlTag::preRenderConditionalComments.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

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

Lets get this in!

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
  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.

joelpittet’s picture

FileSize
15.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,510 pass(es). View

Thanks for the review @tim.plunkett these should address the comments in #152.

tim.plunkett’s picture

  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++

joelpittet’s picture

FileSize
3.09 KB
16.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,783 pass(es). View
2.33 KB

@tim.plunkett whoops sorry, just before I was kicked out of the sprint.

Here is the interdiff for #153 and this one.

aneek’s picture

[++]Commit to core :-)

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Category: Task » Bug report
Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 56af688 on 8.0.x
    Issue #2324371 by lauriii, aneek, chx, joelpittet, webflo, Fabianx,...
mbrett5062’s picture

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.

chx’s picture

Please file a followup and link here. Thanks!

mbrett5062’s picture

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.

Cottser’s picture

Great work everyone! Happy to see this in.

Edit: Moving discussion to follow-up issue.

jibran’s picture

Issue tags: +SafeMarkup

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.