Rename clean_js to js_string and have it return a complete JS string (with delimiters) instead of just the string contents.

Benefits: Using json_encode(), which is very robust. And as a user, it's clearer how to use this API compared to what it was before.

5 files changed, 14 insertions, 22 deletions

diff --git a/modules/gallery/helpers/MY_html.php b/modules/gallery/helpers/MY_html.php
index 75114898..4522d01c 100644
--- a/modules/gallery/helpers/MY_html.php
+++ b/modules/gallery/helpers/MY_html.php
@@ -65,11 +65,11 @@ class html extends html_Core {
    *
    * Example:<pre>
    *   <script type="text/javascript>"
-   *     var some_js_var = "<?= html::clean_js($php_var) ?>";
+   *     var some_js_string = <?= html::js_string($php_string) ?>;
    *   </script>
    * </pre>
    */
-  static function clean_js($string) {
+  static function js_string($string) {
     return SafeString::of($string)->for_js();
   }
 
diff --git a/modules/gallery/libraries/SafeString.php b/modules/gallery/libraries/SafeString.php
index 9614a213..0767a665 100644
--- a/modules/gallery/libraries/SafeString.php
+++ b/modules/gallery/libraries/SafeString.php
@@ -92,17 +92,17 @@ class SafeString_Core {
   }
 
   /**
-   * Safe for use in JavaScript.
+   * Safe for use as JavaScript string.
    *
    * Example:<pre>
    *   <script type="text/javascript>"
-   *     var some_js_var = "<?= $php_var->for_js() ?>";
+   *     var some_js_var = <?= $php_var->for_js() ?>;
    *   </script>
    * </pre>
    * @return the string escaped for use in JavaScript.
    */
   function for_js() {
-    return self::_escape_for_js($this->_raw_string);
+    return json_encode((string) $this->_raw_string);
   }
 
   /**
@@ -152,14 +152,6 @@ class SafeString_Core {
     return html::specialchars($dirty_html);
   }
 
-  // Escapes special chars (quotes, backslash, etc.) with a backslash sequence.
-  private static function _escape_for_js($string) {
-    // From Smarty plugins/modifier.escape.php
-    // Might want to be stricter here.
-    return strtr($string,
-		 array('\\'=>'\\\\',"'"=>"\\'",'"'=>'\\"',"\r"=>'\\r',"\n"=>'\\n','</'=>'<\/'));
-  }
-
   // Purifies the string, removing any potentially malicious or unsafe HTML / JavaScript.
   private static function _purify_for_html($dirty_html) {
     if (empty(self::$_purifier)) {
diff --git a/modules/gallery/tests/Html_Helper_Test.php b/modules/gallery/tests/Html_Helper_Test.php
index a9903256..f5ce7fa4 100644
--- a/modules/gallery/tests/Html_Helper_Test.php
+++ b/modules/gallery/tests/Html_Helper_Test.php
@@ -40,9 +40,9 @@ class Html_Helper_Test extends Unit_Test_Case {
 			$safe_string_2);
   }
 
-  public function clean_js_test() {
-    $string = html::clean_js("hello's <p  >world</p>");
-    $this->assert_equal("hello\\'s <p  >world<\\/p>",
+  public function js_string_test() {
+    $string = html::js_string("hello's <p  >world</p>");
+    $this->assert_equal('"hello\'s <p  >world<\\/p>"',
 			$string);
   }
 
diff --git a/modules/gallery/tests/SafeString_Test.php b/modules/gallery/tests/SafeString_Test.php
index 0fc7f6f3..ede55240 100644
--- a/modules/gallery/tests/SafeString_Test.php
+++ b/modules/gallery/tests/SafeString_Test.php
@@ -49,7 +49,7 @@ class SafeString_Test extends Unit_Test_Case {
   public function for_js_test() {
     $safe_string = new SafeString('"<em>Foo</em>\'s bar"');
     $js_string = $safe_string->for_js();
-    $this->assert_equal('\\"<em>Foo<\\/em>\\\'s bar\\"',
+    $this->assert_equal('"\\"<em>Foo<\\/em>\'s bar\\""',
 			$js_string);
   }
 
@@ -96,21 +96,21 @@ class SafeString_Test extends Unit_Test_Case {
 
   public function of_fluid_api_test() {
     $escaped_string = SafeString::of("Foo's bar")->for_js();
-    $this->assert_equal("Foo\\'s bar", $escaped_string);
+    $this->assert_equal('"Foo\'s bar"', $escaped_string);
   }
 
   public function safestring_of_safestring_preserves_safe_status_test() {
     $safe_string = SafeString::of_safe_html("hello's <p>world</p>");
     $safe_string_2 = new SafeString($safe_string);
     $this->assert_equal("hello's <p>world</p>", $safe_string_2);
-    $this->assert_equal("hello\\'s <p>world<\\/p>", $safe_string_2->for_js());
+    $this->assert_equal('"hello\'s <p>world<\\/p>"', $safe_string_2->for_js());
   }
 
   public function safestring_of_safestring_preserves_html_safe_status_test() {
     $safe_string = SafeString::of_safe_html("hello's <p>world</p>");
     $safe_string_2 = new SafeString($safe_string);
     $this->assert_equal("hello's <p>world</p>", $safe_string_2);
-    $this->assert_equal("hello\\'s <p>world<\\/p>", $safe_string_2->for_js());
+    $this->assert_equal('"hello\'s <p>world<\\/p>"', $safe_string_2->for_js());
   }
 
   public function safestring_of_safestring_safe_status_override_test() {
diff --git a/modules/gallery/tests/Xss_Security_Test.php b/modules/gallery/tests/Xss_Security_Test.php
index b385580d..3a22afc1 100644
--- a/modules/gallery/tests/Xss_Security_Test.php
+++ b/modules/gallery/tests/Xss_Security_Test.php
@@ -188,7 +188,7 @@ class Xss_Security_Test extends Unit_Test_Case {
 	    if (self::_token_matches(array(T_DOUBLE_COLON, "::"), $tokens, $token_number + 1) &&
 		self::_token_matches(array(T_STRING), $tokens, $token_number + 2) &&
 		in_array($tokens[$token_number + 2][1],
-			 array("clean", "purify", "clean_js", "clean_attribute")) &&
+			 array("clean", "purify", "js_string", "clean_attribute")) &&
 		self::_token_matches("(", $tokens, $token_number + 3)) {
               // Not checking for mark_safe(). We want such calls to be marked dirty (thus reviewed).
 
@@ -198,7 +198,7 @@ class Xss_Security_Test extends Unit_Test_Case {
 	      $token_number += 3;
 	      $token = $tokens[$token_number];
 
-              if ("clean_js" == $method) {
+              if ("js_string" == $method) {
                 $frame->is_safe_js(true);
               } else {
                 $frame->is_safe_html(true);