From 873ff0f2ccf08e9e89029c8756e7f19a1d3d3cfd Mon Sep 17 00:00:00 2001 From: joaovitoriasilva <8648976+joaovitoriasilva@users.noreply.github.com> Date: Fri, 6 Nov 2026 22:42:22 +0000 Subject: [PATCH 1/2] feat(providers): tell a caller why a mail send failed and whether it may have been delivered MailError gains kind (a new MailFailure enum naming the failed step), delivery_unknown and message_id, all keyword-only with defaults. SmtpMail now runs MAIL FROM, RCPT TO and DATA itself instead of send_message, so it knows when DATA began: only a connection lost or a deadline fired after that point sets delivery_unknown. The wire conversation is unchanged (EHLO when needed, SIZE when offered, partial refusals still succeed with a warning). Closes #52 Closes #53 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 8 + docs/api-stability.md | 7 +- docs/durable-jobs.md | 5 +- docs/providers-and-backends.md | 27 ++++ jasil/__init__.py | 2 + jasil/_reloading.py | 5 +- jasil/backends/mail_memory.py | 4 +- jasil/backends/mail_smtp.py | 174 ++++++++++++++++----- jasil/providers.py | 63 +++++++- tests/test_mail.py | 275 +++++++++++++++++++++++++++++---- tests/test_reloading.py | 6 +- 11 files changed, 495 insertions(+), 81 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21ee188..60bf3fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ What counts as a breaking change — and what does not — is defined in [API st ## [Unreleased] +### Added + +- **`MailError` says why a send failed and whether it may have been delivered.** `kind` is a new `jasil.providers.MailFailure` naming the failed step (`auth_failed`, `sender_refused`, `recipient_refused`, `message_rejected`, `unreachable`, `connection_lost`, `deadline`, ...). `delivery_unknown` is `True` only when the connection was lost, or the deadline fired, after the message body started going out. `message_id` is set on every failure after the message was built. All three are keyword-only with defaults, so existing code is unaffected. See [MailProvider](docs/providers-and-backends.md#mailprovider). ([#52](https://github.com/endurain-project/jasil/issues/52), [#53](https://github.com/endurain-project/jasil/issues/53)) + +### Changed + +- `SmtpMail` sends `MAIL FROM`, `RCPT TO` and `DATA` itself instead of through `smtplib`'s `send_message`, so it knows which step failed. The conversation on the wire is unchanged, including the `SIZE` declaration and success with a warning when only some recipients are refused. A refused `MAIL FROM` is now reported as a sender refusal, no longer as a rejected message. + ## [0.17.0] - 2026-11-04 This release fixes the first audit write on a fresh SQLite database failing with `database is locked`. Migration `rev0010` creates the `jasil_instance` row, and `ensure_instance_id` is now public. diff --git a/docs/api-stability.md b/docs/api-stability.md index 7c54c8b..6226e61 100644 --- a/docs/api-stability.md +++ b/docs/api-stability.md @@ -45,8 +45,11 @@ modules: ### Provider protocols -`MailProvider.send` returns the `Message-ID` as a `str`, and `MailError` / -`MailNotConfiguredError` are covered. Adding a field to `MailMessage` with a +`MailProvider.send` returns the `Message-ID` as a `str`, and `MailError` (with +`status`, `permanent`, `kind`, `delivery_unknown` and `message_id`) / +`MailNotConfiguredError` are covered. `MailFailure` is covered like +`MailProblemCode`: members may be added in a minor release, and a member is never +renamed or reused. Which `kind` a given failure maps to is documented, not frozen. Adding a field to `MailMessage` with a default is additive. `RealtimeProvider`'s four methods and its contract are covered: best-effort, diff --git a/docs/durable-jobs.md b/docs/durable-jobs.md index e44875f..4136d81 100644 --- a/docs/durable-jobs.md +++ b/docs/durable-jobs.md @@ -559,7 +559,10 @@ failure. `MailError`, so a job on a deployment with no `mail.uri` uses its whole retry budget before dead-lettering. Catch it in the handler if that is not wanted. - **Delivery is at-least-once.** A retry after a lost acknowledgement sends the - message twice. + message twice. A `MailError` with `delivery_unknown` set lost its connection + after the body went out, so the message may already have been delivered; a + handler that must not send twice catches it, records `message_id`, and does + not retry. - **Size `jobs.max_runtime_seconds` for the whole handler, not one send.** One SMTP send can take up to 90 seconds, and the DNS lookups before it are not counted against that deadline. A handler that sends N messages can take N times diff --git a/docs/providers-and-backends.md b/docs/providers-and-backends.md index 0ce97b8..c9debd4 100644 --- a/docs/providers-and-backends.md +++ b/docs/providers-and-backends.md @@ -532,6 +532,33 @@ CR/LF in a header value). `MemoryMail` validates through the same builder as `MailError` messages carry a failure category and SMTP status code only — never an address, subject, body or server text. +A `MailError` also tells a host what to do next without parsing its message: + +| Attribute | Meaning | +|---|---| +| `status` / `permanent` | The SMTP status code, and whether it is a 5xx. | +| `kind` | A `MailFailure` naming the step that failed (table below). | +| `delivery_unknown` | `True` when the connection was lost, or the send deadline fired, after the body started going out. The server may have accepted the message, so a retry may deliver it twice; reconcile with the relay's logs instead. `False` means the message was not accepted. | +| `message_id` | The `Message-ID` of the message that failed, set on every failure after the message was built. Look it up in the relay's or provider's logs. It is never part of `str(error)`. | + +| `MailFailure` | Raised when | Typical reaction | +|---|---|---| +| `auth_failed` | The server refused the credentials. | Fix the configuration; alert an operator. | +| `unsupported` | The server lacks STARTTLS, AUTH, or an AUTH mechanism `smtplib` implements. | Fix the configuration. | +| `sender_refused` | `MAIL FROM` was refused. | When `permanent`, stop using that sender; a 4xx is usually rate limiting. | +| `recipient_refused` | Every recipient was refused, or the server closed the session (421) during `RCPT`. | When `permanent`, record it against the recipient. | +| `message_rejected` | The server refused the message at `DATA` or at its end (content, size, policy). | When `permanent`, do not retry the same content. | +| `unreachable` | DNS, a refused or failed connect, a greeting other than 220, or a cleartext relay that no longer resolves to a private address. | Retry later. | +| `tls_failed` | The TLS handshake failed. | Check the relay's certificate and name. | +| `connection_lost` | The connection broke after the session was established. | Retry, unless `delivery_unknown`. | +| `deadline` | The send ran past its overall deadline. | Retry, unless `delivery_unknown`. | +| `lease_lost` | The durable job lost its lease; nothing was sent. | None: another worker owns the job. | +| `settings_unavailable` | Runtime mail settings could not be loaded or verified. | Retry later. | +| `not_configured` | No backend is configured (`MailNotConfiguredError`). | Configure `mail.uri`. | +| `other` | Anything else, and a `MailError` raised without a kind. | Treat by `status`. | + +Members may be added in a minor release; a member is never renamed or reused. + The sender is `from_address`, independent of the SMTP `username`: relays such as Brevo log you in as a generated identity and reject a `From` that is not a verified sender. The `From` header keeps any display name; the envelope sender is diff --git a/jasil/__init__.py b/jasil/__init__.py index e3ccdec..daaf25e 100644 --- a/jasil/__init__.py +++ b/jasil/__init__.py @@ -28,6 +28,7 @@ EventBusProvider, LockProvider, MailError, + MailFailure, MailMessage, MailNotConfiguredError, MailProvider, @@ -70,6 +71,7 @@ "JobWorkerMode", "LockProvider", "MailError", + "MailFailure", "MailMessage", "MailNotConfiguredError", "MailProvider", diff --git a/jasil/_reloading.py b/jasil/_reloading.py index d6c6e84..b993567 100644 --- a/jasil/_reloading.py +++ b/jasil/_reloading.py @@ -21,6 +21,7 @@ GeocodedPlace, GeocodingProvider, MailError, + MailFailure, MailMessage, MailNotConfiguredError, MailProvider, @@ -181,9 +182,9 @@ def send(self, message: MailMessage) -> str: outcome = self._backend.resolve() except SourceUnavailableError: # Not chained: the source's exception is the host's, and its text may carry a secret. - raise MailError("Mail settings could not be loaded") from None + raise MailError("Mail settings could not be loaded", kind=MailFailure.SETTINGS_UNAVAILABLE) from None if outcome.transient: - raise MailError("Mail settings could not be verified; retry") + raise MailError("Mail settings could not be verified; retry", kind=MailFailure.SETTINGS_UNAVAILABLE) if outcome.backend is None: # The reason carries no setting's value (see ``ReloadingBackend``). raise MailNotConfiguredError(f"Mail settings are invalid: {outcome.rejection}") diff --git a/jasil/backends/mail_memory.py b/jasil/backends/mail_memory.py index 4c26b6f..14e40f5 100644 --- a/jasil/backends/mail_memory.py +++ b/jasil/backends/mail_memory.py @@ -12,7 +12,7 @@ from jasil._core.job_context import lease_lost from jasil._core.mail_message import build_mime -from jasil.providers import ClockProvider, MailError, MailMessage, MailNotConfiguredError +from jasil.providers import ClockProvider, MailError, MailFailure, MailMessage, MailNotConfiguredError # Used only when ``memory://`` is configured without a ``from_address``. _DEFAULT_SENDER = "jasil@localhost" @@ -67,7 +67,7 @@ def send(self, message: MailMessage) -> str: :class:`~jasil.backends.mail_smtp.SmtpMail` would send nothing. """ if lease_lost(): - raise MailError("Job lease lost; send abandoned") + raise MailError("Job lease lost; send abandoned", kind=MailFailure.LEASE_LOST) mime = build_mime(message, sender=self._from_address, now=self._clock.now()) # Recipients copied as sent: the caller's list may be reused or mutated # after ``send`` returns, which would rewrite what the test asserts on. A diff --git a/jasil/backends/mail_smtp.py b/jasil/backends/mail_smtp.py index 5e34c9d..1f56a9b 100644 --- a/jasil/backends/mail_smtp.py +++ b/jasil/backends/mail_smtp.py @@ -23,11 +23,12 @@ import threading import time from email.message import EmailMessage +from typing import Literal import jasil._core.network as network from jasil._core.job_context import lease_lost from jasil._core.mail_message import build_mime, parse_address -from jasil.providers import ClockProvider, MailError, MailMessage +from jasil.providers import ClockProvider, MailError, MailFailure, MailMessage logger = logging.getLogger(__name__) @@ -48,6 +49,61 @@ _WATCH_INTERVAL_SECONDS = 0.1 _DEADLINE_EXCEEDED = "SMTP send exceeded its overall deadline" +# How far a send got: ``connecting`` until the session is up (greeting, and TLS +# when it is used), ``session`` from then on, ``data`` once ``DATA`` was sent. A +# connection lost in ``data`` may have delivered the message. +_Stage = Literal["connecting", "session", "data"] + + +def _failure(error: OSError, stage: _Stage) -> MailError: + """Map an ``smtplib`` or socket failure to a :class:`MailError` that carries no server text. + + Only a lost connection is ambiguous: a reply, even to ``DATA``, says the + server did not accept the message. + """ + if isinstance(error, smtplib.SMTPAuthenticationError): + return MailError( + f"SMTP authentication failed (status {error.smtp_code})", + status=error.smtp_code, + kind=MailFailure.AUTH_FAILED, + ) + if isinstance(error, smtplib.SMTPRecipientsRefused): + # Codes only: 4xx (greylisting, retry) and 5xx (unknown user) differ. + codes = sorted({code for code, _ in error.recipients.values()}) + # ``min``: one 4xx among 5xx is worth a retry, so it is not permanent. + return MailError( + "SMTP server refused the recipients (status " + ", ".join(map(str, codes)) + ")", + status=codes[0] if codes else None, + kind=MailFailure.RECIPIENT_REFUSED, + ) + if isinstance(error, smtplib.SMTPSenderRefused): + return MailError( + f"SMTP server refused the sender (status {error.smtp_code})", + status=error.smtp_code, + kind=MailFailure.SENDER_REFUSED, + ) + if isinstance(error, smtplib.SMTPConnectError): + return MailError( + f"SMTP server refused the connection (status {error.smtp_code})", + status=error.smtp_code, + kind=MailFailure.UNREACHABLE, + ) + if isinstance(error, smtplib.SMTPDataError): + return MailError( + f"SMTP server rejected the message (status {error.smtp_code})", + status=error.smtp_code, + kind=MailFailure.MESSAGE_REJECTED, + ) + if isinstance(error, smtplib.SMTPResponseException): + return MailError(f"SMTP server refused a command (status {error.smtp_code})", status=error.smtp_code) + if isinstance(error, smtplib.SMTPNotSupportedError): + return MailError("SMTP server lacks a required feature", kind=MailFailure.UNSUPPORTED) + if stage == "connecting": + if isinstance(error, ssl.SSLError): + return MailError("TLS negotiation with the SMTP server failed", kind=MailFailure.TLS_FAILED) + return MailError("SMTP server unreachable", kind=MailFailure.UNREACHABLE) + return MailError("SMTP connection lost", kind=MailFailure.CONNECTION_LOST, delivery_unknown=stage == "data") + def _default_local_hostname() -> str: """The EHLO name when none is configured: this host's name, if usable. @@ -184,43 +240,24 @@ def send(self, message: MailMessage) -> str: ValueError: When the message is invalid. MailError: When the server could not be reached or refused the message, or when the durable job sending it has lost its lease - (nothing is sent: another worker owns the job now). If only - *some* recipients are refused the send succeeds and a warning - records the count, since failing would make a retry re-send to - recipients that already got it. + (nothing is sent: another worker owns the job now). Its ``kind`` + names the step that failed, ``delivery_unknown`` is set when the + connection broke after the body started going out, and + ``message_id`` is set on every failure after the message was + built. If only *some* recipients are refused the send succeeds + and a warning records the count, since failing would make a + retry re-send to recipients that already got it. """ if lease_lost(): - raise MailError("Job lease lost; send abandoned") + raise MailError("Job lease lost; send abandoned", kind=MailFailure.LEASE_LOST) mime = build_mime(message, sender=self._from_address, now=self._clock.now()) message_id = str(mime["Message-ID"]) try: refused = self._deliver(mime) - except MailError: + except MailError as error: + # Every failure of a built message passes here, whichever step raised it. + error.message_id = message_id raise - except smtplib.SMTPAuthenticationError as error: - raise MailError(f"SMTP authentication failed (status {error.smtp_code})", status=error.smtp_code) from None - except smtplib.SMTPRecipientsRefused as error: - # Codes only: 4xx (greylisting, retry) and 5xx (unknown user) differ. - codes = sorted({code for code, _ in error.recipients.values()}) - # ``min``: one 4xx among 5xx is worth a retry, so it is not permanent. - raise MailError( - "SMTP server refused every recipient (status " + ", ".join(map(str, codes)) + ")", - status=codes[0] if codes else None, - ) from None - except smtplib.SMTPConnectError as error: - raise MailError( - f"SMTP server refused the connection (status {error.smtp_code})", status=error.smtp_code - ) from None - except smtplib.SMTPResponseException as error: - raise MailError( - f"SMTP server rejected the message (status {error.smtp_code})", status=error.smtp_code - ) from None - except smtplib.SMTPNotSupportedError: - raise MailError("SMTP server lacks a required feature (AUTH or SMTPUTF8)") from None - except ssl.SSLError: - raise MailError("TLS negotiation with the SMTP server failed") from None - except (smtplib.SMTPException, OSError): - raise MailError("SMTP server unreachable or connection lost") from None if refused: logger.warning( "Mail %s accepted with %d of %d recipients refused", @@ -236,7 +273,7 @@ def _arm(connection: smtplib.SMTP, deadline: float) -> None: """Cap the next step to the time left, or abort when none is.""" remaining = deadline - time.monotonic() if remaining <= 0: - raise MailError(_DEADLINE_EXCEEDED) + raise MailError(_DEADLINE_EXCEEDED, kind=MailFailure.DEADLINE) sock = getattr(connection, "sock", None) if sock is not None: sock.settimeout(min(_TIMEOUT_SECONDS, remaining)) @@ -271,7 +308,9 @@ def _connect_hosts(self) -> list[str]: return [self._host] addresses, reason = network.resolve_private_addresses(self._host) if reason is not None or not addresses: - raise MailError("SMTP relay no longer resolves to a private address; cleartext refused") + raise MailError( + "SMTP relay no longer resolves to a private address; cleartext refused", kind=MailFailure.UNREACHABLE + ) return addresses def _connect(self, connection: smtplib.SMTP, deadline: float) -> tuple[int, bytes]: @@ -281,7 +320,7 @@ def _connect(self, connection: smtplib.SMTP, deadline: float) -> tuple[int, byte # A connect is not interruptible by the watchdog, so do not start one # after the deadline. if time.monotonic() >= deadline: - raise MailError(_DEADLINE_EXCEEDED) + raise MailError(_DEADLINE_EXCEEDED, kind=MailFailure.DEADLINE) # ``connection.timeout`` bounds each address tried; cap it to the time # left, and the dial itself stops at the deadline between addresses, # so a connect begun near the deadline cannot run past it. @@ -305,7 +344,11 @@ def _connect(self, connection: smtplib.SMTP, deadline: float) -> tuple[int, byte raise AssertionError("unreachable") # pragma: no cover def _deliver(self, mime: EmailMessage) -> dict[str, tuple[int, bytes]]: - """Connect, secure, authenticate and send; returns refused recipients.""" + """Connect, secure, authenticate and send; returns refused recipients. + + Raises: + MailError: On any failure, classified by the stage it interrupted. + """ context = self._context deadline = time.monotonic() + SEND_DEADLINE_SECONDS # Built without a host so nothing is dialed before the watchdog is armed: @@ -322,6 +365,7 @@ def _deliver(self, mime: EmailMessage) -> dict[str, tuple[int, bytes]]: finished = threading.Event() watchdog = threading.Thread(target=self._watch, args=(connection, expired, finished), daemon=True) watchdog.start() + stage: _Stage = "connecting" try: try: code, reply = self._connect(connection, deadline) @@ -331,22 +375,31 @@ def _deliver(self, mime: EmailMessage) -> dict[str, tuple[int, bytes]]: self._arm(connection, deadline) connection.ehlo() if not connection.has_extn("starttls"): - raise MailError("SMTP server does not offer STARTTLS") + raise MailError("SMTP server does not offer STARTTLS", kind=MailFailure.UNSUPPORTED) self._arm(connection, deadline) connection.starttls(context=context) connection.ehlo() + stage = "session" if self._username and self._password: self._arm(connection, deadline) self._login(connection, self._username, self._password) self._arm(connection, deadline) - # Explicit envelope sender: the bare address, never the display name. - return connection.send_message(mime, from_addr=mime["From"].addresses[0].addr_spec) - except (smtplib.SMTPException, OSError): + body = mime.as_bytes() + refused = self._envelope(connection, mime, len(body)) + self._arm(connection, deadline) + stage = "data" + code, reply = connection.data(body) + if code != 250: + raise smtplib.SMTPDataError(code, reply) + return refused + except (smtplib.SMTPException, OSError) as error: # The watchdog's shutdown surfaces as a dropped connection, and the # last step's socket timeout is the time left, so it can fire first. if expired.is_set() or time.monotonic() >= deadline: - raise MailError(_DEADLINE_EXCEEDED) from None - raise + raise MailError( + _DEADLINE_EXCEEDED, kind=MailFailure.DEADLINE, delivery_unknown=stage == "data" + ) from None + raise _failure(error, stage) from None finally: # Not ``with``: its exit raises on a non-221 reply to QUIT, which would # report a message the server already accepted as a failure (a retry @@ -362,6 +415,41 @@ def _deliver(self, mime: EmailMessage) -> dict[str, tuple[int, bytes]]: if watched is not None: watched.close() + @staticmethod + def _envelope(connection: smtplib.SMTP, mime: EmailMessage, size: int) -> dict[str, tuple[int, bytes]]: + """Send ``MAIL FROM`` and one ``RCPT TO`` per recipient, as ``smtplib.SMTP.sendmail`` does. + + Run step by step rather than through ``sendmail`` so the caller knows when + ``DATA`` begins. ``build_mime`` emits 7-bit content and ASCII addresses, so + neither ``SMTPUTF8`` nor ``8BITMIME`` is needed. + + Returns: + The refused recipients, when some but not all were refused. + + Raises: + smtplib.SMTPSenderRefused: When ``MAIL FROM`` is refused. + smtplib.SMTPRecipientsRefused: When every recipient is refused, or the + server closes the session (421) while recipients are offered. + """ + connection.ehlo_or_helo_if_needed() + options = [f"size={size}"] if connection.does_esmtp and connection.has_extn("size") else [] + # The envelope sender is the bare address, never the display name. + sender = mime["From"].addresses[0].addr_spec + code, reply = connection.mail(sender, options) + if code != 250: + raise smtplib.SMTPSenderRefused(code, reply, sender) + refused: dict[str, tuple[int, bytes]] = {} + recipients = [address.addr_spec for address in mime["To"].addresses] + for recipient in recipients: + code, reply = connection.rcpt(recipient) + if code not in (250, 251): + refused[recipient] = (code, reply) + if code == 421: + raise smtplib.SMTPRecipientsRefused(refused) + if len(refused) == len(recipients): + raise smtplib.SMTPRecipientsRefused(refused) + return refused + @staticmethod def _login(connection: smtplib.SMTP, username: str, password: str) -> None: """Authenticate, naming the one failure ``smtplib`` raises untyped.""" @@ -371,5 +459,7 @@ def _login(connection: smtplib.SMTP, username: str, password: str) -> None: # A bare ``SMTPException`` from ``login`` means the server advertises # AUTH but no mechanism ``smtplib`` implements. if type(error) is smtplib.SMTPException: - raise MailError("SMTP server offers no supported authentication mechanism") from None + raise MailError( + "SMTP server offers no supported authentication mechanism", kind=MailFailure.UNSUPPORTED + ) from None raise diff --git a/jasil/providers.py b/jasil/providers.py index 1b5c513..0945bc8 100644 --- a/jasil/providers.py +++ b/jasil/providers.py @@ -11,6 +11,7 @@ from contextlib import AbstractContextManager from dataclasses import dataclass from datetime import datetime +from enum import StrEnum from pathlib import Path from typing import BinaryIO, Protocol, runtime_checkable @@ -971,6 +972,42 @@ def __hash__(self) -> int: return hash((recipients, self.subject, self.text, self.html, self.reply_to)) +class MailFailure(StrEnum): + """Which step of a send failed, so a host can react without parsing the message. + + Members may be added in a minor release; a member is never renamed or reused. + """ + + #: Not classified: the default for a :class:`MailError` raised without a kind. + OTHER = "other" + #: The server refused the credentials. + AUTH_FAILED = "auth_failed" + #: The server lacks something the send requires: STARTTLS, AUTH, or an AUTH mechanism ``smtplib`` implements. + UNSUPPORTED = "unsupported" + #: ``MAIL FROM`` was refused. Only a permanent (5xx) refusal means the sender itself is unusable; a 4xx is + #: usually rate limiting. + SENDER_REFUSED = "sender_refused" + #: Every recipient was refused, or the server closed the session (421) while recipients were offered. + RECIPIENT_REFUSED = "recipient_refused" + #: The server refused the message at ``DATA`` or at its end (content, size or policy). + MESSAGE_REJECTED = "message_rejected" + #: No session could be opened: DNS, a refused or failed connect, a greeting other than 220, or a cleartext relay + #: that no longer resolves to a private address. + UNREACHABLE = "unreachable" + #: The TLS handshake failed (certificate, hostname or protocol). + TLS_FAILED = "tls_failed" + #: The connection broke after the session was established. + CONNECTION_LOST = "connection_lost" + #: The send ran past its overall deadline. + DEADLINE = "deadline" + #: The durable job sending the message lost its lease; nothing was sent. + LEASE_LOST = "lease_lost" + #: Runtime mail settings could not be loaded or verified; retry later. + SETTINGS_UNAVAILABLE = "settings_unavailable" + #: No mail backend is configured (:class:`MailNotConfiguredError`). + NOT_CONFIGURED = "not_configured" + + class MailError(RuntimeError): """A message could not be handed to the mail server. @@ -981,11 +1018,30 @@ class MailError(RuntimeError): Attributes: status: The SMTP status code, or ``None`` when the failure had none (unreachable server, TLS failure, deadline). + kind: The step that failed. + delivery_unknown: ``True`` when the connection was lost, or the deadline + fired, after the message body started going out: the server may have + accepted it, so a retry may deliver it twice. ``False`` means the + message was not accepted. + message_id: The ``Message-ID`` of the message that failed, angle brackets + included, when the message was built before the failure; the key to + look it up in the relay's logs. Never part of ``str(error)``. """ - def __init__(self, message: str, *, status: int | None = None) -> None: + def __init__( + self, + message: str, + *, + status: int | None = None, + kind: MailFailure = MailFailure.OTHER, + delivery_unknown: bool = False, + message_id: str | None = None, + ) -> None: super().__init__(message) self.status = status + self.kind = kind + self.delivery_unknown = delivery_unknown + self.message_id = message_id @property def permanent(self) -> bool: @@ -994,7 +1050,10 @@ def permanent(self) -> bool: class MailNotConfiguredError(MailError): - """Mail was sent while no backend is configured.""" + """Mail was sent while no backend is configured; its ``kind`` is :attr:`MailFailure.NOT_CONFIGURED`.""" + + def __init__(self, message: str, *, status: int | None = None) -> None: + super().__init__(message, status=status, kind=MailFailure.NOT_CONFIGURED) @runtime_checkable diff --git a/tests/test_mail.py b/tests/test_mail.py index 685cbe4..1cda02c 100644 --- a/tests/test_mail.py +++ b/tests/test_mail.py @@ -8,11 +8,15 @@ """ import contextlib +import email +import email.policy +import functools import logging import os import smtplib import socket import ssl +import sys import threading from datetime import UTC, datetime from types import SimpleNamespace @@ -25,7 +29,7 @@ from jasil._core.mail_message import build_mime from jasil.backends.mail_memory import MemoryMail, NullMail from jasil.backends.mail_smtp import SmtpMail -from jasil.providers import MailError, MailMessage, MailNotConfiguredError, MailProvider +from jasil.providers import MailError, MailFailure, MailMessage, MailNotConfiguredError, MailProvider # Windows: shutdown() does not wake a thread blocked in recv(), so the watchdog cannot interrupt a read there; only the per-read timeout bounds one. CANNOT_INTERRUPT_A_READ = pytest.mark.skipif( @@ -57,6 +61,8 @@ class FakeSMTP: instances: ClassVar[list["FakeSMTP"]] = [] offers_starttls = True + offers_size = False + does_esmtp = True refused: ClassVar[dict] = {} fail_on: str | None = None quit_code = 221 @@ -69,7 +75,9 @@ def __init__(self, host="", port=0, local_hostname=None, timeout=None, context=N self.calls: list[str] = [] self.logged_in_as: str | None = None self.sent = None + self.body: bytes | None = None self.envelope_sender: str | None = None + self.mail_options: list[str] = [] self.tls_started = False FakeSMTP.instances.append(self) @@ -93,11 +101,19 @@ def close(self): def ehlo(self): self.calls.append("ehlo") + def ehlo_or_helo_if_needed(self): + if "ehlo" not in self.calls: + self.calls.append("ehlo") + def has_extn(self, name): - return FakeSMTP.offers_starttls + return {"starttls": FakeSMTP.offers_starttls, "size": FakeSMTP.offers_size}.get(name, False) def starttls(self, context=None): self.calls.append("starttls") + if FakeSMTP.fail_on == "tls": + raise ssl.SSLError("handshake") + if FakeSMTP.fail_on == "starttls-refused": + raise smtplib.SMTPResponseException(454, b"srvtext-xyz") self.tls_started = True def login(self, username, password): @@ -107,26 +123,45 @@ def login(self, username, password): raise smtplib.SMTPAuthenticationError(535, ("bad creds for " + ADDRESS).encode()) if FakeSMTP.fail_on == "no-mechanism": raise smtplib.SMTPException("No suitable authentication method found.") - - def send_message(self, mime, from_addr=None): - self.calls.append("send") - self.sent = mime - self.envelope_sender = from_addr - if FakeSMTP.fail_on == "data": - raise smtplib.SMTPDataError(554, ("srvtext-xyz " + ADDRESS).encode()) - if FakeSMTP.fail_on == "all-refused": - raise smtplib.SMTPRecipientsRefused({ADDRESS: (550, b"no")}) - if FakeSMTP.fail_on == "tls": - raise ssl.SSLError("handshake") if FakeSMTP.fail_on == "unsupported": raise smtplib.SMTPNotSupportedError("srvtext-xyz " + ADDRESS) - return FakeSMTP.refused + + def mail(self, sender, options=()): + self.calls.append("mail") + self.envelope_sender = sender + self.mail_options = list(options) + if FakeSMTP.fail_on == "sender": + return 550, ("srvtext-xyz " + ADDRESS).encode() + return 250, b"ok" + + def rcpt(self, recipient): + self.calls.append("rcpt") + if FakeSMTP.fail_on == "drop-at-rcpt": + raise smtplib.SMTPServerDisconnected("srvtext-xyz " + ADDRESS) + if FakeSMTP.fail_on == "all-refused": + return 550, ("srvtext-xyz " + ADDRESS).encode() + if FakeSMTP.fail_on == "rcpt-closing": + return 421, b"srvtext-xyz closing" + return FakeSMTP.refused.get(recipient, (250, b"ok")) + + def data(self, body): + self.calls.append("data") + self.body = body + self.sent = email.message_from_bytes(body, policy=email.policy.default) + if FakeSMTP.fail_on == "data": + return 554, ("srvtext-xyz " + ADDRESS).encode() + if FakeSMTP.fail_on == "drop-in-data": + raise smtplib.SMTPServerDisconnected("srvtext-xyz " + ADDRESS) + if FakeSMTP.fail_on == "tls-in-data": + raise ssl.SSLEOFError("srvtext-xyz") + return 250, b"queued" @pytest.fixture(autouse=True) def fake_smtp(monkeypatch): FakeSMTP.instances = [] FakeSMTP.offers_starttls = True + FakeSMTP.offers_size = False FakeSMTP.refused = {} FakeSMTP.fail_on = None FakeSMTP.quit_code = 221 @@ -405,7 +440,7 @@ def test_starttls_is_required_before_anything_is_sent(self): _smtp().send(_message()) calls = FakeSMTP.instances[0].calls - assert calls.index("starttls") < calls.index("send") + assert calls.index("starttls") < calls.index("mail") def test_a_server_without_starttls_is_refused_not_downgraded(self): FakeSMTP.offers_starttls = False @@ -413,7 +448,7 @@ def test_a_server_without_starttls_is_refused_not_downgraded(self): with pytest.raises(MailError, match="STARTTLS"): _smtp().send(_message()) - assert "send" not in FakeSMTP.instances[0].calls + assert "mail" not in FakeSMTP.instances[0].calls def test_implicit_tls_skips_starttls(self): _smtp(implicit_tls=True).send(_message()) @@ -441,7 +476,7 @@ def test_login_happens_after_tls(self): _smtp(username="relay-user", password="pw").send(_message()) calls = FakeSMTP.instances[0].calls - assert calls.index("starttls") < calls.index("login") < calls.index("send") + assert calls.index("starttls") < calls.index("login") < calls.index("mail") def test_login_and_sender_are_independent(self): """Relays log you in as a generated identity and verify the From separately.""" @@ -633,10 +668,12 @@ def test_a_send_past_its_deadline_is_aborted(self, monkeypatch): ticks = iter([0.0, 0.0, 1000.0, 1000.0, 1000.0]) monkeypatch.setattr(mail_smtp.time, "monotonic", lambda: next(ticks)) - with pytest.raises(MailError, match="deadline"): + with pytest.raises(MailError, match="deadline") as caught: _smtp().send(_message()) - assert "send" not in FakeSMTP.instances[0].calls + assert "mail" not in FakeSMTP.instances[0].calls + assert caught.value.kind is MailFailure.DEADLINE + assert not caught.value.delivery_unknown def test_a_partial_refusal_succeeds_and_warns_with_a_count(self, caplog): FakeSMTP.refused = {"gone@example.com": (550, b"unknown")} @@ -857,30 +894,195 @@ def test_it_stops_once_the_send_ends(self): class TestFailuresAreRedacted: @pytest.mark.parametrize( - ("mode", "expect"), + ("mode", "expect", "kind", "delivery_unknown"), [ - ("connect", "unreachable"), - ("greeting", "refused the connection (status 421)"), - ("auth", "authentication failed (status 535)"), - ("no-mechanism", "no supported authentication mechanism"), - ("data", "status 554"), - ("all-refused", "refused every recipient (status 550)"), - ("tls", "TLS negotiation"), - ("unsupported", "lacks a required feature"), + ("connect", "unreachable", MailFailure.UNREACHABLE, False), + ("greeting", "refused the connection (status 421)", MailFailure.UNREACHABLE, False), + ("tls", "TLS negotiation", MailFailure.TLS_FAILED, False), + ("starttls-refused", "refused a command (status 454)", MailFailure.OTHER, False), + ("auth", "authentication failed (status 535)", MailFailure.AUTH_FAILED, False), + ("no-mechanism", "no supported authentication mechanism", MailFailure.UNSUPPORTED, False), + ("unsupported", "lacks a required feature", MailFailure.UNSUPPORTED, False), + ("sender", "refused the sender (status 550)", MailFailure.SENDER_REFUSED, False), + ("all-refused", "refused the recipients (status 550)", MailFailure.RECIPIENT_REFUSED, False), + ("rcpt-closing", "refused the recipients (status 421)", MailFailure.RECIPIENT_REFUSED, False), + ("drop-at-rcpt", "connection lost", MailFailure.CONNECTION_LOST, False), + ("data", "rejected the message (status 554)", MailFailure.MESSAGE_REJECTED, False), + ("drop-in-data", "connection lost", MailFailure.CONNECTION_LOST, True), + ("tls-in-data", "connection lost", MailFailure.CONNECTION_LOST, True), ], ) - def test_each_failure_becomes_a_mail_error_with_no_server_text(self, mode, expect, caplog): + def test_each_failure_becomes_a_mail_error_with_no_server_text(self, mode, expect, kind, delivery_unknown, caplog): FakeSMTP.fail_on = mode with caplog.at_level(logging.DEBUG), pytest.raises(MailError) as caught: _smtp(username="u", password="pw").send(_message(subject="Top secret subject", text="secret body")) assert expect in str(caught.value) + assert caught.value.kind is kind + assert caught.value.delivery_unknown is delivery_unknown assert caught.value.__cause__ is None and caught.value.__suppress_context__ for leaked in (ADDRESS, "Top secret subject", "secret body", "bad creds", "srvtext-xyz"): assert leaked not in str(caught.value) assert leaked not in caplog.text + def test_every_failure_after_the_message_was_built_carries_its_message_id(self): + FakeSMTP.fail_on = "drop-in-data" + + with pytest.raises(MailError) as caught: + _smtp().send(_message()) + + message_id = caught.value.message_id + assert message_id == FakeSMTP.instances[0].sent["Message-ID"] + assert message_id not in str(caught.value) + + def test_a_server_without_starttls_is_unsupported(self): + FakeSMTP.offers_starttls = False + + with pytest.raises(MailError) as caught: + _smtp().send(_message()) + + assert caught.value.kind is MailFailure.UNSUPPORTED + assert caught.value.message_id is not None + + def test_a_cleartext_relay_that_stops_resolving_privately_is_unreachable(self, monkeypatch): + monkeypatch.setattr(mail_smtp.network, "resolve_private_addresses", lambda host: ([], "public")) + + with pytest.raises(MailError) as caught: + _smtp(tls=False).send(_message()) + + assert caught.value.kind is MailFailure.UNREACHABLE + + @pytest.mark.parametrize("mail", [_memory, _smtp]) + def test_a_lost_lease_is_its_own_kind_and_nothing_was_built(self, monkeypatch, mail): + provider = mail() + monkeypatch.setattr(sys.modules[type(provider).__module__], "lease_lost", lambda: True) + + with pytest.raises(MailError) as caught: + provider.send(_message()) + + assert caught.value.kind is MailFailure.LEASE_LOST + assert caught.value.message_id is None + assert not caught.value.delivery_unknown + + +class TestTheEnvelope: + """``SmtpMail`` runs MAIL, RCPT and DATA itself, so it knows which step a failure interrupted.""" + + def test_ehlo_comes_before_mail_even_without_starttls_or_login(self): + _smtp(implicit_tls=True).send(_message()) + + calls = FakeSMTP.instances[0].calls + assert calls.index("ehlo") < calls.index("mail") < calls.index("rcpt") < calls.index("data") + + def test_the_size_is_declared_when_the_server_offers_it(self): + FakeSMTP.offers_size = True + + _smtp().send(_message()) + + smtp = FakeSMTP.instances[0] + assert smtp.mail_options == [f"size={len(smtp.body)}"] + + def test_no_size_is_declared_otherwise(self): + _smtp().send(_message()) + + assert FakeSMTP.instances[0].mail_options == [] + + def test_a_refused_sender_stops_before_any_recipient(self): + FakeSMTP.fail_on = "sender" + + with pytest.raises(MailError): + _smtp().send(_message()) + + assert "rcpt" not in FakeSMTP.instances[0].calls + + def test_a_server_closing_during_rcpt_stops_before_data(self): + FakeSMTP.fail_on = "rcpt-closing" + + with pytest.raises(MailError) as caught: + _smtp().send(_message(to=[ADDRESS, "other@example.com"])) + + assert FakeSMTP.instances[0].calls.count("rcpt") == 1 + assert "data" not in FakeSMTP.instances[0].calls + assert not caught.value.permanent + + def test_every_recipient_is_offered(self): + _smtp().send(_message(to=[ADDRESS, "Other "])) + + assert FakeSMTP.instances[0].calls.count("rcpt") == 2 + + def test_the_watchdog_firing_during_data_may_have_delivered(self, monkeypatch): + monkeypatch.setattr(SmtpMail, "_watch", staticmethod(lambda connection, expired, finished: expired.set())) + FakeSMTP.fail_on = "drop-in-data" + + with pytest.raises(MailError) as caught: + _smtp().send(_message()) + + assert caught.value.kind is MailFailure.DEADLINE + assert caught.value.delivery_unknown + + def test_the_watchdog_firing_before_data_did_not_deliver(self, monkeypatch): + monkeypatch.setattr(SmtpMail, "_watch", staticmethod(lambda connection, expired, finished: expired.set())) + FakeSMTP.fail_on = "drop-at-rcpt" + + with pytest.raises(MailError) as caught: + _smtp().send(_message()) + + assert caught.value.kind is MailFailure.DEADLINE + assert not caught.value.delivery_unknown + + def test_real_smtplib_declares_the_size_and_sends_the_built_message(self, monkeypatch): + monkeypatch.setattr(mail_smtp.smtplib, "SMTP", REAL_SMTP) + monkeypatch.setattr(mail_smtp.network, "resolve_private_addresses", lambda host: (["10.0.0.5"], None)) + seen: list[bytes] = [] + + peer = _Peer(monkeypatch, functools.partial(_serve_smtp, seen, drop_after_body=False)) + message_id = _smtp(tls=False).send(_message()) + peer.join() + + verbs = [line.split()[0].upper() for line in seen if line[:1].isalpha()] + assert verbs[:4] == [b"EHLO", b"MAIL", b"RCPT", b"DATA"] + assert b"size=" in next(line for line in seen if line.upper().startswith(b"MAIL")) + assert any(message_id.encode() in line for line in seen) + + def test_real_smtplib_losing_the_connection_after_the_body_may_have_delivered(self, monkeypatch): + monkeypatch.setattr(mail_smtp.smtplib, "SMTP", REAL_SMTP) + monkeypatch.setattr(mail_smtp.network, "resolve_private_addresses", lambda host: (["10.0.0.5"], None)) + + peer = _Peer(monkeypatch, functools.partial(_serve_smtp, [], drop_after_body=True)) + with pytest.raises(MailError) as caught: + _smtp(tls=False).send(_message()) + peer.join() + + assert caught.value.kind is MailFailure.CONNECTION_LOST + assert caught.value.delivery_unknown + + +def _serve_smtp(seen: list[bytes], f, *, drop_after_body: bool) -> None: + """A minimal SMTP peer offering SIZE; with ``drop_after_body`` it hangs up instead of answering the body.""" + f.write(b"220 hi\r\n") + f.flush() + while line := f.readline(): + seen.append(line) + verb = line.split()[0].upper() + if verb == b"EHLO": + f.write(b"250-localhost\r\n250 SIZE 100000\r\n") + elif verb == b"DATA": + f.write(b"354 go\r\n") + f.flush() + while (body := f.readline()) != b".\r\n": + seen.append(body) + if drop_after_body: + return + f.write(b"250 queued\r\n") + elif verb == b"QUIT": + f.write(b"221 bye\r\n") + f.flush() + return + else: + f.write(b"250 ok\r\n") + f.flush() + def test_success_logs_no_personal_data(self, caplog): with caplog.at_level(logging.DEBUG): _smtp().send(_message(subject="Top secret subject")) @@ -931,6 +1133,21 @@ def test_idn_domain_with_uppercase_ascii_label_is_accepted(): assert parse_address("a@Bücher.DE", role="recipient").domain == "xn--bcher-kva.DE" +def test_mail_error_defaults_describe_an_unclassified_failure(): + error = MailError("x") + + assert error.kind is MailFailure.OTHER + assert error.delivery_unknown is False + assert error.message_id is None + + +def test_not_configured_is_its_own_kind(): + assert MailNotConfiguredError("x").kind is MailFailure.NOT_CONFIGURED + with pytest.raises(MailNotConfiguredError) as caught: + NullMail().send(_message()) + assert caught.value.kind is MailFailure.NOT_CONFIGURED + + def test_mail_error_permanent_follows_the_status_code(): from jasil.providers import MailError diff --git a/tests/test_reloading.py b/tests/test_reloading.py index ff4e75e..1ddd557 100644 --- a/tests/test_reloading.py +++ b/tests/test_reloading.py @@ -21,6 +21,7 @@ GeocodedPlace, GeocodingProvider, MailError, + MailFailure, MailMessage, MailNotConfiguredError, MailProvider, @@ -346,6 +347,7 @@ def test_mail_without_a_good_backend_raises_a_transient_error(self): _mail(source).send(MESSAGE) assert not isinstance(caught.value, MailNotConfiguredError) + assert caught.value.kind is MailFailure.SETTINGS_UNAVAILABLE assert caught.value.permanent is False # The source's exception is the host's and may carry a secret, so it is not chained. assert caught.value.__cause__ is None @@ -356,8 +358,9 @@ def test_mail_after_a_rejection_raises_instead_of_reusing_old_credentials(self): provider = _mail(source) provider.send(MESSAGE) source.value = BAD_MAIL - with pytest.raises(MailNotConfiguredError): + with pytest.raises(MailNotConfiguredError) as rejected: provider.send(MESSAGE) + assert rejected.value.kind is MailFailure.NOT_CONFIGURED source.error = RuntimeError("db down") with pytest.raises(MailError, match="could not be loaded"): @@ -581,6 +584,7 @@ def test_an_unresolved_mail_relay_is_retried_after_the_delay(self, monkeypatch): with pytest.raises(MailError) as first: provider.send(MESSAGE) assert not isinstance(first.value, MailNotConfiguredError) + assert first.value.kind is MailFailure.SETTINGS_UNAVAILABLE with pytest.raises(MailError): provider.send(MESSAGE) assert calls["n"] == 1 # remembered until the delay passes From a55cbd46f37073e982e11b0772dc1e2d7d553b96 Mon Sep 17 00:00:00 2001 From: joaovitoriasilva <8648976+joaovitoriasilva@users.noreply.github.com> Date: Sat, 7 Nov 2026 09:33:36 +0000 Subject: [PATCH 2/2] fix(backends): mark a mail send ambiguous only after the 354 reply SmtpMail treated a connection lost while waiting for the reply to DATA as possibly delivered, but no body byte goes out before the 354, so the server cannot have accepted the message. DATA is now sent on its own, and the body, dot-stuffed as smtplib.SMTP.data does it, follows the 354. The success-log redaction test had ended up inside the _serve_smtp helper, so pytest no longer collected it; it is back in its class. Co-Authored-By: Claude Opus 5.5 --- jasil/backends/mail_smtp.py | 22 ++++++++-- tests/test_mail.py | 80 ++++++++++++++++++++++--------------- 2 files changed, 66 insertions(+), 36 deletions(-) diff --git a/jasil/backends/mail_smtp.py b/jasil/backends/mail_smtp.py index a79ee62..bf7dbcb 100644 --- a/jasil/backends/mail_smtp.py +++ b/jasil/backends/mail_smtp.py @@ -17,6 +17,7 @@ import contextlib import functools import logging +import re import smtplib import socket import ssl @@ -48,8 +49,9 @@ _DEADLINE_EXCEEDED = "SMTP send exceeded its overall deadline" # How far a send got: ``connecting`` until the session is up (greeting, and TLS -# when it is used), ``session`` from then on, ``data`` once ``DATA`` was sent. A -# connection lost in ``data`` may have delivered the message. +# when it is used), ``session`` from then on, ``data`` once the server answered +# ``DATA`` with 354 and the body may be on its way. A connection lost in ``data`` +# may have delivered the message. _Stage = Literal["connecting", "session", "data"] @@ -103,6 +105,14 @@ def _failure(error: OSError, stage: _Stage) -> MailError: return MailError("SMTP connection lost", kind=MailFailure.CONNECTION_LOST, delivery_unknown=stage == "data") +def _data_payload(body: bytes) -> bytes: + """``body`` as ``smtplib.SMTP.data`` sends it: leading dots doubled, ended by ``CRLF.CRLF``.""" + payload = re.sub(rb"(?m)^\.", b"..", body) + if not payload.endswith(b"\r\n"): + payload += b"\r\n" + return payload + b".\r\n" + + def _default_local_hostname() -> str: """The EHLO name when none is configured: this host's name, if usable. @@ -385,8 +395,14 @@ def _deliver(self, mime: EmailMessage) -> dict[str, tuple[int, bytes]]: body = mime.as_bytes() refused = self._envelope(connection, mime, len(body)) self._arm(connection, deadline) + # Not ``connection.data``: it waits for the 354 too, and a connection + # lost there cannot have delivered anything. + code, reply = connection.docmd("DATA") + if code != 354: + raise smtplib.SMTPDataError(code, reply) stage = "data" - code, reply = connection.data(body) + connection.send(_data_payload(body)) + code, reply = connection.getreply() if code != 250: raise smtplib.SMTPDataError(code, reply) return refused diff --git a/tests/test_mail.py b/tests/test_mail.py index 5cd2bed..47e1a57 100644 --- a/tests/test_mail.py +++ b/tests/test_mail.py @@ -13,6 +13,7 @@ import functools import logging import os +import re import smtplib import socket import ssl @@ -20,7 +21,7 @@ import threading from datetime import UTC, datetime from types import SimpleNamespace -from typing import ClassVar +from typing import ClassVar, Literal import pytest @@ -151,16 +152,27 @@ def rcpt(self, recipient): return 421, b"srvtext-xyz closing" return FakeSMTP.refused.get(recipient, (250, b"ok")) - def data(self, body): + def docmd(self, cmd): + assert cmd == "DATA" self.calls.append("data") - self.body = body - self.sent = email.message_from_bytes(body, policy=email.policy.default) + if FakeSMTP.fail_on == "drop-before-354": + raise smtplib.SMTPServerDisconnected("srvtext-xyz " + ADDRESS) + if FakeSMTP.fail_on == "data-refused": + return 554, ("srvtext-xyz " + ADDRESS).encode() + return 354, b"go" + + def send(self, payload): + assert payload.endswith(b"\r\n.\r\n") + self.body = re.sub(rb"(?m)^\.\.", b".", payload.removesuffix(b".\r\n")) + self.sent = email.message_from_bytes(self.body, policy=email.policy.default) + if FakeSMTP.fail_on == "tls-in-data": + raise ssl.SSLEOFError("srvtext-xyz") + + def getreply(self): if FakeSMTP.fail_on == "data": return 554, ("srvtext-xyz " + ADDRESS).encode() if FakeSMTP.fail_on == "drop-in-data": raise smtplib.SMTPServerDisconnected("srvtext-xyz " + ADDRESS) - if FakeSMTP.fail_on == "tls-in-data": - raise ssl.SSLEOFError("srvtext-xyz") return 250, b"queued" @@ -914,6 +926,8 @@ class TestFailuresAreRedacted: ("all-refused", "refused the recipients (status 550)", MailFailure.RECIPIENT_REFUSED, False), ("rcpt-closing", "refused the recipients (status 421)", MailFailure.RECIPIENT_REFUSED, False), ("drop-at-rcpt", "connection lost", MailFailure.CONNECTION_LOST, False), + ("data-refused", "rejected the message (status 554)", MailFailure.MESSAGE_REJECTED, False), + ("drop-before-354", "connection lost", MailFailure.CONNECTION_LOST, False), ("data", "rejected the message (status 554)", MailFailure.MESSAGE_REJECTED, False), ("drop-in-data", "connection lost", MailFailure.CONNECTION_LOST, True), ("tls-in-data", "connection lost", MailFailure.CONNECTION_LOST, True), @@ -972,6 +986,12 @@ def test_a_lost_lease_is_its_own_kind_and_nothing_was_built(self, monkeypatch, m assert caught.value.message_id is None assert not caught.value.delivery_unknown + def test_success_logs_no_personal_data(self, caplog): + with caplog.at_level(logging.DEBUG): + _smtp().send(_message(subject="Top secret subject")) + + assert ADDRESS not in caplog.text and "Top secret subject" not in caplog.text + class TestTheEnvelope: """``SmtpMail`` runs MAIL, RCPT and DATA itself, so it knows which step a failure interrupted.""" @@ -1018,55 +1038,53 @@ def test_every_recipient_is_offered(self): assert FakeSMTP.instances[0].calls.count("rcpt") == 2 - def test_the_watchdog_firing_during_data_may_have_delivered(self, monkeypatch): - monkeypatch.setattr(SmtpMail, "_watch", staticmethod(lambda connection, expired, finished: expired.set())) - FakeSMTP.fail_on = "drop-in-data" - - with pytest.raises(MailError) as caught: - _smtp().send(_message()) - - assert caught.value.kind is MailFailure.DEADLINE - assert caught.value.delivery_unknown - - def test_the_watchdog_firing_before_data_did_not_deliver(self, monkeypatch): + @pytest.mark.parametrize( + ("mode", "delivery_unknown"), + [("drop-at-rcpt", False), ("drop-before-354", False), ("drop-in-data", True)], + ) + def test_the_watchdog_firing_may_have_delivered_only_after_the_354(self, monkeypatch, mode, delivery_unknown): monkeypatch.setattr(SmtpMail, "_watch", staticmethod(lambda connection, expired, finished: expired.set())) - FakeSMTP.fail_on = "drop-at-rcpt" + FakeSMTP.fail_on = mode with pytest.raises(MailError) as caught: _smtp().send(_message()) assert caught.value.kind is MailFailure.DEADLINE - assert not caught.value.delivery_unknown + assert caught.value.delivery_unknown is delivery_unknown def test_real_smtplib_declares_the_size_and_sends_the_built_message(self, monkeypatch): monkeypatch.setattr(mail_smtp.smtplib, "SMTP", REAL_SMTP) monkeypatch.setattr(mail_smtp.network, "resolve_private_addresses", lambda host: (["10.0.0.5"], None)) seen: list[bytes] = [] - peer = _Peer(monkeypatch, functools.partial(_serve_smtp, seen, drop_after_body=False)) - message_id = _smtp(tls=False).send(_message()) + peer = _Peer(monkeypatch, functools.partial(_serve_smtp, seen, drop=None)) + message_id = _smtp(tls=False).send(_message(text=".leading dot\n.")) peer.join() verbs = [line.split()[0].upper() for line in seen if line[:1].isalpha()] assert verbs[:4] == [b"EHLO", b"MAIL", b"RCPT", b"DATA"] assert b"size=" in next(line for line in seen if line.upper().startswith(b"MAIL")) assert any(message_id.encode() in line for line in seen) + assert b"..leading dot\r\n" in seen and b"..\r\n" in seen - def test_real_smtplib_losing_the_connection_after_the_body_may_have_delivered(self, monkeypatch): + @pytest.mark.parametrize(("drop", "delivery_unknown"), [("before-354", False), ("after-body", True)]) + def test_real_smtplib_losing_the_connection_may_have_delivered_only_after_the_354( + self, monkeypatch, drop, delivery_unknown + ): monkeypatch.setattr(mail_smtp.smtplib, "SMTP", REAL_SMTP) monkeypatch.setattr(mail_smtp.network, "resolve_private_addresses", lambda host: (["10.0.0.5"], None)) - peer = _Peer(monkeypatch, functools.partial(_serve_smtp, [], drop_after_body=True)) + peer = _Peer(monkeypatch, functools.partial(_serve_smtp, [], drop=drop)) with pytest.raises(MailError) as caught: _smtp(tls=False).send(_message()) peer.join() assert caught.value.kind is MailFailure.CONNECTION_LOST - assert caught.value.delivery_unknown + assert caught.value.delivery_unknown is delivery_unknown -def _serve_smtp(seen: list[bytes], f, *, drop_after_body: bool) -> None: - """A minimal SMTP peer offering SIZE; with ``drop_after_body`` it hangs up instead of answering the body.""" +def _serve_smtp(seen: list[bytes], f, *, drop: Literal["before-354", "after-body"] | None) -> None: + """A minimal SMTP peer offering SIZE; ``drop`` makes it hang up instead of answering ``DATA`` or the body.""" f.write(b"220 hi\r\n") f.flush() while line := f.readline(): @@ -1075,11 +1093,13 @@ def _serve_smtp(seen: list[bytes], f, *, drop_after_body: bool) -> None: if verb == b"EHLO": f.write(b"250-localhost\r\n250 SIZE 100000\r\n") elif verb == b"DATA": + if drop == "before-354": + return f.write(b"354 go\r\n") f.flush() while (body := f.readline()) != b".\r\n": seen.append(body) - if drop_after_body: + if drop == "after-body": return f.write(b"250 queued\r\n") elif verb == b"QUIT": @@ -1090,12 +1110,6 @@ def _serve_smtp(seen: list[bytes], f, *, drop_after_body: bool) -> None: f.write(b"250 ok\r\n") f.flush() - def test_success_logs_no_personal_data(self, caplog): - with caplog.at_level(logging.DEBUG): - _smtp().send(_message(subject="Top secret subject")) - - assert ADDRESS not in caplog.text and "Top secret subject" not in caplog.text - class TestResolvePrivateAddresses: @staticmethod