From 6e9f5f5584dfad284121ab868118772a2f077d87 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Fri, 25 Jul 2014 12:15:43 +1000
Subject: [PATCH] SECURITY: fix XSS in excerpt parser

---
 lib/excerpt_parser.rb               |  9 ++++++++-
 spec/components/pretty_text_spec.rb | 24 +++++++++++++++---------
 spec/models/user_profile_spec.rb    | 28 ++++++++++++++--------------
 3 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/lib/excerpt_parser.rb b/lib/excerpt_parser.rb
index 9afa2f2d991..03485ad812e 100644
--- a/lib/excerpt_parser.rb
+++ b/lib/excerpt_parser.rb
@@ -23,8 +23,15 @@ class ExcerptParser < Nokogiri::XML::SAX::Document
     me.excerpt
   end
 
+  def escape_attribute(v)
+    v.gsub("&", "&amp;")
+     .gsub("\"", "&#34;")
+     .gsub("<", "&lt;")
+     .gsub(">", "&gt;")
+  end
+
   def include_tag(name, attributes)
-    characters("<#{name} #{attributes.map{|k,v| "#{k}='#{v}'"}.join(' ')}>", false, false, false)
+    characters("<#{name} #{attributes.map{|k,v| "#{k}=\"#{escape_attribute(v)}\""}.join(' ')}>", false, false, false)
   end
 
   def start_element(name, attributes=[])
diff --git a/spec/components/pretty_text_spec.rb b/spec/components/pretty_text_spec.rb
index add42031657..3dc0e9fbb43 100644
--- a/spec/components/pretty_text_spec.rb
+++ b/spec/components/pretty_text_spec.rb
@@ -75,6 +75,15 @@ describe PrettyText do
 
   describe "Excerpt" do
 
+    it "sanitizes attempts to inject invalid attributes" do
+
+      spinner = "<a href=\"http://thedailywtf.com/\" data-bbcode=\"' class='fa fa-spin\">WTF</a>"
+      PrettyText.excerpt(spinner, 20).should match_html spinner
+
+      spinner = %q{<a href="http://thedailywtf.com/" title="' class=&quot;fa fa-spin&quot;&gt;&lt;img src='http://thedailywtf.com/Resources/Images/Primary/logo.gif"></a>}
+      PrettyText.excerpt(spinner, 20).should match_html spinner
+    end
+
     context "images" do
 
       it "should dump images" do
@@ -94,8 +103,8 @@ describe PrettyText do
       end
 
       it "should keep spoilers" do
-        PrettyText.excerpt("<div class='spoiler'><img src='http://cnn.com/a.gif'></div>", 100).should == "<span class='spoiler'>[image]</span>"
-        PrettyText.excerpt("<span class='spoiler'>spoiler</div>", 100).should == "<span class='spoiler'>spoiler</span>"
+        PrettyText.excerpt("<div class='spoiler'><img src='http://cnn.com/a.gif'></div>", 100).should match_html "<span class='spoiler'>[image]</span>"
+        PrettyText.excerpt("<span class='spoiler'>spoiler</div>", 100).should match_html "<span class='spoiler'>spoiler</span>"
       end
     end
 
@@ -104,7 +113,7 @@ describe PrettyText do
     end
 
     it "should preserve links" do
-      PrettyText.excerpt("<a href='http://cnn.com'>cnn</a>",100).should == "<a href='http://cnn.com'>cnn</a>"
+      PrettyText.excerpt("<a href='http://cnn.com'>cnn</a>",100).should match_html "<a href='http://cnn.com'>cnn</a>"
     end
 
     it "should deal with special keys properly" do
@@ -125,15 +134,15 @@ describe PrettyText do
     end
 
     it "should not count the surrounds of a link" do
-      PrettyText.excerpt("<a href='http://cnn.com'>cnn</a>",3).should == "<a href='http://cnn.com'>cnn</a>"
+      PrettyText.excerpt("<a href='http://cnn.com'>cnn</a>",3).should match_html "<a href='http://cnn.com'>cnn</a>"
     end
 
     it "uses an ellipsis instead of html entities if provided with the option" do
-      PrettyText.excerpt("<a href='http://cnn.com'>cnn</a>", 2, text_entities: true).should == "<a href='http://cnn.com'>cn...</a>"
+      PrettyText.excerpt("<a href='http://cnn.com'>cnn</a>", 2, text_entities: true).should match_html "<a href='http://cnn.com'>cn...</a>"
     end
 
     it "should truncate links" do
-      PrettyText.excerpt("<a href='http://cnn.com'>cnn</a>",2).should == "<a href='http://cnn.com'>cn&hellip;</a>"
+      PrettyText.excerpt("<a href='http://cnn.com'>cnn</a>",2).should match_html "<a href='http://cnn.com'>cn&hellip;</a>"
     end
 
     it "doesn't extract empty quotes as links" do
@@ -294,9 +303,6 @@ describe PrettyText do
       PrettyText.cook("**你hello**").should match_html "<p><strong>你hello</strong></p>"
     end
 
-    it "sanitizes attempts to inject invalid attributes" do
-      PrettyText.cook("<a href=\"http://thedailywtf.com/\" data-bbcode=\"' class='fa fa-spin\">WTF</a>").should == "<p><a href=\"http://thedailywtf.com/\" rel=\"nofollow\">WTF</a></p>"
-    end
   end
 
 end
diff --git a/spec/models/user_profile_spec.rb b/spec/models/user_profile_spec.rb
index fb3beeccf47..470dbdae60f 100644
--- a/spec/models/user_profile_spec.rb
+++ b/spec/models/user_profile_spec.rb
@@ -57,7 +57,7 @@ describe UserProfile do
     end
 
     context 'with a user that has a link in their bio' do
-      let(:user_profile) { Fabricate.build(:user_profile, bio_raw: "im sissy and i love http://ponycorns.com") }
+      let(:user_profile) { Fabricate.build(:user_profile, bio_raw: "I love http://discourse.org") }
       let(:user) do
         user = Fabricate.build(:user, user_profile: user_profile)
         user_profile.user = user
@@ -66,22 +66,22 @@ describe UserProfile do
 
       let(:created_user) do
         user = Fabricate(:user)
-        user.user_profile.bio_raw = 'im sissy and i love http://ponycorns.com'
+        user.user_profile.bio_raw = 'I love http://discourse.org'
         user.user_profile.save!
         user
       end
 
       it 'includes the link as nofollow if the user is not new' do
         user.user_profile.send(:cook)
-        expect(user_profile.bio_excerpt).to eq("im sissy and i love <a href='http://ponycorns.com' rel='nofollow'>http://ponycorns.com</a>")
-        expect(user_profile.bio_processed).to eq("<p>im sissy and i love <a href=\"http://ponycorns.com\" rel=\"nofollow\">http://ponycorns.com</a></p>")
+        expect(user_profile.bio_excerpt).to match_html("I love <a href='http://discourse.org' rel='nofollow'>http://discourse.org</a>")
+        expect(user_profile.bio_processed).to match_html("<p>I love <a href=\"http://discourse.org\" rel=\"nofollow\">http://discourse.org</a></p>")
       end
 
       it 'removes the link if the user is new' do
         user.trust_level = TrustLevel.levels[:newuser]
         user_profile.send(:cook)
-        expect(user_profile.bio_excerpt).to eq("im sissy and i love http://ponycorns.com")
-        expect(user_profile.bio_processed).to eq("<p>im sissy and i love http://ponycorns.com</p>")
+        expect(user_profile.bio_excerpt).to match_html("I love http://discourse.org")
+        expect(user_profile.bio_processed).to eq("<p>I love http://discourse.org</p>")
       end
 
       context 'leader_links_no_follow is false' do
@@ -90,22 +90,22 @@ describe UserProfile do
         it 'includes the link without nofollow if the user is trust level 3 or higher' do
           user.trust_level = TrustLevel.levels[:leader]
           user_profile.send(:cook)
-          expect(user_profile.bio_excerpt).to eq("im sissy and i love <a href='http://ponycorns.com'>http://ponycorns.com</a>")
-          expect(user_profile.bio_processed).to eq("<p>im sissy and i love <a href=\"http://ponycorns.com\">http://ponycorns.com</a></p>")
+          expect(user_profile.bio_excerpt).to match_html("I love <a href='http://discourse.org'>http://discourse.org</a>")
+          expect(user_profile.bio_processed).to match_html("<p>I love <a href=\"http://discourse.org\">http://discourse.org</a></p>")
         end
 
         it 'removes nofollow from links in bio when trust level is increased' do
           created_user.change_trust_level!(:leader)
-          expect(created_user.user_profile.bio_excerpt).to eq("im sissy and i love <a href='http://ponycorns.com'>http://ponycorns.com</a>")
-          expect(created_user.user_profile.bio_processed).to eq("<p>im sissy and i love <a href=\"http://ponycorns.com\">http://ponycorns.com</a></p>")
+          expect(created_user.user_profile.bio_excerpt).to match_html("I love <a href='http://discourse.org'>http://discourse.org</a>")
+          expect(created_user.user_profile.bio_processed).to match_html("<p>I love <a href=\"http://discourse.org\">http://discourse.org</a></p>")
         end
 
         it 'adds nofollow to links in bio when trust level is decreased' do
           created_user.trust_level = TrustLevel.levels[:leader]
           created_user.save
           created_user.change_trust_level!(:regular)
-          expect(created_user.user_profile.bio_excerpt).to eq("im sissy and i love <a href='http://ponycorns.com' rel='nofollow'>http://ponycorns.com</a>")
-          expect(created_user.user_profile.bio_processed).to eq("<p>im sissy and i love <a href=\"http://ponycorns.com\" rel=\"nofollow\">http://ponycorns.com</a></p>")
+          expect(created_user.user_profile.bio_excerpt).to match_html("I love <a href='http://discourse.org' rel='nofollow'>http://discourse.org</a>")
+          expect(created_user.user_profile.bio_processed).to match_html("<p>I love <a href=\"http://discourse.org\" rel=\"nofollow\">http://discourse.org</a></p>")
         end
       end
 
@@ -115,8 +115,8 @@ describe UserProfile do
         it 'includes the link with nofollow if the user is trust level 3 or higher' do
           user.trust_level = TrustLevel.levels[:leader]
           user_profile.send(:cook)
-          expect(user_profile.bio_excerpt).to eq("im sissy and i love <a href='http://ponycorns.com' rel='nofollow'>http://ponycorns.com</a>")
-          expect(user_profile.bio_processed).to eq("<p>im sissy and i love <a href=\"http://ponycorns.com\" rel=\"nofollow\">http://ponycorns.com</a></p>")
+          expect(user_profile.bio_excerpt).to match_html("I love <a href='http://discourse.org' rel='nofollow'>http://discourse.org</a>")
+          expect(user_profile.bio_processed).to match_html("<p>I love <a href=\"http://discourse.org\" rel=\"nofollow\">http://discourse.org</a></p>")
         end
       end
     end