Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter

1. +++ b/core/lib/Drupal/Component/Utility/Xss.php
   @@ -23,19 +23,13 @@ class Xss {
    
   +  protected static $htmlTags = array('a', 'em', 'strong', 'cite', 'blockquote', 'code', 'ul', 'ol', 'li', 'dl', 'dt', 'dd');
   

   I can haz documentation?

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -737,6 +737,36 @@ public function addCacheableDependency(array &$elements, $dependency) {
   +    if (SafeMarkup::isSafe($elements['#markup'])) {
   +      // Nothing to do as #markup is already marked as safe.
   +      $string = $elements['#markup'];
   +    }
   ...
   +    // @todo Do we need to clean up #markup_safe_strategy and #markup_xss_tags?
   +    return SafeString::create($string);
   

   Would it make sense to do an early return given that we already know its safeness?

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -737,6 +737,36 @@ public function addCacheableDependency(array &$elements, $dependency) {
   +    elseif ($strategy == $this::SAFE_MARKUP_ESCAPE) {
   

   opinions on $this vs. static:: ?

4. +++ b/core/modules/aggregator/src/ItemViewBuilder.php
   @@ -26,7 +26,8 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +          '#markup' => $entity->getDescription(),
   +          '#markup_xss_tags' => aggregator_xss_tags(),
   

   I'm not entirely sure whether making the markup_safe_strategy implicit is the right thing to do here

5. +++ b/core/modules/filter/src/Plugin/Filter/FilterCaption.php
   @@ -37,6 +37,7 @@ public function process($text, $langcode) {
   +      $renderer = \Drupal::service('renderer');
   

   Do you mind putting an inline doc about the type here?

I have some doubts/concerns about this patch.

1. +++ b/core/lib/Drupal/Component/Utility/Xss.php
   @@ -347,4 +343,13 @@ public static function getAdminTagList() {
   +   * Gets the standard list of html tags allowed by Xss::filter().
   ...
   +   *   The list of html tags allowed by Xss::filter().
   

   s/html/HTML/

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -737,6 +737,35 @@ public function addCacheableDependency(array &$elements, $dependency) {
      /**
   +   * Escapes or filters #markup as required.
   +   *
   +   * @param array $elements
   +   *   A render array with #markup set.
   +   *
   +   * @return \Drupal\Component\Utility\SafeStringInterface|string
   +   *   The escaped string wrapped in a SafeString object. If
   +   *   SafeMarkup::isSafe($elements['#markup']) returns TRUE, it won't be
   +   *   escaped again.
   +   */
   

   This is not yet documenting, not even mentioning #markup_safe_strategy and the possible values for that.

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -737,6 +737,35 @@ public function addCacheableDependency(array &$elements, $dependency) {
   +  protected function safeMarkup(array $elements) {
   

   "safeMarkup" is a strange method name IMO, because it doesn't have a verb.

4. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -13,6 +13,22 @@
      /**
   +   * #markup_safe_strategy indicating #markup should be filtered.
   +   *
   +   * @see \Drupal\Core\Render\Renderer::safeMarkup()
   +   * @see \Drupal\Component\Utility\Xss::filter()
   +   */
   +  const SAFE_MARKUP_XSS = 'xss';
   +
   +  /**
   +   * #markup_safe_strategy indicating #markup should be escaped.
   +   *
   +   * @see \Drupal\Core\Render\Renderer::safeMarkup()
   +   * @see \Drupal\Component\Utility\Html::encodeEntities()
   +   */
   +  const SAFE_MARKUP_ESCAPE = 'escape';
   

   It very much concerns me that we're now adding even escaping/filtering logic to Renderer.

   Isn't Renderer more complex than we'd like already?

   Is there really no other way?

5. +++ b/core/modules/aggregator/src/ItemViewBuilder.php
   @@ -26,7 +26,8 @@ public function buildComponents(array &$build, array $entities, array $displays,
   -          '#markup' => aggregator_filter_xss($entity->getDescription()),
   +          '#markup' => $entity->getDescription(),
   +          '#markup_xss_tags' => aggregator_xss_tags(),
   
   +++ b/core/modules/aggregator/src/Plugin/Field/FieldFormatter/AggregatorXSSFormatter.php
   @@ -36,7 +36,8 @@ public function viewElements(FieldItemListInterface $items) {
   -        '#markup' => aggregator_filter_xss($item->value),
   +        '#markup' => $item->value,
   +        '#markup_xss_tags' => aggregator_xss_tags(),
   

   I think these could be converted to #type => 'processed_text', and use a text format that uses filter_html to only allow the specified tags.

   That's really what the aggregator is doing here: consuming content from elsewhere (the content creator is not a person, but an external site), whose output needs to be filtered through the filter system, just like for any node that any content creator creates.

6. +++ b/core/modules/filter/src/Plugin/Filter/FilterCaption.php
   @@ -45,8 +47,8 @@ public function process($text, $langcode) {
   -        $caption = SafeMarkup::xssFilter($caption, array('a', 'em', 'strong', 'cite', 'code', 'br'));
   -
   +        $caption = ['#markup' => $caption, '#markup_xss_tags' => ['a', 'em', 'strong', 'cite', 'code', 'br']];
   +        $caption = $renderer->render($caption);
   

   It seems wrong to use the render system to do filtering in a filter.

7. +++ b/core/modules/filter/src/Plugin/Filter/FilterCaption.php
   @@ -64,7 +66,7 @@ public function process($text, $langcode) {
   -        $altered_html = drupal_render($filter_caption);
   +        $altered_html = $renderer->render($filter_caption);
   

   (This uses the render system to render some markup. Very different to the above.)

Interdiff review:

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -739,6 +739,18 @@ public function addCacheableDependency(array &$elements, $dependency) {
   +   * automatically escapes any html that is not known to be safe. Due to this
   

   s/html/HTML/

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -739,6 +739,18 @@ public function addCacheableDependency(array &$elements, $dependency) {
   +   * the renderer system needs to ensure that all markup it generates is marked
   

   "renderer system" should be either "Renderer" or "render system" :)

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -739,6 +739,18 @@ public function addCacheableDependency(array &$elements, $dependency) {
   +   * By default all #markup is filtered for XSS using the admin tag list. Render
   

   "filtered for XSS using" -> "filtered for XSS protection using" or maybe "filtered to protect against XSS using"

4. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -739,6 +739,18 @@ public function addCacheableDependency(array &$elements, $dependency) {
   +   * arrays can alter the list of tags to filter using the #markup_allowed_tags
   

   s/tags to filter/tags allowed by the filtering/

   ?

5. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -739,6 +739,18 @@ public function addCacheableDependency(array &$elements, $dependency) {
   +   * key. This value should be an array of tags that Xss::filter() would accept.
   ...
   +   * #markup_safe_strategy key to RendererInterface::SAFE_MARKUP_ESCAPE. If the
   

   s/key/property/

6. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -273,21 +273,21 @@
   + *   which tags are using to filter the string. The value should be an array of
   

   s/tags are using/tags are allowed when filtering/

   s/string/markup/ ?

Based on the above changes/discussions, more remarks on other parts of the patch:

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -737,6 +737,51 @@ public function addCacheableDependency(array &$elements, $dependency) {
   +   * @see \Drupal\Core\Render\RendererInterface::SAFE_MARKUP_ESCAPE
   

   Why no @see to the other constant?

2. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -13,6 +13,22 @@
   +  const SAFE_MARKUP_XSS = 'xss';
   ...
   +  const SAFE_MARKUP_ESCAPE = 'escape';
   

   I think MARKUP_SAFE_STRATEGY_something would be clearer, because it'd closely match #markup-safe_strategy.

3. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -271,10 +271,24 @@
   + *   @see \Drupal\Core\Render\RendererInterface::SAFE_MARKUP_ESCAPE
   

   Again only an @see to one of the two constants.

4. +++ b/core/modules/aggregator/aggregator.module
   @@ -157,16 +157,13 @@ function aggregator_cron() {
   + * Gets the permitted list of tags.
   ...
   + *   The array of permitted tags.
   

   s/permitted/allowed/

5. +++ b/core/modules/aggregator/aggregator.module
   @@ -157,16 +157,13 @@ function aggregator_cron() {
   +function aggregator_xss_tags() {
   

   s/xss/allowed/

I have no more remarks, besides one silly nit below. But I think this needs additional review by somebody else before it can be RTBC'd.

+++ b/core/modules/aggregator/aggregator.module
@@ -157,12 +157,12 @@ function aggregator_cron() {
-function aggregator_xss_tags() {
+function aggregator_allowed_tags() {

Should we prefix this with an underscore, to indicate it's not a public API?

Or at least add @internal?

@Wim Leers pinged me this issue. I think removing this method from SafeMarkup is an excellent idea, and it probably makes sense for render arrays to be able to control the escaping strategy of #markup.

I'm somewhat nervous about adding even more magic keys to the Render ArrayPI, but it helps to conceptualize a render array as an object-in-waiting.

I'm also nervous about anything that requires us to add even more renderPlain() calls (basically just using render arrays as "renderable markup objects" locally when the context doesn't support them).

I pinged @catch and @Fabianx for their feedback too.

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -737,6 +738,54 @@ public function addCacheableDependency(array &$elements, $dependency) {
+   * @see htmlspecialchars()

+++ b/core/lib/Drupal/Core/Render/RendererInterface.php
@@ -13,6 +13,22 @@
+   * @see htmlspecialchars()

Shouldn't these now also point to Html::escape()?

+++ b/core/lib/Drupal/Core/Render/RendererInterface.php
@@ -13,6 +13,22 @@
+  const MARKUP_SAFE_STRATEGY_FILTER = 'xss';
...
+  const MARKUP_SAFE_STRATEGY_ESCAPE = 'escape';

I'm wondering whether the constants should reflect that we deal with HTML for those cases ...

Discussed this in irc with alexpott and Wim Leers.

So the main concern I have with this is that search excepts, aggregator, contact messages are all extremely special cases - similar to Wim's point in #9. #markup isn't a special case, but we're adding a capability to it to support things that are.

However page titles, while also a special case aren't exactly esoteric.

So it really comes down to opening follow-ups to look at the places that require this, and see if we can do them differently (like using the filter system for aggregator and possibly search excerpts). Then maybe we can deprecate this in a minor release and/or remove in 9.x.

We also talked about adding a new #type, but that would then conflict with #markup.

In terms of the patch itself, I think it's fine.

Given #22 and #23, I think this is RTBC again.now RTBC!

14:40:34 <WimLeers> catch: do you still need to do an architectural review of https://www.drupal.org/node/2549791?
14:40:37 <Druplicon> https://www.drupal.org/node/2549791 => Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter => 23 comments, 11 IRC mentions
14:40:43 <WimLeers> (i.e. the issue we discussed this morning)
14:42:00 <catch> WimLeers: I could look again, but I feel like it's OK.
14:42:02 <WimLeers> ok
14:42:06 <WimLeers> catch: then I'm RTBC'ing.

Just throwing in a little request: Any chance the key could just be #allowed_tags instead of #markup_allowed_tags? or is the idea to discourage it's use? then I'm fine with it as is.

We don't do that with other types so I don't think it's a good to start that pattern here if that is the reason.

eg. #items isn't #item_list_items because it only works with #type => item_list.

#27: the difference is that #items is specific to #type => 'item_list', i.e. you have to already specify that specific #type. But #markup is special: it's something that Renderer itself directly supports. If you don't specify a #type, you can still use #markup. Therefore I think it's okay to be more explicit and different compared to the common case.

So I was about to commit this, but then I read Wim's comment and thought of #2012818: Remove #type 'markup'.

We also talked about adding a new #type, but that would then conflict with #markup.

A new #type would conflict with #markup, but if we moved #markup back to an element, then the new key support and accompanying logic can live in the render element. Also the original motivation for removing the element was because there was no distinction at all except more typing, but given the extra logic around #markup there's something to use there now. Not 100% sure it's better than the patch, but Wim Leers said in irc he'll try it.

And done, as described in #29. This also moves and renames the constants from RendererInterface::MARKUP_SAFE_STRATEGY_something to Markup::SAFE_STRATEGY_something.

And because of this, I think #27 now does make sense. Doing that next.

• #markup_allowed_tags → #allowed_tags
• #markup_safe_strategy → #safe_strategy

This would effectively revert the CR at https://www.drupal.org/node/2036237.

1. +++ b/core/modules/search/search.module
   @@ -725,7 +725,7 @@ function search_excerpt($keys, $text, $langcode = NULL) {
   -      '#markup_safe_strategy' => RendererInterface::MARKUP_SAFE_STRATEGY_ESCAPE,
   +      '#markup_safe_strategy' => Markup::SAFE_STRATEGY_ESCAPE,
   

   Nice!

2. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php
   @@ -112,6 +113,25 @@ protected function setUp() {
   +      ->will($this->returnCallback(function ($type) {
   

   You can use ->willReturnCallback if you like

2. I had that at one point, but lost it in my many attempts to minimize the changes in this area. Let's see if there are other nitpicks Alex or I can fix in one go :)

Yeah its still RTBC

RTBC++

(I just moved code.)

+++ b/core/lib/Drupal/Core/Render/Element/Markup.php
@@ -0,0 +1,108 @@
+ *

One issue - these docs are for a completely different class.

Should we just say "Provides a render element for HTML as a string, with sanitization." or something?

Ugh, sorry, I forgot to update that :(

+++ b/core/lib/Drupal/Core/Render/Element/Markup.php
@@ -13,12 +13,51 @@
+ * Usage example:
+ * @code
+ * $output['admin_filtered_string'] = array(
+ *   '#type' => 'markup',
+ *   '#markup' => '<em>This is filtered using the admin tag list</em>',
+ * );
+ * $output['filtered_string'] = array(
+ *   '#type' => 'markup',
+ *   '#markup' => '<em>This is filtered</em>',
+ *   '#allowed_tags' => ['strong'],
+ * );
+ * $output['escaped_string'] = array(
+ *   '#type' => 'markup',
+ *   '#markup' => '<em>This is escaped</em>',
+ *   '#safe_strategy' => Markup::SAFE_STRATEGY_ESCAPE,
+ * );
+ * @endcode

Excellent! :)

EDIT: dreditor WTF.

Thanks for the docs update.

Committed/pushed to 8.0.x, thanks!

Woot! Next up: #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping and #2549393: Remove SafeMarkup::escape() and rely more on Twig autoescaping.

In the CR, is it #markup_allowed_tags or #allowed_tags? It seems to contradict itself.

I fixed the change notice now so it uses #allowed_tags consistently.

However the change notice does a good job explaining how to do this with #markup but does not really explain much about how to replace SafeMarkup::xssFilter() elsewhere in a render array.

For example if I have this:

  $render_array = array(
    '#theme' => 'something',
    '#description' => $description,
  );

and if the #theme callback is generic enough that it does not impose restrictions on what can be passed in (e.g. plain text vs HTML) then before this issue the calling code could just call SafeMarkup::xssFilter($description) to have it be treated as HTML.

Now with that no longer an option, I'm not sure what you're supposed to do. This might work:

  $render_array = array(
    '#theme' => 'something',
    '#description' => array(
      '#markup' => $description,
    ),
  );

although the nested properties look a little strange.

However, if you do that, doesn't that mean the theme preprocess function (and anything else that alters or interacts with the render array along the way) now needs to consider the possibility that #description could be either a string or an array?

Or is the idea that it should always be an array, and you're not supposed to use strings in render API properties at all anymore?

I'm confused about how all this works and I think more info is needed in the change notice.

This patch that was committed in modules/search changed the return value of the search_excerpt() function from a string to a render array.

This is actually a fairly large API change. There is no change notice. There are contrib modules that use this function, such as Display Suite, so it absolutely needs a change notice. Please write one. The patch may also have changed other API functions, I haven't looked, but this change (a) should have been at least passed by the Search module maintainers and (b) doesn't have a change notice.

I took a look through the patch. aggregator_filter_xss() is apparently removed, and I cannot see a change notice for this either.

@alexpott, I don't see how those would work with my example in #49, where $description comes from an unknown source (e.g. user input) and is expected to contain HTML.

I guess it's technically possible to do something like this:

  $render_array = array(
    '#theme' => 'something',
    '#description' => SafeMarkup::format('@description', array(
      '@description' => Xss::filterAdmin($description),
    )),
  );

and it would work... but I can't imagine this is what we want to recommend.

In the example you give the $description would be escaped because Xss::filterAdmin() does not put it in the safe list.

My example has SafeMarkup::format() which would put it in the safe list.

I think that if the description contains markup we should be using a render array.

So you're basically suggesting what I had in #49, then?:

  $render_array = array(
    '#theme' => 'something',
    '#description' => array(
      '#markup' => $description,
    ),
  );

But if so, see the questions I had there. Basically we are saying that preprocess functions/etc are going to need to be very aware of this. If they just take a variable and treat it as a string, then the calling code can't really decide to send in HTML (at least not using the recommended, non-contortionist method).

Oh, yikes - I meant !description but in fact that doesn't treat it as safe either now. Anyway, we all agree it wasn't a good workaround anyway :)

We at least need something in the change notice about this since it's a common use case, but I'm not quite sure what to recommend. I think it basically means that if you're defining a theme implementation that you expect other modules to call, you need to consider that the calling code might pass in an array rather than a string and handle both cases (or require/document that they only pass in an array). Or decide on your level that this should never have HTML, in which case you can still decide to treat it as a string only.... Still a little confused how to do this in practice.

There are 2 unpublished change record drafts on this issue.

Are they correct?

There are still 2 unpublished change record drafts on this issue.

Are they correct?