Follow-up to #1825952: Turn on twig autoescape by default

Problem/Motivation

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.

Proposed resolution

Remove as many SafeMarkup::set() calls from core as possible.

Remaining tasks

TBD

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
when this is fixed, unpostpone #2280965: [meta] Remove every SafeMarkup::set() call
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue summary: View changes
xjm’s picture

Assigned: xjm » Unassigned
xjm’s picture

Status: Postponed » Active
aneek’s picture

We 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 via drupal_render(). If needed to call SafeMarkup::set() at any cost, we need to check the string before with SafeMarkup::checkAdminXss() which will be available once #2324371: Fix common HTML escaped render #key values due to Twig autoescape is in core.

xjm’s picture

Issue tags: +SafeMarkup
aneek’s picture

FileSize
11.62 KB

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

YesCT’s picture

Issue summary: View changes
lduerig’s picture

If all interfering issues have been resolved, I can have a look at this.

lduerig’s picture

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

lduerig’s picture

FileSize
3.49 KB
lduerig’s picture

Status: Active » Needs work
aneek’s picture

@lduerig, I'd suggest the following,

  1. Let us not just remove the instances of SafeMarkup::set() or refactor it. Instead lets follow the child issues or you may create some.
  2. Even if it's required to remove SafeMarkup::set() please test those locally and then queue it for bot's review. There are many instances that by removing SafeMarkup::set() breaks the site.
  3. About comment #9, yes there will be many occurrences that you may not find 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!

dawehner’s picture

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

aneek’s picture

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

xjm’s picture

Status: Needs work » Closed (duplicate)
Related issues: +#2280965: [meta] Remove every SafeMarkup::set() call

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