Net::IMAP#starttls doesn't raise an exception when it fails

[rhenium]

The OpenSSL::SSL::SSLError raised from OpenSSL::SSL::SSLSocket#connect* is swallowed somewhere.

Here is a reproducer:

require "net/imap"

TracePoint.new(:raise) { |tp| p [tp, tp.raised_exception] }.enable

# a random IMAP server that supports STARTTLS
imap = Net::IMAP.new("outlook.office365.com")
# empty cert store -> certificate verification will always fail
p imap.starttls(cert_store: OpenSSL::X509::Store.new)
puts "This should not be reached"

puts "Any further command fails because SSLSocket is not ready:"
p imap.capability

Output:

[#<TracePoint:raise /opt/ruby/master/lib/ruby/gems/3.5.0+0/gems/net-protocol-0.2.2/lib/net/protocol.rb:46>, #<OpenSSL::SSL::SSLError: SSL_connect returned=1 errno=0 peeraddr=[2603:1046:c09:1003::2]:143 state=error: certificate verify failed (unable to get local issuer certificate)>]
[#<TracePoint:raise /opt/ruby/master/lib/ruby/3.5.0+0/openssl/buffering.rb:76>, #<EOFError: end of file reached>]
#<struct Net::IMAP::TaggedResponse tag="RUBY0001", name="OK", data=#<struct Net::IMAP::ResponseText code=nil, text="Begin TLS negotiation now.">, raw_data="RUBY0001 OK Begin TLS negotiation now.\r\n">
This should not be reached
Any further command fails because SSLSocket is not ready:
[#<TracePoint:raise /opt/ruby/master/lib/ruby/3.5.0+0/openssl/buffering.rb:366>, #<OpenSSL::SSL::SSLError: SSL_write>]
/opt/ruby/master/lib/ruby/3.5.0+0/openssl/buffering.rb:366:in 'OpenSSL::SSL::SSLSocket#syswrite': SSL_write (OpenSSL::SSL::SSLError)
        from /opt/ruby/master/lib/ruby/3.5.0+0/openssl/buffering.rb:366:in 'OpenSSL::Buffering#do_write'
        from /opt/ruby/master/lib/ruby/3.5.0+0/openssl/buffering.rb:471:in 'OpenSSL::Buffering#print'
        from /opt/ruby/master/lib/ruby/gems/3.5.0+0/gems/net-imap-0.5.5/lib/net/imap.rb:3386:in 'Net::IMAP#put_string'
        from /opt/ruby/master/lib/ruby/gems/3.5.0+0/gems/net-imap-0.5.5/lib/net/imap.rb:3358:in 'block in Net::IMAP#send_command'
        from /opt/ruby/master/lib/ruby/3.5.0+0/monitor.rb:201:in 'Monitor#synchronize'
        from /opt/ruby/master/lib/ruby/3.5.0+0/monitor.rb:201:in 'MonitorMixin#mon_synchronize'
        from /opt/ruby/master/lib/ruby/gems/3.5.0+0/gems/net-imap-0.5.5/lib/net/imap.rb:3353:in 'Net::IMAP#send_command'
        from /opt/ruby/master/lib/ruby/gems/3.5.0+0/gems/net-imap-0.5.5/lib/net/imap.rb:1128:in 'block in Net::IMAP#capability'
        from /opt/ruby/master/lib/ruby/3.5.0+0/monitor.rb:201:in 'Monitor#synchronize'
        from /opt/ruby/master/lib/ruby/3.5.0+0/monitor.rb:201:in 'MonitorMixin#mon_synchronize'
        from /opt/ruby/master/lib/ruby/gems/3.5.0+0/gems/net-imap-0.5.5/lib/net/imap.rb:1127:in 'Net::IMAP#capability'
        from xx.rb:12:in '<main>'

The test case test_starttls_unknown_ca missed this because assert_raise catches the OpenSSL::SSL::SSLError raised from #logout which is implicitly called within starttls_test. Modifying it like this causes a test failure:

diff --git a/test/net/imap/test_imap.rb b/test/net/imap/test_imap.rb
index 10a29c408248..7dc90733a92c 100644
--- a/test/net/imap/test_imap.rb
+++ b/test/net/imap/test_imap.rb
@@ -112,19 +112,17 @@ class IMAPTest < Test::Unit::TestCase
     def test_starttls_unknown_ca
       omit "This test is not working with Windows" if RUBY_PLATFORM =~ /mswin|mingw/

       imap = nil
-      assert_raise(OpenSSL::SSL::SSLError) do
         ex = nil
         starttls_test do |port|
           imap = Net::IMAP.new("localhost", port: port)
           imap.starttls
           imap
         rescue => ex
           imap
         end
-        raise ex if ex
-      end
+      assert_kind_of(OpenSSL::SSL::SSLError, ex)
       assert_equal false, imap.tls_verified?
       assert_equal({}, imap.ssl_ctx_params)
       assert_equal(nil, imap.ssl_ctx.ca_file)
       assert_equal(OpenSSL::SSL::VERIFY_PEER, imap.ssl_ctx.verify_mode)

[nevans]

Oh no! Good catch.

[nevans]

It looks like it's because the exception occurs in the receiver thread, but send_command doesn't check for an exception (other than than NO or BAD). The good news is that the connection is in a permanently broken state, so I don't think it can lead to accidentally using an insecure connection. Please correct me if I'm wrong about that!

But it should raise an exception immediately, outside the receiver thread. I'll have a PR in a few minutes.

[rhenium]

The good news is that the connection is in a permanently broken state, so I don't think it can lead to accidentally using an insecure connection. Please correct me if I'm wrong about that!

That was my thought, too. Thanks for the fix!