Follow-up to #1825952: Turn on twig autoescape by default
Non-technical explanation
After #2264041: Add a test to ensure title callbacks are not vulnerable to XSS have proven that even battle hardened core developers can't write XSS free code we have introduced #1825952: Turn on twig autoescape by default to fix a torrent of security holes already present in core known and unknown and to avoid the most frequent kind of sechole(Security Hole) in the history of Drupal contrib. However, this has broken some places that were already securely written, resulting in broken layout and HTML tags shown to users. We need to find those places and update them to be compatible with the new method.
Problem/Motivation
#1825952: Turn on twig autoescape by default fixed escaping globally and caused HTML escaping on places where we explicitly set HTML in a variable. This was expected. The patch was an absolute must and avoiding / fixing all paths would've taken a lot of time and made an already big patch impossible to review and commit.
Instead we have opted to go ahead with this and let people find the broken pages. If people would've cared to review patches this could've been avoided but we know this is a no-go so instead we forced it.
@ti2m found escaped strings on several paths:
Another (and the last) update on the crawled urls. I enabled all modules on a fresh install and crawled the site as user 1. I only found two more urls with escaped strings (first two in the list below). But the general problem is, that e.g. node edit forms aren't covered at all as no node exists on a vanilla install. I could post a file with all covered urls, roughly 300, if anyone is interested.
The total list of urls with escaped HTML strings that I found:
- /admin/config/regional/translate/settings
- /admin/config/development/logging
- /admin/modules/uninstall
- /admin/config/regional/date-time/formats/add
- /admin/config/regional/date-time/formats/manage/long
- /admin/config/people/accounts/fields/user.user.user_picture
- /admin/config/content/formats/manage/basic_html
- /admin/structure/block
- /admin/structure/block/list/seven
- /admin/structure/views/nojs/rearrange-filter/frontpage/page_1
- /admin/structure/menu/manage/footer
- /filter/tips
- All 'manage fields' pages (ref: Screenshot)
- /admin/config/services/rest
Proposed resolution
If at all possible move all markup into a Twig template. If not then please read https://www.drupal.org/node/2311123 for alternative solutions. See aneek's patch at #2305831-35: Double escaping on /admin/modules/uninstall for an example.
Remaining tasks
Change the offending pieces to properly use Twig templates or use inline templates as described in https://www.drupal.org/node/2311123 .
Comment | File | Size | Author |
---|---|---|---|
#32 | commit_git.png | 31.47 KB | aneek |
#31 | field_form.png | 44.19 KB | andypost |
#28 | Screen Shot 2014-08-14 at 11.31.32.png | 27.44 KB | pbz1912 |
Comments
Comment #1
xjmComment #2
jbrown CreditAttribution: jbrown commentedComment #3
jbrown CreditAttribution: jbrown commentedComment #4
jhodgdonnode/1/translations - added issue today, just made it a child of this one: #2305799: HTML tags (raw) shown in Status column on Translations page
I noticed this on some Views UI pages today too, but hadn't filed issues, will hold off.
Comment #5
StuartJNCC CreditAttribution: StuartJNCC commentedand here is another one: User info on "Search results" page - /search/node?keys=
Comment #6
webchickThe issue summary here could do with some instructions for people who want to help with this initiative. I'm seeing people attempt to use SafeMarkup::set() as HEAD does and then get slapped down for it.
Comment #7
jhodgdonYes. I'm also unclear on whether this issue is a "fix it all up in one fell swoop" or if we're supposed to file child issues.
Comment #8
benjy CreditAttribution: benjy commentedThe permissions page also has double escaped strings.
Comment #9
andyceo CreditAttribution: andyceo commentedOne more: on the page with adding new field, autogenerated field name is double-escaped.
Comment #10
andypostFiled patch for #9 #2309929: HTML double-escaping in field forms
Comment #11
jhodgdonSo... These problems are all over the admin UI now. Can't we fix the root problem that caused them to appear? Or can someone ***** PLEASE ***** update the issue summary so we know what the real cause and correct fix is?
Comment #12
mradcliffeI guess if you sneak it in as usage of StringTranslationTrait::t() it gets passed review?
$this->t() is used over 2000 times in core, which is pretty much SafeMarkup::set().
If the string needs to be translated anyway, then is it really so bad to use SafeMarkup::set() as that's what translation is going to do?
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedI've just noticed #2312361: Combining drupal_render() in theme function causes double escaping
Comment #14
vijaycs85Comment #15
vijaycs85Comment #16
vijaycs85Comment #17
ianthomas_ukI can't help with how to fix it, but I can help people understand why we shouldn't just revert the patch that caused a very visible problem. I've added a non-technical explanation to the issue summary.
Comment #18
chx CreditAttribution: chx commented#2289999: Add an easy way to create HTML on the fly without having to create a theme function / template
Comment #19
chx CreditAttribution: chx commentedComment #20
LinL CreditAttribution: LinL commentedAdding #2310241: Uninstall page showing raw HTML output
Comment #21
LinL CreditAttribution: LinL commentedWrong way round... Made #2310241: Uninstall page showing raw HTML output child of this.
Comment #22
larowlanAdded #2314513: Add test coverage for DiffFormatter not double escaping to children list
Comment #23
chx CreditAttribution: chx commentedGo!
Comment #24
chx CreditAttribution: chx commentedComment #25
chx CreditAttribution: chx commentedComment #26
rpayanmAnother url with double escaped strings that I found was in the install process #2317281: Double escaping of install errors.
Comment #27
Sutharsan CreditAttribution: Sutharsan commentedComment #28
pbz1912 CreditAttribution: pbz1912 commented#2321229: Over escaping html on field help text form element description
Comment #29
aneek CreditAttribution: aneek commentedAnother child issue #2319667: Simpletest Module Double escaped HTML in hook_requirements
Comment #30
aneek CreditAttribution: aneek commentedJust curious, has something changed in last commits in Drupal 8.0.x? All the issues with Twig Autoescapes are seems to be fixed.
I checked with #2309215: HTML double-escaping in revision messages, #2309929: HTML double-escaping in field forms & #2319667: Simpletest Module Double escaped HTML in hook_requirements with the latest code checked out and all seems to be fixed automatically.
Can any one confirm on these?
Thanks!
Comment #31
andypostConfirm that, that's very strange!!!
Comment #32
aneek CreditAttribution: aneek commented@andypost, so this means I'm not wrong. Something do happened in the last commits. Maybe the recent one. Strange!
Get the core committers, ha ha... :-)
Comment #33
andypostProbably #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render
Comment #34
aneek CreditAttribution: aneek commented@andypost, as you have seen also, a fresh install brakes all of them.
Comment #35
Rade CreditAttribution: Rade commentedAnother related issue: #2346611: Wrong characters on field edit page
Comment #36
mgiffordComment #37
hass CreditAttribution: hass commentedTried SafeMarkup::set(), but it's not working with conditionals.
Comment #38
star-szrI just want to mention here that it looks like this ongoing issue will fix a lot of these cases:
#2324371: Fix common HTML escaped render #key values due to Twig autoescape
So please test with the patch there before creating new child issues. Thanks!
Comment #39
znerol CreditAttribution: znerol commentedGuess this is related: #2322439: Titles in a user's activity tab displays as just text and not a link., adding.
Comment #40
star-szrRemoving related issues that are actually child issues. I added the missing relationship to a couple of them.
Comment #41
webchickDo we need a fresh run through all of these to verify them now that #2324371: Fix common HTML escaped render #key values due to Twig autoescape was committed?
Comment #42
chx CreditAttribution: chx commentedYes.
Comment #43
webchickOkie doke.
Comment #44
joelpittetComment #45
xjmComment #46
joelpittetThis is should be active now that #2324371: Fix common HTML escaped render #key values due to Twig autoescape is in.
Comment #47
joelpittetTo clarify, I un-postponed so people don't think that this is a postponed issue that needs fixing elsewhere. This issue "Needs issue summary update" as the active task and it's meta tasks need to be verified and closed if they are no longer valid.
Comment #48
webchickI actually wonder if we should just close this out and open up individual issues for things we find?
Comment #49
Zekvyrin CreditAttribution: Zekvyrin commented@webchick: I worked in a few child issues and, for me, it was very helpful that we had this one as a parent issue, as I could easily access proposed solutions and check other similar issues' solutions as a reference.
Comment #50
webchickFair enough. The first step is go through all of the issues mentioned in the issue summary and sidebar and see if they are still issues anymore.
Comment #51
joelpittetI've went through the child issues and they are all still valid in the context of not being fixed by that render #key values commit.
I was pouring over whether they are the 'correct' solution or not. We are trying to avoid SafeMarkup::set(), some people just side step the issue by using #markup which in some cases is the same but different way of doing SafeMarkup::set().
'#inline_template' also should be discouraged, but it's many times the only viable solution when you are dealing with table cell content with new tags. (#type table we are manipulating the data to fit the table markup, instead of marking up the data in X list format in a specific template. ) This pattern seems to be very consistent, which is good, but also a PITA for both manipulating markup and SafeMarkup::set.
2¢
Comment #52
penyaskitoFound #2406903: HTML double-escaping in views debug messages.
Comment #53
David_Rothstein CreditAttribution: David_Rothstein commentedAdded #2409477: Various ! placeholders throughout core are now showing escaped HTML (especially <a href="!..."> examples) (caused by the recent changed introduced by #2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument).
Comment #54
Cheet CreditAttribution: Cheet commentedSo looking into this issue I noticed that simply changing the "!url" to "@url" totally mangles any GET request that you can make.
For example:
"admin/config/media/image-styles/add?a=b&b=c"
becomes:
"admin/config/media/image-styles/add%3Fa%3Db%26b%3Dc"
It looks like the string is marked as "unsafe" and escaped. Perhaps there should be a way to mark variables as urls or some special use case for urls specifically. However for now, setting the escape character to "@" in a t() string is NOT the way to go.
Comment #55
Adam Clarey CreditAttribution: Adam Clarey commentedI'm not sure how this is supposed to work as templates are nested, some templates are going to be outputting the rendered markup from child templates.
I've created a custom theme and every place I want to output markup from a variable it is always being escaped.
For a specific example, I have a view and without any custom templates it renders fine. However when I copy views-view-fields.html.twig to my custom theme without changing it at all, the view content is then displayed as entirely escaped markup.
From this thread I'm guessing its because field.content is the pre-rendered markup for each field.
Can someone point out how you are supposed to output markup in twig templates please? Maybe using my specific example as a use-case.
Thanks
Comment #56
star-szr@Adam Clarey the nesting is not the issue, the issue is that by default views_view_fields is rendered as a theme function, not a Twig template:
#2348747: Convert theme_views_view_fields() to 100% Twig: remove the theme function
#2363423: views-view-fields.html.twig gets escaped
Comment #57
Adam Clarey CreditAttribution: Adam Clarey commentedSo am I to assume this is a bug that is a bug that is being fixed? The only thing I can gleam from the long issue thread is that if I want to use a template in my own theme, I need to add '|raw' to the end of the field so it will output the raw markup instead of the escaped markup.
Obviously this isn't ideal
Comment #58
star-szr@Adam Clarey if you can please test the latest patch from #2363423: views-view-fields.html.twig gets escaped and comment on that issue with your results that would be super helpful!
You can ignore the first issue, I probably shouldn't have included that link (or not first anyway), that's just how we discovered the issue.
Comment #59
Adam Clarey CreditAttribution: Adam Clarey commentedThe patch didn't help i'm afraid.
Putting the template views-view-fields.html.twig in a custom theme only outputs the sanitized markup.
To output as actual markup the twig variables require '|raw' adding to theme, eg field.content|raw
Comment #60
star-szrPlease, #2363423: views-view-fields.html.twig gets escaped is the issue for these comments. If you can post there the steps you used to test that would be super helpful! Thanks.
Comment #61
cilefen CreditAttribution: cilefen commented#2511780: Node revision log HTML is escaped
Comment #62
joelpittetComment #63
cilefen CreditAttribution: cilefen commented#2610236: Views - "No Results Behavior" for individual fields escapes HTML
Comment #64
cilefen CreditAttribution: cilefen commented#2637550: Quick edit adds markup to node titles which is encoded and displayed in modal titles
Comment #65
DYdave CreditAttribution: DYdave at DAVYIN Internet Solutions / 戴文信息科技有限公司 commentedGot the same problem as mentioned in #59.
Related with #2610236: Views - "No Results Behavior" for individual fields escapes HTML.
Running on 8.0.6.
When using Views Custom text field, whatever you put in the text field, whether it resolves to empty or not, the returned value is not going to be empty, unless the text field is left completely empty (kind of useless and defeats the purpose of the field).
See the very good inline comment in Custom.php line 67:
So whether your pattern in the text field evaluates to empty or not, it doesn't matter, returned value is not going to be empty.
When setting up a No results text, the value then ends up in the wrong case, in my opinion, again, with a great inline comment in FieldPluginBase.php line 1308:
ok, so is it going to be the recommended practice for such field customization or is this perhaps a buggy limit case?
I would be very happy to provide whatever additional information or screenshots, but the test case is exactly the same as #2610236-8: Views - "No Results Behavior" for individual fields escapes HTML with the Custom text field.
Of course, overriding the views field theme with
{{ output|raw -}}
worked fine, as mentioned in #59 and at #2584655-13: Views field "Global > Custom Text" incorrectly escapes HTML.Let me know if there's anything I can do to help on this.
Cheers!
Comment #67
DamienMcKennaThis is causing some small problems for Metatag: #2649592: Write tests to cover apostrophe handling in meta tags
Comment #70
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedAdded #2850496: Double escaping in Rest UI
Comment #72
amaisano CreditAttribution: amaisano commentedRevision log messages when using Views (base table content_revisions) field has escaped HTML shown to user.