Ensure #markup is XSS escaped in Renderer::doRender()

Not retitling the issue just yet. But I think that #safe_markup would be good to have, and a much more friendly API for those writing render arrays than having to instantiate a class.

And then internally, we could have:

if (isset($element['#safe_markup'])) {
  $element['#markup'] = new SafeMarkup($element['#safe_markup']);
}

Critical, non beta blocker per catch.

Can we make the API addition to add #safe_markup and then convert a handful of them and then have a follow-up to convert the rest?

Another question is can we blindly replace #markup with #safe_markup, I suspect not.

Another question is can we blindly replace #markup with #safe_markup, I suspect not.

Much like each SafeMarkup::set()we should take care and think about each particular usecase. Most of them should alwayws be replaceable with some proper templates
and we should in case this is possible.

#markup we'd run check_plain() on.

Why? The semantics of check_plain() is that it expects plain text. The semantics of markup is that it's HTML. Would it make sense instead to run Xss::filterAdmin() on #markup? And for cases where the caller sets #markup to something containing user input, then change those usages to use #type => 'inline_template' instead? If we did those two things, would that remove the need for a #safe_markup property?

@Cottser, I think that if issue #2324371: Fix common HTML escaped render #key values due to Twig autoescape gets into core then we can have a solution to this. As per my thinking drupal_render() sets the $elements['#markup'] and this #markup property will send HTML all the time so we can use a method SafeMarkup::checkAdminXss() instead of SafeMarkup::set().

For further reference please follow patch #142 in #2324371: Fix common HTML escaped render #key values due to Twig autoescape

Posting a patch by only changing the SafeMarkup::set($elements['#markup']) with SafeMarkup::checkAdminXss() check. To me, adding #safe_markup needs more efforts and can't be done so easily.

Let's go for a review.

Anything in that area seriously needs tests, in order to be sure, what is going on.

Perhaps, the generated HTML are stripping some tags that were generated via #markup. This may happen as I've used SafeMarkup::checkAdminXss() method. While viewing the test result, Drupal\system\Tests\Common\RenderTest->testDrupalRenderChildElementRenderCachePlaceholder() shows false result in matching HTML generated as

Value '<pre></pre> ' is identical to value '<pre><bar>elk</bar></pre> '.

Other issues may exists also. Have to check thoroughly.

@joelpittet, just to be on the same page, you are saying that instead of just passing HTML to #markup it should be sent via the inline_template system. So in that case the existing markups and their calls needs to be changed. As an example,

$var = "Hello";
$test_element = array(
      '#markup' => "<p>This is a test $var</p>",
      '#prefix' => '<div>',
      '#suffix' => '</div>',
    );

will become,

$var = "Hello";
$test_element = array(
      '#markup' => '<p>This is a test {{ var }}</p>',
     '#variables' => array('var' => $var),
      '#prefix' => '<div>',
      '#suffix' => '</div>',
    );

Or may be (another thought),

$var = "Hello";
$test_element = array(
      '#markup' => array('markup' => '<p>This is a test {{ var }}</p>', 'variables' => array('var' => $var)),
      '#prefix' => '<div>',
      '#suffix' => '</div>',
    );

In either case, we need to re-structure the public function render() method and calls to #markup. Will this be a feasible solution to do? Or to find out a way to properly check and escape existing markup calls in the render method?

Thanks!

Adding a new patch based on twig template. Instead of directly using "inline_template" twig services are used directly. Question comes,

1. How much feasible to call renderInline() directly? REF: TwigEnvironment::renderInline
2. Though the content of #markup is passed to twig environment for autoescape rendering, how much is the passed string in #markup is secure? REF: Option three: Calling Twig service directly

Reviews please :-)
Thanks!

Any new implementation logic or new idea on the existing patch? I am sure I am missing something.

Help!!! :-)

@joelpittet, thanks for the clarifications. Patch #20 has some issues as you mentioned about bypassing the alters and preprocesses. Also it may pose some security violations (Not sure). However, there is surely other efficient features available.

Though, I'd love to discuss this with you in IRC #drupal-contribute.

Thanks!

Had a discussion with @joelpittet. Both of us agree on removing the usage of inline_template if all of it's usability can be ported in #markup and this can serve the purpose of inline_template usages.
As example,

