Problem/Motivation

\Drupal\Core\Render\Element\HtmlTag::preRenderConditionalComments() calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by using SafeString::create() instead so that the list of safe strings isn't polluted with a fragment

Remaining tasks

Review patch

Manual testing steps

Do these steps both with HEAD and with the patch applied:

  1. Install Drupal 8.
  2. Review the HTML of the page in the <head> area, between the <link> and <script> blocks. Look for the conditional comment that begins with <!--[if lte IE 8]>.
  3. Compare the output above in HEAD and with the patch applied. Confirm that the HTML is identical.

For testing of the prefix/suffix:

Add $element['#prefix'] = '<blink>test&</blink>' and $element['#suffix'] = '<blink>test&</blink>' and verify that the result before and after are the same.

Add $element['#prefix'] = SafeString::create('<blink>test&</blink>') and $element['#suffix'] = SafeString::create('<blink>test&</blink>') and verify that the result before and after are the same.

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#25 interdiff-2544262-20-25.txt921 bytesjosephdpurcell
#25 2544262-25.patch2.81 KBjosephdpurcell
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,566 pass(es). View
#20 2544262-20.patch2.74 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,547 pass(es). View
#20 interdiff-18-20.txt718 bytesstefan.r
#18 2544262-18.patch2.47 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,460 pass(es). View
#16 2544262-16.patch2.37 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,465 pass(es), 5 fail(s), and 0 exception(s). View
#14 2544262-14.patch2.16 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#10 2544262-10.patch2.12 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#2 2544262-2.patch1.31 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,685 pass(es). View

Comments

akalata’s picture

Issue summary: View changes
akalata’s picture

Title: Document acceptable use of SafeMarkup::set in \Drupal\Core\Render\Element\HtmlTag::preRenderConditionalComments() » Refactor use of SafeMarkup::set in \Drupal\Core\Render\Element\HtmlTag::preRenderConditionalComments()
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Twig, +D8 Accelerate
FileSize
1.31 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,685 pass(es). View
akalata’s picture

Issue summary: View changes
YesCT’s picture

so I relooked up the difference between set and format to remind myself why this is one of the acceptable ways of removing set.
And format sanatizes the values for the placeholders first, and then marks the string as safe.

The patch stays in scope.

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

    so manual testing this... doesn't include a value for $prefix (and $suffix). So I'm not 100% confident that we are manually testing the escaping of some of this.

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

    this bit is IR specific, ... do we want to require manual testing in IE?

... I can't remember the tag for manual testing in IE.

akalata’s picture

4.1: The prefix and suffix are already being passed through Xss::filterAdmin, so we can pass those through SafeMarkup::format without worrying about them. However, if we wanted to strictly conform to the usage of SafeMarkup::format() maybe we should use ! instead of @ as the first character of the $args key?

YesCT’s picture

so, the strings we are concatenating, are first being run though Xss::filterAdmin() and my initial worry was that we might be marking the parts of the string safe, and then the whole concatenated string safe, and polluting the list of safe strings with fragments.

but, checking the docs on filterAdmin():

   * This method is preferred to
   * \Drupal\Component\Utility\SafeMarkup::xssFilter() when the result is
   * not being used directly in the rendering system (for example, when its
   * result is being combined with other strings before rendering). This avoids
   * bloating the safe string list with partial strings if the whole result will
   * be marked safe.

this is not marking the fragments safe, and seems to be exactly the recommended use case of filterAdmin().

I think @ is still the thing to use with filter... and looked to #2501683: Remove SafeMarkup::set in _update_message_text() and it seems to use @ in format even with strings we are assuming are safe.

----
so, doing a code review, this looks great.

but we might still need some manual testing steps for the prefix and suffix and for IE.

akalata’s picture

I'm having trouble figuring out where this code is being called, in order to specifically test the $elements['#prefix'] and $elements['#suffix'] pieces that are being checked before SafeMarkup::format() is called:

  • preRenderConditionalComments is only called directly in its own test
  • one other test (AttachedAssetsTest) references preRenderConditionalComments, but does not utilize the prefix/suffix pattern
  • HtmlTag->getInfo() adds preRenderConditionalComments as a #pre_render value, but I couldn't see where in the render() stack prefix/suffix are added
  • The conditional comment added in core is coming from core.libraries.yml, but adding 'prefix' and 'suffix' keys at the same level as the 'browsers' key had no effect

Are we sure that the prefix and suffix are being used for preRenderConditionalComments?

YesCT’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs tests

needs work to add tests for the prefix and suffix that comes from the element array

Wim Leers’s picture

Shouldn't this use SafeString::create()?

To find reviewers: look at the git blame view for this file. I'd say general theme system maintainers are also good candidates to look at this.

stefan.r’s picture

Priority: Normal » Major
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.12 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Seems the prefix and suffix already have test coverage through HtmlTagTest::testPreRenderConditionalComments() :)

@akalata the #prefix/#suffix is actually not being used in core, but for manual testing, we can add the following code in JsCollectionRenderer::render() when it loops through the JS assets:

$element['#prefix'] = '<blink>test</blink>';
$element['#suffix'] = SafeString::create('<blink>test</blink>');

This may as well use SafeString::create(), patch attached.

Status: Needs review » Needs work

The last submitted patch, 10: 2544262-10.patch, failed testing.

Status: Needs work » Needs review

stefan.r queued 10: 2544262-10.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2544262-10.patch, failed testing.

stefan.r’s picture

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

Status: Needs review » Needs work

