Fix two bugs that misreport why a run could not connect

A TLS failure was reported as an unreachable host. ssl.SSLError is a
subclass of OSError, so the reachability handler in connect() - listed
first - caught every one of them and the TLS branch behind it was dead
code. Pointing the tool at a plaintext port (143 instead of 993), or at a
server with an expired or untrusted certificate, printed "Could not reach
host:port" and told the user to check an internet connection that was
working fine, never mentioning the port they had actually mistyped. The
"TLS handshake with ... failed" error the README documents in
Troubleshooting could not be produced at all. Catch ssl.SSLError first,
and name 993 in the hint since a wrong port is the usual cause.

A quoted .env value with a trailing comment kept its quotes. _clean_value
took the quoted branch only when the line *ended* on the closing quote, so
an ordinary annotated line - EMAIL_CLEANER_PASSWORD="abcd efgh"  # gmail
app password - matched neither branch: the inline-comment strip ran
instead and left the quotes in. IMAP then got a password with two stray
'"' characters and rejected it, and the user was told their app password
was wrong. Parse the quoted case up to its closing quote and treat
whatever follows as the comment it is.

Adds regression tests for both, including that an unreachable host and a
rejected password still report as themselves.

Author: darcodev

email_cleaner/config.py

@@ -45,10 +45,19 @@ def _clean_value(value: str) -> str:
     A quoted value keeps everything between the quotes verbatim, so a '#' or
     spaces in a password survive. An unquoted value has a trailing inline
     '# comment' stripped, matching how people annotate a .env.
+
+    The quoted case ends at its closing quote rather than at the end of the
+    line. Requiring the line to *finish* on that quote meant a perfectly
+    ordinary annotated line - PASSWORD="a b c"  # gmail app password - matched
+    neither branch: the quotes stayed in, and IMAP got a password with two
+    stray '"' in it and rejected it as simply wrong.
     """
     value = value.strip()
-    if len(value) >= 2 and value[0] == value[-1] and value[0] in "\"'":
-        return value[1:-1]
+    quote = value[:1]
+    if quote in ("'", '"'):
+        end = value.find(quote, 1)
+        if end != -1:
+            return value[1:end]  # anything past the closing quote is a comment
     marker = value.find(" #")  # inline comment must have space before the hash
     if marker != -1:
         value = value[:marker]

email_cleaner/imap_client.py

@@ -242,16 +242,25 @@ def connect(self) -> None:
             self._imap = imaplib.IMAP4_SSL(
                 self.host, self.port, ssl_context=ssl.create_default_context(), timeout=30
             )
+        # ssl.SSLError is a subclass of OSError, so it has to be caught first.
+        # Behind the reachability handler it was dead code, and every TLS
+        # failure - a plaintext port like 143 answering the handshake, an
+        # expired or untrusted certificate - came out as "could not reach
+        # host", telling the user to check an internet connection that was
+        # working fine and never mentioning the port they had actually mistyped.
+        except ssl.SSLError as exc:
+            raise CleanerError(
+                f"TLS handshake with {self.host} failed ({exc}).",
+                hint=(
+                    "The server may not support implicit TLS on this port. "
+                    "Most IMAP servers use 993; try --port 993."
+                ),
+            ) from exc
         except (socket.gaierror, TimeoutError, OSError) as exc:
             raise CleanerError(
                 f"Could not reach {self.host}:{self.port} ({exc}).",
                 hint="Check your internet connection and the IMAP host name.",
             ) from exc
-        except ssl.SSLError as exc:
-            raise CleanerError(
-                f"TLS handshake with {self.host} failed ({exc}).",
-                hint="The server may not support implicit TLS on this port.",
-            ) from exc
 
         try:
             self._imap.login(self.address, self._password)

tests/test_units.py

@@ -6,6 +6,8 @@
 import imaplib
 import json
 import os
+import socket
+import ssl
 import sys
 import tempfile
 import unittest
@@ -499,6 +501,47 @@ def test_a_server_that_really_has_nothing_is_believed(self):
         self.assertFalse(session.supports_gmail_search)
 
 
+class TestConnectErrors(unittest.TestCase):
+    """ssl.SSLError subclasses OSError, so catching the latter first swallows
+    every TLS failure and blames the network for a wrong port or a bad cert."""
+
+    def _connect_with(self, exc):
+        session = ImapSession("mail.example.com", 143, "me@x.com", "pw")
+        with mock.patch("imaplib.IMAP4_SSL", side_effect=exc):
+            with self.assertRaises(CleanerError) as caught:
+                session.connect()
+        return caught.exception
+
+    def test_tls_failure_is_reported_as_a_tls_failure(self):
+        err = self._connect_with(ssl.SSLError("WRONG_VERSION_NUMBER"))
+        self.assertIn("TLS handshake", str(err))
+        self.assertIn("993", err.hint)
+
+    def test_a_bad_certificate_is_a_tls_failure_too(self):
+        err = self._connect_with(ssl.SSLCertVerificationError("certificate verify failed"))
+        self.assertIn("TLS handshake", str(err))
+
+    def test_an_unreachable_host_still_reports_reachability(self):
+        for exc in (
+            socket.gaierror("name or service not known"),
+            TimeoutError("timed out"),
+            ConnectionRefusedError("refused"),
+        ):
+            with self.subTest(exc=type(exc).__name__):
+                err = self._connect_with(exc)
+                self.assertIn("Could not reach", str(err))
+                self.assertNotIn("TLS", str(err))
+
+    def test_a_rejected_password_is_still_a_login_error(self):
+        session = ImapSession("mail.example.com", 993, "me@x.com", "pw")
+        conn = mock.Mock()
+        conn.login.side_effect = imaplib.IMAP4.error("AUTHENTICATIONFAILED")
+        with mock.patch("imaplib.IMAP4_SSL", return_value=conn):
+            with self.assertRaises(CleanerError) as caught:
+                session.connect()
+        self.assertIn("Login failed", str(caught.exception))
+
+
 class TestSessionTeardown(unittest.TestCase):
     """IMAP CLOSE purges every \\Deleted message in a writable mailbox on its
     way out - the same unscoped wipe UID EXPUNGE exists to avoid. 'clean' opens
