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.
- Concatenating messages (exceptions, requirements, etc.)
- Recommended Solution: Refactor
- Concatenating in a loop (drupal_render etc.)
- Recommended Solution: Refactor/document
- Passing strings between requests (Batch API, form cache, drupal_set_message, etc.)
- Recommended Solution: Document
- Imploding safe strings
- Recommended Solution: use the item-list template such as in #2505931: Remove SafeMarkup::set in ViewListBuilder
Remaining tasks
N/A
Done
- core/modules/system/tests/modules/batch_test/batch_test.callbacks.inc
- #2547851: SafeMarkup::format() should require arguments without them it is just SafeMarkup::set() in disguise
- core/modules/system/src/Tests/Form/ValidationTest.php
- core/themes/engines/twig/twig.engine
- core/includes/errors.inc AND core/lib/Drupal/Core/Utility/Error.php
- core/lib/Drupal/Core/StringTranslation/TranslationManager.php
- SafeMarkup::format() with no arguments is SafeMarkup::set() in disguise
- PHPUnit tests
- #2550991: Remove or document SafeMarkup::set in ViewListBuilder::buildRow()
- core/lib/Drupal/Core/Template/Attribute.php
- core/modules/system/system.install
- core/lib/Drupal/Core/Form/FormCache.php
- core/includes/batch.inc
- core/includes/bootstrap.inc
- core/includes/install.inc
- core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php
- core/modules/file/file.module
- core/modules/views/src/Plugin/views/field/FieldPluginBase.php
- core/modules/views/src/Plugin/views/style/Rss.php #2381277: Make Views use render caching and remove Views' own "output caching"
- core/lib/Drupal/Core/Field/AllowedTagsXssTrait.php
- core/lib/Drupal/Core/Utility/LinkGenerator.php
- core/modules/search/search.module
- core/modules/search/tests/modules/search_embedded_form/search_embedded_form.module
- core/modules/simpletest/src/Form/SimpletestResultsForm.php
- core/modules/views_ui/src/ViewUI.php
- core/modules/views_ui/views_ui.theme.inc
- core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
- core/modules/color/color.module
- core/lib/Drupal/Component/Utility/Xss.php
- core/modules/search/tests/modules/search_extra_type/src/Plugin/Search/SearchExtraTypeSearch.php
- core/modules/views/src/Plugin/views/style/StylePluginBase.php
- core/lib/Drupal/Core/Render/Renderer.php
- #2273925: Ensure #markup is XSS escaped in Renderer::doRender()
only covers one use and will need a followup. - #2506581: Remove SafeMarkup::set() from Renderer::doRender
- #2273925: Ensure #markup is XSS escaped in Renderer::doRender()
- core/includes/form.inc
- core/tests/Drupal/Tests/Core/Form/FormCacheTest.php
- core/modules/file/file.field.inc
- core/modules/dblog/src/Controller/DbLogController.php
- core/modules/ckeditor/ckeditor.admin.inc
- core/modules/system/system.admin.inc
- core/lib/Drupal/Core/Database/Install/Tasks.php
- core/modules/dblog/src/Tests/Views/ViewsIntegrationTest.php
- core/includes/common.inc
- core/lib/Drupal/Core/Render/Element/HtmlTag.php
- core/lib/Drupal/Core/Theme/ThemeManager.php
- core/themes/engines/twig/twig.engine
- core/modules/filter/src/Plugin/Filter/FilterCaption.php
- core/modules/node/src/Plugin/Search/NodeSearch.php
- core/modules/update/update.module
- core/modules/user/src/Tests/UserBlocksTest.php
- core/modules/shortcut/src/Tests/ShortcutSetsTest.php
- core/modules/rdf/rdf.module
- core/modules/rest/src/Plugin/views/display/RestExport.php
- core/modules/views_ui/src/Controller/ViewsUIController.php
- core/modules/views_ui/src/ViewListBuilder.php
- core/modules/views/views.theme.inc
- core/modules/field_ui/src/FieldStorageConfigListBuilder.php
- ViewsUIController::reportPlugins()
- TranslationManager::translate()
- AllowedTagsXssTrait
- core/lib/Drupal/Core/Form/FormErrorHandler.php
Comment | File | Size | Author |
---|---|---|---|
#172 | report-f82e0a3.txt | 1.48 KB | mpdonadio |
#59 | Replacement-Workflow-for_SafeMarkup-set.txt | 1.5 KB | kbaringer |
Comments
Comment #1
seantwalshComment #2
xjmComment #3
chx CreditAttribution: chx commentedShould we set this postponed on #2297703: [meta] Refactor and remove as many SafeMarkup::set() calls as possible ?
Comment #4
catchThat 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.
Comment #5
catchComment #6
xjmComment #7
xjmSo 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.
Comment #8
xjmComment #9
xjmComment #10
star-szrTagging for https://drupaltwig.org.
Comment #11
xjmSo, 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.
Comment #12
xjmComment #13
xjm#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.
Comment #14
xjmAdding the remaining calls to a list in the summary.
Comment #15
xjmComment #16
xjmExisting issues related to each call.
Comment #17
xjmComment #18
xjmComment #19
xjmThe 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.
Comment #20
effulgentsia CreditAttribution: effulgentsia at Acquia commentedhttps://www.drupal.org/node/2311123 lists
inline_template
as option 2 andSafeMarkup::format()
as option 4, but IMO, we should reverse that priority and favorSafeMarkup::format()
overinline_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?
Comment #21
dawehnerIMHO inline template should be used for cases, where things really actually belong into a template, so complex cases, with a lot of HTML inside
Comment #22
star-szrWe should be removing first, documenting if we can't.
Comment #23
chrisfromredfinComment #24
star-szrComment #25
chrisfromredfinComment #26
star-szrComment #27
star-szrComment #28
star-szrComment #29
star-szrComment #30
star-szrComment #31
star-szrAt 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.
Comment #32
star-szrComment #33
dtraft CreditAttribution: dtraft as a volunteer commentedComment #34
sclapp CreditAttribution: sclapp as a volunteer commentedComment #35
leslieg CreditAttribution: leslieg as a volunteer commentedComment #36
cdulude CreditAttribution: cdulude commentedComment #37
chrisfromredfinComment #38
chrisfromredfinComment #39
star-szrComment #40
dani3lr0se CreditAttribution: dani3lr0se commentedComment #41
sclapp CreditAttribution: sclapp as a volunteer commentedComment #42
sclapp CreditAttribution: sclapp as a volunteer commentedComment #43
peezy CreditAttribution: peezy commentedComment #44
lokapujyaComment #45
sclapp CreditAttribution: sclapp as a volunteer commentedComment #46
chrisfromredfinComment #47
peezy CreditAttribution: peezy commentedComment #48
leslieg CreditAttribution: leslieg as a volunteer commentedComment #49
star-szrComment #50
peezy CreditAttribution: peezy commentedComment #51
leslieg CreditAttribution: leslieg as a volunteer commentedComment #52
chrisfromredfinComment #53
kbaringer CreditAttribution: kbaringer as a volunteer commentedAdded "Common Issue Patterns" section to issue description.
Comment #54
cdulude CreditAttribution: cdulude commentedComment #55
star-szrAdding a few that use the implode pattern but don't have issues yet.
Comment #56
chrisfromredfinComment #57
star-szrComment #58
star-szrRelated to pattern #4: #2501975: Determine how to update code that currently joins strings in SafeMarkup::set()
Comment #59
kbaringer CreditAttribution: kbaringer as a volunteer commentedHere is the workflow used during NHDevDay #2 to tackle the list of child issues in this ticket.
Comment #60
star-szrComment #61
star-szrComment #62
star-szrComment #63
star-szrComment #64
joelpittetComment #65
lokapujyaComment #66
joelpittetComment #67
joelpittetAdding #2502095: Remove SafeMarkup::set in template_preprocess_views_ui_view_info()
Comment #68
joelpittetComment #69
star-szrComment #70
lokapujyaComment #71
joelpittetComment #72
webchickTentatively testing our new "Plan" issue type now that #2504039: Adjust critical issue counter/link at /drupal-8.0/get-involved to account for "Plan" issue category is in.
Comment #73
xjmJust 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!
Comment #74
joelpittetAdded 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
Comment #75
xjmI merged two of the issues; updating the summary appropriately.
Comment #76
cilefen CreditAttribution: cilefen commented#2501949: Use the ternary operator for $description_build in Drupal\node\Plugin\views\row\RSS::render() was fixed in another patch and has changed into a minor cleanup.
Comment #77
xjmComment #78
cilefen CreditAttribution: cilefen commentedComment #79
cilefen CreditAttribution: cilefen commentedComment #80
cilefen CreditAttribution: cilefen commentedComment #81
alexpottComment #82
alexpottComment #83
alexpottComment #84
effulgentsia CreditAttribution: effulgentsia at Acquia commentedNice 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.
Comment #85
joelpittetShuffled
Comment #86
joelpittetMoar shuffle.
Comment #87
aneek CreditAttribution: aneek as a volunteer commentedComment #88
aneek CreditAttribution: aneek as a volunteer commentedComment #89
star-szrShuffling down.
Comment #90
webchickMarking this as one of the better D8 criticals in terms of issue summary, docs, etc. for people to work on.
Comment #91
xjmComment #92
lauriii#2501701: Remove SafeMarkup::set in template_preprocess_color_scheme_form() is fixed
Comment #93
davidhernandezComment #94
hass CreditAttribution: hass commentedCould 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?Comment #95
cilefen CreditAttribution: cilefen commented@hass Wrap it in a Safemarkup::format() to not have the combined string escaped.
Comment #96
seantwalsh#2502009: Remove SafeMarkup::set in SearchExtraTypeSearch::execute() is fixed!
Comment #97
cilefen CreditAttribution: cilefen commentedComment #98
cilefen CreditAttribution: cilefen commented#2539300: Remove SafeMarkup::set in \Drupal\Tests\Core\Form\FormCacheTest::testSetCacheWithSafeStrings()
Comment #99
cmanalansan CreditAttribution: cmanalansan commentedComment #100
alimac CreditAttribution: alimac at University of Illinois at Chicago commentedMoving
core/lib/Drupal/Core/Render/Renderer.php
to Done.
Comment #101
davidhernandezComment #102
akalata CreditAttribution: akalata commentedComment #103
YesCT CreditAttribution: YesCT commentedtaking 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)
Comment #104
xjmComment #105
xjm@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...
Comment #106
alexpottComment #107
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedwoohoo one more issue is closed #2502781: Remove SafeMarkup::set in template_preprocess_file_widget_multiple()
Comment #108
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedComment #109
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedComment #110
harjotsingh CreditAttribution: harjotsingh as a volunteer commentedComment #111
davidhernandezComment #112
harjotsingh CreditAttribution: harjotsingh as a volunteer commentedcore/modules/rest/src/Plugin/views/display/RestExport.php also contains SafeMarkup::set() call, shouldn't it be added here ?
Comment #113
joelpittet@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
Comment #114
harjotsingh CreditAttribution: harjotsingh as a volunteer commentedComment #115
harjotsingh CreditAttribution: harjotsingh as a volunteer commentedcore/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.
Comment #116
alexpottComment #117
Wim LeersComment #118
davidhernandezComment #119
xjmComment #120
davidhernandezComment #121
davidhernandezComment #122
alexpottComment #123
davidhernandezComment #124
akalata CreditAttribution: akalata commentedComment #125
akalata CreditAttribution: akalata commentedAdding known blockers to the issue summary.
Comment #126
alexpott#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 introduceFilteredString
for the Filter module. This offers new ways to removeSafeMarkup::set()
's in things like View'sRestExport
plugin.Comment #127
akalata CreditAttribution: akalata commentedupdating blocker list
Comment #128
josephdpurcell CreditAttribution: josephdpurcell commentedI 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
Comment #129
josephdpurcell CreditAttribution: josephdpurcell commentedComment #130
josephdpurcell CreditAttribution: josephdpurcell commentedComment #131
justAChris CreditAttribution: justAChris as a volunteer commentedAdded following children:
#2550985: Remove SafeMarkup::set in _batch_test_finished_helper()
#2550991: Remove or document SafeMarkup::set in ViewListBuilder::buildRow()
Comment #132
akalata CreditAttribution: akalata commentedComment #133
akalata CreditAttribution: akalata commentedComment #134
alexpottComment #135
alexpottWe should actually be able to remove SafeMarkup::set(). I think this issue should be retitled. Adding a new issue to deal with AllowedTagsXssTrait.
Comment #136
josephdpurcell CreditAttribution: josephdpurcell commentedComment #137
mpdonadioHaven't cross referenced this with the issue list, but here is the current usage report from PhpStorm for `git rev-parse --short HEAD` == 'e415ad4'.
Comment #138
justAChris CreditAttribution: justAChris as a volunteer commentedMoved Fixed items in IS to done
Comment #139
JeroenTComment #140
JeroenTThe following issues are now fixed:
Comment #141
joelpittetMoving blocked below to help see the colors better.
Comment #142
joelpittetSuffle the fixed and unblocked
Comment #143
joelpittetComment #144
alexpottComment #145
alexpottComment #146
xjmComment #147
justAChris CreditAttribution: justAChris as a volunteer commentedComment #148
Wim LeersComment #149
davidhernandezComment #150
Wim LeersComment #151
realdream CreditAttribution: realdream commentedHow to display a image link without SafeMarkup::set ?
Comment #152
xjmComment #153
xjmComment #154
alexpott@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.
Comment #155
alexpottI 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.
Comment #156
Wim LeersComment #157
Wim LeersComment #158
Wim LeersAll issues unblocked :) Down to a single list of issues to tackle!
Comment #159
alexpottRetitling.
Comment #160
stefan.r CreditAttribution: stefan.r commentedComment #161
stefan.r CreditAttribution: stefan.r commentedComment #162
stefan.r CreditAttribution: stefan.r commentedComment #163
cilefen CreditAttribution: cilefen commented#2555825: Remove SafeMarkup::set call from TranslationManager::translate
Comment #164
alexpottComment #165
stefan.r CreditAttribution: stefan.r commentedComment #166
stefan.r CreditAttribution: stefan.r commentedComment #167
stefan.r CreditAttribution: stefan.r commentedComment #168
Wim LeersComment #169
alexpottUpdating issue credit to include everyone who has commented.
Comment #170
xjm@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.
Comment #171
davidhernandezI assume it is time for one last search of the code base?
Comment #172
mpdonadioHere is a usage report from PhpStorm against HEAD == f82e0a3.
Comment #173
heddnLooks 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.
Comment #174
cilefen CreditAttribution: cilefen commentedI think #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages() was just finished.
Comment #175
stefan.r CreditAttribution: stefan.r commentedComment #176
xjm4 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!
Comment #177
webchickAMAZING work, all!! :D
Comment #178
dsnopekWoohoo! Congrats to everyone who poured so much time and energy into this! The Drupal community owes you. :-)
Comment #179
star-szr:D
Comment #180
almaudoh CreditAttribution: almaudoh commentedAwesome news!!!
Comment #181
davidhernandezGood work, everyone. Thanks, in particular, stefan.r for working really hard the past couple weeks to finish this.
Comment #182
Wim Leers+1 — thanks, Stefan!
Comment #183
chrisfromredfinA lot of the movement on this really got underway at a D8 Accelerate funded code sprint!