Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support non-ASCII addresses (IDN) #1444

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ on:

jobs:
ruby:
name: ${{ matrix.ruby }} (timeout ${{ matrix.timeout }})
name: ${{ matrix.ruby }} ${{ matrix.env }} (timeout ${{ matrix.timeout }})
runs-on: ubuntu-latest
timeout-minutes: ${{ matrix.timeout }}
strategy:
Expand All @@ -18,6 +18,9 @@ jobs:
include:
- ruby: 2.5
timeout: 5
- ruby: 2.5
timeout: 5
env: SKIP_SIMPLEIDN=true
- ruby: 2.6
timeout: 5
- ruby: 2.7
Expand All @@ -28,6 +31,9 @@ jobs:
timeout: 5
- ruby: 3.2
timeout: 5
- ruby: 3.2
timeout: 5
env: SKIP_SIMPLEIDN=true
- ruby: truffleruby
timeout: 50
- ruby: truffleruby-head
Expand Down Expand Up @@ -55,7 +61,7 @@ jobs:
bundler-cache: true
cache-version: 4
- name: Run tests
run: bundle exec rake spec
run: ${{ matrix.env }} bundle exec rake spec
continue-on-error: ${{ matrix.ruby == 'truffleruby-head' }}
actionmailer:
runs-on: ubuntu-latest
Expand Down
4 changes: 3 additions & 1 deletion CHANGELOG.rdoc
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
== Version 2.9.0 (unreleased)

Breaking changes:
* Mail::Field::FIELDS_MAP now contains class names, not Class instances (c960657)

* Mail::Field::FIELDS_MAP now contains class names, not Class instances @c960657

Compatibility:

Expand All @@ -11,6 +12,7 @@ Features:

* Updated README to improve around sending multipart mail @kapfenho
* Add delivery_interceptors method to Mail class to fetch registered interceptors @ghousemohamed
* Support for non-ASCII addresses (IDN) @c960657

Code Improvements:

Expand Down
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,6 @@ end
gem 'jruby-openssl', :platforms => :jruby

gem 'mini_mime'
gem 'simpleidn' unless ENV.key?('SKIP_SIMPLEIDN')

gem 'byebug', :platforms => :mri
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,10 @@ Installation is fairly simple, I host mail on rubygems, so you can just do:

# gem install mail

If you need support for email addresses with non-ASCII domains (IDNs), and your mail server lacks native support for these, you can install this additional optional dependency:

# gem install simpleidn

## Encodings

If you didn't know, handling encodings in Emails is not as straight forward as you
Expand Down
23 changes: 23 additions & 0 deletions lib/mail/elements/address.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,18 @@ def address(output_type = :decode)
end
end

# Returns the address that is in the address itself with domain IDNA-encoded.
#
# a = Address.new('Mikel Lindsaar (My email address) <mikel@tést.lindsaar.net>')
# a.address_idna #=> '[email protected]'
def address_idna
if d = domain_idna
"#{local}@#{d}"
else
local
end
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this result in two separate cases? You'll either return the local + domain or just the local. Wouldn't it be better to just return the local here and put the domain_idna check in the same area where we return the full address (ie, local plus domain) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow. This method just follows the same pattern as Address#address, i.e. it does not add @ if the address does not include a domain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikel Would you elaborate? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikel Ping :-)


# Provides a way to assign an address to an already made Mail::Address object.
#
# a = Address.new
Expand Down Expand Up @@ -120,6 +132,17 @@ def domain(output_type = :decode)
Encodings.decode_encode(strip_all_comments(get_domain), output_type) if get_domain
end

# Returns the domain part (the right hand side of the @ sign in the email address) of
# the address in IDNA encoding.
#
# a = Address.new('Mikel Lindsaar (My email address) <mikel@tést.lindsaar.net>')
# a.domain_idna #=> 'xn--tst-bma.lindsaar.net'
def domain_idna
if d = domain
Encodings.idna_encode(d)
end
end

# Returns an array of comments that are in the email, or nil if there
# are no comments
#
Expand Down
17 changes: 17 additions & 0 deletions lib/mail/encodings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -310,5 +310,22 @@ def Encodings.each_chunk_byterange(str, max_bytesize_per_chunk)

yield Utilities.string_byteslice(str, offset, chunksize)
end

def Encodings.idna_supported?
c960657 marked this conversation as resolved.
Show resolved Hide resolved
return @idna_supported unless @idna_supported.nil?
@idna_supported ||= begin
require 'simpleidn'
true
rescue LoadError => e
false
end
end

def Encodings.idna_encode(str)
return str if str.ascii_only?
raise 'Must install simpleidn gem' unless Encodings.idna_supported?

