Issue #2324371 by lauriii, aneek, chx, joelpittet, webflo, Fabianx, rteijeiro:...

Issue #2324371 by lauriii, aneek, chx, joelpittet, webflo, Fabianx, rteijeiro: Fix common HTML escaped render #key values due to Twig autoescape.

core/includes/common.inc

@@ -2869,6 +2869,19 @@ function drupal_render(&$elements, $is_root_call = FALSE) {
 
   // Assume that if #theme is set it represents an implemented hook.
   $theme_is_implemented = isset($elements['#theme']);
+  // Check the elements for insecure HTML and pass through sanitization.
+  if (isset($elements)) {
+    $markup_keys = array(
+      '#description',
+      '#field_prefix',
+      '#field_suffix',
+    );
+    foreach ($markup_keys as $key) {
+      if (!empty($elements[$key]) && is_scalar($elements[$key])) {
+        $elements[$key] = SafeMarkup::checkAdminXss($elements[$key]);
+      }
+    }
+  }
 
   // Call the element's #theme function if it is set. Then any children of the
   // element have to be rendered there. If the internal #render_children
@@ -2950,8 +2963,9 @@ function drupal_render(&$elements, $is_root_call = FALSE) {
   // with how render cached output gets stored. This ensures that
   // #post_render_cache callbacks get the same data to work with, no matter if
   // #cache is disabled, #cache is enabled, there is a cache hit or miss.
-  $prefix = isset($elements['#prefix']) ? $elements['#prefix'] : '';
-  $suffix = isset($elements['#suffix']) ? $elements['#suffix'] : '';
+  $prefix = isset($elements['#prefix']) ? SafeMarkup::checkAdminXss($elements['#prefix']) : '';
+  $suffix = isset($elements['#suffix']) ? SafeMarkup::checkAdminXss($elements['#suffix']) : '';
+
   $elements['#markup'] = $prefix . $elements['#children'] . $suffix;
 
   // We've rendered this element (and its subtree!), now update the stack.

core/lib/Drupal/Component/Utility/SafeMarkup.php

@@ -52,7 +52,7 @@ class SafeMarkup {
    * or element that set it. Therefore, only valid HTML should be
    * marked as safe (never partial markup). For example, you should never do:
    * @code
-   *   SafeMarkup::set("<");
+   *   SafeMarkup::set('<');
    * @endcode
    * or:
    * @code
@@ -85,7 +85,7 @@ public static function set($string, $strategy = 'html') {
    * @param string $string
    *   The content to be checked.
    * @param string $strategy
-   *   The escaping strategy. See SafeMarkup::set(). Defaults to 'html'.
+   *   The escaping strategy. See self::set(). Defaults to 'html'.
    *
    * @return bool
    *   TRUE if the string has been marked secure, FALSE otherwise.
@@ -103,7 +103,7 @@ public static function isSafe($string, $strategy = 'html') {
    * added to any safe strings already marked for the current request.
    *
    * @param array $safe_strings
-   *   A list of safe strings as previously retrieved by SafeMarkup::getAll().
+   *   A list of safe strings as previously retrieved by self::getAll().
    *
    * @throws \UnexpectedValueException
    */
@@ -125,17 +125,33 @@ public static function setMultiple(array $safe_strings) {
   /**
    * Encodes special characters in a plain-text string for display as HTML.
    *
-   * @param $string
+   * @param string $string
    *   A string.
    *
    * @return string
    *   The escaped string. If $string was already set as safe with
-   *   SafeString::set, it won't be escaped again.
+   *   self::set(), it won't be escaped again.
    */
   public static function escape($string) {
     return static::isSafe($string) ? $string : String::checkPlain($string);
   }
 
+  /**
+   * Applies a very permissive XSS/HTML filter for admin-only use.
+   *
+   * @param string $string
+   *   A string.
+   *
+   * @return string
+   *   The escaped string. If $string was already set as safe with
+   *   self::set(), it won't be escaped again.
+   *
+   * @see \Drupal\Component\Utility\Xss::filterAdmin()
+   */
+  public static function checkAdminXss($string) {
+    return static::isSafe($string) ? $string : Xss::filterAdmin($string);
+  }
+
   /**
   * Retrieves all strings currently marked as safe.
   *

core/lib/Drupal/Core/Render/Element/HtmlTag.php

@@ -139,27 +139,33 @@ public static function preRenderConditionalComments($element) {
       $expression = '!IE';
     }
     else {
-      $expression = $browsers['IE'];
+      // The IE expression might contain some user input data.
+      $expression = SafeMarkup::checkAdminXss($browsers['IE']);
     }
 
-    // Wrap the element's potentially existing #prefix and #suffix properties with
-    // conditional comment markup. The conditional comment expression is evaluated
-    // by Internet Explorer only. To control the rendering by other browsers,
-    // either the "downlevel-hidden" or "downlevel-revealed" technique must be
-    // used. See http://en.wikipedia.org/wiki/Conditional_comment for details.
-    $element += array(
-      '#prefix' => '',
-      '#suffix' => '',
-    );
+    // If the #prefix and #suffix properties are used, wrap them with
+    // conditional comment markup. The conditional comment expression is
+    // evaluated by Internet Explorer only. To control the rendering by other
+    // browsers, use either the "downlevel-hidden" or "downlevel-revealed"
+    // technique. See http://en.wikipedia.org/wiki/Conditional_comment
+    // for details.
+
+    // Ensure what we are dealing with is safe.
+    // This would be done later anyway in drupal_render().
+    $prefix = isset($elements['#prefix']) ? SafeMarkup::checkAdminXss($elements['#prefix']) : '';
+    $suffix = isset($elements['#suffix']) ? SafeMarkup::checkAdminXss($elements['#suffix']) : '';
+
+    // Now calling SafeMarkup::set is safe, because we ensured the
+    // data coming in was at least admin escaped.
     if (!$browsers['!IE']) {
       // "downlevel-hidden".
-      $element['#prefix'] = "\n<!--[if $expression]>\n" . $element['#prefix'];
-      $element['#suffix'] .= "<![endif]-->\n";
+      $element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]>\n" . $prefix);
+      $element['#suffix'] = SafeMarkup::set($suffix . "<![endif]-->\n");
     }
     else {
       // "downlevel-revealed".
-      $element['#prefix'] = "\n<!--[if $expression]><!-->\n" . $element['#prefix'];
-      $element['#suffix'] .= "<!--<![endif]-->\n";
+      $element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]><!-->\n" . $prefix);
+      $element['#suffix'] = SafeMarkup::set($suffix . "<!--<![endif]-->\n");
     }
 
     return $element;

core/lib/Drupal/Core/Render/Element/MachineName.php

@@ -8,7 +8,6 @@
 namespace Drupal\Core\Render\Element;
 
 use Drupal\Component\Utility\NestedArray;
-use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Language\LanguageInterface;
 
@@ -152,13 +151,13 @@ public static function processMachineName(&$element, FormStateInterface $form_st
     $element['#machine_name']['suffix'] = '#' . $suffix_id;
 
     if ($element['#machine_name']['standalone']) {
-      $element['#suffix'] = SafeMarkup::set($element['#suffix'] . ' <small id="' . $suffix_id . '">&nbsp;</small>');
+      $element['#suffix'] = $element['#suffix'] . ' <small id="' . $suffix_id . '">&nbsp;</small>';
     }
     else {
       // Append a field suffix to the source form element, which will contain
       // the live preview of the machine name.
       $source += array('#field_suffix' => '');
-      $source['#field_suffix'] = SafeMarkup::set($source['#field_suffix'] . ' <small id="' . $suffix_id . '">&nbsp;</small>');
+      $source['#field_suffix'] = $source['#field_suffix'] . ' <small id="' . $suffix_id . '">&nbsp;</small>';
 
       $parents = array_merge($element['#machine_name']['source'], array('#field_suffix'));
       NestedArray::setValue($form_state->getCompleteForm(), $parents, $source['#field_suffix']);

core/modules/editor/editor.admin.inc

@@ -6,7 +6,6 @@
  */
 
 use Drupal\Core\StreamWrapper\StreamWrapperInterface;
-use Drupal\Component\Utility\SafeMarkup;
 use Drupal\editor\Entity\Editor;
 
 /**
@@ -93,8 +92,8 @@ function editor_image_upload_settings_form(Editor $editor) {
   $form['max_dimensions'] = array(
     '#type' => 'item',
     '#title' => t('Maximum dimensions'),
-    '#field_prefix' => SafeMarkup::set('<div class="container-inline clearfix">'),
-    '#field_suffix' => SafeMarkup::set('</div>'),
+    '#field_prefix' => '<div class="container-inline clearfix">',
+    '#field_suffix' => '</div>',
     '#description' => t('Images larger than these dimensions will be scaled down.'),
     '#states' => $show_if_image_uploads_enabled,
   );

core/modules/field_ui/src/Tests/FieldUiTestBase.php

@@ -105,6 +105,7 @@ function fieldUIAddExistingField($bundle_path, $initial_edit, $field_edit = arra
 
     // First step : 'Re-use existing field' on the 'Manage fields' page.
     $this->drupalPostForm("$bundle_path/fields", $initial_edit, t('Save'));
+    $this->assertNoRaw('<', 'The page does not have double escaped HTML tags.');
 
     // Second step : 'Field settings' form.
     $this->drupalPostForm(NULL, $field_edit, t('Save settings'));

core/modules/options/src/Tests/OptionsFieldUITest.php

@@ -278,6 +278,7 @@ protected function createOptionsField($type) {
   function assertAllowedValuesInput($input_string, $result, $message) {
     $edit = array('field_storage[settings][allowed_values]' => $input_string);
     $this->drupalPostForm($this->admin_path, $edit, t('Save field settings'));
+    $this->assertNoRaw('<', 'The page does not have double escaped HTML tags.');
 
     if (is_string($result)) {
       $this->assertText($result, $message);

core/modules/rdf/rdf.module

@@ -498,14 +498,14 @@ function rdf_preprocess_comment(&$variables) {
   }
   // Adds RDF metadata markup above comment body.
   if (!empty($variables['rdf_metadata_attributes'])) {
-    if (!isset($variables['content']['comment_body']['#prefix'])) {
-      $variables['content']['comment_body']['#prefix'] = '';
-    }
     $rdf_metadata = array(
       '#theme' => 'rdf_metadata',
       '#metadata' => $variables['rdf_metadata_attributes'],
     );
-    $variables['content']['comment_body']['#prefix'] = drupal_render($rdf_metadata) . $variables['content']['comment_body']['#prefix'];
+    if (!empty($variables['content']['comment_body']['#prefix'])) {
+      $rdf_metadata['#suffix'] = $variables['content']['comment_body']['#prefix'];
+    }
+    $variables['content']['comment_body']['#prefix'] = drupal_render($rdf_metadata);
   }
 }
 

core/modules/system/src/Tests/Common/RenderTest.php

@@ -808,10 +808,10 @@ function testDrupalRenderRenderCachePlaceholder() {
         ),
       ),
       '#markup' => $placeholder,
-      '#prefix' => '<foo>',
-      '#suffix' => '</foo>'
+      '#prefix' => '<pre>',
+      '#suffix' => '</pre>',
     );
-    $expected_output = '<foo><bar>' . $context['bar'] . '</bar></foo>';
+    $expected_output = '<pre><bar>' . $context['bar'] . '</bar></pre>';
 
     // #cache disabled.
     $element = $test_element;
@@ -852,7 +852,7 @@ function testDrupalRenderRenderCachePlaceholder() {
     $this->assertIdentical($token, $expected_token, 'The tokens are identical');
     // Verify the token is in the cached element.
     $expected_element = array(
-      '#markup' => '<foo><drupal-render-cache-placeholder callback="common_test_post_render_cache_placeholder" token="'. $expected_token . '"></drupal-render-cache-placeholder></foo>',
+      '#markup' => '<pre><drupal-render-cache-placeholder callback="common_test_post_render_cache_placeholder" token="'. $expected_token . '"></drupal-render-cache-placeholder></pre>',
       '#attached' => array(),
       '#post_render_cache' => array(
         'common_test_post_render_cache_placeholder' => array(
@@ -895,11 +895,11 @@ function testDrupalRenderChildElementRenderCachePlaceholder() {
           ],
         ],
         '#markup' => $placeholder,
-        '#prefix' => '<foo>',
-        '#suffix' => '</foo>'
+        '#prefix' => '<pre>',
+        '#suffix' => '</pre>'
       ],
     ];
-    $expected_output = '<foo><bar>' . $context['bar'] . '</bar></foo>' . "\n";
+    $expected_output = '<pre><bar>' . $context['bar'] . '</bar></pre>' . "\n";
 
     // #cache disabled.
     $element = $test_element;
@@ -943,7 +943,7 @@ function testDrupalRenderChildElementRenderCachePlaceholder() {
     $this->assertIdentical($token, $expected_token, 'The tokens are identical for the child element');
     // Verify the token is in the cached element.
     $expected_element = array(
-      '#markup' => '<foo><drupal-render-cache-placeholder callback="common_test_post_render_cache_placeholder" token="'. $expected_token . '"></drupal-render-cache-placeholder></foo>',
+      '#markup' => '<pre><drupal-render-cache-placeholder callback="common_test_post_render_cache_placeholder" token="'. $expected_token . '"></drupal-render-cache-placeholder></pre>',
       '#attached' => array(),
       '#post_render_cache' => array(
         'common_test_post_render_cache_placeholder' => array(
@@ -969,7 +969,7 @@ function testDrupalRenderChildElementRenderCachePlaceholder() {
     $this->assertIdentical($token, $expected_token, 'The tokens are identical for the parent element');
     // Verify the token is in the cached element.
     $expected_element = array(
-      '#markup' => '<foo><drupal-render-cache-placeholder callback="common_test_post_render_cache_placeholder" token="'. $expected_token . '"></drupal-render-cache-placeholder></foo>' . "\n",
+      '#markup' => '<pre><drupal-render-cache-placeholder callback="common_test_post_render_cache_placeholder" token="'. $expected_token . '"></drupal-render-cache-placeholder></pre>' . "\n",
       '#attached' => array(),
       '#post_render_cache' => array(
         'common_test_post_render_cache_placeholder' => array(
@@ -999,7 +999,7 @@ function testDrupalRenderChildElementRenderCachePlaceholder() {
     $this->assertIdentical($token, $expected_token, 'The tokens are identical for the child element');
     // Verify the token is in the cached element.
     $expected_element = array(
-      '#markup' => '<foo><drupal-render-cache-placeholder callback="common_test_post_render_cache_placeholder" token="'. $expected_token . '"></drupal-render-cache-placeholder></foo>',
+      '#markup' => '<pre><drupal-render-cache-placeholder callback="common_test_post_render_cache_placeholder" token="'. $expected_token . '"></drupal-render-cache-placeholder></pre>',
       '#attached' => array(),
       '#post_render_cache' => array(
         'common_test_post_render_cache_placeholder' => array(

core/modules/system/system.admin.inc

@@ -259,7 +259,7 @@ function theme_system_modules_details($variables) {
       '#type' => 'details',
       '#title' => SafeMarkup::set('<span class="text"> ' . drupal_render($module['description']) . '</span>'),
       '#attributes' => array('id' => $module['enable']['#id'] . '-description'),
-      '#description' => SafeMarkup::set($description),
+      '#description' => $description,
     );
     $col4 = drupal_render($details);
     $row[] = array('class' => array('description', 'expand'), 'data' => $col4);