When I install for first time Drupal 8 alpha 14 and I have a permissions problem, the HTML in the error text is in plain text.

See the image for best comprehension.

Files: 
CommentFileSizeAuthor
#39 2317281_matt2000.patch711 bytesmatt2000
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,077 pass(es). View
#33 escaped-html.png47.71 KByoroy
#32 fix_html_tag_interdiff.2341607.32.txt2.15 KBxlin
#32 fix_html_tag_2341607.32.patch2.15 KBxlin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,618 pass(es). View
#32 file_system_html_tag_fixed.png259.74 KBxlin
#26 fix-install-double-escape-2317281-26.patch11.17 KBherom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,154 pass(es). View
#26 interdiff-2317281-20-26.txt1.35 KBherom
#20 fix-install-double-escape-2317281-20.patch11.33 KBherom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,114 pass(es). View
#20 php-extensions-fixed.png74.59 KBherom
#20 memory-limit-fixed.png49.54 KBherom
#20 file-system-fixed.png34.6 KBherom
#20 php-extensions.png39.26 KBherom
#20 memory-limit.png47.82 KBherom
#15 html_database_config-2317281-15.patch1.18 KBherom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,119 pass(es). View
#17 2317281-17-before.png75.23 KBCottser
#17 2317281-17-after.png66.24 KBCottser

Comments

rpayanm’s picture

Issue summary: View changes
FileSize
73.99 KB
longwave’s picture

The uninstall page is being dealt with at #2305831: Double escaping on /admin/modules/uninstall

Michelle’s picture

Version: 8.0.0-alpha14 » 8.0.x-dev
Parent issue: » #2297711: Fix HTML escaping due to Twig autoescape

I added in the parent issue for this.

ivanjaros’s picture

FileSize
80.6 KB

Also error messages during installation are double escaped.
twig auto escape

pbz1912’s picture

CharuAg’s picture

Status: Active » Needs review
FileSize
1013 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,407 pass(es). View
69.47 KB

Fixed double escaped error messages during installation. Addressed comment #4. Screenshot, after fix.

install error

herom’s picture

Status: Needs review » Needs work

Please take a look at #2311123: New inline_template render element for HTML code in PHP. You should try using one of the first three options. Since String::format() is calling SafeMarkup::set() directly, it's considered as option four which is seriously discouraged.

sabert00th3693’s picture

Try changing the file permission of sites/default/files to chmod -R 777 sites/default/files.
error.png

Michelle’s picture

@sabert00th3693 The point isn't to avoid the error; the point is to correct the double escaped HTML when the error comes up.

CharuAg’s picture

Status: Needs work » Needs review
FileSize
944 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,657 pass(es). View

Addressed comment #7 and used inline template.

mdrummond’s picture

Could we try checking if this works without the drupal_render?

It is possible that might be necessary, but ideally we want to leave that message as a render element as long as possible. Once the message is rendered as a string, it can't be altered by anything else. If that is necessary, ok, but leaving as render element is preferred.

CharuAg’s picture

@mdrummond I tried to fix it without using drupal_render, however it didn't work.

mdrummond’s picture

Okay, sounds good then. Probably want some manual testing before we RTBC. May also want a test to check output.

CharuAg’s picture

@mdrummond I am a new contributor, would you please let me know what needs to be done next at my end. Thanks!

herom’s picture

FileSize
1.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,119 pass(es). View

Looking into this, I think there is a better fix. This string is meant to be translatable, but lost its translatability way back in #349508: Require UTF8 database encoding, and it seems to be by accident.
So, we make it translatable, and the double-escape issue is fixed at the same time.

mdrummond’s picture

I'm trying to remember how t() works with auto-escaping and failing. I'm concerned that's similar to a SafeMarkup approach, but I can't remember.

If anybody remembers, feel free to pipe in.

Cottser’s picture

Title: HTML in plain text on install » Double escaping of install errors
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -Needs tests
FileSize
66.24 KB
75.23 KB

I don't think we need to test every one of the double-escaping fixes, maybe we can do that in a followup? Especially because this one is in the installer which I (maybe incorrectly) assume will be harder to test.

I manually tested this, here are before/after screenshots:

Before

After

longwave’s picture

I am not sure this is the correct fix. The message should be translatable, but the <p> tags should not be in the translatable content (nor, probably, the href URL), and adding !message to the end feels like a hack.

herom’s picture

Assigned: Unassigned » herom
Status: Reviewed & tested by the community » Needs work

I will post another patch soon.
There seems to be more cases of double-escape during install.

herom’s picture

Assigned: herom » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
47.82 KB
39.26 KB
34.6 KB
49.54 KB
74.59 KB
11.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,114 pass(es). View

During the requirements check, there are 3 places where can have double-escaped html. One was reported by the OP (in file system check). The other two are in the memory limit check, and required php extensions check.

Before
Memory limit

PHP extensions



After

File system

Memory limit

PHP extensions

herom’s picture

