Problem/Motivation

Follow-up to #1825952: Turn on twig autoescape by default

If drupal_render() produces #markup it is always safe. If, however, the caller sets #markup it is presumed to be safe to make #1825952: Turn on twig autoescape by default reviewable.

Proposed resolution

  1. Run all #markup through SafeMarkup::checkAdminXss().
  2. Add SafeMarkup::replace() - identical to str_replace() - but if all elements are safe, mark the resultant string safe too.

Remaining tasks

None

User interface changes

None

API changes

  • All #markup goes via SafeMarkup::checkAdminXss()
  • New SafeMarkup::replace()
Files: 
CommentFileSizeAuthor
#320 interdiff-dorender-final.txt1.97 KBxjm
#320 dorender-2273925-320.patch20.84 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,679 pass(es). View
#315 interdiff.txt1.82 KBeffulgentsia
#315 2273925-render-xss.315.patch20.66 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,736 pass(es). View
#312 2273925-render-xss.311.patch20.3 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,721 pass(es). View
#312 interdiff.txt1.42 KBlarowlan
#309 2273925-render-xss.308.patch20.18 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#309 interdiff.txt1.04 KBlarowlan
#308 2273925-render-xss.307.patch19.76 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#308 interdiff.txt1.77 KBlarowlan
#304 interdiff-2273925-304.txt1014 byteslokapujya
#304 2273925-304.patch19.82 KBlokapujya
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,712 pass(es). View
#303 interdiff.txt2.23 KBjoelpittet
#303 ensure_markup_is_xss-2273925-302.patch20.01 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,733 pass(es). View
ensure_markup_is_xss-2273925-302.patch20.01 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,724 pass(es). View
#300 interdiff.txt976 bytesjoelpittet
#300 ensure_markup_is_xss-2273925-300.patch19.96 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,720 pass(es). View
#299 ensure_markup_is_xss-2273925-299.patch19.73 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,738 pass(es). View
#299 interdiff.txt1.11 KBlauriii
#297 2273925-render-xss.297.patch18.47 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,717 pass(es), 6 fail(s), and 0 exception(s). View
#297 interdiff.txt845 byteslarowlan
#292 interdiff-291-292.txt821 bytesjoelpittet
#292 ensure_markup_is_xss-2273925-292.patch17.86 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,884 pass(es), 1,708 fail(s), and 11 exception(s). View
#291 2273925-291.patch17.86 KBlokapujya
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,881 pass(es), 1,708 fail(s), and 11 exception(s). View
#269 interdiff-267-269.patch3.88 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#269 renderer-2273925-269.patch20.16 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,275 pass(es). View
#267 interdiff.txt729 byteslauriii
#267 ensure_markup_is_xss-2273925-267.patch20.21 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,272 pass(es). View
#265 interdiff.txt593 byteslauriii
#265 ensure_markup_is_xss-2273925-265.patch20.3 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,234 pass(es), 50 fail(s), and 59 exception(s). View
#261 interdiff.txt2.95 KBlauriii
#261 ensure_markup_is_xss-2273925-260.patch19.59 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,189 pass(es), 3 fail(s), and 0 exception(s). View
#256 render-xss-nits.256.patch20.55 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,183 pass(es). View
#256 interdiff.txt2.29 KBlarowlan
#248 render-xss-nits.248.patch21.45 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,068 pass(es), 1 fail(s), and 0 exception(s). View
#248 interdiff.txt3.96 KBlarowlan
#240 ensure_markup_is_xss-2273925-240.patch20.21 KBgoogletorp
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,042 pass(es). View
#240 interdiff.txt5.7 KBgoogletorp
#238 interdiff.txt1.93 KBlarowlan
#237 render-xss-2272925-2.237.patch19.81 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,840 pass(es). View
#236 interdiff.txt4.25 KBlauriii
#236 ensure_markup_is_xss-2273925-236.patch18.13 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,595 pass(es). View
#228 render-xss-2273925.228.patch17.92 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,489 pass(es). View
#228 interdiff.222.228.txt760 bytesYesCT
#222 render-xss-2273925-2.222.patch17.92 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,375 pass(es). View
#222 interdiff.txt1.04 KBlarowlan
#219 render-xss-2273925-2.219.patch17.81 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,379 pass(es). View
#219 interdiff.txt980 byteslarowlan
#217 render-xss-2273925-2.217.patch17.83 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,330 pass(es), 1 fail(s), and 0 exception(s). View
#217 interdiff.txt1.21 KBlarowlan
#209 interdiff-2273925-206-208.txt1.82 KBjaredsmith
#208 interdiff-2273925-197-208.txt3.72 KBjaredsmith
#208 render-xss-2273925.208.patch17.04 KBjaredsmith
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,399 pass(es), 2 fail(s), and 0 exception(s). View
#197 render-xss-2273925.197.patch16.96 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,377 pass(es). View
#197 interdiff.txt1.05 KBlarowlan
#190 render-xss-2273925.190.patch16.5 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,370 pass(es). View
#189 render-xss-2273925.p188.patch64.49 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,320 pass(es). View
#188 render-xss-2273925.p188.patch64.49 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,312 pass(es). View
#188 interdiff.txt692 byteslarowlan
#185 render-xss-2273925.185.patch15.83 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,344 pass(es), 1 fail(s), and 0 exception(s). View
#185 interdiff.txt1.26 KBlarowlan
#179 render-xss-2273925.178.patch16.81 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,279 pass(es). View
#179 interdiff.txt515 byteslarowlan
#176 render-xss-2273925.176.patch17.03 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/rest/src/Tests/Views/StyleSerializerTest.php. View
#176 interdiff.txt855 byteslarowlan
#163 render-xss-2273925.163.patch17.22 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,210 pass(es). View
#163 interdiff.txt3.18 KBlarowlan
#159 ensure_markup_is_xss-2273925-159.patch16.81 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,921 pass(es). View
#159 interdiff.txt6.7 KBlauriii
#157 markup-xss-2273925.157.patch16.61 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,838 pass(es). View
#157 interdiff.txt2.01 KBlarowlan
#153 ensure_markup_is_xss-2273925-153.patch16.58 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,803 pass(es). View
#153 interdiff.txt900 byteslauriii
#151 interdiff.txt2.41 KBchx
#151 2273925_150.patch16.59 KBchx
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,805 pass(es). View

Comments

chx’s picture

Title: Discuss renaming the current #markup to #safe_markup » Passing in #markup to drupal_render is problematic
Issue summary: View changes
tim.plunkett’s picture

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']);
}
chx’s picture

Priority: Normal » Critical
Issue summary: View changes

Critical, non beta blocker per catch.

crowdcg’s picture

Status: Active » Postponed
xjm’s picture

Status: Postponed » Active
Cottser’s picture

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.

dawehner’s picture

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.

effulgentsia’s picture

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

aneek’s picture

@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

xjm’s picture

Issue tags: +SafeMarkup
aneek’s picture

FileSize
633 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,242 pass(es), 1,061 fail(s), and 71 exception(s). View

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.

aneek’s picture

Status: Active » Needs review
dawehner’s picture

Issue tags: +Needs tests

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

aneek’s picture

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’s picture

I know I'm the original OP but wrote this follow-up because we couldn't deal with it in the "Turn on Twig autoescape" issue's scope, but I'm not convinced this 'safe_markup' key is the right direction and it's just another #key to know how and when to use.

'markup' means it will contain markup, no constraints. I know from a security perspective this is horrible and from maintenance perspective it's a pain because the markup is written in PHP strings and is unstructured/flat representation. (even in D7).

We've been using #type => 'inline_template' so far to fix some of the SafeMarkup::set() issues, and though I don't like the DX there, I would love to see #markup become that, allowing the #context/#variables to exist and be escaped via the Twig engine. Just an idea I wanted to float by y'all:

$build = [
  '#markup' => '{{ title }}',
  '#variables' => ['title' => $title],
];

The DX seems simpler than inline_template(though it would map to the same), the data is escaped, and the raw data is still available. (Chose '#variables' instead of '#context' but could be swayed either way)

This proposal doesn't fix the security and may likely has performance issues. BUT if we want to still allow for '#markup', I don't think we can reasonably manage it's security issues it produces.

aneek’s picture

@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' => "

This is a test $var

", '#prefix' => '
', '#suffix' => '
', );

will become,

