From 253623c2639ef196179b1f1ca9c2c1518f8bd92c Mon Sep 17 00:00:00 2001 From: Sydney Date: Tue, 28 Jul 2026 16:31:09 +0530 Subject: [PATCH 1/7] fix: stop mail loops from bounces and autoresponders Inbound mail was only screened with the X-Auto-Generated header, which only helpdesk's own acks set. A real bounce or out-of-office therefore reached ticket creation and could start three separate loops: the ack in HD Ticket.after_insert replies to raised_by (the address that just bounced), threading onto a portal ticket makes frappe CC the parent doc's owner on every inbound mail, and an account with enable_auto_reply answers mailer-daemon directly. Detect machine-generated mail by the standard markers instead: RFC 3834 Auto-Submitted, the RFC 3464 multipart/report delivery-status type, and the RFC 5321 null return-path. Matches are routed to handle_bad_emails so they land in Unhandled Email rather than disappearing -- the fetch has already marked them seen either way. Co-Authored-By: Claude Opus 5 (1M context) --- helpdesk/overrides/email_account.py | 47 +++++++++++- helpdesk/overrides/test_email_account.py | 98 ++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 4 deletions(-) create mode 100644 helpdesk/overrides/test_email_account.py diff --git a/helpdesk/overrides/email_account.py b/helpdesk/overrides/email_account.py index 6e777f99d8..2ff0770c6c 100644 --- a/helpdesk/overrides/email_account.py +++ b/helpdesk/overrides/email_account.py @@ -9,6 +9,41 @@ from frappe.email.receive import InboundMail +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 RFC 3834 Auto-Submitted, the RFC 3464 + report type, or the RFC 5321 null return-path. + """ + if msg.get("X-Auto-Generated"): + return "X-Auto-Generated" + + # RFC 3834: "no" is the only human-sent value, and it may carry parameters + auto_submitted = (msg.get("Auto-Submitted") or "no").split(";")[0].strip().lower() + if auto_submitted != "no": + return f"Auto-Submitted: {auto_submitted}" + + # 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" + + return None + + class CustomInboundMail(InboundMail): """ Extend InboundMail with robust thread stitching for forwarded emails. @@ -68,15 +103,19 @@ 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) + 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..fe9231cb2c --- /dev/null +++ b/helpdesk/overrides/test_email_account.py @@ -0,0 +1,98 @@ +import unittest +from email import message_from_string + +from helpdesk.overrides.email_account import auto_generated_reason + +# 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-- +""" + +OOO_REPLY = """\ +Return-Path: +From: Someone +To: support@frappe.io +Subject: Out of office +Auto-Submitted: auto-replied + +Back on Monday. +""" + +# 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): + self.assertTrue(auto_generated_reason(message_from_string(GMAIL_BOUNCE))) + + def test_out_of_office_is_dropped(self): + self.assertTrue(auto_generated_reason(message_from_string(OOO_REPLY))) + + 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)) + + +if __name__ == "__main__": + unittest.main() From f80a004ef64ace240dec32e7d8b2f8398946841a Mon Sep 17 00:00:00 2001 From: Sydney Date: Thu, 3 Sep 2026 01:41:35 +0530 Subject: [PATCH 2/7] fix: record parked mail and note it on the ticket Record Unhandled Email on every transport, not just IMAP, and leave an internal comment on the matched ticket when a bounce or auto-reply is parked, so agents still see failed deliveries and out-of-office replies. --- helpdesk/overrides/email_account.py | 94 ++++++++++++++++- helpdesk/overrides/test_email_account.py | 122 ++++++++++++++++++++++- 2 files changed, 210 insertions(+), 6 deletions(-) diff --git a/helpdesk/overrides/email_account.py b/helpdesk/overrides/email_account.py index 2ff0770c6c..1234d732a0 100644 --- a/helpdesk/overrides/email_account.py +++ b/helpdesk/overrides/email_account.py @@ -7,6 +7,7 @@ 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: @@ -25,10 +26,8 @@ def auto_generated_reason(msg) -> str | None: if msg.get("X-Auto-Generated"): return "X-Auto-Generated" - # RFC 3834: "no" is the only human-sent value, and it may carry parameters - auto_submitted = (msg.get("Auto-Submitted") or "no").split(";")[0].strip().lower() - if auto_submitted != "no": - return f"Auto-Submitted: {auto_submitted}" + # 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 ( @@ -41,6 +40,23 @@ def auto_generated_reason(msg) -> str | None: if (msg.get("Return-Path") or "").strip() == "<>": return "null return-path" + # RFC 3834: "no" is the only human-sent value, and it may carry parameters + auto_submitted = (msg.get("Auto-Submitted") or "no").split(";")[0].strip().lower() + if auto_submitted != "no": + 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 @@ -92,6 +108,67 @@ 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) + frappe.db.commit() + + 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 the customer is out of office. + 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 = [] @@ -114,6 +191,15 @@ def process_mail(messages, append_to=None): # 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) diff --git a/helpdesk/overrides/test_email_account.py b/helpdesk/overrides/test_email_account.py index fe9231cb2c..a25f27bda5 100644 --- a/helpdesk/overrides/test_email_account.py +++ b/helpdesk/overrides/test_email_account.py @@ -1,7 +1,11 @@ import unittest from email import message_from_string -from helpdesk.overrides.email_account import auto_generated_reason +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 = """\ @@ -16,6 +20,14 @@ 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-- """ @@ -55,7 +67,19 @@ class TestAutoGeneratedReason(unittest.TestCase): def test_gmail_bounce_is_dropped(self): - self.assertTrue(auto_generated_reason(message_from_string(GMAIL_BOUNCE))) + # 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_dropped(self): self.assertTrue(auto_generated_reason(message_from_string(OOO_REPLY))) @@ -94,5 +118,99 @@ def test_auto_submitted_no_with_parameters_is_kept(self): 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_reply_leaves_out_of_office_comment(self): + self.notify(f"In-Reply-To: <{self.message_id}>\n" + OOO_REPLY) + + comments = self.ticket_comments() + self.assertEqual(len(comments), 1) + self.assertIn("Auto-reply received from someone@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}) + frappe.db.commit() + 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() From 8fdbf36addefa72816611e8aa1cab4b89c84c712 Mon Sep 17 00:00:00 2001 From: Sydney Date: Thu, 3 Sep 2026 01:46:27 +0530 Subject: [PATCH 3/7] fix: ci lint error --- helpdesk/overrides/email_account.py | 4 +++- helpdesk/overrides/test_email_account.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/helpdesk/overrides/email_account.py b/helpdesk/overrides/email_account.py index 1234d732a0..61352db3be 100644 --- a/helpdesk/overrides/email_account.py +++ b/helpdesk/overrides/email_account.py @@ -135,7 +135,9 @@ def handle_bad_emails(self, uid, raw, reason): "email_account": self.name, } ).insert(ignore_permissions=True) - frappe.db.commit() + # 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 diff --git a/helpdesk/overrides/test_email_account.py b/helpdesk/overrides/test_email_account.py index a25f27bda5..f2aa26e22e 100644 --- a/helpdesk/overrides/test_email_account.py +++ b/helpdesk/overrides/test_email_account.py @@ -193,7 +193,9 @@ class TestHandleBadEmailsRecordsAllTransports(IntegrationTestCase): def tearDown(self): frappe.db.delete("Unhandled Email", {"reason": self.REASON}) - frappe.db.commit() + # 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): From b3b717d183c2a3432c1f82888c0b2845928cecf8 Mon Sep 17 00:00:00 2001 From: Sydney Date: Thu, 3 Sep 2026 02:04:58 +0530 Subject: [PATCH 4/7] refactor: move overrides into per-doctype folders --- helpdesk/api/test_onboarding.py | 2 +- helpdesk/hooks.py | 6 +++--- helpdesk/overrides/assignment_rule/__init__.py | 0 helpdesk/overrides/{ => assignment_rule}/assignment_rule.py | 0 helpdesk/overrides/email_account/__init__.py | 0 helpdesk/overrides/{ => email_account}/email_account.py | 0 .../overrides/{ => email_account}/test_email_account.py | 5 ++++- helpdesk/overrides/user_invitation/__init__.py | 0 helpdesk/overrides/{ => user_invitation}/user_invitation.py | 0 9 files changed, 8 insertions(+), 5 deletions(-) create mode 100644 helpdesk/overrides/assignment_rule/__init__.py rename helpdesk/overrides/{ => assignment_rule}/assignment_rule.py (100%) create mode 100644 helpdesk/overrides/email_account/__init__.py rename helpdesk/overrides/{ => email_account}/email_account.py (100%) rename helpdesk/overrides/{ => email_account}/test_email_account.py (98%) create mode 100644 helpdesk/overrides/user_invitation/__init__.py rename helpdesk/overrides/{ => user_invitation}/user_invitation.py (100%) diff --git a/helpdesk/api/test_onboarding.py b/helpdesk/api/test_onboarding.py index 54f7150ba7..8de1070b8f 100644 --- a/helpdesk/api/test_onboarding.py +++ b/helpdesk/api/test_onboarding.py @@ -2,7 +2,7 @@ from frappe.tests import IntegrationTestCase from helpdesk.api.onboarding import mark_persona_captured -from helpdesk.overrides.user_invitation import HelpdeskUserInvitation +from helpdesk.overrides.user_invitation.user_invitation import HelpdeskUserInvitation BRAND = "Acme Support" diff --git a/helpdesk/hooks.py b/helpdesk/hooks.py index 2932311d02..a5730fe6a0 100644 --- a/helpdesk/hooks.py +++ b/helpdesk/hooks.py @@ -121,9 +121,9 @@ # --------------- # Override standard doctype classes override_doctype_class = { - "Email Account": "helpdesk.overrides.email_account.CustomEmailAccount", - "Assignment Rule": "helpdesk.overrides.assignment_rule.HelpdeskAssignmentRule", - "User Invitation": "helpdesk.overrides.user_invitation.HelpdeskUserInvitation", + "Email Account": "helpdesk.overrides.email_account.email_account.CustomEmailAccount", + "Assignment Rule": "helpdesk.overrides.assignment_rule.assignment_rule.HelpdeskAssignmentRule", + "User Invitation": "helpdesk.overrides.user_invitation.user_invitation.HelpdeskUserInvitation", } ignore_links_on_delete = [ diff --git a/helpdesk/overrides/assignment_rule/__init__.py b/helpdesk/overrides/assignment_rule/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/helpdesk/overrides/assignment_rule.py b/helpdesk/overrides/assignment_rule/assignment_rule.py similarity index 100% rename from helpdesk/overrides/assignment_rule.py rename to helpdesk/overrides/assignment_rule/assignment_rule.py diff --git a/helpdesk/overrides/email_account/__init__.py b/helpdesk/overrides/email_account/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/helpdesk/overrides/email_account.py b/helpdesk/overrides/email_account/email_account.py similarity index 100% rename from helpdesk/overrides/email_account.py rename to helpdesk/overrides/email_account/email_account.py diff --git a/helpdesk/overrides/test_email_account.py b/helpdesk/overrides/email_account/test_email_account.py similarity index 98% rename from helpdesk/overrides/test_email_account.py rename to helpdesk/overrides/email_account/test_email_account.py index f2aa26e22e..bd70cf95b2 100644 --- a/helpdesk/overrides/test_email_account.py +++ b/helpdesk/overrides/email_account/test_email_account.py @@ -4,7 +4,10 @@ import frappe from frappe.tests import IntegrationTestCase -from helpdesk.overrides.email_account import _failed_recipient, auto_generated_reason +from helpdesk.overrides.email_account.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 diff --git a/helpdesk/overrides/user_invitation/__init__.py b/helpdesk/overrides/user_invitation/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/helpdesk/overrides/user_invitation.py b/helpdesk/overrides/user_invitation/user_invitation.py similarity index 100% rename from helpdesk/overrides/user_invitation.py rename to helpdesk/overrides/user_invitation/user_invitation.py From 6a1693dcc6d5495bb7d5bd8186b3580ce5727786 Mon Sep 17 00:00:00 2001 From: Sydney Date: Thu, 3 Sep 2026 02:12:57 +0530 Subject: [PATCH 5/7] Revert "refactor: move overrides into per-doctype folders" This reverts commit b3b717d183c2a3432c1f82888c0b2845928cecf8. --- helpdesk/api/test_onboarding.py | 2 +- helpdesk/hooks.py | 6 +++--- helpdesk/overrides/{assignment_rule => }/assignment_rule.py | 0 helpdesk/overrides/assignment_rule/__init__.py | 0 helpdesk/overrides/{email_account => }/email_account.py | 0 helpdesk/overrides/email_account/__init__.py | 0 .../overrides/{email_account => }/test_email_account.py | 5 +---- helpdesk/overrides/{user_invitation => }/user_invitation.py | 0 helpdesk/overrides/user_invitation/__init__.py | 0 9 files changed, 5 insertions(+), 8 deletions(-) rename helpdesk/overrides/{assignment_rule => }/assignment_rule.py (100%) delete mode 100644 helpdesk/overrides/assignment_rule/__init__.py rename helpdesk/overrides/{email_account => }/email_account.py (100%) delete mode 100644 helpdesk/overrides/email_account/__init__.py rename helpdesk/overrides/{email_account => }/test_email_account.py (98%) rename helpdesk/overrides/{user_invitation => }/user_invitation.py (100%) delete mode 100644 helpdesk/overrides/user_invitation/__init__.py diff --git a/helpdesk/api/test_onboarding.py b/helpdesk/api/test_onboarding.py index 8de1070b8f..54f7150ba7 100644 --- a/helpdesk/api/test_onboarding.py +++ b/helpdesk/api/test_onboarding.py @@ -2,7 +2,7 @@ from frappe.tests import IntegrationTestCase from helpdesk.api.onboarding import mark_persona_captured -from helpdesk.overrides.user_invitation.user_invitation import HelpdeskUserInvitation +from helpdesk.overrides.user_invitation import HelpdeskUserInvitation BRAND = "Acme Support" diff --git a/helpdesk/hooks.py b/helpdesk/hooks.py index a5730fe6a0..2932311d02 100644 --- a/helpdesk/hooks.py +++ b/helpdesk/hooks.py @@ -121,9 +121,9 @@ # --------------- # Override standard doctype classes override_doctype_class = { - "Email Account": "helpdesk.overrides.email_account.email_account.CustomEmailAccount", - "Assignment Rule": "helpdesk.overrides.assignment_rule.assignment_rule.HelpdeskAssignmentRule", - "User Invitation": "helpdesk.overrides.user_invitation.user_invitation.HelpdeskUserInvitation", + "Email Account": "helpdesk.overrides.email_account.CustomEmailAccount", + "Assignment Rule": "helpdesk.overrides.assignment_rule.HelpdeskAssignmentRule", + "User Invitation": "helpdesk.overrides.user_invitation.HelpdeskUserInvitation", } ignore_links_on_delete = [ diff --git a/helpdesk/overrides/assignment_rule/assignment_rule.py b/helpdesk/overrides/assignment_rule.py similarity index 100% rename from helpdesk/overrides/assignment_rule/assignment_rule.py rename to helpdesk/overrides/assignment_rule.py diff --git a/helpdesk/overrides/assignment_rule/__init__.py b/helpdesk/overrides/assignment_rule/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/helpdesk/overrides/email_account/email_account.py b/helpdesk/overrides/email_account.py similarity index 100% rename from helpdesk/overrides/email_account/email_account.py rename to helpdesk/overrides/email_account.py diff --git a/helpdesk/overrides/email_account/__init__.py b/helpdesk/overrides/email_account/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/helpdesk/overrides/email_account/test_email_account.py b/helpdesk/overrides/test_email_account.py similarity index 98% rename from helpdesk/overrides/email_account/test_email_account.py rename to helpdesk/overrides/test_email_account.py index bd70cf95b2..f2aa26e22e 100644 --- a/helpdesk/overrides/email_account/test_email_account.py +++ b/helpdesk/overrides/test_email_account.py @@ -4,10 +4,7 @@ import frappe from frappe.tests import IntegrationTestCase -from helpdesk.overrides.email_account.email_account import ( - _failed_recipient, - auto_generated_reason, -) +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 diff --git a/helpdesk/overrides/user_invitation/user_invitation.py b/helpdesk/overrides/user_invitation.py similarity index 100% rename from helpdesk/overrides/user_invitation/user_invitation.py rename to helpdesk/overrides/user_invitation.py diff --git a/helpdesk/overrides/user_invitation/__init__.py b/helpdesk/overrides/user_invitation/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 From acd2e2860062cd7d64b3891cb17c3a987305097a Mon Sep 17 00:00:00 2001 From: Sydney Date: Thu, 3 Sep 2026 02:53:36 +0530 Subject: [PATCH 6/7] fix: let auto-replied mail thread onto its ticket An out of office should reach agents on the ticket, and auto-replied senders rate-limit themselves so they cannot sustain a loop the way bounces and auto-generated feeds can. Those still get parked. --- helpdesk/overrides/email_account.py | 18 ++++++++++----- helpdesk/overrides/test_email_account.py | 28 +++++++++++++++++++----- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/helpdesk/overrides/email_account.py b/helpdesk/overrides/email_account.py index 61352db3be..a710f5c7f6 100644 --- a/helpdesk/overrides/email_account.py +++ b/helpdesk/overrides/email_account.py @@ -20,8 +20,12 @@ def auto_generated_reason(msg) -> str | None: (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 RFC 3834 Auto-Submitted, the RFC 3464 - report type, or the RFC 5321 null return-path. + 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" @@ -40,9 +44,11 @@ def auto_generated_reason(msg) -> str | None: if (msg.get("Return-Path") or "").strip() == "<>": return "null return-path" - # RFC 3834: "no" is the only human-sent value, and it may carry parameters + # 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 != "no": + if auto_submitted not in ("no", "auto-replied"): return f"Auto-Submitted: {auto_submitted}" return None @@ -141,8 +147,8 @@ def handle_bad_emails(self, uid, raw, reason): 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 the customer is out of office. - Leave an internal comment on the ticket the mail belongs to.""" + 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 diff --git a/helpdesk/overrides/test_email_account.py b/helpdesk/overrides/test_email_account.py index f2aa26e22e..e0bde9b8df 100644 --- a/helpdesk/overrides/test_email_account.py +++ b/helpdesk/overrides/test_email_account.py @@ -41,6 +41,17 @@ 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 @@ -81,8 +92,15 @@ def test_failed_recipient_read_from_delivery_status(self): 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_dropped(self): - self.assertTrue(auto_generated_reason(message_from_string(OOO_REPLY))) + 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 @@ -169,12 +187,12 @@ def test_bounce_leaves_delivery_failed_comment(self): self.assertIn("Delivery failed", comments[0]) self.assertIn("dead@example.com", comments[0]) - def test_auto_reply_leaves_out_of_office_comment(self): - self.notify(f"In-Reply-To: <{self.message_id}>\n" + OOO_REPLY) + 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 someone@example.com", comments[0]) + self.assertIn("Auto-reply received from postmaster@example.com", comments[0]) def test_unmatched_bounce_leaves_no_comment(self): self.notify(GMAIL_BOUNCE) From 4592c40f4e5026d97369664858e4ef1482202945 Mon Sep 17 00:00:00 2001 From: Sydney Date: Thu, 3 Sep 2026 16:19:27 +0530 Subject: [PATCH 7/7] chore: shorten comments --- helpdesk/overrides/email_account.py | 54 +++++++++-------------------- 1 file changed, 17 insertions(+), 37 deletions(-) diff --git a/helpdesk/overrides/email_account.py b/helpdesk/overrides/email_account.py index a710f5c7f6..b67a5f2a84 100644 --- a/helpdesk/overrides/email_account.py +++ b/helpdesk/overrides/email_account.py @@ -11,42 +11,30 @@ def auto_generated_reason(msg) -> str | None: - """Why this mail is machine-generated (bounce or autoresponder), else None. + """Why this mail must not become a ticket or a reply, 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. + A bounce or alert that gets in starts a mail loop: the new-ticket ack, + the portal CC, and enable_auto_reply all answer it, and it answers back. + Out-of-office mail (auto-replied) passes on purpose: senders rate-limit + it, and agents should see it on the ticket. """ 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) + # bounce markers first: a DSN usually carries Auto-Submitted too - # RFC 3464 delivery status notification -- survives a stripped Return-Path + # RFC 3464 delivery report -- works even without a 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) + # bounces must use an empty sender (RFC 5321) 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 + # RFC 3834; "no" means a human sent it and may carry parameters 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}" @@ -115,12 +103,8 @@ 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. - """ + """The framework version only records for IMAP; POP3 and Frappe + Mail drops deserve the same trace, so this one has no gate.""" try: raw_str = ( raw.decode("ASCII", "replace") @@ -141,14 +125,12 @@ def handle_bad_emails(self, uid, raw, reason): "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 + # keep the record even if a later mail in this batch fails 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.""" + """Parked mail never shows on the ticket, so leave a comment + there -- agents must know their reply bounced.""" communication = CustomInboundMail(message, self).parent_communication() if not communication or communication.reference_doctype != "HD Ticket": return @@ -171,7 +153,7 @@ def notify_ticket_of_parked_mail(self, message, msg, reason): ) comment = frappe.new_doc("HD Ticket Comment") - # not frappe.session.user: no human acted, even on a manual pull + # a system note, not the pulling user's comment.commented_by = "Administrator" comment.reference_ticket = communication.reference_name comment.content = content @@ -194,12 +176,10 @@ def process_mail(messages, append_to=None): 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. + # machine mail starts loops -- park it, with 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 + # our own looped-back ack is not news for agents if reason != "X-Auto-Generated": try: self.notify_ticket_of_parked_mail(message, _msg, reason)