Skip to content

Commit

Permalink
SMTP: allow disabling starttls_auto since it's now true by default in…
Browse files Browse the repository at this point in the history 
… Ruby 3

Passing enable_tls/starttls/starttls_auto: false now explicitly disables
these options. They used to be all false by default, so they could only
be turned ON. So we ignored turning them OFF!

We now disable_tls/starttls/starttls_auto when they're set to false.
Leaving them set to nil inherits the upstream default.
  • Loading branch information
@jeremy @GChuf
jeremy authored and GChuf committed Oct 7, 2022
1 parent 873aa38 commit 6cf3a56
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 55 deletions.
12 changes: 6 additions & 6 deletions CHANGELOG.rdoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ Security:
* #1097 – SMTP security: prevent command injection via To/From addresses. (jeremy)

Features:
* #647 – IMAP: specify IMAP server search charset with Mail.find(search_charset: 'UTF-8'). (yalab)
* #647 – IMAP: specify IMAP server search charset with Mail.find(search_charset: 'UTF-8'). (yalab)
* #650 - UTF-7 charset support. (johngrimes)
* #664 - RSpec: with_html and with_text matchers. (zakkie)
* #723 – IMAP: support `enable_starttls: true` for TLS upgrade on non-IMAPS/SSL servers. (doits)
* #723 – IMAP: support `enable_starttls: true` for TLS upgrade on non-IMAPS/SSL servers. (doits)
* #804 - Configurable SMTP open_timeout and read_timeout. (ankane)
* #853 - `Mail::Message#set_sort_order` overrides the default message part sort order. (rafbm)
* #856 - Added :logger delivery method. (zacholauson)
Expand Down Expand Up @@ -75,7 +75,7 @@ Bugs:
* #792 - Allow blank filenames in Content-Disposition field. (robinroestenburg)
* #876 - Strip valid RFC-1342 separator characters between non-matching encoded-words. (Caleb W. Corliss)
* #895 - Fix that Mail::Message#add_file was adding a stray filename header. (kirikak2)
* #923 – Fix decoding nested quotes around non-US-ASCII addresses. (averell23)
* #923 – Fix decoding nested quotes around non-US-ASCII addresses. (averell23)
* #978 - Fix for invalid chars being left in a string for invalid b_value from encoding. (kjg)
* #996 - Fix that multipart/mixed emails with a delivery-status part could be interpreted as bounces. (kjg)
* #998 - Fix header parameter parsing (such as attachment names) for values encoded with a blank charset or language code. (kjg)
Expand All @@ -87,7 +87,7 @@ Bugs:
* #1074 - Fix that the first address in a list is dropped when a subsequent address has non-US-ASCII characters. (domininik)
* #1107 - Fix Address#display_name and other formatting flip-flopping between encoded and decoded forms depending on whether #encoded or #decoded was called last. (jeremy)
* #1110 - Fix that Mail::Multibyte::Chars#initialize mutated its argument by calling force_encoding on it. (jeremy)
* #1122 – Fix that tilde (~) shouldn't be escaped for Exim delivery. (Benabik)
* #1122 – Fix that tilde (~) shouldn't be escaped for Exim delivery. (Benabik)
* #1113 - Eliminate attachment corruption caused by CRLF conversion. (jeremy)
* #1131 - Fix that Message#without_attachments! didn't parse the remaining parts. (jeremy)
* #1019 - Fix b value encoder incorrectly splitting multibyte characters. (Kenneth-KT)
Expand All @@ -98,7 +98,7 @@ Bugs:
Features:
* #772 - Normalize encoding matchers (grosser)
* #775 - Avoid failed encodings / stop bad charsets early (grosser)
* #782 – Make the gem compatible with Rubinius (robin850)
* #782 – Make the gem compatible with Rubinius (robin850)
* #865 - Allow a body with an invalid encoding to be round tripped (kjg)
* #866 - Support decoding message bodies with non-Ruby-standard charsets (jeremy)
* #868 - Use the Ruby19.charset_encoder when decoding message bodies (kjg)
Expand All @@ -115,7 +115,7 @@ Bugs:
* #789 - Fix encoding collapsing not dealing with multiple encodings in 1 line (grosser)
* #808 - Mail::Field correctly responds_to? the methods of its instantiated field (thegcat)
* #849 - Handle calling Part#inline? when the Content-Disposition field couldn't be parsed (kjg)
* #874 – Stay under 1000-char SMTP line length limits (pushrax)
* #874 – Stay under 1000-char SMTP line length limits (pushrax)
* #877 - Make Mail::Field == other take the field value into account (kjg)
* #907 - Mail::ContentDispositionField should work with nil value (kjg)
* #910 - Mail::Address should handle b_value_encoded local and domain parts (kjg)
Expand Down
28 changes: 22 additions & 6 deletions lib/mail/network/delivery_methods/smtp.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,17 +111,33 @@ def start_smtp_session(&block)