@@ -689,6 +732,27 @@ def test_hash_inside_quoted_value_is_kept(self):
         config.load_dotenv(self._write('EMAIL_CLEANER_PASSWORD="a # b"\n'))
         self.assertEqual(os.environ["EMAIL_CLEANER_PASSWORD"], "a # b")
 
+    def test_quoted_value_with_a_trailing_comment_loses_its_quotes(self):
+        # the quotes used to survive into the value, because the line no longer
+        # *ended* on one - so IMAP got a password with two stray '"' in it and
+        # the user was told their app password was wrong
+        config.load_dotenv(
+            self._write('EMAIL_CLEANER_PASSWORD="abcd efgh"  # gmail app password\n')
+        )
+        self.assertEqual(os.environ["EMAIL_CLEANER_PASSWORD"], "abcd efgh")
+
+    def test_single_quoted_value_with_a_trailing_comment(self):
+        config.load_dotenv(self._write("EMAIL_CLEANER_HOST='h.example.com'  # main\n"))
+        self.assertEqual(os.environ["EMAIL_CLEANER_HOST"], "h.example.com")
+
+    def test_a_hash_after_the_closing_quote_is_a_comment_one_inside_is_not(self):
+        config.load_dotenv(self._write('EMAIL_CLEANER_PASSWORD="a # b"  # note\n'))
+        self.assertEqual(os.environ["EMAIL_CLEANER_PASSWORD"], "a # b")
+
+    def test_unterminated_quote_is_left_alone(self):
+        config.load_dotenv(self._write('EMAIL_CLEANER_HOST="h.example.com\n'))
+        self.assertEqual(os.environ["EMAIL_CLEANER_HOST"], '"h.example.com')
+
     def test_real_env_is_not_overwritten(self):
         os.environ["EMAIL_CLEANER_EMAIL"] = "real@env.com"
         config.load_dotenv(self._write("EMAIL_CLEANER_EMAIL=file@env.com\n"))