Issue #2506133 by alexpott, joelpittet, dawehner, pwolanin: Replace SafeMarkup::set() in \Drupal\Core\Template\Attribute

core/lib/Drupal/Component/Utility/SafeMarkup.php

@@ -82,7 +82,7 @@ class SafeMarkup {
   /**
    * Checks if a string is safe to output.
    *
-   * @param string $string
+   * @param string|\Drupal\Component\Utility\SafeStringInterface $string
    *   The content to be checked.
    * @param string $strategy
    *   The escaping strategy. See self::set(). Defaults to 'html'.
@@ -91,7 +91,9 @@ class SafeMarkup {
    *   TRUE if the string has been marked secure, FALSE otherwise.
    */
   public static function isSafe($string, $strategy = 'html') {
-    return isset(static::$safeStrings[(string) $string][$strategy]) ||
+    // Do the instanceof checks first to save unnecessarily casting the object
+    // to a string.
+    return $string instanceOf SafeStringInterface || isset(static::$safeStrings[(string) $string][$strategy]) ||
       isset(static::$safeStrings[(string) $string]['all']);
   }
 

core/lib/Drupal/Component/Utility/SafeStringInterface.php

@@ -0,0 +1,36 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Component\Utility\SafeStringInterface.
+ */
+
+namespace Drupal\Component\Utility;
+
+/**
+ * Marks an object's __toString() method as returning safe markup.
+ *
+ * This interface should only be used on objects that emit known safe strings
+ * from their __toString() method. If there is any risk of the method returning
+ * user-entered data that has not been filtered first, it must not be used.
+ *
+ * @internal
+ *   This interface is marked as internal because it should only be used by
+ *   objects used during rendering. Currently, there is no use case for this
+ *   interface in contrib or custom code.
+ *
+ * @see \Drupal\Component\Utility\SafeMarkup::set()
+ * @see \Drupal\Component\Utility\SafeMarkup::isSafe()
+ * @see \Drupal\Core\Template\TwigExtension::escapeFilter()
+ */
+interface SafeStringInterface {
+
+  /**
+   * Returns a safe string.
+   *
+   * @return string
+   *   The safe string.
+   */
+  public function __toString();
+
+}

core/lib/Drupal/Core/Template/Attribute.php

@@ -7,7 +7,7 @@
 
 namespace Drupal\Core\Template;
 
-use Drupal\Component\Utility\SafeMarkup;
+use Drupal\Component\Utility\SafeStringInterface;
 
 /**
  * Collects, sanitizes, and renders HTML attributes.
@@ -42,8 +42,7 @@ use Drupal\Component\Utility\SafeMarkup;
  * The attribute keys and values are automatically sanitized for output with
  * htmlspecialchars() and the entire attribute string is marked safe for output.
  */
-class Attribute implements \ArrayAccess, \IteratorAggregate {
+class Attribute implements \ArrayAccess, \IteratorAggregate, SafeStringInterface {
-
   /**
    * Stores the attribute data.
    *
@@ -259,10 +258,7 @@ class Attribute implements \ArrayAccess, \IteratorAggregate {
         $return .= ' ' . $rendered;
       }
     }
-    // The implementations of AttributeValueBase::render() call
+    return $return;
-    // htmlspecialchars() on the attribute name and value so we are confident
-    // that the return value can be set as safe.
-    return SafeMarkup::set($return);
   }
 
   /**

core/lib/Drupal/Core/Template/TwigExtension.php

@@ -13,6 +13,7 @@
 namespace Drupal\Core\Template;
 
 use Drupal\Component\Utility\SafeMarkup;
+use Drupal\Component\Utility\SafeStringInterface;
 use Drupal\Core\Render\RendererInterface;
 use Drupal\Core\Routing\UrlGeneratorInterface;
 use Drupal\Core\Theme\ThemeManagerInterface;
@@ -397,7 +398,7 @@ class TwigExtension extends \Twig_Extension {
     }
 
     // Keep Twig_Markup objects intact to support autoescaping.
-    if ($autoescape && $arg instanceOf \Twig_Markup) {
+    if ($autoescape && ($arg instanceOf \Twig_Markup || $arg instanceOf SafeStringInterface)) {
       return $arg;
     }
 

core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php

@@ -69,6 +69,18 @@ class SafeMarkupTest extends UnitTestCase {
     $this->assertFalse(SafeMarkup::isSafe($returned, 'all'), 'String set with "html" provider is not safe for "all"');
   }
 
+  /**
+   * Tests SafeMarkup::isSafe() with different objects.
+   *
+   * @covers ::isSafe
+   */
+  public function testIsSafe() {
+    $safe_string = $this->getMock('\Drupal\Component\Utility\SafeStringInterface');
+    $this->assertTrue(SafeMarkup::isSafe($safe_string));
+    $string_object = new SafeMarkupTestString('test');
+    $this->assertFalse(SafeMarkup::isSafe($string_object));
+  }
+
   /**
    * Tests SafeMarkup::setMultiple().
    *
@@ -258,3 +270,17 @@ class SafeMarkupTest extends UnitTestCase {
   }
 
 }
+
+class SafeMarkupTestString {
+
+  protected $string;
+
+  public function __construct($string) {
+    $this->string = $string;
+  }
+
+  public function __toString() {
+    return $this->string;
+  }
+
+}

core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php

@@ -99,4 +99,43 @@ class TwigExtensionTest extends UnitTestCase {
     $this->assertEquals('test_theme', $result);
   }
 
+  /**
+   * Tests the escaping of objects implementing SafeStringInterface.
+   *
+   * @covers ::escapeFilter
+   */
+  public function testSafeStringEscaping() {
+    $renderer = $this->getMock('\Drupal\Core\Render\RendererInterface');
+    $twig = new \Twig_Environment(NULL, array(
+      'debug' => TRUE,
+      'cache' => FALSE,
+      'autoescape' => TRUE,
+      'optimizations' => 0
+    ));
+    $twig_extension = new TwigExtension($renderer);
+
+    // By default, TwigExtension will attempt to cast objects to strings.
+    // Ensure objects that implement SafeStringInterface are unchanged.
+    $safe_string = $this->getMock('\Drupal\Component\Utility\SafeStringInterface');
+    $this->assertSame($safe_string, $twig_extension->escapeFilter($twig, $safe_string, 'html', 'UTF-8', TRUE));
+
+    // Ensure objects that do not implement SafeStringInterface are escaped.
+    $string_object = new TwigExtensionTestString("<script>alert('here');</script>");
+    $this->assertSame('&lt;script&gt;alert(&#039;here&#039;);&lt;/script&gt;', $twig_extension->escapeFilter($twig, $string_object, 'html', 'UTF-8', TRUE));
+  }
+
+}
+
+class TwigExtensionTestString {
+
+  protected $string;
+
+  public function __construct($string) {
+    $this->string = $string;
+  }
+
+  public function __toString() {
+    return $this->string;
+  }
+
 }