From 031fb0fe0c9f36fd5a9b0de0f04455cd21c7ea9f Mon Sep 17 00:00:00 2001
From: nick evans <nicholas.evans@gmail.com>
Date: Fri, 20 Jan 2023 23:52:57 -0500
Subject: [PATCH] =?UTF-8?q?=F0=9F=97=91=EF=B8=8F=20Add=20deprecation=20war?=
 =?UTF-8?q?nings=20to=20.new=20and=20#starttls=20[=F0=9F=9A=A7=20WIP]?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Moved all deprecated option handling to DeprecatedClientOptions, which
  is prepended to Net::IMAP.  This allows `Net::IMAP` to cleanly
  implement and document the simplified keyword args API, while still
  providing a place to document the old APIs.

  Some weird and potentially confusing edge-cases were converted to
  `ArgumentError`.  The very old forms are documented and will warn
  loudly.  Some cases are obsolete but don't print any warnings... yet.

* `ssl` was renamed to `tls` in most places, with backwards compatible
  aliases.  Using `ssl` does not print any deprecation warnings.  Using
  both `tls` and `ssl` keywords raises an ArgumentError.

* ♻️ Additionally, split `initialize` up into small helper methods making
  it easier to understand at a glance.

* Preparing for a (backwards-incompatible) secure-by-default
  configuration, `Net::IMAP.default_tls` will determine the value for
  `tls` when no explicit port or tls setting is provided.  Using port
  143 will be insecure by default.  Using port 993 will be secure by
  default.  Providing no explicit port will use `Net::IMAP.default_tls`
  with the appropriate port.  And providing any other unknown port will
  use `default_tls` with a warning.

  🚧 TODO: should we use a different config var for default tls params
  when port is 993 and `tls` is unspecified?

  🚧 TODO: should we use a different config var for choosing `tls` when
  `port` is non-standard vs choosing `port` and `tls` when neither are
  specified?

  🚧 TODO: should we use a different var for `default_tls` be used to
  config params when port is 993 but tls is implicit? Another var?
---
 lib/net/imap.rb                           | 268 ++++++++++++----------
 lib/net/imap/deprecated_client_options.rb | 166 ++++++++++++++
 2 files changed, 319 insertions(+), 115 deletions(-)
 create mode 100644 lib/net/imap/deprecated_client_options.rb

diff --git a/lib/net/imap.rb b/lib/net/imap.rb
index 754d32826..bd763c019 100644
--- a/lib/net/imap.rb
+++ b/lib/net/imap.rb
@@ -725,6 +725,107 @@ class << self
       alias default_imap_port default_port
       alias default_imaps_port default_tls_port
       alias default_ssl_port default_tls_port