drupal_render(['#markup' => '<div>{{ var }}</div>', '#context' => ['var' => 'awesome & escaped']];

can be the same as inline_template usages. But #markup has to be properly escaped. This issue may be the right place to do this one.

But first, need to find and fix issues with Renderer.php and any reasons that patch #20 has failed. Any guidance in this will be really helpful.

Love to hear more reviews and suggestions about this from other contributes.

Thanks!

One point of making inline_template not as simple as possible was that we the plan was to encourage people to use proper templates,
though I think this point is less important than the issues caused by autoescaping.

Getting a bit confused here. Every time the patch is re-queued and shows new types of test fails. :-/

Any ideas?

@aneek, Re: #27 I agree, if #markup can do what inline_template was doing, there seems to be a good case for removing the usage of inline_template. But I also agree with @dawehner that proper templating should be encouraged where ever possible.

@cpj, good thought. But I doubt that "inline_template" would be removed. Had a discussion with @alexpott and seems the chances are low on removing it. But the main target should be to make this patch up and running. Do you have any suggestions on this?

Thanks!

Tagging for https://drupaltwig.org.

I believe this issue has gotten off track a lot from the original intent.

I am sorry for blowing the party ( :-D ), but I would like to try - if I may - to state again our original intent of making #markup safe.

What we want to avoid is someone to write:

  $build['title']['#markup'] = '<h1>' . $entity->title . '</h1>';

where $entity->title == <script>alert('xss');</script>.

The situation when this issue was written was different as there was no real alternative to #markup.

But we now have #inline_template, we have templates, we have render trees, etc.

And we also solved it for #prefix and #suffix.

I think at this point all we want to do is to apply the following patch:

- // @todo Simplify after https://drupal.org/node/2273925
   if (isset($elements ['#markup'])) {
-    $elements ['#markup'] = SafeMarkup::set($elements ['#markup']);
+   $elements ['#markup'] = SafeMarkup::checkAdminXss($elements ['#markup']);
   }

And thats it.

If you use it right, your #markup is already secure, hence this is fast.

If you do not use it right, then your markup goes via the XSS filter and then is secure.

I would do this explicitly and not combine with #description, because #markup needs to be a scalar. Everything else is a bug. Fortunately isset() is a no-op in terms of performance.

--

Does anyone have any objections to this really minimal impact solution to fix a (needs triage) critical?

Here is a patch, lets see if it works.

This probably needs tests, I'll try to write some.

The patch in #40 seem to cause failures, but I've attached here now only with tests (which fails until proposed solution is implemented) and then with the #40 implementation.

I put status to "Needs review" for triggering testbot, the implementation still needs work I guess (as it doesn't pass the tests).

Apparently after retesting #40 implementation, it look like it does not cause failures anymore. Therefore status "Needs review" is valid!

EDIT: Will see now with the implementation :)

Patch #11 had implementation of SafeMarkup::checkAdminXSS but there were many failures. I think dueto adminxss elements are getting stripped off.

@Fabianx, my first implementation was to introduce SafeMarkup::checkAdminXss() in the doRender() method but along with huge lists of failure I figured out that some tags in #markup are getting replaced by SafeMarkup::checkAdminXss().
If you see the patch #11 and the comment #15 you will see Drupal\system\Tests\Common\RenderTest->testDrupalRenderChildElementRenderCachePlaceholder() gives false due to some tags are stripped out of #markup.
Such as Value '<pre></pre> ' is identical to value '<pre><bar>elk</bar></pre> '..

Since we have inline template so I've used the inline template renderer service in #20 patch.

I hope there will be a much better way of doing this. What are your thoughts on these?

There is 3 cases:

- a) Invalid markup, e.g. '<bar>', if you need that you need to ensure your string is safe. If tests use that it should be replaced with something else.
- b) Valid #markup that needs to use '<script></script>', again in this code you need to mark it safe and be done.
- c) Valid #markup that was safe, but was concat-ed in an unsafe way, hence leading to things being stripped.

Given those cases I don't think the approach to XSS-escape #markup is wrong or too much, because it will be rare to put things that are not XSS safe in #markup directly.

I think this is the main culprit, just putting #markup through renderInlineTemplate won't work as that would mean the template is secure, which it is not in this case.

I think b) should be rare in core, but a) or c) likely are problems of the test failures.

So task list would be:

- Fix test failures purely based on forbidden tags, e.g '<bar>'
- Fix test failures that accept '<script>', not sure we have that. (Edit: we have)
- Fix concat-problems, where #markup is concat'ed.

The following command can be used to find concats:

$ grep '#markup' -r core/ | grep '\.'

Note: Not every #markup needs to be replaced with #inline_template, it is IMHO okay to still support the #prefix, #markup, #suffix version for some cases or even using String::format for simple concats.

See #2446995: Block content titles are not escaped on new block form (Port SA-CONTRIB-2013-082) for an example of a security issue that this would help prevent.

Re-roll for the changes to render tests.

This is still really broken in lots of places in core.

I also noticed that this broke some of the rendering tests, which seems to be related the fact that it's stripping out placeholders for the #post_render_cache.

There's also some failures in Drupal\Tests\Core\Render\RendererBubblingTest::testBubblingWithPrerender that I wasn't able to identify.

#54: Genius, #post_render_cache explains a lot of the many test failures ...

/me singing 'We need a placeholder API, a placeholder API, etc.'

For now I think drupal_render_cache_generate_placeholder might need a SafeMarkup::set added and that might fix a lot :).

It looks like we might be able to get this adding 'drupal-render-cache-placeholder' to the allowed list of tags, and reworking Xss::attributes() to skip that callback attribute...but then I don't know if we're getting into dangerous territory of remote code execution via render callbacks?

Lets see!

#58 looks good to me. Waiting on testbot still though.

Contextual Fixed now ...

Updated to fix failures in render tests with fake tags.

The failure in Drupal\filter\Tests\FilterAPITest shows something interesting.

Our whole text filter chain is not distinguishing between secure and insecure in and outputs - even though it could.

Obviously its possible to do:

     $placeholder = drupal_render_cache_generate_placeholder($callback, $context);
-    $result = new FilterProcessResult($text . '<p>' . $placeholder . '</p>');
+    $result = new FilterProcessResult(SafeMarkup::set($text . '<p>' . $placeholder . '</p>'));
     $result->addPostRenderCacheCallback($callback, $context);
     return $result;

to fix the #post_render_cache case, but this would probably break when applying another filter afterwards and this should probably be secure in the first place ...

I am assigning shortly to effulgentsia to get his thoughts / feedback on that (Thanks for all the work on String::format and l()!).

