Double escaping on /admin/modules/uninstall

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

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

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

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.

Patch submitted against the current dev branch for D8.

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.

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

#2289999: Add an easy way to create HTML on the fly without having to create a theme function / template is in

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

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.

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.

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!

Something like $this->assertNoRaw('<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!

@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!

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

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

@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!

Is this issue duplicate of #2310241: Uninstall page showing raw HTML output

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

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 :)

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?

@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, 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.

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

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.

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

same as above.

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

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

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 <em class="placeholder">sites/default/files</em> for sites/default/files

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