From 7d6b348d0b6cd85b474a53299dc378d58c70b865 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Tue, 18 Sep 2018 08:54:44 +1000
Subject: [PATCH] SECURITY: correct XSS on long topic titles

---
 app/models/topic.rb       |  8 ++++----
 spec/models/topic_spec.rb | 12 +++++++++---
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/app/models/topic.rb b/app/models/topic.rb
index c0bbb021630..e2e0fb449c8 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -221,8 +221,9 @@ class Topic < ActiveRecord::Base
     unless skip_callbacks
       ensure_topic_has_a_category
     end
+
     if title_changed?
-      write_attribute :fancy_title, Topic.fancy_title(title)
+      write_attribute(:fancy_title, Topic.fancy_title(title))
     end
 
     if category_id_changed? || new_record?
@@ -346,10 +347,9 @@ class Topic < ActiveRecord::Base
   end
 
   def self.fancy_title(title)
-    escaped = ERB::Util.html_escape(title)
-    return unless escaped
+    return unless escaped = ERB::Util.html_escape(title)
     fancy_title = Emoji.unicode_unescape(HtmlPrettify.render(escaped))
-    fancy_title.length > Topic.max_fancy_title_length ? title : fancy_title
+    fancy_title.length > Topic.max_fancy_title_length ? escaped : fancy_title
   end
 
   def fancy_title
diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb
index df1b61225dc..5e28d0d618a 100644
--- a/spec/models/topic_spec.rb
+++ b/spec/models/topic_spec.rb
@@ -296,10 +296,17 @@ describe Topic do
       expect(topic_image.fancy_title).to eq("Topic with &lt;img src=&lsquo;something&rsquo;&gt; image in its title")
     end
 
+    it "always escapes title" do
+      topic_script.title = topic_script.title + "x" * Topic.max_fancy_title_length
+      expect(topic_script.fancy_title).to eq(ERB::Util.html_escape(topic_script.title))
+      # not really needed, but just in case
+      expect(topic_script.fancy_title).not_to include("<script>")
+    end
+
   end
 
   context 'fancy title' do
-    let(:topic) { Fabricate.build(:topic, title: "\"this topic\" -- has ``fancy stuff''") }
+    let(:topic) { Fabricate.build(:topic, title: %{"this topic" -- has ``fancy stuff''}) }
 
     context 'title_fancy_entities disabled' do
       before do
@@ -319,7 +326,6 @@ describe Topic do
       it "converts the title to have fancy entities and updates" do
         expect(topic.fancy_title).to eq("&ldquo;this topic&rdquo; &ndash; has &ldquo;fancy stuff&rdquo;")
         topic.title = "this is my test hello world... yay"
-        topic.user.save!
         topic.save!
         topic.reload
         expect(topic.fancy_title).to eq("This is my test hello world&hellip; yay")
@@ -336,7 +342,7 @@ describe Topic do
       end
 
       it "works with long title that results in lots of entities" do
-        long_title = "NEW STOCK PICK: PRCT - LAST PICK UP 233%, NNCO.................................................................................................................................................................. ofoum"
+        long_title = "NEW STOCK PICK: PRCT - LAST PICK UP 233%, NNCO#{"." * 150} ofoum"
         topic.title = long_title
 
         expect { topic.save! }.to_not raise_error