def build_smtp_session
Net::SMTP.new(settings[:address], settings[:port]).tap do |smtp|
if settings[:tls] || settings[:ssl]
if smtp.respond_to?(:enable_tls)
tls = settings[:tls] || settings[:ssl]
if !tls.nil?
case tls
when true
smtp.enable_tls(ssl_context)
when false
smtp.disable_tls
else
raise ArgumentError, "Unrecognized :tls value #{settings[:tls].inspect}; expected true, false, or nil"
end
elsif settings[:enable_starttls]
if smtp.respond_to?(:enable_starttls)
elsif settings.include?(:enable_starttls) && !settings[:enable_starttls].nil?
case settings[:enable_starttls]
when true
smtp.enable_starttls(ssl_context)
when false
smtp.disable_starttls
else
raise ArgumentError, "Unrecognized :enable_starttls value #{settings[:enable_starttls].inspect}; expected true, false, or nil"
end
elsif settings[:enable_starttls_auto]
if smtp.respond_to?(:enable_starttls_auto)
elsif settings.include?(:enable_starttls_auto) && !settings[:enable_starttls_auto].nil?
case settings[:enable_starttls_auto]
when true
smtp.enable_starttls_auto(ssl_context)
when false
smtp.disable_starttls_auto
else
raise ArgumentError, "Unrecognized :enable_starttls_auto value #{settings[:enable_starttls_auto].inspect}; expected true, false, or nil"
end
end

Expand Down
45 changes: 26 additions & 19 deletions spec/mail/network/delivery_methods/smtp_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,10 @@
describe "SMTP Delivery Method" do

before(:each) do
MockSMTP.reset

# Reset all defaults back to original state
Mail.defaults do
delivery_method :smtp, { :address => "localhost",
:port => 25,
:domain => 'localhost.localdomain',
:user_name => nil,
:password => nil,
:authentication => nil,
:enable_starttls => nil,
:enable_starttls_auto => true,
:openssl_verify_mode => nil,
:tls => nil,
:ssl => nil,
:open_timeout => nil,
:read_timeout => nil
}
end
MockSMTP.clear_deliveries
Mail.defaults { delivery_method :smtp, {} }
end

describe "general usage" do
Expand Down Expand Up @@ -221,7 +207,7 @@ def redefine_verify_none(new_value)

message.deliver!

expect(MockSMTP.security).to eq :enable_starttls_auto
expect(MockSMTP.starttls).to eq :auto
end

it 'should allow forcing STARTTLS' do
Expand All @@ -242,7 +228,28 @@ def redefine_verify_none(new_value)

message.deliver!

expect(MockSMTP.security).to eq :enable_starttls
expect(MockSMTP.starttls).to eq :always
end

it 'should allow disabling automatic STARTTLS' do
message = Mail.new do
from 'mikel@test.lindsaar.net'
to 'ada@test.lindsaar.net'
subject 'Re: No way!'
body 'Yeah sure'
delivery_method :smtp, { :address => "localhost",
:port => 25,
:domain => 'localhost.localdomain',
:user_name => nil,
:password => nil,
:authentication => nil,
:enable_starttls => false }

end

message.deliver!

expect(MockSMTP.starttls).to eq false
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/mail/network_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ class MyRetriever; def initialize(settings); end; end

before(:each) do
# Set the delivery method to test as the default
MockSMTP.clear_deliveries
MockSMTP.reset
end

it "should deliver a mail message" do
Expand Down
63 changes: 40 additions & 23 deletions spec/spec_helper.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,19 +71,42 @@ def strip_heredoc(string)
string.gsub(/^[ \t]{#{indent}}/, '')
end

class Net::SMTP
class << self
alias unstubbed_new new
end

def self.new(*args)
MockSMTP.new
end
end

# Original mockup from ActionMailer
class MockSMTP
def self.reset
test = Net::SMTP.unstubbed_new('example.com')
@@tls = test.tls?
@@starttls = test.starttls?

@@deliveries = []
end

reset

def self.deliveries
@@deliveries
end

def self.security
@@security
def self.tls
@@tls
end

def self.starttls
@@starttls
end

def initialize
@@deliveries = []
@@security = nil
self.class.reset
end

def sendmail(mail, from, to)
Expand All @@ -103,38 +126,32 @@ def finish
return true
end

def self.clear_deliveries
@@deliveries = []
end

def self.clear_security
@@security = nil
end

def enable_tls(context)
raise ArgumentError, "SMTPS and STARTTLS is exclusive" if @@security && @@security != :enable_tls
@@security = :enable_tls
raise ArgumentError, "SMTPS and STARTTLS is exclusive" if @@starttls == :always
@@tls = true
context
end

def disable_tls
@@tls = false
end

def enable_starttls(context = nil)
raise ArgumentError, "SMTPS and STARTTLS is exclusive" if @@security == :enable_tls
@@security = :enable_starttls
raise ArgumentError, "SMTPS and STARTTLS is exclusive" if @@tls
@@starttls = :always
context
end

def enable_starttls_auto(context)
raise ArgumentError, "SMTPS and STARTTLS is exclusive" if @@security == :enable_tls
@@security = :enable_starttls_auto
raise ArgumentError, "SMTPS and STARTTLS is exclusive" if @@tls
@@starttls = :auto
context
end

end

class Net::SMTP
def self.new(*args)
MockSMTP.new
def disable_starttls
@@starttls = false
end

end

class MockPopMail
Expand Down

0 comments on commit 6cf3a56

Please sign in to comment.