Remove SafeMarkup::set in theme_system_modules_details()

Minor title tweak :)

Couple of nitpicks but I think this is looking good thanks @sclapp

1. +++ b/core/modules/system/system.admin.inc
   @@ -229,8 +229,13 @@ function theme_system_modules_details($variables) {
   +        ['@id' => $id,
   +        '@enableid' => $module['enable']['#id'],
   +        '@modulename' => $module_name]);
   

   The format of the array is not in the format of our coding standards, will discuss IRL.

2. +++ b/core/modules/system/system.admin.inc
   @@ -229,8 +229,13 @@ function theme_system_modules_details($variables) {
   +    $row[] = array('class' => array('module'), 'data' => $col2);
   

   Array syntax should be the short syntax.

I think this should fix some of the fails... one sec I'll explain...

+++ b/core/modules/system/system.admin.inc
@@ -229,8 +229,8 @@ function theme_system_modules_details($variables) {
     // Add the module label and expand/collapse functionality.
     $id = Html::getUniqueId('module-' . $key);

@@ -259,7 +259,7 @@ function theme_system_modules_details($variables) {
-      '#title' => SafeMarkup::set('<span class="text"> ' . drupal_render($module['description']) . '</span>'),
+      '#title' => SafeMarkup::format('<span class="text"> @description</span>', ['@description' => drupal_render($module['description'])]),

There was another SafeMarkup::set in that file.

1. +++ b/core/modules/system/system.admin.inc
   @@ -225,17 +225,16 @@ function theme_system_modules_details($variables) {
   -    $row[] = array('class' => array('checkbox'), 'data' => drupal_render($module['enable']));
   +    $row[] = ['class' => ['checkbox'], 'data' => drupal_render($module['enable'])];
   

   As much as I love short syntax this line wasn't changed for the issue so the short syntax doesn't need to change.

2. +++ b/core/modules/system/system.admin.inc
   @@ -225,17 +225,16 @@ function theme_system_modules_details($variables) {
   -    // Add the module label and expand/collapse functionality.
   -    $id = Html::getUniqueId('module-' . $key);
   

   These lines were accidentally deleted. Likely the reason for the failures.

3. +++ b/core/modules/system/system.admin.inc
   @@ -225,17 +225,16 @@ function theme_system_modules_details($variables) {
   +    $col2 = SafeMarkup::format('<label id="@id" for="@enableid" class="module-name table-filter-text-source">@modulename</label>',['@id' => $id, '@enableid' => $module['enable']['#id'], '@modulename' => $module_name]);
   

   There was a missing space between the , and the [. And all the words in keywords I put an underscore to separate the words.

4. +++ b/core/modules/system/system.admin.inc
   @@ -225,17 +225,16 @@ function theme_system_modules_details($variables) {
   -    $description .= '<div class="admin-requirements">' . t('Machine name: !machine-name', array('!machine-name' => '<span dir="ltr" class="table-filter-text-source">' . $key . '</span>')) . '</div>';
   +    $description .= '<div class="admin-requirements">' . t('Machine name: !machine-name', ['!machine-name' => '<span dir="ltr" class="table-filter-text-source">' . $key . '</span>']) . '</div>';
   

   This was also out of scope.

5. +++ b/core/modules/update/update.module
   @@ -517,10 +517,10 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
   -        $text = t('There was a problem checking <a href="@update-report">available updates</a> for Drupal.', array('@update-report' => \Drupal::url('update.status')), array('langcode' => $langcode));
   +        $text = t('There was a problem checking <a href="@update-report">available updates</a> for Drupal.', ['@update-report' => \Drupal::url('update.status')], ['langcode' => $langcode]);
   ...
   -        $text = t('There was a problem checking <a href="@update-report">available updates</a> for your modules or themes.', array('@update-report' => \Drupal::url('update.status')), array('langcode' => $langcode));
   +        $text = t('There was a problem checking <a href="@update-report">available updates</a> for your modules or themes.', ['@update-report' => \Drupal::url('update.status')], ['langcode' => $langcode]);
   
   @@ -532,10 +532,10 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
   -      $text .= ' ' . t('See the <a href="@available_updates">available updates</a> page for more information and to install your missing updates.', array('@available_updates' => \Drupal::url('update.report_update', [], ['language' => $language])), array('langcode' => $langcode));
   +      $text .= ' ' . t('See the <a href="@available_updates">available updates</a> page for more information and to install your missing updates.', ['@available_updates' => \Drupal::url('update.report_update', [], ['language' => $language])], ['langcode' => $langcode]);
   ...
   -      $text .= ' ' . t('See the <a href="@available_updates">available updates</a> page for more information.', array('@available_updates' => \Drupal::url('update.status', [], ['language' => $language])), array('langcode' => $langcode));
   +      $text .= ' ' . t('See the <a href="@available_updates">available updates</a> page for more information.', ['@available_updates' => \Drupal::url('update.status', [], ['language' => $language])], ['langcode' => $langcode]);
   

   more of my favourite syntax that is out of scope.

manually tested #10; the HTML output is identical. (joelpiettet's comments in #11 apply to #8)

Manually tested this patch; the HTML output is identical.

Patch replaces SafeMarkup::set with SafeMarkup::format in 2 instances

1. +++ b/core/modules/system/system.admin.inc
   @@ -229,8 +229,8 @@ function theme_system_modules_details($variables) {
   +    $row[] = ['class' => ['module'], 'data' => $col2];
   

   - @id/@enable_id/@module_name are generated using module-specific values generated by system_rebuild_module_data(). I could not find specifically if/where ExtensionDiscovery::scan, InfoParser, etc sanitizes data pulled from the module's .info.yml file
   - updating second line to short array syntax

2. +++ b/core/modules/system/system.admin.inc
   @@ -259,7 +259,7 @@ function theme_system_modules_details($variables) {
   +      '#title' => SafeMarkup::format('<span class="text"> @description</span>', ['@description' => drupal_render($module['description'])]),
   

   - @description uses module-specific values from same source as above

It does not appear that content provided in the .info.yml file is appropriately sanitized. In some quick testing, adding

So the variables are being sanitized because they are using @placeholder format. We still need to investigate if its necessary to have the drupal_render in the theme_system_modules_details.

> Or are we just trusting module devs at this point?

Yes, we do, you are about to run their PHP code on your server and you are worried about them XSS'ing you?? I don't think this is a real problem. Also, it makes bad_judgement auto enabling itself possible :P

Since safemarkup::format approach is poo-poo'd. Inline template is probably better and @lauriii is right no need for the drupal_render, especially with this approach.

I gave it a go, but could use a second pair of eyes.

Tested this manually and seems to work. The code is also good and we will have one less drupal_render call! How ever we still need test coverage for this.

Is the test needed for an XSS in the module description? That is what I did here. Is is not escaped.

This should fix the tests

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -264,7 +264,7 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
-    $row['description']['#markup'] = $this->t($module->info['description']);
+    $row['description']['#markup'] = $this->t(SafeMarkup::escape($module->info['description']));

Isn't this case similar to the local task/menu links issue? So should the description here actually return a Translation Wrapper?

This doesn't protected against XSS, but to be honest a module author can produce an XSS in a different way SO easily

I agree that there is too many ways module author can produce a XSS or even worse things to happen so maybe it doesn't make any sense to test this even though it would be nice. So lets remove the test and the SafeMarkup::escape().

I'm not getting why "This doesn't protected against XSS,"
the patch in #24 looks to be escaping html from the description and has a test to prove it is.

#24 looks good to me...

So this is interesting.

I've temporarily unpublished this issue because the issue also exists in D7. @alexpott pointed out that one difference between this and other exploits a module author could introduce is that it can be executed without even installing the module; it just needs to be on the file system. That probably isn't a reason to consider this a sec issue and it is probably security hardening at most. But we should confirm this first with the sec team. @alexpott said he'd do so.

There is a use case for putting HTML in a module description I think, so XSS filtering over escaping would be preferable if we do decide it needs to be sanitized. Can someone manually test safe HTML in a module description, and if that is currently supported, add test coverage for it as well?

Also, in either case, we need to document what is allowed for module descriptions and whether or not it's expected to be safe.

Also, @alexpott pointed out that doing the escaping inside the translation breaks the translation, so the filtering or escaping should be done on the result of the t() call instead.

theme_system_modules_details() could also potentially be converted to a template straight up, but that is likely a lot of work. It would be good to review it.

The security issue is known and can be public.

also: #2527544: Module list page is not XSS safe

I would worry more (especially for D7) about a module that just buried a PHP shell among its files.

So I think this issue should drop trying to make the description safe and leave that to #2527544: Module list page is not XSS safe. It should just focus on Remove or document SafeMarkup::set in theme_system_modules_details()

This is a rerolled version of #20 with extra space removed from the inline template.

Here is the related conversion issue.

This looks really close, thanks for the re-roll.

+++ b/core/modules/system/system.admin.inc
@@ -255,14 +263,18 @@ function theme_system_modules_details($variables) {
+      '#template' => '<span class="text">{{ module_description }}</span>',
...
-      '#title' => SafeMarkup::set('<span class="text module-description">' . drupal_render($module['description']) . '</span>'),

missing the CSS class "module-description".

I've added that missing class name.

Thank you @golddragon007. I did a markup comparsion to make sure things aren't missing. Looks good thank you!

Late rendering++

Committed 8402939 and pushed to 8.0.x. Thanks!