$var = "Hello";
$test_element = array(
      '#markup' => '

This is a test {{ var }}

', '#variables' => array('var' => $var), '#prefix' => '
', '#suffix' => '
', );

Or may be (another thought),

$var = "Hello";
$test_element = array(
      '#markup' => array('markup' => '

This is a test {{ var }}

', 'variables' => array('var' => $var)), '#prefix' => '
', '#suffix' => '
', );

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!

chx’s picture

Assigned: chx » Unassigned
aneek’s picture

FileSize
701 bytes
691 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,667 pass(es), 199 fail(s), and 0 exception(s). View

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!

aneek’s picture

Status: Needs work » Needs review
aneek’s picture

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

Help!!! :-)

joelpittet’s picture

@aneek sorry for the slow replies. The idea is to be as simple as possible, if the intent is to write markup using #markup, then there would be no need to use prefix/suffix.

#type => inline_template and #markup in my mind are synonymous with one another. Though I'm not sure everybody agrees and ideally we would get rid of #markup is the goal but it's so heavily used that it would be a chore and like not a favourable outcome.

My idea is to try to encourage more responsible use of #markup by passing in the variables they can be escaped. And a side benefit is that it's a bit less to type because we don't have to declare the #type with #markup, which makes it a bit simpler than inline_template.

I don't know maybe it's a crazy idea.

Re: #20 I'd prefer to call it directly but it bypasses render cache and alter hooks that way. So that is unlikely a thing we will be doing in core.

#markup is secure as we make it. We could still concatenate strings with variables into it... which could make it insecure. Or we can escape them before hand, or let twig do that for us. I'm pulling for the latter but it may not be the best idea and there may be better ideas out there...

The reason it's not the best is in my mind because we have to enforce a convention and we will likely as developers take the shortest route to solve a problem. Though because that approach looks shorter than #type inline_template, it may be easier to adopt.

Does that help clarify my proposal?

aneek’s picture

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

xjm’s picture

aneek’s picture

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' => '
{{ var }}
', '#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!

dawehner’s picture

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.

aneek’s picture

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

Any ideas?

cpj’s picture

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

aneek’s picture

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

Cottser’s picture

Issue tags: +Twig
Fabianx’s picture

Status: Needs work » Active

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'] = '' . $entity->title . '';

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?

Fabianx’s picture

Title: Passing in #markup to drupal_render is problematic » Ensure #markup is XSS escaped in Renderer::doRender()
Status: Active » Needs review
Issue tags: +Needs issue summary update
FileSize
681 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,666 pass(es), 1,168 fail(s), and 70 exception(s). View

Here is a patch, lets see if it works.

iMiksu’s picture

Assigned: Unassigned » iMiksu

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

iMiksu’s picture

Assigned: iMiksu » Unassigned
Status: Needs work » Needs review
FileSize
684 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,303 pass(es), 1 fail(s), and 0 exception(s). View
1.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,709 pass(es), 1,164 fail(s), and 70 exception(s). View

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

iMiksu’s picture

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

aneek’s picture

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

kim.pepper’s picture

Status: Needs work » Needs review
aneek’s picture

Status: Needs review » Needs work

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

Fabianx’s picture

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.

Berdir’s picture

mikey_p’s picture

FileSize
1.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,381 pass(es), 1,142 fail(s), and 70 exception(s). View

Re-roll for the changes to render tests.

effulgentsia’s picture

Status: Needs work » Needs review
mikey_p’s picture

Status: Needs review » Needs work

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.

Fabianx’s picture

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

mikey_p’s picture

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?

Fabianx’s picture

Status: Needs work » Needs review
FileSize
1.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,585 pass(es), 1,024 fail(s), and 59 exception(s). View
689 bytes

Lets see!

cbiggins’s picture

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

Fabianx’s picture

Status: Needs work » Needs review
FileSize
2.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,995 pass(es), 2,571 fail(s), and 121 exception(s). View
795 bytes

Contextual Fixed now ...

mikey_p’s picture

Status: Needs work » Needs review
FileSize
4.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,029 pass(es), 2,562 fail(s), and 121 exception(s). View
1.78 KB

Updated to fix failures in render tests with fake tags.

Fabianx’s picture

Assigned: Unassigned » effulgentsia

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

mikey_p’s picture

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

mikey_p’s picture

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.

Wim Leers’s picture

This actually reminds me of #1333730: [Meta] PHP DOM (libxml2) only understands XHTML4, misinterprets HTML5, but D8 must cope with 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.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned
Status: Needs work » Needs review
FileSize
4.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,785 pass(es), 1,014 fail(s), and 59 exception(s). View
1.39 KB
+++ 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.

Fabianx’s picture

Thanks effulgentsia!

That makes sense.

killua99’s picture

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

mikey_p’s picture

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.

mikey_p’s picture

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.

mikey_p’s picture

Status: Needs work » Needs review
FileSize
5.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,920 pass(es), 877 fail(s), and 58 exception(s). View
1011 bytes

Fix for views_pre_render_views_form_views_form().

effulgentsia’s picture

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?

mikey_p’s picture

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’s picture

Status: Needs work » Needs review
FileSize
4.16 KB
8.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,930 pass(es), 873 fail(s), and 58 exception(s). View
aneek’s picture

@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: &lt;em class="placeholder"&gt;pTKMZtsq&lt;/em&gt; &lt;em class="placeholder"&gt;&amp;lt;pTKMZtsq@example.com&amp;gt;&lt;/em&gt;.

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.

mikey_p’s picture

FileSize
83.35 KB

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.

Wim Leers’s picture

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

mikey_p’s picture

Status: Needs work » Needs review
FileSize
623 bytes
8.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,730 pass(es), 26 fail(s), and 0 exception(s). View

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.

aneek’s picture

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

Fabianx’s picture

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

Fabianx’s picture

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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
10.49 KB
3.29 KB

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

aneek’s picture

FileSize
2.79 KB
10.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,801 pass(es), 9 fail(s), and 0 exception(s). View

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.

aneek’s picture

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

lauriii’s picture

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

aneek’s picture

FileSize
806 bytes
10.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch ensure_markup_is_xss-2273925-95.patch. Unable to apply patch. See the log in the details link for more information. View

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

aneek’s picture

Status: Needs work » Needs review
Fabianx’s picture

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

joelpittet’s picture

@Fabianx I believe that is due to a change that says you need to wrap the markup as SafeMarkup before passing it through to format.
#2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument

I've been saying it's not the right way to go but seems not many agree.

dimaro’s picture

Status: Needs work » Needs review
FileSize
8.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,755 pass(es), 1,311 fail(s), and 1,132 exception(s). View

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

dimaro’s picture

Status: Needs work » Needs review
FileSize
10.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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

dimaro’s picture

Status: Needs work » Needs review
FileSize
2.88 KB
10.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,007 pass(es), 968 fail(s), and 1,121 exception(s). View

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

b0unty’s picture

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -94,7 +94,13 @@ public static function preRenderHtmlTag($element) {
+      $element['#markup'] = array(
+        '#type' => 'inline_template',
+        '#inline_template' => '<noscript> {{ markup }} </noscript>',
+        '#context' => array(
+          'markup' => $markup,
+        ),
+      );

this causes a hefty amount of exceptions as the #markup is expected to be a string

could probably be

          $template =  '<noscript> {{ markup }} </noscript>';
          $variables = array('markup' => $markup);
          $element['#markup'] = \Drupal::service('twig')->renderInline($template, $variables);
dimaro’s picture

Status: Needs work » Needs review
FileSize
10.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,965 pass(es), 961 fail(s), and 81 exception(s). View
897 bytes

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

dimaro’s picture

FileSize
897 bytes

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

joelpittet’s picture

There are probably many other things but this is just a quick pass on that last change. A style note, if you are changing an hunk that has an array in it, change the array style to the short syntax(but don't change arrays outside the hunk or it will introduce unnessasary noise and conflicts).

  1. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
    @@ -94,7 +94,9 @@ public static function preRenderHtmlTag($element) {
    -      $element['#markup'] = '<noscript>' . $markup . '</noscript>';
    +      $template =  '<noscript> {{ markup }} </noscript>';
    

    Before there was no space around the markup variable. And an extra space showed up after the = sign.

  2. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
    @@ -94,7 +94,9 @@ public static function preRenderHtmlTag($element) {
    +      $variables = array('markup' => $markup);
    

    Use array short syntax here. ['markup' => $markup] please.

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

    Periods at the ends of sentences.

  4. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
    @@ -81,6 +81,10 @@ public function providerTestRenderBasic() {
    +      'child' => ['#markup' => 'This is <script>alert(\'XSS\')</script> test'],
    +    ], 'This is alert(\'XSS\') test'];
    

    Use double quotes on the inside and avoid the unnecessary escaping.

dawehner’s picture

  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

larowlan’s picture

FileSize
4.48 KB
10.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,874 pass(es), 962 fail(s), and 81 exception(s). View

Fixes 115 and 112

larowlan’s picture

Status: Needs work » Needs review
effulgentsia’s picture

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.

larowlan’s picture

Assigned: Unassigned » larowlan

kicking tyres

larowlan’s picture

Assigned: larowlan » Unassigned
FileSize
773 bytes
10.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,692 pass(es), 9 fail(s), and 0 exception(s). View

Reverts 95, more tomorrow

larowlan’s picture

Status: Needs work » Needs review

perchance kind bot

lauriii’s picture

woot! Only 9 fails left :)

aneek’s picture

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

Cottser’s picture

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

aneek’s picture

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

larowlan’s picture

Status: Needs work » Needs review

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’s picture

FileSize
3.11 KB
13.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,783 pass(es), 6 fail(s), and 0 exception(s). View
aneek’s picture

FileSize
1.41 KB
13.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,843 pass(es), 5 fail(s), and 0 exception(s). View

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

aneek’s picture

Status: Needs work » Needs review
aneek’s picture

FileSize
1.34 KB
14.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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.

aneek’s picture

Status: Needs work » Needs review
aneek’s picture

FileSize
497 bytes
14.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,651 pass(es), 34 fail(s), and 2 exception(s). View

Ahh, code had a bug. Uploading again.

aneek’s picture

Status: Needs work » Needs review
aneek’s picture

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

larowlan’s picture

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

larowlan’s picture

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

aneek’s picture

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

chx’s picture

FileSize
25.54 KB

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.

aneek’s picture

FileSize
3.4 KB
14.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,803 pass(es), 8 fail(s), and 0 exception(s). View

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.
aneek’s picture

Status: Needs work » Needs review
aneek’s picture

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

chx’s picture

Status: Needs work » Needs review
FileSize
16.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,805 pass(es). View
2.41 KB
joelpittet’s picture

@chx nice, that is interesting, I did a very similar shuffle of that same remove link over here at one point in time: https://www.drupal.org/node/1963978#comment-9200453

And this related patch may conflict as it's fixing that JS to actually work for this VDC modal:
https://www.drupal.org/node/2168893#comment-9856001

+++ b/core/modules/views_ui/views_ui.theme.inc
@@ -147,21 +147,35 @@ function theme_views_ui_build_group_filter_form($variables) {
+      'default' => drupal_render($default),
...
+      'remove' => drupal_render($remove),

Can we avoid the explicit drupal_render() and put it as 'default' => ['data' => $default], and 'remove' => ['data' => $remove],

lauriii’s picture

FileSize
900 bytes
16.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,803 pass(es). View

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

joelpittet’s picture

Follow up for sure:) I treat those changes the same as short array syntax to avoid needless rerolls of other patches on unrelated changes. Changing them only in the changed hunks.

lauriii’s picture

Issue tags: -Needs tests

I think we have tests from #43

Cottser’s picture

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

larowlan’s picture

FileSize
2.01 KB
16.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,838 pass(es). View

fixes #156

joelpittet’s picture

Status: Needs review » Needs work

Ok going through this with a bit of a fineish tooth comb for what I can catch/grasp:

  1. +++ 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);
    

    Need to document this one still.

  2. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -315,6 +316,21 @@ public function testFieldapiField() {
    +    // Make sure that serialized fields are not exposed to XSS.
    +    unset($node);
    +    unset($result);
    +    $node = $this->drupalCreateNode();
    

    Are these unset()'s needed?

  3. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -315,6 +316,21 @@ public function testFieldapiField() {
    +    // @todo: find a proper solution & fix https://www.drupal.org/node/2477157
    

    Nit: No need for colon and yes to capital 'Find'.

  4. +++ 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();
    

    This change needed?

lauriii’s picture

Status: Needs work » Needs review
FileSize
6.7 KB
16.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,921 pass(es). View
lauriii’s picture

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

dawehner’s picture

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.

larowlan’s picture

Assigned: Unassigned » larowlan

poking

larowlan’s picture

Assigned: larowlan » Unassigned
FileSize
3.18 KB
17.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,210 pass(es). View

wow styleserializertest is so slow to run locally

larowlan’s picture

#163 fixes #161

dawehner’s picture

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

larowlan’s picture

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.

Wim Leers’s picture

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

dawehner’s picture

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

Wim Leers’s picture

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?

dawehner’s picture

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

Wim Leers’s picture

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

Can somebody please update the IS?

larowlan’s picture

Issue summary: View changes

Updated issue summary

aneek’s picture

Status: Needs review » Needs work
aneek’s picture

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!

larowlan’s picture

Assigned: Unassigned » larowlan
larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review
FileSize
855 bytes
17.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/rest/src/Tests/Views/StyleSerializerTest.php. View
larowlan’s picture

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?

larowlan’s picture

Status: Needs work » Needs review
FileSize
515 bytes
16.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,279 pass(es). View
dawehner’s picture

Status: Needs review » Needs work

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

larowlan’s picture

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.

dawehner’s picture

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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.26 KB
15.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,344 pass(es), 1 fail(s), and 0 exception(s). View

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.

larowlan’s picture

Failed because of double escaping

larowlan’s picture

Status: Needs work » Needs review
FileSize
692 bytes
64.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,312 pass(es). View
larowlan’s picture

FileSize
64.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,320 pass(es). View

forgot to merge

larowlan’s picture

FileSize
16.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,370 pass(es). View

wrong file again

Fabianx’s picture

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?

MiroslavBanov’s picture

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.

lauriii’s picture

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

dawehner’s picture

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.

MiroslavBanov’s picture

Cool, if you do so, otherwise nevermind

Pretty good summary, thanks :).

googletorp’s picture

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

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.05 KB
16.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,377 pass(es). View

Fixes 196

lauriii’s picture

Issue tags: +Needs change record

I think change record would be quite useful for this

dawehner’s picture

Added a quick one for SafeMarkup::replace()

lauriii’s picture

@dawehner++

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

dawehner’s picture

lauriii’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

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.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

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.

jaredsmith’s picture

Status: Needs work » Needs review
FileSize
17.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,377 pass(es). View
4.26 KB

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.

YesCT’s picture

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

jaredsmith’s picture

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

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

jaredsmith’s picture

Adding an interdiff between 206 and 208 for @YesCT.

YesCT’s picture

I read the interdiff. Those changes look great.

We are now going to work on the change record.

YesCT’s picture

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.

Cottser’s picture

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.

YesCT’s picture

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.

larowlan’s picture

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

larowlan’s picture

Assigned: Unassigned » larowlan

/me takes baton

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review
FileSize
1.21 KB
17.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,330 pass(es), 1 fail(s), and 0 exception(s). View

This gives us the piece of mind that escaping happens, without relying on the randomness of randomString() - which could throw out markup on some instances (eg &, < or >).

Passes locally.

larowlan’s picture

Status: Needs work » Needs review
FileSize
980 bytes
17.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,379 pass(es). View

should have checked the diff first, was too aggressive with cleanup - this is what I meant to submit

YesCT’s picture

Status: Needs review » Needs work

Thanks!

1.
We should still fix the confusing "false is TRUE" assert messages and make them mean something.


2.
+++ b/core/modules/views/src/Tests/Handler/AreaTest.php
@@ -92,9 +93,9 @@ public function testRenderArea() {
+    $header_string = '<script type="text/javascript">alert("boo");</script><p>' . $this->randomMachineName() . '</p>';
+    $footer_string = '<script type="text/javascript">alert("boo");</script><p>' . $this->randomMachineName() . '</p>';
+    $empty_string = '<script type="text/javascript">alert("boo");</script><p>' . $this->randomMachineName() . '</p>';

do these need to be unique? boo1, boo2, boo3?

YesCT’s picture

Merged the change record and deleted the unused one.
Added this issue to the related issues for that main change record.

Please,
check the merge at https://www.drupal.org/node/2296163/revisions/view/8472544/8473166

(Since we are changing and existing change record, to get it tweeted out, we can toggle "published" to trigger the actions to announce this when it goes in.)

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
17.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,375 pass(es). View

Fixes #220.1
220.2 - nope, don't need to be unique as $this->randomMachineName() ensures that.

larowlan’s picture

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

ok. I looked and everything @xjm asked for in #205 is done. so re-rtbc'ing

catch’s picture

Have given this a once over and only found one issue which is fixable on commit. Bit too early in the morning for me to want to commit this without another review, so I'll try to look again later if no-one's beaten me to it. Also think the DX of SafeMarkup::replace() is decent.

+++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
@@ -188,4 +188,73 @@ function testPlaceholder() {
+  public function testReplaces($search, $replace, $subject, $expected, $is_safe) {

Nit: should be testReplace()

jcnventura’s picture

Well, if you want to be pedantic, you can always convert the arrays to short syntax...

YesCT’s picture

I can go ahead and change the name now.

YesCT’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
760 bytes
17.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,489 pass(es). View

oh... sorry. I get it. It was a test name and only there in that one place. well, anyway, here it is.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

@YesCT's is what @catch suggested.

YesCT’s picture

@jcnventura Both array syntaxes are allowed right now. Until something is decided in #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8. So no need to change that here.

Fabianx’s picture

After looking it over again: RTBC + 1, great work everyone!

xjm’s picture

Assigned: Unassigned » xjm

Reviewing (again) on an airplane. Whee!

xjm’s picture

Assigned: xjm » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup

Sorry... I found more things. A lot of this is very much followup material, but some of it is docs gate, and some of it is about the robustness of the test coverage for Views specifically.

  1. +++ b/core/modules/views/src/Tests/Handler/AreaTest.php
    @@ -92,9 +93,9 @@ public function testRenderArea() {
    -    $header_string = $this->randomString();
    -    $footer_string = $this->randomString();
    -    $empty_string = $this->randomString();
    +    $header_string = '<script type="text/javascript">alert("boo");</script><p>' . $this->randomMachineName() . '</p>';
    +    $footer_string = '<script type="text/javascript">alert("boo");</script><p>' . $this->randomMachineName() . '</p>';
    +    $empty_string = '<script type="text/javascript">alert("boo");</script><p>' . $this->randomMachineName() . '</p>';
    
    @@ -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, Xss::filterAdmin($header_string)) !== FALSE, 'Views header output is sanitized');
    +    $this->assertTrue(strpos($output, Xss::filterAdmin($footer_string)) !== FALSE, 'Views footer output is sanitized');
    +    $this->assertTrue(strpos($output, Xss::filterAdmin($empty_string)) !== FALSE, 'Views empty output is sanitized');
    

    Well done on tracking down the random fail!

    I had a good solid think about whether it was correct to use randomMachineName() instead here, and I think it is. When it fails randomly, Views is (correctly) converting e.g. < to &lt;. (Related issue: #2487498: Make randomString always return a > to avoid random test fails.)

    The assertion messages are a definite improvement, as it also makes it explicit what the intent of the check is. That said, I think either they're not quite complete, or the inline comments around them in this test need to be updated.

    The original intent of the test was just to verify that each area got rendered properly. Now, we're implicitly adding that it should also check that it protects against XSS. Wouldn't it be better to test for the expected escaped string, rather than checking the results of Xss::filterAdmin()? And maybe expand the assertion messages (and the inline comment above them) to indicate we're testing both that it's sanitized to remove the script tag, and that it's rendered.
     

  2. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
    @@ -282,4 +282,51 @@ public static function placeholder($text) {
    +   * Replace all occurrences of the search string with the replacement string.
    

    Nit: Replaces.
     

  3. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
    @@ -282,4 +282,51 @@ public static function placeholder($text) {
    +    if (!is_array($replace)) {
    ...
    +      // If any replacement is unsafe, then the output is also unsafe, so just
    +      // return the output.
    

    Minor: Move me up above if (!is_array($replace)) { since it helps explain both the if and the else.
     

  4. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
    @@ -282,4 +282,51 @@ public static function placeholder($text) {
    +    // If we have reached this point, then all replacements were safe, and
    +    // therefore if the subject was also safe, then the entire output is also
    +    // safe, and should be marked as such.
    

    This should go down inside the final else now.
     

  5. +++ 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::format('<noscript>!markup</noscript>', array('!markup' => $markup));
         }
    

    Needs followup: the #noscript element is not documented at alllll.
     

  6. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -253,9 +253,8 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    -      $elements['#markup'] = SafeMarkup::set($elements['#markup']);
    +      $elements['#markup'] = SafeMarkup::checkAdminXss($elements['#markup']);
    

    So, here's a followup I'd like to see:

    It would seem sane to add documentation that #markup goes through checkAdminXss(), wherever we document #markup. Which appears to be in theme.api.php in the theme and render topic group, and took me ages to find. (I looked in ElementInfoManagerInterface::getInfo() and ElementInterface and Renderer and RendererInterface and several other places...) It says:

    #markup: Specifies that the array provides HTML markup directly. Unless the
     *   markup is very simple, such as an explanation in a paragraph tag, it is
     *   normally preferable to use #theme or #type instead, so that the theme can   
     *   customize the markup.
     *
    

    Is that... true and complete? Still even actually best practice? Should it mention, you know, templates and inline templates? Can we consolidate links to all the things and add references from the places I originally looked back to this topic? And shouldn't #markup be documented/mentioned somehow wherever the magic happens, rather than only in the topic documentation?

    In any case, followup aside, I think it's a docs gate thing to document that the #markup now goes through checkAdminXss(). So for this issue, we can presumably just start by updating that bullet in theme.api.php to mention this.

    Also maybe in scope for this issue: HtmlTag::preRenderHtmlTag() says:

    Note: It is the caller's responsibility to sanitize any input parameters. This callback does not perform sanitization.

    So, that callback still doesn't perform sanitization. True. But admin XSS filtering action still does happen later in the Renderer, I think? Can someone who really groks this confirm one way or another and suggest how the docs there should be updated?
     

  7. +++ b/core/modules/filter/src/Element/ProcessedText.php
    @@ -118,9 +119,11 @@ public static function preRenderText($element) {
    +    // Filtering and sanitizing have been done in
    +    // \Drupal\filter\Plugin\FilterInterface. Store the content in #markup,
    

    Also sorta contradicts the earlier thing about it being the caller's responsibility to sanitize. Maybe we should clarify exactly what filtering and sanitizing have been done, and what exactly is still the caller's responsibility? (Like, don't use #markup if you need it checkPlain()'ed, obviously.)
     

  8. +++ b/core/modules/views/src/Tests/Handler/AreaTest.php
    @@ -7,6 +7,7 @@
    +use Drupal\Component\Utility\Xss;
    

    Once, not too long ago, this use statement caused Views tests to explode with PHP crossing this with the Xss views plugin. It wasn't quite the same situation, but are we safe from that here? See #2420421: HEAD BROKEN: Fatal error: Cannot use Drupal\Component\Utility\Xss as Xss because the name is already in use in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/src/Plugin/views/field/Field.php on line 11. Or, this becomes moot if we remove the direct usage of Xss (see my first comment above).
     

  9. +++ b/core/modules/views_ui/views_ui.theme.inc
    @@ -147,21 +147,35 @@ function theme_views_ui_build_group_filter_form($variables) {
    +      '#title' => SafeMarkup::format('<span>@text</span>', ['@text' => t('Remove')]),
    +    ];
    +    $remove = [$form['group_items'][$group_id]['remove'], $link];
    ...
    +      'default' => ['data' => $default],
    ...
    +      'remove' => ['data' => $remove],
    

    I left @jaredsmith a tell about this, but did anyone ever get to that manual testing in Views UI to ensure there are no markup or escaping regressions for the remove link thingy as per my comment in #205?
     

  10. +++ 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'] . '',

    Non-actionable remark: I still haven't refreshed my memory on the whole history of these nor read all of RendererPostRenderCacheTest for context, but I got as far as checking that these tests with the <bar> tag came originally from #2324371: Fix common HTML escaped render #key values due to Twig autoescape and/or #2118703: Introduce #post_render_cache callback to allow for personalization without breaking the render cache (or those are related at least).
     

  11. Finally, I read over the updated change records. Great work; I think they are ready for this issue. A related followup I'd like to see is further consolidating the change records into one and/or at least referencing them as "see also". (E.g., SafeMarkup::format()'s vastly improved and de-fragilified (probably not a word) use of the tokens and how they behave with the SafeMarkup list.
     

Edits: typos and formatting.

xjm’s picture

jaredsmith’s picture

I did indeed do quite a bit of manual testing on this issue at the extented sprints at DrupalCon Los Angeles -- but it seems my comment didn't get saved.

On the advice of @joelpittet, I first looked at the Views UI without the patch installed. In particular, I looked at the People (Users) view, as it has a filtered group. I ensured that I was able to delete a filter (whether or not it was grouped), and that none of the text seemed to be double-escaped.

Next, I applied the patch (my patch from comment #208), and did the same thing -- namely, I looked at the People view UI, added a filter (grouped), removed it, and saw that I could remove other filters as well. I also looked for any text that was double-escaped, and couldn't find anything.

lauriii’s picture

Status: Needs work » Needs review
FileSize
18.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,595 pass(es). View
4.25 KB

I think I fixed points 1-4 from #233. I checked point 8 and it shouldn't be a problem because its a test and in that namespace there is no Xss class.

larowlan’s picture

Issue tags: -Needs followup
FileSize
19.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,840 pass(es). View

Follow-ups

So @lauriii did 1-4,8; I've done 5,6,11; @jaredsmith did 9; and 7 and 10 are non-actionable - I think we're good again here.

On #233.7 - the comment there is correct. As to is the comment on HtmlTag::preRenderHtmlTag - I've expanded the later to make it clear why 7 is correct.

larowlan’s picture

FileSize
1.93 KB
xjm’s picture

Thanks @lauriii, @larowlan, and @jaredsmith! That all looks great. Just a few comments:

  1. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
    @@ -46,7 +46,12 @@ public function getInfo() {
    +   * \Drupal\Component\Utility\Xss::filterAdmin. This is because it is marked
    +   * safe here, which causes \Drupal\Component\Utility\SafeMarkup::checkAdminXss
    +   * to regard it as safe and bypass the call to
    +   * \Drupal\Component\Utility\Xss::filterAdmin,
    

    Minor: Method names should have () and there is a comma at the end instead of a period.

  2. +++ b/core/modules/system/theme.api.php
    @@ -265,7 +265,12 @@
    + *   \Drupal\Component\Utility\XSS::filterAdmin which allows most tags with the
    ...
    + *   @see hook_theme
    

    Minor: parens after the function/method name.
     

  3. +++ b/core/modules/system/theme.api.php
    @@ -265,7 +265,12 @@
    + *   exception of script and style. If your markup needs these tags, then you
    

    Should <script> and <style> have their angle brackets? (api.d.o escapes it properly; see ImageStyleInterface::buildUrl() for example.)
     

  4. I think I fixed points 1-4 from #233. I checked point 8 and it shouldn't be a problem because its a test and in that namespace there is no Xss class.

    The other was a test too, with no direct indication of any other use of any other Xss class in its namespace. :( But I'm probably being paranoid just because the bug was so weird. Core usage suggests so:

    [mandelbrot:maintainer | Mon 06:00:58] $ grep -r "use Drupal\\\Component\\\Utility\\\Xss;" * | wc -l
          60
    [mandelbrot:maintainer | Mon 06:03:03] $ grep -r "use Drupal\\\Component\\\Utility\\\Xss as " * | wc -l
           2
    

     

  5. Any thoughts on:

    Wouldn't it be better to test for the expected escaped string, rather than checking the results of Xss::filterAdmin()?

    (The answer might be "No it would not be better".) :)
     

  6. For point 7 from #233, maybe the comment should say what exactly in FilterInterface did? (This might be out of scope; I can't remember right now.)
     
googletorp’s picture

FileSize
5.7 KB
20.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,042 pass(es). View

I addressed 1-3 from #239, not sure what to do about 4-6 if anything. Also fixed some array syntax, to use the short syntax instead of the old one.

xjm’s picture

Oh, regarding #233 point 10 and #205 point 7, what I'm asking myself there is whether the test is still providing the intended coverage when we are replacing first one and then the other arbitrary tags (<foo> and <bar>) with actual tags defined in HTML5. So others can feel free to investigate and opine on that as well. :)

Thanks @googletorp for the updated patch! For 4-6 I think more feedback is needed.

Also fixed some array syntax, to use the short syntax old.

We really should actually avoid doing this when updating an existing array, even when we are otherwise changing the line. I often review with a word diff and converting array syntax adds irrelevant stuff to the diff. (This is also part of why in #205 I asked for manual testing of the bit in the Views UI.)

If others have used array() for new lines in a patch, that's okay and we don't need to change their preference with #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8 still allowing both. And when we're changing lines in HEAD, we should make the minimum change possible.

I do personally prefer the short syntax and the added uses here are okay I guess. It's more about scope management and reviewability in general. :)

googletorp’s picture

@xjm Thanks for clearing up array() vs []. I've had quite a few patches bumped from RTBC to needs work due to old array syntax, so I thought that short array syntax was required in new patches for consistency or something.

xjm’s picture

Issue summary: View changes

Thanks @googletorp! Sorry to hear about patches being bumped back for the syntax; that's part of what I want to avoid for sure. If you see a patch set to NW for that in the future, you could suggest the reviewer talk to me, alexpott, etc. for clarification. Hopefully the policy issue link is a useful resource there (although it adds its own problems since it's still a subject of debate).

I'm updating the summary with the remaining tasks after @googletorp's updated patch.

xjm’s picture

Issue summary: View changes
xjm’s picture

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -48,10 +48,10 @@ public function getInfo() {
+   * safe here, which causes \Drupal\Component\Utility\SafeMarkup::checkAdminXss()
    * to regard it as safe and bypass the call to

Oh, looks like we need to rewrap the comment here since this is now over 80 characters.

larowlan’s picture

Assigned: Unassigned » larowlan

addressing open items

larowlan’s picture

FileSize
3.96 KB
21.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,068 pass(es), 1 fail(s), and 0 exception(s). View

#246 fixed
#233.10 and #205.7 the bar isn't markup, it's a post_render_cache context. See \Drupal\Tests\Core\Render\PostRenderCache::placeholder - the intention is the context is embedded inside lt;code> tags.
#239.5 - added assert to ensure script tag was escaped
#239.6 - after reading this I'm not convinced we should be marking the result of the filter as safe - just because it has been through the OO equivalent of check_markup doesn't mean its safe - consider an input format with no html filter (ala Full HTML). So I added some extra logic there - this may break tests. But it may also allow me to tighten one of my biggest gripes with the standard install profile - the full html input format.

larowlan’s picture

Assigned: larowlan » Unassigned
larowlan’s picture

+++ b/core/modules/filter/src/Element/ProcessedText.php
@@ -111,19 +111,35 @@ public static function preRenderText($element) {
     $metadata = BubbleableMetadata::createFromRenderArray($element);
+    // Track if any of the filters apply HTML restrictions.
+    $html_restricted = FALSE;
     foreach ($filters as $filter) {
       if ($filter_must_be_applied($filter)) {
         $result = $filter->process($text, $langcode);
+        // Keep track if HTML restrictions have been applied.
+        $html_restricted = $html_restricted || (bool) $filter->getHTMLRestrictions();
         $metadata = $metadata->merge($result);
         $text = $result->getProcessedText();
       }
     }
 
     // Filtering and sanitizing have been done in
-    // \Drupal\filter\Plugin\FilterInterface. Store the content in #markup,
+    // \Drupal\filter\Plugin\FilterInterface::process(). Each applicable filter
+    // has been applied in turn. Store the content in #markup,
     // set the updated bubbleable rendering metadata, and set the text format's
-    // cache tag.
-    $element['#markup'] = SafeMarkup::set($text);
+    // cache tag, but only mark the result as safe if at least one of the
+    // filters have applied HTML restrictions.
+    if ($html_restricted) {
+      $element['#markup'] = SafeMarkup::set($text);
+    }
+    else {
+      // None of the filters applied to the text indicated that HTML
+      // restrictions have been applied. To prevent marking an unsafe string as
+      // safe, we err on the side of caution - the renderer will run the given
+      // text through \Drupal\Component\Utility\Xss::filterAdmin() and strip
+      // dangerous tags.
+      $element['#markup'] = $text;
+    }

So this is why the test fails.

But I don't think we can say that just the text is safe because the text has been filtered via the configured filter format. What if my filter format is Full html? Does that guarantee its safe? Or do we assume the user knows what they're doing?

If so, reverse that change.

If not, we've got issues, because post render cache callbacks are stripped otherwise.

Wim Leers’s picture

after reading this I'm not convinced we should be marking the result of the filter as safe - just because it has been through the OO equivalent of check_markup doesn't mean its safe - consider an input format with no html filter (ala Full HTML). So I added some extra logic there - this may break tests. But it may also allow me to tighten one of my biggest gripes with the standard install profile - the full html input format.

Indeed. And even the presence of a HTML-restricting filter does NOT imply the resulting HTML can be considered safe either.

Berdir’s picture

I think that's exactly what it should mean?

After a text has been checked with a text format, it *must* be printed as-is and should be considered safe. If the text format is misconfigured, so be it, there's no way for us to know if that's the case or not. Even a very simple text format might have some sort of placeholders/token that will result in all kind of HTML, javascript and so on, and that needs to be considered safe?

If a user has access to the full html text format and decides to use it, then everything is allowed. Whether that user should be able to use that text format or not is a completely different question.

Wim Leers’s picture

If a user has access to the full html text format and decides to use it, then everything is allowed. Whether that user should be able to use that text format or not is a completely different question.

Haha, you're right, of course.

Text formats/processed text deals with user input. There is no automagic way to guarantee safety. Some filters do some level of HTML restrictions. But you still cannot know if that just means "disallowing the <marquee> tag" or "disallowing all unsafe input". Your site's configuration of text formats reflects which users you trust blindly, and which you don't. But in the end of the day, what comes out of the filter system, is what must be displayed, without further post-processing.

(I answered #252 purely from a "safe markup" POV, which text formats absolutely do not guarantee 100%, because the site builder can surely change the configuration of text formats which would make them unsafe. But that, as Berdir points out, is a completely different question.)

larowlan’s picture

OK so we can reverse that hunk and use @berdirs comment as basis for documenting the new safe markup set call, will do this tomorrow

larowlan’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.29 KB
20.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,183 pass(es). View

Does #255 as promised, think this is ready again

Updating remaining tasks

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed again, back to RTBC!

lauriii’s picture

RTBC++

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, but I think I need to kick this back.

  1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -253,9 +253,8 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +      $elements['#markup'] = SafeMarkup::checkAdminXss($elements['#markup']);
    

    Hrm, doesn't this mean that certain HTML tags are now effectively impossible to render? I.e. any tag that is not in \Drupal\Component\Utility\Xss::$adminTags?

    A simple example would be custom HTML tags. I'd like to see a test disproving me :)

  2. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
    @@ -284,7 +284,7 @@ public function execute() {
    -    $response = new CacheableResponse($this->renderer->renderRoot($output), 200);
    +    $response = new CacheableResponse($output['#markup'], 200);
    

    Why this change? In the old code, #post_render_cache callbacks were being executed. Not anymore now. Or is a renderRoot() call already happening elsewhere?

  3. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
    @@ -284,7 +284,7 @@ public function execute() {
    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 ba3ca6f..33f2fc7 100644
    
    index ba3ca6f..33f2fc7 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;
    

    Unnecessary changes.

  4. +++ b/core/modules/system/theme.api.php
    @@ -262,10 +262,16 @@
    + *   can customize the markup. Note that the value is passed through
    + *   \Drupal\Component\Utility\XSS::filterAdmin() which allows most tags with
    + *   the exception of <script> and <style>. If your markup needs these tags,
    + *   then you should implement a theme hook and template file and/or an asset
    + *   library.
    

    This is only part of the answer to my first question.

    Also: this text is misleading, because it suggests a blacklist is used, but actually a whitelist is used.

  5. +++ b/core/modules/views/src/Tests/Handler/AreaTest.php
    @@ -92,9 +93,9 @@ public function testRenderArea() {
         // Insert a random string to the test area plugin and see whether it is
         // rendered for both header, footer and empty text.
    -    $header_string = $this->randomString();
    -    $footer_string = $this->randomString();
    -    $empty_string = $this->randomString();
    +    $header_string = '<script type="text/javascript">alert("boo");</script><p>' . $this->randomMachineName() . '</p>';
    +    $footer_string = '<script type="text/javascript">alert("boo");</script><p>' . $this->randomMachineName() . '</p>';
    +    $empty_string = '<script type="text/javascript">alert("boo");</script><p>' . $this->randomMachineName() . '</p>';
    

    The comment is now wrong.

Fabianx’s picture

#259:

1. Use:

$element['#markup'] = SafeMarkup::format('I really need those tags and I am too lazy to write a template, however I know it is safe.');

OR an inline template.

2. Yes, I am still uncomfortable with that as well - but there was lots of discussions and a test added already, given we push back the data via a response, we should as well be able to SafeMarkup:: set() before calling renderRoot() - the end result is the same, the markup is pushed verbatim to the user.

3. Can be fixed :)

4. Yeah, should be improved. I think for the 99% case #markup is not needed. If you need '' or '

' need to do some work :). 5. Oh, nice find :).
lauriii’s picture

Status: Needs work » Needs review
FileSize
19.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,189 pass(es), 3 fail(s), and 0 exception(s). View
2.95 KB

#259.1: Isn't it enough that it is possible to set markup as safe in the parts where someone wants to use non-whitelisted html element? That is not the right way because templates should be used anyway but I don't think there is better way.

Wim Leers’s picture

#260.1/#261: thanks, that makes sense.

#261 changed #259.2, but #260.2 suggests that this is intentional. So… I'm confused now about what it is that we want here.

Fabianx’s picture

+++ a/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -284,7 +284,7 @@
+    $response = new CacheableResponse($this->renderer->renderRoot($output), 200);
-    $response = new CacheableResponse($output['#markup'], 200);

For tests to pass need:

$output['#markup'] = SafeMarkup::set($output['#markup']);

before this.

lauriii’s picture

Status: Needs work » Needs review
FileSize
20.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,234 pass(es), 50 fail(s), and 59 exception(s). View
593 bytes

Status: Needs review » Needs work

The last submitted patch, 265: ensure_markup_is_xss-2273925-265.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
20.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,272 pass(es). View
729 bytes

Now this should pass

xjm’s picture

Assigned: Unassigned » xjm

Fixing a few grammar things.

xjm’s picture

Assigned: xjm » Unassigned
FileSize
20.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,275 pass(es). View
3.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Attached cleans up a few comments. Mostly just grammar polish except for this:

+++ b/core/modules/filter/src/Element/ProcessedText.php
@@ -118,9 +119,15 @@ public static function preRenderText($element) {
+    // Filtering and sanitizing have been done in
+    // \Drupal\filter\Plugin\FilterInterface. Store the content in #markup,
+    // set the updated bubbleable rendering metadata, and set the text format's
+    // cache tag.
+    // Whilst we cannot guarantee that $text is safe, after it has passed
+    // through the filter system and has been checked with a text format it
+    // must be printed as-is and hence needs to be marked as safe to avoid being
+    // escaped again.
+    $element['#markup'] = SafeMarkup::set($text);
     $metadata->applyTo($element);
     $element['#cache']['tags'] = Cache::mergeTags($element['#cache']['tags'], $format->getCacheTags());

So, this is subtle, but I don't think we should ever say we use SafeMarkup::set() to "avoid [text] being escaped again" as that is exactly what we don't want other code to do. I've rewritten the comment in the attached patch to clarify.

xjm’s picture

I rolled my eyes at myself and cancelled the test run for my interdiff. However, I was going to set this to NW anyway for this:

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -284,6 +284,7 @@ public function execute() {
     $header = [];
     $header['Content-type'] = $this->getMimeType();
 
+    $output['#markup'] = SafeMarkup::set($output['#markup']);
     $response = new CacheableResponse($this->renderer->renderRoot($output), 200);
     $cache_metadata = CacheableMetadata::createFromRenderArray($output);
 

Hmm, I'm not keen on adding another stray SafeMarkup::set() call in Views, nor another one so far away from where the markup is composed. How do we know this markup is safe? Looking in the render() method on this class, the output is checkPlain()ed stuff wrapped in a <pre> tag if it's a live preview, but what about when it's actually being rendered?

At a minimum, we need to document why the call here is safe, but it'd be better to find a way to remove it. (That's the parent critical, after all.)

dawehner’s picture

Hmm, I'm not keen on adding another stray SafeMarkup::set() call in Views, nor another one so far away from where the markup is composed. How do we know this markup is safe? Looking in the render() method on this class, the output is checkPlain()ed stuff wrapped in a

 tag if it's a live preview, but what about when it's actually being rendered?

At a minimum, we need to document why the call here is safe, but it'd be better to find a way to remove it. (That's the parent critical, after all.)
Oh yes, document why this is needed. What about confused me here though is the code which follows that bit of change:
    $response = new CacheableResponse($output, 200);

... in other words, we are not dealing with HTML nor with twig templates. In this case we have already constructed the entire HTML, so why care about it? I totally agree that we need some explanation here.

Fabianx’s picture

#271:

What we before was doing was:

$response = new CacheableResponse($output['#markup'], 200);

By doing so we implicitly trusted #markup and send it to the browser. (that is similar to marking it as safe in effect).

Therefore the explicit SafeMarkup::set() is way better already. (as we make explicit, which we implicitly assumed before).

Also renderRoot() will in the near future return a cacheable render array, so not calling the renderer - even though it is markup is wrong in several ways.

xjm’s picture

By doing so we implicitly trusted #markup and send it to the browser. (that is similar to marking it as safe in effect).

Therefore the explicit SafeMarkup::set() is way better already. (as we make explicit, which we implicitly assumed before).

But why would we do this for a response that isn't even being handled for Twig?

Here was the test failure this was intended to fix:
https://qa.drupal.org/pifr/test/1057283

The expected XML output was found.	Other	StyleSerializerTest.php	158	Drupal\rest\Tests\Views\StyleSerializerTest->testSerializerResponses()	
The expected XML output was found.	Other	StyleSerializerTest.php	178	Drupal\rest\Tests\Views\StyleSerializerTest->testSerializerResponses()	
Value '<p>iIt7mL0lRJvYQx4FKJb2yuPJJBN8AeYc&lt;/p&gt; ' is equal to value '<p>iIt7mL0lRJvYQx4FKJb2yuPJJBN8AeYc</p> '.	Other	StyleSerializerTest.php	392	Drupal\rest\Tests\Views\StyleSerializerTest->testFieldapiField()

Which... I'm worried that the closing tag for that paragraph is escaped but not the first one; what's going on with that? Also, we should figure out if this is being escaped in the parent environment or the Views preview or what, because I still feel like this SafeMarkup::set() should not be added.

Fabianx’s picture

#274 This is likely due to how Xss::filterAdmin() works.

Twig is not involved, but the renderRoot() sees #markup, sees its not marked as safe and runs Xss::filterAdmin() on it.

xjm’s picture

Issue summary: View changes
dawehner’s picture

Twig is not involved, but the renderRoot() sees #markup, sees its not marked as safe and runs Xss::filterAdmin() on it.

I was honestly NEVER a fan of abusing the render API here. We already set #markup below, no #attachements will be bubbled etc.
#2381277: Make Views use render caching and remove Views' own "output caching" thought will use the renderRoot() call in order to use render caching here.

Given that explanation, a SafeMarkup::set() calls seems to be the semantically right thing to do, we know its safe output, and not intended to be used for the browser,
or at least not intended to be used as HTML.

xjm’s picture

Issue summary: View changes

So the issue summary says:

3. Stop double rendering rest (xml/json) resources

Did that get reverted or removed from the patch? I don't see any changes in the patch that seem to be doing that. Is that was reverted based on #260 and following? And that sounds like it would have bearing on the REST plugin output issue.

A question for @dawehner:

I was honestly NEVER a fan of abusing the render API here. We already set #markup below, no #attachements will be bubbled etc.
#2381277: Make Views use render caching and remove Views' own "output caching" thought will use the renderRoot() call in order to use render caching here.

Abusing the render API in what sense? I'm not sure if this means the one renderRoot() call in the Views plugin or the entire approach for filtering #markup in this patch. :) If SafeMarkup::set() is the correct approach with the current state of HEAD for this patch, is there a followup we could file, something posptoned on #2381277: Make Views use render caching and remove Views' own "output caching", that would get rid of it and address your concern here?

In general it seems weird to me that SafeMarkup should have any bearing on output that never even is going to see a Twig template. (I also worried for a moment about the memory overhead being added to REST calls. OTOH any translated string is already going to be added to the SafeMarkup list anyway, so that's hardly new here.)

At a minimum though we still for sure need a big long comment explaining it (and referencing a relevant followup if there is one). Updated the remaining tasks to clarify that.

dawehner’s picture

If you look at \Drupal\rest\Plugin\views\display\RestExport::render and \Drupal\rest\Plugin\views\display\RestExport::execute
basically everything which is done is the following:

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

Well you know, we can just add it, but add a todo

xjm’s picture

So I discussed this at length with @WimLeers and @dawehner.

  1. In general, "normal" REST responses (for entities) do not interact with the render API.
     
  2. In Views, following #1938380: Let ViewExecutable->preview() return a render array, the Views output is built as a render array for the live preview, thus supporting both eventual output in an HTML page (in the Views UI) and the primary output as serialized JSON or whatever for the REST response.
     
  3. There also has not been much work done on cacheability for REST responses so far; the focus is rightly on HTML responses.
     
  4. However, Views does already support caching for Views output. In #2477157: rest_export Views display plugin does not set necessary cache metadata, it was discovered that the caching for it was never being invalidated. I'm unclear on whether the never-invalidated caching was because of the use of a render array and the render API (introduced for #1938380: Let ViewExecutable->preview() return a render array), or if the caching was happening elsewhere and we just attached caching data to the existing render array to fix it. I think it's the latter. @dawehner said it's more related to the page cache, which is implemented on the Request/Response level, and before when it was only a Response rather than a CacheableResponse, it'd only get invalidated on cache expiration rather than on any changes to the data in the view (violating the expectations of D8 cacheability and generally not behaving like a view should for its cache tags).
     
  5. In general, Views does not expect any particular kind of output, but using the render API to integrate caching is useful. @dawehner says this will also be leveraged in #2381277: Make Views use render caching and remove Views' own "output caching".
     
  6. In this issue, we're adding behavior that (sanely) expects a render array's #markup element to be, well, markup, and sanitizing it as such.
     
  7. However, we should support render arrays for non-markup as well. This should be a followup issue: "Discuss how to support non-HTML output in the render API".
     
  8. Regarding the semantics of using SafeMarkup::set() there: It's "safe" markup in the sense that it's not "markup" (except in the case of the Views preview). Since there's no markup, there's no markup to inject XSS into. :P (The preview is checkPlain()ed but I think that's irrelevant for the SafeMarkup::set() call because I think DisplayPluginInterface::execute() and DisplayPluginInterface::preview() are mutually exclusive.)
     

So @dawehner and I agreed that for now, a SafeMarkup::set() call is acceptable for this issue to skip the filtering in doRender(), and we should add it with an @todo linking the followup issue mentioned above.

Phew!

Edit: reformatting for more readability.

larowlan’s picture

Issue summary: View changes

We're not double rendering rest exports anymore, so removed that line

dawehner’s picture

Thank you xjm to write things down properly!
So we now need someone documenting the new call, due to the great writeup in #280

xjm’s picture

Issue summary: View changes

Just adding that to the remaining tasks. Thanks @larowlan and @dawehner.

xjm’s picture

Issue summary: View changes
lokapujya’s picture

lauriii’s picture

Issue summary: View changes
lokapujya’s picture

Assigned: Unassigned » lokapujya
joelpittet’s picture

Issue tags: +Needs reroll
+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -94,7 +99,7 @@ public static function preRenderHtmlTag($element) {
-      $element['#markup'] = '<noscript>' . $markup . '</noscript>';
+      $element['#markup'] = SafeMarkup::format('<noscript>!markup</noscript>', ['!markup' => $markup]);

This should be @ instead of ! since it's dealing with markup.

We've been trying to re-roll this but many failures on RendererPlaceholdersTest are holding us back a bit. Most of which came from this change it seems #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache

xjm’s picture

Re: #288 and @ vs. !, what if someone wants to use the API to render a <script> tag? The function already carries a warning that it is not sanitized. (This goes back to the discussion about reverting that change to !.)

joelpittet’s picture

@xjm re #289 from that other issue we can now do the same as @ with !. The only real difference is that if the variable is unsafe going in will change the output of the whole string. If they both have safe strings going in they will produce the exact same output.

So in general you should use @ always. May I misinterpreted your question? Do you have some example code?

#2399037-41: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument

lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
17.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,881 pass(es), 1,708 fail(s), and 11 exception(s). View

Rerolled, but this will have a lot of fails. Somehow, the placeholders patch is interfering with XSS escaping. Possibly seeing the errors will help figure out what is conflicting.

joelpittet’s picture

FileSize
17.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,884 pass(es), 1,708 fail(s), and 11 exception(s). View
821 bytes

Here is that change I mentioned in #288 against #291 Should fail the same from that lazy_builder stuff.

xjm’s picture

@joelpittet, yeah, my previous comment in #289 was kinda nonsense. :P Sorry about that. Obviously there's no sane usecase to put a <script> tag inside a <noscript>. I think I will sleep before trying to remember or explain what it was I was thinking. :)

The last submitted patch, 291: 2273925-291.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 292: ensure_markup_is_xss-2273925-292.patch, failed testing.

larowlan’s picture

Assigned: lokapujya » larowlan

So it looks like drupal_set_message is broken in the tests, digging in

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review
FileSize
845 bytes
18.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,717 pass(es), 6 fail(s), and 0 exception(s). View

the placeholder markup is always safe

Status: Needs review » Needs work

The last submitted patch, 297: 2273925-render-xss.297.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
19.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,738 pass(es). View
joelpittet’s picture

Issue tags: +D8 Accelerate
FileSize
19.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,720 pass(es). View
976 bytes

Thank you @lauriii and @larowlan for resolving that.

@lokapujya and I are reviewing this in depth today at the sprint.

I've added a test to make sure we know we are getting a simple strong tag in #markup still because we noticed there is no other checks for tags in #markup.

xjm’s picture

Status: Needs review » Needs work

Thanks @lauriii and @larowlan.

  1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -612,7 +612,10 @@ protected function createPlaceholder(array $element) {
    -    $placeholder_element['#markup'] = $placeholder_markup;
    +    // A placeholder should not be removed by SafeMarkup::checkAdminXss(), so
    +    // mark it as safe markup. We have complete control over its generation so
    +    // know it is safe.
    +    $placeholder_element['#markup'] = SafeMarkup::set($placeholder_markup);
    

    My concern about this is that we're not only marking this placeholder as safe for generation here; we're also saying it's something someone can enter somewhere else and not get sanitized. And possibly even cause whatever the placeholder turns into to be rendered in their field!? I feel like a correct fix would be to change how the render API assembles the placeholders into the string.

    In general, when we added the SafeMarkup API, we said the only way it could really work would be if the safe markup list did not get individual tags added to it, and I feel like the placeholder is similar to that.

    We might be able to move that to a followup (since the placeholder itself is known safe and it would unblock other work here), but I'm afraid it would also be a critical followup.

  2. +++ b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
    @@ -82,7 +83,7 @@ public function providerPlaceholders() {
    -      '#markup' => $generate_placeholder_markup(),
    +      '#markup' => SafeMarkup::set($generate_placeholder_markup()),
    
    @@ -151,7 +152,7 @@ public function providerPlaceholders() {
    -    $x['#markup'] = $generate_placeholder_markup($keys);
    +    $x['#markup'] = SafeMarkup::set($generate_placeholder_markup($keys));
    

    I thought about it a bit and am thinking adding SafeMarkup::set() to the unit test is okay if we add the other call in the Renderer, because (a) it's a unit test and (b) it mimics an API call in the thing it's testing. It needs a comment documenting the use though, and if we end up creating a followup issue, a todo linking to it just like the main call in the Renderer.

  3. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
    @@ -84,6 +84,10 @@ public function providerTestRenderBasic() {
    +    // Ensure non XSS tags are not filtered out.
    

    Nitpick: "non-XSS" (with a hyphen). :) +1 for this test!

I'll ping a couple people for more feedback on #1.

joelpittet’s picture

This may be a bit clearer on how it's safe than that comment was showing and we don't have to write SafeMarkup::set() here.

Fixes 301.1 and 301.3

joelpittet’s picture

FileSize
20.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,733 pass(es). View
2.23 KB

So that was weird... I some how posted the files not to the comment... try again.

lokapujya’s picture

FileSize
19.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,712 pass(es). View
1014 bytes

Removing another SafeMarkup::Set().

lauriii’s picture

Status: Needs review » Needs work

For #302.2

xjm’s picture

Awesome work on those!

+++ b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
@@ -82,7 +83,7 @@ public function providerPlaceholders() {
-      '#markup' => $generate_placeholder_markup(),
+      '#markup' => SafeMarkup::set($generate_placeholder_markup()),

@@ -151,7 +152,7 @@ public function providerPlaceholders() {
-    $x['#markup'] = $generate_placeholder_markup($keys);
+    $x['#markup'] = SafeMarkup::set($generate_placeholder_markup($keys));

Are we good to revert these now too?

effulgentsia’s picture

This patch looks great! Awesome work! #306 can be resolved by replacing with a SafeMarkup::format() inside the $generate_placeholder_markup() function. Also:

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -284,6 +284,7 @@ public function execute() {
+    $output['#markup'] = SafeMarkup::set($output['#markup']);

Needs a comment (with some of the info from #280.8) and link to #2501313: Discuss how to support non-HTML output in the render system.

Other than that, this looks ready to me.

larowlan’s picture

Assigned: Unassigned » larowlan
Status: Needs work » Needs review
FileSize
1.77 KB
19.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
larowlan’s picture

Assigned: larowlan » Unassigned
FileSize
1.04 KB
20.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Fixes 307, think we're close here

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.42 KB
20.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,721 pass(es). View

Following feedback/discussion on IRC with @xjm

larowlan’s picture

Issue summary: View changes

Updating remaining tasks to None to reflect the fact that we're back to needs review or possibly rtbc again.

xjm’s picture

Issue tags: -Needs followup

Also all the followups are filed. :)

effulgentsia’s picture

FileSize
20.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,736 pass(es). View
1.82 KB

How about this doc change? If this isn't ok, I'm ok with #312 committed instead and us working it out in the followup.

joelpittet’s picture

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -284,13 +284,20 @@
+    if ($this->view->getRequest()->getFormat($header['Content-type']) !== 'html') {

I like this but isn't it text/html?

larowlan’s picture

No, the request format will extract the format from the mime-type - e.g. this data provider for the unit test

public function getFormatToMimeTypeMapProvider()
    {
        return array(
            array(null, array(null, 'unexistent-mime-type')),
            array('txt', array('text/plain')),
            array('js', array('application/javascript', 'application/x-javascript', 'text/javascript')),
            array('css', array('text/css')),
            array('json', array('application/json', 'application/x-json')),
            array('xml', array('text/xml', 'application/xml', 'application/x-xml')),
            array('rdf', array('application/rdf+xml')),
            array('atom',array('application/atom+xml')),
        );
    }
effulgentsia’s picture

Assigned: Unassigned » xjm
Status: Needs review » Reviewed & tested by the community

Since #315 is a small change, and given #316 and #317, I think I'm eligible to RTBC, so doing so. Assigning to @xjm for commit so she can verify all her concerns have been addressed.

dawehner’s picture

I like the doc change in #315!

xjm’s picture

FileSize
20.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,679 pass(es). View
1.97 KB

I reviewed this ONE LAST TIME and everything is looking really great. Some specific notes:

  1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
    @@ -284,6 +284,20 @@ public function execute() {
    +    if ($this->view->getRequest()->getFormat($header['Content-type']) !== 'html') {
    +      // This display plugin is primarily for returning non-HTML formats.
    +      // However, we still invoke the renderer to collect cacheability metadata.
    +      // Because the renderer is designed for HTML rendering, it filters
    +      // #markup for XSS unless it is already known to be safe, but that filter
    +      // only works for HTML. Therefore, we mark the contents as safe to bypass
    +      // the filter. So long as we are returning this in a non-HTML response
    +      // (checked above), this is safe, because an XSS attack only works when
    +      // executed by an HTML agent.
    +      // @todo Decide how to support non-HTML in the render API in
    +      //   https://www.drupal.org/node/2501313.
    +      $output['#markup'] = SafeMarkup::set($output['#markup']);
    +    }
    

    This conditional statement is more reliable and the documentation is excellent!

  2. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -390,6 +390,16 @@ public function testFieldapiField() {
    +    $this->assertEqual($result[1]['nid'], $node->id());
    

    +1 for adding this assertion; it's always good to provide one like this before a negative assertion. (The assertTrue() following is a negative assertion because it's asserting true that it's exactly false. assertFalse() would be incorrect because that would match 0 for strpos().)

  3. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -390,6 +390,16 @@ public function testFieldapiField() {
    +    $this->assertTrue(strpos($this->getRawContent(), "<script") === FALSE, "The Raw page contents are escaped.");
    

    I had to squint at this assertion a second because 1. It's asserting that it's true that something is exactly false. 2. The docs for getRawContent() are on the less helpful side.

    I think a fix is to make the assertion message a little more specific and thus the assertion more self-documenting. Done in attached.

  4. +++ b/core/modules/system/theme.api.php
    @@ -262,10 +262,16 @@
    + *   can customize the markup. Note that the value is passed through
    + *   \Drupal\Component\Utility\XSS::filterAdmin() which allows tags that are
    + *   known to be safe. I.e <script> and <style> are not allowed. If your markup
    

    I'm going to reroll to improve this slightly. The casing in the class name is wrong, we can link directly to the list of allowed tags, and clarify a little more. Attached.

I'm still comfortable with committing this since my updates here and back in #269 were small docs updates, but I would like a teeny review or signoff on my change here first. Leaving at RTBC though to emphasize that I think it's still RTBC either way...

xjm’s picture

+++ b/core/modules/system/theme.api.php
@@ -266,10 +266,13 @@
+ *   vectors. (I.e, <script> and<style> are not allowed.) See

augh missing space. THAT I'll fix on commit. ;)

joelpittet’s picture

A bit better comments thanks @xjm, I'm signing off save for the the space you can fix on commit:)

  1. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -400,6 +400,6 @@ public function testFieldapiField() {
    -    $this->assertTrue(strpos($this->getRawContent(), "<script") === FALSE, "The Raw page contents are escaped.");
    +    $this->assertTrue(strpos($this->getRawContent(), "<script") === FALSE, "No script tag is present in the raw page contents.");
    

    +1 better description.

  2. +++ b/core/modules/system/theme.api.php
    @@ -266,10 +266,13 @@
    + *   vectors. (I.e, <script> and<style> are not allowed.) See
    

    needs a space before <style>.

Cottser’s picture

I just gave this a once-over as well. Looks great, thanks all!

  • xjm committed 2f903e4 on 8.0.x
    Issue #2273925 by larowlan, aneek, lauriii, mikey_p, joelpittet, dimaro...
xjm’s picture

Assigned: xjm » Unassigned
Status: Reviewed & tested by the community » Fixed

Alright, committed and pushed to 8.0.x!

https://twitter.com/xjmdrupal/status/607620233671659520

Thanks everyone for your sharp eyes, sharp thinking, and perseverance on this issue.

willzyx’s picture

+1 on doing this but currently are there alternatives for insert markup with css/js through the render api? variables pretty print in Devel (Krumo, Kint) is broken (see #2503591: (entity)/edit/devel pages are broken) because of this and the suggested alternatives in the CR are a little vague and do not offer a valid solution:

If you have a genuine use case for script tags or other markup stripped by XSS::filerAdmin, you need to use proper templates or other APIs such as the Library API.

The only solution seems to be use SafeMarkup::set() for printing output generated from third part debug library (Krumo,Kint etc).
Any suggestion?

effulgentsia’s picture

Personally, I think calling SafeMarkup::set() on the return value from a library such as Krumo is legitimate. Especially if you add a comment on why you believe it to be safe (e.g., that it's not including any user-entered strings, or if it is, then they've been properly filtered). Curious if anyone here has other suggestions.

effulgentsia’s picture

You can also do:

[
'#type' => 'inline_template',
'#template' => '{{ foo|raw }}',
'#context' => ['foo' => $krumo_generated_html],
]

Which would have the benefit of not directly affecting the SafeMarkup registry, thereby benefiting from future optimizations such as #2503447: Use Twig_Markup to help with the SafeMarkup::$safeStrings memory load from Renderer::doRender().

You can also use the |raw filter in a larger template (not just a dedicated inline one as above), but be very careful with that, since any template that uses the raw filter needs to be absolutely certain that whatever is passed in doesn't contain an XSS attack.

xjm’s picture

Wim Leers’s picture

My concern about this is that we're not only marking this placeholder as safe for generation here; we're also saying it's something someone can enter somewhere else and not get sanitized. And possibly even cause whatever the placeholder turns into to be rendered in their field!? I feel like a correct fix would be to change how the render API assembles the placeholders into the string.

In general, when we added the SafeMarkup API, we said the only way it could really work would be if the safe markup list did not get individual tags added to it, and I feel like the placeholder is similar to that.

We might be able to move that to a followup (since the placeholder itself is known safe and it would unblock other work here), but I'm afraid it would also be a critical followup.

I think I'm reading that the concern is that whatever markup the placeholder is replaced with, that that replacing markup is not guaranteed to be safe? That indeed used to be the responsibility of the callback that replaced the placeholder. But as of #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache, that is actually guaranteed to be safe, since replacing the placeholder is always done in the same standardized way that goes through the Render API. See \Drupal\Core\Render\Renderer::replacePlaceholders().

pwolanin’s picture

For something like the Krumo markup - is that a use case for wrapping it in Twig_Markup?

Status: Fixed » Closed (fixed)

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

xjm’s picture

alimac’s picture

Follow-ups filed from this issue:

Since this issue is fixed, and all follow-ups have been opened, then we can move

core/lib/Drupal/Core/Render/Renderer.php
#2273925: Ensure #markup is XSS escaped in Renderer::doRender() only covers one use and will need a followup.
#2506581: Remove SafeMarkup::set() from Renderer::doRender

to the Done section in #2280965: [meta] Remove every SafeMarkup::set() call