Follow-up to #1825952: Turn on twig autoescape by default
SafeMarkup::set() is mostly for internal use only. For the most part, existing APIs like t(), String::checkPlain(), XSS::filter(), drupal_render(), etc. should be marking the things they sanitize, and markup in general should be moved into templates wherever possible so the themer has control of it.
#2280965: [meta] Remove every SafeMarkup::set() call is postponed on this issue's progress.
Remove as many SafeMarkup::set() calls from core as possible.
TBD
| Task | Novice task? | Contributor instructions | Complete? |
|---|---|---|---|
| when this is fixed, unpostpone #2280965: [meta] Remove every SafeMarkup::set() call |
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 996eb1637c7cd46f16c0350623f7fbfae02bf530-safemarkup.txt | 11.95 KB | aneek |
| #10 | modified.txt | 3.49 KB | alsiukola |
| #9 | meta_refactor_and-2297703-9.patch | 9.55 KB | alsiukola |
| #6 | safemarkup.txt | 11.62 KB | aneek |
Comments
Comment #1
xjmComment #2
xjmComment #3
xjmComment #4
aneek commentedWe can work on this issue once #2324371: Fix common HTML escaped render #key values due to Twig autoescape is in core. We can remove
SafeMarkup::set()calls from those sections which pass viadrupal_render(). If needed to callSafeMarkup::set()at any cost, we need to check the string before withSafeMarkup::checkAdminXss()which will be available once #2324371: Fix common HTML escaped render #key values due to Twig autoescape is in core.Comment #5
xjmComment #6
aneek commentedThere are 99 matches to
SafeMarkup::set()in 51 files in core. Not all are necessary to remove or re-factor. But the attached file can help any contributer to find and remove the calls.This find list is based on last commit "36122d4cfc3a421b524d3fde74f219a7cb2301a9" by Alex Pott on Sat Nov 29 09:07:04 2014 +0000.
Comment #7
yesct commentedComment #8
alsiukola commentedIf all interfering issues have been resolved, I can have a look at this.
Comment #9
alsiukola commentedHere is a partial patch. See attached modified.txt for a list of "how far I got".
- Some of these were cleaned by drupal_render() or other deprecated function, I am assuming drupal_render() will be replaced by something similar, so I removed them.
- Lines marked R, I removed the SafeMarkup::set() from. Lines marked U are untouched. ! means I didn't find the line.
- Some lines I changed didn't match the line numbers in safemarkup.txt, but compared to the code snippet, it is the same line.
- Lines in safemarkup.txt that aren't mentioned here (starting where my list ends) I didn't look at.
Comment #10
alsiukola commentedComment #11
alsiukola commentedComment #12
aneek commented@lduerig, I'd suggest the following,
SafeMarkup::set()or refactor it. Instead lets follow the child issues or you may create some.SafeMarkup::set()please test those locally and then queue it for bot's review. There are many instances that by removingSafeMarkup::set()breaks the site.SafeMarkup::set()calls anymore as they are addressed and getting committed. You can also follow the tag "SafeMarkup" for further references.I hope these makes sense. Thanks!
Comment #13
dawehnerThat sounds like a great suggestion. We should lay out the remaining calls in the issue summary and make a list of subissues.
Once done the work can be spread pretty good, I hope.
Comment #14
aneek commented@dawehner, indeed. Lets also discuss in IRC before raising a new issue.
As per the recent HEAD (996eb1637c7cd46f16c0350623f7fbfae02bf530) in 8.0.x branch the attached text file lists all usages of
SafeMarkup::set()calls. Some needs re-factoring in terms of removing or making it more secure based on user input.Comment #15
xjmThanks everyone for your work here so far!
Awhile back, @dawehner mentioned to me that having the critical meta postponed on this major meta makes it so there's not enough visibility on these issues. Looking at the history of this issue and the fact that the postponed issue is the one critical without updates on the issue proper in the past couple weeks, it's clear he's right.
So, I'm going to close this as a duplicate of #2280965: [meta] Remove every SafeMarkup::set() call, unpostpone it, and re-parent the child issues to reference that.