Module names on the uninstall page are double-encoded:

<label for="edit-uninstall-block" class="module-name table-filter-text-source">Block</label>
CommentFileSizeAuthor
#44 2305831-44.patch595 bytesmarcus7777
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,745 pass(es). View
#43 Screenshot from 2014-09-29 12:07:52.png833.51 KBmarcus7777
#37 interdiff.txt882 bytesCottser
#37 2305831-37.patch2.64 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,671 pass(es). View
#35 2305831_35.patch2.65 KBchx
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,170 pass(es). View
#35 interdiff.txt750 byteschx
#31 modules-uninstall-double-escape-2305831-31-testwithfix-pass.patch2.71 KBaneek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,514 pass(es). View
#31 modules-uninstall-double-escape-2305831-31-testonly-fail.patch660 bytesaneek
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,168 pass(es), 1 fail(s), and 0 exception(s). View
#17 modules-uninstall-double-install-2305831-17-d8.patch2.07 KBaneek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,393 pass(es). View
#15 modules-uninstall-double-install-2305831-15-d8.patch2.33 KBaneek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,378 pass(es). View
#10 modules-uninstall-double-install-2305831-10-D8.patch1.07 KBaneek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,260 pass(es). View
#1 modules-uninstall-double-install.patch1.14 KBjbrown
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jbrown’s picture

FileSize
1.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Here's the patch - is there a better way?

jbrown’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: modules-uninstall-double-install.patch, failed testing.

Damien Tournoud’s picture

SafeMarkup::set(drupal_render(...))

^ The output of drupal_render() is already a SafeMarkup, no?

jbrown’s picture

When you perform string concatenation the object is converted to a string and is therefore no longer marked as safe.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: modules-uninstall-double-install.patch, failed testing.

jbrown’s picture

Why is this patch failing?

LinL’s picture

Needs to be diff'd against the new 8.0.x branch instead of the old 8.x branch?

https://groups.drupal.org/node/434068

aneek’s picture

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

Diff'd against the new dev branch 8.0.x (https://www.drupal.org/node/2311847). Code is still the same but suggestions are made in issue queue to use SafeMarkup::implode or its suggested to use

appropriate sanitization functions or the theme and render systems so that the output can can be themed, escaped, and altered properly

from class SafeMarkup.

aneek’s picture

Status: Needs work » Needs review

Patch submitted against the current dev branch for D8.

m.stenta’s picture

Status: Needs review » Reviewed & tested by the community

The patch in #10 fixes the issue.

@Damien, regarding comment #4: it's worth noting that the main module list page (/admin/modules) uses a drupal_render() inside SafeMarkup::set(), as well. I'm not sure if that's the proper usage, but perhaps we can still commit this and then fix both of those in another issue,if necessary.

Marking RTBC since this solves the main issue.

chx’s picture

Status: Reviewed & tested by the community » Postponed

We are not adding more SafeMarkup::set calls to core. We will fix these based on #2289999: Add an easy way to create HTML on the fly without having to create a theme function / template

vijaycs85’s picture

aneek’s picture

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

Based on #2289999 issue and the newly introduced element "inline_template" this page is created. Needs a logical review as well. :)

aneek’s picture

Status: Active » Needs review
aneek’s picture

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

The checkboxes in the uninstall page doesn't require the inline templates. So removing the logic to add the template in the checkboxes row. Patch prepared with the new changes.

chx’s picture

Status: Needs review » Needs work

Thanks so much for patching this! I am really glad it worked out. Could we please add a test? Or, I guess, amend an existing one.

aneek’s picture

Welcome chx. :)
About the testcase, is it regarding the #type introduced? If then I guess its already in place Commit @ 76608ff. If its not about the theme system then it will be really helpful for me to know your thoughts in details, so I can make a automated test case.

Thanks!

chx’s picture

Something like $this->assertNoRaw('&lt;label') to make sure there's no double escaped HTML. Please post a test only , failing patch and the patch with the test to show before/after. Thanks!

aneek’s picture

@chx,
Got your point. Since this will again be related to the theme implementation I think I'll amend test cases in core/modules/system/src/Tests/Theme/TwigEnvironmentTest.php file.

