The double-escaping in this issue was fixed in #2502089: Remove SafeMarkup::set() in template_preprocess_views_view_table(). However, that patch did not add a test for double-escaping this element. This issue has a test-only patch to address this.
Issue summary update:
Problem/Motivation
While reviewing #2502089: Remove SafeMarkup::set() in template_preprocess_views_view_table() it was discovered that customizing a label wrapper in a view that is being displayed as a table results in the double-escaping of the label in the table header.
Steps to reproduce
- Install Drupal 8
- Edit the view for admin/content and customize the label wrapper for one of the fields
- Visit admin/content and observe that the label is not being correctly rendered
(YesCT's screenshots shamelessly stolen from the previous issue)
Remaining tasks
Fix it.
User interface changes
n/a
API changes
n/a
Comment | File | Size | Author |
---|---|---|---|
#30 | 2506485-interdiff-14-30.txt | 2 KB | justAChris |
#30 | 2506485-30-test-only.patch | 3.16 KB | justAChris |
#16 | 2506485_XSS_AfterPatch.jpg | 156.86 KB | justAChris |
#16 | 2506485_XSS_BeforePatch.jpg | 199.96 KB | justAChris |
#11 | ManualTest-2506485-11.png | 139.06 KB | justAChris |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedComment #2
akalata CreditAttribution: akalata as a volunteer commentedFixes the issue following xjm's preferred method of using render arrays (from #2502089-21: Remove SafeMarkup::set() in template_preprocess_views_view_table()).
Comment #3
akalata CreditAttribution: akalata as a volunteer commentedComment #4
dawehnerCan't we move that code already into the template itself?
Comment #5
akalata CreditAttribution: akalata as a volunteer commentedPerhaps, but it's pretty complex. We were discussing it this weekend at the NJ sprint (see #2502089-17: Remove SafeMarkup::set() in template_preprocess_views_view_table()).
Even looking at just this specific change in the table header as a candidate to move, the template is only has
column.content
as an option. The field label wrapper defined via the UI is being added as part of this column.content to one specific column, not every single one. I think it would add unnecessary complexity to the template to deal with the off chance that Views is defining a custom wrapper element for a particular label.I suppose we could add a theme template for just content.column when it appears in the table header, and then another one to address the field content wrapper customizations for table rows -- but I'm think that's out-of-scope to this issue?
Comment #6
subhojit777Patch looks good to me, but there should be this small change.
We don't need this. We can directly render from the array itself.
Comment #7
subhojit777Comment #8
xjmAh thanks @subhojit777! I was hoping that would be possible.
Can someone manually test with the same steps to ensure this patch works? Since there was a double-escaping bug, we also need automated tests.
Finally, this is a better use of the API, but I'm still concerned about the XSS filtering expectations. How do we know for sure the tag type is coming from a trusted source? This is also relevant for #2502089: Remove SafeMarkup::set() in template_preprocess_views_view_table() and #2363423: views-view-fields.html.twig gets escaped. An automated test would help prove (or disprove) the sanitization.
It's possible this should also be postponed on #2363423: views-view-fields.html.twig gets escaped, but setting NW for now since it describes a specific bug and the tests would be useful.
Comment #9
xjmComment #10
dawehnerAt the moment we simply return what is configured in the config entity. I guess if someone can change that, you have way too much power already?
Comment #11
justAChris CreditAttribution: justAChris as a volunteer commentedManual test looks good to me.
Was going to attempt an automated test, but after looking at the test just added by Cottser on the related issue #2363423: views-view-fields.html.twig gets escaped, I think this test will be an extension of the test developed there (part of ViewsEscapingTest)
Comment #12
subhojit777In that case we should postpone this issue until #2363423: views-view-fields.html.twig gets escaped gets into core.
Comment #13
stefan.r CreditAttribution: stefan.r commentedComment #14
justAChris CreditAttribution: justAChris as a volunteer commentedAdded automated test for the escaping of the wrapper tags in the table header label. Adds a test view configuration to support the html element selection. Included a test for the escaping of XSS in this element tag since that was a concern in #8. Interdiff is the test only patch.
Manually tested the XSS attempt in the table header label, screenshots for before patch and after patch have been uploaded. Entered tag value in views configurations was:
script>alert("XSS")</script
since the leading and trailing tag brackets are added automatically.Re-uploading manual test screenshots in later comment
Comment #16
justAChris CreditAttribution: justAChris as a volunteer commentedManually tested the XSS attempt in the table header label, screenshots for before patch and after patch have been uploaded.
In comment #14, the method used to add the xss tag was:
I was worried about this method escaping part of the tag in the views settings so instead the tags added in these screenshots:
Entered tag value in views.settings.yml was:
script>alert("XSS")</script: XSS
Did not include leading and trailing angle brackets since they should be added automatically.
Comment #17
dawehnerHistorically we did not ensured about whether the user with "administer views" might do something wrong, but you never know
Is there no way to move that into the twig template itself? Its kinda sad to have to add a render array for that, if we can avoid it.
Comment #18
akalata CreditAttribution: akalata as a volunteer commentedRe #17.2, it looks like this has already happened in head (as part of SafeMarkup fixes). Retesting @justachris's test-only patch from #14, since that may be all we need now.
Comment #21
justAChris CreditAttribution: justAChris as a volunteer commentedSpecifically, I think it was resolved in this issue: #2502089: Remove SafeMarkup::set() in template_preprocess_views_view_table(). Wonder how many other double escaping issues have been resolved now?
Comment #22
justAChris CreditAttribution: justAChris as a volunteer commentedTest only patch should not fail since the issue was resolved elsewhere. CI liked it well enough, giving PIFR another chance.
Comment #25
akalata CreditAttribution: akalata as a volunteer commented@justAChris thanks for tracking down that issue!
The test failed the latest time (and possibly earlier ones),
Drupal\update\Tests\UpdateTestBase->standardTests()
, isn't one touched by this patch, so no idea what's going on there..Comment #27
akalata CreditAttribution: akalata as a volunteer commentedRe-uploading @justachris's test-only patch for clarity. Should be RTBC'able if testbot is happy.
Comment #28
joelpittetI think the problem is fixed but maybe the test coverage should still get committed. Tagging as rc eligible.
Comment #29
alexpottOkay so no test coverage was added by #2502089: Remove SafeMarkup::set() in template_preprocess_views_view_table() so it seems sensible to add test coverage. However it is certain that the test view will need re-exporting since it is over 2 months old.
Comment #30
justAChris CreditAttribution: justAChris as a volunteer commentedOddly, test was still coming back ok. The view is old so re-exported as suggested, mainly cacheability configuration added.
Comment #31
joelpittetThanks @justAChris and @alexpott. The view was re-exported and should address #29
Setting back to RTBC.
Comment #33
justAChris CreditAttribution: justAChris as a volunteer commentedBack to RTBC per #31. Test fail in #32 seems to be random, so retested and it came back green.
Comment #34
alexpottTest only patch - can go into the next patch release. Working from @xjm'a post release triage document, committed 76e6a03 and pushed to 8.0.x and 8.1.x. Thanks!