That probably warrants its own sub-issue ...

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -801,7 +800,7 @@ public function generateCachePlaceholder($callback, array &$context) {
+    return SafeMarkup::set('<drupal-render-cache-placeholder callback="' . $callback . '" token="' . $context['token'] . '"></drupal-render-cache-placeholder>');

Is there anything that prevents a cache placeholder from being embedded in a larger string of text within #markup? I suppose we could say that's not best practices, but it seems like our current API would totally support that, and this patch would break that.

Also I doubt that most of these failures are related to post_render_cache tokens since that's only used in a few places in core.

This actually reminds me of #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5 — PHP being unable to parse HTML5 correctly, and #952964: String stripped from title and alt attribute if contains a colon/#2105841: Xss filter() mangles image captions and title/alt/data attributes/#2321061: Xss::split() fails on custom HTML elements with colons in the name — basically, our XSS filtering stripping things that it should not strip. Quite possibly the problem being seen here is a duplicate of that.

+++ b/core/modules/contextual/src/Element/ContextualLinksPlaceholder.php
@@ -47,7 +47,8 @@ public function getInfo() {
-    $element['#markup'] = '<div' . new Attribute(array('data-contextual-id' => $element['#id'])) . '></div>';
+    // @todo Consider String::format instead.
+    $element['#markup'] = SafeMarkup::set('<div' . new Attribute(array('data-contextual-id' => $element['#id'])) . '></div>');

There's no corresponding use statement, so this is broken. Removing it from this patch, though maybe we should bring it back with a use statement?

Our whole text filter chain is not distinguishing between secure and insecure in and outputs - even though it could.

Maybe I'm missing something, but I think simply running the filters is enough to consider the output safe (if not, then that's a bug with the filters, not something for us to babysit), so here's a way to do that and fix that FilterApiTest.

Thanks effulgentsia!

That makes sense.

I'm reviewing the patch. I'll bring some updates in 10 or 20 (if drush or something pass the test)

So I took a look at this today, and the very first example I ran into was Drupal\action\Tests\BulkFormTest, which tests views bulk action forms which use placeholders to place the checkboxes. The placeholder replacement works fine, but in views_pre_render_views_form_views_form() the substituted output is placed into $element['output']['#markup'], which of course strips input and label tags when rendered. Going back to the comments in #50, I'm not sure what the right approach would be here. Obviously I could just do SafeMarkup::set() on the output, but I have a few concerns about this:

1. I suspect this patterns of dumping large rendered strings into #markup is probably somewhat common
2. We can try to be careful to only use SafeMarkup::set() when we know something is safe, but it's really hard to tell what is in the rendered element. Marking such large chunks of HTML as safe could make it easier for a contrib module to modify something that gets marked as safe, when it hasn't been sanitized.
3. I have no idea if this is true, or I'm just talking out of my ass, but it seems like marking increasingly large chunks of HTML as safe could affect the performance of SafeMarkup, or it's memory usage.

To expand on #2, we're assuming that everything coming out of drupal_render is safe, and I don't know if that is a safe assumption or not. I guess overall, we should probably try to stick to our general principle of not trying to overly babysit contrib. At least with this new API, we can search of usages of SafeMarkup::set() and check to see if output is truly safe or not.

I suppose if we ensure that only output directly from drupal_render is passed to SafeMarkup::set, we should be reasonably safe after this issue is finished.

Fix for views_pre_render_views_form_views_form().

What views_pre_render_views_form_views_form() does seems very similar to what String::format() does, except the tokenized string isn't a literal, and the token names don't all start with the handy @, !, % prefixes. What about adding a new method to the String class, like replace(), and have it treat each substitution token as the equivalent of a pass-through (but without requiring the ! prefix)? And then having String::replace() only mark the output as safe if 1) the tokenized string is already marked safe and 2) every substitution parameter is already marked safe?

That sounds pretty similar to the SafeMarkup::concat stuff that was confusing earlier, but I think this is better, for the following reasons:

1) It wouldn't actually mark anything as safe, unless all of it's components were already safe
2) It'd be setting a good example and better API for contrib developers (given the security nature of this task, is pretty important).

@mikey_p,

Applied patch #79 and had a look at the fails (Drupal\dblog\Tests\DbLogTest).
In /core/modules/dblog/src/Tests/DbLogTest.php @ line # 321 we can see that the test suite is checking for database logger event. The event message is generated by,

$message = t('Deleted user: %name %email.', array('%name' => $name, '%email' => '<' . $user->getEmail() . '>'));

code and it should generate a proper HTML Markup. Instead it's generating,

Deleted user: <em class="placeholder">pTKMZtsq</em> <em class="placeholder"><pTKMZtsq@example.com></em>.

This is double escaped. The next code block that used html_entity_decode can't properly decode it. So,

$this->assertTrue($link, 'DBLog event was recorded: [delete user]');

fails.

As per my thoughts adding SafeMarkup::set() can solve this issue. But I don't think adding SafeMarkup::set() in those areas (many cases) where double escaping is happening is worth.

Looked into some more of these and a bunch are due to the fact that this broke the BatchAPI somehow. I'm pretty baffled as to what's going on, when I check the verbose messages, I'm seeing that the batch error is a 406 Not Acceptable. I'll have to look into this some more later, but thought I'd throw this up in case someone elese wants to look into the failures.

The worst part is that unlike the other errors, I can't recreate this one with manual testing. It appears to be some artifact of the way simpletest is executing the batch.

null is listed as one of the acceptable MIME types in that screenshot. That's highly suspicious.

That took me a lot longer to figure out that I would like to admit. The whole 406 thing was a bit of a red herring, due to JS triggering on the verbose debug pages. This should fix a number of the config import tests, batch test, node access rebuild test, etc.

@mikey_p, great find. But still there are 2 issues.

1. Contextual Links (Drupal\block\Tests\Views\DisplayBlockTest, Drupal\menu_ui\Tests\MenuTest etc.)
2. Double escaping of characters. (Drupal\rest\Tests\Views\StyleSerializerTest/)

These might need some attention. I hope contextual link was solved in patch #61? But removed later on somehow.

Yes, lets bring #61 back with a use statement :).

Here is an analysis of all SafeMarkup::set().

TL;DR, only Filter and String class should need to use SafeMarkup::set(), the rest more high-level API functions.