+
+      # The default value for the +tls+ option of ::new, when +port+ is
+      # unspecified or non-standard.
+      #
+      # *Note*: A future release of Net::IMAP will set the default to +true+, as
+      # per RFC7525[https://tools.ietf.org/html/rfc7525],
+      # RFC7817[https://tools.ietf.org/html/rfc7817], and
+      # RFC8314[https://tools.ietf.org/html/rfc8314].
+      #
+      # Set to +true+ for the secure default without warnings.  Set to
+      # +false+ to globally silence warnings and use insecure defaults.
+      attr_accessor :default_tls
+      alias default_ssl default_tls
+    end
+
+    # Creates a new Net::IMAP object and connects it to the specified
+    # +host+.
+    #
+    # Accepts the following options:
+    #
+    # [port]
+    #   Port number (default value is 143 for imap, or 993 for imaps)
+    # [tls]
+    #   When +true+, the connection will use TLS with the default params set by
+    #   {OpenSSL::SSL::SSLContext#set_params}[https://docs.ruby-lang.org/en/master/OpenSSL/SSL/SSLContext.html#method-i-set_params].
+    #   Assign a hash to override TLS params—the keys are assignment methods on
+    #   SSLContext[https://docs.ruby-lang.org/en/master/OpenSSL/SSL/SSLContext.html].
+    #
+    #   When <tt>port: 993</tt>, +tls+ defaults to +true+.
+    #   When <tt>port: 143</tt>, +tls+ defaults to +false+.
+    #   When port is unspecified or non-standard, +tls+ defaults to
+    #   ::default_tls.  When ::default_tls is also +nil+, a warning is printed
+    #   and the connection does _not_ use TLS.
+    #
+    #   When +nil+ or unassigned a default value is assigned: the default is
+    #   +true+ if <tt>port: 993</tt>, +false+ if <tt>port: 143</tt>, and
+    #   ::default_tls when +port+ is unspecified or non-standard.  When
+    #   ::default_tls is +nil+, a back
+    #
+    # [open_timeout]
+    #   Seconds to wait until a connection is opened
+    # [idle_response_timeout]
+    #   Seconds to wait until an IDLE response is received
+    #
+    # The most common errors are:
+    #
+    # Errno::ECONNREFUSED:: Connection refused by +host+ or an intervening
+    #                       firewall.
+    # Errno::ETIMEDOUT:: Connection timed out (possibly due to packets
+    #                    being dropped by an intervening firewall).
+    # Errno::ENETUNREACH:: There is no route to that network.
+    # SocketError:: Hostname not known or other socket error.
+    # Net::IMAP::ByeResponseError:: Connected to the host successfully, but
+    #                               it immediately said goodbye.
+    def initialize(host,
+                   port: nil,
+                   tls:  nil,
+                   open_timeout: 30,
+                   idle_response_timeout: 5)
+      super() # (MonitorMixin)
+      @host                  = host
+      @tls, @port            = default_tls_and_port(tls, port)
+      @open_timeout          = Integer(open_timeout)
+      @idle_response_timeout = Integer(idle_response_timeout)
+
+      # Basic Client state
+      @tls_verified = false
+      @greeting = nil
+      @capabilities = nil
+      @utf8_strings = false # TODO: use @enabled instead
+      @debug_output_bol = true
+
+      # Client Protocol Reciever
+      @parser = ResponseParser.new
+      @receiver_thread = nil
+      @receiver_thread_terminating = false
+      @exception = nil
+
+      # Client Protocol Sender
+      @tag_prefix = "RUBY"
+      @tagno = 0
+
+      # Response handlers
+      @continuation_request_arrival = new_cond
+      @continuation_request_exception = nil
+      @tagged_response_arrival = new_cond
+      @tagged_responses = {}
+      @response_handlers = []
+      @responses = Hash.new {|h, k| h[k] = [] }
+
+      # Command execution state
+      @logout_command_tag = nil
+      @continued_command_tag = nil
+      @idle_done_cond = nil
+
+      # DEPRECATED
+      @client_thread = Thread.current
+
+      # create the connection
+      @sock = nil
+      start_connection
     end
 
     def client_thread # :nodoc:
@@ -800,7 +901,7 @@ def capabilities
     # servers will drop all <tt>AUTH=</tt> mechanisms from #capabilities after
     # the connection has authenticated.
     #
-    #    imap = Net::IMAP.new(hostname, ssl: false)
+    #    imap = Net::IMAP.new(hostname, tls: false)
     #    imap.capabilities    # => ["IMAP4REV1", "LOGINDISABLED"]
     #    imap.auth_mechanisms # => []
     #
@@ -975,15 +1076,9 @@ def logout
     # Server capabilities may change after #starttls, #login, and #authenticate.
     # Cached #capabilities will be cleared when this method completes.
     #
-    def starttls(options = {}, verify = true)
+    def starttls(**options)
       send_command("STARTTLS") do |resp|
         if resp.kind_of?(TaggedResponse) && resp.name == "OK"
-          begin
-            # for backward compatibility
-            certs = options.to_str
-            options = create_ssl_params(certs, verify)
-          rescue NoMethodError
-          end
           clear_cached_capabilities
           clear_responses
           start_tls_session(options)
@@ -2218,100 +2313,62 @@ def remove_response_handler(handler)
 
     @@debug = false
 
