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.

Proposed resolution

For every SafeMarkup::set() call, create an major issue to do remove it:

  • Determine where the string(s) from the call are displayed so that it can be tested for double escaping bugs.
  • Identify (or add) automated test coverage for the string being escaped when appropriate, and not double-escaped when not appropriate.
  • Determine if the call can simply be removed, convert it to a template, or convert it to one of the other correct APIs for string and markup handling (e.g. a template). Reference: https://www.drupal.org/node/2311123
  • If necessary, refactor the code internally to make it clear that the string is safe. (E.g., strings should not be concatenated together across many lines.)

Child issues should be major unless the issue is individually critical (e.g., we confirm that unsanitized markup is getting marked as safe). Tag issues with "SafeMarkup" and set this issue as the parent. Some issues may already exist; see the sidebar.

For existing double-escaping bugs in HEAD, see: #2297711: Fix HTML escaping due to Twig autoescape

Theme System Security Documentation - Draft

https://docs.google.com/document/d/1cCHtwLxVgVhvScOPt39iYpYixQw_9QdJtmo4tZpXcn4/edit?usp=sharing

Common Issue Patterns

Below are issue patterns that have been identified so far.

  1. Concatenating messages (exceptions, requirements, etc.)
    • Recommended Solution: Refactor
  2. Concatenating in a loop (drupal_render etc.)
    • Recommended Solution: Refactor/document
  3. Passing strings between requests (Batch API, form cache, drupal_set_message, etc.)
    • Recommended Solution: Document
  4. Imploding safe strings

Remaining tasks

N/A


Done

  1. core/modules/system/tests/modules/batch_test/batch_test.callbacks.inc
  2. #2547851: SafeMarkup::format() should require arguments without them it is just SafeMarkup::set() in disguise
  3. core/modules/system/src/Tests/Form/ValidationTest.php
  4. core/themes/engines/twig/twig.engine
  5. core/includes/errors.inc AND core/lib/Drupal/Core/Utility/Error.php
  6. core/lib/Drupal/Core/StringTranslation/TranslationManager.php
  7. SafeMarkup::format() with no arguments is SafeMarkup::set() in disguise
  8. PHPUnit tests
  9. #2550991: Remove or document SafeMarkup::set in ViewListBuilder::buildRow()
  10. core/lib/Drupal/Core/Template/Attribute.php
  11. core/modules/system/system.install
  12. core/lib/Drupal/Core/Form/FormCache.php
  13. core/includes/batch.inc
  14. core/includes/bootstrap.inc
  15. core/includes/install.inc
  16. core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php
  17. core/modules/file/file.module
  18. core/modules/views/src/Plugin/views/field/FieldPluginBase.php
  19. core/modules/views/src/Plugin/views/style/Rss.php #2381277: Make Views use render caching and remove Views' own "output caching"
  20. core/lib/Drupal/Core/Field/AllowedTagsXssTrait.php
  21. core/lib/Drupal/Core/Utility/LinkGenerator.php
  22. core/modules/search/search.module
  23. core/modules/search/tests/modules/search_embedded_form/search_embedded_form.module
  24. core/modules/simpletest/src/Form/SimpletestResultsForm.php
  25. core/modules/views_ui/src/ViewUI.php
  26. core/modules/views_ui/views_ui.theme.inc
  27. core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
  28. core/modules/color/color.module
  29. core/lib/Drupal/Component/Utility/Xss.php
  30. core/modules/search/tests/modules/search_extra_type/src/Plugin/Search/SearchExtraTypeSearch.php
  31. core/modules/views/src/Plugin/views/style/StylePluginBase.php
  32. core/lib/Drupal/Core/Render/Renderer.php
  33. core/includes/form.inc
  34. core/tests/Drupal/Tests/Core/Form/FormCacheTest.php
  35. core/modules/file/file.field.inc
  36. core/modules/dblog/src/Controller/DbLogController.php
  37. core/modules/ckeditor/ckeditor.admin.inc
  38. core/modules/system/system.admin.inc
  39. core/lib/Drupal/Core/Database/Install/Tasks.php
  40. core/modules/dblog/src/Tests/Views/ViewsIntegrationTest.php
  41. core/includes/common.inc
  42. core/lib/Drupal/Core/Render/Element/HtmlTag.php
  43. core/lib/Drupal/Core/Theme/ThemeManager.php
  44. core/themes/engines/twig/twig.engine
  45. core/modules/filter/src/Plugin/Filter/FilterCaption.php
  46. core/modules/node/src/Plugin/Search/NodeSearch.php
  47. core/modules/update/update.module
  48. core/modules/user/src/Tests/UserBlocksTest.php
  49. core/modules/shortcut/src/Tests/ShortcutSetsTest.php
  50. core/modules/rdf/rdf.module
  51. core/modules/rest/src/Plugin/views/display/RestExport.php
  52. core/modules/views_ui/src/Controller/ViewsUIController.php
  53. core/modules/views_ui/src/ViewListBuilder.php
  54. core/modules/views/views.theme.inc
  55. core/modules/field_ui/src/FieldStorageConfigListBuilder.php
  56. ViewsUIController::reportPlugins()
  57. TranslationManager::translate()
  58. AllowedTagsXssTrait
    1. core/lib/Drupal/Core/Form/FormErrorHandler.php