Thanks!

chx’s picture

No, there should be an uninstall test -- you are testing the implementation of this twig functionality on the uninstall page.

aneek’s picture

Thanks for the info. I'll start working on it.

jbrown’s picture

Assigned: jbrown » Unassigned
aneek’s picture

Assigned: Unassigned » aneek
aneek’s picture

@chx,
While developing the test case I ran into one issue,
Once the page "admin/modules/uninstall" is loaded and its status 200, I added a code to check if the page has any multiple HTML escapes. Following is the code,

class UninstallPageLoad extends WebTestBase {

  /**
   * Loads module uninstall page and checks HTML escaping
   */
  function testUninstallPageHtmlEscape() {
    // Create new admin user and test page load.
    $account = $this->drupalCreateUser(array('administer modules'));
    $this->drupalLogin($account);
    $this->drupalGet('admin/modules/uninstall');
    $this->assertResponse(200, 'Module Uninstall page loaded.');

    // Test the page if it has multiple HTML sanitizations.
    $this->assertNoRaw('<label');
    $this->assertNoPattern('/<label[^>]*>/');
  }
}

The page has the following RAW HTML,

<tr class="odd">
      <td align="center">
        <div class="form-item form-type-checkbox form-item-uninstall-block form-disabled form-no-label">
          <label class="visually-hidden" for="edit-uninstall-block">Uninstall Block module</label>
          <input disabled="disabled" type="checkbox" id="edit-uninstall-block" name="uninstall[block]" value="1" class="form-checkbox" />
        </div>
      </td>
      <td>&lt;label for=&quot;edit-uninstall-block&quot; class=&quot;module-name table-filter-text-source&quot;&gt;Block&lt;/label&gt;</td>
      <td class="description">Controls the visual building blocks a page is constructed with. Blocks are boxes of content rendered into an area, or region, of a web page.&lt;div class=&quot;admin-requirements&quot;&gt;To uninstall Block, the following module must be uninstalled first: Custom Block&lt;/div&gt;</td>
    </tr>

While I ran the test case, isn't that the method should give me a failed error as the page has multiple HTML escapes? But I am getting a success result as the given input to $this->assertNoRaw is not found. Same happens for the $this->assertNoPattern method.

Am I missing something? It will be really helpful if you can have a look at this and help me out.

Further found that, using the test code the RAW HTML output is not the same as I gave earlier. Its like,

<table class="responsive-enabled">
  <thead>
    <tr>
      <th>Uninstall</th>
      <th>Name</th>
      <th>Description</th>
    </tr>
  </thead>
  <tbody>
    <tr class="odd">
      <td colspan="3" class="empty message">No modules are available to uninstall.</td>
    </tr>
  </tbody>
</table>

This explains one thing as why the asserts were not working. But now how can we reproduce the page HTML output of theme_system_modules_uninstall theme function to this test case?

Thanks!

CharuAg’s picture

chx’s picture

That's really odd. Try $this->assertNoRaw('&<label') ... I know, ridiculous but I wonder what escaping / de-escaping goes on with SimpleXML.

aneek’s picture

I'll try as you mentioned chx. But still I think its due to the '#empty' => t('No modules are available to uninstall.') code present in the table theme.
I think if I could get the system_modules_uninstall form to my test case via

$form = \Drupal::formBuilder()->getForm('Drupal\system\Form\ModulesUninstallForm');

Once done, hopefully I can either use $this->assertThemeOutput() or just can use the strpos the same way used in Drupal\system\Tests\Form\CheckboxTest

Thanks again chx :)

aneek’s picture

chx, I just tested with the snippet you gave me but still no success. Then I tried to use the getForm option to load the form and then tried to render it via drupal_render. The form loaded but with some missing parts as,
$form['modules'] & $form['uninstall'].
So again I am getting, No modules are available to uninstall. message.
For this testing purpose we do need these arrays to be generated as the faulty HTML resides over here.

The code is as follows,

class UninstallPageLoad extends WebTestBase {

