Create SafeMarkup::filterSeparator and make safe_join safe by default

> Consider not marking safe any strings that are lower than 3 characters.

Most excellent suggestion!

This is just a sketch.

index 56bd518..8f52e5d 100644
--- a/core/lib/Drupal/Component/Utility/SafeMarkup.php

--- a/core/lib/Drupal/Component/Utility/SafeMarkup.php
+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php

@@ -153,6 +153,17 @@ public static function checkAdminXss($string) {
   /**
+   * @todo Document this function.
+   *
+   * @param string $string
+   *
+   * @return SafeMarkup|string
+   */
+  public static function filterSeparator($string) {
+    return twig_drupal_join_filter(array($string));
+  }
+

Mh, so we add a dependency from a component into a drupal specific function, not sure whether this is right and it should not be the other way round. Move the code of twig_drupal_join_filter here

On top of that, it could be also helpful to write a dedicated test for it.

.

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -153,6 +153,21 @@ public static function checkAdminXss($string) {
+   *
+   * Strings smaller than 3 characters will not be set as safe.
+   *

We should explain why would never be a problem: ... there is no single html tag which has less than 3 chars: <b> ... already needs 3

@dawehner: Thank you for reviewing.

Great!

1. +++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
   @@ -101,4 +101,23 @@ public function testInvalidSetMultiple() {
   +  /**
   +   * Tests SafeMarkup::filterSeparator().
   +   *
   +   * @covers ::filterSeparator
   +   */
   +  public function testFilterSeparator() {
   +    $returned = SafeMarkup::filterSeparator('<br />');
   +    $this->assertEquals('<br />', $returned, 'Separator string "<br />" was not modified.');
   +    $this->assertTrue(SafeMarkup::isSafe($returned), 'Separator string "<br />" was marked as safe');
   +    $returned = SafeMarkup::filterSeparator('---');
   +    $this->assertEquals('---', $returned, 'Separator string "---" was not modified');
   +    $this->assertTrue(SafeMarkup::isSafe($returned), 'Separator string "---" was marked as safe');
   +    $returned = SafeMarkup::filterSeparator('<');
   +    $this->assertEquals('<', $returned, 'Separator string "<" was not modified');
   +    $this->assertFalse(SafeMarkup::isSafe($returned), 'Separator string "<" was not marked as safe');
   +    $returned = SafeMarkup::filterSeparator('<script>alert("hello");</script>');
   +    $this->assertEquals('alert("hello");', $returned, 'Separator string "<script>alert("hello");</script>" was modified');
   +    $this->assertTrue(SafeMarkup::isSafe($returned), 'Modified separator string "alert("hello");" was marked as safe');
   +  }
   

   This test would be neater and more maintainable as a provider / test combination. But we can do that in followup as the patch adds the necessary test coverage.

2. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -153,6 +153,23 @@ public static function checkAdminXss($string) {
   +    return strlen($string) < 3 ? $string : Xss::filterAdmin($string);
   

   Unicode::strlen()? And should we not move this further up into SafeMarkup::set()?

3. What about twig_drupal_join_filter()? Shouldn't that use this?

This is Unicode::strlen() and calling SafeMarkup::filterSeparator() from twig_drupal_join_filter(). There is still a problem here: twig_drupal_join_filter() sets the whole string as safe even though SafeMarkup::filterSeparator() may have considered the separator unsafe (such as "<").

As for whether we should move this to SafeMarkup::set(), perhaps.

Another issue is that in the first place we had been and still are using Xss:filterAdmin(), which itself would replace a lone "<" even if that was the actual character desired for the separator. It isn't clear if we want to allow that.

I suppose SafeMarkup::filterSeparator() could return an escaped string if less than 3 characters.

I cannot reproduce a problem. I created Views with separators and the separators were escaped properly, including greater than or less than signs.

Note that depending on how you view the source code of a page, the less than/greater than might get processed and displayed as symbol.

Xss::filterAdmin does not mark '>' as safe, it escapes it, and marks that as safe. I tested this against HEAD and '>' gets escaped. Can we close this?

Rerolled

#NW for #13.3

Dicussed with @xjm, @webchick, @catch, @effulgentsia. Based on #19 and #20 and looking at twig_drupal_join_filter() I think we can downgrade this issue.

Leaving to @cilefen to confirm if this can be closed.

From the summary:

However this is problematic as that means a malicious admin user can essentially get e.g. '>' marked as safe with that.

Manual testing showed that partial tags as separators are escaped, and this is why:

Field separators are passed through Xss::filterAdmin which restricts the allowed tags to 'a', 'em', 'strong', 'cite', 'blockquote', 'code', 'ul', 'ol', 'li', 'dl', 'dt', 'dd' by calling Xss:filter which in turn replaces offending partial tags.