Files: 

Comments

crowdcg’s picture

Status: Active » Postponed
xjm’s picture

Status: Postponed » Active
chx’s picture

catch’s picture

Status: Active » Postponed

That seems reasonable, this issue then just has to do a sweep of all existing calls when either that issue is resolved or the critical count overall is much lower.

catch’s picture

Issue summary: View changes
xjm’s picture

Title: Document every SafeMarkup::create() call » Document every SafeMarkup::set() call
xjm’s picture

Title: Document every SafeMarkup::set() call » [meta] Document or remove every SafeMarkup::set() call
Related issues: +#2297703: [meta] Refactor and remove as many SafeMarkup::set() calls as possible

So what is actually critical is to either document or remove every call. Let's recategorize this as a meta issue, working first through sub-issue #2297703: [meta] Refactor and remove as many SafeMarkup::set() calls as possible.

xjm’s picture

Issue tags: +SafeMarkup
xjm’s picture

Issue tags: +Triaged D8 critical
Cottser’s picture

Issue tags: +Twig
xjm’s picture

So, as @dawehner and others have pointed out, it's weird to have a critical postponed on a major, and it's resulting in the issues not getting the focus they need. Therefore, I'm merging the two metas and unpostponing this.

Child issues of this should be filed as major, unless they include something that makes them critical on their own, like #2273925: Ensure #markup is XSS escaped in Renderer::doRender().

Started updating the summary to the new scope and some more specific action items.

xjm’s picture

xjm’s picture

Issue summary: View changes

#2273925: Ensure #markup is XSS escaped in Renderer::doRender() is the place to help right now. There's 3-4 questions on it that I had that could use feedback, and then it needs someone to do a final code review to potentially re-RTBC.

For now, I'd suggest doing any research for the rest of the issues with the patch from that issue applied.

xjm’s picture

Issue summary: View changes

Adding the remaining calls to a list in the summary.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes

Existing issues related to each call.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

The latest patch in #2296929: Remove system_requirements() SafeMarkup::set() use is interesting and worth review and discussion for how we proceed with other issues as well.

effulgentsia’s picture

https://www.drupal.org/node/2311123 lists inline_template as option 2 and SafeMarkup::format() as option 4, but IMO, we should reverse that priority and favor SafeMarkup::format() over inline_template. There are some benefits to inline_template over SafeMarkup::format(), such as ability for alter hooks to manipulate the variables and the ability to use Twig control structures, but IMO, use cases where those benefits matter should probably be promoted to full Twig file templates (which is, and should remain, option 1).

Any thoughts from folks here on that before we update that CR?

dawehner’s picture

IMHO inline template should be used for cases, where things really actually belong into a template, so complex cases, with a lot of HTML inside

Cottser’s picture

Title: [meta] Document or remove every SafeMarkup::set() call » [meta] Remove or document every SafeMarkup::set() call
Issue tags: +D8 Accelerate

We should be removing first, documenting if we can't.

cwells’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
cwells’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
Cottser’s picture

At the sprint we are going to work on the call in drupal_check_module() in core/includes/install.inc, I'm claiming it here because I want to show creating the issue as well.

Cottser’s picture

Issue summary: View changes
dtraft’s picture

Issue summary: View changes
sclapp’s picture

Issue summary: View changes
leslieg’s picture

Issue summary: View changes
cdulude’s picture

Issue summary: View changes
cwells’s picture

Issue summary: View changes
cwells’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
Daniel_Rose’s picture

Issue summary: View changes
sclapp’s picture

Issue summary: View changes
sclapp’s picture

Issue summary: View changes
peezy’s picture

Issue summary: View changes
lokapujya’s picture

Issue summary: View changes
sclapp’s picture

Issue summary: View changes
cwells’s picture

Issue summary: View changes
peezy’s picture

Issue summary: View changes
leslieg’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
peezy’s picture

Issue summary: View changes
leslieg’s picture

Issue summary: View changes
cwells’s picture

Issue summary: View changes
kbaringer’s picture

Issue summary: View changes

Added "Common Issue Patterns" section to issue description.

cdulude’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes

Adding a few that use the implode pattern but don't have issues yet.

cwells’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
Cottser’s picture

kbaringer’s picture

Here is the workflow used during NHDevDay #2 to tackle the list of child issues in this ticket.

Cottser’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
joelpittet’s picture

