Simplifying SafeString a bit: From a XSS HTML security point of view, treat clean() and purify() the same.

No longer run a safe HTML string through the HTML purifier (since it's already marked as safe).

This also addresses the issue of calling purify() when no purifier is installed. In that case, we'd run clean() on a clean string (double HTML encoding).

If this approach doesn't work out, we can still modify the fallback code of purify() to check if the string is already clean before calling clean() instead of purify().

2 files changed, 8 insertions, 9 deletions

diff --git a/modules/gallery/libraries/SafeString.php b/modules/gallery/libraries/SafeString.php
index 3328fed5..ba3a8ffd 100644
--- a/modules/gallery/libraries/SafeString.php
+++ b/modules/gallery/libraries/SafeString.php
@@ -24,13 +24,11 @@
 class SafeString_Core {
   private $_raw_string;
   protected $_is_safe_html = false;
-  protected $_is_purified_html = false;
 
   /** Constructor */
   function __construct($string) {
     if ($string instanceof SafeString) {
       $this->_is_safe_html = $string->_is_safe_html;
-      $this->_is_purified_html = $string->_is_purified_html;
       $string = $string->unescaped();
     }
     $this->_raw_string = (string) $string;
@@ -49,14 +47,13 @@ class SafeString_Core {
    */
   static function purify($string) {
     if ($string instanceof SafeString) {
-      if ($string->_is_purified_html) {
+      if ($string->_is_safe_html) {
         return $string;
       } else {
         $string = $string->unescaped();
       }
     }
     $safe_string = self::of_safe_html(self::_purify_for_html($string));
-    $safe_string->_is_purified_html = true;
     return $safe_string;
   }
 
@@ -135,11 +132,7 @@ class SafeString_Core {
    * @return the string escaped for use in HTML.
    */
   function purified_html() {
-    if ($this->_is_purified_html) {
-      return $this;
-    } else {
-      return self::purify($this);
-    }
+    return self::purify($this);
   }
 
   /**
diff --git a/modules/gallery/tests/SafeString_Test.php b/modules/gallery/tests/SafeString_Test.php
index fdab5c58..2c07d934 100644
--- a/modules/gallery/tests/SafeString_Test.php
+++ b/modules/gallery/tests/SafeString_Test.php
@@ -106,6 +106,12 @@ class SafeString_Test extends Unit_Test_Case {
     $this->assert_equal($expected, $safe_string_2);
   }
 
+  public function purify_safe_html_test() {
+    $safe_string = SafeString::of_safe_html("hello <p  >world</p>");
+    $actual = SafeString::purify($safe_string);
+    $this->assert_equal("hello <p  >world</p>", $actual);
+  }
+
   public function of_fluid_api_test() {
     $escaped_string = SafeString::of("Foo's bar")->for_js();
     $this->assert_equal('"Foo\'s bar"', $escaped_string);