From 932b7f2446b2bb79e0ee5a7f2f89dd07f3af9e6b Mon Sep 17 00:00:00 2001 From: nubs Date: Mon, 8 Jun 2026 19:21:41 +0000 Subject: [PATCH] fix(email): close IMAP socket when connect/login fails (#3174) (#3363) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 | 43 ++++++++- routes/email_helpers.py | 24 ++++- tests/test_imap_leak_fixes.py | 174 ++++++++++++++++++++++++++++++++++ 3 files changed, 235 insertions(+), 6 deletions(-) diff --git a/mcp_servers/email_server.py b/mcp_servers/email_server.py index 3b1ba84d1..d1c2ac07e 100644 --- a/mcp_servers/email_server.py +++ b/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 diff --git a/routes/email_helpers.py b/routes/email_helpers.py index 6364c58d4..890680a87 100644 --- a/routes/email_helpers.py +++ b/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 diff --git a/tests/test_imap_leak_fixes.py b/tests/test_imap_leak_fixes.py index a30c7c216..520a50e1e 100644 --- a/tests/test_imap_leak_fixes.py +++ b/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')}")