Test that Header label + customized label wrapper of a Views table display is not double escaped

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

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.

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.

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?

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)

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

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

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.

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.

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?

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

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

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.

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

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

Setting back to RTBC.

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

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!