Feature #31154
closed
Reject setting RFC non-compliant emission email addresses
0%
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 issues
Updated by Go MAEDA over 5 years ago
- Related to Feature #5913: Authors name in from address of email notifications added
Updated by Go MAEDA over 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.
Updated by Mizuki ISHIKAWA over 5 years ago
- File feature-31154.patch feature-31154.patch added
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
Updated by Marius BĂLTEANU over 5 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
Updated by Mizuki ISHIKAWA over 5 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_REGEXPregex fromURI::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.
Updated by Go MAEDA over 5 years ago
Marius BALTEANU wrote:
Mizuki, do you see any problem if we use the existing
EMAIL_REGEXPregex fromURI::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.
Updated by Go MAEDA over 5 years ago
- Target version set to 4.1.0
Setting the target version to 4.1.0.
Updated by Go MAEDA over 5 years ago
- Status changed from New to Closed
- Assignee set to Go MAEDA
- Resolution set to Fixed
Committed the patch. Thank you.
Updated by Go MAEDA over 5 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"].
Updated by Yuichi HARADA over 5 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']
Updated by Go MAEDA over 5 years ago
- Status changed from Reopened to Closed
Yuichi HARADA wrote:
Go MAEDA wrote:
The test sometimes fails:
[...]
I think
::I18n.localewas set to'fr'in other tests. I think it will be solved with this patch.[...]
Committed the fix in r18434. Thanks.