Introduce HtmlEscapedText and remove SafeMarkup::setMultiple() and SafeMarkup::getAll() and remove the static safeStrings list

This patch also concludes #2569699-41: Remove SafeMarkup::checkPlain() for Drupal 9.0.x and seems to be OK. Do we need an update on the docs at https://api.drupal.org/api/drupal/core!lib!Drupal!Component!Utility!SafeMarkup.php/class/SafeMarkup/8?

Great idea, let's don't break even more contrib modules. Removing checkPlain doens't add additional security.

Comment #42 in issue #2569699

The CR also needs to be updated to include these.

fix title

I made a first pass at updating the CR.

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -51,82 +38,19 @@ class SafeMarkup {
+    return $string instanceOf SafeStringInterface;

Do we need an is_object() check here? It seems it's ok for scalar variables but not sure if there is any edge case.

Ah so less double escaping with EscapedString now :)

"Escapes a string for HTML display." is pretty clear, and I think we already document on the interface where *not* to use it, but should we do so here as well?

Nitpicks:

1. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -51,82 +38,19 @@ class SafeMarkup {
   +   * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
   +   *   Check if an object implements
   +   *   \Drupal\Component\Utility\SafeStringInterface instead.
   

   Should we more literally say this?

   Instead, you should just check if a variable is an instance of \Drupal\Component\Utility\SafeStringInterface
   

2. +++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
   @@ -270,8 +159,7 @@ public function __toString() {
   - * SafeMarkupTestSafeString is used to mark text as safe because
   - * SafeMarkup::$safeStrings is a global static that affects all tests.
   + * SafeMarkupTestSafeString is used to mark text as safe.
     */
   

   Remove the "mark" terminology here?

+++ b/core/lib/Drupal/Component/Utility/EscapedString.php
@@ -0,0 +1,32 @@
+class EscapedString implements SafeStringInterface {
+  use SafeStringTrait;
+

don't use this trait or the ::create() method will return any object implementing SafeStringInterface unchanged instead of escaping it.

Working on a fix for this now.

inline the methods including a valid ::create() and 2 little test cases.

ok, great

oops - missing method. need a constructor instead.

per @alexpott also directly test the class to make sure it behaves the same and simplfy the data provider.

Also, need to use the Component test class, not the core class in the test.

1. +++ b/core/lib/Drupal/Component/Utility/EscapedString.php
   @@ -0,0 +1,67 @@
   + * @deprecated Will be removed before Drupal 9.0.0. Rely on Twig's
   + *   auto-escaping feature, or use the @link theme_render #plain_text @endlink
   + *   key when constructing a render array that contains plain text in order to
   + *   use the renderer's auto-escaping feature. If neither of these are
   + *   possible, \Drupal\Component\Utility\Html::escape() can be used in places
   + *   where explicit escaping is needed.
   + */
   

   This should not be deprecated, for 9.0.0 given that tokens will use it

2. +++ b/core/lib/Drupal/Component/Utility/EscapedString.php
   @@ -0,0 +1,67 @@
   +  /**
   +   * Returns the string length.
   +   *
   +   * @return int
   +   *   The length of the string.
   +   */
   ...
   +  /**
   +   * Returns a representation of the object for use in JSON serialization.
   +   *
   +   * @return string
   +   *   The safe string content.
   +   */
   

   {@inheritdoc}

3. +++ b/core/lib/Drupal/Component/Utility/EscapedString.php
   @@ -0,0 +1,67 @@
   +  }
   +}
   

   Not a nitpick at all: we need a new line

4. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -51,82 +38,19 @@ class SafeMarkup {
      public static function isSafe($string, $strategy = 'html') {
        // Do the instanceof checks first to save unnecessarily casting the object
        // to a string.
   ...
   -  public static function getAll() {
   -    return static::$safeStrings;
   +    return $string instanceOf SafeStringInterface;
      }
    
      /**
   

   beatiful

5. +++ b/core/lib/Drupal/Core/Form/FormCache.php
   @@ -169,13 +169,6 @@ protected function loadCachedFormState($form_build_id, FormStateInterface $form_
          $form_state->setBuildInfo($build_info);
   

   It is not 100% obvious why we need to reset the build info, given that we almost doesn't change $build_info

6. +++ b/core/phpunit.xml.dist
   @@ -46,8 +46,6 @@
   -    <listener class="\Drupal\Tests\Listeners\SafeMarkupSideEffects">
   -    </listener>
   

   OMG

+++ b/core/includes/batch.inc
index 16353d8..9feb1ac 100644
--- a/core/includes/form.inc

--- a/core/includes/form.inc
+++ b/core/includes/form.inc

Form.inc: has a reference to -safe_strings still

looks great

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -51,82 +38,19 @@ class SafeMarkup {
+   * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
+   *   Instead, you should just check if a variable is an instance of
+   *   \Drupal\Component\Utility\SafeStringInterface.
    */
   public static function isSafe($string, $strategy = 'html') {

And yet, core still calls this from a bunch of places, such as from TwigExtension::escapeFilter(), theme_render_and_autoescape(), PlaceholderTrait::placeholderEscape(), views_pre_render_views_form_views_form(), and more.

Do we want to change all those usages here or in a follow-up? If in a follow-up, is that issue created already? If not, can someone create it?

https://www.drupal.org/node/2549395 has been updated, removing tag.

updating remaining tasks in the issue summary

Note: This issue would also help a lot with #2567257: hook_tokens() $sanitize option incompatible with Html sanitisation requirements

How does this interact with the SafeString behaviour we added to https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Template%... ?

Looks like we'd:

- escape in EscapedString::__toString()
- convert to plain text with PlainTextOutput::renderFromHtml() (because it was detected as a SafeString)
- escape in AttributeString

at the moment.

Regardless of the answer to that, it would be documented best with explicit test coverage of that interaction.

Sad that we won't get rid of SafeMarkup::checkPlain() but yay for getting rid of the safe list!

So it isn't changed because SafeMarkup::isSafe() will return TRUE for something returned from SafeMarkup::checkPlain() both before and after this patch.

I guess that is fine. However my other question here is whether we should add EscapedStringInterface so we can type hint/instanceof for that in places like this.

+++ b/core/lib/Drupal/Component/Utility/EscapedStringInterface.php
@@ -0,0 +1,27 @@
+   * Returns an escaped string..

Nitpick for the commit, two dots.

+++ b/core/lib/Drupal/Component/Utility/EscapedString.php
@@ -0,0 +1,55 @@
+class EscapedString implements EscapedStringInterface {

I'm curious whether for API similarity we should provide a ::create() method much like SafeStringTrait provides.

Strangely enough in my local this patch is not applying even after pulling from 8.0.x.
Testbot however is applying with no issues.

+++ b/core/lib/Drupal/Component/Utility/EscapedStringInterface.php<br>@@ -0,0 +1,27 @@<br>+&nbsp;&nbsp; * Returns an escaped string..
#43: agreed. Please remove 1 dot. I would write the patch if my local was not misbehaving :-)

Issue title still says escapedstring - that seems fine especially if it implements markupinterface

Are we expecting to add this to the API after all, i.e. does Token still need this? If we just mark it @internal, then it doesn't really matter what we call it :)

So in this case the input can be either plain text or markup, but the output will always be HTML-escaped HTML text. So it seems this can be used for two purposes:

1. Converting plain text to HTML text (in which case we have a naming problem as we can't say EscapableMarkup to be consistent with FormattableMarkup et al)
2. Escaping HTML markup and turning it into HTML text (in which case EscapableMarkup would be sort-of correct and sort-of consistent with the other naming)

EscapedString is confusing but I can't really come up with a better name due to the nature of the thing. I'd still prefer it if this had the word Markup in it somehow, EscapedMarkup at least hints that it implements MarkupInterface and that the __toString outputs HTML-escaped text.

Well I guess as long as they're @internal EscapedString or EscapableString work despite the ugliness.

Reviewed this and I think this looks great! It is great to finally get rid of the safe list, which seemed impossible only a month ago.

1. Discussed the naming on IRC with catch, alexpott and others - EscapableString was the best we could come up with so I think we can just go with that. It's not @internal so do we need to further document its document its behavior and use cases a bit in a followup?
2. The SafeMarkup class docs are now incorrect - can we rewrite this to something like:
   

    * Contains deprecated functionality related to sanitization of markup.
    *
    * @deprecated Will be removed before Drupal 9.0.0. Use the appropriate
    * @link sanitization sanitization functions @endlink or the
    * @link theme_render theme and render systems @endlink so that the output can
    * be themed, escaped, and altered properly.

3. Should we get rid of the @ingroup sanitization on the deprecated SafeMarkup methods?

RTBC assuming this will come back green

@alexpott, FWIW, no batch nor multiform tests failed that I could see in #1825952-139: Turn on twig autoescape by default when the safe list was first added (or the sandbox results earlier). But I may have missed an implicit fail.

I think this needs to be critical actually. The patch committed in #2570431: Document that certain (non-"href") attribute values in t() and SafeMarkup::format() are not supported and may be insecure assumed the patch here is already in core. Without it, the advice it gives in the documentation is currently wrong/insecure. For example:

   *   - @variable: When the placeholder replacement value is:
   *     - A string, the replaced value in the returned string will be sanitized
   *       using \Drupal\Component\Utility\Html::escape().

Minor (and could be fixed on commit):

   public static function isSafe($string, $strategy = 'html') {
     // Do the instanceof checks first to save unnecessarily casting the object
     // to a string.
-    return $string instanceOf MarkupInterface || isset(static::$safeStrings[(string) $string][$strategy]) ||
-      isset(static::$safeStrings[(string) $string]['all']);
-  }
....
+    return $string instanceOf MarkupInterface;

The code comment is wrong now and could just be removed.

Would it be ok to not add EscapableStringInterface here if nothing in this patch uses it? HEAD doesn't have any other sub-interfaces of MarkupInterface, and I'm not 100% convinced we want one. If we decide on it being useful in #2576523: Add an EscapedStringInterface and use it in Attribute, can we add it in that issue?

As far as naming EscapableString, what about HtmlEscapedText instead? I don't quite follow the nuances in #50. As far as I understand it, if it's "html escaped", then it is text: in the sense of it would all become a single text node within an HTML DOM. Whether the unescaped version of the string has "<" and ">" characters in it doesn't matter, or does it?

"needs review" for #61. If the answer to #61 is no, then this can be re-RTBC'd.

I agree that EscapableString is a not a great name. HtmlEscapedText or HtmlEscapedString would be better.

I also agree with effulgentsia that this MarkupInterface interface seems a bit weird, and really does it make any sense to have other implementations?

Liking that last class docs improvement and the name works for me as well. As opposed to some of our other string value objects the name refers to the __toString output, but in a way that's still clearer than "EscapableString" :)

Back to RTBC considering #64 is green and #66 looks to have renamed them all, so this should come back green as well.

Pushed to 8.0.x!

I'm assuming there are CRs that need updating, but if I'm wrong about that, then please untag and mark fixed.

https://www.drupal.org/node/2549395 is already mentioning this issue and looks more or less up-to-date, at least as far as this issue is concerned.

https://www.drupal.org/node/2549395 is great, but I think the intro of the "Escaping markup (or, how to check_plain() in Drupal 8)" section and its option #3 could be improved a bit. For example,

The most direct replacement for SafeMarkup::checkPlain() is Html::escape(), except that it does not mark the resulting string as safe.

With this issue, the most direct replacement is now new HtmlEscapedText() and that does mark the string as safe. And actually the "mark as safe" terminology might now be wrong as well, since MarkupInterface now has a slightly different semantics than "safe".

Html::escape() and SafeMarkup::format() for render arrays and HTML output

Those aren't symmetrical options, because the output of SafeMarkup::format() is a MarkupInterface object, but the output of Html::escape() is not. Should we change this recommendation to new HtmlEscapedText and new FormattableMarkup instead? Do we want a section on when to use Html::escape() vs. when to use new HtmlEscapedText?

The answers to the above questions aren't obvious (at least not to me), so that's why I'm asking them here instead of updating the CR myself. In fact, should we open a new issue for figuring this out, or does it make sense to keep using this one?

Additionally, it would be great if someone read through all the CRs in https://www.drupal.org/list-changes/published/drupal?keywords_descriptio... and flagged either in this issue or some other one things that are no longer accurate or recommended.

I'm familiar with wording changes in this area, so I will start on #75. but i dont know all the answers, so ping to collaborate. :)

I didn't answer/make a change for the "Those aren't symmetrical options..." concern. I'm leaning toward opening up another issue for that (normal task?)

for easier reviewing, here are changes

https://www.drupal.org/node/2549395/revisions/view/8949021/8949983

and going through the list https://www.drupal.org/list-changes/published/drupal?keywords_descriptio... from @effulgentsia
(no change to https://www.drupal.org/node/2575573 )
(no change to https://www.drupal.org/node/2574697 )
https://www.drupal.org/node/2571689/revisions/view/8928627/8949999
https://www.drupal.org/node/2560027/revisions/view/8924353/8950021
https://www.drupal.org/node/2549107/revisions/view/8838371/8950037
(no change to https://www.drupal.org/node/2559263 )
https://www.drupal.org/node/2506757/revisions/view/8838455/8950069
(no change to https://www.drupal.org/node/2457593 )
https://www.drupal.org/node/2392803/revisions/view/8838279/8950075 <- not sure what to change the new recommendation to
https://www.drupal.org/node/2445441/revisions/view/8865509/8950101 <- might need more old stuff like examples using SafeMarkup::format() updated
https://www.drupal.org/node/2296163/revisions/view/8928695/8950169 <- a main change record that could use review, and I think a better example of "simple concatenations"
https://www.drupal.org/node/2302363/revisions/view/8838291/8950187
https://www.drupal.org/node/2153775/revisions/view/8844725/8950197
https://www.drupal.org/node/2067859/revisions/view/8844663/8950205
https://www.drupal.org/node/1312890/revisions/view/8837809/8950209

edited the previous comments to have links to all the revisions. They need review.

Also, created #2579655: Document when to use Html::escape() vs. when to use new HtmlEscapedText

1. The example linked for SafeMarkup::replace() , views_pre_render_views_form_views_form() uses SafeMarkup::isSafe which it shouldn't. At least a () note or find another example.

2. Alternatively, it is possible to escape output in JavaScript instead. has no code example.

Now on to https://www.drupal.org/node/2296163

3. renderPlain is an extremely poor name and we should open a new issue and add a much better name! Anyone sane would expect *plain text* to come out of it but it renders HTML? WTF!

4. ['list_style' => 'comma-list'], is it possible to specify the delimiter or is it hardwired comma

5. FormattableMarkup "will return an object" should mention "implementing MarkupInterface" just to reinforce it's MarkupInterface as mentioned in the SafeMarkup::isSafe replacement

6. If you have a genuine use-case for script tags or other markup stripped by Xss::filerAdmin(), you will need to use proper templates or other APIs such as the Library API. <= example? Isn't library api for attaching libraries? how is this relevant? I am baffled.

1.
opened #2579691: Remove usages of SafeMarkup::isSafe()
and #2579697: Remove usages of SafeMarkup::checkPlain() in documentation
and #2579701: remove SafeMarkup class
and added a note about the example using a deprecated method https://www.drupal.org/node/2549395/revisions/view/8949983/8950651

3.
opened #2579709: rename renderPlain to something by making renderPlain a wrapper and adding another better named method

4.
core/modules/system/templates/item-list.html.twig
puts the value of the list type in the css, (so it is not hardcoded)
but, comma is the only list type core uses,
and we have some css for it in
core/themes/bartik/css/components/item-list.css
core/themes/classy/css/components/item-list.css
core/themes/seven/css/components/menus-and-lists.css

5.
and added the object Interface type for clarification
https://www.drupal.org/node/2296163/revisions/view/8950169/8950449

2.
https://www.drupal.org/node/2549395/revisions/view/8950651/8950667

added a javascript Drupal.formatString example, but
I’m not sure if the javascript note is general or part of “4. Non-HTML responses" drupal.org/node/2549395
and looking at the issue that was linked, I'm not sure if that is appropriate,
or if
return '<span class="field">' + Drupal.checkPlain(settings.fieldLabel) + '</span>' + Drupal.checkPlain(settings.entityLabel);
in Drupal.theme.quickeditEntityToolbarLabel in core/modules/quickedit/js/theme.js
would be better.

I will try and find something for 6. next, ping to help. :) I'm not sure how successful I will be.
[edit:]
#2273925: Ensure #markup is XSS escaped in Renderer::doRender() might have a clue.
core/lib/Drupal/Core/Render/theme.api.php

 * - #markup: Specifies that the array provides HTML markup directly. Unless
 *   the markup is very simple, such as an explanation in a paragraph tag, it
 *   is normally preferable to use #theme or #type instead, so that the theme
 *   can customize the markup. Note that the value is passed through
 *   \Drupal\Component\Utility\Xss::filterAdmin(), which strips known XSS
 *   vectors while allowing a permissive list of HTML tags that are not XSS
 *   vectors. (I.e, <script> and <style> are not allowed.) See
 *   \Drupal\Component\Utility\Xss::$adminTags for the list of tags that will
 *   be allowed. If your markup needs any of the tags that are not in this
 *   whitelist, then you can implement a theme hook and template file and/or
 *   an asset library. Aternatively, you can use the render array key
 *   #allowed_tags to alter which tags are filtered.

About 2.
formatString example (now in the CR), maybe is not best example. Because formatString not always escapes the text, depends on the arguments as you see here:
http://cgit.drupalcode.org/drupal/tree/core/misc/drupal.js#n296

I would go for your example of .checkPlain

About #83 question. I would consider the JavaScript note to be part of 4. Non HTML responses. It's where it has more sense.

If you are delivering non-escaped HTML from the server to be escaped only in JavaScript (via AJAX), you are doing it wrong

If you are on it, Cathy.. this text needs fix " There are three possibilities:" (Now there are actually 4 possibilities)

@corbacho - we should remove the ! placeholder in the JS api also. I think that was overlooked.

I'm away for a few hours. CR improvements open to anyone. :) edit away! (post revisions links and questions here please)

Great work by yesCT fixing up the CR and opening follow-up issues. I think may be ready to called this fixed assuming the CR update is all that remained?

I fixed "three possibilities" -> "four possibilities"

For JS turns out we already have #2570101: Remove !placeholder support from Drupal.formatString

Great!
I replaced the JavaScript example to use Drupal.checkPlain() in the CR, I find it more suitable.

Ha, the bot chokes on the patch already being applied. Let's call this fixed then?

thanks!

https://www.drupal.org/node/2296163/revisions/view/8950449/8950977 is my best guess right now at addressing 6.

(note, bot is setting to needs work, cause it is trying to apply the patch that was already committed. not sure how to trick it. maybe an empty patch file?) [edit: cross post]

Thanks a lot yesct!

Good enough for me to have this fixed. Who can update https://api.drupal.org/api/drupal/core!lib!Drupal!Component!Utility!Safe... pages ?

@Novitsh, you need to file core patches for that.