diff --git a/core/includes/theme.inc b/core/includes/theme.inc index 5d645e2..340ac77 100644 --- a/core/includes/theme.inc +++ b/core/includes/theme.inc @@ -1372,7 +1372,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'] = (theme_get_setting('features.slogan') ? SafeMarkup::checkAdminXss($site_config->get('slogan')) : ''); // An exception might be thrown. try { diff --git a/core/lib/Drupal/Component/Utility/SafeMarkup.php b/core/lib/Drupal/Component/Utility/SafeMarkup.php index 69d12fe..114a60c 100644 --- a/core/lib/Drupal/Component/Utility/SafeMarkup.php +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php @@ -82,7 +82,7 @@ public static function set($string, $strategy = 'html') { /** * Checks if a string is safe to output. * - * @param string $string + * @param string|\Drupal\Component\Utility\SafeStringInterface|\Twig_Markup $string * The content to be checked. * @param string $strategy * The escaping strategy. See self::set(). Defaults to 'html'. @@ -91,7 +91,9 @@ public static function set($string, $strategy = 'html') { * TRUE if the string has been marked secure, FALSE otherwise. */ public static function isSafe($string, $strategy = 'html') { - return isset(static::$safeStrings[(string) $string][$strategy]) || + // Do the instance of checks first to save unnecessarily casting the object + // to a string. + return $string instanceof \Twig_Markup || $string instanceOf SafeStringInterface || isset(static::$safeStrings[(string) $string][$strategy]) || isset(static::$safeStrings[(string) $string]['all']); } @@ -149,7 +151,38 @@ public static function escape($string) { * @see \Drupal\Component\Utility\Xss::filterAdmin() */ public static function checkAdminXss($string) { - return static::isSafe($string) ? $string : Xss::filterAdmin($string); + if (!static::isSafe($string)) { + $string = Xss::filterAdmin($string); + static::set($string); + } + return $string; + } + + /** + * Filters HTML to prevent cross-site-scripting (XSS) vulnerabilities. + * + * This method is preferred to \Drupal\Component\Utility\Xss::filter() when + * the result is being used directly in the rendering system. + * + * @param $string + * The string with raw HTML in it. It will be stripped of everything that + * can cause an XSS attack. + * @param array $html_tags + * An array of HTML tags. + * + * @return string + * An XSS safe version of $string, or an empty string if $string is not + * valid UTF-8. The string has been marked safe. If the string has already + * been marked safe, it won't be escaped again. + * + * @see \Drupal\Component\Utility\Xss::filter() + */ + public static function filterXss($string, $html_tags = array('a', 'em', 'strong', 'cite', 'blockquote', 'code', 'ul', 'ol', 'li', 'dl', 'dt', 'dd')) { + if (!static::isSafe($string)) { + $string = Xss::filter($string, $html_tags); + static::set($string); + } + return $string; } /** @@ -209,10 +242,12 @@ public static function checkPlain($text) { * any key in $args are replaced with the corresponding value, after * optional sanitization and formatting. The type of sanitization and * formatting depends on the first character of the key: - * - @variable: Escaped to HTML using self::escape(). Use this as the - * default choice for anything displayed on a page on the site. - * - %variable: Escaped to HTML and formatted using self::placeholder(), - * which makes the following HTML code: + * - @variable: Escaped to plain text using htmlspecialchars() unless + * self::isSafe() returns TRUE to indicate it was already escaped. Use + * this as the default choice for anything displayed in HTML on the site. + * - %variable: Escaped to plain text using htmlspecialchars() unless + * self::isSafe() returns TRUE to indicate it was already escaped. It is + * formatted with EM tags to yield HTML code like: * @code * text output here. * @endcode @@ -240,13 +275,13 @@ public static function format($string, array $args = array()) { switch ($key[0]) { case '@': // Escaped only. - $args[$key] = static::escape($value); + $args[$key] = static::isSafe($value) ? $value : htmlspecialchars($value, ENT_QUOTES, 'UTF-8'); break; case '%': default: // Escaped and placeholder. - $args[$key] = static::placeholder($value); + $args[$key] = static::doPlaceholder($value); break; case '!': @@ -268,21 +303,38 @@ public static function format($string, array $args = array()) { /** * Formats text for emphasized display in a placeholder inside a sentence. * - * Used automatically by self::format(). + * The return value is flagged as a safe markup string for rendering. * * @param string $text * The text to format (plain-text). * * @return string - * The formatted text (html). + * The escaped text formatted with EM tags to yield HTML code like: + * @code + * text output here. + * @endcode */ public static function placeholder($text) { - $string = '' . static::escape($text) . ''; + $string = static::doPlaceholder($text); static::$safeStrings[$string]['html'] = TRUE; return $string; } /** + * Formats text for emphasized display in a placeholder inside a sentence. + * + * @param string $text + * The text to format (plain-text). + * + * @return string + * The formatted text (HTML) with wrapping EM tags. + */ + protected static function doPlaceholder($text) { + $text = static::isSafe($text) ? $text : htmlspecialchars($text, ENT_QUOTES, 'UTF-8'); + return '' . $text . ''; + } + + /** * Replaces all occurrences of the search string with the replacement string. * * Functions identically to str_replace(), but marks the returned output as diff --git a/core/lib/Drupal/Component/Utility/SafeStringInterface.php b/core/lib/Drupal/Component/Utility/SafeStringInterface.php new file mode 100644 index 0000000..1414a37 --- /dev/null +++ b/core/lib/Drupal/Component/Utility/SafeStringInterface.php @@ -0,0 +1,14 @@ +]*(>|$) # a string that starts with a <, up until the > or the end of the string | # or > # just a > - )%x', $splitter, $string)); + )%x', $splitter, $string); } /** diff --git a/core/lib/Drupal/Core/Render/Renderer.php b/core/lib/Drupal/Core/Render/Renderer.php index a91b14b..7b5112b 100644 --- a/core/lib/Drupal/Core/Render/Renderer.php +++ b/core/lib/Drupal/Core/Render/Renderer.php @@ -221,10 +221,10 @@ protected function doRender(&$elements, $is_root_call = FALSE) { // to mark them as safe too. The parent markup contains the child // markup, so if the parent markup is safe, then the markup of the // individual children must be safe as well. - $elements['#markup'] = SafeMarkup::set($elements['#markup']); + $elements['#markup'] = new \Twig_Markup($elements['#markup'], 'UTF-8'); if (!empty($elements['#cache_properties'])) { foreach (Element::children($cached_element) as $key) { - SafeMarkup::set($cached_element[$key]['#markup']); + $cached_element[$key]['#markup'] = new \Twig_Markup($cached_element[$key]['#markup'], 'UTF-8'); } } // The render cache item contains all the bubbleable rendering metadata @@ -392,7 +392,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) { foreach ($children as $key) { $elements['#children'] .= $this->doRender($elements[$key]); } - $elements['#children'] = SafeMarkup::set($elements['#children']); + $elements['#children'] = new \Twig_Markup($elements['#children'], 'UTF-8'); } // If #theme is not implemented and the element has raw #markup as a @@ -403,7 +403,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) { // required. Eventually #theme_wrappers will expect both #markup and // #children to be a single string as #children. if (!$theme_is_implemented && isset($elements['#markup'])) { - $elements['#children'] = SafeMarkup::set($elements['#markup'] . $elements['#children']); + $elements['#children'] = new \Twig_Markup($elements['#markup'] . $elements['#children'], 'UTF-8'); } // Let the theme functions in #theme_wrappers add markup around the rendered @@ -490,7 +490,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) { $this->bubbleStack(); $elements['#printed'] = TRUE; - $elements['#markup'] = SafeMarkup::set($elements['#markup']); + $elements['#markup'] = new \Twig_Markup($elements['#markup'], 'UTF-8'); return $elements['#markup']; } diff --git a/core/lib/Drupal/Core/Template/Attribute.php b/core/lib/Drupal/Core/Template/Attribute.php index ad4e1b1..b8c0590 100644 --- a/core/lib/Drupal/Core/Template/Attribute.php +++ b/core/lib/Drupal/Core/Template/Attribute.php @@ -7,7 +7,7 @@ namespace Drupal\Core\Template; -use Drupal\Component\Utility\SafeMarkup; +use Drupal\Component\Utility\SafeStringInterface; /** * Collects, sanitizes, and renders HTML attributes. @@ -42,8 +42,7 @@ * The attribute keys and values are automatically sanitized for output with * htmlspecialchars() and the entire attribute string is marked safe for output. */ -class Attribute implements \ArrayAccess, \IteratorAggregate { - +class Attribute implements \ArrayAccess, \IteratorAggregate, SafeStringInterface { /** * Stores the attribute data. * @@ -259,10 +258,7 @@ public function __toString() { $return .= ' ' . $rendered; } } - // The implementations of AttributeValueBase::render() call - // htmlspecialchars() on the attribute name and value so we are confident - // that the return value can be set as safe. - return SafeMarkup::set($return); + return $return; } /** diff --git a/core/lib/Drupal/Core/Template/TwigExtension.php b/core/lib/Drupal/Core/Template/TwigExtension.php index 5a73640..4dfbdb6 100644 --- a/core/lib/Drupal/Core/Template/TwigExtension.php +++ b/core/lib/Drupal/Core/Template/TwigExtension.php @@ -13,6 +13,7 @@ namespace Drupal\Core\Template; use Drupal\Component\Utility\SafeMarkup; +use Drupal\Component\Utility\SafeStringInterface; use Drupal\Core\Render\RendererInterface; use Drupal\Core\Routing\UrlGeneratorInterface; use Drupal\Core\Theme\ThemeManagerInterface; @@ -397,7 +398,7 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $ } // Keep Twig_Markup objects intact to support autoescaping. - if ($autoescape && $arg instanceOf \Twig_Markup) { + if ($autoescape && ($arg instanceOf \Twig_Markup || $arg instanceOf SafeStringInterface)) { return $arg; } @@ -423,13 +424,16 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $ // We have a string or an object converted to a string: Autoescape it! if (isset($return)) { - if ($autoescape && SafeMarkup::isSafe($return, $strategy)) { + if (SafeMarkup::isSafe($return, $strategy)) { return $return; } // Drupal only supports the HTML escaping strategy, so provide a // fallback for other strategies. if ($strategy == 'html') { - return SafeMarkup::checkPlain($return); + // Call htmlspecialchars() directly as there is no point in marking + // something safe at this point. + // @see \Drupal\Component\Utility\SafeMarkup::checkPlain() + return htmlspecialchars($return, ENT_QUOTES, 'UTF-8'); } return twig_escape_filter($env, $return, $strategy, $charset, $autoescape); } diff --git a/core/lib/Drupal/Core/Theme/ThemeManager.php b/core/lib/Drupal/Core/Theme/ThemeManager.php index 6850fcb..c991cf0 100644 --- a/core/lib/Drupal/Core/Theme/ThemeManager.php +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php @@ -385,7 +385,7 @@ protected function theme($hook, $variables = array()) { $output = $render_function($template_file, $variables); } - return (string) $output; + return ($output instanceof \Twig_Markup) ? $output : (string) $output; } /** diff --git a/core/modules/content_translation/src/ContentTranslationHandler.php b/core/modules/content_translation/src/ContentTranslationHandler.php index 28529eb..7086cc8 100644 --- a/core/modules/content_translation/src/ContentTranslationHandler.php +++ b/core/modules/content_translation/src/ContentTranslationHandler.php @@ -290,8 +290,8 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En $title = $this->entityFormTitle($entity); // When editing the original values display just the entity label. if ($form_langcode != $entity_langcode) { - $t_args = array('%language' => $languages[$form_langcode]->getName(), '%title' => $entity->label(), '!title' => $title); - $title = empty($source_langcode) ? t('!title [%language translation]', $t_args) : t('Create %language translation of %title', $t_args); + $t_args = array('%language' => $languages[$form_langcode]->getName(), '%title' => $entity->label(), '@title' => $title); + $title = empty($source_langcode) ? t('@title [%language translation]', $t_args) : t('Create %language translation of %title', $t_args); } $form['#title'] = $title; } diff --git a/core/modules/editor/tests/src/Unit/EditorXssFilter/StandardTest.php b/core/modules/editor/tests/src/Unit/EditorXssFilter/StandardTest.php index 99daa3b..c7f8e1d 100644 --- a/core/modules/editor/tests/src/Unit/EditorXssFilter/StandardTest.php +++ b/core/modules/editor/tests/src/Unit/EditorXssFilter/StandardTest.php @@ -105,7 +105,7 @@ public function providerTestFilterXss() { // Default SRC tag by leaving it empty. // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Default_SRC_tag_by_leaving_it_empty - $data[] = array('', ''); + $data[] = array('', ''); // Default SRC tag by leaving it out entirely. // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Default_SRC_tag_by_leaving_it_out_entirely diff --git a/core/modules/filter/filter.module b/core/modules/filter/filter.module index 400d6cc..96d6aa3 100644 --- a/core/modules/filter/filter.module +++ b/core/modules/filter/filter.module @@ -430,7 +430,7 @@ 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']); + $tiplist[$tip_key]['tip'] = SafeMarkup::checkAdminXss($tiplist[$tip_key]['tip']); } $variables['tips'][$name] = array( diff --git a/core/modules/filter/src/Plugin/Filter/FilterCaption.php b/core/modules/filter/src/Plugin/Filter/FilterCaption.php index 32977ec..acd4e09 100644 --- a/core/modules/filter/src/Plugin/Filter/FilterCaption.php +++ b/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
. $caption = Html::decodeEntities($caption); - $caption = Xss::filter($caption, array('a', 'em', 'strong', 'cite', 'code', 'br')); + $caption = SafeMarkup::filterXss($caption, array('a', 'em', 'strong', 'cite', 'code', 'br')); // The caption must be non-empty. if (Unicode::strlen($caption) === 0) { diff --git a/core/modules/search/search.module b/core/modules/search/search.module index b4c4126..a888b1b 100644 --- a/core/modules/search/search.module +++ b/core/modules/search/search.module @@ -8,7 +8,6 @@ use Drupal\Component\Utility\SafeMarkup; use Drupal\Component\Utility\Html; use Drupal\Component\Utility\Unicode; -use Drupal\Component\Utility\Xss; use Drupal\Core\Cache\Cache; use Drupal\Core\Form\FormStateInterface; use Drupal\Core\Routing\RouteMatchInterface; @@ -768,7 +767,7 @@ function search_excerpt($keys, $text, $langcode = NULL) { // Highlight keywords. Must be done at once to prevent conflicts ('strong' // and ''). $text = trim(preg_replace('/' . $boundary . '(?:' . implode('|', $keys) . ')' . $boundary . '/iu', '\0', ' ' . $text . ' ')); - return Xss::filter($text, ['strong']); + return SafeMarkup::filterXss($text, ['strong']); } /** diff --git a/core/modules/simpletest/src/AssertContentTrait.php b/core/modules/simpletest/src/AssertContentTrait.php index 240aea9..8232cb0 100644 --- a/core/modules/simpletest/src/AssertContentTrait.php +++ b/core/modules/simpletest/src/AssertContentTrait.php @@ -808,7 +808,7 @@ protected function assertNoTitle($title, $message = '', $group = 'Other') { * TRUE on pass, FALSE on fail. */ protected function assertThemeOutput($callback, array $variables = array(), $expected = '', $message = '', $group = 'Other') { - $output = \Drupal::theme()->render($callback, $variables); + $output = (string) \Drupal::theme()->render($callback, $variables); $this->verbose( '
' . 'Result:' . '
' . SafeMarkup::checkPlain(var_export($output, TRUE)) . '
' . '
' . 'Expected:' . '
' . SafeMarkup::checkPlain(var_export($expected, TRUE)) . '
' diff --git a/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php index c5ebaa5..f619bd1 100644 --- a/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php +++ b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php @@ -7,6 +7,7 @@ namespace Drupal\system\Plugin\Block; +use Drupal\Component\Utility\SafeMarkup; use Drupal\Core\Block\BlockBase; use Drupal\Core\Cache\Cache; use Drupal\Core\Config\ConfigFactoryInterface; @@ -173,7 +174,7 @@ public function build() { ); $build['site_slogan'] = array( - '#markup' => Xss::filterAdmin($site_config->get('slogan')), + '#markup' => SafeMarkup::checkAdminXss($site_config->get('slogan')), '#access' => $this->configuration['use_site_slogan'], ); diff --git a/core/modules/system/templates/dropbutton-wrapper.html.twig b/core/modules/system/templates/dropbutton-wrapper.html.twig index ca0ff7e..d4c8d90 100644 --- a/core/modules/system/templates/dropbutton-wrapper.html.twig +++ b/core/modules/system/templates/dropbutton-wrapper.html.twig @@ -12,7 +12,7 @@ * @ingroup themeable */ #} -{% if children %} +{% if children|length %} {% spaceless %}
diff --git a/core/themes/engines/twig/twig.engine b/core/themes/engines/twig/twig.engine index 9f8bc4a..b7df114 100644 --- a/core/themes/engines/twig/twig.engine +++ b/core/themes/engines/twig/twig.engine @@ -103,7 +103,8 @@ function twig_render_template($template_file, array $variables) { $output['debug_info'] .= "\n\n"; $output['debug_suffix'] .= "\n\n\n"; } - return SafeMarkup::set(implode('', $output)); + // This output has already been rendered and is therefore considered safe. + return new \Twig_Markup(implode('', $output), 'UTF-8'); } /**