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

Problem/Motivation

In #1825952: Turn on twig autoescape by default, \Drupal\Core\Render\Element\HtmlTag::preRenderHtmlTag() assembles HTML markup based on input and it is subsequently marked as safe. However, there is no guarantee that the rendered markup is actually safe.

This is not a security regression because the same is true in HEAD; HtmlTag::preRenderHtmlTag() will render whatever the caller tells it to, so it's the caller's responsibility to sanitize the input. However, this is one of the only places that we are marking as safe markup strings that are not explicitly known to be safe.

Proposed resolution

  • Remove the call by refactoring the code.
  • (done)Refactor the code so we can be more explicit with what is being sanitized and what we must assume will be safe.
  • Thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required. This class may be removed completely in a follow-up task (#2544318: Remove #type => html_tag usages).
  • Remaining tasks

    1. (done) Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
    2. (done) Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.
    3. See #47

    4. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

    Manual testing steps

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

    1. Install Drupal 8.
    2. Log into Drupal.
    3. Review the HTML of the <script> code block at the bottom of any page. This code block has inline javascript beginning with "var drupalSettings =".
    4. Compare the output above in HEAD and with the patch applied. Confirm that the HTML is identical.

    User interface changes

    NA

    API changes

    NA

    CommentFileSizeAuthor
    #73 remove-2296101-73.patch6.38 KBjosephdpurcell
    #73 interdiff-remove-2296101-70-73.txt842 bytesjosephdpurcell
    #70 remove-2296101-70.patch6.38 KBjosephdpurcell
    #70 interdiff-remove-2296101-68-70.txt498 bytesjosephdpurcell
    #68 remove-2296101-68.patch6.62 KBjosephdpurcell
    #68 interdiff-remove-2296101-63-68.txt492 bytesjosephdpurcell
    #63 interdiff-remove-2296101-62-63.txt850 bytesjosephdpurcell
    #63 remove-2296101-63.patch6.58 KBjosephdpurcell
    #62 interdiff-remove-2296101-60-62.txt1.4 KBjosephdpurcell
    #62 remove-2296101-62.patch6.58 KBjosephdpurcell
    #60 remove-2296101-60.patch6.51 KBjosephdpurcell
    #56 remove-2296101-56.patch6.64 KBcilefen
    #56 interdiff-2296101.txt246 bytescilefen
    #53 remove-2296101-53.patch6.76 KBcilefen
    #53 2296101-interdiff.txt2.22 KBcilefen
    #50 interdiff-of-fail.txt759 bytesakalata
    #47 interdiff-2291606-45-47.txt720 bytesakalata
    #47 2291606-47_test_should_fail.patch7.45 KBakalata
    #47 2296101-47.patch7.29 KBakalata
    #45 interdiff-2291606-41-45.txt1.53 KBakalata
    #45 2296101-45.patch6.95 KBakalata
    #41 interdiff-2291606-29-41.txt1.94 KBakalata
    #41 2296101-41.patch7.34 KBakalata
    #35 interdiff.2296101.29.35.txt1.91 KBYesCT
    #35 2296101.35.patch9.41 KBYesCT
    #29 2296101-2.29.patch7.5 KBalexpott
    #15 evaluate_safemarkup_use-2296101-15.patch2.12 KBaneek
    #15 interdiff-2296101-12-15.txt1.22 KBaneek
    #12 interdiff.txt1.01 KBjoelpittet
    #12 evaluate_safemarkup_use-2296101-12.patch2.19 KBjoelpittet
    #11 Screenshot from 2015-06-10 00:26:02.png24.7 KBaneek
    #9 evaluate_safemarkup_use-2296101-9.patch2.12 KBjoelpittet
    Support from Acquia helps fund testing for Drupal Acquia logo

    Comments

    xjm’s picture

    xjm’s picture

    Status: Postponed » Active
    jibran’s picture

    Issue tags: +SafeMarkup
    dawehner’s picture

    The amount of spaces where we still use #type => 'html_tag' is really limited. I think we should try to maybe get rid of it
    or at least move every place in core into a template. (maybe 5 non-test cases)

    Anonymous’s picture

    drupal_pre_render_html_tag got renamed/removed, but my google skills and I can't figure out where it went. Could this be updated in the title?

    Re #4: I can't find #type => 'html_tag' either.

    Can the summary be updated, or can this issue be closed?

    star-szr’s picture

    Title: Evaluate SafeMarkup use in drupal_pre_render_html_tag() » Evaluate SafeMarkup use in \Drupal\Core\Render\Element\HtmlTag::preRenderHtmlTag()
    Issue summary: View changes

    @pjonckiere - this is where I love git log -S drupal_pre_render_html_tag :) updated the title and issue summary. Thanks!

    catch’s picture

    Priority: Normal » Major

    I'd be fine with removing #type => 'html_tag' and then not having to solve this.

    We originally added that to allow themes to switch between XHTML and HTML5 easily, but 8.x (and everything else) is firmly HTML5 at this point, so there's really no reason for it at all.

    Also bumping to major since the more of these we fix, the smaller #2297703: [meta] Refactor and remove as many SafeMarkup::set() calls as possible becomes.

    xjm’s picture

    joelpittet’s picture

    Status: Active » Needs review
    FileSize
    2.12 KB

    I'm curious if this will fly.

    Status: Needs review » Needs work

    The last submitted patch, 9: evaluate_safemarkup_use-2296101-9.patch, failed testing.

    aneek’s picture

    @joelpittet, this totally breaks the application. Tested manually once. PFA the screenshot. I wonder that the usage of "@" in the tags might making double escaping. Though I haven't debugged thoroughly.

    But this one I have checked,
    Using patch #9, while loading a tag of type script is double escaped. So in the body footer the JavaScript generated is double escaped like,

    <script>
    &lt;!--//--&gt;&lt;![CDATA[//&gt;&lt;!--
    var drupalSettings = {&quot;path&quot;:{&quot;baseUrl&quot;:&quot;\/&quot;,&quot;scriptPath&quot;:&quot;\/index.php&quot;,&quot;pathPrefix&quot;:&quot;&quot;,&quot;currentPath&quot;:&quot;admin\/config\/user-interface\/shortcut&quot;,&quot;currentPathIsAdmin&quot;:true,&quot;isFront&quot;:false,&quot;currentLanguage&quot;:&quot;en&quot;},&quot;pluralDelimiter&quot;:&quot;\u0003&quot;,&quot;toolbar&quot;:{&quot;breakpoints&quot;:{&quot;toolbar.wide&quot;:&quot;only screen and (min-width: 61em)&quot;,&quot;toolbar.standard&quot;:&quot;only screen and (min-width: 38.125em)&quot;,&quot;toolbar.narrow&quot;:&quot;only screen and (min-width: 16.5em)&quot;},&quot;subtreesHash&quot;:&quot;O6215R6uor7oPBWQxYj6Ce0K6EusAK5M6XWWhsQ08Bo&quot;,&quot;langcode&quot;:&quot;en&quot;},&quot;user&quot;:{&quot;uid&quot;:&quot;1&quot;,&quot;permissionsHash&quot;:&quot;b22f4ddab0751b52a110bccea0222b907ce4f983f628523a818ae935f542a746&quot;}};
    //--&gt;&lt;!]]&gt;
    </script>
    

    Where it should be,

        <script>
    <!--//--><![CDATA[//><!--
    var drupalSettings = {"path":{"baseUrl":"\/","scriptPath":"\/index.php","pathPrefix":"","currentPath":"admin\/config\/user-interface\/shortcut","currentPathIsAdmin":true,"isFront":false,"currentLanguage":"en"},"pluralDelimiter":"\u0003","toolbar":{"breakpoints":{"toolbar.wide":"only screen and (min-width: 61em)","toolbar.standard":"only screen and (min-width: 38.125em)","toolbar.narrow":"only screen and (min-width: 16.5em)"},"subtreesHash":"O6215R6uor7oPBWQxYj6Ce0K6EusAK5M6XWWhsQ08Bo","langcode":"en"},"user":{"uid":"1","permissionsHash":"b22f4ddab0751b52a110bccea0222b907ce4f983f628523a818ae935f542a746"}};
    //--><!]]>
    </script>
    

    As one of the solution we have to find an way to deal with the script type of tags, where $element[#tag] == 'script'.

    joelpittet’s picture

    Status: Needs work » Needs review
    FileSize
    2.19 KB
    1.01 KB

    Thanks for the manual review @aneek. Since we are using SafeMarkup::checkAdminXss() on prefix/suffix and of late #markup. We can do the same here for value and it's prefix/suffix.

    Let's see how this blends...

    joelpittet’s picture

    Title: Evaluate SafeMarkup use in \Drupal\Core\Render\Element\HtmlTag::preRenderHtmlTag() » Remove SafeMarkup::set() use in \Drupal\Core\Render\Element\HtmlTag::preRenderHtmlTag()
    Issue tags: +Needs manual testing

    Nice green! @aneek mind giving that a try on for size?

    aneek’s picture

    @joelpittet, I might. Okay tonight then.

    aneek’s picture

    @joelpittet, so I only removed two variable initializations $value_prefix & $value_suffix. Do we require those? Those are not used anywhere else, so might save very very little memory on this.
    I hope this is acceptable if the result is green. :-)

    joelpittet’s picture

    Oh good call thanks @aneek it also removes the function calls to the checkAdminXss if not needed!

    aneek’s picture

    @joelpittet, most welcome :-)

    Thanks!

    seantwalsh’s picture

    Status: Needs review » Needs work

    Looked over the patch on the Shortcuts page as in #11. While visually the patch in #15 appears fine and it passes test, the data in the

    tag at the bottom of the page is still double escaped. Before:
    <!--//--><![CDATA[//><!--
    var drupalSettings = {"path":{"baseUrl":"\/","scriptPath":"\/index.php","pathPrefix":"","currentPath":"admin\/config\/user-interface\/shortcut\/manage\/default\/customize","currentPathIsAdmin":true,"isFront":false,"currentLanguage":"en"},"pluralDelimiter":"\u0003","tableDrag":{"shortcuts":{"shortcut-weight":[{"target":"shortcut-weight","source":"shortcut-weight","relationship":"sibling","action":"order","hidden":true,"limit":0}]}},"toolbar":{"breakpoints":{"toolbar.wide":"only screen and (min-width: 61em)","toolbar.standard":"only screen and (min-width: 38.125em)","toolbar.narrow":"only screen and (min-width: 16.5em)"},"subtreesHash":"W8wt8H_4hdCMusHvFuKa7pMBqZAFmijMGw8fflx92AM","langcode":"en"},"user":{"uid":"1","permissionsHash":"cc212b02b361d0cce5313691111c4ea694d69acd423fc41d8f14c4101c67bf6f"}};
    //--><!]]>
    
    After:
    var drupalSettings = {"path":{"baseUrl":"\/","scriptPath":"\/index.php","pathPrefix":"","currentPath":"admin\/config\/user-interface\/shortcut\/manage\/default\/customize","currentPathIsAdmin":true,"isFront":false,"currentLanguage":"en"},"pluralDelimiter":"\u0003","tableDrag":{"shortcuts":{"shortcut-weight":[{"target":"shortcut-weight","source":"shortcut-weight","relationship":"sibling","action":"order","hidden":true,"limit":0}]}},"toolbar":{"breakpoints":{"toolbar.wide":"only screen and (min-width: 61em)","toolbar.standard":"only screen and (min-width: 38.125em)","toolbar.narrow":"only screen and (min-width: 16.5em)"},"subtreesHash":"W8wt8H_4hdCMusHvFuKa7pMBqZAFmijMGw8fflx92AM","langcode":"en"},"user":{"uid":"1","permissionsHash":"cc212b02b361d0cce5313691111c4ea694d69acd423fc41d8f14c4101c67bf6f"}};
    //--&gt;
    
    seantwalsh’s picture

    If I change SafeMarkup::checkAdminXss to SafeMarkup::format it renders correctly. The cause seems to be that SafeMarkup::checkAdminXss does not allow the CDATA markup that should render in this case.

    <!--//--><![CDATA[//><!--
    ...
    //--><!]]>
    akalata’s picture

    Did you make any other changes besides just the method name? Simply switching the method names would actually make this less secure, because you'd be passing $element['#value'] as a literal string.

    However, calling SafeMarkup::format('<!--//--><![CDATA[//><!-- @element //--><!]]>', array(@element => $element['#value'])) seems like it could work?

    seantwalsh’s picture

    @akalata and I spoke about this IRL, attempting to summarize that discussion and potential next steps.

    Based on looking at what the patch in #15 does, it seems that either:

    1. The Note may need to be changed because SafeMarkup::checkAdminXss() is actually being used, however the note says "It is the caller's responsibility to sanitize any input parameters. This callback does not perform sanitization.", or
    2. Leave the Note as is and change the SafeMarkup::checkAdminXss() to SafeMarkup::format() on value_prefix, value, and value_suffix.
    akalata’s picture

    Status: Needs work » Needs review

    1. For easier evaluation, here's the full comment for preRenderHtmlTag(), which is what led us to question the purpose of this issue:

       * Note: It is the caller's responsibility to sanitize any input parameters.
       * This callback does not perform sanitization. Despite the result of this
       * pre-render callback being a #markup element, it is not passed through
       * \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().
    

    2. Clarification: If we do decide to include some sanitization here, I think that only the value_prefix and value_suffix should be flagged using SafeMarkup::format (accepting these as provided by the caller), while potentially continuing to have $element['#value'] run through a filter? This seems to be a gray area we're running into while addressing these conversions.

    davidhernandez’s picture

    Issue tags: +D8 Accelerate
    joelpittet’s picture

    Status: Needs review » Needs work

    @akalata re #22.2. Yeah it's a bit grey for sure. I was going on the side of caution for #value but still permissive to an extent. But maybe you are right let's just let it escape if it's not safe. That's probably a good call.

    joelpittet’s picture

    @akalata re #22.1 @chx and I put that in there because we didn't know what to do with it at the time during the initial auto-escape conversion;) I recall this conversion explicitly and it resulted in throwing arms in the air and saying "we can't prevent people from doing stupid things entirely or DX will suffer" Paraphrasing from what I remember

    It is the caller's responsibility to sanitize any input parameters.

    alexpott’s picture

    I've done some investigation as to whether we can remove the SafeMarkup::set() calls completely from this file and I think we can! But we need both #2506581: Remove SafeMarkup::set() from Renderer::doRender and #2506195: Remove SafeMarkup::set() from Xss::filter().

    If we do that then we can make the html_tag element have way less side effects and completely remove the possibility of it marking something unintentionally as safe. Therefore I'm going to postpone this issue on them

    alexpott’s picture

    Status: Needs work » Postponed

    Now doing the real postponing as per #26 :)

    alexpott’s picture

    Status: Postponed » Needs work

    All the blocking issues have landed

    alexpott’s picture

    Status: Needs work » Needs review
    FileSize
    7.5 KB

    As this is definitely part of the render system I think it is appropriate to auto-escape and auto-filter if things are marked as safe and use SafeString::create here. That way no SafeMarkup::set() is used.

    No interdiff because a completely different approach.

    YesCT’s picture

    I'm going to review this.

    YesCT’s picture

    I'm not understanding how user input gets into the drupalSettings in the script tag at the end of the pages.
    What are we worried about that might not be safe?

    YesCT’s picture

    Status: Needs review » Needs work

    in head

        <script>
    <!--//--><![CDATA[//><!--
    var drupalSettings = {"path":{"baseUrl":"\/drupal\/","scriptPath":null,"pathPrefix":"","currentPath":"admin\/config\/user-interface\/shortcut","currentPathIsAdmin":true,"isFront":false,"currentLanguage":"en"},"pluralDelimiter":"\u0003","toolbar":{"breakpoints":{"toolbar.narrow":"only screen and (min-width: 16.5em)","toolbar.standard":"only screen and (min-width: 38.125em)","toolbar.wide":"only screen and (min-width: 61em)"},"subtreesHash":"GZx7CEcpvx_E04GfvBy7MqlRhF9GNei0SMMjy6261II"},"user":{"uid":"1","permissionsHash":"95cbee68ac608d190bcd4414d1a5ef708dfeecd50d9aaac6fa431c76ce93ae65"}};
    //--><!]]>
    </script>
    

    with the patch:

        <script>
    var drupalSettings = {"path":{"baseUrl":"\/drupal2\/","scriptPath":null,"pathPrefix":"","currentPath":"admin\/config\/user-interface\/shortcut","currentPathIsAdmin":true,"isFront":false,"currentLanguage":"en"},"pluralDelimiter":"\u0003","toolbar":{"breakpoints":{"toolbar.narrow":"only screen and (min-width: 16.5em)","toolbar.standard":"only screen and (min-width: 38.125em)","toolbar.wide":"only screen and (min-width: 61em)"},"subtreesHash":"NOm9ZCPCMn94RUx7ndVbbyPB7gUBChrT3XC1G91I_mA"},"user":{"uid":"1","permissionsHash":"9f5f78e60f161e1f1e2270f53f27ab32db394d5b9f750b8535997598c66fea44"}};
    //--&gt;
    </script>
    

    looks like it is missing the prefix and suffix.

    I'm going to take a look at that now.

    davidhernandez’s picture

    +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
    @@ -193,4 +182,24 @@ public static function preRenderConditionalComments($element) {
    +      $string = Xss::filterAdmin($string);
    

    I'm not sure what we're considering acceptable or not. This is going to filter out the CDATA prefix.

    davidhernandez’s picture

    What else can actually make a prefix or suffix? For the most part this seems to only be used for script tags now?

    YesCT’s picture

    Status: Needs work » Needs review
    FileSize
    9.41 KB
    1.91 KB

    so, it seems to me that we either
    a)
    dont check if prefixes and suffixes are safe, and assume they are,
    or
    b)
    to use the new protected static function xssFilterAdminIfUnsafe($string) {
    we go to where those are set and mark them safe (and document why) there.

    this starts doing b) (but still needs the comments)

    [edit] ag CDATA core --ignore="core/vendor" --ignore="test" --ignore="*Test.php" --ignore="*.js" was how I started looking for places to mark safe at the source of the prefix and suffix

    the change in
    core/lib/Drupal/Core/Asset/JsCollectionRenderer.php
    can be seen like the previous manual reviews here on just about any page (like the shortcuts page)
    in the script tag at the bottom of the html source file.

    the change in
    core/lib/Drupal/Component/Utility/Html.php
    I'm not sure where to see, maybe by making drupal output xhtml?

    uploading code mostly to demonstrate what I'm talking about.

    YesCT’s picture

    to help in thinking about @catch's suggestion in #7
    ag "\'html_tag" core
    is helpful.

    dsnopek’s picture

    Another option (which we discussed at DrupalCorn) is just not using #value_prefix and #value_suffix in core for making CSS and Javascript compatible with XHTML, because Drupal 8 is all HTML5, but leaving those as valid arguments for the theme function just in case someone in contrib wanted to create an XHTML theme.

    akalata’s picture

    In discussing our options, I think we missed the suggestion in #4 and #7 to completely remove #type => 'html_tag'. This is may out-of-scope based on the original issue, but after review the non-test uses in core can be grouped into 2 categories: <head> elements such as <meta>, <link>, and <script>, and simple combinations where SafeMarkup::format could be used instead.

    _drupal_add_html_head() is listed as deprecated (#2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc.), so we may need to postpone on that. In the meantime, we could work on the 6 places where non-head-type tags are using this #type? (One of those places is FilterCaption.php, which has its own use of SafeMarkup).

    akalata’s picture

    To prevent scope creep, #2544318: Remove #type => html_tag usages has been created. I've also identified #2120113: XHTML is not a thing anymore, remove <!--//--><![CDATA[//><!-- //--><!]]> for escaping inline JS/CSS as the additional task suggested in #37.

    I will be updating the issue summary and proceed with documenting as suggested in #35 - option A.

    akalata’s picture

    akalata’s picture

    Updating from #29, updating #value_prefix and #value_suffix since filtering strips CDATA tags.

    akalata’s picture

    Issue summary: View changes

    Status: Needs review » Needs work

    The last submitted patch, 41: 2296101-41.patch, failed testing.

    akalata’s picture

    Issue summary: View changes
    Issue tags: +Needs manual testing

    Adding manual testing instructions.

    akalata’s picture

    Category: Bug report » Task
    Status: Needs work » Needs review
    FileSize
    6.95 KB
    1.53 KB

    Updating tests since we're no longer marking #value_prefix or #value_suffix as safe.

    YesCT’s picture

    +++ b/core/tests/Drupal/Tests/Core/Render/Element/HtmlTagTest.php
    @@ -85,25 +85,19 @@
    -      '#value_prefix' => '<script>alert()</script>',
    -      '#value_suffix' => '<script>alert()</script>',
    ...
    -    $tags[] = array($element, "<p>alert()valuealert()</p>\n");
    +    $tags[] = array($element, "<p>value</p>\n");
    

    ah, we are assuming prefixes and suffixes are safe, so we are *not* filtering them. so we can not have a test that asserts script tag is filtered from prefix or suffix. ok.

    but maybe we should have a test that asserts we *are* letting script through via prefix and suffex... which is weird to assert we can do something dangerous. but we are making the decision here to do it on purpose. so at least a blame on the test would link back to this issue which has our logic for doing this.

    akalata’s picture

    Adding a test per #46 the ensure #value_prefix and #value_suffix are not escaped/filtered. Includes an example patch that DOES filter #value_prefix and #value_suffix, causing the test to fail.

    Status: Needs review » Needs work

    The last submitted patch, 47: 2291606-47_test_should_fail.patch, failed testing.

    akalata’s picture

    Status: Needs work » Needs review
    akalata’s picture

    FileSize
    759 bytes

    Per @YesCT's in-person request, posting an interdiff of what was changed in order to make the test fail.

    YesCT’s picture

    That is perfect. I like the new test.

    manually tested again on head and with the patch, and the html markup is exactly the same (except the hash)

        <script>
    <!--//--><![CDATA[//><!--
    var drupalSettings = {"path":{"baseUrl":"\/drupal2\/","scriptPath":null,"pathPrefix":"","currentPath":"node","currentPathIsAdmin":false,"isFront":true,"currentLanguage":"en"},"pluralDelimiter":"\u0003","ajaxPageState":{"libraries":"bartik\/global-styling,classy\/base,contextual\/drupal.contextual-links,contextual\/drupal.contextual-toolbar,core\/drupal.active-link,core\/html5shiv,core\/normalize,quickedit\/quickedit,shortcut\/drupal.shortcut,system\/base,toolbar\/toolbar,toolbar\/toolbar.escapeAdmin,tour\/tour,user\/drupal.user.icons,views\/views.contextual-links","theme":"bartik","theme_token":null},"toolbar":{"breakpoints":{"toolbar.narrow":"only screen and (min-width: 16.5em)","toolbar.standard":"only screen and (min-width: 38.125em)","toolbar.wide":"only screen and (min-width: 61em)"},"subtreesHash":"4DQFzwFHHXU0zoxyMueTaoF92DX8Fb-feeXDBSsz7OU"},"user":{"uid":"1","permissionsHash":"c0415734e0747ccd37a5ea7cc4391c636d5b4beb82233710366209b9a1947125"}};
    //--><!]]>
    </script>
    
    1. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
      @@ -46,30 +47,23 @@ public function getInfo() {
      -   * Note: It is the caller's responsibility to sanitize any input parameters.
      -   * This callback does not perform sanitization. Despite the result of this
      -   * pre-render callback being a #markup element, it is not passed through
      -   * \Drupal\Component\Utility\Xss::filterAdmin(). This is because it is marked
      -   * safe here, which causes
      -   * \Drupal\Core\Render\Renderer::xssFilterAdminIfUnsafe() to regard it as safe
      -   * and bypass the call to \Drupal\Component\Utility\Xss::filterAdmin().
      -   *
      ...
          *   - #value_prefix: (optional) A string to prepend to #value, e.g. a CDATA
      -   *     wrapper prefix.
      +   *     wrapper prefix. The value of #value_prefix cannot be filtered and is
      +   *     assumed to be safe.
          *   - #value_suffix: (optional) A string to append to #value, e.g. a CDATA
      -   *     wrapper suffix.
      +   *     wrapper suffix. The value of #value_suffix cannot be filtered and is
      +   *     assumed to be safe.
      

      prefix and the suffix have an additional warning about being assumed to be safe.

      but this is less strongly worded and in a place not as noticable as the warning was before.

      we should keep some kind of general warning near the top of the doc block for this method.

    2. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
      @@ -79,35 +73,30 @@ public function getInfo() {
      +    $element['#markup'] = SafeMarkup::set($markup);
      

      in comment #41 the interdiff shows this ::set() being added.

      but we need to document why it is ok, or refactor this one too.

    3. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
      @@ -193,4 +182,24 @@ public static function preRenderConditionalComments($element) {
      +  protected static function xssFilterAdminIfUnsafe($string) {
      

      this was added in #29 and called 3 times, on the prefix, value, suffix.

      Now, we are assuming prefix and suffix are save and *NOT* filtering if unsafe. (cause it broke CDATA)

      so... we should think a bit about if we still want the HtmlTag::xssFilterAdminIfUnsafe()

      @akalata suggests maybe we could just use filterAdmin() .. the difference would be not calling that if the value was already safe.

      I can't totally wrap my head around this to have an opinion.

    other than these things I think this looks really good.

    alexpott’s picture

    Status: Needs review » Needs work
    1. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
      @@ -79,35 +73,30 @@ public function getInfo() {
      +    $element['#markup'] = SafeMarkup::set($markup);
      

      Let's use SafeString::create() here since this is mimicking what's going on in Renderer::render().

    2. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
      @@ -79,35 +73,30 @@ public function getInfo() {
      +      $markup .= static::xssFilterAdminIfUnsafe($element['#value']);
      

      We could just inline the isSafe() check now.

    3. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
      @@ -193,4 +182,24 @@ public static function preRenderConditionalComments($element) {
      +    return SafeString::create($string);
      

      If we do keep this the SafeString::create is not necessary as the result is immediately stringified.

    cilefen’s picture

    Status: Needs work » Needs review
    FileSize
    2.22 KB
    6.76 KB
    YesCT’s picture

    Status: Needs review » Reviewed & tested by the community
    Issue tags: -Needs manual testing

    this addresses all of my concerns from #51 and alexpott's from #52.

    if it is green, rtbc from me.

    i did manual testing again in #51 so removing that tag.

    YesCT’s picture

    Status: Reviewed & tested by the community » Needs review
    +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
    @@ -181,25 +184,4 @@ public static function preRenderConditionalComments($element) {
    -
     }
    

    (super small nit) one too many newlines taken out when this function was removed.

    ----

    and... the issue summary still has a remaining task to evaluate if there is test coverage and document so in the issue summary. :/

    cilefen’s picture

    davidhernandez’s picture

    Issue summary: View changes

    #47 is clearly indicating test coverage. Does the last remaining task still need review?

    josephdpurcell’s picture

    Reviewing this now.

    josephdpurcell’s picture

    Status: Needs review » Needs work
    Issue tags: +Needs reroll

    I read through the patch to see if the third task is complete. I think that #tag, #value, #value_prefix, and #value_suffix are documented well.

    Here are a few comments I have from review:

    1. +++ b/core/tests/Drupal/Tests/Core/Render/Element/HtmlTagTest.php
      @@ -7,6 +7,7 @@
      +use Drupal\Core\Render\SafeString;
      

      This use statement is out of alphabetical order. The UnitTestCase is also out of order, so either sort all of them, or just append to the end.

    2. +++ b/core/tests/Drupal/Tests/Core/Render/Element/HtmlTagTest.php
      @@ -77,6 +78,36 @@ public function providerPreRenderHtmlTag() {
      +    // Ensure that #value is filtered if they are not safe.
      

      Change "if they are" to "if it is".

    3. Clarify why #attributes does not need escaped here. It does not need to be escaped, because the attribute names are escaped at Drupal\Core\Template\AttributeValueBase:render() and the attribute values are escaped at Drupal\Core\Template\AttributeValueBase:__toString(). The class documentation for Drupal\Core\Template\Attribute explains some of these assumptions on a high level, so perhaps simply link to that doc?
    4. I received a conflict of this patch with the latest 8.0.x.

    I will plan to address these now.

    josephdpurcell’s picture

    Rerolling. No conflicts. Changes still pending, this is just reroll.

    josephdpurcell’s picture

    Status: Needs work » Needs review
    josephdpurcell’s picture

    Addresses #59.

    1. Not going to do since that is no longer directly relevant to this ticket.
    2. Done.
    3. Done.
    4. Done. (no conflicts)
    josephdpurcell’s picture

    Updating doc to include leading backslash on class name, see https://www.drupal.org/node/1354#classes

    The last submitted patch, 60: remove-2296101-60.patch, failed testing.

    The last submitted patch, 62: remove-2296101-62.patch, failed testing.

    Status: Needs review » Needs work

    The last submitted patch, 63: remove-2296101-63.patch, failed testing.

    YesCT’s picture

    +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
    @@ -10,6 +10,7 @@
     use Drupal\Component\Utility\SafeMarkup;
     use Drupal\Core\Render\SafeString;
     use Drupal\Component\Utility\Xss;
    +use Drupal\Core\Render\SafeString;
     use Drupal\Core\Template\Attribute;
    

    the 3-way auto-merging didn't have any conflicts... but it ended up putting in a duplicate use statement. #2544262: Refactor use of SafeMarkup::set in \Drupal\Core\Render\Element\HtmlTag::preRenderConditionalComments()

    josephdpurcell’s picture

    Status: Needs work » Needs review
    FileSize
    492 bytes
    6.62 KB

    Doh! Fixes duplicate use pointed out in #67.

    YesCT’s picture

    Status: Needs review » Needs work
    +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
    @@ -8,8 +8,8 @@
    -use Drupal\Core\Render\SafeString;
     use Drupal\Component\Utility\Xss;
    +use Drupal\Core\Render\SafeString;
    

    ... ack. I know it is frustrating, but the reordering is not needed as part of this issue. We can just leave it where it was (even though the use statements are out of alphabetical order... and alphabetical order isn't really a standard we have #1624564: Coding standards for "use" statements)

    josephdpurcell’s picture

    Unalphanumericreordering per #69.

    josephdpurcell’s picture

    Status: Needs work » Needs review
    YesCT’s picture

    Issue summary: View changes
    Status: Needs review » Needs work

    I read the whole patch again. and it all looks good. just a couple nits.

    1. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
      @@ -80,35 +76,30 @@ public function getInfo() {
      +    // An html tag should not contain any special characters. Escape them to
      

      html in comments should be all caps: HTML

      (I verified this by:
      grep -R "\/\/.* HTML " core |wc -l
      which had 248

      grep -R "\/\/.* html " core |wc -l
      which had 19

      so caps is a clear winner.)

    2. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
      @@ -80,35 +76,30 @@ public function getInfo() {
      +    // ensure this can not be abused.
      

      I think "cannot" is what we mean here.

    josephdpurcell’s picture

    Status: Needs work » Needs review
    FileSize
    842 bytes
    6.38 KB

    Updating the grammers from #72.

    YesCT’s picture

    Status: Needs review » Reviewed & tested by the community

    no code changes since the last manual testing. so I think this is ready.

    davidhernandez’s picture

    html in comments should be all caps: HTML

    Actually, we're changing that. I wouldn't hold up this issue either way on it though.

    See #2546248: Use consistent style to mention HTML tags in code comments

    alexpott’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed 8071554 and pushed to 8.0.x. Thanks!

    • alexpott committed 8071554 on 8.0.x
      Issue #2296101 by josephdpurcell, akalata, cilefen, joelpittet, aneek,...

    Status: Fixed » Closed (fixed)

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