Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Follow-up to #2280965: [meta] Remove every SafeMarkup::set() call
Problem/Motivation
The Attribute class and it's helper class call SafeMarkup::checkPlain() to sanitize attribute values, but this bloats the list of safe strings when only the final assembled set of attributes needs to be marked safe.
Proposed resolution
Use htmlspecialchars() directly and then call SafeMarkup::set()
in a carefully documented way. (DONE)
Comment | File | Size | Author |
---|---|---|---|
#9 | increment.txt | 438 bytes | pwolanin |
#9 | 2505701-9.patch | 4.1 KB | pwolanin |
#7 | increment.txt | 554 bytes | pwolanin |
#7 | 2505701-7.patch | 3.92 KB | pwolanin |
#5 | increment.txt | 987 bytes | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin at Acquia commentedComment #2
pwolanin CreditAttribution: pwolanin at Acquia commentedComment #4
star-szrCool idea!
Comment #5
pwolanin CreditAttribution: pwolanin at Acquia commentedSomething wrong with my conversion to implode(), but that wasn't really needed anyhow.
Comment #6
akalata CreditAttribution: akalata as a volunteer commentedThis patch documents an acceptable use of SafeMarkup::set, replacing 4 SafeMarkup calls with htmlspecialchars() and only marking the final result as safe.
I did notice a comment in Attribute.php that needs to be updated:
Comment #7
pwolanin CreditAttribution: pwolanin at Acquia commentedThanks, here's a fix for that comment.
Comment #8
YesCT CreditAttribution: YesCT commentedread the whole patch,
and applied it and looked at it in context.
grepped for checkPlain in core/lib/Drupal/Core/Template
and after the patch, the only occurrence is TwigExtension
so, got all them out of the Attribute helpers. ok.
most of the replacements are of the pattern:
and I was wondering how the args to htmlspecialchars was choosen, why ENT_QUOTES and UTF-8?
looking at checkPlain, can see that that is what checkplain was doing anyway, so that makes sense.
got most of the (now) unused use's out... except for the one in AttributeArray.
I think if a patch with that out comes back ok, this is rtbc.
Comment #9
pwolanin CreditAttribution: pwolanin at Acquia commentedComment #10
YesCT CreditAttribution: YesCT commentedas long as it comes back green, rtbc.
Comment #11
star-szrLooks good. Also makes me think we might want a check plain that doesn't mark as safe, similar to what was being discussed in #2399261: Remove SafeMarkup::set and Recheck and Mark Safe the Output of Unicode::truncate() in DbLog. Not sure though.
Comment #12
pwolanin CreditAttribution: pwolanin at Acquia commented@Cottser - maybe, but it's also not hard to use htmlspecialchars()
Partly why we didn't use it directly in Drupal 7 and before and had check_plain() as a wrapper is that it did not include UTF-8 validation.
Comment #13
xjmThis is another great cleanup to reduce SafeMarkup overuse. Thanks @pwolanin.
To clarify #12, checkPlain() is exactly
htmlspecialchars($text, ENT_QUOTES, 'UTF-8')
plus adding the string to the safe strings list. I checked with @pwolanin and it looks like the UTF-8 validation was added tohtmlspecialchars()
following PHP 5.2.5; @pwolanin linked: https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/chec...Can we get a followup to either document on AttributeValueBase::render() (note there isn't an interface...) that child implementations are responsible for sanitizing values?
This issue is a required part of a critical task and is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x.
Comment #15
xjm