SECURITY: Cross-Site Scripting in Category and Group Settings

app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs

@@ -31,7 +31,7 @@
     <label>
       {{fa-icon "envelope-o"}}
       {{i18n 'category.email_in'}}
-      {{text-field value=category.email_in}}
+      {{text-field class="email-in" value=category.email_in}}
     </label>
   </section>
 {{/if}}

app/models/category.rb

@@ -316,12 +316,14 @@ SQL
   def email_in_validator
     return if self.email_in.blank?
     email_in.split("|").each do |email|
+
+      escaped = Rack::Utils.escape_html(email)
       if !Email.is_valid?(email)
-        self.errors.add(:base, I18n.t('category.errors.invalid_email_in', email: email))
+        self.errors.add(:base, I18n.t('category.errors.invalid_email_in', email: escaped))
       elsif group = Group.find_by_email(email)
-        self.errors.add(:base, I18n.t('category.errors.email_already_used_in_group', email: email, group_name: group.name))
+        self.errors.add(:base, I18n.t('category.errors.email_already_used_in_group', email: escaped, group_name: Rack::Utils.escape_html(group.name)))
       elsif category = Category.where.not(id: self.id).find_by_email(email)
-        self.errors.add(:base, I18n.t('category.errors.email_already_used_in_category', email: email, category_name: category.name))
+        self.errors.add(:base, I18n.t('category.errors.email_already_used_in_category', email: escaped, category_name: Rack::Utils.escape_html(category.name)))
       end
     end
   end

app/models/group.rb

@@ -82,13 +82,15 @@ class Group < ActiveRecord::Base
 
   def incoming_email_validator
     return if self.automatic || self.incoming_email.blank?
+
     incoming_email.split("|").each do |email|
+      escaped = Rack::Utils.escape_html(email)
       if !Email.is_valid?(email)
-        self.errors.add(:base, I18n.t('groups.errors.invalid_incoming_email', email: email))
+        self.errors.add(:base, I18n.t('groups.errors.invalid_incoming_email', email: escaped))
       elsif group = Group.where.not(id: self.id).find_by_email(email)
-        self.errors.add(:base, I18n.t('groups.errors.email_already_used_in_group', email: email, group_name: group.name))
+        self.errors.add(:base, I18n.t('groups.errors.email_already_used_in_group', email: escaped, group_name: Rack::Utils.escape_html(group.name)))
       elsif category = Category.find_by_email(email)
-        self.errors.add(:base, I18n.t('groups.errors.email_already_used_in_category', email: email, category_name: category.name))
+        self.errors.add(:base, I18n.t('groups.errors.email_already_used_in_category', email: escaped, category_name: Rack::Utils.escape_html(category.name)))
       end
     end
   end

spec/models/category_spec.rb

@@ -309,7 +309,7 @@ describe Category do
     it "renames the definition when renamed" do
       @category.update_attributes(name: 'Troutfishing')
       @topic.reload
-      expect(@topic.title).to match /Troutfishing/
+      expect(@topic.title).to match(/Troutfishing/)
     end
 
     it "doesn't raise an error if there is no definition topic to rename (uncategorized)" do
@@ -593,4 +593,38 @@ describe Category do
     end
   end
 
+  describe "validate email_in" do
+    let(:user) { Fabricate(:user) }
+
+    it "works with a valid email" do
+      expect(Category.new(name: 'test', user: user, email_in: 'test@example.com').valid?).to eq(true)
+    end
+
+    it "adds an error with an invalid email" do
+      category = Category.new(name: 'test', user: user, email_in: '<sup>test</sup>')
+      expect(category.valid?).to eq(false)
+      expect(category.errors.full_messages.join).not_to match(/<sup>/)
+    end
+
+    context "with a duplicate email in a group" do
+      let(:group) { Fabricate(:group, name: 'testgroup', incoming_email: 'test@example.com') }
+
+      it "adds an error with an invalid email" do
+        category = Category.new(name: 'test', user: user, email_in: group.incoming_email)
+        expect(category.valid?).to eq(false)
+      end
+    end
+
+    context "with duplicate email in a category" do
+      let!(:category) { Fabricate(:category, user: user, name: '<b>cool</b>', email_in: 'test@example.com') }
+
+      it "adds an error with an invalid email" do
+        category = Category.new(name: 'test', user: user, email_in: "test@example.com")
+        expect(category.valid?).to eq(false)
+        expect(category.errors.full_messages.join).not_to match(/<b>/)
+      end
+    end
+
+  end
+
 end

test/javascripts/acceptance/category-edit-test.js.es6

@@ -1,7 +1,10 @@
 import DiscourseURL from 'discourse/lib/url';
 import { acceptance } from "helpers/qunit-helpers";
 
-acceptance("Category Edit", { loggedIn: true });
+acceptance("Category Edit", {
+  loggedIn: true,
+  settings: { email_in: true }
+});
 
 test("Can open the category modal", assert => {
   visit("/c/bug");
@@ -41,3 +44,16 @@ test("Change the topic template", assert => {
     assert.equal(DiscourseURL.redirectedTo, '/c/bug', 'it does one of the rare full page redirects');
   });
 });
+
+test("Error Saving", assert => {
+  visit("/c/bug");
+
+  click('.edit-category');
+  click('.edit-category-settings');
+  fillIn('.email-in', 'duplicate@example.com');
+  click('#save-category');
+  andThen(() => {
+    assert.ok(visible('#modal-alert'));
+    assert.equal(find('#modal-alert').html(), "duplicate email");
+  });
+});

test/javascripts/helpers/create-pretender.js.es6

@@ -102,7 +102,13 @@ export default function() {
     });
 
     this.put('/categories/:category_id', request => {
+
       const category = parsePostData(request.requestBody);
+
+      if (category.email_in === "duplicate@example.com") {
+        return response(422, {"errors": ['duplicate email']});
+      }
+
       return response({category});
     });