♻️ Extract InvalidTaggedResponseError

This converts some instances of `InvalidResponseError` into a subclass:
`InvalidTaggedResponseError`.  This refactors code that was introduced
by #664, which raises an exception when the server sends a tagged `OK`
response for a command *before* that command has been fully sent.

Please note that the exception class documents more error conditions
than are currently handled.

It also adds a very simple `Command` data object to encapsulate
information that's helpful for this error reporting.  Future commits
may use these command objects to track the state of active commands.

Author: nevans

lib/net/imap.rb

@@ -3201,7 +3201,8 @@ def idle(timeout = nil, &response_handler)
 
       synchronize do
         tag = Thread.current[:net_imap_tag] = generate_tag
-        guard_against_tagged_response_skipping_handler!(tag, "IDLE")
+        command = Command[tag:, name: "IDLE"]
+        finish_sending_command(command)
         put_string("#{tag} IDLE#{CRLF}")
 
         begin
@@ -3661,13 +3662,14 @@ def send_command(cmd, *args, &block)
           validate_data(i)
         end
         tag = generate_tag
+        command = Command[tag:, name: cmd]
         put_string(tag + " " + cmd)
         args.each do |i|
           put_string(" ")
           send_data(i, tag)
         end
         @logout_command_tag = tag if cmd == "LOGOUT"
-        guard_against_tagged_response_skipping_handler!(tag, cmd)
+        finish_sending_command(command)
         add_response_handler(&block) if block
         begin
           put_string(CRLF)
@@ -3681,14 +3683,12 @@ def send_command(cmd, *args, &block)
       raise
     end
 
-    def guard_against_tagged_response_skipping_handler!(tag, cmd)
-      return unless (resp = @tagged_responses[tag])&.name&.upcase == "OK"
-      raise InvalidResponseError, format(
-        "Received tagged 'OK' to incomplete %s command (tag=%s).  " \
-        "This could indicate a malicious server, a man-in-the-middle, or " \
-        "client-side command injection.  Disconnecting.",
-        cmd, tag
-      )
+    # NOTE: This must be synchronized with sending the command's final CRLF and
+    # adding any command-related response handlers.
+    def finish_sending_command(command)
+      if (response = @tagged_responses[command.tag])&.name&.casecmp?("OK")
+        raise InvalidTaggedResponseError.new(:incomplete, command:, response:)
+      end
     end
 
     def generate_tag

lib/net/imap/command_data.rb

@@ -141,6 +141,9 @@ def send_list_data(list, tag = nil)
     def send_date_data(date) put_string Net::IMAP.encode_date(date) end
     def send_time_data(time) put_string Net::IMAP.encode_time(time) end
 
+    # NOTE: Currently for internal use only.  The API is expected to change.
+    Command = Data.define(:tag, :name) # :nodoc:
+
     CommandData = Data.define(:data) do # :nodoc:
       def self.validate(...)
         data = new(...)

lib/net/imap/errors.rb

@@ -279,7 +279,11 @@ class ByeResponseError < ResponseError
     #
     # This is different from UnknownResponseError: the response has been
     # rejected.  Although it may be parsable, the server is forbidden from
-    # sending it in the current context.  The client should automatically
+    # sending it in the current context.
+    #
+    # This could be caused by a bug in the server or in Net::IMAP.  Or it
+    # might indicate a malicious server, a man-in-the-middle attack, or
+    # client-side command injection.  So the client should automatically
     # disconnect, abruptly (without logout).
     #
     # Note that InvalidResponseError does not inherit from ResponseError: it
@@ -288,6 +292,67 @@ class ByeResponseError < ResponseError
     class InvalidResponseError < Error
     end
 
