emailjs / emailjs-imap-client

Low-level JS IMAP client for all your IMAP needs.
MIT License
552 stars 123 forks source link

Don't ignore errors from updateId (ID commands) while connecting. #260

Closed gudmundurg74 closed 2 years ago

gudmundurg74 commented 2 years ago

We have been running into socket errors that get ignored which puts the connection in an unusable state, leaving it with a hanging promise/await which breaks our connection management code.

I wonder if that catch was put there for the case where the server returns BAD because it doesn't support the ID command, but there is actually a check for that at the beginning of updateId(). In any case this catch wouldn't just ignore BAD responses but also any and all errors which it shouldn't do.

nifgraup commented 2 years ago

Here is the original commit for the try-catch

commit b8eae12b0ccea5cd0034b8b72e3cb370ff904e43 (geir/dont-fail-id)
Author: geir.gunnarsson <geir@vivaldi.com>
Date:   Tue Dec 27 16:25:08 2016 +0000

    Do not fail connection even though ID command fails

    As an example poczta.o2.pl lists ID capability both before and after login
    while it only works after login.

diff --git a/src/emailjs-imap-client.js b/src/emailjs-imap-client.js
index bfb1ba8..41c4396 100644
--- a/src/emailjs-imap-client.js
+++ b/src/emailjs-imap-client.js
@@ -117,7 +117,8 @@
         }).then(() => {
             return this.upgradeConnection();
         }).then(() => {
-            return this.updateId(this.options.id);
+            return this.updateId(this.options.id)
+            .catch(err => this.logger.warn('Failed to update id', err));
         }).then(() => {
             return this.login(this.options.auth);
         }).then(() => {

updateId already had an ID capability check at that point. If we remove the try-catch, then we need to find another solution for servers that claim to support ID capability while not supporting it.