Problem/Motivation

Just before we launched the beta at Drupalcon Amsterdam we commit one of the final criticals #1825952: Turn on twig autoescape by default. This patch created the SafeMarkup class to core and created a static list of strings that are considered safe. Prior to the patch going in there was a lot of discuss about how safe and performant this would be and we stated intentions to try to remove early escaping and filtering and just rely on the renderer and Twig to do the job for us. This never occurred and we continued to add calls SafeMarkup::methods() and expand the list of strings being marked safe. However, as the patches below show this is incredibly tricky to get right and fraught with unexpected side effects. Perhaps the most tricky is that the safe list contains both filtered and escaped code.

Now that the render system and Twig are stable it possible to remove several of the SafeMarkup methods and just use the render system and twig. The result is a simple system with way less possibilities side effects.

I'm making this plan critical because:

  • the current use of SafeMarkup functions in HEAD is a mess and extremely buggy
  • the mix of filter and escape makes it XSS possible
  • the API is totally unpredictable
  • it's consuming memory and resources unnecessarily
  • All of the above is only going to get worse on sites with lots on contrib and custom

Examples for the above list.

1. Mess
    // Also add a HTTP header "Link:".
    $href = '<' . SafeMarkup::checkPlain($attributes['href']) . '>;';

Immediately adding string to a SafeMarkup string makes the entry in the safe string list pointless.
We have bugs coming out of our ears, for example, and many many more undiscovered issues #2531430: Page's <title> double escaped

2. XSS

FilterAdminTest::testFilterTipHtmlEscape() is actually testing that something is not escaped when it should be.

3. Unpredictable
4. Memory

Form state and batch before store lists of all the safe strings. We should do everything we can to minimise this list. Related issue: #2295823: Ensure that we don't store excessive lists of safe strings

5. Contrib

If we're struggling to do it right in HEAD contrib will struggle more and as more strings are in the safe list the chances of XSS and resource usage becoming problematic increase.

Proposed resolution

We try to fix SafeMarkup and maintain the current API however this is going to be more work and continue to leave us with potential problems. We should do everything we can to reduce surface area before release. To quote @catch:

So if we explicitly try to remove it we'll get there quicker than muddling along with it.

Commit the following patches in this order:

  1. #2544684: Expand @internal documentation on SafeString and SafeStringInterface and introduce ViewsRenderPipelineSafeString (blocker)
  2. #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender (blocker)
  3. #2550945: Add Html::escape() (blocker)
  4. #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter (xssFilter) (fixed, but open for change record updates)
  5. #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping (checkPlain)
  6. #2555473: Deprecate SafeMarkup::checkPlain() for Drupal 8.0.x (checkPlain)
  7. #2554889: Remove SafeMarkup::set() from the codebase (set)
  8. #2549393: Remove SafeMarkup::escape() and rely more on Twig autoescaping (escape)
  9. #2575615: Introduce HtmlEscapedText and remove SafeMarkup::setMultiple() and SafeMarkup::getAll() and remove the static safeStrings list (setMultiple and getAll)

Other patches to do:

Remaining tasks

  1. Agree
  2. Do it
  3. Update documentation, including change records: #2494297: [no patch] Consolidate change records relating to safe markup and filtering/escaping to ensure cross references exist
  4. Profit and close #2549943: [plan] Remove as much of the SafeMarkup class's methods as possible

User interface changes

None

API changes

Plenty - see sub issues for details. Biggest changes are the removal of SafeMarkup::checkPlain(), SafeMarkup::escape() and SafeMarkup::xssFilter().

Data model changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Plan because this issue groups together several steps to achieve aim of SafeMarkup sanity
Issue priority Critical because see issue summary above.
Prioritized changes The main goal of this issue is security since making output safe is a key task of the render API and Drupal as a whole
Disruption Highly disruptive for core/contributed and custom modules/themes because it will require a BC break

Comments

alexpott created an issue. See original summary.

alexpott’s picture

I'm yet to complete the beta evaluation because I think we need to discuss this on issue. I think this issue is critical. As shown in the patches linked in the issue summary SafeMarkup and it's list are out of control and dangerous. We've got it wrong in HEAD and we created the system. Contrib will find it much tougher.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Priority: Normal » Critical
Issue summary: View changes

I discussed this at length with @catch. Issue summary updated accordingly.

alexpott’s picture

Issue summary: View changes
Codenator’s picture

+1 for issue! Great idea. This make theming more clean.

arlinsandbulte’s picture

How is this related (or maybe a duplicate) of #2280965: [meta] Remove every SafeMarkup::set() call?

catch’s picture

I asked alexpott the same question in irc.

The answer was that this issue is about all SafeMarkup methods, where as that issue only covers SafeMarkup::set().

Codenator’s picture

May be better rename #2280965: [meta] Remove every SafeMarkup::set() call to this issue then? Or add as related/child issue?

alexpott’s picture

@Codenator I think we should keep the two effort apart.. the scope of #2280965: [meta] Remove every SafeMarkup::set() call is nice a tight and we have plans for all the sub issues that don;t involve the methods that are being removed by the children of this issue.

webchick’s picture

Title: [plan] Remove as much of SafeMarkup as possible » [plan] Remove as much of the SafeMarkup class's methods as possible

I think this is a more accurate title since I almost passed out from stress at the old one. ;)

alexpott’s picture

davidhernandez’s picture

You related this issue to itself? lol

xjm’s picture

This will help resolve #2295823: Ensure that we don't store excessive lists of safe strings. I've referenced that issue in the summary here. We can probably eventually close it as a duplicate.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
Related issues: +#2531430: Page's <title> double escaped
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Pressing save to see the progress :)

Wim Leers’s picture

Pressing save to see the progress :)

:D

alexpott’s picture

Pressing save for progress.

alexpott’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Priority: Critical » Major

Discussed with @alexpott, @effulgentsia, @webchick, and @catch. We agreed to demote this overall plan issue to major, but to promote certain children to critical.

xjm’s picture

xjm’s picture

alexpott’s picture

Issue summary: View changes
Status: Active » Reviewed & tested by the community

We now have one issue left #2575615: Introduce HtmlEscapedText and remove SafeMarkup::setMultiple() and SafeMarkup::getAll() and remove the static safeStrings list therefore I we can close this issue and call it a success! Marking as rtbc so we can celebrate and then close it.

effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Fabulous work, everyone! I'm thrilled that we got this far with it. I think we now have a really nice combination of security, performance, and DX.

Status: Fixed » Closed (fixed)

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