-    # :call-seq:
-    #    Net::IMAP.new(host, options = {})
-    #
-    # Creates a new Net::IMAP object and connects it to the specified
-    # +host+.
-    #
-    # +options+ is an option hash, each key of which is a symbol.
-    #
-    # The available options are:
-    #
-    # port::  Port number (default value is 143 for imap, or 993 for imaps)
-    # ssl::   If +options[:ssl]+ is true, then an attempt will be made
-    #         to use SSL (now TLS) to connect to the server.
-    #         If +options[:ssl]+ is a hash, it's passed to
-    #         OpenSSL::SSL::SSLContext#set_params as parameters.
-    # open_timeout:: Seconds to wait until a connection is opened
-    # idle_response_timeout:: Seconds to wait until an IDLE response is received
-    #
-    # The most common errors are:
-    #
-    # Errno::ECONNREFUSED:: Connection refused by +host+ or an intervening
-    #                       firewall.
-    # Errno::ETIMEDOUT:: Connection timed out (possibly due to packets
-    #                    being dropped by an intervening firewall).
-    # Errno::ENETUNREACH:: There is no route to that network.
-    # SocketError:: Hostname not known or other socket error.
-    # Net::IMAP::ByeResponseError:: The connected to the host was successful, but
-    #                               it immediately said goodbye.
-    def initialize(host, port_or_options = {},
-                   usessl = false, certs = nil, verify = true)
-      super()
-      @host = host
-      begin
-        options = port_or_options.to_hash
-      rescue NoMethodError
-        # for backward compatibility
-        options = {}
-        options[:port] = port_or_options
-        if usessl
-          options[:ssl] = create_ssl_params(certs, verify)
+    def default_tls_and_port(tls, port)
+      if tls.nil? && port
+        tls = true  if port == SSL_PORT || /\Aimaps\z/i === port
+        tls = false if port == PORT
+      elsif port.nil? && !tls.nil?
+        port = tls ? SSL_PORT : PORT
+      end
+      if tls.nil? && port.nil?
+        tls = self.class.default_tls.dup.freeze
+        port = tls ? SSL_PORT : PORT
+        if tls.nil?
+          warn "A future version of Net::IMAP.default_tls " \
+               "will default to 'true', for secure connections by default.  " \
+               "Use 'Net::IMAP.new(host, tls: false)' or set " \
+               "Net::IMAP.default_tls = false' to silence this warning."
         end
       end
-      @port = options[:port] || (options[:ssl] ? SSL_PORT : PORT)
-      @tag_prefix = "RUBY"
-      @tagno = 0
-      @utf8_strings = false
-      @open_timeout = options[:open_timeout] || 30
-      @idle_response_timeout = options[:idle_response_timeout] || 5
-      @tls_verified = false
-      @parser = ResponseParser.new
+      tls &&= tls.respond_to?(:to_hash) ? tls.to_hash : {}
+      [tls, port]
+    end
+
+    def start_connection
       @sock = tcp_socket(@host, @port)
       begin
-        if options[:ssl]
-          start_tls_session(options[:ssl])
-          @usessl = true
-        else
-          @usessl = false
-        end
-        @responses = Hash.new {|h, k| h[k] = [] }
-        @tagged_responses = {}
-        @response_handlers = []
-        @tagged_response_arrival = new_cond
-        @continued_command_tag = nil
-        @continuation_request_arrival = new_cond
-        @continuation_request_exception = nil
-        @idle_done_cond = nil
-        @logout_command_tag = nil
-        @debug_output_bol = true
-        @exception = nil
-
+        start_tls_session(@tls) if @tls
         @greeting = get_response
-        if @greeting.nil?
-          raise Error, "connection closed"
-        end
-        record_untagged_response_code @greeting
-        @capabilities = capabilities_from_resp_code @greeting
-        if @greeting.name == "BYE"
-          raise ByeResponseError, @greeting
-        end
-
-        @client_thread = Thread.current
-        @receiver_thread = Thread.start {
-          begin
-            receive_responses
-          rescue Exception
-          end
-        }
-        @receiver_thread_terminating = false
+        handle_server_greeting
+        @receiver_thread = start_receiver_thread
       rescue Exception
         @sock.close
         raise
       end
     end
 
+    def handle_server_greeting
+      if @greeting.nil?
+        raise Error, "connection closed"
+      end
+      record_untagged_response_code(@greeting)
+      @capabilities = capabilities_from_resp_code @greeting
+      if @greeting.name == "BYE"
+        raise ByeResponseError, @greeting
+      end
+    end
+
+    def start_receiver_thread
+      Thread.start do
+        receive_responses
+      rescue Exception
+        # don't exit the thread with an exception
+      end
+    rescue Exception
+      @sock.close
+      raise
+    end
+
     def tcp_socket(host, port)
       s = Socket.tcp(host, port, :connect_timeout => @open_timeout)
       s.setsockopt(:SOL_SOCKET, :SO_KEEPALIVE, true)
