Merge pull request bernat#192 from guigs/master

Refactor sanitize to escape text only when rendering

lib/assets/javascripts/best_in_place.js

@@ -39,10 +39,14 @@ BestInPlaceEditor.prototype = {
       to_display = this.original_content;
     }
     else {
-      to_display = this.element.html();
+      if (this.sanitize) {
+        to_display = this.element.text();
+      } else {
+        to_display = this.element.html();
+      }
     }
 
-    var elem = this.isNil ? "-" : this.element.html();
+    var elem = this.isNil ? "-" : this.sanitize ? this.element.text() : this.element.html();
     this.oldValue = elem;
     this.display_value = to_display;
     jQuery(this.activator).unbind("click", this.clickHandler);
@@ -51,8 +55,8 @@ BestInPlaceEditor.prototype = {
   },
 
   abort : function() {
-    if (this.isNil) this.element.html(this.nil);
-    else            this.element.html(this.oldValue);
+    if (this.isNil) this.element.text(this.nil);
+    else            this.element.text(this.oldValue);
     jQuery(this.activator).bind('click', {editor: this}, this.clickHandler);
     this.element.trigger(jQuery.Event("best_in_place:abort"));
     this.element.trigger(jQuery.Event("best_in_place:deactivate"));
@@ -95,7 +99,7 @@ BestInPlaceEditor.prototype = {
     } else if (this.formType == "checkbox") {
       editor.element.html(this.getValue() ? this.values[1] : this.values[0]);
     } else {
-      editor.element.html(this.getValue() !== "" ? this.getValue() : this.nil);
+      editor.element.text(this.getValue() !== "" ? this.getValue() : this.nil);
     }
     editor.element.trigger(jQuery.Event("best_in_place:update"));
   },
@@ -172,10 +176,10 @@ BestInPlaceEditor.prototype = {
   },
 
   initNil: function() {
-    if (this.element.html() === "")
+    if (this.element.text() === "")
     {
       this.isNil = true;
-      this.element.html(this.nil);
+      this.element.text(this.nil);
     }
   },
 
@@ -185,12 +189,6 @@ BestInPlaceEditor.prototype = {
 
   // Trim and Strips HTML from text
   sanitizeValue : function(s) {
-    if (this.sanitize)
-    {
-      var tmp = document.createElement("DIV");
-      tmp.innerHTML = s;
-      s = jQuery.trim(tmp.textContent || tmp.innerText).replace(/"/g, '"');
-    }
    return jQuery.trim(s);
   },
 
@@ -220,9 +218,9 @@ BestInPlaceEditor.prototype = {
   loadSuccessCallback : function(data) {
     var response = jQuery.parseJSON(jQuery.trim(data));
     if (response !== null && response.hasOwnProperty("display_as")) {
-      this.element.attr("data-original-content", this.element.html());
-      this.original_content = this.element.html();
-      this.element.html(response["display_as"]);
+      this.element.attr("data-original-content", this.element.text());
+      this.original_content = this.element.text();
+      this.element.text(response["display_as"]);
     }
     this.element.trigger(jQuery.Event("ajax:success"), data);
 
@@ -232,7 +230,7 @@ BestInPlaceEditor.prototype = {
   },
 
   loadErrorCallback : function(request, error) {
-    this.element.html(this.oldValue);
+    this.element.text(this.oldValue);
 
     this.element.trigger(jQuery.Event("best_in_place:error"), [request, error])
     this.element.trigger(jQuery.Event("ajax:error"));

lib/best_in_place/helper.rb

@@ -62,9 +62,9 @@ def best_in_place(object, field, opts = {})
       end
       if !opts[:sanitize].nil? && !opts[:sanitize]
         out << " data-sanitize='false'>"
-        out << sanitize(value.to_s, :tags => %w(b i u s a strong em p h1 h2 h3 h4 h5 ul li ol hr pre span img br), :attributes => %w(id class href))
+        out << value.to_s
       else
-        out << ">#{sanitize(value.to_s, :tags => nil, :attributes => nil)}"
+        out << ">#{h(value.to_s)}"
       end
       out << "</span>"
       raw out

lib/best_in_place/test_helpers.rb

@@ -1,11 +1,13 @@
 module BestInPlace
   module TestHelpers
+
+    include ActionView::Helpers::JavaScriptHelper
 
     def bip_area(model, attr, new_value)
       id = BestInPlace::Utils.build_best_in_place_id model, attr
       page.execute_script <<-JS
         jQuery("##{id}").click();
-        jQuery("##{id} form textarea").val('#{new_value}');
+        jQuery("##{id} form textarea").val('#{escape_javascript new_value.to_s}');
         jQuery("##{id} form textarea").blur();
       JS
     end
@@ -14,7 +16,7 @@ def bip_text(model, attr, new_value)
       id = BestInPlace::Utils.build_best_in_place_id model, attr
       page.execute_script <<-JS
         jQuery("##{id}").click();
-        jQuery("##{id} input[name='#{attr}']").val('#{new_value}');
+        jQuery("##{id} input[name='#{attr}']").val('#{escape_javascript new_value.to_s}');
         jQuery("##{id} form").submit();
       JS
     end

spec/integration/js_spec.rb

@@ -745,6 +745,26 @@
     end
   end
 
+  it "should keep the same value after multipe edits" do
+    @user.save!
+
+    retry_on_timeout do
+      visit double_init_user_path(@user)
+
+      bip_area @user, :description, "A <a href=\"http://google.es\">link in this text</a> not sanitized."
+      visit double_init_user_path(@user)
+
+      page.should have_link("link in this text", :href => "http://google.es")
+
+      id = BestInPlace::Utils.build_best_in_place_id @user, :description
+      page.execute_script <<-JS
+        $("##{id}").click();
+      JS
+
+      page.find("##{id} textarea").value.should eq("A <a href=\"http://google.es\">link in this text</a> not sanitized.")
+    end
+  end
+
   it "should display single- and double-quotes in values appropriately" do
     @user.height = %{5' 6"}
     @user.save!
@@ -782,4 +802,53 @@
     end
   end
 
+  it "should escape javascript in test helpers" do
+    @user.save!
+
+    retry_on_timeout do
+      visit user_path(@user)
+
+      bip_text @user, :last_name, "Other '); alert('hi');"
+      sleep 1
+
+      @user.reload
+      @user.last_name.should eq("Other '); alert('hi');")
+    end
+  end
+
+  it "should save text in database without encoding" do
+    @user.save!
+
+    retry_on_timeout do
+      visit user_path(@user)
+
+      bip_text @user, :last_name, "Other \"thing\""
+      sleep 1
+
+      @user.reload
+      @user.last_name.should eq("Other \"thing\"")
+    end
+  end
+
+  it "should not strip html tags" do
+    @user.save!
+
+    retry_on_timeout do
+      visit user_path(@user)
+
+      bip_text @user, :last_name, "<script>alert('hi');</script>"
+      within("#last_name") { page.should have_content("<script>alert('hi');</script>") }
+
+      visit user_path(@user)
+
+      id = BestInPlace::Utils.build_best_in_place_id @user, :last_name
+      page.execute_script <<-JS
+        $("##{id}").click();
+      JS
+
+      page.find("##{id} input").value.should eq("<script>alert('hi');</script>")
+    end
+
+  end
+
 end