diff --git a/core/lib/Drupal/Component/Utility/SafeMarkup.php b/core/lib/Drupal/Component/Utility/SafeMarkup.php index 172a323..ee540cc 100644 --- a/core/lib/Drupal/Component/Utility/SafeMarkup.php +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php @@ -282,4 +282,51 @@ public static function placeholder($text) { return $string; } + /** + * Replaces all occurrences of the search string with the replacement string. + * + * Functions identically to str_replace(), but marks the returned output as + * safe if all the inputs and the subject have also been marked as safe. + * + * @param string|array $search + * The value being searched for. An array may be used to designate multiple + * values to search for. + * @param string|array $replace + * The replacement value that replaces found search values. An array may be + * used to designate multiple replacements. + * @param string $subject + * The string or array being searched and replaced on. + * + * @return string + * The passed subject with replaced values. + */ + public static function replace($search, $replace, $subject) { + $output = str_replace($search, $replace, $subject); + + // If any replacement is unsafe, then the output is also unsafe, so just + // return the output. + if (!is_array($replace)) { + if (!SafeMarkup::isSafe($replace)) { + return $output; + } + } + else { + foreach ($replace as $replacement) { + if (!SafeMarkup::isSafe($replacement)) { + return $output; + } + } + } + + if (!SafeMarkup::isSafe($subject)) { + return $output; + } + else { + // If we have reached this point, then all replacements were safe, and + // therefore if the subject was also safe, then the entire output is also + // safe, and should be marked as such. + return SafeMarkup::set($output); + } + } + } diff --git a/core/lib/Drupal/Core/Render/Element/HtmlTag.php b/core/lib/Drupal/Core/Render/Element/HtmlTag.php index 553767f..e25f8c7 100644 --- a/core/lib/Drupal/Core/Render/Element/HtmlTag.php +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php @@ -94,7 +94,7 @@ public static function preRenderHtmlTag($element) { $markup = SafeMarkup::set($markup); } if (!empty($element['#noscript'])) { - $element['#markup'] = ''; + $element['#markup'] = SafeMarkup::format('', ['!markup' => $markup]); } else { $element['#markup'] = $markup; diff --git a/core/lib/Drupal/Core/Render/Renderer.php b/core/lib/Drupal/Core/Render/Renderer.php index 827330f..0bb6ba9 100644 --- a/core/lib/Drupal/Core/Render/Renderer.php +++ b/core/lib/Drupal/Core/Render/Renderer.php @@ -253,9 +253,8 @@ protected function doRender(&$elements, $is_root_call = FALSE) { $elements['#children'] = ''; } - // @todo Simplify after https://drupal.org/node/2273925 if (isset($elements['#markup'])) { - $elements['#markup'] = SafeMarkup::set($elements['#markup']); + $elements['#markup'] = SafeMarkup::checkAdminXss($elements['#markup']); } // Assume that if #theme is set it represents an implemented hook. @@ -553,7 +552,10 @@ public function generateCachePlaceholder($callback, array &$context) { 'token' => Crypt::randomBytesBase64(55), ]; - return ''; + return SafeMarkup::format('', [ + '@callback' => $callback, + '@token' => $context['token'], + ]); } } diff --git a/core/modules/contextual/src/Element/ContextualLinksPlaceholder.php b/core/modules/contextual/src/Element/ContextualLinksPlaceholder.php index d10078b..a0c2739 100644 --- a/core/modules/contextual/src/Element/ContextualLinksPlaceholder.php +++ b/core/modules/contextual/src/Element/ContextualLinksPlaceholder.php @@ -9,6 +9,7 @@ use Drupal\Core\Template\Attribute; use Drupal\Core\Render\Element\RenderElement; +use Drupal\Component\Utility\SafeMarkup; /** * Provides a contextual_links_placeholder element. @@ -47,7 +48,10 @@ public function getInfo() { * @see _contextual_links_to_id() */ public static function preRenderPlaceholder(array $element) { - $element['#markup'] = ' $element['#id'])) . '>'; + // This markup is safe because the arguments will always be instances of + // \Drupal\Core\Template\AttributeString, which is passed through + // \Drupal\Component\Utility\SafeMarkup::checkPlain() before being output. + $element['#markup'] = SafeMarkup::set(' $element['#id'])) . '>'); return $element; } diff --git a/core/modules/filter/src/Element/ProcessedText.php b/core/modules/filter/src/Element/ProcessedText.php index d007b5f..51c8b83 100644 --- a/core/modules/filter/src/Element/ProcessedText.php +++ b/core/modules/filter/src/Element/ProcessedText.php @@ -8,6 +8,7 @@ namespace Drupal\filter\Element; use Drupal\Component\Utility\NestedArray; +use Drupal\Component\Utility\SafeMarkup; use Drupal\Core\Cache\Cache; use Drupal\Core\Render\BubbleableMetadata; use Drupal\Core\Render\Element\RenderElement; @@ -118,9 +119,11 @@ public static function preRenderText($element) { } } - // Filtering done, store in #markup, set the updated bubbleable rendering - // metadata, and set the text format's cache tag. - $element['#markup'] = $text; + // Filtering and sanitizing have been done in + // \Drupal\filter\Plugin\FilterInterface. Store the content in #markup, + // set the updated bubbleable rendering metadata, and set the text format's + // cache tag. + $element['#markup'] = SafeMarkup::set($text); $metadata->applyTo($element); $element['#cache']['tags'] = Cache::mergeTags($element['#cache']['tags'], $format->getCacheTags()); diff --git a/core/modules/rest/src/Plugin/views/display/RestExport.php b/core/modules/rest/src/Plugin/views/display/RestExport.php index 2efdbc7..18d0341 100644 --- a/core/modules/rest/src/Plugin/views/display/RestExport.php +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php @@ -284,7 +284,7 @@ public function execute() { $header = []; $header['Content-type'] = $this->getMimeType(); - $response = new CacheableResponse($this->renderer->renderRoot($output), 200); + $response = new CacheableResponse($output['#markup'], 200); $cache_metadata = CacheableMetadata::createFromRenderArray($output); $response->addCacheableDependency($cache_metadata); diff --git a/core/modules/rest/src/Plugin/views/style/Serializer.php b/core/modules/rest/src/Plugin/views/style/Serializer.php index ba3ca6f..33f2fc7 100644 --- a/core/modules/rest/src/Plugin/views/style/Serializer.php +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php @@ -7,7 +7,9 @@ namespace Drupal\rest\Plugin\views\style; +use Drupal\Component\Utility\SafeMarkup; use Drupal\Core\Form\FormStateInterface; +use Drupal\rest\Plugin\views\row\DataFieldRow; use Drupal\views\ViewExecutable; use Drupal\views\Plugin\views\display\DisplayPluginBase; use Drupal\views\Plugin\views\style\StylePluginBase; diff --git a/core/modules/rest/src/Tests/Views/StyleSerializerTest.php b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php index 0d16fcb..27d5e30 100644 --- a/core/modules/rest/src/Tests/Views/StyleSerializerTest.php +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php @@ -390,6 +390,16 @@ public function testFieldapiField() { $result = $this->drupalGetJSON('test/serialize/node-field'); $this->assertEqual($result[0]['nid'], $node->id()); $this->assertEqual($result[0]['body'], $node->body->processed); - } + // Make sure that serialized fields are not exposed to XSS. + $node = $this->drupalCreateNode(); + $node->body = array( + 'value' => '' . $this->randomMachineName(32), + 'format' => filter_default_format(), + ); + $node->save(); + $result = $this->drupalGetJSON('test/serialize/node-field'); + $this->assertEqual($result[1]['nid'], $node->id()); + $this->assertTrue(strpos($this->getRawContent(), "randomString(); - $footer_string = $this->randomString(); - $empty_string = $this->randomString(); + $header_string = '

' . $this->randomMachineName() . '

'; + $footer_string = '

' . $this->randomMachineName() . '

'; + $empty_string = '

' . $this->randomMachineName() . '

'; $view->header['test_example']->options['string'] = $header_string; $view->header['test_example']->options['empty'] = TRUE; @@ -104,12 +105,13 @@ public function testRenderArea() { $view->empty['test_example']->options['string'] = $empty_string; - // Check whether the strings exists in the output. + // Check whether the strings exists in the output and they are being + // sanitized. $output = $view->preview(); $output = drupal_render($output); - $this->assertTrue(strpos($output, $header_string) !== FALSE); - $this->assertTrue(strpos($output, $footer_string) !== FALSE); - $this->assertTrue(strpos($output, $empty_string) !== FALSE); + $this->assertTrue(strpos($output, Xss::filterAdmin($header_string)) !== FALSE, 'Views header exists in the output and is sanitized'); + $this->assertTrue(strpos($output, Xss::filterAdmin($footer_string)) !== FALSE, 'Views footer exists in the output and is sanitized'); + $this->assertTrue(strpos($output, Xss::filterAdmin($empty_string)) !== FALSE, 'Views empty exists in the output and is sanitized'); } /** diff --git a/core/modules/views/views.module b/core/modules/views/views.module index 6fc41fa..bc89c7d 100644 --- a/core/modules/views/views.module +++ b/core/modules/views/views.module @@ -683,7 +683,7 @@ function views_pre_render_views_form_views_form($element) { } // Apply substitutions to the rendered output. - $element['output'] = array('#markup' => str_replace($search, $replace, drupal_render($element['output']))); + $element['output'] = array('#markup' => SafeMarkup::replace($search, $replace, drupal_render($element['output']))); // Sort, render and add remaining form fields. $children = Element::children($element, TRUE); diff --git a/core/modules/views_ui/views_ui.theme.inc b/core/modules/views_ui/views_ui.theme.inc index f2f2b67..4aafde4 100644 --- a/core/modules/views_ui/views_ui.theme.inc +++ b/core/modules/views_ui/views_ui.theme.inc @@ -147,21 +147,35 @@ function theme_views_ui_build_group_filter_form($variables) { foreach (Element::children($form['group_items']) as $group_id) { $form['group_items'][$group_id]['value']['#title'] = ''; + $default = array( + $form['default_group'][$group_id], + $form['default_group_multiple'][$group_id], + ); + $link = [ + '#type' => 'link', + '#url' => Url::fromRoute('', [], [ + 'attributes' => [ + 'id' => 'views-remove-link-' . $group_id, + 'class' => [ + 'views-hidden', + 'views-button-remove', + 'views-groups-remove-link', + 'views-remove-link', + ], + 'alt' => t('Remove this item'), + 'title' => t('Remove this item'), + ], + ]), + '#title' => SafeMarkup::format('@text', ['@text' => t('Remove')]), + ]; + $remove = [$form['group_items'][$group_id]['remove'], $link]; $data = array( - 'default' => array( - 'data' => array( - '#markup' => drupal_render($form['default_group'][$group_id]) . drupal_render($form['default_group_multiple'][$group_id]), - ), - ), + 'default' => ['data' => $default], 'weight' => drupal_render($form['group_items'][$group_id]['weight']), 'title' => drupal_render($form['group_items'][$group_id]['title']), 'operator' => drupal_render($form['group_items'][$group_id]['operator']), 'value' => drupal_render($form['group_items'][$group_id]['value']), - 'remove' => array( - 'data' => array( - '#markup' => drupal_render($form['group_items'][$group_id]['remove']) . \Drupal::l(SafeMarkup::format('@text', array('@text' => t('Remove'))), Url::fromRoute('', [], array('attributes' => array('id' => 'views-remove-link-' . $group_id, 'class' => array('views-hidden', 'views-button-remove', 'views-groups-remove-link', 'views-remove-link'), 'alt' => t('Remove this item'), 'title' => t('Remove this item'))))), - ), - ), + 'remove' => ['data' => $remove], ); $rows[] = array('data' => $data, 'id' => 'views-row-' . $group_id, 'class' => array('draggable')); } diff --git a/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php index 1e7d355..b7c2538 100644 --- a/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php +++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php @@ -188,4 +188,73 @@ function testPlaceholder() { $this->assertEquals('Some text', SafeMarkup::placeholder('Some text')); } + /** + * Tests SafeMarkup::replace(). + * + * @dataProvider providerReplace + * @covers ::replace + */ + public function testReplace($search, $replace, $subject, $expected, $is_safe) { + $result = SafeMarkup::replace($search, $replace, $subject); + $this->assertEquals($expected, $result); + $this->assertEquals($is_safe, SafeMarkup::isSafe($result)); + } + + /** + * Data provider for testReplace(). + * + * @see testReplace() + */ + public function providerReplace() { + $tests = []; + + // Subject unsafe. + $tests[] = [ + '', + SafeMarkup::set('foo'), + 'bazqux', + 'foobazqux', + FALSE, + ]; + + // All safe. + $tests[] = [ + '', + SafeMarkup::set('foo'), + SafeMarkup::set('barbaz'), + 'foobarbaz', + TRUE, + ]; + + // Safe subject, but should result in unsafe string because replacement is + // unsafe. + $tests[] = [ + '', + 'fubar', + SafeMarkup::set('barbaz'), + 'fubarbarbaz', + FALSE, + ]; + + // Array with all safe. + $tests[] = [ + ['', '', ''], + [SafeMarkup::set('foo'), SafeMarkup::set('bar'), SafeMarkup::set('baz')], + SafeMarkup::set(''), + 'foobarbaz', + TRUE, + ]; + + // Array with unsafe replacement. + $tests[] = [ + ['', '', '',], + [SafeMarkup::set('bar'), SafeMarkup::set('baz'), 'qux'], + SafeMarkup::set(''), + 'barbazqux', + FALSE, + ]; + + return $tests; + } + } diff --git a/core/tests/Drupal/Tests/Core/Render/RendererPostRenderCacheTest.php b/core/tests/Drupal/Tests/Core/Render/RendererPostRenderCacheTest.php index 50ccc2e..d96e4fd 100644 --- a/core/tests/Drupal/Tests/Core/Render/RendererPostRenderCacheTest.php +++ b/core/tests/Drupal/Tests/Core/Render/RendererPostRenderCacheTest.php @@ -432,7 +432,7 @@ public function testPlaceholder() { '#prefix' => '
',
       '#suffix' => '
', ]; - $expected_output = '
' . $context['bar'] . '
'; + $expected_output = '
' . $context['bar'] . '
'; // #cache disabled. $element = $test_element; @@ -530,7 +530,7 @@ public function testChildElementPlaceholder() { '#suffix' => '' ], ]; - $expected_output = '
' . $context['bar'] . '
' . "\n"; + $expected_output = '
' . $context['bar'] . '
' . "\n"; // #cache disabled. $element = $test_element; diff --git a/core/tests/Drupal/Tests/Core/Render/RendererTest.php b/core/tests/Drupal/Tests/Core/Render/RendererTest.php index 298a743..6fcad8d 100644 --- a/core/tests/Drupal/Tests/Core/Render/RendererTest.php +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php @@ -81,6 +81,10 @@ public function providerTestRenderBasic() { $data[] = [[ 'child' => ['#markup' => 'bar'], ], 'bar']; + // XSS filtering test. + $data[] = [[ + 'child' => ['#markup' => "This is test"], + ], "This is alert('XSS') test"]; // #children set but empty, and renderable children. $data[] = [[ '#children' => '', diff --git a/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php index 903adce..fb0aeb9 100644 --- a/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php +++ b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php @@ -255,7 +255,7 @@ public static function callback(array $element, array $context) { public static function placeholder(array $element, array $context) { $placeholder = \Drupal::service('renderer')->generateCachePlaceholder(__NAMESPACE__ . '\\PostRenderCache::placeholder', $context); $replace_element = array( - '#markup' => '' . $context['bar'] . '', + '#markup' => '' . $context['bar'] . '', '#attached' => array( 'drupalSettings' => [ 'common_test' => $context,