diff --git a/helpdesk/overrides/email_account.py b/helpdesk/overrides/email_account.py index 6e777f99d8..a710f5c7f6 100644 --- a/helpdesk/overrides/email_account.py +++ b/helpdesk/overrides/email_account.py @@ -7,6 +7,63 @@ from frappe.email.doctype.email_account.email_account import EmailAccount from frappe.email.doctype.email_queue.email_queue import EmailQueue from frappe.email.receive import InboundMail +from frappe.utils import parse_addr + + +def auto_generated_reason(msg) -> str | None: + """Why this mail is machine-generated (bounce or autoresponder), else None. + + One such mail can start three different loops: opening a ticket acks + raised_by straight back to the address that just bounced + (HD Ticket.after_insert), threading onto a portal ticket makes frappe CC the + parent doc's owner -- the same dead address -- on every inbound mail + (mail_cc), and an account with enable_auto_reply answers mailer-daemon. + + X-Auto-Generated only catches helpdesk's own acks, which stamp it. Real + bounces announce themselves with the RFC 3464 report type or the RFC 5321 + null return-path; other machine mail with RFC 3834 Auto-Submitted. + + Auto-replied mail (out of office, read receipts) is deliberately let + through: it threads onto its ticket so agents see it, and well-behaved + responders rate-limit themselves, so it cannot sustain a loop. + """ + if msg.get("X-Auto-Generated"): + return "X-Auto-Generated" + + # bounce markers first, so the reason distinguishes a dead address from a + # mere autoresponder (a DSN usually carries Auto-Submitted too) + + # RFC 3464 delivery status notification -- survives a stripped Return-Path + if ( + msg.get_content_type() == "multipart/report" + and msg.get_param("report-type") == "delivery-status" + ): + return "delivery status notification" + + # bounces MUST carry a null envelope sender (RFC 5321 ยง6.1) + if (msg.get("Return-Path") or "").strip() == "<>": + return "null return-path" + + # RFC 3834 ("no" may carry parameters, e.g. "no; owner=..."). auto-replied + # passes: an out-of-office should reach the ticket, and it is rate-limited + # by the sender so it cannot loop the way a bounce or an alert feed can + auto_submitted = (msg.get("Auto-Submitted") or "no").split(";")[0].strip().lower() + if auto_submitted not in ("no", "auto-replied"): + return f"Auto-Submitted: {auto_submitted}" + + return None + + +def _failed_recipient(msg) -> str | None: + """The address a DSN reports as undeliverable, if it names one.""" + for part in msg.walk(): + if part.get_content_type() != "message/delivery-status": + continue + for status_block in part.get_payload(): + recipient = status_block.get("Final-Recipient") or "" + if ";" in recipient: + return recipient.split(";", 1)[1].strip() + return None class CustomInboundMail(InboundMail): @@ -57,6 +114,69 @@ def parent_communication(self): class CustomEmailAccount(EmailAccount): + def handle_bad_emails(self, uid, raw, reason): + """Same record as the framework version, without its use_imap gate. + + POP3 and Frappe Mail hit the same drop paths as IMAP, and nothing + that reads Unhandled Email is IMAP-specific -- a silent drop would + hide a misclassified customer mail, so record on every transport. + """ + try: + raw_str = ( + raw.decode("ASCII", "replace") + if isinstance(raw, bytes) + else raw.encode(errors="replace").decode() + ) + message_id = message_from_string(raw_str).get("Message-ID") + except Exception: + raw_str = message_id = "can't be parsed" + + frappe.get_doc( + { + "doctype": "Unhandled Email", + "raw": raw_str, + "uid": uid, + "reason": reason, + "message_id": message_id, + "email_account": self.name, + } + ).insert(ignore_permissions=True) + # the record must survive a later mail in the batch failing mid-pull; + # the framework version commits here for the same reason + frappe.db.commit() # nosemgrep + + def notify_ticket_of_parked_mail(self, message, msg, reason): + """Parked mail no longer threads onto tickets, so agents would never + learn that a reply bounced or that a machine answered. Leave an + internal comment on the ticket the mail belongs to.""" + communication = CustomInboundMail(message, self).parent_communication() + if not communication or communication.reference_doctype != "HD Ticket": + return + + if reason.startswith("Auto-Submitted"): + sender = parse_addr(msg.get("From") or "")[1] + content = _("Auto-reply received from {0}.").format( + sender or _("the customer") + ) + else: + recipient = _failed_recipient(msg) + content = ( + _( + "Delivery failed: the reply to this ticket could not be delivered to {0}." + ).format(recipient) + if recipient + else _( + "Delivery failed: the reply to this ticket could not be delivered." + ) + ) + + comment = frappe.new_doc("HD Ticket Comment") + # not frappe.session.user: no human acted, even on a manual pull + comment.commented_by = "Administrator" + comment.reference_ticket = communication.reference_name + comment.content = content + comment.save(ignore_permissions=True) + def get_inbound_mails(self) -> list[InboundMail]: """retrive and return inbound mails.""" mails = [] @@ -68,15 +188,28 @@ def process_mail(messages, append_to=None): message.decode("utf-8", errors="replace") ) - # Important: If the email is auto-generated, we do not create a ticket - if _msg.get("X-Auto-Generated"): - continue - uid = ( messages["uid_list"][index] if messages.get("uid_list") else None ) + + # Important: auto-generated mail must never reach a ticket, it + # starts a mail loop. The fetch already marked it seen, so park + # it in Unhandled Email instead of dropping it without a trace. + if reason := auto_generated_reason(_msg): + self.handle_bad_emails(uid, message, reason) + # our own looped-back ack carries no news for agents + if reason != "X-Auto-Generated": + try: + self.notify_ticket_of_parked_mail(message, _msg, reason) + except Exception: + frappe.log_error( + title=_("Could not note parked mail on ticket"), + message=frappe.get_traceback(), + ) + continue + seen_status = messages.get("seen_status", {}).get(uid) if self.email_sync_option != "UNSEEN" or seen_status != "SEEN": _inbound_mail = CustomInboundMail( diff --git a/helpdesk/overrides/test_email_account.py b/helpdesk/overrides/test_email_account.py new file mode 100644 index 0000000000..e0bde9b8df --- /dev/null +++ b/helpdesk/overrides/test_email_account.py @@ -0,0 +1,236 @@ +import unittest +from email import message_from_string + +import frappe +from frappe.tests import IntegrationTestCase + +from helpdesk.overrides.email_account import _failed_recipient, auto_generated_reason +from helpdesk.test_utils import make_ticket + +# Real Gmail DSN headers -- the shape that caused the 95-mail loop on ticket 74703 +GMAIL_BOUNCE = """\ +Return-Path: <> +From: Mail Delivery Subsystem +To: support@frappe.io +Subject: Delivery Status Notification (Failure) +Auto-Submitted: auto-replied +Content-Type: multipart/report; report-type=delivery-status; boundary="b" + +--b +Content-Type: text/plain + +The email account that you tried to reach does not exist. +--b +Content-Type: message/delivery-status + +Reporting-MTA: dns; googlemail.com + +Final-Recipient: rfc822; dead@example.com +Action: failed +Status: 5.1.1 +--b-- +""" + +OOO_REPLY = """\ +Return-Path: +From: Someone +To: support@frappe.io +Subject: Out of office +Auto-Submitted: auto-replied + +Back on Monday. +""" + +# machine-originated mail that is not a reply to anything human +QUARANTINE_ALERT = """\ +Return-Path: +From: Mail Gateway +To: support@frappe.io +Subject: Message held in quarantine +Auto-Submitted: auto-generated + +A message addressed to you was held for review. +""" + +# same DSN reaching us over POP3/Frappe Mail, where no MTA added a Return-Path +BARE_DSN = """\ +From: Mail Delivery Subsystem +To: support@frappe.io +Subject: Undelivered Mail Returned to Sender +Content-Type: multipart/report; report-type=delivery-status; boundary="b" + +--b +Content-Type: text/plain + +Recipient address rejected: User unknown. +--b-- +""" + +REAL_CUSTOMER_REPLY = """\ +Return-Path: +From: Omkar +To: support@frappe.io +Subject: Re: URGENT : Tickets raised outside working hours + +Any update on this? +""" + + +class TestAutoGeneratedReason(unittest.TestCase): + def test_gmail_bounce_is_dropped(self): + # the DSN marker must win over Auto-Submitted, the comment wording keys off it + self.assertEqual( + auto_generated_reason(message_from_string(GMAIL_BOUNCE)), + "delivery status notification", + ) + + def test_failed_recipient_read_from_delivery_status(self): + self.assertEqual( + _failed_recipient(message_from_string(GMAIL_BOUNCE)), "dead@example.com" + ) + + def test_failed_recipient_absent_when_dsn_names_none(self): + self.assertIsNone(_failed_recipient(message_from_string(BARE_DSN))) + + def test_out_of_office_is_kept(self): + # auto-replied threads onto its ticket so agents see it + self.assertIsNone(auto_generated_reason(message_from_string(OOO_REPLY))) + + def test_auto_generated_alert_is_dropped(self): + self.assertEqual( + auto_generated_reason(message_from_string(QUARANTINE_ALERT)), + "Auto-Submitted: auto-generated", + ) + + def test_dsn_without_return_path_is_dropped(self): + # only the RFC 3464 report type is left to go on + self.assertEqual( + auto_generated_reason(message_from_string(BARE_DSN)), + "delivery status notification", + ) + + def test_legacy_x_auto_generated_still_dropped(self): + msg = message_from_string(REAL_CUSTOMER_REPLY) + msg["X-Auto-Generated"] = "auto-replied" + self.assertTrue(auto_generated_reason(msg)) + + def test_real_reply_is_kept(self): + self.assertIsNone( + auto_generated_reason(message_from_string(REAL_CUSTOMER_REPLY)) + ) + + def test_missing_headers_do_not_look_like_a_bounce(self): + # no Return-Path / Auto-Submitted at all -- must not be mistaken for a DSN + msg = message_from_string("From: a@b.com\nTo: support@frappe.io\n\nhi") + self.assertIsNone(auto_generated_reason(msg)) + + def test_auto_submitted_no_is_kept(self): + msg = message_from_string(REAL_CUSTOMER_REPLY) + msg["Auto-Submitted"] = "no" + self.assertIsNone(auto_generated_reason(msg)) + + def test_auto_submitted_no_with_parameters_is_kept(self): + # RFC 3834 allows parameters after the value -- still human-sent + msg = message_from_string(REAL_CUSTOMER_REPLY) + msg["Auto-Submitted"] = "no; owner=someone@example.com" + self.assertIsNone(auto_generated_reason(msg)) + + +@unittest.skipUnless( + getattr(frappe.local, "site", None), "needs a site (run via bench run-tests)" +) +class TestParkedMailTicketComment(IntegrationTestCase): + """A parked bounce or auto-reply that belongs to a ticket must leave an + internal comment there, so agents still see what happened.""" + + def setUp(self): + super().setUp() + self.ticket = make_ticket( + subject="Parked mail comment test", + raised_by="parked-mail-customer@example.com", + ) + self.message_id = f"reply-{self.ticket.name}@test.local" + frappe.get_doc( + { + "doctype": "Communication", + "communication_type": "Communication", + "communication_medium": "Email", + "sent_or_received": "Sent", + "subject": "Re: Parked mail comment test", + "sender": "support@example.com", + "recipients": self.ticket.raised_by, + "reference_doctype": "HD Ticket", + "reference_name": self.ticket.name, + "message_id": self.message_id, + } + ).insert(ignore_permissions=True) + self.account = frappe.get_last_doc("Email Account") + + def ticket_comments(self): + return frappe.get_all( + "HD Ticket Comment", + filters={"reference_ticket": self.ticket.name}, + pluck="content", + ) + + def notify(self, raw): + msg = message_from_string(raw) + reason = auto_generated_reason(msg) + self.assertIsNotNone(reason) + self.account.notify_ticket_of_parked_mail(raw, msg, reason) + + def test_bounce_leaves_delivery_failed_comment(self): + self.notify(f"In-Reply-To: <{self.message_id}>\n" + GMAIL_BOUNCE) + + comments = self.ticket_comments() + self.assertEqual(len(comments), 1) + self.assertIn("Delivery failed", comments[0]) + self.assertIn("dead@example.com", comments[0]) + + def test_auto_generated_reply_leaves_comment(self): + self.notify(f"In-Reply-To: <{self.message_id}>\n" + QUARANTINE_ALERT) + + comments = self.ticket_comments() + self.assertEqual(len(comments), 1) + self.assertIn("Auto-reply received from postmaster@example.com", comments[0]) + + def test_unmatched_bounce_leaves_no_comment(self): + self.notify(GMAIL_BOUNCE) + + self.assertEqual(self.ticket_comments(), []) + + +@unittest.skipUnless( + getattr(frappe.local, "site", None), "needs a site (run via bench run-tests)" +) +class TestHandleBadEmailsRecordsAllTransports(IntegrationTestCase): + """The framework skips the Unhandled Email record for non-IMAP accounts; + the override must leave a trace for POP3/Frappe Mail drops too.""" + + REASON = "test: bounce trace" + + def tearDown(self): + frappe.db.delete("Unhandled Email", {"reason": self.REASON}) + # handle_bad_emails commits, so the cleanup must commit too or the + # class-level rollback would resurrect the row + frappe.db.commit() # nosemgrep + super().tearDown() + + def test_non_imap_drop_lands_in_unhandled_email(self): + account = frappe.get_last_doc("Email Account") + account.use_imap = 0 + + account.handle_bad_emails(None, GMAIL_BOUNCE, self.REASON) + + row = frappe.db.get_value( + "Unhandled Email", + {"reason": self.REASON}, + ["email_account", "message_id"], + as_dict=True, + ) + self.assertIsNotNone(row) + self.assertEqual(row.email_account, account.name) + + +if __name__ == "__main__": + unittest.main()