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']) . '>;';
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
- The order of arguments being passed into SafeMarkup::format is significant?!?! #2549393-6: Remove SafeMarkup::escape() and rely more on Twig autoescaping
- SafeMarkup::replace will mark text as unsafe if replacements are unsafe even if the replacement has not been made #2550055-2: Remove SafeMarkup::replace() and use ViewsRenderPipelineSafeString instead
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:
- #2544684: Expand @internal documentation on SafeString and SafeStringInterface and introduce ViewsRenderPipelineSafeString (blocker)
- #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender (blocker)
- #2550945: Add Html::escape() (blocker)
- #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)
- #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping (checkPlain)
- #2555473: Deprecate SafeMarkup::checkPlain() for Drupal 8.0.x (checkPlain)
- #2554889: Remove SafeMarkup::set() from the codebase (set)
- #2549393: Remove SafeMarkup::escape() and rely more on Twig autoescaping (escape)
- #2575615: Introduce HtmlEscapedText and remove SafeMarkup::setMultiple() and SafeMarkup::getAll() and remove the static safeStrings list (setMultiple and getAll)
Other patches to do:
- #2550055: Remove SafeMarkup::replace() and use ViewsRenderPipelineSafeString instead (replace)
- #2558791: "!"-prefixed tokens should Xss::filterAdmin() but not affect safeness (!placeholder XSS-filtering)
- #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand (remove support for !placeholder)
- #2551335: Remove unnecessary SafeMarkup usage in tests and clear the static cache before every run
- #2552579: Remove SafeMarkup::placeholder(), deprecate drupal_placeholder() and stop drupal_placeholder() from marking safe.
Remaining tasks
- Agree
- Do it
- Update documentation, including change records: #2494297: [no patch] Consolidate change records relating to safe markup and filtering/escaping to ensure cross references exist
- 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
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
Comment #2
alexpottI'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.
Comment #3
alexpottComment #4
alexpottComment #5
alexpottI discussed this at length with @catch. Issue summary updated accordingly.
Comment #6
alexpottComment #7
alexpottComment #8
Codenator CreditAttribution: Codenator as a volunteer commented+1 for issue! Great idea. This make theming more clean.
Comment #9
arlinsandbulte CreditAttribution: arlinsandbulte as a volunteer commentedHow is this related (or maybe a duplicate) of #2280965: [meta] Remove every SafeMarkup::set() call?
Comment #10
catchI 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().
Comment #11
Codenator CreditAttribution: Codenator as a volunteer commentedMay be better rename #2280965: [meta] Remove every SafeMarkup::set() call to this issue then? Or add as related/child issue?
Comment #12
alexpott@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.
Comment #13
webchickI think this is a more accurate title since I almost passed out from stress at the old one. ;)
Comment #14
alexpottComment #15
davidhernandezYou related this issue to itself? lol
Comment #16
xjmThis 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.
Comment #17
alexpottComment #18
alexpottComment #19
alexpottComment #20
alexpottComment #21
alexpottPressing save to see the progress :)
Comment #22
Wim Leers:D
Comment #23
alexpottPressing save for progress.
Comment #24
alexpottComment #25
xjmAdding #2554889: Remove SafeMarkup::set() from the codebase to the scope here.
Comment #26
xjmComment #27
xjmDiscussed with @alexpott, @effulgentsia, @webchick, and @catch. We agreed to demote this overall plan issue to major, but to promote certain children to critical.
Comment #28
xjmAdding #2558791: "!"-prefixed tokens should Xss::filterAdmin() but not affect safeness.
Comment #29
xjmExisting issue to disentangle change records: #2494297: [no patch] Consolidate change records relating to safe markup and filtering/escaping to ensure cross references exist
Comment #30
alexpottWe 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.
Comment #31
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFabulous 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.