diff --git a/core/lib/Drupal/Core/Template/TwigExtension.php b/core/lib/Drupal/Core/Template/TwigExtension.php index 99e807a..4cec15b 100644 --- a/core/lib/Drupal/Core/Template/TwigExtension.php +++ b/core/lib/Drupal/Core/Template/TwigExtension.php @@ -136,6 +136,7 @@ public function getFilters() { // Replace twig's escape filter with our own. new \Twig_SimpleFilter('drupal_escape', [$this, 'escapeFilter'], array('needs_environment' => true, 'is_safe_callback' => 'twig_escape_filter_is_safe')), + new \Twig_SimpleFilter('drupal_escape_placeholder', [$this, 'escapePlaceholder'], array('needs_environment' => true, 'is_safe_callback' => 'twig_escape_filter_is_safe')), // Implements safe joining. // @todo Make that the default for |join? Upstream issue: @@ -351,6 +352,28 @@ public function attachLibrary($library) { } /** + * Provides a placeholder wrapper around ::escapeFilter. + * + * @param \Twig_Environment $env + * A Twig_Environment instance. + * @param mixed $arg + * The value to be escaped. + * @param string $strategy + * The escaping strategy. Defaults to 'html'. + * @param string $charset + * The charset. + * @param bool $autoescape + * Whether the function is called by the auto-escaping feature (TRUE) or by + * the developer (FALSE). + * + * @return string|null + * The escaped, rendered output, or NULL if there is no valid output. + */ + public function escapePlaceholder(\Twig_Environment $env, $arg, $strategy = 'html', $charset = NULL, $autoescape = FALSE) { + return '' . $this->escapeFilter($env, $arg, $strategy, $charset, $autoescape) . ''; + } + + /** * Overrides twig_escape_filter(). * * Replacement function for Twig's escape filter. diff --git a/core/lib/Drupal/Core/Template/TwigNodeVisitor.php b/core/lib/Drupal/Core/Template/TwigNodeVisitor.php index a08ec2d..585013f 100644 --- a/core/lib/Drupal/Core/Template/TwigNodeVisitor.php +++ b/core/lib/Drupal/Core/Template/TwigNodeVisitor.php @@ -19,9 +19,19 @@ class TwigNodeVisitor implements \Twig_NodeVisitorInterface { /** + * Defines whether we are in a trans node. + * + * @var bool + */ + protected $inTransNode = FALSE; + + /** * {@inheritdoc} */ function enterNode(\Twig_NodeInterface $node, \Twig_Environment $env) { + if ($node instanceof TwigNodeTrans) { + $this->inTransNode = TRUE; + } return $node; } @@ -29,6 +39,10 @@ function enterNode(\Twig_NodeInterface $node, \Twig_Environment $env) { * {@inheritdoc} */ function leaveNode(\Twig_NodeInterface $node, \Twig_Environment $env) { + if ($node instanceof TwigNodeTrans) { + $this->inTransNode = FALSE; + } + // We use this to inject a call to render_var -> TwigExtension->renderVar() // before anything is printed. if ($node instanceof \Twig_Node_Print) { @@ -47,6 +61,13 @@ function leaveNode(\Twig_NodeInterface $node, \Twig_Environment $env) { // Change the 'escape' filter to our own 'drupal_escape' filter. else if ($node instanceof \Twig_Node_Expression_Filter) { $name = $node->getNode('filter')->getAttribute('value'); + if (!$this->inTransNode && 'placeholder' === $name) { + // Use our own escape filter that is SafeMarkup aware. + $node->getNode('filter')->setAttribute('value', 'drupal_escape_placeholder'); + + // Store that we have a filter active already that knows how to deal with render arrays. + $this->skipRenderVarFunction = TRUE; + } if ('escape' == $name || 'e' == $name) { // Use our own escape filter that is SafeMarkup aware. $node->getNode('filter')->setAttribute('value', 'drupal_escape'); diff --git a/core/modules/system/src/Tests/Theme/TwigTransTest.php b/core/modules/system/src/Tests/Theme/TwigTransTest.php index 08057dc..2a49f73 100644 --- a/core/modules/system/src/Tests/Theme/TwigTransTest.php +++ b/core/modules/system/src/Tests/Theme/TwigTransTest.php @@ -7,7 +7,9 @@ namespace Drupal\system\Tests\Theme; +use Drupal\Component\Utility\SafeMarkup; use Drupal\Core\Language\LanguageInterface; +use Drupal\Core\Url; use Drupal\language\Entity\ConfigurableLanguage; use Drupal\simpletest\WebTestBase; @@ -80,7 +82,7 @@ protected function setUp() { /** * Test Twig "trans" tags. */ - public function testTwigTransTags() { + public function ptestTwigTransTags() { $this->drupalGet('twig-theme-test/trans', array('language' => \Drupal::languageManager()->getLanguage('xx'))); $this->assertText( @@ -160,7 +162,7 @@ public function testTwigTransTags() { /** * Test Twig "trans" debug markup. */ - public function testTwigTransDebug() { + public function ptestTwigTransDebug() { // Enable debug, rebuild the service container, and clear all caches. $parameters = $this->container->getParameter('twig.config'); $parameters['debug'] = TRUE; @@ -176,6 +178,20 @@ public function testTwigTransDebug() { } /** + * Tests rendering a placeholder outside of translate. + * + * This test ensures that the security problem described in + * https://www.drupal.org/node/2495179 doesn't exist. + */ + public function testPlaceholderOutsideOfTrans() { + $this->drupalGet(Url::fromRoute('twig_theme_test.placeholder_outside_trans')); + + $script = ''; + $this->assertNoRaw($script); + $this->assertEqual(2, substr_count($this->getRawContent(), '' . SafeMarkup::checkPlain($script)) . ''); + } + + /** * Helper function: test twig debug translation markup. * * @param bool $visible diff --git a/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php index 040b521..d49ebba 100644 --- a/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php +++ b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php @@ -31,6 +31,16 @@ public function transBlockRender() { } /** + * Controller for testing the twig placeholder filter outside of {% trans %} + */ + public function placeholderOutsideTransRender() { + return [ + '#theme' => 'twig_theme_test_placeholder_outside_trans', + '#var' => '', + ]; + } + + /** * Renders for testing url_generator functions in a Twig template. */ public function urlGeneratorRender() { diff --git a/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.placeholder_outside_trans.html.twig b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.placeholder_outside_trans.html.twig new file mode 100644 index 0000000..8bf68c3 --- /dev/null +++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.placeholder_outside_trans.html.twig @@ -0,0 +1,5 @@ +Placeholder outside trans: {{ var | placeholder }} + +{% trans %} + Placeholder inside trans: {{ var | placeholder }} +{% endtrans %} diff --git a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module index 9aaccfa..d5e098c 100644 --- a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module +++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module @@ -15,6 +15,10 @@ function twig_theme_test_theme($existing, $type, $theme, $path) { 'variables' => array(), 'template' => 'twig_theme_test.trans', ); + $items['twig_theme_test_placeholder_outside_trans'] = array( + 'variables' => array('var' => ''), + 'template' => 'twig_theme_test.placeholder_outside_trans', + ); $items['twig_namespace_test'] = array( 'variables' => array(), 'template' => 'twig_namespace_test', diff --git a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml index 4cdfeca..73fcc4a 100644 --- a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml +++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml @@ -12,6 +12,13 @@ twig_theme_test.trans: requirements: _access: 'TRUE' +twig_theme_test.placeholder_outside_trans: + path: '/twig-theme-test/placeholder_outside_trans' + defaults: + _controller: '\Drupal\twig_theme_test\TwigThemeTestController::placeholderOutsideTransRender' + requirements: + _access: 'TRUE' + twig_theme_test_url_generator: path: '/twig-theme-test/url-generator' defaults: