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)

Files: 
CommentFileSizeAuthor
#9 increment.txt438 bytespwolanin
#9 2505701-9.patch4.1 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,866 pass(es). View
#7 increment.txt554 bytespwolanin
#7 2505701-7.patch3.92 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,906 pass(es). View
#5 increment.txt987 bytespwolanin
#5 2505701-5.patch3.6 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,869 pass(es). View
#2 2505701-2.patch3.77 KBpwolanin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 57,481 pass(es), 22,361 fail(s), and 1,259 exception(s). View

Comments

pwolanin’s picture

pwolanin’s picture

Status: Active » Needs review
FileSize
3.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 57,481 pass(es), 22,361 fail(s), and 1,259 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 2: 2505701-2.patch, failed testing.

Cottser’s picture

Issue tags: +Twig

Cool idea!

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,869 pass(es). View
987 bytes

Something wrong with my conversion to implode(), but that wasn't really needed anyhow.

akalata’s picture

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

This 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:

 * The attribute keys and values are automatically sanitized for output with
 * \Drupal\Component\Utility\SafeMarkup::checkPlain().
pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,906 pass(es). View
554 bytes

Thanks, here's a fix for that comment.

YesCT’s picture

Title: Use htmlspecialchars() directly in Attribute() so we don't bloat the list of safe strings » Document SafeMarkup::set and Use htmlspecialchars() directly in Attribute() so we don't bloat the list of safe strings
Status: Needs review » Needs work

read 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:

-    return $this->value === FALSE ? '' : SafeMarkup::checkPlain($this->name);
+    return $this->value === FALSE ? '' : htmlspecialchars($this->name, ENT_QUOTES, 'UTF-8');

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.

  public static function checkPlain($text) {
    $string = htmlspecialchars($text, ENT_QUOTES, 'UTF-8');
    static::$safeStrings[$string]['html'] = TRUE;
    return $string;
  }

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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,866 pass(es). View
438 bytes
YesCT’s picture

Status: Needs review » Reviewed & tested by the community

as long as it comes back green, rtbc.

Cottser’s picture

Looks 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.

pwolanin’s picture

@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.

xjm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

This 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 to htmlspecialchars() 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.

  • xjm committed fd67183 on 8.0.x
    Issue #2505701 by pwolanin, YesCT, akalata: Document SafeMarkup::set and...
xjm’s picture

Status: Fixed » Closed (fixed)

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