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

@@ -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}"

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

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

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

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

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