SimpleIDN.to_ascii(str)
end
end
end
4 changes: 3 additions & 1 deletion lib/mail/network/delivery_methods/smtp_connection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ def initialize(values)
# Send the message via SMTP.
# The from and to attributes are optional. If not set, they are retrieve from the Message.
def deliver!(mail)
envelope = Mail::SmtpEnvelope.new(mail)
# Net::SMTP#cabable? was added in net-smtp 0.3.1 (included in Ruby 3.1).
smtputf8 = smtp.respond_to?(:capable?) && smtp.capable?('SMTPUTF8')
envelope = Mail::SmtpEnvelope.new(mail, smtputf8_supported: smtputf8)
response = smtp.sendmail(envelope.message, envelope.from, envelope.to)
settings[:return_response] ? response : self
end
Expand Down
36 changes: 29 additions & 7 deletions lib/mail/smtp_envelope.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ class SmtpEnvelope #:nodoc:

attr_reader :from, :to, :message

def initialize(mail)
self.from = mail.smtp_envelope_from
def initialize(mail, smtputf8_supported: false)
# Net::SMTP::Address was added in net-smtp 0.3.1 (included in Ruby 3.1).
@smtputf8_supported = smtputf8_supported && defined?(Net::SMTP::Address)
self.to = mail.smtp_envelope_to
self.from = mail.smtp_envelope_from
self.message = mail.encoded
end

Expand All @@ -19,7 +21,10 @@ def from=(addr)
raise ArgumentError, "SMTP From address may not be blank: #{addr.inspect}"
end

@from = validate_addr 'From', addr
addr = validate_addr 'From', addr
addr = Net::SMTP::Address.new(addr, 'SMTPUTF8') if @smtputf8

@from = addr
end

def to=(addr)
Expand All @@ -43,14 +48,31 @@ def message=(message)

private
def validate_addr(addr_name, addr)
if addr.bytesize > MAX_ADDRESS_BYTESIZE
raise ArgumentError, "SMTP #{addr_name} address may not exceed #{MAX_ADDRESS_BYTESIZE} bytes: #{addr.inspect}"
end

if /[\r\n]/.match?(addr)
raise ArgumentError, "SMTP #{addr_name} address may not contain CR or LF line breaks: #{addr.inspect}"
end

if !addr.ascii_only?
if @smtputf8_supported
# The SMTP server supports the SMTPUTF8 extension, so we can legally pass
# non-ASCII addresses, if we specify the SMTPUTF8 parameter for MAIL FROM.
@smtputf8 = true
elsif Encodings.idna_supported?
# The SMTP server does not announce support for the SMTPUTF8 extension, so do the
# IDNa encoding of the domain part client-side.
addr = Address.new(addr).address_idna
end

# If we cannot IDNa-encode the domain part, of if the local part contains
# non-ASCII characters, there is no standards-complaint way to send the
# mail via a server without SMTPUTF8 support. Our best chance is to just
# pass the UTF8-encoded address to the server.
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, could the Address class handle this without having to spread the idna checks in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only place where Encodings.idna_supported? is called (apart from the guard in Encodings.idna_encode). Deciding what to do if there is no IDNa support is a tradeoff – there is no “right” thing to do. Hiding this decision inside Address, making it IDNa-decode or silently return the input string, would be confusing IMO – if this is what you are suggesting.

I could remove the addr.ascii_only? check from here and just rely and that in Address, but with the addition of SMTPUTF8 we need it here anyway.


if addr.to_s.bytesize > MAX_ADDRESS_BYTESIZE
raise ArgumentError, "SMTP #{addr_name} address may not exceed #{MAX_ADDRESS_BYTESIZE} bytes: #{addr.inspect}"
end

addr
end
end
Expand Down
25 changes: 24 additions & 1 deletion spec/mail/elements/address_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@
end
end

it "should allow us to instantiate an empty address object and call address_idna" do
[nil, '', ' '].each do |input|
expect(Mail::Address.new(input).address_idna).to be_nil
end
end

it "should allow us to instantiate an empty address object and call local" do
[nil, '', ' '].each do |input|
expect(Mail::Address.new(input).local).to be_nil
Expand All @@ -38,6 +44,12 @@
end
end

it "should allow us to instantiate an empty address object and call domain_idna" do
[nil, '', ' '].each do |input|
expect(Mail::Address.new(input).domain_idna).to be_nil
end
end

it "should allow us to instantiate an empty address object and call name" do
[nil, '', ' '].each do |input|
expect(Mail::Address.new(input).name).to be_nil
Expand Down Expand Up @@ -119,7 +131,18 @@
expect(a.domain).to eq result
end

it "should give back the formated address" do
it "should give back the IDNA-encoded domain" do
parse_text = 'Mikel Lindsaar <test@lindsäär.net>'
result = 'xn--lindsr-fuaa.net'
a = Mail::Address.new(parse_text)
if Mail::Encodings.idna_supported?
expect(a.domain_idna).to eq result
else
expect {a.domain_idna}.to raise_error("Must install simpleidn gem")
end
end

it "should give back the formatted address" do
parse_text = 'Mikel Lindsaar <[email protected]>'
result = 'Mikel Lindsaar <[email protected]>'
a = Mail::Address.new(parse_text)
Expand Down
31 changes: 31 additions & 0 deletions spec/mail/encodings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -932,4 +932,35 @@ def convert(from, to)
expect(Mail::Utilities.pick_encoding("ISO-Foo")).to eq Encoding::BINARY
end
end