Issue summary: View changes
lokapujya’s picture

Issue summary: View changes
joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

joelpittet’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
lokapujya’s picture

Issue summary: View changes
joelpittet’s picture

Issue summary: View changes
webchick’s picture

Category: Task » Plan
xjm’s picture

Just a note that #2502095: Remove SafeMarkup::set in template_preprocess_views_ui_view_info() is an excellent example of addressing the issue with best practices for themeable output. :) Go team!

joelpittet’s picture

Issue summary: View changes

Added security documentation draft to the the issue summary as a google doc, feel free to edit it there.

https://www.drupal.org/node/2280965#docs

xjm’s picture

Issue summary: View changes

I merged two of the issues; updating the summary appropriately.

cilefen’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
cilefen’s picture

Issue summary: View changes
cilefen’s picture

Issue summary: View changes
cilefen’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
effulgentsia’s picture

Nice to see a growing number of fixed issues in the currently-48-item list that's in the summary. At some point, it'll probably be helpful to split the list into two (i.e., "Remaining" and "Done") for easier scanning of what's not done yet. But I leave it to you all to decide on when the right time for that is.

joelpittet’s picture

Issue summary: View changes

Shuffled

joelpittet’s picture

Issue summary: View changes

Moar shuffle.

aneek’s picture

Issue summary: View changes
aneek’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes

Shuffling down.

webchick’s picture

Issue tags: +Actionable D8 critical

Marking this as one of the better D8 criticals in terms of issue summary, docs, etc. for people to work on.

xjm’s picture

Issue tags: +D8 Accelerate London
lauriii’s picture

davidhernandez’s picture

Issue summary: View changes
hass’s picture

Could someone give me a hint why http://cgit.drupalcode.org/google_analytics/tree/src/Form/GoogleAnalytic... shows now <div class="description"> and how I should fix it now, please?

cilefen’s picture

@hass Wrap it in a Safemarkup::format() to not have the combined string escaped.

crowdcg’s picture

cilefen’s picture

Issue summary: View changes
cilefen’s picture

cmanalansan’s picture

Issue summary: View changes
alimac’s picture

davidhernandez’s picture

Issue summary: View changes
akalata’s picture

Issue summary: View changes
YesCT’s picture

Issue summary: View changes

taking out the location
core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php

from the list since there is no call to set in there.

(the other locations are still listed)

xjm’s picture

Issue summary: View changes
xjm’s picture

@YesCT, @alexpott, @effulgentsia, @joelpittet, @cilefen, @lauriii, @davidhernandez, @joelpittet, and I discussed all the outstanding children of this meta today. Notes from the discussion are here:
https://docs.google.com/document/d/1wBSwpCa0tm_CKAV1_R0xTAdKZSsVSUc2fQuS...

alexpott’s picture

Issue summary: View changes
RavindraSingh’s picture

RavindraSingh’s picture

Issue summary: View changes
RavindraSingh’s picture

Issue summary: View changes
harjotsingh’s picture

Issue summary: View changes
davidhernandez’s picture

Issue summary: View changes
harjotsingh’s picture

core/modules/rest/src/Plugin/views/display/RestExport.php also contains SafeMarkup::set() call, shouldn't it be added here ?

joelpittet’s picture

@harjotsingh yes please, though it's documented it may be nice to have it here as well. It's referencing #2501313: Discuss how to support non-HTML output in the render system

harjotsingh’s picture

Issue summary: View changes
harjotsingh’s picture

core/modules/user/src/Tests/UserBlocksTest.php
core/modules/shortcut/src/Tests/ShortcutSetsTest.php
core/modules/system/src/Tests/Form/ValidationTest.php
core/modules/filter/src/Element/ProcessedText.php
core/tests/Drupal/Tests/Core/Render/Element/HtmlTagTest.php
core/lib/Drupal/Core/Form/FormErrorHandler.php
All these contains SafeMarkup::set() call but aren't added over here.

alexpott’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
davidhernandez’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
davidhernandez’s picture

Issue summary: View changes
davidhernandez’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
davidhernandez’s picture

Issue summary: View changes
akalata’s picture

Issue summary: View changes
akalata’s picture

Issue summary: View changes

Adding known blockers to the issue summary.

alexpott’s picture

#2544684: Expand @internal documentation on SafeString and SafeStringInterface and introduce ViewsRenderPipelineSafeString has introduced the possibility module specific versions of SafeString for the rare instance when a module injects itself into the render pipeline and needs to communicate safeness back to the render pipeline. That patch introduces ViewsRenderPipeLineSafeString for Views and #2547741: Introduce FilteredString and get rid of all SafeMarkup::set() calls in the Filter module introduce FilteredString for the Filter module. This offers new ways to remove SafeMarkup::set()'s in things like View's RestExport plugin.

akalata’s picture

