fix(email): close IMAP socket when connect/login fails (#3174) (#3363)

* fix(email): close IMAP socket when connect/login fails (#3174)

_imap_connect opened a live socket via _open_imap_connection and then
called conn.login() with no try/finally, and _open_imap_connection called
conn.starttls() unguarded. When auth fails (e.g. an Office 365 app password
on an MFA-enabled tenant, #3174) or STARTTLS is rejected, the already-open
socket was orphaned. Every IMAP caller funnels through _imap_connect,
including the 30-minute _auto_summarize_poller, so a persistently
misconfigured account leaked one descriptor per pass toward FD exhaustion.

The previously merged leak fixes (#1325/#1330/#1423/#1530) only guard the
post-connect body and monkeypatch _imap_connect to succeed, so this
connect-time path was uncovered. Wrap login() and starttls() so a failure
calls conn.shutdown() (low-level close; logout() can't run pre-auth) before
re-raising. Adds two regression tests that fail without the guard.

* fix(email): guard MCP IMAP+SMTP connect-time leaks too (#3174)

Folds in the sibling connect-time leaks vdmkenny flagged on #3363, so the
whole connect-then-step leak class is closed in one place:

- mcp_servers/email_server.py::_imap_connect — guard starttls() and login();
  close pre-auth with conn.shutdown() before re-raising.
- mcp_servers/email_server.py::_smtp_connect — guard starttls() and login();
  SMTP has no shutdown(), so close with conn.close() (socket close, no QUIT).

Routes SMTP (_send_smtp_message) is already safe via 'with smtplib.SMTP(...)'.
Adds four regression tests (one per guard), verified to fail without the fix.

mcp_servers/email_server.py

@@ -245,10 +245,27 @@ def _imap_connect(account: str | None = None):
             timeout=EMAIL_SOCKET_TIMEOUT,
         )
         if cfg["imap_starttls"]:
-            conn.starttls()
+            try:
+                conn.starttls()
+            except Exception:
+                # Don't leak the open plain socket on a rejected STARTTLS. (#3174)
+                try:
+                    conn.shutdown()
+                except Exception:
+                    pass
+                raise
     if getattr(conn, "sock", None):
         conn.sock.settimeout(EMAIL_SOCKET_TIMEOUT)
-    conn.login(cfg["imap_user"], cfg["imap_password"])
+    try:
+        conn.login(cfg["imap_user"], cfg["imap_password"])
+    except Exception:
+        # A failed login otherwise orphans the connected socket; close it
+        # before propagating (shutdown() is the pre-auth low-level close). (#3174)
+        try:
+            conn.shutdown()
+        except Exception:
+            pass
+        raise
     return conn
 
 
@@ -778,7 +795,16 @@ def _smtp_connect(account=None, cfg=None):
             port,
             timeout=EMAIL_SOCKET_TIMEOUT,
         )
-        conn.starttls()
+        try:
+            conn.starttls()
+        except Exception:
+            # Don't leak the open plain socket on a rejected STARTTLS. SMTP has
+            # no shutdown(); close() is the low-level socket close (no QUIT). (#3174)
+            try:
+                conn.close()
+            except Exception:
+                pass
+            raise
     elif security == "ssl":
         conn = smtplib.SMTP_SSL(
             cfg["smtp_host"],
@@ -792,7 +818,16 @@ def _smtp_connect(account=None, cfg=None):
             timeout=EMAIL_SOCKET_TIMEOUT,
         )
     if cfg["smtp_user"] and cfg["smtp_password"]:
-        conn.login(cfg["smtp_user"], cfg["smtp_password"])
+        try:
+            conn.login(cfg["smtp_user"], cfg["smtp_password"])
+        except Exception:
+            # A failed login otherwise orphans the connected socket; close it
+            # before propagating (SMTP has no shutdown(); close() = socket close). (#3174)
+            try:
+                conn.close()
+            except Exception:
+                pass
+            raise
     return conn
 
 

routes/email_helpers.py

@@ -738,7 +738,16 @@ def _open_imap_connection(host: str, port: int, *, starttls: bool, timeout: int
     port = int(port or 993)
     if starttls:
         conn = imaplib.IMAP4(host, port, timeout=timeout)
-        conn.starttls()
+        try:
+            conn.starttls()
+        except Exception:
+            # Don't leak the open plain socket if the STARTTLS upgrade is
+            # rejected; close it before propagating. (#3174)
+            try:
+                conn.shutdown()
+            except Exception:
+                pass
+            raise
     elif port == 993:
         conn = imaplib.IMAP4_SSL(host, port, timeout=timeout)
     else:
@@ -771,7 +780,18 @@ def _imap_connect(account_id: str | None = None, owner: str = ""):
         starttls=bool(cfg.get("imap_starttls")),
         timeout=_IMAP_TIMEOUT_SECONDS,
     )
-    conn.login(cfg["imap_user"], cfg["imap_password"])
+    try:
+        conn.login(cfg["imap_user"], cfg["imap_password"])
+    except Exception:
+        # A failed AUTHENTICATE (e.g. an Office 365 app password on an
+        # MFA-enabled tenant, #3174) otherwise orphans the already-connected
+        # socket; close it before propagating so a misconfigured account
+        # can't leak one descriptor per retry / background poller pass.
+        try:
+            conn.shutdown()
+        except Exception:
+            pass
+        raise
     return conn
 
 

tests/test_imap_leak_fixes.py

@@ -9,6 +9,7 @@ Functions covered:
     _download_attachment
 """
 
+import imaplib
 import os
 import sys
 import tempfile
@@ -228,3 +229,176 @@ def test_mcp_download_attachment_logs_out_on_select_failure(monkeypatch):
         f"conn.logout() must be called after select raises in _download_attachment. "
         f"Got logout_calls={captured.get('logout_calls')}"
     )
+
+
+# ── connect-time leak: _imap_connect / _open_imap_connection (#3174) ──────────
+# The cases above all monkeypatch _imap_connect to *succeed*; these cover the
+# gap where the connect itself fails (bad/expired app password, rejected
+# STARTTLS) and the already-open socket would otherwise be orphaned.
+
+
+def test_imap_connect_shuts_down_socket_on_login_failure(monkeypatch):
+    """A failed login() must close the already-connected socket, not leak it."""
+    import routes.email_helpers as helpers
+
+    captured = {}
+    conn = MagicMock()
+    conn.shutdown = MagicMock(side_effect=lambda: captured.__setitem__(
+        "shutdown_calls", captured.get("shutdown_calls", 0) + 1
+    ))
+    conn.login = MagicMock(side_effect=imaplib.IMAP4.error(b"AUTHENTICATE failed."))
+
+    monkeypatch.setattr(helpers, "_get_email_config", lambda *a, **kw: {
+        "imap_host": "imap.example.com",
+        "imap_port": 993,
+        "imap_starttls": False,
+        "imap_user": "user@example.com",
+        "imap_password": "wrong",
+    })
+    monkeypatch.setattr(helpers, "_open_imap_connection", lambda *a, **kw: conn)
+
+    raised = False
+    try:
+        helpers._imap_connect()
+    except Exception:
+        raised = True
+
+    assert raised, "login failure must propagate to the caller"
+    assert captured.get("shutdown_calls", 0) == 1, (
+        f"conn.shutdown() must be called exactly once when login fails. "
+        f"Got shutdown_calls={captured.get('shutdown_calls')}"
+    )
+
+
+def test_open_imap_connection_shuts_down_on_starttls_failure(monkeypatch):
+    """A rejected STARTTLS upgrade must close the open plain socket."""
+    import routes.email_helpers as helpers
+
+    captured = {}
+    conn = MagicMock()
+    conn.shutdown = MagicMock(side_effect=lambda: captured.__setitem__(
+        "shutdown_calls", captured.get("shutdown_calls", 0) + 1
+    ))
+    conn.starttls = MagicMock(side_effect=RuntimeError("STARTTLS rejected"))
+
+    monkeypatch.setattr(helpers.imaplib, "IMAP4", lambda *a, **kw: conn)
+
+    raised = False
+    try:
+        helpers._open_imap_connection("imap.example.com", 143, starttls=True)
+    except Exception:
+        raised = True
+
+    assert raised, "starttls failure must propagate to the caller"
+    assert captured.get("shutdown_calls", 0) == 1, (
+        f"conn.shutdown() must be called exactly once when STARTTLS fails. "
+        f"Got shutdown_calls={captured.get('shutdown_calls')}"
+    )
+
+
+# ── connect-time leak: mcp_servers/email_server.py (folded in per review #3363) ──
+# Same connect-then-step pattern as the routes path. IMAP closes pre-auth with
+# shutdown(); SMTP has no shutdown(), so close() (socket close, no QUIT).
+
+
+def _cfg_imap(ssl=True, starttls=False):
+    return {
+        "imap_ssl": ssl, "imap_starttls": starttls,
+        "imap_host": "imap.example.com", "imap_port": 993,
+        "imap_user": "user@example.com", "imap_password": "wrong",
+    }
+
+
+def test_mcp_imap_connect_shuts_down_on_login_failure(monkeypatch):
+    import mcp_servers.email_server as srv
+
+    captured = {}
+    conn = MagicMock()
+    conn.shutdown = MagicMock(side_effect=lambda: captured.__setitem__(
+        "shutdown_calls", captured.get("shutdown_calls", 0) + 1))
+    conn.login = MagicMock(side_effect=imaplib.IMAP4.error(b"AUTHENTICATE failed."))
+    monkeypatch.setattr(srv, "_load_config", lambda *a, **kw: _cfg_imap(ssl=True))
+    monkeypatch.setattr(srv.imaplib, "IMAP4_SSL", lambda *a, **kw: conn)
+
+    raised = False
+    try:
+        srv._imap_connect()
+    except Exception:
+        raised = True
+    assert raised, "login failure must propagate"
+    assert captured.get("shutdown_calls", 0) == 1, (
+        f"shutdown() must be called once on MCP IMAP login failure. Got {captured.get('shutdown_calls')}")
+
+
+def test_mcp_imap_connect_shuts_down_on_starttls_failure(monkeypatch):
+    import mcp_servers.email_server as srv
+
+    captured = {}
+    conn = MagicMock()
+    conn.shutdown = MagicMock(side_effect=lambda: captured.__setitem__(
+        "shutdown_calls", captured.get("shutdown_calls", 0) + 1))
+    conn.starttls = MagicMock(side_effect=RuntimeError("STARTTLS rejected"))
+    monkeypatch.setattr(srv, "_load_config", lambda *a, **kw: _cfg_imap(ssl=False, starttls=True))
+    monkeypatch.setattr(srv.imaplib, "IMAP4", lambda *a, **kw: conn)
+
+    raised = False
+    try:
+        srv._imap_connect()
+    except Exception:
+        raised = True
+    assert raised, "starttls failure must propagate"
+    assert captured.get("shutdown_calls", 0) == 1, (
+        f"shutdown() must be called once on MCP IMAP STARTTLS failure. Got {captured.get('shutdown_calls')}")
+
+
+def _cfg_smtp(security):
+    return {
+        "smtp_host": "smtp.example.com",
+        "smtp_port": 587 if security == "starttls" else 465,
+        "smtp_security": security, "smtp_user": "user@example.com",
+        "smtp_password": "wrong", "account_name": "test",
+    }
+
+
+def test_mcp_smtp_connect_closes_on_login_failure(monkeypatch):
+    import mcp_servers.email_server as srv
+
+    captured = {}
+    conn = MagicMock()
+    conn.close = MagicMock(side_effect=lambda: captured.__setitem__(
+        "close_calls", captured.get("close_calls", 0) + 1))
+    conn.login = MagicMock(side_effect=Exception("SMTP auth failed"))
+    monkeypatch.setattr(srv, "_load_config", lambda *a, **kw: _cfg_smtp("ssl"))
+    monkeypatch.setattr(srv, "_smtp_ready", lambda cfg: True)
+    monkeypatch.setattr(srv.smtplib, "SMTP_SSL", lambda *a, **kw: conn)
+
+    raised = False
+    try:
+        srv._smtp_connect()
+    except Exception:
+        raised = True
+    assert raised, "login failure must propagate"
+    assert captured.get("close_calls", 0) == 1, (
+        f"close() must be called once on MCP SMTP login failure. Got {captured.get('close_calls')}")
+
+
+def test_mcp_smtp_connect_closes_on_starttls_failure(monkeypatch):
+    import mcp_servers.email_server as srv
+
+    captured = {}
+    conn = MagicMock()
+    conn.close = MagicMock(side_effect=lambda: captured.__setitem__(
+        "close_calls", captured.get("close_calls", 0) + 1))
+    conn.starttls = MagicMock(side_effect=Exception("STARTTLS rejected"))
+    monkeypatch.setattr(srv, "_load_config", lambda *a, **kw: _cfg_smtp("starttls"))
+    monkeypatch.setattr(srv, "_smtp_ready", lambda cfg: True)
+    monkeypatch.setattr(srv.smtplib, "SMTP", lambda *a, **kw: conn)
+
+    raised = False
+    try:
+        srv._smtp_connect()
+    except Exception:
+        raised = True
+    assert raised, "starttls failure must propagate"
+    assert captured.get("close_calls", 0) == 1, (
+        f"close() must be called once on MCP SMTP STARTTLS failure. Got {captured.get('close_calls')}")