XssFoliate correctly escapes html entities when stripping text from i…

…mplicitly-scrubbed fields. Closes #17.
flavorjones · Feb 1, 2010 · 03e781b · 03e781b
1 parent 688d2fd
commit 03e781b
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 11 deletions.
diff --git a/CHANGELOG.rdoc b/CHANGELOG.rdoc
@@ -1,5 +1,11 @@
 = Changelog
 
+== 0.4.4 (2010-02-01)
+
+Bug fixes:
+
+  * Loofah::XssFoliate was not properly escaping HTML entities when implicitly scrubbing a string attribute. GH #17
+
 == 0.4.3 (2010-01-29)
 
 Enhancements:

diff --git a/lib/loofah/xss_foliate.rb b/lib/loofah/xss_foliate.rb
@@ -180,7 +180,7 @@ def xss_foliate_fields # :nodoc:
 
           # :text if we're here
           fragment = Loofah.scrub_fragment(value, :strip)
-          self[field] = fragment.nil? ? "" : fragment.text
+          self[field] = fragment.nil? ? "" : fragment.to_s
         end
       end
 

diff --git a/test/test_ad_hoc.rb b/test/test_ad_hoc.rb
@@ -251,7 +251,7 @@ def test_removal_of_all_tags
       What's up <strong>doc</strong>?
     HTML
     stripped = Loofah.scrub_document(html, :prune).text
-    assert_equal "What's up doc?".strip, stripped.strip
+    assert_equal %Q(What\'s up doc?).strip, stripped.strip
   end
 
   def test_dont_remove_whitespace
@@ -268,5 +268,4 @@ def test_removal_of_entities
     html = "<p>this is &lt; that &quot;&amp;&quot; the other &gt; boo&apos;ya</p>"
     assert_equal 'this is < that "&" the other > boo\'ya', Loofah.scrub_document(html, :prune).text
   end
-
 end
diff --git a/test/test_xss_foliate.rb b/test/test_xss_foliate.rb
@@ -56,7 +56,7 @@ def new_post(overrides={})
       end
 
       context "when passed a symbol" do
-        should "do the right thing" do
+        should "calls the right scrubber" do
           assert_nothing_raised(ArgumentError) { Post.xss_foliate :prune => :plain_text }
           Loofah.expects(:scrub_fragment).with(HTML_STRING, :strip).once
           Loofah.expects(:scrub_fragment).with(PLAIN_TEXT, :prune).once
@@ -65,7 +65,7 @@ def new_post(overrides={})
       end
 
       context "when passed an array of symbols" do
-        should "do the right thing" do
+        should "calls the right scrubbers" do
           assert_nothing_raised(ArgumentError) {
             Post.xss_foliate :prune => [:plain_text, :html_string]
           }
@@ -76,7 +76,7 @@ def new_post(overrides={})
       end
 
       context "when passed a string" do
-        should "do the right thing" do
+        should "calls the right scrubber" do
           assert_nothing_raised(ArgumentError) { Post.xss_foliate :prune => 'plain_text' }
           Loofah.expects(:scrub_fragment).with(HTML_STRING, :strip).once
           Loofah.expects(:scrub_fragment).with(PLAIN_TEXT, :prune).once
@@ -85,7 +85,7 @@ def new_post(overrides={})
       end
 
       context "when passed an array of strings" do
-        should "do the right thing" do
+        should "calls the right scrubbers" do
           assert_nothing_raised(ArgumentError) {
             Post.xss_foliate :prune => ['plain_text', 'html_string']
           }
@@ -107,7 +107,7 @@ def new_post(overrides={})
           Loofah.expects(:scrub_fragment).with(HTML_STRING, :strip).once.returns(mock_doc)
           Loofah.expects(:scrub_fragment).with(PLAIN_TEXT, :strip).once.returns(mock_doc)
           Loofah.expects(:scrub_fragment).with(INTEGER_VALUE, :strip).never
-          mock_doc.expects(:text).twice
+          mock_doc.expects(:to_s).twice
           assert new_post.valid?
         end
       end
@@ -118,9 +118,11 @@ def new_post(overrides={})
         end
 
         should "not scrub omitted field" do
-          Loofah.expects(:scrub_fragment).with(HTML_STRING, :strip).once
+          mock_doc = mock
+          Loofah.expects(:scrub_fragment).with(HTML_STRING, :strip).once.returns(mock_doc)
           Loofah.expects(:scrub_fragment).with(PLAIN_TEXT, :strip).never
           Loofah.expects(:scrub_fragment).with(INTEGER_VALUE, :strip).never
+          mock_doc.expects(:to_s).once
           assert new_post.valid?
         end
       end
@@ -132,9 +134,11 @@ def new_post(overrides={})
           end
 
           should "not that field appropriately" do
-            Loofah.expects(:scrub_fragment).with(HTML_STRING, :strip).once
-            Loofah.expects(:scrub_fragment).with(PLAIN_TEXT, method).once
+            mock_doc = mock
+            Loofah.expects(:scrub_fragment).with(HTML_STRING, :strip).once.returns(mock_doc)
+            Loofah.expects(:scrub_fragment).with(PLAIN_TEXT, method).once.returns(mock_doc)
             Loofah.expects(:scrub_fragment).with(INTEGER_VALUE, :strip).never
+            mock_doc.expects(:to_s).twice
             assert new_post.valid?
           end
         end
@@ -167,5 +171,18 @@ def new_post(overrides={})
       end
     end
 
+    context "given an XSS attempt" do
+      setup do
+        Post.xss_foliate :strip => :html_string
+      end
+
+      should "escape html entities" do
+        hackattack = "&lt;script&gt;alert('evil')&lt;/script&gt;"
+        post = new_post :html_string => hackattack, :plain_text => hackattack
+        post.valid?
+        assert_equal "&lt;script&gt;alert('evil')&lt;/script&gt;", post.html_string
+        assert_equal "&lt;script&gt;alert('evil')&lt;/script&gt;", post.plain_text
+      end
+    end
   end
 end