Issue summary: View changes

updating blocker list

josephdpurcell’s picture

I have created all issues for #115.

I did not find calls in:
core/modules/filter/src/Element/ProcessedText.php
core/tests/Drupal/Tests/Core/Render/Element/HtmlTagTest.php

josephdpurcell’s picture

Issue summary: View changes
josephdpurcell’s picture

Issue summary: View changes
justAChris’s picture

akalata’s picture

Issue summary: View changes
akalata’s picture

Issue summary: View changes
alexpott’s picture

alexpott’s picture

Issue summary: View changes

We should actually be able to remove SafeMarkup::set(). I think this issue should be retitled. Adding a new issue to deal with AllowedTagsXssTrait.

josephdpurcell’s picture

Issue summary: View changes
mpdonadio’s picture

FileSize
8.89 KB

Haven't cross referenced this with the issue list, but here is the current usage report from PhpStorm for `git rev-parse --short HEAD` == 'e415ad4'.

justAChris’s picture

Issue summary: View changes

Moved Fixed items in IS to done

JeroenT’s picture

Issue summary: View changes
JeroenT’s picture

Issue summary: View changes

The following issues are now fixed:

  1. core/modules/shortcut/src/Tests/ShortcutSetsTest.php
  2. core/modules/rdf/rdf.module
joelpittet’s picture

Issue summary: View changes

Moving blocked below to help see the colors better.

joelpittet’s picture

Issue summary: View changes

Suffle the fixed and unblocked

joelpittet’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
justAChris’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
davidhernandez’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
realdream’s picture

$link_options = array(
    'attributes' => array('class' => array($link_class)),
    'query' => $params,
);
$image = '<img src="/images/arrow.png" title="down" >';
//$link_image = \Drupal::l(SafeMarkup::set($image), new Url('<current>', [], $link_options));
$link_image = \Drupal::l($image, new Url('<current>', [], $link_options));
debug($link_image);

How to display a image link without SafeMarkup::set ?

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
alexpott’s picture

@realdream so at least the title "down" need to be translatable. But also adding an image such as an arrow to a link should be done with CSS.

alexpott’s picture

I propose that we re-title this issue to be "[meta] Remove every SafeMarkup::set() call" and add #2554889: Remove SafeMarkup::set() from the codebase as the final patch to commit.

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes

All issues unblocked :) Down to a single list of issues to tackle!

alexpott’s picture

Title: [meta] Remove or document every SafeMarkup::set() call » [meta] Remove every SafeMarkup::set() call
Issue summary: View changes

Retitling.

stefan.r’s picture

Issue summary: View changes
stefan.r’s picture

Issue summary: View changes
stefan.r’s picture

Issue summary: View changes
cilefen’s picture

alexpott’s picture

Issue summary: View changes
stefan.r’s picture

Issue summary: View changes
stefan.r’s picture

Issue summary: View changes
stefan.r’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
alexpott’s picture

Updating issue credit to include everyone who has commented.

xjm’s picture

Issue summary: View changes

@alexpott and I agreed to move #2554889: Remove SafeMarkup::set() from the codebase to the other meta on account of the fact that it includes an API break.

davidhernandez’s picture

I assume it is time for one last search of the code base?

mpdonadio’s picture

FileSize
1.48 KB

Here is a usage report from PhpStorm against HEAD == f82e0a3.

heddn’s picture

Status: Active » Reviewed & tested by the community

Looks like the single outstanding "usage" is covered by #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages(). Can we move the critical to it and close this meta? #2554889: Remove SafeMarkup::set() from the codebase is open to remove the functionality of set, so I can't think of any reason to keep this open. With that in mind, I'm marking this RTBC.

cilefen’s picture

stefan.r’s picture

Issue summary: View changes
xjm’s picture

Status: Reviewed & tested by the community » Fixed

4 sprints, 444 days, and ohsomany discussions and debates and forays into Drupal's dark corners later.... this issue is fixed!

Every SafeMarkup::set() call in core is gone, aside from those in the class itself and its tests. (Plus a couple of dangling comment references that should also be removed in #2554889: Remove SafeMarkup::set() from the codebase).

Well done all!

webchick’s picture

AMAZING work, all!! :D

dsnopek’s picture

Woohoo! Congrats to everyone who poured so much time and energy into this! The Drupal community owes you. :-)

Cottser’s picture

almaudoh’s picture

Awesome news!!!

davidhernandez’s picture

Good work, everyone. Thanks, in particular, stefan.r for working really hard the past couple weeks to finish this.

Wim Leers’s picture

Good work, everyone. Thanks, in particular, stefan.r for working really hard the past couple weeks to finish this.

+1 — thanks, Stefan!

cwells’s picture

A lot of the movement on this really got underway at a D8 Accelerate funded code sprint!

Status: Fixed » Closed (fixed)

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