Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#7 | 2557519.7.patch | 97.04 KB | alexpott |
#7 | 4-7-interdiff.txt | 612 bytes | alexpott |
#4 | 2557519.4.patch | 96.63 KB | alexpott |
#4 | 2-4-interdiff.txt | 910 bytes | alexpott |
#2 | 2557519.2.patch | 96.63 KB | alexpott |
Comments
Comment #2
alexpottThis will make #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping considerably smaller and easier to review.
Comment #4
alexpottOops...
Comment #5
joelpittetGot a quarter through the patch and just spotted one thing which was I believe mentioned in the other issue:
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).
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?
Comment #6
alexpottThe 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.
Comment #7
alexpottMissed one... there are probably others but I think we should get this one in since it will reduce the noise in subsequent patches.
Comment #8
alexpott@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:
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).
Comment #9
Wim LeersI 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.
Comment #10
alexpottThanks 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.
Comment #11
catchCommitted/pushed to 8.0.x, thanks!