Issue #2506195 by alexpott, joelpittet, xjm, David_Rothstein, Fabianx,...

Issue #2506195 by alexpott, joelpittet, xjm, David_Rothstein, Fabianx, pwolanin: Remove SafeMarkup::set() from Xss::filter()

core/includes/theme.inc

@@ -1350,7 +1350,7 @@ function template_preprocess_page(&$variables) {
   $variables['language']          = $language_interface;
   $variables['logo']              = theme_get_setting('logo.url');
   $variables['site_name']         = (theme_get_setting('features.name') ? SafeMarkup::checkPlain($site_config->get('name')) : '');
-  $variables['site_slogan']       = (theme_get_setting('features.slogan') ? Xss::filterAdmin($site_config->get('slogan')) : '');
+  $variables['site_slogan']['#markup'] = (theme_get_setting('features.slogan') ? $site_config->get('slogan') : '');
 
   // An exception might be thrown.
   try {

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

@@ -15,9 +15,9 @@
  * provides a store for known safe strings and methods to manage them
  * throughout the page request.
  *
- * Strings sanitized by self::checkPlain() or Xss::filter() are automatically
- * marked safe, as are markup strings created from render arrays via
- * drupal_render().
+ * Strings sanitized by self::checkPlain() and self::escape() or
+ * self::xssFilter() are automatically marked safe, as are markup strings
+ * created from @link theme_render render arrays @endlink via drupal_render().
  *
  * This class should be limited to internal use only. Module developers should
  * instead use the appropriate
@@ -141,17 +141,79 @@ public static function escape($string) {
   /**
    * Applies a very permissive XSS/HTML filter for admin-only use.
    *
-   * @param string $string
-   *   A string.
+   * Note: This method only filters if $string is not marked safe already.
    *
-   * @return string
-   *   The escaped string. If $string was already set as safe with
-   *   self::set(), it won't be escaped again.
+   * @deprecated as of Drupal 8.0.x, will be removed before Drupal 8.0.0. If the
+   *   string used as part of a @link theme_render render array @endlink use
+   *   #markup to allow the render system to filter automatically. If the result
+   *   is not being used directly in the rendering system (for example, when its
+   *   result is being combined with other strings before rendering), use
+   *   Xss::filterAdmin(). Otherwise, use SafeMarkup::xssFilter() and the tag
+   *   list provided by Xss::getAdminTagList() instead. In the rare instance
+   *   that the caller does not want to filter strings that are marked safe
+   *   already, it needs to check SafeMarkup::isSafe() itself.
    *
+   * @see \Drupal\Component\Utility\SafeMarkup::xssFilter()
+   * @see \Drupal\Component\Utility\SafeMarkup::isSafe()
    * @see \Drupal\Component\Utility\Xss::filterAdmin()
+   * @see \Drupal\Component\Utility\Xss::getAdminTagList()
    */
   public static function checkAdminXss($string) {
-    return static::isSafe($string) ? $string : Xss::filterAdmin($string);
+    return static::isSafe($string) ? $string : static::xssFilter($string, Xss::getAdminTagList());
+  }
+
+  /**
+   * Filters HTML for XSS vulnerabilities and marks the result as safe.
+   *
+   * Calling this method unnecessarily will result in bloating the safe string
+   * list and increases the chance of unintended side effects.
+   *
+   * If Twig receives a value that is not marked as safe then it will
+   * automatically encode special characters in a plain-text string for display
+   * as HTML. Therefore, SafeMarkup::filterXss() should only be used when the
+   * string might contain HTML that needs to be rendered properly by the
+   * browser.
+   *
+   * If you need to filter for admin use, like Xss::filterAdmin(), then:
+   * - If the string is used as part of a @link theme_render render array @endlink,
+   *   use #markup to allow the render system to filter by the admin tag list
+   *   automatically.
+   * - Otherwise, use the SafeMarkup::xssFilter() with tag list provided by
+   *   Xss::getAdminTagList() instead.
+   *
+   * This method should only be used instead of Xss::filter() when the result is
+   * being added to a render array that is constructed before rendering begins.
+   *
+   * In the rare instance that the caller does not want to filter strings that
+   * are marked safe already, it needs to check SafeMarkup::isSafe() itself.
+   *
+   * @param $string
+   *   The string with raw HTML in it. It will be stripped of everything that
+   *   can cause an XSS attack. The string provided will always be escaped
+   *   regardless of whether the string is already marked as safe.
+   * @param array $html_tags
+   *   (optional) An array of HTML tags. If omitted, it uses the default tag
+   *   list defined by \Drupal\Component\Utility\Xss::filter().
+   *
+   * @return string
+   *   An XSS-safe version of $string, or an empty string if $string is not
+   *   valid UTF-8. The string is marked as safe.
+   *
+   * @ingroup sanitization
+   *
+   * @see \Drupal\Component\Utility\Xss::filter()
+   * @see \Drupal\Component\Utility\Xss::filterAdmin()
+   * @see \Drupal\Component\Utility\Xss::getAdminTagList()
+   * @see \Drupal\Component\Utility\SafeMarkup::isSafe()
+   */
+  public static function xssFilter($string, $html_tags = NULL) {
+    if (is_null($html_tags)) {
+      $string = Xss::filter($string);
+    }
+    else {
+      $string = Xss::filter($string, $html_tags);
+    }
+    return static::set($string);
   }
 
   /**

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

@@ -29,14 +29,19 @@ class Xss {
    * Based on kses by Ulf Harnhammar, see http://sourceforge.net/projects/kses.
    * For examples of various XSS attacks, see: http://ha.ckers.org/xss.html.
    *
-   * This code does five things:
+   * This method is preferred to
+   * \Drupal\Component\Utility\SafeMarkup::xssFilter() when the result is not
+   * being used directly in the rendering system (for example, when its result
+   * is being combined with other strings before rendering). This avoids
+   * bloating the safe string list with partial strings if the whole result will
+   * be marked safe.
+   *
+   * This code does four things:
    * - Removes characters and constructs that can trick browsers.
    * - Makes sure all HTML entities are well-formed.
    * - Makes sure all HTML tags and attributes are well-formed.
    * - Makes sure no HTML tags contain URLs with a disallowed protocol (e.g.
    *   javascript:).
-   * - Marks the sanitized, XSS-safe version of $string as safe markup for
-   *   rendering.
    *
    * @param $string
    *   The string with raw HTML in it. It will be stripped of everything that
@@ -49,7 +54,7 @@ class Xss {
    *   valid UTF-8.
    *
    * @see \Drupal\Component\Utility\Unicode::validateUtf8()
-   * @see \Drupal\Component\Utility\SafeMarkup
+   * @see \Drupal\Component\Utility\SafeMarkup::xssFilter()
    *
    * @ingroup sanitization
    */
@@ -83,7 +88,7 @@ public static function filter($string, $html_tags = array('a', 'em', 'strong', '
     // for output. All other known XSS vectors have been filtered out by this
     // point and any HTML tags remaining will have been deliberately allowed, so
     // it is acceptable to call SafeMarkup::set() on the resultant string.
-    return SafeMarkup::set(preg_replace_callback('%
+    return preg_replace_callback('%
       (
       <(?=[^a-zA-Z!/])  # a lone <
       |                 # or
@@ -92,7 +97,7 @@ public static function filter($string, $html_tags = array('a', 'em', 'strong', '
       <[^>]*(>|$)       # a string that starts with a <, up until the > or the end of the string
       |                 # or
       >                 # just a >
-      )%x', $splitter, $string));
+      )%x', $splitter, $string);
   }
 
   /**
@@ -103,6 +108,13 @@ public static function filter($string, $html_tags = array('a', 'em', 'strong', '
    * is desired (so \Drupal\Component\Utility\SafeMarkup::checkPlain() is
    * not acceptable).
    *
+   * This method is preferred to
+   * \Drupal\Component\Utility\SafeMarkup::xssFilter() when the result is
+   * not being used directly in the rendering system (for example, when its
+   * result is being combined with other strings before rendering). This avoids
+   * bloating the safe string list with partial strings if the whole result will
+   * be marked safe.
+   *
    * Allows all tags that can be used inside an HTML body, save
    * for scripts and styles.
    *
@@ -111,6 +123,12 @@ public static function filter($string, $html_tags = array('a', 'em', 'strong', '
    *
    * @return string
    *   The filtered string.
+   *
+   * @ingroup sanitization
+   *
+   * @see \Drupal\Component\Utility\SafeMarkup::xssFilter()
+   * @see \Drupal\Component\Utility\Xss::getAdminTagList()
+   *
    */
   public static function filterAdmin($string) {
     return static::filter($string, static::$adminTags);
@@ -319,4 +337,14 @@ protected static function needsRemoval($html_tags, $elem) {
     return !isset($html_tags[strtolower($elem)]);
   }
 
+  /**
+   * Gets the list of html tags allowed by Xss::filterAdmin().
+   *
+   * @return array
+   *   The list of html tags allowed by filterAdmin().
+   */
+  public static function getAdminTagList() {
+    return static::$adminTags;
+  }
+
 }

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

@@ -8,6 +8,7 @@
 namespace Drupal\Core\Render\Element;
 
 use Drupal\Component\Utility\SafeMarkup;
+use Drupal\Component\Utility\Xss;
 use Drupal\Core\Template\Attribute;
 
 /**
@@ -50,7 +51,7 @@ public function getInfo() {
    * pre-render callback being a #markup element, it is not passed through
    * \Drupal\Component\Utility\Xss::filterAdmin(). This is because it is marked
    * safe here, which causes
-   * \Drupal\Component\Utility\SafeMarkup::checkAdminXss() to regard it as safe
+   * \Drupal\Core\Render\Renderer::xssFilterAdminIfUnsafe() to regard it as safe
    * and bypass the call to \Drupal\Component\Utility\Xss::filterAdmin().
    *
    * @param array $element
@@ -161,7 +162,7 @@ public static function preRenderConditionalComments($element) {
     }
     else {
       // The IE expression might contain some user input data.
-      $expression = SafeMarkup::checkAdminXss($browsers['IE']);
+      $expression = Xss::filterAdmin($browsers['IE']);
     }
 
     // If the #prefix and #suffix properties are used, wrap them with
@@ -173,8 +174,8 @@ public static function preRenderConditionalComments($element) {
 
     // 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']) : '';
+    $prefix = isset($elements['#prefix']) ? Xss::filterAdmin($elements['#prefix']) : '';
+    $suffix = isset($elements['#suffix']) ? Xss::filterAdmin($elements['#suffix']) : '';
 
     // Now calling SafeMarkup::set is safe, because we ensured the
     // data coming in was at least admin escaped.

core/lib/Drupal/Core/Render/Renderer.php

@@ -393,7 +393,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
     if (isset($elements['#markup'])) {
       // @todo Decide how to support non-HTML in the render API in
       //   https://www.drupal.org/node/2501313.
-      $elements['#markup'] = SafeMarkup::checkAdminXss($elements['#markup']);
+      $elements['#markup'] = $this->xssFilterAdminIfUnsafe($elements['#markup']);
     }
 
     // Assume that if #theme is set it represents an implemented hook.
@@ -407,7 +407,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
       );
       foreach ($markup_keys as $key) {
         if (!empty($elements[$key]) && is_scalar($elements[$key])) {
-          $elements[$key] = SafeMarkup::checkAdminXss($elements[$key]);
+          $elements[$key] = $this->xssFilterAdminIfUnsafe($elements[$key]);
         }
       }
     }
@@ -493,8 +493,8 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
     // with how render cached output gets stored. This ensures that placeholder
     // replacement logic gets 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']) ? SafeMarkup::checkAdminXss($elements['#prefix']) : '';
-    $suffix = isset($elements['#suffix']) ? SafeMarkup::checkAdminXss($elements['#suffix']) : '';
+    $prefix = isset($elements['#prefix']) ? $this->xssFilterAdminIfUnsafe($elements['#prefix']) : '';
+    $suffix = isset($elements['#suffix']) ? $this->xssFilterAdminIfUnsafe($elements['#suffix']) : '';
 
     $elements['#markup'] = $prefix . $elements['#children'] . $suffix;
 
@@ -651,4 +651,23 @@ public function addCacheableDependency(array &$elements, $dependency) {
     $meta_a->merge($meta_b)->applyTo($elements);
   }
 
+  /**
+   * Applies a very permissive XSS/HTML filter for admin-only use.
+   *
+   * Note: This method only filters if $string is not marked safe already. This
+   * ensures that HTML intended for display is not filtered.
+   *
+   * @param string $string
+   *   A string.
+   *
+   * @return string
+   *   The escaped string. If SafeMarkup::isSafe($string) returns TRUE, it won't
+   *   be escaped again.
+   */
+  protected function xssFilterAdminIfUnsafe($string) {
+    // @todo https://www.drupal.org/node/2506581 replace with
+    //   SafeMarkup::isSafe() and Xss::filterAdmin().
+    return SafeMarkup::checkAdminXss($string);
+  }
+
 }

core/modules/aggregator/aggregator.module

@@ -6,7 +6,7 @@
  */
 
 use Drupal\aggregator\Entity\Feed;
-use Drupal\Component\Utility\Xss;
+use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Core\Routing\RouteMatchInterface;
 
 /**
@@ -166,7 +166,7 @@ function aggregator_cron() {
  *   The filtered content.
  */
 function aggregator_filter_xss($value) {
-  return Xss::filter($value, preg_split('/\s+|<|>/', \Drupal::config('aggregator.settings')->get('items.allowed_html'), -1, PREG_SPLIT_NO_EMPTY));
+  return SafeMarkup::xssFilter($value, preg_split('/\s+|<|>/', \Drupal::config('aggregator.settings')->get('items.allowed_html'), -1, PREG_SPLIT_NO_EMPTY));
 }
 
 /**

core/modules/aggregator/src/Controller/AggregatorController.php

@@ -7,7 +7,7 @@
 
 namespace Drupal\aggregator\Controller;
 
-use Drupal\Component\Utility\Xss;
+use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Core\Controller\ControllerBase;
 use Drupal\Core\Datetime\DateFormatter;
 use Drupal\aggregator\FeedInterface;
@@ -187,7 +187,7 @@ public function pageLast() {
    *   The feed label.
    */
   public function feedTitle(FeedInterface $aggregator_feed) {
-    return Xss::filter($aggregator_feed->label());
+    return SafeMarkup::xssFilter($aggregator_feed->label());
   }
 
 }

core/modules/block_content/block_content.pages.inc

@@ -5,7 +5,6 @@
  * Provides page callbacks for custom blocks.
  */
 
-use Drupal\Component\Utility\Xss;
 use Drupal\Core\Url;
 use Drupal\block_content\Entity\BlockContentType;
 use Drupal\block_content\Entity\BlockContent;
@@ -28,7 +27,9 @@ function template_preprocess_block_content_add_list(&$variables) {
   foreach ($variables['content'] as $type) {
     $variables['types'][$type->id()] = array(
       'link' => \Drupal::l($type->label(), new Url('block_content.add_form', array('block_content_type' => $type->id()), array('query' => $query))),
-      'description' => Xss::filterAdmin($type->getDescription()),
+      'description' => array(
+        '#markup' => $type->getDescription(),
+      ),
       'title' => $type->label(),
       'localized_options' => array(
         'query' => $query,

core/modules/block_content/src/BlockContentTypeListBuilder.php

@@ -7,7 +7,6 @@
 
 namespace Drupal\block_content;
 
-use Drupal\Component\Utility\Xss;
 use Drupal\Core\Config\Entity\ConfigEntityListBuilder;
 use Drupal\Core\Entity\EntityInterface;
 
@@ -45,7 +44,7 @@ public function buildHeader() {
    */
   public function buildRow(EntityInterface $entity) {
     $row['type'] = $entity->link();
-    $row['description'] = Xss::filterAdmin($entity->getDescription());
+    $row['description']['data']['#markup'] = $entity->getDescription();
     return $row + parent::buildRow($entity);
   }
 

core/modules/comment/src/CommentTypeListBuilder.php

@@ -46,7 +46,7 @@ public function buildHeader() {
    */
   public function buildRow(EntityInterface $entity) {
     $row['type'] = SafeMarkup::checkPlain($entity->label());
-    $row['description'] = Xss::filterAdmin($entity->getDescription());
+    $row['description']['data'] = ['#markup' => $entity->getDescription()];
     return $row + parent::buildRow($entity);
   }
 

core/modules/dblog/src/Controller/DbLogController.php

@@ -207,7 +207,7 @@ public function overview() {
           $this->dateFormatter->format($dblog->timestamp, 'short'),
           $message,
           array('data' => $username),
-          Xss::filter($dblog->link),
+          SafeMarkup::xssFilter($dblog->link),
         ),
         // Attributes for table row.
         'class' => array(Html::getClass('dblog-' . $dblog->type), $classes[$dblog->severity]),
@@ -285,7 +285,7 @@ public function eventDetails($event_id) {
         ),
         array(
           array('data' => $this->t('Operations'), 'header' => TRUE),
-          SafeMarkup::checkAdminXss($dblog->link),
+          array('data' => array('#markup' => $dblog->link)),
         ),
       );
       $build['dblog_table'] = array(

core/modules/filter/filter.module

@@ -353,7 +353,10 @@ function _filter_tips($format_id, $long = FALSE) {
       if ($filter->status) {
         $tip = $filter->tips($long);
         if (isset($tip)) {
-          $tips[$format->label()][$name] = array('tip' => $tip, 'id' => $name);
+          $tips[$format->label()][$name] = array(
+            'tip' => array('#markup' => $tip),
+            'id' => $name,
+          );
         }
       }
     }
@@ -430,7 +433,6 @@ function template_preprocess_filter_tips(&$variables) {
   foreach ($variables['tips'] as $name => $tiplist) {
     foreach ($tiplist as $tip_key => $tip) {
       $tiplist[$tip_key]['attributes'] = new Attribute();
-      $tiplist[$tip_key]['tip'] = Xss::filterAdmin($tiplist[$tip_key]['tip']);
     }
 
     $variables['tips'][$name] = array(

core/modules/filter/src/Plugin/Filter/FilterCaption.php

@@ -45,7 +45,7 @@ public function process($text, $langcode) {
         // Sanitize caption: decode HTML encoding, limit allowed HTML tags; only
         // allow inline tags that are allowed by default, plus <br>.
         $caption = Html::decodeEntities($caption);
-        $caption = Xss::filter($caption, array('a', 'em', 'strong', 'cite', 'code', 'br'));
+        $caption = SafeMarkup::xssFilter($caption, array('a', 'em', 'strong', 'cite', 'code', 'br'));
 
         // The caption must be non-empty.
         if (Unicode::strlen($caption) === 0) {

core/modules/forum/forum.module

@@ -540,7 +540,7 @@ function template_preprocess_forum_list(&$variables) {
   $row = 0;
   // Sanitize each forum so that the template can safely print the data.
   foreach ($variables['forums'] as $id => $forum) {
-    $variables['forums'][$id]->description = Xss::filterAdmin($forum->description->value);
+    $variables['forums'][$id]->description = array('#markup' => $forum->description->value);
     $variables['forums'][$id]->link = forum_uri($forum);
     $variables['forums'][$id]->name = SafeMarkup::checkPlain($forum->label());
     $variables['forums'][$id]->is_container = !empty($forum->forum_container->value);

core/modules/language/src/Form/NegotiationConfigureForm.php

@@ -301,7 +301,7 @@ protected function configureFormTable(array &$form, $type)  {
           $table_form['enabled'][$method_id]['#attributes'] = array('disabled' => 'disabled');
         }
 
-        $table_form['description'][$method_id] = array('#markup' => Xss::filterAdmin($method['description']));
+        $table_form['description'][$method_id] = array('#markup' => $method['description']);
 
         $config_op = array();
         if (isset($method['config_route_name'])) {

core/modules/menu_ui/src/Controller/MenuController.php

@@ -7,7 +7,7 @@
 
 namespace Drupal\menu_ui\Controller;
 
-use Drupal\Component\Utility\Xss;
+use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Core\Controller\ControllerBase;
 use Drupal\Core\Menu\MenuParentFormSelectorInterface;
 use Drupal\system\MenuInterface;
@@ -77,7 +77,7 @@ public function getParentOptions(Request $request) {
    *   The menu label.
    */
   public function menuTitle(MenuInterface $menu) {
-    return Xss::filter($menu->label());
+    return SafeMarkup::xssFilter($menu->label());
   }
 
 }

core/modules/menu_ui/src/MenuListBuilder.php

@@ -39,7 +39,7 @@ public function buildRow(EntityInterface $entity) {
       'data' => $this->getLabel($entity),
       'class' => array('menu-label'),
     );
-    $row['description'] = Xss::filterAdmin($entity->getDescription());
+    $row['description']['data'] = ['#markup' => $entity->getDescription()];
     return $row + parent::buildRow($entity);
   }
 

core/modules/node/node.module

@@ -508,7 +508,9 @@ function template_preprocess_node_add_list(&$variables) {
       $variables['types'][$type->id()] = array(
         'type' => $type->id(),
         'add_link' => \Drupal::l($type->label(), new Url('node.add', array('node_type' => $type->id()))),
-        'description' => Xss::filterAdmin($type->getDescription()),
+        'description' => array(
+          '#markup' => $type->getDescription(),
+        ),
       );
     }
   }

core/modules/node/src/Controller/NodeController.php

@@ -8,7 +8,6 @@
 namespace Drupal\node\Controller;
 
 use Drupal\Component\Utility\SafeMarkup;
-use Drupal\Component\Utility\Xss;
 use Drupal\Core\Controller\ControllerBase;
 use Drupal\Core\Datetime\DateFormatter;
 use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
@@ -195,7 +194,7 @@ public function revisionOverview(NodeInterface $node) {
           '#context' => [
             'date' => $link,
             'username' => $this->renderer->renderPlain($username),
-            'message' => Xss::filter($revision->revision_log->value),
+            'message' => SafeMarkup::xssFilter($revision->revision_log->value),
           ],
         ],
       ];

core/modules/node/src/NodeTypeListBuilder.php

@@ -10,7 +10,6 @@
 use Drupal\Core\Config\Entity\ConfigEntityListBuilder;
 use Drupal\Core\Url;
 use Drupal\Core\Entity\EntityInterface;
-use Drupal\Component\Utility\Xss;
 
 /**
  * Defines a class to build a listing of node type entities.
@@ -39,7 +38,7 @@ public function buildRow(EntityInterface $entity) {
       'data' => $this->getLabel($entity),
       'class' => array('menu-label'),
     );
-    $row['description'] = Xss::filterAdmin($entity->getDescription());
+    $row['description']['data'] = ['#markup' => $entity->getDescription()];
     return $row + parent::buildRow($entity);
   }