Feature #31154

Reject setting RFC non-compliant emission email addresses

Added by Go MAEDA 8 months ago. Updated 3 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Go MAEDA% Done:

0%

Category:Administration
Target version:4.1.0
Resolution:Fixed

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")

feature-31154.patch Magnifier (2.33 KB) Mizuki ISHIKAWA, 2019-07-09 06:56


Related issues

Related to Redmine - Feature #5913: Authors name in from address of email notifications Closed 2010-07-20

Associated revisions

Revision 18396
Added by Go MAEDA 4 months ago

Reject setting RFC non-compliant emission email addresses (#31154).

Patch by Mizuki ISHIKAWA.

Revision 18434
Added by Go MAEDA 3 months ago

Fix that test_mail_from_format_should_be_validated randomly fails (#31154).

Patch by Yuichi HARADA.

History

#1 Updated by Go MAEDA 8 months ago

  • Related to Feature #5913: Authors name in from address of email notifications added

#2 Updated by Go MAEDA 8 months ago

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.

#3 Updated by Mizuki ISHIKAWA 5 months ago

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

#4 Updated by Marius BALTEANU 5 months ago

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

#5 Updated by Mizuki ISHIKAWA 5 months ago

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.

#6 Updated by Go MAEDA 5 months ago

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.

#7 Updated by Go MAEDA 4 months ago

  • Target version set to 4.1.0

Setting the target version to 4.1.0.

#8 Updated by Go MAEDA 4 months ago

  • Status changed from New to Closed
  • Assignee set to Go MAEDA
  • Resolution set to Fixed

Committed the patch. Thank you.

#9 Updated by Go MAEDA 4 months ago

  • 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"].

#10 Updated by Yuichi HARADA 3 months ago

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']

#11 Updated by Go MAEDA 3 months ago

  • 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