From 03ac04f38ae247ca88e49289fdf65634643c588e Mon Sep 17 00:00:00 2001
From: Alex Pott <alex.a.pott@googlemail.com>
Date: Sat, 26 Sep 2015 17:50:12 +0200
Subject: [PATCH] Issue #2572929 by dawehner, pwolanin, mikeker, xjm, stefan.r,
David_Rothstein, hussainweb, lauriii, Wim Leers, catch, chx, alexpott:
Document lack of auto-escape in theme functions and add a theme autoescape
helper function
---
core/includes/theme.inc | 71 ++++++++++++++
core/lib/Drupal/Core/Render/theme.api.php | 8 +-
.../Drupal/Core/Template/TwigExtension.php | 6 ++
core/modules/language/language.admin.inc | 2 +-
.../Theme/ThemeRenderAndAutoescapeTest.php | 93 +++++++++++++++++++
5 files changed, 178 insertions(+), 2 deletions(-)
create mode 100644 core/tests/Drupal/KernelTests/Core/Theme/ThemeRenderAndAutoescapeTest.php
diff --git a/core/includes/theme.inc b/core/includes/theme.inc
index 06ba27100373..4ce12c599a69 100644
--- a/core/includes/theme.inc
+++ b/core/includes/theme.inc
@@ -12,10 +12,12 @@
use Drupal\Component\Utility\Crypt;
use Drupal\Component\Utility\Html;
use Drupal\Component\Utility\SafeMarkup;
+use Drupal\Component\Utility\SafeStringInterface;
use Drupal\Component\Utility\Unicode;
use Drupal\Component\Utility\Xss;
use Drupal\Core\Config\Config;
use Drupal\Core\Config\StorageException;
+use Drupal\Core\Render\RenderableInterface;
use Drupal\Core\Template\Attribute;
use Drupal\Core\Theme\ThemeSettings;
use Drupal\Component\Utility\NestedArray;
@@ -357,6 +359,75 @@ function theme_get_setting($setting_name, $theme = NULL) {
return $cache[$theme]->get($setting_name);
}
+/**
+ * Escapes and renders variables for theme functions.
+ *
+ * This method is used in theme functions to ensure that the result is safe for
+ * output inside HTML fragments. This mimics the behavior of the auto-escape
+ * functionality in Twig.
+ *
+ * Note: This function should be kept in sync with
+ * \Drupal\Core\Template\TwigExtension::escapeFilter().
+ *
+ * @param mixed $arg
+ * The string, object, or render array to escape if needed.
+ *
+ * @return string
+ * The rendered string, safe for use in HTML. The string is not safe when used
+ * as any part of an HTML attribute name or value.
+ *
+ * @throws \Exception
+ * Thrown when an object is passed in which cannot be printed.
+ *
+ * @see \Drupal\Core\Template\TwigExtension::escapeFilter()
+ *
+ * @todo Discuss deprecating this in https://www.drupal.org/node/2575081.
+ * @todo Refactor this to keep it in sync with Twig filtering in
+ * https://www.drupal.org/node/2575065
+ */
+function theme_render_and_autoescape($arg) {
+ if ($arg instanceOf SafeStringInterface) {
+ return (string) $arg;
+ }
+ $return = NULL;
+
+ if (is_scalar($arg)) {
+ $return = (string) $arg;
+ }
+ elseif (is_object($arg)) {
+ if ($arg instanceof RenderableInterface) {
+ $arg = $arg->toRenderable();
+ }
+ elseif (method_exists($arg, '__toString')) {
+ $return = (string) $arg;
+ }
+ // You can't throw exceptions in the magic PHP __toString methods, see
+ // http://php.net/manual/en/language.oop5.magic.php#object.tostring so
+ // we also support a toString method.
+ elseif (method_exists($arg, 'toString')) {
+ $return = $arg->toString();
+ }
+ else {
+ throw new \Exception(t('Object of type "@class" cannot be printed.', array('@class' => get_class($arg))));
+ }
+ }
+
+ // We have a string or an object converted to a string: Escape it!
+ if (isset($return)) {
+ return SafeMarkup::isSafe($return, 'html') ? $return : Html::escape($return);
+ }
+
+ // This is a normal render array, which is safe by definition, with special
+ // simple cases already handled.
+
+ // Early return if this element was pre-rendered (no need to re-render).
+ if (isset($arg['#printed']) && $arg['#printed'] == TRUE && isset($arg['#markup']) && strlen($arg['#markup']) > 0) {
+ return (string) $arg['#markup'];
+ }
+ $arg['#printed'] = FALSE;
+ return (string) \Drupal::service('renderer')->render($arg);
+}
+
/**
* Converts theme settings to configuration.
*
diff --git a/core/lib/Drupal/Core/Render/theme.api.php b/core/lib/Drupal/Core/Render/theme.api.php
index 313922479dcb..bf95672290f0 100644
--- a/core/lib/Drupal/Core/Render/theme.api.php
+++ b/core/lib/Drupal/Core/Render/theme.api.php
@@ -70,7 +70,10 @@
* hook_theme() implementations can also specify that a theme hook
* implementation is a theme function, but that is uncommon. It is only used for
* special cases, for performance reasons, because rendering using theme
- * functions is somewhat faster than theme templates.
+ * functions is somewhat faster than theme templates. Note that while Twig
+ * templates will auto-escape variables, theme functions must explicitly escape
+ * any variables by using theme_render_and_autoescape(). Failure to do so is
+ * likely to result in security vulnerabilities.
*
* @section sec_overriding_theme_hooks Overriding Theme Hooks
* Themes may register new theme hooks within a hook_theme() implementation, but
@@ -93,6 +96,9 @@
* bartik_search_result() in the bartik.theme file, if the search_result hook
* implementation was a function instead of a template). Normally, copying the
* default function is again a good starting point for overriding its behavior.
+ * Again, note that theme functions (unlike templates) must explicitly escape
+ * variables using theme_render_and_autoescape() or risk security
+ * vulnerabilities.
*
* @section sec_preprocess_templates Preprocessing for Template Files
* If the theme implementation is a template file, several functions are called
diff --git a/core/lib/Drupal/Core/Template/TwigExtension.php b/core/lib/Drupal/Core/Template/TwigExtension.php
index 735a43426856..13a7d16fab91 100644
--- a/core/lib/Drupal/Core/Template/TwigExtension.php
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -350,6 +350,9 @@ public function escapePlaceholder($env, $string) {
*
* Replacement function for Twig's escape filter.
*
+ * Note: This function should be kept in sync with
+ * theme_render_and_autoescape().
+ *
* @param \Twig_Environment $env
* A Twig_Environment instance.
* @param mixed $arg
@@ -364,6 +367,9 @@ public function escapePlaceholder($env, $string) {
*
* @return string|null
* The escaped, rendered output, or NULL if there is no valid output.
+ *
+ * @todo Refactor this to keep it in sync with theme_render_and_autoescape()
+ * in https://www.drupal.org/node/2575065
*/
public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $charset = NULL, $autoescape = FALSE) {
// Check for a numeric zero int or float.
diff --git a/core/modules/language/language.admin.inc b/core/modules/language/language.admin.inc
index 3f860de03ff5..93736725c9e1 100644
--- a/core/modules/language/language.admin.inc
+++ b/core/modules/language/language.admin.inc
@@ -201,5 +201,5 @@ function template_preprocess_language_content_settings_table(&$variables) {
* @ingroup themeable
*/
function theme_language_content_settings_table($variables) {
- return '<h4>' . $variables['build']['#title'] . '</h4>' . drupal_render($variables['build']);
+ return '<h4>' . theme_render_and_autoescape($variables['build']['#title']) . '</h4>' . theme_render_and_autoescape($variables['build']);
}
diff --git a/core/tests/Drupal/KernelTests/Core/Theme/ThemeRenderAndAutoescapeTest.php b/core/tests/Drupal/KernelTests/Core/Theme/ThemeRenderAndAutoescapeTest.php
new file mode 100644
index 000000000000..f43ea1331e7b
--- /dev/null
+++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeRenderAndAutoescapeTest.php
@@ -0,0 +1,93 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\KernelTests\Core\Theme\ThemeRenderAndAutoescapeTest.
+ */
+
+namespace Drupal\KernelTests\Core\Theme;
+
+use Drupal\Component\Utility\Html;
+use Drupal\Core\Link;
+use Drupal\Core\Render\RenderContext;
+use Drupal\Core\Render\SafeString;
+use Drupal\KernelTests\KernelTestBase;
+
+/**
+ * Tests the theme_render_and_autoescape() function.
+ *
+ * @group Theme
+ */
+class ThemeRenderAndAutoescapeTest extends KernelTestBase {
+
+ /**
+ * {@inheritdoc}
+ */
+ public static $modules = ['system'];
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function setUp() {
+ parent::setUp();
+
+ $this->installSchema('system', 'router');
+ \Drupal::service('router.builder')->rebuild();
+ }
+
+ /**
+ * @dataProvider providerTestThemeRenderAndAutoescape
+ */
+ public function testThemeRenderAndAutoescape($arg, $expected) {
+ if (is_array($arg) && isset($arg['#type']) && $arg['#type'] === 'link') {
+ $arg = Link::createFromRoute($arg['#title'], $arg['#url']);
+ }
+
+ $context = new RenderContext();
+ // Use a closure here since we need to render with a render context.
+ $theme_render_and_autoescape = function () use ($arg) {
+ return theme_render_and_autoescape($arg);
+ };
+ /** @var \Drupal\Core\Render\RendererInterface $renderer */
+ $renderer = \Drupal::service('renderer');
+ $output = $renderer->executeInRenderContext($context, $theme_render_and_autoescape);
+ $this->assertEquals($expected, $output);
+ $this-> assertInternalType('string', $output);
+ }
+
+ /**
+ * Provide test examples.
+ */
+ public function providerTestThemeRenderAndAutoescape() {
+ return [
+ 'empty string unchanged' => ['', ''],
+ 'simple string unchanged' => ['ab', 'ab'],
+ 'int (scalar) cast to string' => [111, '111'],
+ 'float (scalar) cast to string' => [2.10, '2.10'],
+ '> is escaped' => ['>', '>'],
+ 'SafeString EM tag is unchanged' => [SafeString::create('<em>hi</em>'), '<em>hi</em>'],
+ 'SafeString SCRIPT tag is unchanged' => [SafeString::create('<script>alert("hi");</script>'), '<script>alert("hi");</script>'],
+ 'EM tag in string is escaped' => ['<em>hi</em>', Html::escape('<em>hi</em>')],
+ 'type link render array is rendered' => [['#type' => 'link', '#title' => 'Text', '#url' => '<none>'], '<a href="">Text</a>'],
+ 'type markup with EM tags is rendered' => [['#markup' => '<em>hi</em>'], '<em>hi</em>'],
+ 'SCRIPT tag in string is escaped' => [
+ '<script>alert(123)</script>',
+ Html::escape('<script>alert(123)</script>')
+ ],
+ 'type plain_text render array EM tag is escaped' => [['#plain_text' => '<em>hi</em>'], Html::escape('<em>hi</em>')],
+ 'type hidden render array is rendered' => [['#type' => 'hidden', '#name' => 'foo', '#value' => 'bar'], "<input type=\"hidden\" name=\"foo\" value=\"bar\" />\n"],
+ ];
+ }
+
+ /**
+ * Ensures invalid content is handled correctly.
+ *
+ * @expectedException \Exception
+ */
+ public function testThemeEscapeAndRenderNotPrintable() {
+ theme_render_and_autoescape(new NonPrintable());
+ }
+
+}
+
+class NonPrintable { }
--
GitLab