  /**
   * Loads module uninstall page and checks HTML escaping
   */
  function testUninstallPageHtmlEscape() {
    // Create new admin user and test page load.
    /*$account = $this->drupalCreateUser(array('administer modules'));
    $this->drupalLogin($account);
    $this->drupalGet('admin/modules/uninstall');
    $this->assertResponse(200, 'Module Uninstall page loaded.');*/


    // Test the page if it has multiple HTML sanitizations.
    $form = \Drupal::formBuilder()->getForm('Drupal\system\Form\ModulesUninstallForm');
    $form_op = drupal_render($form);
    $fp = fopen('/home/aneek/www/drupal8/data.txt', 'w');
    fwrite($fp, var_export($form, TRUE));
    fwrite($fp, $form_op);
    fclose($fp);
  }
}

The output I pasted in pastebin

Any ideas how to load the proper form array over in the test method? It will not be a good idea to use the hardcoded form array in the test case as an input to assertThemeOutput() method. So it's needed to have a proper dynamic way to load all the modules and then finally call the theme function system_modules_uninstall from the system.admin.inc page.
Is it desired that when I call $this->drupalGet('admin/modules/uninstall'); it will load a blank page with no module listed?

aneek’s picture

FileSize
660 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,168 pass(es), 1 fail(s), and 0 exception(s). View
2.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,514 pass(es). View

@chx,
I had a look at the other testcase files and saw that I was missing a part where some modules were assigned to an array to enable while the test ran. The "Verbose message" generated the faulty HTML and from there I got a simple fix to use my test scenario.

Uploading the fail patch and the pass patch. Also marking this issue for review.

aneek’s picture

Status: Needs work » Needs review

jbrown’s picture

Works for me!

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
750 bytes
2.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,170 pass(es). View

aneek, thanks for your tenacity and fixing this properly. I am really happy it worked out. I have attached a minor wording fix -- and I do not think we need a comment repeating the assert message.

Cottser’s picture

I'm curious, why is the default filter used here?

Cottser’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,671 pass(es). View
882 bytes

Talked to @chx in IRC and we agreed that rolling a patch would be good to 1. try it to see if there is a difference (not sure if the tests go this far) and 2. show what I mean :)

The only other change besides removing the |default filter (I don't know what it would do if not given an argument, http://twig.sensiolabs.org/doc/filters/default.html doesn't mention that) is getting rid of whitespace inside the <div> that wasn't there before.

Edited to add missing code tags.

wylbur’s picture

Issue tags: +TCDrupal 2014

Reviewed patch #37

Installed Drupal 8.0.0-dev
mysql 5.5
PHP 5.5

Ran install script
Visited /admin/modules/uninstall
Uninstall listings showed escaped html in name and description columns

Applied 2305831-37.patch
Patch applied changes without any errors.

Visited /admin/modules/uninstall
Uninstall listings contained no escaped html in name and description columns
Searched for >< in the page code - not found
Searched for <label - not found

Uninstalled all modules
The markeup around the resulting "No modules are available to uninstall." also did not contain any escaped html

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

same as above.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1a8367c and pushed to 8.0.x. Thanks!

  • alexpott committed 1a8367c on 8.0.x
    Issue #2305831 by aneek, Cottser, chx, jbrown: Fixed Double escaping on...

Status: Fixed » Closed (fixed)

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

marcus7777’s picture

on core/install.php?langcode=en&profile=standard still getting double escaping.Screenshot

tracking it down:
on line 403 of core/modules/system/system.install :

$error .= t('The directory %directory does not exist.', array('%directory' => $directory)) . ' ';

and $directory is sites/default/files

so t is outputing &lt;em class=&quot;placeholder&quot;&gt;sites/default/files&lt;/em&gt; for sites/default/files

marcus7777’s picture

FileSize
595 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,745 pass(es). View

fixed output. this is my first patch. Thanks to asplicious

Only local images are allowed.

marcus7777’s picture

Status: Closed (fixed) » Needs review
jbrown’s picture

Status: Needs review » Closed (fixed)

Thanks for the patch, but this is the wrong issue.

Please use this issue: #2346287: Installer requirements errors escaped HTML in variables..

jibran’s picture

Issue tags: +SafeMarkup