From f9bed0d4b575936e9ac9b210dde6714c72596384 Mon Sep 17 00:00:00 2001 From: nick evans Date: Fri, 3 Jul 2026 13:35:14 -0400 Subject: [PATCH 1/5] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Refactor=20`#starttls`?= =?UTF-8?q?=20error=20handling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Combine the two error raising conditionals into a single if/elsif stmt. --- lib/net/imap.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/net/imap.rb b/lib/net/imap.rb index e9fbb3ef..2614b6db 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -1482,8 +1482,7 @@ def starttls(**options) if error disconnect reraise error - end - unless handled + elsif !handled disconnect raise InvalidResponseError, "STARTTLS handler was bypassed, although server responded %p" % [ From 26d17f1a07cc66318fe4ac84a6bd94cbea9ee116 Mon Sep 17 00:00:00 2001 From: nick evans Date: Fri, 3 Jul 2026 13:36:41 -0400 Subject: [PATCH 2/5] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Refactor=20`#starttls`?= =?UTF-8?q?=20disconnect=20error=20handling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Combine `#disconnect` and raise for both error handlers. --- lib/net/imap.rb | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/lib/net/imap.rb b/lib/net/imap.rb index 2614b6db..f4c806cf 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -1479,15 +1479,18 @@ def starttls(**options) rescue Exception => error raise # note that the error backtrace is in the receiver_thread end - if error - disconnect - reraise error - elsif !handled + begin + if error + reraise error + elsif !handled + raise InvalidResponseError, + "STARTTLS handler was bypassed, although server responded %p" % [ + ok.raw_data.chomp + ] + end + rescue Exception => error disconnect - raise InvalidResponseError, - "STARTTLS handler was bypassed, although server responded %p" % [ - ok.raw_data.chomp - ] + raise end ok end From b4da8c2aa451c788a6b0476427791711304829ef Mon Sep 17 00:00:00 2001 From: nick evans Date: Fri, 3 Jul 2026 13:11:11 -0400 Subject: [PATCH 3/5] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Drop=20redundant=20Inv?= =?UTF-8?q?alidResponseError=20disconnect?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `#get_tagged_response` is called twice, from `#send_command` and `#idle`, and both callers already handle `InvalidResponseError` by disconnecting. --- lib/net/imap.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/net/imap.rb b/lib/net/imap.rb index f4c806cf..3eebfacb 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -3738,7 +3738,6 @@ def get_tagged_response(tag, cmd, timeout = nil) when /\A(?:BAD)\z/ni raise BadResponseError, resp else - disconnect raise InvalidResponseError, "invalid tagged resp: %p" % [resp.raw_data.chomp] end end From 0be2c07d9d8ce4f7211241f27416bef22f9c9fe9 Mon Sep 17 00:00:00 2001 From: nick evans Date: Fri, 3 Jul 2026 13:40:01 -0400 Subject: [PATCH 4/5] =?UTF-8?q?=F0=9F=A5=85=20Save=20client=20thread=20con?= =?UTF-8?q?nection=20dropping=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The client thread may decide to drop the connection when it sees these exceptions: * `InvalidResponseError` in `#send_command` * `InvalidResponseError` in `#idle` (TODO: refactor to `#send_command`) * _Any_ `Exception` after `#starttls` returns tagged `OK` This saves those to an instance variable, which may be used as a cause for other raised errors. --- lib/net/imap.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/net/imap.rb b/lib/net/imap.rb index 3eebfacb..6daae8da 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -1479,7 +1479,7 @@ def starttls(**options) rescue Exception => error raise # note that the error backtrace is in the receiver_thread end - begin + synchronize do if error reraise error elsif !handled @@ -1489,6 +1489,7 @@ def starttls(**options) ] end rescue Exception => error + @client_disconnect_error ||= error disconnect raise end @@ -3271,7 +3272,8 @@ def idle(timeout = nil, &response_handler) response = get_tagged_response(tag, "IDLE", idle_response_timeout) end end - rescue InvalidResponseError + rescue InvalidResponseError => error + @client_disconnect_error ||= error disconnect raise end @@ -3701,7 +3703,8 @@ def send_command(cmd, *args, &block) ensure remove_response_handler(block) if block end - rescue InvalidResponseError + rescue InvalidResponseError => error + @client_disconnect_error ||= error disconnect raise end From 0841b089223dc493ef037320d1fdda5151561afb Mon Sep 17 00:00:00 2001 From: nick evans Date: Tue, 2 Jun 2026 16:57:42 -0400 Subject: [PATCH 5/5] =?UTF-8?q?=F0=9F=A5=85=20Guard=20against=20tagged=20r?= =?UTF-8?q?esponses=20sent=20too=20soon?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The premature tagged response guard that was added for #644 only checked for premature `OK`, not for premature `BAD` or `NO`. When those are detected prior to the tag being sent, this treats those cases as the same sort of error. Note that this explicitly raises a "closed stream" IOError when disconnected. This is the error that would be raised anyway, if `send_command` were allowed to write to the connection. But, if we can test that the socket is already closed, we can raise the error directly. There's no need to attempt to format and send data. --- lib/net/imap.rb | 17 +++++++++++++---- test/net/imap/test_imap.rb | 23 ++++++++++++++--------- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/lib/net/imap.rb b/lib/net/imap.rb index 6daae8da..6d740b9f 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -3252,8 +3252,8 @@ def idle(timeout = nil, &response_handler) response = nil synchronize do - tag = Thread.current[:net_imap_tag] = generate_tag - command = Command[tag:, name: "IDLE"] + command = start_command(name: "IDLE") + tag = Thread.current[:net_imap_tag] = command.tag # TODO: remove this put_string("#{tag} IDLE#{CRLF}") finish_sending_command(command) @@ -3687,8 +3687,8 @@ def send_command_with_continuations(cmd, *args) def send_command(cmd, *args, &block) args.each do validate_data _1 end synchronize do - tag = generate_tag - command = Command[tag:, name: cmd] + command = start_command(name: cmd) + tag = command.tag put_string(tag + " " + cmd) args.each do |i| put_string(" ") @@ -3710,6 +3710,15 @@ def send_command(cmd, *args, &block) end end + def start_command(**) + raise IOError, "closed stream" if disconnected? + command = Command.new(**, tag: generate_tag) + if (response = @tagged_responses[command.tag]) + raise InvalidTaggedResponseError.new(:unstarted, command:, response:) + end + command + end + # NOTE: This must be synchronized with sending the command's final CRLF and # adding any command-related response handlers. def finish_sending_command(command) diff --git a/test/net/imap/test_imap.rb b/test/net/imap/test_imap.rb index d005bc50..ade5f1af 100644 --- a/test/net/imap/test_imap.rb +++ b/test/net/imap/test_imap.rb @@ -220,13 +220,16 @@ def test_starttls_stripping_ok_sent_before_response end end - # Similar to STARTTLS stripping test, but checks other commands too + # Similar to STARTTLS stripping test, but checks other commands and statuses data( - "IDLE" => ->imap do imap.idle(1) do end end, - "NOOP" => ->imap do imap.noop end, - "SELECT" => ->imap do imap.select("inbox") end, + "OK for IDLE" => ["OK", ->imap do imap.idle(1) do end end], + "OK for NOOP" => ["OK", ->imap do imap.noop end], + "NO for IDLE" => ["NO", ->imap do imap.idle(1) do end end], + "NO for EXAMINE" => ["NO", ->imap do imap.examine("inbox") end], + "BAD for IDLE" => ["BAD", ->imap do imap.idle(1) do end end], + "BAD for SELECT" => ["BAD", ->imap do imap.select("inbox") end], ) - test "premature tagged OK response" do |cmd| + test "premature tagged response" do |(status, cmd)| timeout = 5 timeout *= EnvUtil.timeout_scale || 1 if defined?(EnvUtil.timeout_scale) Timeout.timeout(timeout) do @@ -240,9 +243,9 @@ def test_starttls_stripping_ok_sent_before_response begin sock.print("* OK test server\r\n") assert_equal :send_malicious_responses, client_to_server.pop - sock.print("RUBY0001 OK invalid\r\n") - sock.print("RUBY0002 OK false\r\n") - sock.print("RUBY0003 OK tricky\r\n") + sock.print("RUBY0001 #{status} invalid\r\n") + sock.print("RUBY0002 #{status} false\r\n") + sock.print("RUBY0003 #{status} tricky\r\n") server_to_client << :sent_malicious_responses sock.gets ensure @@ -260,7 +263,9 @@ def test_starttls_stripping_ok_sent_before_response assert_equal :sent_malicious_responses, server_to_client.pop assert_equal [1, 2, 3], 3.times.map { rcvr_to_client.pop } # should respond this way for _any_ command - assert_local_raise(Net::IMAP::InvalidTaggedResponseError) do + assert_local_raise( + Net::IMAP::InvalidTaggedResponseError, / unstarted .*tag=RUBY0001/ + ) do cmd.(imap) end assert imap.disconnected?