The last submitted patch, 14: 2544262-14.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
2.37 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,465 pass(es), 5 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 16: 2544262-16.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
2.47 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,460 pass(es). View

joelpittet’s picture

This looks like a decent change to make, especially for performance.

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

Although in either case we need to know that we've done our homework.

So variables involved $expression, $prefix, $suffix, need to be documented as to why they are 'safe' (aka have they been escaped/filtered/etc previously?) and why the resulting string is also safe.

This needs work for documentation but I believe the direction should be good to go. #2 didn't need this extra documentation because it escaped thorugh @

stefan.r’s picture

Status: Needs work » Needs review
FileSize
718 bytes
2.74 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,547 pass(es). View

The documentation was actually already there, just not visible in the patch.. it just still referred to SafeMarkup::set().

Looking at the complete actual function it's clear that everything has been at least admin-escaped: $expression is either IE, !IE or the safe value of $element['#browsers'], and $prefix/$suffix are the safe values of $element['#prefix'] / $element['#suffix'] themselves.

josephdpurcell’s picture

Status: Needs review » Reviewed & tested by the community

From manual testing I see before:

<!--[if lte IE 8]>
<script src="http://dev.d8.local/sites/default/files/js/js_IaU4wfZlByZ8at0yjyUe8AB03hw-jS1ZW5dZD4ep-wI.js"></script>
<![endif]-->

And after applying 2544262-20.patch I see:

<!--[if lte IE 8]>
<script src="http://dev.d8.local/sites/default/files/js/js_IaU4wfZlByZ8at0yjyUe8AB03hw-jS1ZW5dZD4ep-wI.js"></script>
<![endif]-->

So, these are exactly the same as expected.

Then, after modifying Drupal\Core\Asset\JsCollectionRenderer:render to be:

54	    // Defaults for each SCRIPT element.
55	    $element_defaults = array(
56	      '#prefix' => '<blink>test&</blink>',
57	      '#type' => 'html_tag',
58	      '#tag' => 'script',
59	      '#value' => '',
60	      '#suffix' => '<blink>test&</blink>',
61	    );

I see before:

<!--[if lte IE 8]>
test&amp;<script src="http://dev.d8.local/sites/default/files/js/js_IaU4wfZlByZ8at0yjyUe8AB03hw-jS1ZW5dZD4ep-wI.js"></script>
test&amp;<![endif]-->

And after:

<!--[if lte IE 8]>
test&amp;<script src="http://dev.d8.local/sites/default/files/js/js_IaU4wfZlByZ8at0yjyUe8AB03hw-jS1ZW5dZD4ep-wI.js"></script>
test&amp;<![endif]-->

Which is expected.

And after changing it to:

55	    // Defaults for each SCRIPT element.
56	    $element_defaults = array(
57	      '#prefix' => SafeString::create('<blink>test&</blink>'),
58	      '#type' => 'html_tag',
59	      '#tag' => 'script',
60	      '#value' => '',
61	      '#suffix' => SafeString::create('<blink>test&</blink>'),
62	    );

I see before:

<!--[if lte IE 8]>
<blink>test&</blink><script src="http://dev.d8.local/sites/default/files/js/js_IaU4wfZlByZ8at0yjyUe8AB03hw-jS1ZW5dZD4ep-wI.js"></script>
<blink>test&</blink><![endif]-->

And after I see:

<!--[if lte IE 8]>
<blink>test&</blink><script src="http://dev.d8.local/sites/default/files/js/js_IaU4wfZlByZ8at0yjyUe8AB03hw-jS1ZW5dZD4ep-wI.js"></script>
<blink>test&</blink><![endif]-->

Which I expect.

I also confirmed that all calls to SafeMarkup::set() are removed from the \Drupal\Core\Render\Element\HtmlTag::preRenderConditionalComments() method.

I reviewed the documentation on why SafeString::create() is still there and believe it is clear that $expression, $prefix, and $suffix are safe.

I believe it to meet the requirements of #2280965: [meta] Remove every SafeMarkup::set() call. Setting to RTBC.

YesCT’s picture

josephdpurcell’s picture

Issue summary: View changes
joelpittet’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -183,17 +184,17 @@ public static function preRenderConditionalComments($element) {
-    // Now calling SafeMarkup::set is safe, because we ensured the
-    // data coming in was at least admin escaped.
+    // We ensured earlier in this method that $expression, $prefix and $suffix
+    // were at least admin escaped, so now calling SafeString::create() is safe.

It's not "admin escaped" but "admin XSS filtered" and could maybe be massaged a bit more explicit to explain why the entire string is safe.

Also a note on the manual testing, since we changed from SafeMarkup::format() to SafeString::create() there should be no difference in the output and that test would really not test this specific issue but whether SafeString::create() == SafeMarkup::set() which very much should be the case (except for maybe passing in batch where the big list gets lost). Although good to test probably doesn't have much extra to say about this patch.

josephdpurcell’s picture

Status: Needs work » Needs review
FileSize
2.81 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,566 pass(es). View
921 bytes

Updated the comment according to #24.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

that interdiff looks like what was asked for. and #21 reviewed all the things. :) rtbc.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This IS nice. :) SafeString is good to use here because we definitely are internal to the render system and $prefix and $suffix have been admin filtered above if they are not marked safe.

Committed 7c4205a and pushed to 8.0.x. Thanks!

  • alexpott committed 7c4205a on 8.0.x
    Issue #2544262 by stefan.r, josephdpurcell, akalata, YesCT, joelpittet:...

Status: Fixed » Closed (fixed)

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