Issue summary update:

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.

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

  1. Install Drupal 8
  2. Edit the view for admin/content and customize the label wrapper for one of the fields
  3. 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

Comments

YesCT’s picture

akalata’s picture

FileSize
1.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,983 pass(es). View

Fixes the issue following xjm's preferred method of using render arrays (from #2502089-21: Remove SafeMarkup::set() in template_preprocess_views_view_table()).

akalata’s picture

Status: Active » Needs review
dawehner’s picture

Can't we move that code already into the template itself?

akalata’s picture

Perhaps, 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?

subhojit777’s picture

Status: Needs review » Needs work

Patch looks good to me, but there should be this small change.

+++ b/core/modules/views/views.theme.inc
@@ -517,7 +517,12 @@ function template_preprocess_views_view_table(&$variables) {
+          $variables['header'][$field]['content'] = drupal_render($label_with_wrapper);

We don't need this. We can directly render from the array itself.

subhojit777’s picture

Status: Needs work » Needs review
FileSize
977 bytes
1010 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,216 pass(es). View
xjm’s picture

Priority: Normal » Major
Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs manual testing

Ah 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.

xjm’s picture

dawehner’s picture

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?

At 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?

justAChris’s picture

Issue summary: View changes
Issue tags: -Needs manual testing
FileSize
139.06 KB

Manual 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)

ManualTest 2506485

subhojit777’s picture

Status: Needs work » Postponed

In that case we should postpone this issue until #2363423: views-view-fields.html.twig gets escaped gets into core.

stefan.r’s picture

Status: Postponed » Active
justAChris’s picture

Status: Active » Needs review
FileSize
111.62 KB
187.36 KB
2.65 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,998 pass(es). View
3.64 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,589 pass(es). View

Added 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

The last submitted patch, 14: 2506485-14-test-only.patch, failed testing.

justAChris’s picture

Issue tags: -Needs tests
FileSize
199.96 KB
156.86 KB

Manually 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:

  • Performed Standard Drupal 8 Install
  • exporting the views.settings, adding the xss tag then re-importing.
  • Updated Title field in /admin/content view and selected the new xss tag as the label wrapper

I was worried about this method escaping part of the tag in the views settings so instead the tags added in these screenshots:

  • Added xss tag to views.settings.yml before installation
  • Performed Standard Drupal 8 Install
  • Updated Title field in /admin/content view and selected the new xss tag as the label wrapper

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.

XSS Before Patch
XSS After Patch

dawehner’s picture

  1. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_field_header.yml
    @@ -0,0 +1,49 @@
    +            element_label_type: script>alert("XSS")</script
    

    Historically we did not ensured about whether the user with "administer views" might do something wrong, but you never know

  2. +++ b/core/modules/views/views.theme.inc
    @@ -521,7 +521,11 @@ function template_preprocess_views_view_table(&$variables) {
    +          $variables['header'][$field]['content'] = array(
    +            '#type' => 'html_tag',
    +            '#tag' => $element_label_type,
    +            '#value' => $variables['header'][$field]['content'],
    +          );
    

    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.

akalata’s picture

Re #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.

The last submitted patch, 14: 2506485-14-test-only.patch, failed testing.

justAChris’s picture

Specifically, 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?

justAChris’s picture

Test only patch should not fail since the issue was resolved elsewhere. CI liked it well enough, giving PIFR another chance.

The last submitted patch, 14: 2506485-14-test-only.patch, failed testing.

akalata’s picture

Issue summary: View changes

@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..

akalata’s picture

FileSize
2.65 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,495 pass(es). View

Re-uploading @justachris's test-only patch for clarity. Should be RTBC'able if testbot is happy.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +rc eligible

I think the problem is fixed but maybe the test coverage should still get committed. Tagging as rc eligible.

alexpott’s picture

Title: Header label + customized label wrapper of a Views table display is double escaped » Test that Header label + customized label wrapper of a Views table display is not double escaped
Category: Bug report » Task
Priority: Major » Normal
Status: Reviewed & tested by the community » Needs work

Okay 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.

justAChris’s picture

Status: Needs work » Needs review
FileSize
3.16 KB
2 KB

Oddly, test was still coming back ok. The view is old so re-exported as suggested, mainly cacheability configuration added.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @justAChris and @alexpott. The view was re-exported and should address #29

Setting back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: 2506485-30-test-only.patch, failed testing.

justAChris’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC per #31. Test fail in #32 seems to be random, so retested and it came back green.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Test 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!

  • alexpott committed 62dc3b3 on 8.1.x
    Issue #2506485 by justAChris, akalata, subhojit777: Test that Header...

  • alexpott committed 76e6a03 on
    Issue #2506485 by justAChris, akalata, subhojit777: Test that Header...

Status: Fixed » Closed (fixed)

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