describe "IDNA encoding" do
after do
Mail::Encodings.instance_variable_set(:@idna_supported, nil)
end

it "should report on IDNA support" do
expect(Mail::Encodings.idna_supported?).to be !ENV.key?('SKIP_SIMPLEIDN')
end

it "should encode a string correctly" do
skip "simpleidn gem not installed" unless Mail::Encodings.idna_supported?

raw = 'tést.example.com'
encoded = 'xn--tst-bma.example.com'
expect(Mail::Encodings.idna_encode(raw)).to eq encoded
end

it "should raise on non-ASCII string if simpleidn gem is missing" do
Mail::Encodings.instance_variable_set(:@idna_supported, false)
raw = 'tést.example.com'
expect {Mail::Encodings.idna_encode(raw)}.to raise_error("Must install simpleidn gem")
end

it "should not raise on ASCII string if simpleidn gem is missing" do
Mail::Encodings.instance_variable_set(:@idna_supported, false)
raw = 'example.com'
encoded = Mail::Encodings.idna_encode(raw)
expect(encoded).to eq raw
end
end
end
2 changes: 2 additions & 0 deletions spec/mail/network/delivery_methods/smtp_connection_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
smtp = Net::SMTP.start('127.0.0.1', 25)
delivery_method :smtp_connection, :connection => smtp
end

MockSMTP.reset
end

after(:each) do
Expand Down
91 changes: 91 additions & 0 deletions spec/mail/network/delivery_methods/smtp_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,97 @@ def redefine_verify_none(new_value)
expect(MockSMTP.deliveries[0][2]).to eq %w([email protected])
end

it "IDNA-encodes non-ASCII From and To addresses without SMTPUTF8 support" do
Mail.defaults do
delivery_method :smtp
end

Mail.deliver do
from "fröm@soméemail.com"
to "tö@soméemail.com"
end

if Mail::Encodings.idna_supported?
expect(MockSMTP.deliveries[0][1]).to eq 'frö[email protected]'
expect(MockSMTP.deliveries[0][2]).to eq %w(tö@xn--somemail-d1a.com)
else
expect(MockSMTP.deliveries[0][1]).to eq 'fröm@soméemail.com'
expect(MockSMTP.deliveries[0][2]).to eq %w(tö@soméemail.com)
end
end

it "does not pass SMTPUTF8 parameter for ASCII From and To addresses with SMTPUTF8 support" do
Mail.defaults do
delivery_method :smtp
end

MockSMTP.capabilities = ['SMTPUTF8']

Mail.deliver do
to "[email protected]"
from "[email protected]"
end

expect(MockSMTP.deliveries[0][1]).to eq '[email protected]'
expect(MockSMTP.deliveries[0][2]).to eq %w([email protected])
end

it "passes SMTPUTF8 parameter for non-ASCII From address with SMTPUTF8 support" do
Mail.defaults do
delivery_method :smtp
end

MockSMTP.capabilities = ['SMTPUTF8']

Mail.deliver do
from "fröm@soméemail.com"
to "[email protected]"
end

if defined?(Net::SMTP::Address)
from_address = MockSMTP.deliveries[0][1]
expect(from_address).to be_instance_of Net::SMTP::Address
expect(from_address.to_s).to eq 'fröm@soméemail.com'
expect(from_address.parameters).to eq ['SMTPUTF8']
elsif Mail::Encodings.idna_supported?
expect(MockSMTP.deliveries[0][1]).to eq 'frö[email protected]'
else
expect(MockSMTP.deliveries[0][1]).to eq 'fröm@soméemail.com'
end

expect(MockSMTP.deliveries[0][2]).to eq %w([email protected])
end

it "passes SMTPUTF8 parameter for non-ASCII To address with SMTPUTF8 support" do
Mail.defaults do
delivery_method :smtp
end

MockSMTP.capabilities = ['SMTPUTF8']

Mail.deliver do
to "tö@soméemail.com"
from "[email protected]"
end

if defined?(Net::SMTP::Address)
from_address = MockSMTP.deliveries[0][1]
expect(from_address).to be_instance_of Net::SMTP::Address
expect(from_address.to_s).to eq '[email protected]'
expect(from_address.parameters).to eq ['SMTPUTF8']

expect(MockSMTP.deliveries[0][2]).to eq %w(tö@soméemail.com)
else
expect(MockSMTP.deliveries[0][1]).to eq '[email protected]'

if Mail::Encodings.idna_supported?
expect(MockSMTP.deliveries[0][2]).to eq %w(tö@xn--somemail-d1a.com)
else
expect(MockSMTP.deliveries[0][2]).to eq %w(tö@soméemail.com)
end
end
end

it "supports the null sender in the envelope from address" do
Mail.deliver do
to "[email protected]"
Expand Down
Loading