Simpletest Module Double escaped HTML in hook_requirements

Tagging Amsterdam2014 + Novice because I think the issue has at least one novice task in it. Here's my impression:

Please decide for yourself if "Add steps to reproduce the issue" is needed. If you cannot see easily from the description/screenshot, this is neeeded.

If you can in fact reproduce it easily, you can probably find the affected strings in the source code. Creating the patch may not be that hard, because the suggested solution (inline_templates), as defined in the linked page, is well defined.

You may be able to find another person (novice contributor) to review/discuss if 'inline_templates' is actually the right solution to implement, after reading the linked page.

We are reproducing the issue dankh, henkit.

To reproduce the issue

• installed D8 from git
• enabled the display of Errors and warnings
• changed the owner of the folder ./sites/simpletest to root:root
• changed the permissions to 0700
• enabled the module
• the error message was shown
• the error message still contains code tag

Error message after installing the Testing module

We will work on a patch.

First we tried fixing the issue in the simpletest module itself, but after discussing the issue with Joel Pittet (@joelpittet) he pointed us out that the issue is more general and a better way to fix it is to modify the way strings are printed by drupal_check_module in install.inc.

In drupal_check_module requirements description strings get concatenated. Some of them are safe and some are not. When the strings get concatenated they are all marked as not safe. This is why tags in these strings get escaped and they are printed as is in the error message. This patch checks if the description string is safe and if it is it flags the whole concatenated string as safe.

This patch could also fix similar issues in other hook_requirements implementations.

People participating in this patch : @dankh, @henkit, @joelpittet .

1. +++ b/core/includes/install.inc
   @@ -7,6 +7,7 @@
   +use Drupal\Component\Utility\SafeMarkup;
   

   This is super minor but this should move down 3 for alphabetical.

2. +++ b/core/includes/install.inc
   @@ -1018,11 +1019,23 @@ function drupal_check_module($module) {
   +      $description_issafe = TRUE;
   ...
   +
   +        if (!SafeMarkup::isSafe($requirement['description'])) {
   

   This can be compressed a bit if you move the if condition directly around the SafeMarkup::set()

I start to work on this...

We applied the stuff joel suggested in #8.
the patch is a lot shorter now ;)

We attached the patch, an interdiff and an image showing all possible requierement error messages with HTML tags.

People participating in this patch : @dankh, @henkit, @vurt, @gngn, @joelpittet.

What do you suggest as an alternative?

Thanks I think this is a more global fix for all. Thanks everybody for going through it!

Hope you found it helpful to get a better understanding on how we are keeping the theme layer as safe as we can.

@aneek The only variable that is unknown in that equation is the 'description' key. We only mark the complete string as safe if that variable is also safe. I'm quite positive this is the best way to deal with this.

I'm marking this to "needs work" because to prevent people questioning we should add a comment above the condition to that effect. Could one of you assign yourself and add that comment?

Thanks Aneek, that does the same thing in a different way. Can you put a comment above the SafeMarkup::set() because that is the likely scrutinized but and we'd prefer this doesn't get reverted by a misunderstanding. Otherwise both are RTBC. Thanks for the iterations and finding the bug!

I think the comment gives a good enough indication. @vurt or @dankh if you have any additions to that comment to help clarify or clean up, please feel free to submit a patch or suggestion.

In the mean time I'm marking this as RTBC to get it in. Thanks again all!

I tested the new patch and it works as expected.

Any suggestions on the comment explaining the SafeMarkup::set @vurt?

The comments make sense for me (but I must add that I am no expert on this topic...).

There is a typo: "Prepair" -> "Prepare" in one comment.

Nice catch, we can remove that comment. We want it to be succinct and clear as to what we are doing.

Couple things I was thinking too:

1. +++ b/core/includes/install.inc
   @@ -1019,11 +1020,15 @@ function drupal_check_module($module) {
   +        // Prepair set of safe strings for rendering.
   

   We can likely remove this comment and it's spelled wrong as @vurt pointed out.

2. +++ b/core/includes/install.inc
   @@ -1019,11 +1020,15 @@ function drupal_check_module($module) {
   +        // Make sure to mark the message string as secure.
   

   This can be removed as well, I think it doesn't add much.

3. +++ b/core/includes/install.inc
   @@ -1019,11 +1020,15 @@ function drupal_check_module($module) {
   +        // The string was previously checked as safe or escaped properly with
   +        // SafeMarkup::escape() method.
   

   Maybe add " and the concatinated string is desginated safe by running through the t() function."

I tested again - output is still fine.

Perfect! Thanks for your patience. Let's get it in:)

Discussed with @chx on IRC since we are trying to limit the number of calls to SafeMarkup::set() but this is the installer and no one should be copying code from here.

Committed d96918c and pushed to 8.0.x. Thanks!