+    # Error raised when the server sends a tagged #response that is invalid for
+    # the #command #state.
+    #
+    # This could be caused by a bug in the server or in Net::IMAP.  Or it
+    # might indicate a malicious server, a man-in-the-middle attack, or
+    # client-side command injection, so the client should disconnect
+    # automatically and abruptly (without logout).
+    class InvalidTaggedResponseError < InvalidResponseError
+      # A symbol representing the state of the matching tagged command.
+      #
+      # +:unknown+::
+      #   #response does not match any known command (#command will be +nil+).
+      # +:unstarted+::
+      #   Any tagged #response is invalid before #command starts sending.
+      # +:incomplete+::
+      #   A tagged +OK+ #response is invalid before #command is fully sent.
+      # +:completed+::
+      #   Multiple tagged responses were received for the same command.
+      #
+      # NOTE: Command state is neither anticipated nor remembered indefinitely.
+      # +:unknown+ is used before the matching command is called and after the
+      # it is forgotten.
+      #
+      # NOTE: This version of Net::IMAP does not detect or raise an exception
+      # for all of these states.
+      attr_reader :state
+
+      # Metadata about the matching IMAP command.
+      #
+      # Returns +nil+ when #state is +:unknown+.
+      #
+      # NOTE: The non-nil return type is an unstable API, for debug only.  It
+      # may be changed by any release, without warning or deprecation.
+      attr_reader :command
+
+      # The TaggedResponse which triggered this error
+      attr_reader :response
+
+      def initialize(state, response:, command: nil)
+        response => TaggedResponse[tag:, name: status]
+        case [state, status, command]
+        in :unknown,                _,    nil
+        in :incomplete,             "OK", {tag: ^tag, name:}
+        in :unstarted | :completed, _,    {tag: ^tag, name:}
+        end
+        @state, @command, @response = state, command, response
+        cmd_desc = name ? "#{state} #{name}" : state
+        super "Received tagged #{status} to #{cmd_desc} command (tag=#{tag})"
+      rescue NoMatchingPatternError => err
+        raise ArgumentError, err.message
+      end
+
+      def detailed_message(**)
+        "#{message}.\n" \
+          "Disconnecting: This could indicate a malicious server, a " \
+          "man-in-the-middle attack, a client-side command injection, or " \
+          "a bug in net-imap.\n" \
+          "response.data=#{response.data.inspect}"
+      end
+    end
+
     # Error raised upon an unknown response from the server.
     #
     # This is different from InvalidResponseError: the response may be a

test/net/imap/test_errors.rb

@@ -219,4 +219,52 @@ def self.SGR(*attr) = CSI attr.join(?;), ?m
                  "exceeds max_response_size (1200B)", err.message)
   end
 
+  test "InvalidTaggedResponseError" do
+    assert_raise(ArgumentError) do Net::IMAP::InvalidTaggedResponseError.new end
+    assert_raise(ArgumentError) do
+      Net::IMAP::InvalidTaggedResponseError.new("foo", response: nil)
+    end
+    assert_raise(ArgumentError) do
+      Net::IMAP::InvalidTaggedResponseError.new(:unknown, response: nil)
+    end
+
+    response = Net::IMAP::TaggedResponse[
+      tag: "RUBY0001", name: "NO",
+      data: Net::IMAP::ResponseText[code: nil, text: "cheating"]
+    ]
+    assert_raise(ArgumentError) do
+      Net::IMAP::InvalidTaggedResponseError.new(:unstarted, response:)
+    end
+
+    err = Net::IMAP::InvalidTaggedResponseError.new(:unknown, response:)
+    assert_match(/tagged NO to unknown command.*tag=RUBY0001\b/i, err.message)
+    assert_match(/\bdisconnecting\b/i, err.detailed_message)
+    assert_match(/response.data=#<struct Net::IMAP::ResponseText.+cheating.*>/i,
+                 err.detailed_message)
+
+    command = Net::IMAP::Command[tag: "mismatch", name: "NOOP"]
+    assert_raise(ArgumentError) do
+      Net::IMAP::InvalidTaggedResponseError.new(:unstarted, response:, command:)
+    end
+
+    command = Net::IMAP::Command[tag: "RUBY0001", name: "NOOP"]
+    err = Net::IMAP::InvalidTaggedResponseError.new(:unstarted, response:, command:)
+    assert_match(/tagged NO to unstarted NOOP command.*tag=RUBY0001\b/i,
+                 err.message)
+    assert_match(/\bdisconnecting\b/i, err.detailed_message)
+    assert_match(/response.data=#<struct Net::IMAP::ResponseText.+cheating.*>/i,
+                 err.detailed_message)
+
+    response = Net::IMAP::TaggedResponse[
+      tag: "RUBY0001", name: "OK",
+      data: Net::IMAP::ResponseText[code: nil, text: "cheating"]
+    ]
+    command = Net::IMAP::Command[tag: "RUBY0001", name: "STARTTLS"]
+    err = Net::IMAP::InvalidTaggedResponseError.new(:incomplete, response:, command:)
+    assert_match(/tagged OK to incomplete STARTTLS command.*tag=RUBY0001\b/i,
+                 err.message)
+    assert_match(/\bdisconnecting\b/i, err.detailed_message)
+    assert_match(/response.data=#<struct Net::IMAP::ResponseText.+cheating.*>/i,
+                 err.detailed_message)
+  end
 end

test/net/imap/test_imap.rb

@@ -195,7 +195,7 @@ def test_starttls_stripping_ok_sent_before_response
         client_to_server << :send_malicious_response
         assert_equal :malicious_response_sent, server_to_client.pop
         sleep 0.010 # to be sure the network buffers have flushed, etc
-        assert_raise(Net::IMAP::InvalidResponseError) do
+        assert_raise(Net::IMAP::InvalidTaggedResponseError) do
           imap.starttls(:ca_file => CA_FILE)
         end
         assert imap.disconnected?