ruby / net-imap

Ruby client api for Internet Message Access Protocol
https://ruby.github.io/net-imap
Other
61 stars 29 forks source link

"ResponseParseError: unexpected token `NUMBER` (expected `ATOM`)" on old exchange mail servers #250

Open hoffi opened 10 months ago

hoffi commented 10 months ago

Needed to work with an old exchange mail server. The ID command reports: ("name" "Microsoft.Exchange.Imap4.Imap4Server" "version" "15.0") which according to this microsoft document should be an Exchange 2013 server.

After i sent an UID MOVE command it failed with the mentioned ResponseParserError. I enabled the debug logging and got the following output:

[...]
S: RUBY0003 OK [READ-WRITE] SELECT completed.
C: RUBY0004 UID MOVE 10 PROCESSING
S: [COPYUID 124 10 233]
# (net-imap crashes at this point)
S: * 1 EXPUNGE
S: * 69 EXISTS
S: RUBY0004 OK MOVE completed.

As i understand RFC 2359 there should be an OK in front of the COPYUID?

Should the parser be able to handle such incorrect responses? I have managed to build a hack for the parser locally to get it parsed and could submit a PR for the fix after some refactoring.

I have also checked against the Exchange implementation for Microsoft 365 and this does not have the error. The ID command reports: ("name" "Microsoft.Exchange.Imap4.Imap4Server" "version" "15.20") here.

nevans commented 10 months ago

(Sorry for the slow reply)

Yeah, every response must start with one of:

It boggles my mind how often servers, including the ones from the biggest software companies in the world, fail at the absolute basics of the protocol. I do think that Net::IMAP should be more resilient to buggy responses, in general. But I'm hesitant to include something this far out-of-spec into the parser... yet. I've actually been working through basic quoted string escape bugs from a certain fruit-named company's iMAP iServer, for the last week or so. Ultimately, I don't think I'll even be pushing a PR for that workaround either. 🙁

nevans commented 10 months ago

I do have other ideas for improving robustness:

So, although I'm reluctant to include it in the generic parser code, I'm curious to see what you came up with, and how you solved this for yourself. If you post something (in this ticket or in a PR), at the very least that documents the workaround for others who get to this issue via search engines.

nevans commented 10 months ago

Oh, although I doubt that it makes any difference for this issue, you should use RFC4315 for UIDPLUS (or RFC9051, which includes UIDPLUS). RFC2359 is obsolete (and was already obsolete for a nearly decade by 2013). But it is sometimes informative to read the obsolete RFCs, especially when dealing with very old servers. 🙂

hoffi commented 10 months ago

(Sorry for the slow reply)

No worries :)

So, although I'm reluctant to include it in the generic parser code, I'm curious to see what you came up with, and how you solved this for yourself. If you post something (in this ticket or in a PR), at the very least that documents the workaround for others who get to this issue via search engines.

I have got it working using these changes. But i am not sure if there are any unwanted side effects with other servers and so on.

diff --git a/lib/net/imap/response_parser.rb b/lib/net/imap/response_parser.rb
index 1aab798..d2dc08e 100644
--- a/lib/net/imap/response_parser.rb
+++ b/lib/net/imap/response_parser.rb
@@ -655,6 +655,7 @@ module Net
         resp = case lookahead!(T_PLUS, T_STAR, *TAG_TOKENS).symbol
                when T_PLUS then continue_req
                when T_STAR then response_data
+               when T_LBRA then invalid_exchange_response
                else             response_tagged
                end
         accept_spaces # QUIRKY: Ignore trailing space (MS Exchange Server?)
@@ -776,6 +777,10 @@ module Net
       alias comparator_data         response_data__unhandled
       alias message_data__converted response_data__unhandled

+      def invalid_exchange_response
+        UntaggedResponse.new(OK, resp_text, @str)
+      end
+
       # RFC3501 & RFC9051:
       #   response-tagged = tag SP resp-cond-state CRLF
       def response_tagged

However i do not use this parser change anymore as i have seen that it seems to work with the UID COPY command. Basically i now use UID COPY to copy it to the new mailbox and after that i use UID STORE and UID EXPUNGE to delete the old message to mimic a MOVE that way:

@imap.select(source_mailbox)
response = @imap.uid_copy(uid_to_copy, destination_mailbox)
@imap.uid_store(uid_to_copy, '+FLAGS', [Net::IMAP::DELETED])
@imap.uid_expunge(uid_to_copy)
uid_after_copy = response.data.code.data.assigned_uids.first

Ironically the UID COPY also responds with a COPYUID, but for whatever reason it is correct in that case, so it works without any parser errors. Strange.

Here the debug log for this:

[...]
S: RUBY0009 OK [READ-WRITE] SELECT completed.
C: RUBY0010 UID COPY 58 PROCESSED
S: RUBY0010 OK [COPYUID 6017 58 295] COPY completed.
C: RUBY0011 UID STORE 58 +FLAGS (\Deleted)
S: * 1 FETCH (FLAGS (\Deleted))
S: RUBY0011 OK STORE completed.
C: RUBY0012 UID EXPUNGE 58
S: * 1 EXPUNGE
S: * 66 EXISTS
S: RUBY0012 OK EXPUNGE completed.
C: RUBY0013 LOGOUT
S: * BYE Microsoft Exchange Server 2013 IMAP4 server signing off.
S: RUBY0013 OK LOGOUT completed.

At the very least, you should be able to supply your own parser when you instantiate a client. And we can add server-specific hacks into parser subclasses or into modules that are prepended to the parser.

That sounds interesting. Also the existing hacks in the net-imap gem could be extracted into such modules and can be included by the user when needed?