From f416634ea076c9f27899ba52e3c2086615c3a263 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 01aeb2d15be..e568c83fe56 100644 --- a/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs +++ b/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs @@ -31,7 +31,7 @@ {{/if}} diff --git a/app/models/category.rb b/app/models/category.rb index 5dcaaa46bfd..e49291920c6 100644 --- a/app/models/category.rb +++ b/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 diff --git a/app/models/group.rb b/app/models/group.rb index f5696d02ea3..13949964d27 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 4506c0bbe36..8fde29a12d9 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 @@ -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: '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 0aaad1ff901..d936c3c7720 100644 --- a/test/javascripts/helpers/create-pretender.js.es6 +++ b/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}); });