Discovered during #2506195: Remove SafeMarkup::set() from Xss::filter()

    // Ensure what we are dealing with is safe.
    // This would be done later anyway in drupal_render().
    $prefix = isset($elements['#prefix']) ? SafeMarkup::checkAdminXss($elements['#prefix']) : '';
    $suffix = isset($elements['#suffix']) ? SafeMarkup::checkAdminXss($elements['#suffix']) : '';

Is suppose to escape the prefix and suffix. Unfortunately $elements can never be set as the variable is actually $element.

The issue should also change the code to use SafeMarkup::isSafe() and Xss::filterAdmin().

CommentFileSizeAuthor
#12 2525908.12.patch3 KBalexpott
#6 2525908.5.patch3.19 KBalexpott
#6 4-5-interdiff.txt2.85 KBalexpott
#6 2525908.5.test-only.patch1.94 KBalexpott
#4 htmltag_render-2525908-4.patch1.17 KBcilefen
#2 htmltag_render-2525908-1.patch1.95 KBcilefen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Issue tags: +Security improvements, +Needs tests
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Active » Needs review
FileSize
htmltag_render-2525908-1.patch1.95 KB

Is it as simple as this?

cilefen’s picture

cilefen CreditAttribution: cilefen commented

No - I should read the isSafe() docs...

cilefen’s picture

cilefen CreditAttribution: cilefen commented
FileSize
htmltag_render-2525908-4.patch1.17 KB

The last submitted patch, 2: htmltag_render-2525908-1.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2525908.5.test-only.patch1.94 KB
4-5-interdiff.txt2.85 KB
2525908.5.patch3.19 KB

Here's a test... also I'm not sure about the readability of nested ternary statements.

The last submitted patch, 6: 2525908.5.test-only.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented 
+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -173,8 +174,14 @@ public static function preRenderConditionalComments($element) {
+    $prefix = isset($element['#prefix']) ? $element['#prefix'] : '';
+    if ($prefix && !SafeMarkup::isSafe($prefix)) {
+      $prefix = Xss::filterAdmin($prefix);
+    }
+    $suffix = isset($element['#suffix']) ? $element['#suffix'] : '';
+    if ($suffix && !SafeMarkup::isSafe($suffix)) {
+      $suffix = Xss::filterAdmin($suffix);
+    }

Hmm. I'm wondering if this gives a false sense of security and/or might introduce some bugs. Aren't #prefix and #suffix often used to wrap markup around something, like with an opening and closing tag?

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented

#8: Yes, exactly, that is why Xss::filterAdmin is used, so it prevents that you use '<script>' or any variation thereof in $prefix / $suffix.

This just fixes the code, the discussion about this did already take place in the issue that introduced this bug.

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
CreditAttribution: legolasbo as a volunteer commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

I've reviewed the patch and the code looks clean to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2525908.5.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
2525908.12.patch3 KB

rerolled

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
CreditAttribution: legolasbo as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Really? just the use statement?

Assuming testbot agrees i'll mark this RTBC again.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue tags: +SafeMarkup
Parent issue: » #2280965: [meta] Remove every SafeMarkup::set() call

@alexpott this looks like duplicate of #2296101: Remove SafeMarkup::set() use in \Drupal\Core\Render\Element\HtmlTag::preRenderHtmlTag() though this has tests and does pretty much the same thing, so leaving this open, just a heads up.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@joelpittet nope that deals with code below this specifically #value_prefix and #value_suffix and the safe markup set on the whole return value.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@alexpott we covered off this as well in there, that's why i'm linking them and mentioning the overlap.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@alexpott No, you are totally right my bad got these turkeys confused, carry on...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 2525908.12.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented 
Test name	Pass	Fail	Exception
CollapsedDrupal\system\Tests\System\PageTitleTest	123	4	0
Message	Group	Filename	Line	Function	Status
Page title 'Static title | Drupal' is equal to 'Static title translated | Drupal'.	Other	PageTitleTest.php	131	Drupal\system\Tests\System\PageTitleTest->testRoutingTitle()	
Value 'Static title translated' is equal to value 'Static title'.	Other	PageTitleTest.php	133	Drupal\system\Tests\System\PageTitleTest->testRoutingTitle()	
Page title 'Static title | Drupal' is equal to 'Static title translated | Drupal'.	Other	PageTitleTest.php	131	Drupal\system\Tests\System\PageTitleTest->testRoutingTitle()	
Value 'Static title translated' is equal to value 'Static title'.	Other	PageTitleTest.php	133	Drupal\system\Tests\System\PageTitleTest->testRoutingTitle()	
CollapsedDrupal\system\Tests\System\TrustedHostsTest	54	2	0
Message	Group	Filename	Line	Function	Status
Raw "The trusted_host_patterns setting is set to allow" found	Other	TrustedHostsTest.php	62	Drupal\system\Tests\System\TrustedHostsTest->testStatusPageWithConfiguration()	
"Host: ec2-52-2-188-11.compute-1.amazonaws.com" found	Other	TrustedHostsTest.php	83	Drupal\system\Tests\System\TrustedHostsTest->testFakeRequests()

Are the tested that failed... pressing retest.

Status: Needs work » Needs review

alexpott queued 12: 2525908.12.patch for re-testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Looks good to me - thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

So is the plan to fix this, but then to remove it later in #2544318: Remove #type => html_tag usages?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

I think #2544318: Remove #type => html_tag usages is not release blocking and I'm not sure it even qualifies as beta eligible. Whereas the SafeMarkup::set() issues and the security hole I think are. So yes let's fix this and discuss more in #2544318: Remove #type => html_tag usages.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Seeing SafeMarkup::isSafe() makes me uncomfortable due to #2504529: SafeMarkup does not escape some filter tips - remove SafeMarkup usage from FilterHtml - it only tells you that the string was previously marked as safe, not in which context.

@alexpott pointed out that Renderer:render() does the same thing, and this patch just brings HtmlTag into line with that. I'd rather see us remove the feature altogether since it's never been used for its original purpose, but I guess since it's still around we should bring it into line with other elements for now.

Committed/pushed to 8.0.x, thanks!

  • catch committed d88f278 on 8.0.x 
    Issue #2525908 by alexpott, cilefen: HtmlTag render element's prefix and...

Status: Fixed » Closed (fixed)

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