Problem/Motivation

The requirements errors are escaped in installer. Check screenshots:

Before

After

Proposed resolution

Fix them using inline template SafeMarkup::set() function. Also cleaned up a bit the code to make it more readable.

Files: 
CommentFileSizeAuthor
#32 after_patch.JPG19.68 KBveerasekar89
#32 before_patch.JPG26.08 KBveerasekar89
#30 2346287-30.patch8.52 KBZekvyrin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,030 pass(es). View
#30 interdiff_25_30.txt1.6 KBZekvyrin
#26 screencapture-192-168-139-128-8082-core-install-php.jpg148.75 KBZekvyrin
#25 2346287.25.patch8.47 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,021 pass(es). View
#25 13-25-interdiff.txt2.67 KBalexpott
#24 Screenshot from 2014-10-04 14:32:59.png208.51 KBmarcus7777
#17 2346287_interdiff_10_13.txt2.22 KBbserem
#13 2346287-d8_install_double_escaped-13.patch8.35 KBbserem
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,037 pass(es). View
#10 2346287-d8_install_double_escaped_10.patch8.39 KBbserem
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,732 pass(es). View
#7 2346287-d8_install_double_escaped.patch8.82 KBbserem
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,733 pass(es). View
#3 installer-requirements-errors-double-escaped-2346287-3.patch3.31 KBrteijeiro
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,703 pass(es). View

Comments

rteijeiro’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, installer-requirements-errors-double-escaped.patch, failed testing.

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
3.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,703 pass(es). View

A few fixes and nitpicks discussed with @joelpittet. Thanks man!

rteijeiro’s picture

Issue tags: +Autoescape, +Amsterdam2014

Tagging...

