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().
Comment | File | Size | Author |
---|---|---|---|
#12 | 2525908.12.patch | 3 KB | alexpott |
#6 | 2525908.5.patch | 3.19 KB | alexpott |
#6 | 4-5-interdiff.txt | 2.85 KB | alexpott |
#6 | 2525908.5.test-only.patch | 1.94 KB | alexpott |
#4 | htmltag_render-2525908-4.patch | 1.17 KB | cilefen |
Comments
Comment #1
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #2
cilefen CreditAttribution: cilefen commentedIs it as simple as this?
Comment #3
cilefen CreditAttribution: cilefen commentedNo - I should read the isSafe() docs...
Comment #4
cilefen CreditAttribution: cilefen commentedComment #6
alexpottHere's a test... also I'm not sure about the readability of nested ternary statements.
Comment #8
xjmHmm. 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?Comment #9
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.
Comment #10
legolasboI've reviewed the patch and the code looks clean to me.
Comment #12
alexpottrerolled
Comment #13
legolasboReally? just the use statement?
Assuming testbot agrees i'll mark this RTBC again.
Comment #14
joelpittet@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.
Comment #15
alexpott@joelpittet nope that deals with code below this specifically
#value_prefix
and#value_suffix
and the safe markup set on the whole return value.Comment #16
joelpittet@alexpott we covered off this as well in there, that's why i'm linking them and mentioning the overlap.
Comment #17
joelpittet@alexpott No, you are totally right my bad got these turkeys confused, carry on...
Comment #19
alexpottAre the tested that failed... pressing retest.
Comment #21
larowlanLooks good to me - thanks!
Comment #22
catchSo is the plan to fix this, but then to remove it later in #2544318: Remove #type => html_tag usages?
Comment #23
alexpottI 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.
Comment #24
catchSeeing 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!