Project

General

Profile

Actions

Feature #31154

closed

Reject setting RFC non-compliant emission email addresses

Added by Go MAEDA almost 5 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Administration
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
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")

Files

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

Related issues

Related to Redmine - Feature #5913: Authors name in from address of email notificationsClosedGo MAEDA2010-07-20

Actions
Actions #1

Updated by Go MAEDA almost 5 years ago

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

Updated by Go MAEDA almost 5 years 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.

Actions #3

Updated by Mizuki ISHIKAWA over 4 years 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
Actions #4

Updated by Marius BĂLTEANU over 4 years 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

Actions #5

Updated by Mizuki ISHIKAWA over 4 years 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.

Actions #6

Updated by Go MAEDA over 4 years 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.

Actions #7

Updated by Go MAEDA over 4 years ago

  • Target version set to 4.1.0

Setting the target version to 4.1.0.

Actions #8

Updated by Go MAEDA over 4 years ago

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

Committed the patch. Thank you.

Actions #9

Updated by Go MAEDA over 4 years 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"].
Actions #10

Updated by Yuichi HARADA over 4 years 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']
Actions #11

Updated by Go MAEDA over 4 years 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.

Actions

Also available in: Atom PDF