joelpittet’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.install
@@ -385,7 +385,6 @@ function system_requirements($phase) {
-  $error = '';

@@ -397,26 +396,26 @@ function system_requirements($phase) {
+      $description = [];

The error presumably get's built up separately from the description for some reason. There is a code path where the error could be added to but not trigger the description to be displayed.

Maybe go with $error = [];
And build that up separately and prepend it on to your $description?

bserem’s picture

Above patch uses "SafeMarkupSet" which is to be avoided based on the discussion on [#2311123]

bserem’s picture

Status: Needs work » Needs review
FileSize
8.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,733 pass(es). View

Attached patch solves double escaping for:

  • php extensions (issue 2346509)
  • memory limit
  • writable filesystem

It doesn't use SafeMarkup but uses 'inline_template'.

Patch was created together with Zekvyrin (https://www.drupal.org/u/zekvyrin) at Drupalcon Amsterdam.

Zekvyrin’s picture

FileSize
2.11 MB
1.96 MB

Confirming that #7 patch solved the issues

kostask’s picture

Status: Needs review » Needs work

Variable names need to follow the coding standards. I would also use an array, $description[] with keys (ex $description['phase'], $description['memory'] ) rather than $desc_first and $desc_second.

bserem’s picture

Status: Needs work » Needs review
FileSize
8.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,732 pass(es). View

Good point, makes sense.

Attached patch is properly naming variables :)

Zekvyrin’s picture

FileSize
120.66 KB
120.21 KB

Before patch

After

rteijeiro’s picture

Status: Needs review » Needs work

Ok, great job! A few little nitpicks. Sorry :)

  1. +++ b/core/modules/system/system.install
    @@ -115,7 +115,18 @@ function system_requirements($phase) {
    +        'var' => $missing_extensions_list,
    

    Why don't use directly 'var' => drupal_render($item_list), here?

  2. +++ b/core/modules/system/system.install
    @@ -190,24 +201,37 @@ function system_requirements($phase) {
    +      $requirements['php_memory_limit']['description'] = $description;
    

    It seems that the indentation is wrong from here.

bserem’s picture

FileSize
8.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,037 pass(es). View

Good catch! Attaching new patch, don't really know how to make an interdiff :|

bserem’s picture

Status: Needs work » Needs review
rteijeiro’s picture

Ping me in IRC and I'll help you to make the interdiff. I'm at the Novotel lobby if you want to do it live!

Also assign the issue to you if you are going to fix it ;)

bserem’s picture

Assigned: Unassigned » bserem

I'll do my best to fix it. I think I'm on a good track!

update: I was at the hackerspace, I don't see you online atm so I'll ping tomorrow (unless I find the answer myself first)

bserem’s picture

FileSize
2.22 KB
marcus7777’s picture

Status: Needs review » Reviewed & tested by the community

Tested and working as expected.
I was trying to fix this issue. And it looks like you fixed it in a lot better way than i had. I think this is a good patch

JKingsnorth’s picture

ChristianAdamski’s picture

@akoe and me just encountred this one as well. Specifically the missing files/ directory one is also showing up in the status report page.

We approached this with SafeMarkup as well, before stumpling on this issue.

We tested and reviewed this patch and it works fine on install as well as status page.

It was hard to find this issue, though I have not better title proposal.

marcus7777’s picture

Can we add a test to test for double escaped html?

bserem’s picture

While I was working on the patch, Zekvyrin created an installation environment that would fail on all requirements (!) so as to test all situations.

I can't really tell if we can add a test

marcus7777’s picture

so we would need a add new environment to the test we can do that. I was thinking can we test for double escaped looking at the output for the tests?
We have it all over D8

marcus7777’s picture

like on error on the module page

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.67 KB
8.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,021 pass(es). View
+++ b/core/modules/system/system.install
@@ -115,7 +115,17 @@ function system_requirements($phase) {
-    $description .= drupal_render($item_list);
...
+        'var' => drupal_render($item_list),
...
+    $description = drupal_render($description);

@@ -190,24 +200,37 @@ function system_requirements($phase) {
+      $description = drupal_render($description);

@@ -234,7 +257,17 @@ function system_requirements($phase) {
+        $description = drupal_render($description);

@@ -400,22 +433,31 @@ function system_requirements($phase) {
+        $description = drupal_render($description);

There is no need to do the rendering here - we are returning one big render array. Which means we can do something like the patch attached.

Zekvyrin’s picture

I can confirm that alexpott's modification works perfectly to my enviroment:

install-php

joelpittet’s picture

Status: Needs review » Needs work

Some nitpicks a but some possible caveats still to take care of as well in this iteration:

Thanks for keeping on top of this issue!

  1. +++ b/core/modules/system/system.install
    @@ -112,11 +112,18 @@ function system_requirements($phase) {
    +    // We use twig inline_template to avoid double escaping.
    
    @@ -231,11 +250,17 @@ function system_requirements($phase) {
    +        // We use twig inline_template to avoid double escaping.
    

    We are avoiding "escaping", not "double escaping".

  2. +++ b/core/modules/system/system.install
    @@ -191,24 +198,36 @@ function system_requirements($phase) {
         $description = '';
    ...
    +      $description['phase'] = t('Consider increasing your PHP memory limit to %memory_minimum_limit to help prevent errors in the installation process.', array('%memory_minimum_limit' => DRUPAL_MINIMUM_PHP_MEMORY_LIMIT));
    

    Description is being treated as an array and probably should be initialized as one.

  3. +++ b/core/modules/system/system.install
    @@ -401,22 +426,30 @@ function system_requirements($phase) {
    -        $error .= t('The directory %directory does not exist.', array('%directory' => $directory)) . ' ';
    +        $error = t('The directory %directory does not exist.', array('%directory' => $directory));
    ...
    -        $error .= t('The directory %directory is not writable.', array('%directory' => $directory)) . ' ';
    

    Where is the $error initialized before for this concatenation, we may need more work here...

  4. +++ b/core/modules/system/system.install
    @@ -401,22 +426,30 @@ function system_requirements($phase) {
    +        $description = array(
    +         '#type' => 'inline_template',
    +         '#template' => '{{ error }} {{ description }}',
    +         '#context' => array(
    +           'error' => $error,
    +           'description' => $description,
    +         ),
    +        );
    

    just needs one more space of indent to make it 2 spaces.

joelpittet’s picture

Title: Installer requirements errors double escaped » Installer requirements errors escaped
Issue summary: View changes
joelpittet’s picture

Title: Installer requirements errors escaped » Installer requirements errors escaped HTML in variables.
Zekvyrin’s picture

Status: Needs work » Needs review
FileSize
1.6 KB
8.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,030 pass(es). View

Fixed 1,2 & 4 from comment #26.

About 3 ($error's concat), I don't think that this should change.
After $error is initialized, and might or might not be overridden once. It's no longer concatenated with description the way is was.

karthikkumarbodu’s picture

Good Work :)
I agree with your comments, description cannot be initialized as an array as it was treated as string in the previous statements.

veerasekar89’s picture

FileSize
26.08 KB
19.68 KB

Its working.

veerasekar89’s picture

Tested #30 working fine refer screenshot #32

veerasekar89’s picture

Status: Needs review » Reviewed & tested by the community

Tested #30 working fine refer screenshot #32

joelpittet’s picture

Thanks @Zekvyrin for the fixes and I agree with you on the #27.3 regarding the $error variable. It's weird because it looks like it could be collecting the $errors but the likelihood of that ever happening is 0 I believe because the $phase won't change in the loop.

RTBC++

lauriii’s picture

Assigned: bserem » Unassigned
jibran’s picture

Issue tags: +SafeMarkup
catch’s picture

Status: Reviewed & tested by the community » Needs review

Is it worth an actual template for this rather than an inline one?

bserem’s picture

@catch what do you mean by actual template? None of the patches introduced a template file.

catch’s picture

@bserem yes that's the question - would it be better to introduce an actual template file.

bserem’s picture

Understood now.

I can't really answer that. The issue seems resolved with the patches provided, and it is not a bad solution either I believe.
A template file would definitely postpone this more, not that we are in a hurry :)
Plus I don't get what the benefits of a template file would be (I really don't, I'm not that up to date with D8 yet).
Can you please share some intel on the subject?

Zekvyrin’s picture

@catch: I don't think we can answer that properly in this issue. Maybe a more 'global' answer is needed.
We thought this issue is in "small snippets of HTML" category, so we implemented 2nd option and not 1st from instructions:
https://www.drupal.org/node/2311123

As I saw, parent issue #2297711: Fix HTML escaping due to Twig autoescape is postponed for now.. so I guess this one has to wait.
I guess other issues like #2273925: Ensure #markup is XSS escaped in Renderer::doRender() should be resolved first and then we'll review these issues.

heddn’s picture

This one doesn't need to wait. The parent was postponed while all the children issues are gone through. Which is what we are doing here.

catch, fixes of #2297711: Fix HTML escaping due to Twig autoescape have tended to be in the "getting things fixed" category. Not introducing new functionality. To convert to a template, let's do that in a follow-up.

Zekvyrin’s picture

Status: Needs review » Reviewed & tested by the community

Ok then, I'm setting it to RTBC as it was before questioning about making it a template.

And we might revisit this issue as a follow up.

ifrik’s picture

This patch works for me. The html tags are displayed correctly.

However, I noticed several other issues: the links should be https instead http, we are not using the phrase "handbook" anymore but "online documentation", and the link should rather go to the installation guide and not to info about webhosting.

Since these issues are also relevant for some of the other messages in the system.install file: Should I wait until this patch is committed, and then make a new patch for all of them?

bserem’s picture

The issues you are mentioning are not exactly relevant to what we are trying to solve here.

Valid as they are, they should probably be solved on another issue.
Just my opinion :)

  • catch committed 7177b56 on 8.0.x
    Issue #2346287 by bserem, rteijeiro, Zekvyrin, alexpott: Fixed Installer...
catch’s picture

Status: Reviewed & tested by the community » Fixed

OK thanks for the answers.

@ifrik well spotted - please open a new issue for the things you found!

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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