Re #18: There are ~500 t() calls in core that include html, and 4 of them start with a <p> tag (find using grep -rn "[^a-zA-Z]t('<p"). Also, the !message is already properly escaped when it is generated (inside runTestQuery()):

    $this->fail(t($fail, array('%query' => $query, '%error' => $e->getMessage(), '%name' => $this->name())));

So, I don't think there is a problem with the t() call. But, you were right about the href URL, fixed that in the last patch.

aneek’s picture

@herom,
Don't you think that instead of having so much in inline_template we could use a proper twig template? it will be simple and much easier to manage and good for readability.
What are your thoughts on these?

ivanjaros’s picture

@aneek: so you want to have templates for almost 200 languages?

aneek’s picture

@ivanjaros, I don't get your thought, sorry! But don't we have {% trans %}? Please correct me if I'm wrong. Why we do require to create 200 templates?

sun’s picture

  1. +++ b/core/modules/system/system.install
    @@ -107,15 +107,19 @@ function system_requirements($phase) {
         $item_list = array(
           '#theme' => 'item_list',
           '#items' => $missing_extensions,
         );
    -    $description .= drupal_render($item_list);
    

    I don't understand why we have to merge the list and the preceding paragraph into a single template.

    Can't we simply use a simple t() string in #prefix in this case?

    Or alternatively, two separate render element keys, first #markup, second #theme item_list.

  2. +++ b/core/modules/system/system.install
    @@ -188,26 +192,32 @@ function system_requirements($phase) {
    +      $description = array(
    +          '#type' => 'inline_template',
    +          '#template' => "{% if phase == 'install' %}
    
    @@ -396,39 +407,64 @@ function system_requirements($phase) {
    +    $requirements['file system']['description'] = array(
    +      '#type' => 'inline_template',
    +      '#template' => "{% for directory in directories %}
    

    This looks pretty horrible to my eyes... What's the reason for using an inline template here?

    We're still able to concatenate translated strings, no?

herom’s picture

FileSize
1.35 KB
11.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,154 pass(es). View

@aneek, I don't know. Should I move them into core/modules/system/templates/status-report.html.twig (where they are currently printed), or create separate templates (status-report-memory-limit.html.twig, status-report-file-system.html.twig)?

@sun,
1. moved into #prefix.
2. If the t() calls generate html (%variables do), we can't concatenate them without another SafeMarkup::set() call. But, I don't know if there is a better way than using template?

ivanjaros’s picture

@aneek - sorry, I don't know why but I was thiking in D7 terms :D

aneek’s picture

@herom,
I think it will be good to place those in separate template files. Lets ask othes too.

@ivanjaros, it's okay man. Happens :-)

@sun, do you think it will be a good idea to clean these inline_template and placing those as twig templates in system module?

sun’s picture

I'd recommend to ditch the template based approach altogether, primarily because it duplicates the same logic into two spots.

If Drupal is no longer able to concatenate strings (…really?), then I'd simply recompose the individual strings into separate #markup elements of a render array. It looks like most of them are exclusive/substitutes for a single 'error' anyway, optionally followed by a 'help'.

aneek’s picture

@sun, then we can wait for #2324371: Fix common HTML escaped render #key values due to Twig autoescape to get into core. This will eventually solve this issue, if we are using #markup elements.

Cottser’s picture

Closed #2341607: HTML tags in installer error message as a duplicate of this issue.

xlin’s picture

FileSize
259.74 KB
2.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,618 pass(es). View
2.15 KB

@Cottser, I'm posting patch for #2317281: Double escaping of install errors for review/incorporated.

yoroy’s picture

FileSize
47.71 KB

Are the bits of html I see in the Field UI part of this same issue or something else?

rteijeiro’s picture

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

Patch doesn't apply anymore. Working on it :)

rteijeiro’s picture

I fixed database settings form errors in #2346249: Fix wrong escaped errors in Installer - Database settings. Working on the rest errors.

rteijeiro’s picture

joelpittet’s picture

@yoroy I think that #33 is refereing to field_prefix/field_suffix... which I'm looking at fixing here #2324371: Fix common HTML escaped render #key values due to Twig autoescape though I stalled on that one because I feel a bit uneasy about the solution direction.

@rteijeiro is splitting this patch up a bit because it's easier to review/test and commit that way in chunks that can be individually confirmed that the strings are coming from a safe source or an alternative solution to their escaping is provided. Thanks you for that and please continue, we already got the first one in on this so it can be rolled out of this patch.

jibran’s picture

Issue tags: +SafeMarkup
matt2000’s picture

Status: Needs work » Needs review
FileSize
711 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,077 pass(es). View

I have a bias toward the simplest effective solution here, so here's the one line patch.

joelpittet’s picture

Assigned: rteijeiro » Unassigned
Status: Needs review » Closed (duplicate)

#2346287: Installer requirements errors escaped HTML in variables. This one got committed, not sure which one is right, but at least it's fixed in some regard.
Closing this as a duplicate, I do like the solution here too. May be worth opening a follow-up to clean that up a bit if possible.

#markup although is roughly equivalent to SafeMarkup::set in terms of security concerns and also because it will call that in the end anyway. Though the source of variables in question are safe.