1. +++ b/core/lib/Drupal/Component/Utility/String.php
   @@ -146,5 +146,33 @@ public static function placeholder($text) {
   +    if (SafeMarkup::isSafe($subject)) {
   +      return SafeMarkup::set($output);
   +    }
   

   This is fine for ::set

2. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
   @@ -94,7 +94,7 @@ public static function preRenderHtmlTag($element) {
        if (!empty($element['#noscript'])) {
   -      $element['#markup'] = '<noscript>' . $markup . '</noscript>';
   +      $element['#markup'] = SafeMarkup::set('<noscript>' . $markup . '</noscript>');
        }
   

   This can use String::format with ! as $markup was already marked safe.

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -803,7 +802,7 @@ public function generateCachePlaceholder($callback, array &$context) {
    
   -    return '<drupal-render-cache-placeholder callback="' . $callback . '" token="' . $context['token'] . '"></drupal-render-cache-placeholder>';
   +    return SafeMarkup::set('<drupal-render-cache-placeholder callback="' . $callback . '" token="' . $context['token'] . '"></drupal-render-cache-placeholder>');
      }
   

   This can use String::format with % for callback and token, should be safe to check_plain() it being base64 and a valid PHP function.

4. +++ b/core/modules/filter/src/Element/ProcessedText.php
   @@ -120,7 +121,7 @@ public static function preRenderText($element) {
   -    $element['#markup'] = $text;
   +    $element['#markup'] = SafeMarkup::set($text);
   

   This means the text was filtered for a known text format.

   It is safe by design in that case.

5. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php
   @@ -207,7 +207,7 @@ public static function callback(array $element, array $context) {
   -      '#markup' => '<bar>' . $context['bar'] . '</bar>',
   +      '#markup' => '<code>' . $context['bar'] . '

   ',
   

   If we want to keep this can use String::format with % for the context['bar'], but this is fine, too.

Double escaping of characters at Drupal\rest\Tests\Views\StyleSerializerTest still needs to be figured out

Posting a patch addressing some of the findings by @Fabianx.
Btw, @Fabianx why use % placeholder for String::format()? This will run String::placeholder method to add extra <em class="placeholder">. So I've used @ and the string in it will be check plain'd.
Haven't changed the <bar></bar> tags. Lets see how it runs through and then in the next patch we can address those.
FYI, double escaping is not solved yet. Checked locally. Have to debug Drupal\rest\Tests\Views\StyleSerializerTest->testFieldapiField() throughly.

@lauriii, my friend, sorry I posted the same patch (only different in some placeholders). Can you please make your patches for review too?

I think it got messed up somehow because of double patches. Probably should reupload?

@Fabianx,
HtmlTagTest failed as we implemented String::format() with ! placeholder. Maybe SafeMarkup::isSafe is not returning the string as safe so this is happening. Trying with inline_template instead of String::format. Let's see :-)

Hm, I don't get it, why !markup is deemed not safe. Will do a local test later today.

The latest patch does not apply, I have the hope that this reroll work.

Trying to reroll again #95 moving the functions testReplaces and providerReplace from StringTest to SafeMarkupTest and function replace from String to SafeMarkup.

I've replaced all String calls to SafeMarkup for avoiding fatal errors. Trying again!

Send new patch to try to correct the multiple exceptions, but any ideas for errors?

Send new patch to try to correct the multiple exceptions, but any ideas for errors?

1. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -282,4 +282,34 @@ public static function placeholder($text) {
   +
   +    if (SafeMarkup::isSafe($subject)) {
   +      return SafeMarkup::set($output);
   +    }
   +    else {
   +      return $output;
   +    }
   

   Stuff like that seriously should have some kind of documentation.

2. +++ b/core/modules/contextual/src/Element/ContextualLinksPlaceholder.php
   @@ -47,7 +48,7 @@ public function getInfo() {
       */
      public static function preRenderPlaceholder(array $element) {
   -    $element['#markup'] = '<div' . new Attribute(array('data-contextual-id' => $element['#id'])) . '></div>';
   +    $element['#markup'] = SafeMarkup::set('<div' . new Attribute(array('data-contextual-id' => $element['#id'])) . '></div>');
        return $element;
      }
   

   Also for stuff like that documentation would be great, especially in the context of #2280965: [meta] Remove every SafeMarkup::set() call

Fixes 115 and 112

I discussed this with @alexpott, @xjm, and @webchick, and we agreed to keep this prioritized as Critical, because it is so common for modules to set #markup to a variable that might or might not contain user input that failing to fix this will likely result in many XSS vulnerabilities in contrib (and quite possibly, some in core). If we get close to an RC without this being close to fixed, we could instead document somewhere very prominently that it is the caller's responsibility to only set #markup to trusted or sanitized strings (as it is in 7.x), but it would be so much nicer to close off this vector instead.

kicking tyres

Reverts 95, more tomorrow

perchance kind bot

woot! Only 9 fails left :)

Tried to reduce the remaining issues and found the following.
The issue: Drupal\rest\Tests\Views\StyleSerializerTest Value '<p>ujTalrVZ2au3FQyRgGIiuXnxVK5H9Qz9&lt;/p&gt; ' is equal to value '<p>ujTalrVZ2au3FQyRgGIiuXnxVK5H9Qz9</p> '.

While debugging I found RestExport::execute() in /core/modules/rest/src/Plugin/views/display/RestExport.php returns an JSON data.

array (
  '#markup' => '[{"nid":"1","body":"<p>haQmpp1G8d6zOAAmbemKiFwYiDokerkp<\\/p>\\n"}]',
)

This calls doRender() method with $element['#markup'] set as [{"nid":"1","body":"<p>haQmpp1G8d6zOAAmbemKiFwYiDokerkp<\\/p>\\n"}] which is a JSON. So in SafeMarkup::checkAdminXss() this returns as FALSE by SafeMarkup::isSafe().

This is why the JSON string is double escaped and test is failing in StyleSerializerTest::testFieldapiField() @ line 317.

I hope this can be useful. I will try to debug/fix this or more later this week.

Love to hear about it's solution and thoughts from others :-)

May not be relevant but thought it worth mentioning: Twig does have different escaping "strategies", including one for JS.

http://twig.sensiolabs.org/doc/filters/escape.html

@Cottser thanks for this link. This is also mentioned in the SafeMarkup.php file. This may come handy if we have to remove SafeMarkup::set() in this issue.

Consider the following case, (Same as the test case testFieldapiField())
The data is a JSON format and sent via the REST and it will be double escaped. So there can be two things as per my understanding.

1. Add a logic to implement $strategy = 'json' in SafeMarkup.php
2. Or, RestExport::render() should consider escaping JSON value while returning it to Drupal render.

Please suggest if these seems good or can have a better approach.
Thanks!

Individual fields in the DataFieldRow plugin are sanitized in \Drupal\views\Plugin\views\field\FieldPluginBase::advancedRender() and we can safely assume that the Serializer does not introduce XSS when transforming the array into the particular format, hence we can safely mark the whole serialized string as safe.

Added a test to verify that.

Also fixed AreaTest

More tomorrow

@larowlan, fixed the "StyleSerializerTest" error. Uploading a new one.

Based on #2435493: Change String::format()'s '@' and '%' placeholders to be auto-escaped rather than always-escaped, '@' will not double escape an already escaped/marked safe string. Uploading a new patch to fix HtmlTagTest.php error.

Ahh, code had a bug. Uploading again.

Ahh, crap. Not fixed. I will debug more. In the mean time please use patch #133 for now.

+++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
@@ -315,13 +315,22 @@ public function testFieldapiField() {
+  public function testFieldapiFieldXSS() {

Why a separate method, this means a whole new Drupal install just for this tiny test.

+++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
@@ -315,6 +315,13 @@ public function testFieldapiField() {
+    $node->body = '<script type="text/javascript">alert("boo");</script>';

We should do this for the title as well, body is a formatted text field, so would already have check_markup equivalent called on it. Using title as well would add extra safeguard

@larowlan,
For #143 - It seemed that using $result = $this->drupalGetJSON('test/serialize/node-field'); twice was returning the previous node's results. So I made the tests different. Well you are right with burdening the system, I will try to merge these 2 functions together and check.

For #144 - Yes we could add an XSS check for the title as well.

I am slowly getting back to this issue, the two tests in #133 , one is trivial to fix, we discussed with aneek (who is very awesome for doing this finally!). As for other, the Views fail in #133 is due to the broken page attached. Now, in views_ui.theme.inc we see these:

'#markup' => drupal_render($form['default_group'][$group_id]) . drupal_render($form['default_group_multiple'][$group_id]),
'#markup' => drupal_render($form['group_items'][$group_id]['remove']) . \Drupal::l(/* long and irrelevant, I see the link */),

The test is missing

    $edit['options[group_info][default_group]'] = 2;
    $edit['options[group_info][group_items][3][remove]'] = 1;

It stands to reason the drupal_renders either come back empty handed or they just get filtered out wholesale. Haven't really dug what happens but I feel someone who is more into this can easily carry the issue home from here -- if not I will take a deeper look tomorrow.

Uploading a new patch. Considering,

1. A fix for HtmlTagTest. Thanks @chx.
2. Merging two test functions for StyleSerializerTest but this gave birth to a new major issue.
   #2477157: rest_export Views display plugin does not set necessary cache metadata We should follow-up on this. @todo comment is also added in this patch.
   Thanks @Berdir & @Fabianx.

Only FilterBooleanWebTest issue is left. If someone with knowledge in views can address this will be great as @chx said.

@joelpittet it should be working. Should we also remove all 20 other drupal_render calls from there or should we just open a follow-up for them?

I think we have tests from #43

It's green! Thank you everyone. This is looking good to me overall, some minor points below.

1. +++ b/core/modules/views_ui/views_ui.theme.inc
   @@ -147,21 +147,35 @@ function theme_views_ui_build_group_filter_form($variables) {
   +    $default = array(
   +      $form['default_group'][$group_id],
   +      $form['default_group_multiple'][$group_id]
   +    );
   

   Minor: missing trailing comma per https://www.drupal.org/coding-standards#array.

2. +++ b/core/modules/views_ui/views_ui.theme.inc
   @@ -147,21 +147,35 @@ function theme_views_ui_build_group_filter_form($variables) {
   +    $link = array(
   +      '#type' => 'link',
   +      '#url' => Url::fromRoute('<none>', [], array(
   

   Minor: Mixing short and long array syntax, why not just go short?

3. +++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
   @@ -188,4 +188,72 @@ function testPlaceholder() {
   +    // Replacement unsafe safe.
   

   Huh? I think this comment can be improved :)

fixes #156

Change 4. is needed to make tests pass. randomString can contain characters that are being autoescaped which makes tests fail

Some questions

1. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
   @@ -130,7 +132,16 @@ public function render() {
   +    if ($this->view->rowPlugin instanceof DataFieldRow) {
   +      // Individual fields in the DataFieldRow plugin are sanitized in
   +      // \Drupal\views\Plugin\views\field\FieldPluginBase::advancedRender() and
   +      // we can safely assume that the Serializer does not introduce XSS when
   +      // transforming the array into the particular format, hence we can safely
   +      // mark the whole serialized string as safe.
   +      SafeMarkup::set($output);
   +    }
   

   Mh, that feels kinda odd honestly, the serializer style plugin should not care about its individual row plugins. Why are we not able to mark both the DataFieldRow and DataEntityRow as safe? ... The problem is that contrib has no way to fix the problem, in case they have the same.

2. +++ b/core/modules/views/src/Tests/Handler/AreaTest.php
   @@ -92,9 +92,9 @@ public function testRenderArea() {
   -    $header_string = $this->randomString();
   -    $footer_string = $this->randomString();
   -    $empty_string = $this->randomString();
   +    $header_string = $this->randomMachineName();
   +    $footer_string = $this->randomMachineName();
   +    $empty_string = $this->randomMachineName();
   

   Should we not rather escape the expected output to avoid the random test failures? By doing that, we would ensure that the escaping actually happens. By using randomMachineName() this information is not given.

poking

wow styleserializertest is so slow to run locally

#163 fixes #161

@larowlan It would have been nice if you would have explained, why you made some of the changes :)

1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -259,7 +259,8 @@ public function execute() {
   -    return new Response(drupal_render_root($output), 200, array('Content-type' => $this->getMimeType()));
   

   What is the movitation to remove the drupal_render_root() calll, ... Is there really a technical need? Note: We might want to bubble up some metadata at some point, and then not calling could be a problem. I mean do we need to change RestExport at all here, given that it is just one unnecessary conflict with 2477157

2. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
   @@ -133,14 +133,6 @@ public function render() {
   -    if ($this->view->rowPlugin instanceof DataFieldRow) {
   -      // Individual fields in the DataFieldRow plugin are sanitized in
   -      // \Drupal\views\Plugin\views\field\FieldPluginBase::advancedRender() and
   -      // we can safely assume that the Serializer does not introduce XSS when
   -      // transforming the array into the particular format, hence we can safely
   -      // mark the whole serialized string as safe.
   -      SafeMarkup::set($output);
   -    }
   

   So we don't actually need that?

3. +++ b/core/modules/views/src/Tests/Handler/AreaTest.php
   @@ -107,9 +108,9 @@ public function testRenderArea() {
   +    $this->assertTrue(strpos($output, SafeMarkup::checkPlain($header_string)) !== FALSE);
   +    $this->assertTrue(strpos($output, SafeMarkup::checkPlain($footer_string)) !== FALSE);
   +    $this->assertTrue(strpos($output, SafeMarkup::checkPlain($empty_string)) !== FALSE);
   

   For testing purposes it feels better to rely on something which doens't change state internally but I guess there is no way to do that :(

Sorry, should have explained.
Yeah we don't need the Drupal render call, it's already a string (XML/json) that's been rendered once already, but then shoved into #markup which gets rendered again. Agree on the metadata so added the to-do.

Why not just return a CacheableResponse instead? Why defer the cacheability metadata to a todo?

Why not just return a CacheableResponse instead? Why defer the cacheability metadata to a todo?

Because, its entirely out of scope of this issue! Even more, we have a dedicated issue to just do that: #2477157: rest_export Views display plugin does not set necessary cache metadata, which is RTBC

LOL, right, that's why I was confused then :P I was reviewing both issues after each other, and got confused about that :D I even thought that the @todo linked to this issue.

/me goes to get lunch, need more sugar for thinking :P

Then I think this patch is close to ready?

Well, certainly the issue summary seems entirely out of date.

Yes, indeed. Just referring to the patch itself now.

Can somebody please update the IS?

Updated issue summary

My previous comment deleted somehow.Anyway,
#2477157: rest_export Views display plugin does not set necessary cache metadata is now fixed so we have to remove/refactor the following,

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
+    // @todo Cache Metadata - see https://www.drupal.org/node/2477157
+    return new Response($output['#markup'], 200, array('Content-type' => $this->getMimeType()));

And

+++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
+    // @todo Find a proper solution & fix https://www.drupal.org/node/2477157
+    // Invalidate the cache since the RestExport returns the cached data.
+    \Drupal::cache('render')->deleteAll();

Thanks!

Not sure if this will fail until such time as #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache is in.

Not sure how the credit thing works. How can you tell it the first 5 patches were done on my own time and the last one was done on on company time? Or is not that sophisticated?

Not sure how the credit thing works. How can you tell it the first 5 patches were done on my own time and the last one was done on on company time? Or is not that sophisticated?

Well, I would think given that the attribute thing is a checkbox now, you can simply tick both.

Stop double rendering rest (xml/json) resources

+++ b/core/modules/filter/src/Element/ProcessedText.php
@@ -118,9 +119,11 @@ public static function preRenderText($element) {
diff --git a/core/modules/rest/src/Plugin/views/display/RestExport.php b/core/modules/rest/src/Plugin/views/display/RestExport.php

diff --git a/core/modules/rest/src/Plugin/views/display/RestExport.php b/core/modules/rest/src/Plugin/views/display/RestExport.php
index 8af1a75..0cd43e9 100644

index 8af1a75..0cd43e9 100644
--- a/core/modules/rest/src/Plugin/views/display/RestExport.php

--- a/core/modules/rest/src/Plugin/views/display/RestExport.php
+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -278,7 +278,7 @@ public function execute() {

@@ -278,7 +278,7 @@ public function execute() {
     $header = [];
     $header['Content-type'] = $this->getMimeType();
 
-    $response = new CacheableResponse($this->renderer->renderRoot($output), 200);
+    $response = new CacheableResponse($output['#markup'], 200);
     $cache_metadata = CacheableMetadata::createFromRenderArray($output);
 
     $response->addCacheableDependency($cache_metadata);
diff --git a/core/modules/rest/src/Plugin/views/style/Serializer.php b/core/modules/rest/src/Plugin/views/style/Serializer.php

diff --git a/core/modules/rest/src/Plugin/views/style/Serializer.php b/core/modules/rest/src/Plugin/views/style/Serializer.php
index 09e94e6..ac41a00 100644

index 09e94e6..ac41a00 100644
--- a/core/modules/rest/src/Plugin/views/style/Serializer.php

--- a/core/modules/rest/src/Plugin/views/style/Serializer.php
+++ b/core/modules/rest/src/Plugin/views/style/Serializer.php

+++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
+++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
@@ -7,7 +7,9 @@

@@ -7,7 +7,9 @@
 
 namespace Drupal\rest\Plugin\views\style;
 
+use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Core\Form\FormStateInterface;
+use Drupal\rest\Plugin\views\row\DataFieldRow;
 use Drupal\views\ViewExecutable;
 use Drupal\views\Plugin\views\display\DisplayPluginBase;
 use Drupal\views\Plugin\views\style\StylePluginBase;
@@ -130,7 +132,8 @@ public function render() {

@@ -130,7 +132,8 @@ public function render() {
       $content_type = $this->options['formats'] ? reset($this->options['formats']) : 'json';
     }
 
-    return $this->serializer->serialize($rows, $content_type);
+    $output = $this->serializer->serialize($rows, $content_type);
+    return $output;
   }
 
   /**

Can't re revert all this now? I don't see an advantage ... well $output is a render array and so to convert it to a string you call a render function ...

We can revert this

-    return $this->serializer->serialize($rows, $content_type);
+    $output = $this->serializer->serialize($rows, $content_type);
+    return $output;

But I don't think we can revert this

-     $response = new CacheableResponse($this->renderer->renderRoot($output), 200);
+    $response = new CacheableResponse($output['#markup'], 200);

I think reverting the second hunk will result in the stuff that's in $output['#markup'] being escaped twice.

Wait, so running the serializer also escapes certain entries? In case it does, I would expect that the same string is safe already, so double escaping should not happen. It seems to be at least worth documenting what is happening.

The reason why I think we should keep the call is that we want to ensure that potential processing done by the renderer, which might be important in the future, is executed.
Sorry for maybe being annoying here.

Let's see

The issue is, the components inside the serialized string are escaped, but not the whole string.

So the whole string isn't marked as safe (the xml/json meta bits aren't marked safe) but the field values (that went via rendering) are - so the aggregate string isn't previously marked safe.

Failed because of double escaping

forgot to merge

wrong file again

So far we have treated the concat of safe strings as safe as well.

Maybe we need to do so in the renderer as well?

I saw it mentioned in #112 that short array should be used on changed lines but I see it in 6 places on last patch:
+ $element['output'] = array('#markup'
+ $default = array(
+ $element['#markup'] = SafeMarkup::format('<noscript>!markup</noscript>', array('!markup'
+ return SafeMarkup::format('<drupal-render-cache-placeholder callback="@callback" token="@token"></drupal-render-cache-placeholder>', array(
+ $element['#markup'] = SafeMarkup::set('<div' . new Attribute(array('data-contextual-id'
+ $node->body = array(

Not a big deal for me but I am curious about the overall policy on this.

@MiroslavBanov there is no solid decision about that but I've seen most of people using the new array syntax on the changed lines so in some point we would have every piece of code using the new array syntax at some point.

Not a big deal for me but I am curious about the overall policy on this.

#2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8, short story: Cool, if you do so, otherwise nevermind.

Cool, if you do so, otherwise nevermind

Pretty good summary, thanks :).

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -282,4 +282,37 @@ public static function placeholder($text) {
+  /**
+   * Replace all occurrences of the search string with the replacement string.
+   *
+   * Functions identically to str_replace, but marks the returned output as safe
+   * if all the inputs and the subject have also been marked as safe.
+   */
+  public static function replace($search, $replace, $subject) {

Missing php doc for params and return.

Fixes 196

I think change record would be quite useful for this

Added a quick one for SafeMarkup::replace()

@dawehner++

We could still add another for the fact that #markup is being escaped automaticly. That might be very confusing for people.

Two change recoreds got ridden: https://www.drupal.org/node/2489968 https://www.drupal.org/node/2489900

Issue summary looks good to me so no need to update that. We have also the change records here so this is RTBC to me.

Nice work on this. Overall I think this is pretty much ready. I like the DX of SafeMarkup::replace() and I think this is a good security improvement as well. It's also great to see the detailed comments justifying each use of SafeMarkup::set() as this will ensure best practices are followed and that we aren't unintentionally leaving unsafe things unescaped.

The change records look great, but can we move https://www.drupal.org/node/2489968 into the main SafeMarkup CR?

Also, it would be good to see some before-and-after testing of the Views UI stuff to ensure we're not (e.g.) introducing double-escaping or other formatting errors. (40% of the time I spent reviewing this patch went into parsing that diff with the array reformatting. It's definitely more readable now but it was hard to parse out the actual changes.) Thanks!

Finally, some extremely minor nitpicks that are totally not worth blocking this on, but that @jsmith said he could fix for us here at the LA extended sprints. :)

1. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -282,4 +282,49 @@ public static function placeholder($text) {
   +   * Functions identically to str_replace, but marks the returned output as safe
   +   * if all the inputs and the subject have also been marked as safe.
   

   Nit: str_replace() should have parens so it gets linked automatically to php.net (at least this used to work).

2. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -282,4 +282,49 @@ public static function placeholder($text) {
   +   *   The value being searched for, an array may be used to designate multiple
   +   *   values.
   

   Nit: Comma splice.

3. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -282,4 +282,49 @@ public static function placeholder($text) {
   +    else {
   +      foreach ($replace as $replacement) {
   +        if (!SafeMarkup::isSafe($replacement)) {
   +          return $output;
   +        }
   +      }
   

   Had to read this twice. If any one of the replacements is unsafe, then we don't mark anything new as safe. I'd add a comment before the if something like "If any replacement is unsafe, then the output is also unsafe, so just return the output."

4. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -282,4 +282,49 @@ public static function placeholder($text) {
   +    if (SafeMarkup::isSafe($subject)) {
   +      return SafeMarkup::set($output);
   +    }
   +    else {
   +      return $output;
   +    }
   

   Nit: This is minorly backwards from the rest of the method and it might be more intuitive if it were the other way around, so that doing the SafeMarkup::set() is the last thing.

5. +++ b/core/modules/contextual/src/Element/ContextualLinksPlaceholder.php
   @@ -47,7 +48,11 @@ public function getInfo() {
   +    // Because the only arguments to this markup will be instance of
   +    // \Drupal\Core\Template\AttributeString, which is passed through
   +    // \Drupal\Component\Utility\SafeMarkup::checkPlain() before being output
   +    // this markup is safe, and is marked as such.
   

   Nit: instances. I also got a bit confused reading this. I'd reword this as: "This markup is safe because the only arguments will be instances of \..., which is passed through \...checkPlain().

6. +++ b/core/modules/filter/src/Element/ProcessedText.php
   @@ -118,9 +119,11 @@ public static function preRenderText($element) {
   +    // Filtering and sanitizing has been done in
   +    // \Drupal\filter\Plugin\FilterInterface. Store its content in #markup,
   +    // set the updated bubbleable rendering metadata, and set the text format's
   +    // cache tag.
   

   Nit: filtering and sanitizing have been done. Also what does "its" refer to?

7. +++ b/core/tests/Drupal/Tests/Core/Render/RendererPostRenderCacheTest.php
   @@ -432,7 +432,7 @@ public function testPlaceholder() {
   -    $expected_output = '
   ' . $context['bar'] . '
   ';
   
   +    $expected_output = '
   ' . $context['bar'] . '
   ';
   @@ -530,7 +530,7 @@ public function testChildElementPlaceholder() {
   
   -    $expected_output = '
   ' . $context['bar'] . '
   ' . "\n";
   
   +    $expected_output = '
   ' . $context['bar'] . '
   ' . "\n";
   +++ b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php
   
   @@ -255,7 +255,7 @@ public static function callback(array $element, array $context) {
   
   -      '#markup' => '' . $context['bar'] . '',
   
   +      '#markup' => '' . $context['bar'] . '',
   

   Note to me: look up the intent of the use of <bar> in these tests.

Updating patch to address @xjm's comments in comment 205. Note that I didn't make any changes for part 7 of the code review, as it was a note for @xjm to check before committing.

Will work on manual testing and change records next.

@jsmith thank you

1. +++ render-xss-2273925.206.patch	2015-05-17 12:46:54.894298568 -0700
   @@ -1,20 +1,20 @@
   -+   * Functions identically to str_replace, but marks the returned output as safe
   ++   * Functions identically to str_replace(), but marks the returned output as safe
   

   since we are nitting and fixing immediately... the added () push this (further) over 80 chars.

2. +++ render-xss-2273925.206.patch	2015-05-17 12:46:54.894298568 -0700
   @@ -104,21 +106,20 @@
   -+    // Because the only arguments to this markup will be instance of
   ++    // This markup is safe because the only arguments will be instances of ¶
    +    // \Drupal\Core\Template\AttributeString, which is passed through
    +    // \Drupal\Component\Utility\SafeMarkup::checkPlain() before being output
   -+    // this markup is safe, and is marked as such.
   

   bit awkward wording.
   How about "is safe because the arguments will only be instances of"

   Missing a period at the end of the sentence.

rest of the changes look great.

Updating the patch based on the comments on 207. Also re-doing the interdiff (from 197 to this patch) properly to make it easier to follow. I'll plead ignorance that my interdiffs weren't being done exactly correctly, and repent of my sins :-)

Adding an interdiff between 206 and 208 for @YesCT.

I read the interdiff. Those changes look great.

We are now going to work on the change record.

We found the change record @xjm asked "#markup is now automatically escaped" https://www.drupal.org/node/2489968 to be merged into: "Twig autoescape enabled. New SafeMarkup class added." https://www.drupal.org/node/2296163

@jaredsmith is doing that now.

I'm looking into the fails.

Fantastic team work everyone! Just want to note that @larowlan's patch in #197 had those same fails until I hit retest. Potentially a random factor somewhere worth testing.

the interdiff since 206 should not have caused something different in the test results....

fails in #208 are

FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,399 pass(es), 2 fail(s), and 0 exception(s).

CollapsedDrupal\views\Tests\Handler\AreaTest	93	2	0
Message	Group	Filename	Line	Function	Status
Value false is TRUE.	Other	AreaTest.php	111	Drupal\views\Tests\Handler\AreaTest->testRenderArea()	
Value false is TRUE.	Other	AreaTest.php	113	Drupal\views\Tests\Handler\AreaTest->testRenderArea()	

#206 was PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,377 pass(es).

"false is TRUE"

+++ b/core/modules/views/src/Tests/Handler/AreaTest.php
@@ -107,9 +108,9 @@ public function testRenderArea() {
-    $this->assertTrue(strpos($output, $header_string) !== FALSE);
-    $this->assertTrue(strpos($output, $footer_string) !== FALSE);
-    $this->assertTrue(strpos($output, $empty_string) !== FALSE);
+    $this->assertTrue(strpos($output, SafeMarkup::checkPlain($header_string)) !== FALSE);
+    $this->assertTrue(strpos($output, SafeMarkup::checkPlain($footer_string)) !== FALSE);
+    $this->assertTrue(strpos($output, SafeMarkup::checkPlain($empty_string)) !== FALSE);

these could be clearer as to what they are trying to assert, and have a better assert message. "string from the header does not appear" ... ??

--
taking a break. anyone else is welcome to look into this.

See #161.2 and #163 - personally I prefer just making it ->randomMachineName() to remove chance of randoms