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 3 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
3 changes: 3 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ env:

matrix:
include:
- name: without-simpleidn
rvm: 2.5
env: SIMPLEIDN=no
- name: actionmailer
script: spec/travis-actionmailer.sh
rvm: 2.5
c960657 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ elsif RUBY_VERSION < '2.2.2'
end

gem 'mini_mime'
gem 'simpleidn' if RUBY_VERSION >= '1.9.3' && ENV['SIMPLEIDN'] != 'no'
c960657 marked this conversation as resolved.
Show resolved Hide resolved

if RUBY_VERSION >= '2.0'
gem 'byebug', :platforms => :mri
Expand Down
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,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 @@ -339,5 +339,22 @@ def Encodings.each_chunk_byterange(str, max_bytesize_per_chunk)

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

def Encodings.idna_supported?
c960657 marked this conversation as resolved.
Show resolved Hide resolved
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?

require 'simpleidn'
SimpleIDN.to_ascii(str)
end
end
end
12 changes: 8 additions & 4 deletions lib/mail/smtp_envelope.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,18 @@ 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]/ =~ addr
raise ArgumentError, "SMTP #{addr_name} address may not contain CR or LF line breaks: #{addr.inspect}"
end

if !addr.ascii_only? && Encodings.idna_supported?
addr = Address.new(addr).address_idna
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.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
20 changes: 20 additions & 0 deletions spec/mail/encodings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -966,4 +966,24 @@ def convert(from, to)
end
end
end

describe "IDNA encoding" do
it "should report on IDNA support" do
if RUBY_VERSION >= '1.9.3'
expect(Mail::Encodings.idna_supported?).to be true
else
expect(Mail::Encodings.idna_supported?).to be false
end
end

it "should encode a string correctly" do
raw = 'tést.example.com'
if Mail::Encodings.idna_supported?
encoded = 'xn--tst-bma.example.com'
expect(Mail::Encodings.idna_encode(raw)).to eq encoded
else
expect {Mail::Encodings.idna_encode(raw)}.to raise_error("Must install simpleidn gem")
end
end
end
end
18 changes: 18 additions & 0 deletions spec/mail/network/delivery_methods/smtp_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,24 @@ 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" do
Mail.defaults do
delivery_method :smtp, :idna => true
end

Mail.deliver do
to "tö@soméemail.com"
from "fröm@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 "supports the null sender in the envelope from address" do
Mail.deliver do
to "[email protected]"
Expand Down