From f7d2fc05244d20c05e8c747c1e6ea66331d60d05 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Fri, 6 Feb 2015 12:08:37 +0100
Subject: [PATCH] FEATURE: 'reply by email address' validator
Prevent infinite email loophole when the 'reply_by_email_address' site setting is the same as the 'notification_email'.
---
config/locales/server.en.yml | 1 +
config/site_settings.yml | 21 ++++++------
lib/email/message_builder.rb | 5 +--
lib/site_setting_extension.rb | 11 +++++--
.../reply_by_email_address_validator.rb | 17 ++++++++++
.../reply_by_email_address_validator_spec.rb | 33 +++++++++++++++++++
6 files changed, 72 insertions(+), 16 deletions(-)
create mode 100644 lib/validators/reply_by_email_address_validator.rb
create mode 100644 spec/components/validators/reply_by_email_address_validator_spec.rb
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 611dd62af..51e279c77 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -1096,6 +1096,7 @@ en:
invalid_string_min_max: "Must be between %{min} and %{max} characters."
invalid_string_min: "Must be at least %{min} characters."
invalid_string_max: "Must be no more than %{max} characters."
+ invalid_reply_by_email_address: "Value must contain '%{reply_key}' and be different from the notification email."
notification_types:
mentioned: "%{display_username} mentioned you in %{link}"
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 6967d21ab..733164d60 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -1,13 +1,14 @@
# Available options:
#
-# default - The default value of the setting.
-# client - Set to true if the javascript should have access to this setting's value.
-# refresh - Set to true if clients should refresh when the setting is changed.
-# min - For a string setting, the minimum length. For an integer setting, the minimum value.
-# max - For a string setting, the maximum length. For an integer setting, the maximum value.
-# regex - A regex that the value must match.
-# enum - The setting has a fixed set of allowed values, and only one can be chosen.
-# Set to the class name that defines the set.
+# default - The default value of the setting.
+# client - Set to true if the javascript should have access to this setting's value.
+# refresh - Set to true if clients should refresh when the setting is changed.
+# min - For a string setting, the minimum length. For an integer setting, the minimum value.
+# max - For a string setting, the maximum length. For an integer setting, the maximum value.
+# regex - A regex that the value must match.
+# validator - The name of the class that will be use to validate the value of the setting.
+# enum - The setting has a fixed set of allowed values, and only one can be chosen.
+# Set to the class name that defines the set.
# type: email - Must be a valid email address.
# type: username - Must match the username of an existing user.
# type: list - A list of values, chosen from a set of valid values defined in the choices option.
@@ -423,7 +424,7 @@ email:
reply_by_email_enabled: false
reply_by_email_address:
default: ''
- regex: "%{reply_key}"
+ validator: "ReplyByEmailAddressValidator"
pop3_polling_enabled: false
pop3_polling_ssl: true
pop3_polling_period_mins: 5
@@ -491,7 +492,7 @@ files:
enum: 'S3RegionSiteSetting'
s3_upload_bucket:
default: ''
- regex: "^[^\\.]*$"
+ regex: "^[^A-Z._]+$"
allow_profile_backgrounds:
client: true
default: true
diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb
index cff0e2b64..5e1428d96 100644
--- a/lib/email/message_builder.rb
+++ b/lib/email/message_builder.rb
@@ -146,10 +146,7 @@ module Email
end
def private_reply?
- SiteSetting.reply_by_email_enabled? &&
- reply_by_email_address.present? &&
- @opts[:allow_reply_by_email] &&
- @opts[:private_reply]
+ allow_reply_by_email? && @opts[:private_reply]
end
def from_value
diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb
index c9efa2ca3..9b87a8e18 100644
--- a/lib/site_setting_extension.rb
+++ b/lib/site_setting_extension.rb
@@ -72,18 +72,22 @@ module SiteSettingExtension
self.defaults[name] = default
categories[name] = opts[:category] || :uncategorized
current_value = current.has_key?(name) ? current[name] : default
+
if opts[:enum]
enum = opts[:enum]
enums[name] = enum.is_a?(String) ? enum.constantize : enum
end
+
if opts[:choices]
choices.has_key?(name) ?
choices[name].concat(opts[:choices]) :
choices[name] = opts[:choices]
end
+
if opts[:type] == 'list'
lists << name
end
+
if opts[:hidden]
hidden_settings << name
end
@@ -105,8 +109,11 @@ module SiteSettingExtension
previews[name] = opts[:preview]
end
- if validator_type = validator_for(opts[:type] || get_data_type(name, defaults[name]))
- validators[name] = {class: validator_type, opts: opts}
+ opts[:validator] = opts[:validator].try(:constantize)
+ type = opts[:type] || get_data_type(name, defaults[name])
+
+ if validator_type = opts[:validator] || validator_for(type)
+ validators[name] = { class: validator_type, opts: opts }
end
current[name] = current_value
diff --git a/lib/validators/reply_by_email_address_validator.rb b/lib/validators/reply_by_email_address_validator.rb
new file mode 100644
index 000000000..f5c19d2bd
--- /dev/null
+++ b/lib/validators/reply_by_email_address_validator.rb
@@ -0,0 +1,17 @@
+class ReplyByEmailAddressValidator
+ def initialize(opts={})
+ @opts = opts
+ end
+
+ def valid_value?(val)
+ return true if val.blank?
+
+ !!(val =~ /@/i) &&
+ !!(val =~ /%{reply_key}/i) &&
+ val.gsub(/\+?%{reply_key}/i, "") != SiteSetting.notification_email
+ end
+
+ def error_message
+ I18n.t('site_settings.errors.invalid_reply_by_email_address')
+ end
+end
diff --git a/spec/components/validators/reply_by_email_address_validator_spec.rb b/spec/components/validators/reply_by_email_address_validator_spec.rb
new file mode 100644
index 000000000..81f6f24a0
--- /dev/null
+++ b/spec/components/validators/reply_by_email_address_validator_spec.rb
@@ -0,0 +1,33 @@
+require 'spec_helper'
+
+describe ReplyByEmailAddressValidator do
+
+ describe '#valid_value?' do
+ subject(:validator) { described_class.new }
+
+ it "returns true for blank values" do
+ expect(validator.valid_value?('')).to eq(true)
+ expect(validator.valid_value?(nil)).to eq(true)
+ end
+
+ it "returns false if value is not an email address" do
+ expect(validator.valid_value?('WAT%{reply_key}.com')).to eq(false)
+ end
+
+ it "returns false if value does not contain '%{reply_key}'" do
+ expect(validator.valid_value?('foo@bar.com')).to eq(false)
+ end
+
+ it "returns false if value is the same as SiteSetting.notification_email" do
+ SiteSetting.expects(:notification_email).returns("foo@bar.com")
+ expect(validator.valid_value?('foo+%{reply_key}@bar.com')).to eq(false)
+ end
+
+ it "returns true when value is OK" do
+ SiteSetting.expects(:notification_email).returns("foo@bar.com")
+ expect(validator.valid_value?('bar%{reply_key}@foo.com')).to eq(true)
+ end
+
+ end
+
+end