Part of #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping

Problem/Motivation

SafeMarkup::checkPlain() marks strings as safe - there are many usages where this is irrelevant.

Proposed resolution

In Drupal 8 we've added Html::escape() we should use it in place of SafeMarkup::checkPlain() where safeness of the result is not important or preserved.

Remaining tasks

  • Review
  • Commit

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#7 2557519.7.patch97.04 KBalexpott
#7 4-7-interdiff.txt612 bytesalexpott
#4 2557519.4.patch96.63 KBalexpott
#4 2-4-interdiff.txt910 bytesalexpott
#2 2557519.2.patch96.63 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2557519.2.patch96.63 KB

This will make #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping considerably smaller and easier to review.

Status: Needs review » Needs work

The last submitted patch, 2: 2557519.2.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2-4-interdiff.txt910 bytes
new 2557519.4.patch96.63 KB

Oops...

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: +Needs manual testing

Got a quarter through the patch and just spotted one thing which was I believe mentioned in the other issue:

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -433,7 +434,7 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
-        return SafeMarkup::checkPlain($return);
+        return Html::escape($return);

    HUGE +1. Didn't realize we were storing the escaped values on the SafeMarkup list when we've already passed the "point of no escape" (pun intended). 

  2. +++ b/core/modules/comment/comment.tokens.inc
@@ -161,7 +161,7 @@ function comment_tokens($type, $tokens, array $data, array $options, BubbleableM
-          $replacements[$original] = $sanitize ? SafeMarkup::checkPlain($comment->language()->getId()) : $comment->language()->getId();
+          $replacements[$original] = $sanitize ? Html::escape($comment->language()->getId()) : $comment->language()->getId();

    These sanitized tokens likely need to check for double escaping. The automated tests seem to cover that they were escaped but not in a twig template from what I could spot, did I miss something?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

The use of SafeMarkup::checkPlain() in tokens is completely and utterly pointless. If something is building strings up from tokens or replacing tokens within a string (i.e. its whole point) then it has to handle the fact that safe-ness is lost the moment you combine it with other text.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 4-7-interdiff.txt612 bytes
new 2557519.7.patch97.04 KB

Missed one... there are probably others but I think we should get this one in since it will reduce the noise in subsequent patches.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@joelpittet the token system never outputs directly to Twig. Something has to do the token replace on the user input. Whatever that something is has to deal with issues of safe-ness and sanitisation. For example, the Field system permits tokens in the help text for files, it does it like this:

'#description' => $this->fieldFilterXss(\Drupal::token()->replace($this->fieldDefinition->getDescription())),

So this one will pass TRUE to sanitise (that's the default) and then #description will admin filtered by field's special trait. So for example if the description is something like "[site:name] permits blah blah" it'll escape the site name (as it should) and the filter the entire description (as it should).

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

I already thoroughly reviewed #2545972, including manual testing (see #2545972-91: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping). This is just a subset of the changes there (the non-contentious ones). Therefore, RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Thanks for the review @Wim Leers. It's not that the other changes in #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping are particularly contentious - it is just easier to review small patches and spot where a change might introduce security holes. This patch has no possibility of introducing security issues because we're always escaping. There is a small risk of new double escapes but that is minimised here as well.

catch’s picture

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

Committed/pushed to 8.0.x, thanks!

  • catch committed f384961 on 8.0.x 
    Issue #2557519 by alexpott, joelpittet: Remove many usages SafeMarkup::...

Status: Fixed » Closed (fixed)

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