Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping

Looks right to me, agreed we need this - function body is identical to check_plain() in Drupal 7, and it's worth having the wrapper in case we ever needed to adjust those arguments again.

I agree we should have this. Patch looks solid and even has test coverage, thanks!

1. +++ b/core/lib/Drupal/Component/Utility/Html.php
   +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -348,4 +348,22 @@ public static function decodeEntities($text) {
   
   @@ -348,4 +348,22 @@ public static function decodeEntities($text) {
   +  public static function encodeEntities($text) {
   

   Symmetry! :)

2. +++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
   @@ -259,4 +259,36 @@ public function providerDecodeEntities() {
   +    return array(
   +      array('Drupal', 'Drupal'),
   +      array('&lt;script&gt;', '<script>'),
   +      array('&amp;lt;script&amp;gt;', '&lt;script&gt;'),
   +      array('&amp;#34;', '&#34;'),
   +      array('&quot;', '"'),
   +      array('&amp;quot;', '&quot;'),
   +      array('&#039;', "'"),
   +      array('&amp;#039;', '&#039;'),
   +      array('©', '©'),
   +      array('→', '→'),
   +      array('➼', '➼'),
   +      array('€', '€'),
   +    );
   

   Nit: Could've used [] instead of array().

Hm, I really feel like this is bloat. We are wrapping a PHP function. Every single method we add to the string sanitization API makes it harder to understand and people are already overwhelmed by it. See all the discussion in #2506195: Remove SafeMarkup::set() from Xss::filter().

The reason checkPlain() exists in the first place was a unicode support bug prior to PHP 5.2.x, no? D7 including check_plain() was basically just BC. In D8 we've retained it even though we renamed it... and now we're going to add yet another copy of it, so the caller has to choose between two different wrappers for a PHP function?

And yes, to @alexpott's point, I also think decodeEntities() is probably bloat. But at least it's not bloat in what is already an overwhelming space.

I also disagree with the assertion that this is a bug, because the bad code in head should just use htmlspecialchars() instead.

It looks as though we could (maybe *should*) call ini_set("default_charset", 'UTF-8') in DrupalKernel. Then we could omit the 3rd argument in usages since it defaults to UTF-8 in PHP 5.5 and to the default charset in 5.6+.

The only risk would be someone putting the wrong second argument (or no argument) in a case like HTML attribute values where we rely on all quotes being escaped.

Every single method we add to the string sanitization API makes it harder to understand and people are already overwhelmed by it.

If we have Html::encodeEntities() that provides a place to document where it's used. Calls to htmlspecialchars() with two optional arguments don't provide anywhere for this clarification to happen.

For me the question comes how much to do we want to see htmlspecialchars() all over the place and have to remember the args.

Yes it comes down to this, and also whether those arguments to htmlspecialchars() might ever have to change in the future - as #12 suggests they could (although at least no bc break for now).

This should knock off one more of those the Drupal\views\Tests\Plugin\DisplayFeedTest.

To note, this is currently broken currently broken in HEAD because if a field value has a new line character, the SafeMarkup'd string is now dirty when the <br> tags are added it will be passed to the link and won't match the SafeMarkup::safe_strings list, thus escaping it.

I'm not sure my solution is the right one, but at least it will do the trick for the time being.

Really like the direction of this patch though, big fan!

Well that sucks, fixed one test, broke 4 more:S @alexpott you set traps! I'm not so sure the search title's should be escaping those values? @see interdiff The UUID was just a casting problem.

It is odd to have Html::encodeEntities vs. SafeMarkup::checkPlain, so can we implement a a SafeMarkup::encodeEntities calling to ::checkPlain or wise versa?

Meh, so I reviewed the other issues for fun, meh

Whoops that was part of a comment that ended in the issue summary related to tracking down the views feed test failure in stringformatter.

Regarding more rendering, we'd like to avoid early rendering as much as possible, but in this case it may already be rendered... I'll have to keep an eye out in a review.

@alexpott sorry was on phone checking email, hadn't read the patch yet and read them as the same thing. Fewf

I'll add a test tonight at the airport for the bug found in #21 which should be resolved now.

I'm adding a "Needs manual testing" because with this change there is a possibility that we double escaped a few things by accident.
A few places to check (because checking the whole patch would be crazy):

1. Twig Debug Comments (must turn on twig debugging in your services.yml)
2. Site name in Bartik (in header)
3. Views token replacement
4. A view_mode with an & in the views UI entity view mode choices
5. Views exposed form options.
6. User names with special characters.
7. User Permissions Page
8. Quick Edit tool
9. Language Content Settings table
10. Config Translation Overview
11. Image Style edit form
12. Comment Author name & title

And anywhere else in your travels, feel free to use simplytest.me for this task. And ask me if you need clarification on those pointer areas. Try to including <tag>tags and & in input to see what you get. and looking for where it's encoded in source as & which will show up on the screen as &

Note to myself, stopped before views.

1. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
   @@ -112,10 +111,10 @@ protected function getEntityIds() {
      protected function getLabel(EntityInterface $entity) {
   -    return SafeMarkup::checkPlain($entity->label());
   +    return $entity->label();
      }
   

   +1 for not filtering/escaping here

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionLoggingSubscriber.php
   @@ -45,7 +45,7 @@ public function __construct(LoggerChannelFactoryInterface $logger) {
   -    $this->logger->get('access denied')->warning(SafeMarkup::checkPlain($request->getRequestUri()));
   +    $this->logger->get('access denied')->warning(Html::encodeEntities($request->getRequestUri()));
   
   @@ -56,7 +56,7 @@ public function on403(GetResponseForExceptionEvent $event) {
   -    $this->logger->get('page not found')->warning(SafeMarkup::checkPlain($request->getRequestUri()));
   +    $this->logger->get('page not found')->warning(Html::encodeEntities($request->getRequestUri()));
   
   +++ b/core/lib/Drupal/Core/EventSubscriber/Fast404ExceptionHtmlSubscriber.php
   @@ -79,7 +79,7 @@ public function on404(GetResponseForExceptionEvent $event) {
        if ($config->get('fast_404.enabled') && $exclude_paths && !preg_match($exclude_paths, $request->getPathInfo())) {
   ...
   +        $fast_404_html = strtr($config->get('fast_404.html'), ['@path' => Html::encodeEntities($request->getUri())]);
            $response = new Response($fast_404_html, Response::HTTP_NOT_FOUND);
   

   Mh, don't we want to just escape on output time?

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceIdFormatter.php
   @@ -33,7 +33,7 @@ public function viewElements(FieldItemListInterface $items) {
   -          '#markup' => SafeMarkup::checkPlain($entity->id()),
   +          '#markup' => Html::encodeEntities($entity->id()),
   

   Do we really need to Html::encodeEntities what is passed into #markup? Just curious here

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -132,7 +132,7 @@ public function viewElements(FieldItemListInterface $items) {
   -          '#title' => $string,
   +          '#title' => ['#markup' => $string],
   

   This is odd I mean do we really need that? Maybe we could add some small comment?

5. +++ b/core/modules/action/action.views_execution.inc
   @@ -5,14 +5,14 @@
   -use Drupal\Component\Utility\SafeMarkup;
   +use Drupal\Component\Utility\Html;
   ...
   -  $select_all_placeholder = SafeMarkup::checkPlain('<!--action-bulk-form-select-all-->');
   +  $select_all_placeholder = Html::encodeEntities('<!--action-bulk-form-select-all-->');
   

   Why do we need to escape a known string?

6. +++ b/core/modules/block/src/BlockListBuilder.php
   @@ -261,7 +260,7 @@ protected function buildBlocksForm() {
   -            '#markup' => SafeMarkup::checkPlain($info['label']),
   +            '#markup' => Html::encodeEntities($info['label']),
   

   So there are places in the patch which use #markup with Html::encodeEntities and without. I think we should make it clear, which one is the one to use

7. +++ b/core/modules/block/src/Controller/BlockLibraryController.php
   @@ -109,7 +108,7 @@ public function listBlocks(Request $request, $theme) {
   -      $row['category']['data'] = SafeMarkup::checkPlain($plugin_definition['category']);
   +      $row['category']['data'] = $plugin_definition['category'];
   

   If I understand template_preprocess_table() correctly it will then be passed to $variables[$section][$row_key]['cells'][$col_key]['content'] = $cell_content which is a variable which will be escaped by twig

8. +++ b/core/modules/block/src/Controller/CategoryAutocompleteController.php
   @@ -59,7 +59,7 @@ public function autocomplete(Request $request) {
   -        $matches[] = array('value' => $category, 'label' => SafeMarkup::checkPlain($category));
   +        $matches[] = array('value' => $category, 'label' => Html::encodeEntities($category));
   

   Sounds like a classical usecase where we should apply render side escaping ... This is used in autocomplete.js:renderItem

9. +++ b/core/modules/comment/comment.tokens.inc
   @@ -161,7 +161,7 @@ function comment_tokens($type, $tokens, array $data, array $options, BubbleableM
   -          $replacements[$original] = $sanitize ? SafeMarkup::checkPlain($comment->language()->getId()) : $comment->language()->getId();
   +          $replacements[$original] = $sanitize ? Html::encodeEntities($comment->language()->getId()) : $comment->language()->getId();
   

   So just as example I guess the entire resulting token string will be marked as safe then?

10. +++ b/core/modules/editor/src/EditorXssFilter/Standard.php
    @@ -114,7 +113,7 @@ protected static function filterXssDataAttributes($html) {
             // value. There is no need to explicitly decode $node->value, since the
             // DOMAttr::value getter returns the decoded value.
             $value = Xss::filterAdmin($node->value);
    -        $node->value = SafeMarkup::checkPlain($value);
    +        $node->value = Html::encodeEntities($value);
    

    It is interesting that we filter first and then still encode the entities

11. +++ b/core/modules/editor/src/Tests/EditorSecurityTest.php
    @@ -388,7 +387,6 @@ function testSwitchingSecurity() {
         //  - switch to every other text format/editor
         //  - assert the XSS-filtered values that we get from the server
    -    $value_original_attribute = SafeMarkup::checkPlain(self::$sampleContent);
         $this->drupalLogin($this->privilegedUser);
    

    Just went back in time and indeed this was never used

12. +++ b/core/modules/field_ui/src/FieldStorageConfigListBuilder.php
    @@ -116,16 +117,10 @@ public function buildRow(EntityInterface $field_storage) {
    -    $usage_escaped = '';
    -    $separator = '';
    -    foreach ($usage as $usage_item) {
    -      $usage_escaped .=  $separator . SafeMarkup::escape($usage_item);
    -      $separator = ', ';
    -    }
    -    $row['data']['usage'] = SafeMarkup::set($usage_escaped);
    +    $row['data']['usage'] = SafeMarkup::set(implode($usage, ', '));
    

    Nice, this is quite better.

13. +++ b/core/modules/node/src/NodeTypeForm.php
    @@ -55,7 +54,7 @@ public function form(array $form, FormStateInterface $form_state) {
    -      $form['#title'] = SafeMarkup::checkPlain($this->t('Add content type'));
    +      $form['#title'] = $this->t('Add content type');
           $fields = $this->entityManager->getBaseFieldDefinitions('node');
    

    Nice one!

14. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -335,7 +336,7 @@ protected function prepareResults(StatementInterface $found) {
             $this->renderer->renderPlain($build) . ' ' .
    -        SafeMarkup::escape($this->moduleHandler->invoke('comment', 'node_update_index', array($node, $item->langcode)))
    +        $this->moduleHandler->invoke('comment', 'node_update_index', array($node, $item->langcode))
    

    So the result of comment_node_update_index() is safe because we use render API there?

15. +++ b/core/modules/node/src/Plugin/views/row/Rss.php
    @@ -78,7 +77,7 @@ public function buildOptionsForm_summary_options() {
       public function summaryTitle() {
         $options = $this->buildOptionsForm_summary_options();
    -    return SafeMarkup::checkPlain($options[$this->options['view_mode']]);
    +    return $options[$this->options['view_mode']];
       }
    

    This should be fine, as the result is rendered as Link using the LinkGenerator at the end

16. +++ b/core/modules/rdf/rdf.module
    @@ -416,7 +416,7 @@ function rdf_preprocess_username(&$variables) {
    -    $variables['attributes']['content'] = SafeMarkup::checkPlain($variables['name_raw']);
    +    $variables['attributes']['content'] = $variables['name_raw'];
    

    So AttributeValueBase/AttributeString escape key and value so we are safe here

17. +++ b/core/modules/shortcut/src/Form/SwitchShortcutSet.php
    @@ -71,7 +70,7 @@ public function buildForm(array $form, FormStateInterface $form_state, UserInter
         $options = array_map(function (ShortcutSet $set) {
    -      return SafeMarkup::checkPlain($set->label());
    +      return $set->label();
         }, $this->shortcutSetStorage->loadMultiple());
    

    Btw. it is really great that you no longer have to escape for #type select options. This is quite a DX win

18. +++ b/core/modules/taxonomy/taxonomy.tokens.inc
    @@ -131,14 +131,14 @@ function taxonomy_tokens($type, $tokens, array $data, array $options, Bubbleable
    -          $replacements[$original] = SafeMarkup::checkPlain($vocabulary->label());
    +          $replacements[$original] = Html::encodeEntities($vocabulary->label());
    ...
    -            $replacements[$original] = SafeMarkup::checkPlain($parent->getName());
    +            $replacements[$original] = Html::encodeEntities($parent->getName());
    

    Out of scope of this issue but I guess those instances should use $sanitize ? as well?

Wow EntityListBuilder::getLabel() is now completely pointless :)

Changed issue title from "Replace" to "Remove" as per #27

+++ b/core/modules/views/views.module
@@ -66,8 +66,8 @@ function views_views_pre_render($view) {
-          'view_args' => SafeMarkup::checkPlain(implode('/', $view->args)),
-          'view_path' => SafeMarkup::checkPlain(Url::fromRoute('<current>')->toString()),
+          'view_args' => implode('/', $view->args),
+          'view_path' => Url::fromRoute('<current>')->toString(),

Mh, so these variables are used in JS, does that mean we need some protected there instead?

Escaping in JS would be consistent with #2503963: XSS in Quick Edit: entity title is not safely encoded.

I opened #2550945: Add Html::escape() to talk about adding Html::encodeEntities() or not. I think the wrapper is useful both in the interim state before this overall patch gets committed and after.

I'm sprinting at the Midwest Developers Summit in Chicago right now, and I'm taking a look at #40.

Re-rolled #32. Fixed conflicting lines in core/themes/engines/twig/twig.engine, preferring #32 lines over changes made elsewhere.

I'm no longer working on this ticket. Planning to pick it up tomorrow.

Patch requires another re-roll. Addressing this now.

Fixed merge conflict in core/modules/node/src/Plugin/Search/NodeSearch.php, favoring Patch #32 line 338 over conflicting line. Re-rolled.

Updating the Teaser label in the core.entity_view_mode.node.teaser.yml file to include an '&' character causes double escaping as a result of Patch #52. Picture:

Steps to reproduce

1. Apply Patch #52
2. Apply 2545972.53-temp-label-do-not-test.patch
3. Re-install Drupal
4. Navigate to: admin/structure/views/view/frontpage

I'm done working on this issue for the day.

Here's a re-roll and this is no longer postponed, but it does need the review @ericjenkins did in #53 taken into consideration and after that is resolved, more manual testing.

Which is cases where the escaped value gets passed into link generator.

#2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter just landed, this is now fully unpostponed.

Updated IS to match approach in #60.

Note #41 is still true (mostly)

#63++

#60: interdiff review:

1. +++ b/core/includes/form.inc
   @@ -568,10 +568,11 @@ function template_preprocess_form_element_label(&$variables) {
   - * \Drupal\Component\Utility\SafeMarkup::checkPlain() or
   - * \Drupal\Component\Utility\Xss::filter(). Furthermore, if the batch operation
   ...
   + * \Drupal\Component\Utility\SafeMarkup::format(),
   + * \Drupal\Component\Utility\Xss::filter(), or rely on the auto-escaping of the
   

   This docs update looks a bit strange. Why are we explicitly recommending SafeMarkup::format() here, while we were not before?

   And while that makes some sense, I don't think the mention of Xss::filter() still makes sense; I think that should point to #markup's #safe_strategy property?

2. +++ b/core/includes/form.inc
   @@ -595,8 +596,9 @@ function template_preprocess_form_element_label(&$variables) {
   + *   // Twig will auto-escape this.
   + *   $context['message'] = $node->label();
   

   Why explicitly mention it here? We don't do that elsewhere? Specifically because this is the node title and that'll cause people to raise eyebrows when they read this?

3. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -15,9 +15,9 @@
   + * markup strings created from @link theme_render render arrays @endlink via
   + * drupal_render().
   

   Pre-existing technically, but reading markup strings created from render arrays are automatically marked safe sounds scary. If it said markup strings created from render arrays are automatically filtered/escaped as necessary and then marked as safe or something like that, that'd be less scary :)

4. +++ b/core/lib/Drupal/Core/Controller/TitleResolverInterface.php
   @@ -18,10 +18,10 @@
   -   * implementation should call \Drupal\Component\Utility\SafeMarkup::checkPlain()
   -   * or \Drupal\Component\Utility\Xss::filterAdmin() on the string, or use
   -   * appropriate placeholders to sanitize dynamic content inside a localized
   -   * string before returning it. The title may contain HTML such as EM tags.
   +   * implementation should call \Drupal\Component\Utility\Xss::filterAdmin() on
   +   * the string, or use appropriate placeholders to sanitize dynamic content
   +   * inside a localized string before returning it. The title may contain HTML
   +   * such as EM tags.
   

   Shouldn't this instead be recommending to use ['#markup' => 'title', '#allowed_tags' => […]]?

5. +++ b/core/lib/Drupal/Core/Field/AllowedTagsXssTrait.php
   @@ -23,7 +23,7 @@
       * Used for items entered by administrators, like field descriptions, allowed
       * values, where some (mainly inline) mark-up may be desired (so
   -   * \Drupal\Component\Utility\SafeMarkup::checkPlain() is not acceptable).
   +   * \Drupal\Component\Utility\Html::escape() is not acceptable).
   

   Shouldn't this also mention #markup + #allowed_tags?

6. +++ b/core/modules/system/tests/modules/test_page_test/src/Controller/Test.php
   @@ -55,7 +55,7 @@ public function dynamicTitle() {
   +   *   Whether or not to mark the title as safe using the render system.
   

   s/the render system/the Markup element's "escape" safe strategy/

7. +++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
   @@ -180,18 +180,18 @@ public function testReplace($search, $replace, $subject, $expected, $is_safe) {
   +    // SafeMarkup::escape() will escape the markup tag since the string is not
   

   s/markup tag/tag/

8. +++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
   @@ -180,18 +180,18 @@ public function testReplace($search, $replace, $subject, $expected, $is_safe) {
   +    // marked.
   

   s/marked/marked safe/

#61: interdiff review:

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -321,7 +322,7 @@ public function render() {
-      $build['#markup'] = Html::escape($build['#markup']);
+      $build['#safe_strategy'] = Markup::SAFE_STRATEGY_ESCAPE;

+++ b/core/modules/views/src/Plugin/views/display/Feed.php
@@ -99,7 +99,8 @@ public function preview() {
-        '#markup' => Html::escape(drupal_render_root($output)),
+        '#markup' => drupal_render_root($output),
+        '#safe_strategy' => Markup::SAFE_STRATEGY_ESCAPE,

This one is wrong. drupal_render_root() already explicitly returns a safe string, and we specifically still want to escape it because we want to preview the source (markup) of the feed.

Could use specific documentation explaining this probably.

This would also fix 3 of the 4 failures AFAICT.

Next: another complete review.

#65:

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -322,6 +322,8 @@ public function render() {
+      // Cause #markup to lose its SafeStringInterface object wrapper.
+      $build['#markup'] = (string) $build['#markup'];
       $build['#safe_strategy'] = Markup::SAFE_STRATEGY_ESCAPE;

+++ b/core/modules/views/src/Plugin/views/display/Feed.php
@@ -99,7 +99,9 @@ public function preview() {
-        '#markup' => drupal_render_root($output),
+        // Cast the result of rendering to a string so it loses its
+        // SafeStringInterface object wrapper.
+        '#markup' => (string) drupal_render_root($output),
         '#safe_strategy' => Markup::SAFE_STRATEGY_ESCAPE,

Hah, that's definitely an alternative solution to the problem I also found in my review of #61 in my previous comment. But I think it is much clearer to use Html::escape(), because that's more explicit.

Chatted with Alex Pott about #67/#68. It doesn't make sense to use Html::escape(), because that doesn't mark the result as a safe string. Which means that #markup will still attempt to do filtering. Which is excessive work. Hence, the approach that #65 uses already is the best option.

I do think the comment can be made clearer though, IMO the intent is only clear from context and would be better spelled out explicitly. i.e.

+        // Cast the result of rendering to a string so it loses its
+        // SafeStringInterface object wrapper.

->

+        // Cast the result of rendering to a string so it loses its
+        // SafeStringInterface object wrapper. Do this so that the #safe_strategy takes effect and
+        // escapes the markup: unlike most things this intends to display the actual markup.

(Or something like that.)

If you disagree that that's better, it's fine to leave it as is.

And finally, the full review of #65, as promised.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/BasicStringFormatter.php
   @@ -37,7 +37,7 @@ public function viewElements(FieldItemListInterface $items) {
   -      $elements[$delta] = array('#markup' => nl2br(SafeMarkup::checkPlain($item->value)));
   +      $elements[$delta] = array('#markup' => nl2br(Html::escape($item->value)));
   

   It's a pre-existing problem, but this will cause Markup's filtering safe strategy to still be applied. Even though that's absolutely unnecessary.

   The only tag that is going to be in there is <br>, by definition. And that is allowed by \Drupal\Component\Utility\Xss::getAdminTagList().

   However, using SafeString::create() here or creating another SafeStringInterface implementation for this also feels kinda overkill.

   Not sure what's best.

   But if we don't improve this here, I think we should at least document it, because in the future we won't be scrutinizing string safeness etc quite as much again.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -132,7 +132,7 @@ public function viewElements(FieldItemListInterface $items) {
   -          '#title' => $string,
   +          '#title' => ['#markup' => $string],
   

   Why? $string is already escaped.

3. +++ b/core/modules/block/src/Controller/CategoryAutocompleteController.php
   @@ -59,7 +59,7 @@ public function autocomplete(Request $request) {
   -        $matches[] = array('value' => $category, 'label' => SafeMarkup::checkPlain($category));
   +        $matches[] = array('value' => $category, 'label' => Html::escape($category));
   
   +++ b/core/modules/block/tests/src/Unit/CategoryAutocompleteTest.php
   @@ -48,7 +48,7 @@ protected function setUp() {
   -      return array('value' => $suggestion, 'label' => SafeMarkup::checkPlain($suggestion));
   +      return array('value' => $suggestion, 'label' => Html::escape($suggestion));
   

   In #2503963: XSS in Quick Edit: entity title is not safely encoded, it was determined that this escaping actually belongs on the client side.

4. +++ b/core/modules/content_translation/content_translation.admin.inc
   --- a/core/modules/dblog/src/Controller/DbLogController.php
   +++ b/core/modules/dblog/src/Controller/DbLogController.php
   

   Unused uses: SafeMarkup & Xss.

5. +++ b/core/modules/editor/src/Tests/EditorSecurityTest.php
   @@ -388,7 +387,6 @@ function testSwitchingSecurity() {
   -    $value_original_attribute = SafeMarkup::checkPlain(self::$sampleContent);
   

   LOL, yes, that was dead code for some time already. Yay for no more dragging it along.

6. +++ b/core/modules/field_ui/src/FieldStorageConfigListBuilder.php
   @@ -116,16 +117,10 @@ public function buildRow(EntityInterface $field_storage) {
   -        $usage[] = $this->bundles[$entity_type_id][$bundle]['label'];
   +        $usage[] = Html::escape($this->bundles[$entity_type_id][$bundle]['label']);
          }
        }
   -    $usage_escaped = '';
   -    $separator = '';
   -    foreach ($usage as $usage_item) {
   -      $usage_escaped .=  $separator . SafeMarkup::escape($usage_item);
   -      $separator = ', ';
   -    }
   -    $row['data']['usage'] = SafeMarkup::set($usage_escaped);
   +    $row['data']['usage'] = SafeMarkup::set(implode($usage, ', '));
   

   Much nicer :)

7. +++ b/core/modules/filter/filter.module
   @@ -616,8 +615,8 @@ function _filter_url_parse_full_links($match) {
   -  $caption = SafeMarkup::checkPlain(_filter_url_trim($match[$i]));
   -  $match[$i] = SafeMarkup::checkPlain($match[$i]);
   +  $caption = Html::escape(_filter_url_trim($match[$i]));
   +  $match[$i] = Html::escape($match[$i]);
   
   @@ -631,8 +630,8 @@ function _filter_url_parse_email_links($match) {
   -  $caption = SafeMarkup::checkPlain(_filter_url_trim($match[$i]));
   -  $match[$i] = SafeMarkup::checkPlain($match[$i]);
   +  $caption = Html::escape(_filter_url_trim($match[$i]));
   +  $match[$i] = Html::escape($match[$i]);
   

   Lots of side effects removed (needless additions to the safe markup list), yay!

8. +++ b/core/modules/filter/src/FilterFormatFormBase.php
   @@ -79,7 +79,7 @@ public function form(array $form, FormStateInterface $form_state) {
   -      '#options' => array_map('\Drupal\Component\Utility\SafeMarkup::checkPlain', user_role_names()),
   +      '#options' => user_role_names(),
          '#disabled' => $is_fallback,
          '#weight' => -10,
        );
   +++ b/core/modules/filter/src/FilterFormatListBuilder.php
   +++ b/core/modules/filter/src/FilterFormatListBuilder.php
   @@ -107,7 +107,7 @@ public function buildRow(EntityInterface $entity) {
        }
        else {
          $row['label'] = $this->getLabel($entity);
   -      $roles = array_map('\Drupal\Component\Utility\SafeMarkup::checkPlain', filter_get_roles_by_format($entity));
   +      $roles = filter_get_roles_by_format($entity);
   

   Hm. These #options changes are in many places. Can you explain why this is okay? Where are they being escaped now?

9. +++ b/core/modules/image/src/Form/ImageStyleEditForm.php
   @@ -9,10 +9,11 @@
   +use Drupal\Component\Utility\Html;
   

   Unused.

10. +++ b/core/modules/locale/src/Form/TranslateEditForm.php
    @@ -73,7 +73,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -            '#markup' => '<span lang="en">' . SafeMarkup::checkPlain($source_array[0]) . '</span>',
    +            '#markup' => '<span lang="en">' . Html::escape($source_array[0]) . '</span>',
               );
             }
             else {
    @@ -82,13 +82,13 @@ public function buildForm(array $form, FormStateInterface $form_state) {
               $original_singular = [
                 '#type' => 'item',
                 '#title' => $this->t('Singular form'),
    -            '#markup' => '<span lang="en">' . SafeMarkup::checkPlain($source_array[0]) . '</span>',
    +            '#markup' => '<span lang="en">' . Html::escape($source_array[0]) . '</span>',
                 '#prefix' => '<span class="visually-hidden">' . $this->t('Source string (@language)', array('@language' => $this->t('Built-in English'))) . '</span>',
               ];
               $original_plural = [
                 '#type' => 'item',
                 '#title' => $this->t('Plural form'),
    -            '#markup' => '<span lang="en">' . SafeMarkup::checkPlain($source_array[1]) . '</span>',
    +            '#markup' => '<span lang="en">' . Html::escape($source_array[1]) . '</span>',
    

    Can't these be updated to have the <span>s in #prefix & #suffix?

11. +++ b/core/modules/node/src/Controller/NodePreviewController.php
    @@ -61,7 +60,7 @@ public function view(EntityInterface $node_preview, $view_mode_id = 'full', $lan
       public function title(EntityInterface $node_preview) {
    -    return SafeMarkup::checkPlain($this->entityManager->getTranslationFromContext($node_preview)->label());
    +    return $this->entityManager->getTranslationFromContext($node_preview)->label();
       }
    

    <3

12. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -448,7 +448,7 @@ protected function indexNode(NodeInterface $node) {
           $rendered = $this->renderer->renderPlain($build);
     
    -      $text = '<h1>' . SafeMarkup::checkPlain($node->label($language->getId())) . '</h1>' . $rendered;
    +      $text = '<h1>' . Html::escape($node->label($language->getId())) . '</h1>' . $rendered;
    

    This could be implemented more cleanly by doing:

    $build['search_title'] = [
      '#prefix' => '<h1>',
      '#markup' => $node->label(), '#safe_strategy' => Markup::SAFE_STRATEGY_ESCAPE
      '#suffix' => '</h1>',
    ];
    $text = $this->renderer->renderPlain($build);
    

13. +++ b/core/modules/rdf/rdf.module
    +++ b/core/modules/rdf/rdf.module
    @@ -419,7 +419,7 @@ function rdf_preprocess_username(&$variables) {
       // Long usernames are truncated by template_preprocess_username(). Store the
       // full name in the content attribute so it can be extracted in RDFa.
       if ($variables['truncated']) {
    -    $variables['attributes']['content'] = SafeMarkup::checkPlain($variables['name_raw']);
    +    $variables['attributes']['content'] = $variables['name_raw'];
    

    SafeMarkup is still imported.

14. +++ b/core/modules/search/tests/modules/search_embedded_form/search_embedded_form.module
    @@ -9,6 +9,8 @@
    +use Drupal\Component\Utility\SafeMarkup;
    +
    

    Unused.

15. +++ b/core/modules/system/tests/modules/test_page_test/src/Controller/Test.php
    @@ -55,16 +55,24 @@ public function dynamicTitle() {
    +   * @todo What is mark_safe doing here now?
    

    Isn't it effectively testing the same thing? A title that still needs to be escaped (and is not yet marked safe) and a title that already is escaped (and already is marked safe)?

    I don't see the difference.

16. +++ b/core/modules/tracker/src/Controller/TrackerUserTab.php
    @@ -28,6 +27,6 @@ public function getContent(UserInterface $user) {
       public function getTitle(UserInterface $user) {
    -    return SafeMarkup::checkPlain($user->getUsername());
    +    return $user->getUsername();
       }
    

    <3

17. +++ b/core/modules/user/src/Tests/Views/AccessPermissionTest.php
    @@ -35,7 +35,7 @@ function testAccessPerm() {
    -    $this->assertEqual($access_plugin->pluginTitle(), t('Permission'));
    +    $this->assertEqual((string) $access_plugin->pluginTitle(), t('Permission'));
    

    This already uses assertEqual(), so why is this change necessary?

18. +++ b/core/modules/user/tests/src/Unit/Views/Argument/RolesRidTest.php
    @@ -7,7 +7,7 @@
    +use Drupal\Component\Utility\Html;
    

    Unused.

19. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -365,7 +365,8 @@ protected function viewsTokenReplace($text, $tokens) {
    +        // Twig will auto-escape any encoded HTML entities.
    +        $twig_tokens[$token] = Html::decodeEntities($replacement);
    

    Does this need additional test coverage?

20. +++ b/core/modules/views/src/Tests/SearchIntegrationTest.php
    @@ -7,6 +7,7 @@
    +use Drupal\Component\Utility\Html;
    

    Unused.

Awesome reroll, Alex, thanks!

• #41.3's fix is missing
• #41.8: can we open that follow-up already, and track the ones that were pointed out here? Such an issue will be much easier if we already have a list of things that should do this. Especially if they have // @todo: … in the code. Also related to this: #44, #71.3.
• #71.1: okay, curious how we're going to end up resolving that one :)
• #71.8: AFAICT that test is not yet present? (Or perhaps it's not in the interdiff?)
• #71.12: there's no additional rendering in my proposal. All I'm proposing is to not first do rendering, and then still concatenate something with the result. But, instead, add another child to the render array with a low enough weight (I see I forgot to mention that: it should have '#weight' => -1000). Then, when we render, there's no more need for us to prefix the rendered output with <h1>TITLE</h1> — the rendering already took care of that for us, and we just get a single safe string!

Interdiff review:

1. +++ b/core/includes/form.inc
   @@ -566,13 +566,13 @@ function template_preprocess_form_element_label(&$variables) {
   + * \Drupal\Core\StringTranslation\TranslationManager::translate. If they are not
   

   s/translate/translate()/

2. +++ b/core/modules/user/src/Tests/Views/AccessPermissionTest.php
   @@ -35,6 +35,8 @@ function testAccessPerm() {
   +    // The plugin title will be a tranlsation wrapper, cast to a string to test
   

   s/tranlsation/translation/
   :)

So, the only remaining bits:

• #41.3
• #53
• #71.1
• #71.8
• #71.12

• #74 looks splendid!
• #41.8: Ok, so we basically draw the line at "D7-like features still do server-side escaping" then. That's good enough for now I guess.
• #71.1: +1 to follow-up — this issue is blocking a critical, and that pre-existing problem shouldn't make it block further.
• I do think a test for #71.8 is important, because that's rather opaque right now.

So the only remaining thing is then #71.8.

Follow-up filed for #71.1: #2554755: BasicStringFormatter's nl2br() forces escaping *and* filtering — improve this.

I think this is ready.

We still need that manual testing though — see #40 for steps.

So FWIW, I'm tentatively in favor of making this change, despite the BC break it causes, because (a) it will be a clean break (so easy to find and fix) and (b) it has significant performance, security, DX, etc. implications.

However, I do think we need to have a discussion about the change with the committers before we commit this one, because it is so sweeping.

Also, has anyone looked over what other change records might need to be updated? It'd be good to have a plan for that as well. (We should search all the CRs for check_plain and checkPlain, etc.)

So FWIW, I'm tentatively in favor of making this change, despite the BC break it causes, because (a) it will be a clean break (so easy to find and fix) and (b) it has significant performance, security, DX, etc. implications.

Right if we break BC we should break it hard!

Also considering how widely used this is in contrib it would be good to have a release with the method deprecated and give time for everyone to convert their modules.

Shouldn't this patch then mark it for deprecation? (Presumably in beta 16, so that during beta 15, developers can upgrade.)

Patch is green. Before RTBC'ing, we still needed that round of manual testing described in #40. None of the exact steps showed problems, but in the process, I did find two double escaping bugs, which are actually the same bug: the Frontpage view's title has a double-escaped site name, this shows up on the actual frontpage, but also in the preview.

However… this is a pre-existing bug in HEAD!

Hence: RTBC. (I think a patch needs to be RTBC for a framework & release manager to even look at the patch? I could be wrong. Feel free to un-RTBC if I'm wrong.)

And issue filed: #2555503: Site name from frontpage view is double-escaped on frontpage and in preview.

So I suggested deferring the deprecation and removal to another issue, so that we could get the main patch in without a full discussion on contrib impact. That should mean we can remove both tags from this issue (apart from it needing to be reviewed by a committer), but assigning to xjm since she added the tags to confirm that was the only thing that needed a wider discussion.

The underlying problem with #53 seems to be if you filter or escape a known safe value and pass it in to the link generator, it will escape it again. The filter one is pre-existing in head because we removed the SafeMarkup::set() safe from it in a recent issue.

I'm glad we removed the premature escaping, and in most cases may be the correct course of action but I feel that part will pop it's head up again.

So as far as I see, removing the premature escaping is great fix but only treats the symptoms. If the solution is from:

\Drupal::l(SafeMarkup::checkPlain('&>'), '<none>');
\Drupal::l(SafeMarkup::filterXssAdmin('&>'), '<none>');

to

\Drupal::l(SafeString::create(Html::escape('&>')), '<none>');
\Drupal::l(SafeString::create(Xss::filterAdmin('&>')), '<none>');

I think we should add that to the change record so people know how to deal with it? If that is not correct and I'm way off base, then the correct way to deal with those pre-escaped variables would be nice to cover in the CR regardless as it seem to cause me and others a bit of confusion.

#93 makes sense to me too.

I found other CRs not mentioned in #84:
https://www.drupal.org/node/2073933
https://www.drupal.org/node/1895936
https://www.drupal.org/node/1876852
https://www.drupal.org/node/2457593
https://www.drupal.org/node/2372691
https://www.drupal.org/node/2153775
https://www.drupal.org/node/2083471
https://www.drupal.org/node/2067859
https://www.drupal.org/node/1876710
https://www.drupal.org/node/1762604
but those we could address in the issue that does the deprecation.

I'm a little confused by this flag

+      '#safe_strategy' => Markup::SAFE_STRATEGY_ESCAPE,

Can we define a different element like #plain_text instead of overloading #markup?

If not can we make the constant at least shorter? like Markup::ESCAPE

As much as I love this issue, #94 needs to be addressed and if we can't I'd even propose reverting #2506195: Remove SafeMarkup::set() from Xss::filter() until it can be.

Edit: wrong referenced thing to revert

@joelpittet

part of your question doesn't make sense. Since l() escapes the title, I think you'd never do this:

\Drupal::l(SafeString::create(Html::escape('&>')), '<none>');

Rather you'd just do:

\Drupal::l('&>', '<none>');

For the other you'd use a render array like:

\Drupal::l(['#markup' => '&>'], '<none>');

However, per my comment in #96, I don't like overloading #markup for plain text also.

My cases are assuming you have markup you don't want to double escape and have previously sanitized.

It does make sense because the fact that l() will escape unsafe strings and since they aren't marked safe any more to prevent the escaping in l(), they will be double escaped now.

So passing a render array to the title is another approach that maps to xss admin filter if not safe. Will prevent double escaping but may do xss filter twice.

I agree with @pwolanin. '#safe_strategy' => Markup::SAFE_STRATEGY_ESCAPE is just plain wrong, as it mixes two radically different issues: the issue of the format (plain text vs. html), and the issue of the sanitization.

This looks fine to me:

[ '#plain_text' => '&>' ]

Unfortunately, the Markup element is hardcoded inside \Drupal\Core\Render\Renderer so it is the only one that can omit #type right now.

The examples are contrived to be succinct. Imagine those variables were filter/escaped into a variable and passed in.

Possibly we should just go ahead here, but I think the DX is a bit crummy and we should have a follow-up to improve it.

In terms of omitting #type I think it could be ok to let the markup type handle both #markup and #plain_text. Basically instead of passing a #safe_strategy just use a different key name.

Sorry for raising alarm bells, I am only trying to make sure people who run into double escaping after this change lands that they will know how to "not do it wrong". And that we add that to the change record so they can get support and pointers in the right direction.

So we are recommending people (obviously I know), let the Drupal::l() escape the text passed in and avoid pre-escaping unsafe markup with SafeMarkup::checkPlain() or it's replacement Html::escape() if they were doing that before. Alternatively, pass in ['#markup' => $string_with_markup] if you want to filter out Xss but you are fine with or expecting un-escaped(no xss) tags and characters. If the value has previously run Html::escape() from somewhere else in the system, then you (a: might be doing it wrong? b: should pass it through ['#markup' => $pre_escaped]?. And we are not recommending SafeString::create()'s usage for known pre-escaped strings?

I have no opinion on the #plain_text comment because I don't understand it's use yet, though a follow-up seems like a likely candidate considering this patch's size. I'll follow the follow-up to try and get a grasp on it.

I agree with #102 (regarding the problems of conflating escaping with filtering), #105 (that the DX for this whole space is definitely "crummy"), and #107 (that we need the change records and other documentation to communicate what people actually need to do. These problems are not introduced by this patch, though. The one change is that this patch does make a much wider use of the #safe_strategy introduced in #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter, which in turn was addressing limitations of the change made in #2273925: Ensure #markup is XSS escaped in Renderer::doRender(), which happened in response to the original SafeMarkup API needing to conflate escaped and filtered text in the first place, all of which put a lot more on the semantics of #markup.

So I think we definitely need to address those issues in followups. I do think the change record updates probably need to block this though. I'm planning to review the patch here (either as a reviewer or committer if it gets RTBCed again by someone else) and I'll see if I can help with that.

From #106, maybe this should be marked postponed on #2555931: Add #plain_text to escape text in render arrays

+++ b/core/includes/common.inc
@@ -281,10 +280,10 @@ function valid_email_address($mail) {
 function check_url($uri) {
-  return SafeMarkup::checkPlain(UrlHelper::stripDangerousProtocols($uri));
+  return Html::escape(UrlHelper::stripDangerousProtocols($uri));
 }

So, this is interesting in light of the concerns above about URL escaping.

I thought "Huh, surprised that's not a method on the URL class now," and then t turns out check_url() is kind of redundant with UrlHelper::filterBadProtocol()... but for an entity decode that's in the method but not the function. This means (I think) that those two functions would have slightly different results on a URL that was passed in as http://www.example.com?a=1&b=2. (@alexpott also pointed out that the case where the decodeEntities() would matter isn't actually in the unit tests.) Regardless, I think that check_url() should be deprecated in a followup to just become a wrapper for filterBadProtocol().

There are places that check_url() is used with @ tokens for t() in HEAD that could result in double-escaping following the removal from the safe list of checkPlain() results for the URLs. @alexpott is updating the patch for this. @todo file followup.

(Just taking notes during review.)

+++ b/core/includes/file.inc
@@ -371,7 +371,7 @@ function file_save_htaccess($directory, $private = TRUE, $force_overwrite = FALS
-    $variables = array('%directory' => $directory, '!htaccess' => '<br />' . nl2br(SafeMarkup::checkPlain($htaccess_lines)));
+    $variables = array('%directory' => $directory, '!htaccess' => '<br />' . nl2br(Html::escape($htaccess_lines)));

This is entertaining. The lines for $htaccess_lines are statically defined in code in FileStorage::htaccessLines(). The purpose of the escaping here is not to sanitize user input, but actually to encode the angle brackets in the example .htaccess for display to the user inside a code tag. I think this may be a double-escaping bug in HEAD regardless of whether a ! or @ placeholder is used (which would be a regression introduced by #2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument). Related: #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests (which in its current form also leaves this bug there). This one is out of the scope of the patch and the patch does not affect it.

1. +++ b/core/includes/form.inc
   @@ -566,12 +566,13 @@ function template_preprocess_form_element_label(&$variables) {
   - * 'error_message' could contain any user input, it is the responsibility of
   - * the code calling batch_set() to sanitize them first with a function like
   - * \Drupal\Component\Utility\SafeMarkup::checkPlain() or
   - * \Drupal\Component\Utility\Xss::filter(). Furthermore, if the batch operation
   - * returns any user input in the 'results' or 'message' keys of $context, it
   - * must also sanitize them first.
   + * 'error_message' could contain any HTML that you want to display then, it is
   + * the responsibility of the code calling batch_set() to mark them safe using a
   + * function like \Drupal\Component\Utility\SafeMarkup::format() or
   + * \Drupal\Core\StringTranslation\TranslationManager::translate(). If they are
   + * not marked safe then Twig will auto-escape them. Furthermore, if the batch
   + * operation returns any HTML in the 'results' or 'message' keys of $context,
   + * it must also consider sanitization.
   

   So, bit confusing, but turns out the topic documentation for the Batch API is in form.inc. Cool...

   This updated documentation is a bit unclear to me and I had to read it several times and discuss with @alexpott to understand. The change is that previously, passing unsanitized text could result in XSS, whereas now, passing any HTML in the messages will result in escaping unless the strings are in the safe list...

   ...Or so the updated comment claims, but actually, I think that at least parts of it will not get escaped by Twig. See in _batch_process_page(). The 'init_message' and 'error_message' get added in JS so it's not clear to me they would actually be escaped by Twig. Related: #2503963: XSS in Quick Edit: entity title is not safely encoded

   It's not uniform either; there's a fallback case where 'error_message' does get passed to renderBarePage() in a #markup which eventually goes through renderRoot() and therefore gets XSS-filtered in that case but not obviously in the other.

   @alexpott also found the use of the 'message' key in _batch_do() and it is similarly put into a JSON response, so therefore has the same escaping concern.

   Per @alexpott the 'results' key in the $context array is used by the specific implementation, so the comment might hold true for that one. Regardless, at least part of this docs change is incorrect and could result in XSS vulnerabilities in batch implementations if they try to follow this documentation.

   Also, the updated documentation is a bit confusing and switches between passive voice and second person, but we can worry about that once we make it say the right thing.

2. +++ b/core/includes/form.inc
   @@ -595,8 +596,9 @@ function template_preprocess_form_element_label(&$variables) {
   - *   $context['results'][] = $node->id() . ' : ' . SafeMarkup::checkPlain($node->label());
   - *   $context['message'] = SafeMarkup::checkPlain($node->label());
   + *   // Twig will auto-escape these variables.
   + *   $context['results'][] = $node->id() . ' : ' .$node->label();
   + *   $context['message'] = $node->label();
   
   @@ -615,10 +617,10 @@ function template_preprocess_form_element_label(&$variables) {
   - *     $context['results'][] = $row->id . ' : ' . SafeMarkup::checkPlain($row->title);
   + *     $context['results'][] = $row->id . ' : ' . $row->title;
   ...
   - *     $context['message'] = SafeMarkup::checkPlain($row->title);
   + *     $context['message'] = $row->title;
   

   Twig will auto-escape these variables, except when it won't. It's definitely a lie about the second line now.

   OTOH, as @alexpott pointed out, the first example line in HEAD is a auto double-escape bug waiting to happen.

1. +++ b/core/includes/update.inc
   @@ -215,7 +214,7 @@ function update_do_one($module, $number, $dependency_map, &$context) {
   -  $context['message'] = 'Updating ' . SafeMarkup::checkPlain($module) . ' module';
   +  $context['message'] = 'Updating ' . $module . ' module';
   

   Another example in HEAD of where we protect site builders from XSS exploits introduced by module authors by embedding XSS (or is it SSS, "same-site scripting"?) in their module names. :P But if this were actual user input, I think this is a case where an XSS bug would be introduced.

2. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -206,8 +210,8 @@ public static function checkPlain($text) {
   -   *       printed as an HTML attribute value and therefore formatted with
   -   *       self::checkPlain() as part of that.
   +   *       printed as an HTML attribute value and therefore escaped as part of
   +   *       that.
   

   @todo for self find the code change relevant for this docs change.

3. +++ b/core/lib/Drupal/Component/Utility/Xss.php
   @@ -107,10 +107,9 @@ public static function filter($string, array $html_tags = NULL) {
   -   * Use only for fields where it is impractical to use the
   -   * whole filter system, but where some (mainly inline) mark-up
   -   * is desired (so \Drupal\Component\Utility\SafeMarkup::checkPlain() is
   -   * not acceptable).
   +   * Use only for fields where it is impractical to use the whole filter system,
   +   * but where some (mainly inline) mark-up is desired (so
   +   * \Drupal\Component\Utility\Html::escape() is not acceptable).
   

   NIH, but this hunk of documentation is already a little misleading in HEAD. Xss::filterAdmin() has way more usecases than non-input-filter-filtered fields.

+++ b/core/lib/Drupal/Core/Controller/TitleResolverInterface.php
@@ -17,18 +17,20 @@
    *
-   * The returned title string must be safe to output in HTML. For example, an
-   * implementation should call \Drupal\Component\Utility\SafeMarkup::checkPlain()
-   * or \Drupal\Component\Utility\Xss::filterAdmin() on the string, or use
-   * appropriate placeholders to sanitize dynamic content inside a localized
-   * string before returning it. The title may contain HTML such as EM tags.
+   * If the returned title can contain HTML that should not be escaped it should
+   * return a render array, for example:
+   * @code
+   * ['#markup' => 'title', '#allowed_tags' => ['em']]
+   * @endcode
+   * If the method returns a string and it is not marked safe then it will be
+   * auto-escaped.
...
-   * @return string|null
+   * @return array|string|null

Note to self: @alexpott says that the support for render arrays here was introduced by #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter and there is discussion on that issue. The docs for the interface just didn't get updated. ( title_resolver service used for _title_callback on routes etc.) Edit: the discussion apparently actually was in IRC; on that issue there is just comment #22.

+++ b/core/lib/Drupal/Core/Diff/DiffFormatter.php
@@ -172,7 +172,7 @@ protected function emptyLine() {
   protected function _added($lines) {
     foreach ($lines as $line) {
-      $this->rows[] = array_merge($this->emptyLine(), $this->addedLine(SafeMarkup::checkPlain($line)));
+      $this->rows[] = array_merge($this->emptyLine(), $this->addedLine(Html::escape($line)));

@@ -181,7 +181,7 @@ protected function _added($lines) {
   protected function _deleted($lines) {
     foreach ($lines as $line) {
-      $this->rows[] = array_merge($this->deletedLine(SafeMarkup::checkPlain($line)), $this->emptyLine());
+      $this->rows[] = array_merge($this->deletedLine(Html::escape($line)), $this->emptyLine());

@@ -190,7 +190,7 @@ protected function _deleted($lines) {
   protected function _context($lines) {
     foreach ($lines as $line) {
-      $this->rows[] = array_merge($this->contextLine(SafeMarkup::checkPlain($line)), $this->contextLine(SafeMarkup::checkPlain($line)));
+      $this->rows[] = array_merge($this->contextLine(Html::escape($line)), $this->contextLine(Html::escape($line)));

It might be worth testing manually to ensure no double-escaping bug is introduced here.

1. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
   @@ -112,10 +111,10 @@ protected function getEntityIds() {
       * @return string
   -   *   The escaped entity label.
   +   *   The entity label.
       */
      protected function getLabel(EntityInterface $entity) {
   -    return SafeMarkup::checkPlain($entity->label());
   +    return $entity->label();
   

   I confirmed this protected method is only used in core by the base implementation's render() method and that only the block implementation overrides it, and that in both cases, the label will get escaped anyway (so the checkPlain() is redundant). However, it is possible that a contrib or custom list builder implementation that used the label in a different way could have an XSS vulnerability following this patch, because this patch does change the documented behavior of the method. It is a correct change (see discussion on #2503963: XSS in Quick Edit: entity title is not safely encoded), but it might merit its own change record for this reason.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
   @@ -235,7 +234,7 @@ public function getReferenceableEntities($match = NULL, $match_operator = 'CONTA
   -      $options[$bundle][$entity_id] = SafeMarkup::checkPlain($entity->label());
   +      $options[$bundle][$entity_id] = $entity->label();
   

   This is similarly changing the documented behavior of SelectionInterface::getReferenceableEntities(), which says (emphasis added):

   A nested array of entities, the first level is keyed by the entity bundle, which contains an array of entity labels (safe HTML), keyed by the entity ID.

   This is also a public method and, being Entity Reference and therefore JavaScripty, something we should check very carefully for potential XSS.

https://twitter.com/xjmdrupal/status/313805301931446273

One suggestion I had for issue management here was to split off all the things that are straight conversions of checkPlain() to Html::escape() because there is no risk of introducing XSS vulnerabilities whatsoever, and the only thing they need to be checked for is the introduction of double- (edit:single- or double-) escaping bugs that lack test coverage. That could land first and make the rest more manageable.

Divide and conquer! +1 to #121

#2557519: Remove many usages SafeMarkup::checkPlain() and replace with Html::escape() landed. So, soon this patch will be much easier to review :)

I didn't see any discussion about this in the issue so how are we going to support the PHP templating engine if we start trusting on the Twig autoescape?

@lauriii I think the short answer is that ship sailed long ago.

This is blocking a critical: #2549393: Remove SafeMarkup::escape() and rely more on Twig autoescaping

Here's a reroll after #2555931: Add #plain_text to escape text in render arrays.

After chatting with @alexpott here's a new issue and patch up for review, nice and straightforward: #2559605: Convert SafeMarkup::checkPlain() in render arrays to #plain_text

Given that last patch will fatal error now that we don't use the Markup class anymore, here's a combined patch with #2559605: Convert SafeMarkup::checkPlain() in render arrays to #plain_text that does a simple search and replace on all other instances of #markup + Markup::SAFE_STRATEGY_ESCAPE with #plain_text. Which we can't just do as some things will be double escaped, so this will give a few test failures.

Maybe let's keep future patches in this issue synchronized with #2559605: Convert SafeMarkup::checkPlain() in render arrays to #plain_text and do a reverse patch as soon as it goes in?

reverting those

per #145

Just blocked on #2549393: Remove SafeMarkup::escape() and rely more on Twig autoescaping anymore, the other 3 issues have been commited now.

I have updated all the CRs mentioned in #94 except https://www.drupal.org/node/2153775 and https://www.drupal.org/node/1762604 where I wasn't sure what should be done.

#156 thanks for updating the CRs. I'm not totally sure about some of them, though. In [#2372691], the change was essentially

from

$this->assertRaw(String::checkPlain($string1));
$this->assertNoRaw(String::checkPlain($string2));

to

$this->assertRaw(Html::escape($string1));
$this->assertNoRaw(Html::escape($string2));

In some of these, given the newness of Html::escape do we maybe want to keep both example of what not to use? In other words, people reading the CR may see Html::escape and not think the same thing applies to an equivalent usage of String::checkPlain? Especially since we are just deprecating SafeMarkup::checkPlain and not removing it altogether.

I updated that CR as an example.

I don't understand why we would keep any references to checkPlain() in any change record. If someone doesn't know what Html::escape() is, they will look it up.

Oh, I read https://www.drupal.org/node/2372691 and I think I get it. If the change record is specifically about updating D7 code, then yes, having the D7 "before" snippet makes sense.

Just regarding the issue summary about nl2br, l did a fix here that's pretty handy for that case.
#2554755: BasicStringFormatter's nl2br() forces escaping *and* filtering — improve this