From cf5b756b1ae93713c6b16ad04afd56bd5761627a Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Thu, 28 Jul 2016 11:57:30 -0400 Subject: [PATCH] SECURITY: Cross-Site Scripting in Category and Group Settings --- .../components/edit-category-settings.hbs | 2 +- app/models/category.rb | 8 +++-- app/models/group.rb | 8 +++-- spec/models/category_spec.rb | 36 ++++++++++++++++++- .../acceptance/category-edit-test.js.es6 | 18 +++++++++- .../helpers/create-pretender.js.es6 | 6 ++++ 6 files changed, 69 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs b/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs index 03b2bc4deef..cfe6553153d 100644 --- a/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs +++ b/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs @@ -32,7 +32,7 @@ {{/plugin-outlet}} diff --git a/app/models/category.rb b/app/models/category.rb index 71e42064a4a..957e7f56ee2 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -329,12 +329,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 diff --git a/app/models/group.rb b/app/models/group.rb index c0f55284f62..3e7adbc30a8 100644 --- a/app/models/group.rb +++ b/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 diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index c35875b0b8b..72b4764fee9 100644 --- a/spec/models/category_spec.rb +++ b/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 @@ -617,4 +617,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: 'test') + expect(category.valid?).to eq(false) + expect(category.errors.full_messages.join).not_to match(//) + 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: 'cool', 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(//) + end + end + + end + end diff --git a/test/javascripts/acceptance/category-edit-test.js.es6 b/test/javascripts/acceptance/category-edit-test.js.es6 index d80e3678537..389919018ef 100644 --- a/test/javascripts/acceptance/category-edit-test.js.es6 +++ b/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"); + }); +}); diff --git a/test/javascripts/helpers/create-pretender.js.es6 b/test/javascripts/helpers/create-pretender.js.es6 index 2597fa6b9f1..faa28c26e73 100644 --- a/test/javascripts/helpers/create-pretender.js.es6 +++ b/test/javascripts/helpers/create-pretender.js.es6 @@ -109,7 +109,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}); });