Feature #31154
closed
Reject setting RFC non-compliant emission email addresses
Added by Go MAEDA about 5 years ago.
Updated over 4 years ago.
Description
RFC non-compliant emission causes Mail::Field::IncompleteParseError while performing email delivery and breaks email notification. No email notifications are delivered if Setting.mail_from has an RFC non-compliant value. The problem occurs in r17870 or later.
[ActiveJob] [ActionMailer::DeliveryJob] [f5bd6903-4f3f-4f40-9d9a-36ab998126e8] Error performing ActionMailer::DeliveryJob (Job ID: f5bd6903-4f3f-4f40-9d9a-36ab998126e8) from Async(mailers) in 1.2ms: Mail::Field::IncompleteParseError (Mail::AddressList can not parse |Redmine[xyz] <redmine@example.com>|: Only able to parse up to "Redmine"):
To avoid this, I think a validation for Setting.mail_from that rejects RFC non-compliant value should be implemented. It can be implemented using Mail::Address.new.
irb(main):001:0> Mail::Address.new('Redmine[xyz] <redmine@example.com>')
Traceback (most recent call last):
Mail::Field::IncompleteParseError (Mail::AddressList can not parse |Redmine[xyz] <redmine@example.com>|: Only able to parse up to "Redmine")
Files
- Related to Feature #5913: Authors name in from address of email notifications added
A workaround for this issue is committed in r18050. Redmine works as same as before r17870 and never raises an exception when the emission email address is not RFC compliant.
But I think this proposed validation is still necessary. Redmine should make an effort not to send emails which violates RFC.
This patch adds validation of the mail_from value.
- Mail::Address.new(mail_from) does not return an exception
- Mail::Address.new(mail_from).adress matches EmailAddress::EMAIL_REGEXP
Mizuki ISHIKAWA wrote:
This patch adds validation of the mail_from value.
- Mail::Address.new(mail_from) does not return an exception
- Mail::Address.new(mail_from).adress matches EmailAddress::EMAIL_REGEXP
Mizuki, do you see any problem if we use the existing EMAIL_REGEXP regex from URI::MailTo? https://www.rubydoc.info/stdlib/uri/URI/MailTo
Marius BALTEANU wrote:
Mizuki ISHIKAWA wrote:
This patch adds validation of the mail_from value.
- Mail::Address.new(mail_from) does not return an exception
- Mail::Address.new(mail_from).adress matches EmailAddress::EMAIL_REGEXP
Mizuki, do you see any problem if we use the existing EMAIL_REGEXP regex from URI::MailTo? https://www.rubydoc.info/stdlib/uri/URI/MailTo
I agree to use URI::MailTo::EMAIL_REGEXP because URI::MailTo::EMAIL_REGEXP is often used.
However, I think that it is better to hear other people's opinions because I am not familiar with e-mail address validation rules.
Marius BALTEANU wrote:
Mizuki, do you see any problem if we use the existing EMAIL_REGEXP regex from URI::MailTo? https://www.rubydoc.info/stdlib/uri/URI/MailTo
URI::MailTo::EMAIL_REGEXP cannot be used for Redmine because it rejects email addresses with Unicode characters such as "ドメイン例.jp" ("ドメイン名例" means "domain name example" in Japanese). Jean-Philippe Lang wrote in #15985#note-3 that Redmine should not reject domain names with non-ASCII characters.
Go MAEDA wrote:
Non-ASCII characters cannot be used in an email address.
Yes, they can. More and more clients and server support that.
We cannot simply disallow them in Redmine.
If you have any intarest in the support for Unicode domain (IDN), please see also #29208.
- Target version set to 4.1.0
Setting the target version to 4.1.0.
- Status changed from New to Closed
- Assignee set to Go MAEDA
- Resolution set to Fixed
Committed the patch. Thank you.
- Status changed from Closed to Reopened
The test sometimes fails:
$ bin/rails test --seed 64692
(snip)
Failure:
SettingTest#test_mail_from_format_should_be_validated [/Users/maeda/redmines/redmine-trunk/test/unit/setting_test.rb:140]:
Expected [[:mail_from, "n'est pas valide"]] to include [:mail_from, "is invalid"].
Go MAEDA wrote:
The test sometimes fails:
[...]
I think ::I18n.locale was set to 'fr' in other tests. I think it will be solved with this patch.
diff --git a/test/unit/setting_test.rb b/test/unit/setting_test.rb
index 253f3c037..145961539 100644
--- a/test/unit/setting_test.rb
+++ b/test/unit/setting_test.rb
@@ -20,6 +20,7 @@
require File.expand_path('../../test_helper', __FILE__)
class SettingTest < ActiveSupport::TestCase
+ fixtures :users
def setup
User.current = nil
@@ -134,7 +135,7 @@ YAML
end
def test_mail_from_format_should_be_validated
- with_settings :default_language => 'en' do
+ with_locale 'en' do
['[Redmine app] <redmine@example.net>', 'redmine'].each do |invalid_mail_from|
errors = Setting.set_all_from_params({:mail_from => invalid_mail_from})
assert_includes errors, [:mail_from, 'is invalid']
- Status changed from Reopened to Closed
Yuichi HARADA wrote:
Go MAEDA wrote:
The test sometimes fails:
[...]
I think ::I18n.locale was set to 'fr' in other tests. I think it will be solved with this patch.
[...]
Committed the fix in r18434. Thanks.
Also available in: Atom
PDF
Updated by 
Updated by 
Updated by 
Updated by