In time of enabling the "Testing" module in Drupal 8 if the requirements for this module is not matched then some error messages are shown. If there are any issues related to permission in the "simpletest" directory created by this module then the errors shown have double escaped string.
This one is one of many issues related to [meta] Fix double-escaping due to Twig autoescape.

HTML escape

Possible Solution:
Fixing this with New inline_template implementations.

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Add steps to reproduce the issue Novice/needed? Instructions
Files: 
CommentFileSizeAuthor
#25 simpletest_module-2319667-25.patch1.4 KBaneek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,027 pass(es). View
#25 interdiff-2319667-18-25.txt1.13 KBaneek
#18 simpletest_module-2319667-18.patch1.41 KBaneek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,988 pass(es). View
#18 interdiff-2319667-15-18.txt716 bytesaneek
#15 simpletest_module-2319667-15.patch1.24 KBaneek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,767 pass(es). View
#15 interdiff-2319667-10-15.txt1.05 KBaneek
#10 simpletest-working.png22.16 KBvurt
#10 interdiff-2319667-7-10.txt1.49 KBvurt
#10 simpletest_module-2319667-10.patch957 bytesvurt
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,765 pass(es). View
#7 simpletest_module-2319667-7.patch1.36 KBdankh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,764 pass(es). View
#6 2319667_simpletest_error_reproduce.png15.34 KBdankh
simpletest_error.png12.11 KBaneek

Comments

aneek’s picture

Component: simpletest.module » theme system
aneek’s picture

Issue summary: View changes
aneek’s picture

Issue summary: View changes
roderik’s picture

Issue summary: View changes
Issue tags: +Novice, +Amsterdam2014

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.

dankh’s picture

We are reproducing the issue dankh, henkit.

dankh’s picture

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.

dankh’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,764 pass(es). View

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 .

joelpittet’s picture

Status: Needs review » Needs work
  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()

vurt’s picture

Assigned: Unassigned » vurt

I start to work on this...

vurt’s picture

Status: Needs work » Needs review
FileSize
957 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,765 pass(es). View
1.49 KB
22.16 KB

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.

aneek’s picture

@vurt & @joelpittet, should we be really using SafeMarkup::set() ?

vurt’s picture

What do you suggest as an alternative?

joelpittet’s picture

Assigned: vurt » Unassigned
Status: Needs review » Needs work

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?

aneek’s picture

@joelpittet, okay got your point. Indeed we are also using SafeMarkup::isSafe so I think its quite a good approach.
Thanks @vurt for the patch. I can add a comment to the code block.

aneek’s picture

FileSize
1.05 KB
1.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,767 pass(es). View

Guys here is another approach.

Instead of using SafeMarkup::isSafe(), I've used SafeMarkup::escape(). This function will do the safe checks and also if the string is not safe then sanitize it also. Please see SafeMarkup::escape for more reference.

Attaching a patch for review.

aneek’s picture

Assigned: Unassigned » aneek
Status: Needs work » Needs review
joelpittet’s picture

Status: Needs review » Needs work

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!

aneek’s picture

FileSize
716 bytes
1.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,988 pass(es). View

Sure @joelpittet. I've added comment just before the SafeMarkup::set(). Please have a look at the diff and let me know if the comment is understandable or needs more details.

Thanks!

aneek’s picture

Status: Needs work » Needs review
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Bad html Formatting

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!

vurt’s picture

Status: Reviewed & tested by the community » Needs review

I tested the new patch and it works as expected.

joelpittet’s picture

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

vurt’s picture

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.

joelpittet’s picture

Status: Needs review » Needs work

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

aneek’s picture

FileSize
1.13 KB
1.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,027 pass(es). View

Thanks @joelpittet & @vurt for your valuable feedbacks. I've addressed those mentioned points in the patch. Please verify.

Thanks!

aneek’s picture

Status: Needs work » Needs review
vurt’s picture

I tested again - output is still fine.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed d96918c on 8.0.x
    Issue #2319667 by aneek, vurt, dankh: Fixed Simpletest Module Double...

Status: Fixed » Closed (fixed)

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

jibran’s picture

Issue tags: +SafeMarkup