@@ -2598,34 +2655,12 @@ def normalize_searching_criteria(keys)
       end
     end
 
-    def create_ssl_params(certs = nil, verify = true)
-      params = {}
-      if certs
-        if File.file?(certs)
-          params[:ca_file] = certs
-        elsif File.directory?(certs)
-          params[:ca_path] = certs
-        end
-      end
-      if verify
-        params[:verify_mode] = VERIFY_PEER
-      else
-        params[:verify_mode] = VERIFY_NONE
-      end
-      return params
-    end
-
     def start_tls_session(params = {})
       unless defined?(OpenSSL::SSL)
-        raise "SSL extension not installed"
+        raise "OpenSSL extension not installed"
       end
       if @sock.kind_of?(OpenSSL::SSL::SSLSocket)
-        raise RuntimeError, "already using SSL"
-      end
-      begin
-        params = params.to_hash
-      rescue NoMethodError
-        params = {}
+        raise RuntimeError, "already using TLS"
       end
       context = SSLContext.new
       context.set_params(params)
@@ -2662,3 +2697,6 @@ def self.saslprep(string, **opts)
 require_relative "imap/response_data"
 require_relative "imap/response_parser"
 require_relative "imap/authenticators"
+
+require_relative "imap/deprecated_client_options"
+Net::IMAP.prepend Net::IMAP::DeprecatedClientOptions
diff --git a/lib/net/imap/deprecated_client_options.rb b/lib/net/imap/deprecated_client_options.rb
new file mode 100644
index 000000000..12dbacc67
--- /dev/null
+++ b/lib/net/imap/deprecated_client_options.rb
@@ -0,0 +1,166 @@
+# frozen_string_literal: true
+
+module Net
+  class IMAP < Protocol
+
+    # This module handles deprecated arguments to various methods.
+    module DeprecatedClientOptions
+      UNDEF = Module.new.freeze
+      private_constant :UNDEF
+
+      # :call-seq:
+      #   Net::IMAP.new(host, ssl: nil, **options)
+      #   Net::IMAP.new(host, port)
+      #   Net::IMAP.new(host, options = {})
+      #   Net::IMAP.new(host, port = nil, usessl = false, certs = nil, verify = true)
+      #
+      # Translates Net::IMAP.new arguments for backward compatibility.
+      #
+      # ==== Obsolete arguments
+      #
+      # If +ssl+ is given, it is silently converted to the +tls+ keyword
+      # argument.  Combining both +ssl+ and +tls+ raises an ArgumentError.  Both
+      # of the following behave identically:
+      #
+      #     Net::IMAP.new("imap.example.com", port: 993, ssl: {ca_path: "path/to/certs"})
+      #     Net::IMAP.new("imap.example.com", port: 993, tls: {ca_path: "path/to/certs"})
+      #
+      # If a second positional argument is given and it is an integer, it is
+      # silently converted to the +port+ keyword argument.
+      #
+      #     Net::IMAP.new("imap.example.com", 993)
+      #     Net::IMAP.new("imap.example.com", port: 993)
+      #
+      # Obsolete arguments may print deprecation warnings in a future release.
+      #
+      # ==== Deprecated arguments
+      #
+      # Using any deprecated arguments will print a warning.  Deprecated
+      # arguments will be removed in a future release.
+      #
+      # If a second positional argument is given and it is a hash, it is
+      # converted to keyword arguments.
+      #
+      #     # DEPRECATED:
+      #     Net::IMAP.new("imap.example.com", options_hash) # => prints a warning
+      #     # Not deprecated:
+      #     Net::IMAP.new("imap.example.com", **options_hash)
+      #
+      # If a second positional argument is given and it is not an integer or
+      # hash, it is converted to the +port+ keyword argument—with a warning.
+      #
+      #     # DEPRECATED:
+      #     Net::IMAP.new("imap.example.com", "imap") # => prints a warning
+      #     # Not deprecated:
+      #     Net::IMAP.new("imap.example.com", port: "imap")
+      #
+      # If +usessl+ is false, +certs+, and +verify+ are ignored.  When it true,
+      # all three arguments are converted to the +tls+ keyword argument.
+      #
+      #     # DEPRECATED: usessl = true
+      #     Net::IMAP.new("imap.example.com", 993, true)
+      #     # Not deprecated: keywords port and tls
+      #     Net::IMAP.new("imap.example.com", port: 993, tls: true)
+      #
+      #     # DEPRECATED: certs = path to a directory
+      #     Net::IMAP.new("imap.example.com", nil, true, "/path/to/certs") # => prints a warning
+      #     # Not deprecated: OpenSSL::SSL::SSLContext param: ca_path
+      #     Net::IMAP.new("imap.example.com", tls: {ca_path: "/path/to/certs", verify_mode: VERIFY_PEER})
+      #
+      #     # DEPRECATED: certs = path to a file
+      #     Net::IMAP.new("imap.example.com", nil, true, "/path/to/cert.pem") # => prints a warning
+      #     # Not deprecated: OpenSSL::SSL::SSLContext param: ca_file
+      #     Net::IMAP.new("imap.example.com", tls: {ca_file: "/path/to/cert.pem", verify_mode: VERIFY_PEER})
+      #
+      #     # DEPRECATED: verify = false
+      #     Net::IMAP.new("imap.example.com", nil, true, nil, false) # => prints a warning
+      #     # Not deprecated: +tls+ OpenSSL::SSL::SSLContext param: verify_mode
+      #     Net::IMAP.new("imap.example.com", tls: {verify_mode: VERIFY_NONE})
+      #
+      def initialize(host,
+                     port_or_options = UNDEF,
+                     usessl          = UNDEF,
+                     certs           = UNDEF,
+                     verify          = UNDEF,
+                     **options)
+        if port_or_options == UNDEF
+          translate_ssl_to_tls(options)
+          super(host, **options)
+        elsif options.any?
+          raise ArgumentError, "send options as keyword arguments only"
+
+        elsif usessl == UNDEF
+          if (options = Hash.try_convert(port_or_options)&.dup)
+            translate_ssl_to_tls(options)
+            super(host, **options)
+          else
+            super(host, port: port_or_options)
+          end
+        elsif port_or_options.respond_to?(:to_hash)
+          raise ArgumentError, "do not send extra arguments with options hash"
+
+        else
+          warn "DEPRECATED: Net::IMAP.new(host, port, usessl, certs, verify).  " \
+               "Use Net::IMAP.new(host, **options) instead."
+          super(host,
+                port: port_or_options,
+                tls: usessl && obsolete_create_ssl_params(certs, verify))
+        end
+      end
+
+      # :call-seq:
+      #   starttls(options = {})
+      #   starttls(certs, verify = true)
+      #
+      # For backward compatibility.  A future release will only accept
+      # OpenSSL::SSL::SSLContext.set_params options.
+      def starttls(options_or_certs = UNDEF, verify = UNDEF, **options)
+        if options_or_certs == UNDEF
+          super(**options)
+        elsif options.any?
+          raise ArgumentError, "send options as keyword arguments only"
+
+        elsif (options = Hash.try_convert(options_or_certs))
+          if verify != UNDEF
+            raise ArgumentError, "do not send extra arguments with options hash"
+          end
+          super(**options)
+
+        elsif (certs = String.try_convert(options_or_certs))
+          warn "DEPRECATED: starttls(certs, verify).  " \
+               "Use starttls(**ssl_context_params) instead."
+          super(**obsolete_create_ssl_params(certs, verify))
+
+        else
+          raise ArgumentError, "send options as keyword arguments"
+        end
+      end
+
+      private
+
+      def translate_ssl_to_tls(options)
+        return unless options.key?(:ssl)
+        if options.key?(:tls)
+          raise ArgumentError, "conflicting :ssl and :tls keyword arguments"
+        end
+        options.merge!(tls: options.delete(:ssl))
+      end
+
+      def obsolete_create_ssl_params(certs, verify)
+        certs  = nil  if certs  == UNDEF
+        verify = true if verify == UNDEF
+        params = {}
+        if certs
+          if File.file?(certs)
+            params[:ca_file] = certs
+          elsif File.directory?(certs)
+            params[:ca_path] = certs
+          end
+        end
+        params[:verify_mode] = verify ? VERIFY_PEER : VERIFY_NONE
+        params
+      end
+
+    end
+  end
+end