Remove SafeMarkup::set in template_preprocess_color_scheme_form()

I suggested this approach at the sprint. I don't think it's any less secure than the previous code but would probably be nice to hear from someone on the security team on this one.

+++ b/core/modules/color/color.module
@@ -284,7 +284,11 @@ function template_preprocess_color_scheme_form(&$variables) {
+  $variables['html_preview'] = array (
+    '#type' => 'inline_template',
+    '#template' => file_get_contents($preview_html_path),
+  );

Minor (coding standards): Either remove the space between array and (, or convert to short array syntax (square brackets). https://www.drupal.org/coding-standards#array

Try this on for size.

@cdulude good question. SafeMarkup::checkAdminXss() will run Xss::filterAdmin() (which I guess we could just run directly, maybe that's better?) And that will filter out the tags and attributes that may be harmful in an Xss attack.

Though on the other hand, in this case we are loading a file in the theme directory and it's very unlikely that there will be an attack from that kind of file.

We can go either way but this will save us from having to justify it's usage and the very slight chance we are wrong and someone finds out how to exploit this.

@cdulude What do you think?

#7 it's not basic knowledge, there is actually another issue where the other maintainers and committers are debating a very similar problem with valid points on both sides. Just @Cottser and I are on the same page for most things so it makes it easier to get things done:P

But to be clear one thing we are trying to avoid is passing variables into templates. This problem is very similar to variables passed to the t() function in D7.

So if you pass a variable to SafeMarkup::set($danger) or t($danger) or $variables['html_preview'] = ['#type' => 'inline_template', '#template' => $danger];. You are essentially doing the exact same thing, passing a safe variable as markup and it won't be filtered for Xss attacks.

The t() function docs say it in D7:

You should never use t() to translate variables, such as calling t($text); , unless the text that the variable holds has been passed through t() elsewhere (e.g., $text is one of several translated literal strings in an array). It is especially important never to call...

https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7

Manually tested #4, HTML is identical.

Also want to note that the use of SafeMarkup::checkAdminXss is preferred over an inline template based on joel's explanation in #8.

Updating my review notes based on YesCT's input:

Patch replaces one instance of SafeMarkup::set with SafeMarkup::checkAdminXss

Do we need a test for checking preview.html specifically (Remaining Task #2), or will that now be covered under the tests for checkAdminXss?

It looks like this is a nice security improvement. Good stuff!

I'm re-rolling this for the more direct method call

progress, but the new test isn't passing - not sure I'm on the right path (and path needs to be replaced by route name) and need to put e.g. SCRIPT tags into the new html file to make sure they are stripped.

I've consolidated pwolanin's new test theme with the existing color_test_theme, and updated the test to confirm that preview.html is rendered, and that offending <script> tags are removed.

pwolanin provided some feedback IRL at the NJ shore sprint.

• Switched the test for the settings form page to use the menu route, rather than passing a string for the URL.
• Verified that the change in color.module works by making sure the test fails when that change is not included.

Needs a tweak to the test. The "test fails" patch should use color.module from HEAD as is is with SafeMarkup::set() and check for the literal <script> tag getting passed through to the user.

In both cases we need an added assertion in the test that a safe tag like H3 or EM get passed through as HTML in HEAD and with the change to Xss::filterAdmin()

I've updated the failing example so the new test is against HEAD, and added a new test that makes sure Xss:filterAdmin() does not get double-escaped (though shouldn't that test be will filterAdmin(), rather than our specific implementation of it?).

I think this new test method needs a better name

+  function testTwigCacheOverride()

I think that was copied from another test (maybe my fault)

+++ b/core/modules/color/src/Tests/ColorSafePreviewTest.php
@@ -0,0 +1,62 @@
+
+

Minor double line break can likely be fixed on commit.

This looks good to me even has tests which may be overkill but sure. Reviewed the tests, bigUser was a funny name, I like it:)

bigUser isn't new - actually coming from existing Color module tests used as an example :)

Still funny to me:D

Can we use SafeMarkup::checkAdminXss() since we're trying to remove SafeMarkup side effects from the XSS class - see #2506195: Remove SafeMarkup::set() from Xss::filter()

Sorry for all the confusion about which methods to use - we're all trying to get this sorted asap.

Chris Luckhardt, Tony Nash and I are working on this at the #DrupalNorth sprint.

Crud ... I need to reapply an earlier patch and re-roll. Learning git

flagging as NR for testbot

@Chris and Tony ... cross your fingers!!! Thank you Akalata!
New Patch made after applying #24.
Can I set this to Needs review or does someone else do that?

@GreenSkunk thanks, nice meeting you at Drupal North! You should set the status to needs review when uploading a new patch.

Can we use SafeMarkup::checkAdminXss() since we're trying to remove SafeMarkup side effects from the XSS class - see #2506195: Remove SafeMarkup::set() from XSS::filter()

This makes sense here. It is great that we have added test coverage here!

Wanted to actually RTBC here.

Sorry for jumping in

Thanks everyone! I agree that doing an admin XSS filter is the right fix here.

1. +++ b/core/modules/color/color.module
   @@ -284,7 +284,7 @@ function template_preprocess_color_scheme_form(&$variables) {
   -  $variables['html_preview'] = SafeMarkup::set(file_get_contents($preview_html_path));
   

   Discussed with @alexpott. This should work equivalently with a #markup render array element actually. I.e.:

   $variables['html_preview']['#markup']
   

   That's also preferable because we are trying to reduce the use of SafeMarkup during the render/theme process in general (to reduce early rendering and escaping, reduce memory use, etc. etc. etc.).

2. +++ b/core/modules/color/src/Tests/ColorSafePreviewTest.php
   @@ -0,0 +1,62 @@
   +    // Install the test theme
   

   Minor: missing period at the end of the sentence.

3. +++ b/core/modules/color/src/Tests/ColorSafePreviewTest.php
   @@ -0,0 +1,62 @@
   +    $url_object = Url::fromRoute('system.theme_settings_theme', ['theme' => 'color_test_theme']);
   +
   +    $this->drupalLogin($this->bigUser);
   +    $this->drupalGet($url_object);
   +    $this->assertText('TEST COLOR PREVIEW');
   +
   +    $this->assertNoRaw('<script>alert("security filter test");</script>');
   +    $this->assertRaw('<h2>TEST COLOR PREVIEW</h2>');
   

   Nice complete test coverage!

   We should specifically reference the file where the preview is stored to make the source for these assertions clear. I'd suggest an inline comment. @alexpott suggested writing the file directly to the files directory during the test so that the markup is in the same place in the code. Either way. :)

Changes look good and I think all feedback from #40 has been addressed.

Alright, this looks great! Thanks @lauriii and @Cottser.

This issue is a required part of a critical issue and so is acceptable during the beta per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x. Thanks!