From a8d8e5b4359fb72b1b159fc2303b5e83748773f6 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste PENRATH Date: Wed, 29 Jul 2026 13:33:17 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(inbound)=20thread=20replies=20whos?= =?UTF-8?q?e=20subject=20was=20rewritten=20(#765)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A reply carrying In-Reply-To to a message we already hold was still rejected when its subject differed, because the delivery path required both a reference match and an identical canonical subject. A subject edited mid-conversation therefore started a brand new thread. The import path had its own laxer rule (message-ids only), so the very same conversation was grouped differently depending on whether it was imported or received over SMTP. Both paths now share find_thread_for_message: In-Reply-To is trusted on its own (RFC 8621 §3 only allows splitting on subject, never requires it), while References — which some clients recycle to start unrelated topics — still needs a matching canonical subject. The canonicalization also accepts "Re:subject" with no space, common on mobile clients. --- src/backend/core/mda/inbound_create.py | 119 ++++++----------- .../core/tests/api/test_inbound_mta.py | 46 ++++++- src/backend/core/tests/mda/test_inbound.py | 124 ++++++++++++++++-- 3 files changed, 195 insertions(+), 94 deletions(-) diff --git a/src/backend/core/mda/inbound_create.py b/src/backend/core/mda/inbound_create.py index 2295ae25..1e570db3 100644 --- a/src/backend/core/mda/inbound_create.py +++ b/src/backend/core/mda/inbound_create.py @@ -78,82 +78,66 @@ def inbound_mailbox_lock(mailbox_id: uuid.UUID): def _canonicalize_subject(subject: str | None) -> str: """Strip leading ``Re:`` / ``Fwd:`` (and i18n variants) for thread match.""" + # ``\s*`` after the colon, not ``\s+``: mobile clients regularly send + # "Re:subject" with no space, and a prefix left in place makes two + # messages of the same conversation compare as different subjects. return re.sub( - r"^((re|fwd|fw|rep|tr|rép)\s*:\s+)+", + r"^((re|fwd|fw|rep|tr|rép)\s*:\s*)+", "", (subject or "").lower(), flags=re.IGNORECASE, ).strip() -def find_thread_for_inbound_message( - parsed_email: JmapEmail, mailbox: models.Mailbox -) -> models.Thread | None: - """Attempt to find an existing thread for an inbound message. - - Follows JMAP spec recommendations: - https://www.ietf.org/rfc/rfc8621.html#section-3 - """ - in_reply_to = first_msgid(parsed_email.get("inReplyTo")) - references = parsed_email.get("references") or [] - - all_referenced_ids = set(references) - if in_reply_to: - all_referenced_ids.add(in_reply_to) - - if not all_referenced_ids: - return None # No headers to match on - - # Find potential parent messages in the target mailbox based on references - potential_parents = list( +def _parent_messages(mime_ids: list[str], mailbox: models.Mailbox): + """Messages of ``mailbox`` whose Message-ID is one of ``mime_ids``, newest first.""" + return ( models.Message.objects.filter( - mime_id__in=list(all_referenced_ids), + mime_id__in=mime_ids, thread__accesses__mailbox=mailbox, ) .select_related("thread") .order_by("-created_at") # Prefer newer matches if multiple found ) - if len(potential_parents) == 0: - return None # No matching messages found by ID in this mailbox - # Strategy 1: Match by reference AND canonical subject - incoming_subject_canonical = _canonicalize_subject(parsed_email.get("subject")) - for parent in potential_parents: - parent_subject_canonical = _canonicalize_subject(parent.subject) - if incoming_subject_canonical == parent_subject_canonical: - return parent.thread # Found a match! - - # Strategy 2 (Fallback): If no subject match, return thread of the most recent parent message - # The list is ordered by -created_at, so the first element is the latest match. - return None # potential_parents.first().thread - - -def find_thread_for_import( +def find_thread_for_message( parsed_email: JmapEmail, mailbox: models.Mailbox ) -> models.Thread | None: - """ - During import, try to find an existing thread that contains messages - with the same subject or referenced message IDs. - """ + """Attempt to find the existing thread an incoming message belongs to. - subject = parsed_email.get("subject", "") + Two levels of evidence, strongest first: + + 1. ``In-Reply-To`` — an explicit, unambiguous pointer to a single + parent. When that parent is already in the mailbox the message + belongs to its thread whatever the subject says: participants + rewrite subjects mid-conversation, and RFC 8621 §3 only *allows* + splitting a thread on subject, it never requires it. + 2. ``References`` — a chain some MUAs recycle across unrelated + conversations (replying to an old mail to start a new topic), so a + matching canonical subject is required before trusting it. + + Shared by the SMTP delivery path and the importers: a mailbox that is + imported and then kept in sync over SMTP must be threaded by one rule, + otherwise the same conversation splits differently on each path. + """ in_reply_to = first_msgid(parsed_email.get("inReplyTo")) - references = parsed_email.get("references") or [] + references = [ + ref for ref in (parsed_email.get("references") or []) if ref != in_reply_to + ] - # First try to find a thread by message IDs - thread = _find_thread_by_message_ids(in_reply_to, references, mailbox) + if in_reply_to: + parent = _parent_messages([in_reply_to], mailbox).first() + if parent: + return parent.thread - # If no thread found by message IDs, try by subject - if not thread and subject: - # Look for threads with similar subjects - canonical_subject = _canonicalize_subject(subject) - thread = models.Thread.objects.filter( - subject__iregex=rf"^(re|fwd|fw|rep|tr|rép)\s*:\s*{re.escape(canonical_subject)}$", - accesses__mailbox=mailbox, - ).first() + if references: + incoming_subject_canonical = _canonicalize_subject(parsed_email.get("subject")) + for parent in _parent_messages(references, mailbox): + if _canonicalize_subject(parent.subject) == incoming_subject_canonical: + return parent.thread - return thread + return None def _create_thread(parsed_email: JmapEmail, mailbox: models.Mailbox) -> models.Thread: @@ -183,24 +167,6 @@ def _create_thread(parsed_email: JmapEmail, mailbox: models.Mailbox) -> models.T return thread -def _find_thread_by_message_ids( - in_reply_to: str, references: list[str], mailbox: models.Mailbox -) -> models.Thread | None: - """Find thread by message IDs (``inReplyTo`` and ``references``).""" - if in_reply_to or references: - thread = models.Thread.objects.filter( - messages__mime_id__in=[in_reply_to] if in_reply_to else [], - accesses__mailbox=mailbox, - ).first() - if not thread and references: - thread = models.Thread.objects.filter( - messages__mime_id__in=references, - accesses__mailbox=mailbox, - ).first() - return thread - return None - - def _record_divergent_rcpt( postmark: dict, recipient_email: str, parsed_email: JmapEmail ) -> None: @@ -315,14 +281,7 @@ def _create_message_from_inbound( # pylint: disable=too-many-arguments # --- 3. Find or Create Thread --- # try: - thread = None - if is_import: - thread = find_thread_for_import(parsed_email, mailbox) - - # If no thread found or not an import, use normal thread finding logic - if not thread: - thread = find_thread_for_inbound_message(parsed_email, mailbox) - + thread = find_thread_for_message(parsed_email, mailbox) if not thread: thread = _create_thread(parsed_email, mailbox) diff --git a/src/backend/core/tests/api/test_inbound_mta.py b/src/backend/core/tests/api/test_inbound_mta.py index 2a4f2f06..f4a9b705 100644 --- a/src/backend/core/tests/api/test_inbound_mta.py +++ b/src/backend/core/tests/api/test_inbound_mta.py @@ -842,10 +842,14 @@ class TestMTAInboundEmailThreading: assert new_message.thread == initial_thread assert new_message.subject == reply_subject - def test_reply_creates_new_thread_different_subject( + def test_reply_keeps_thread_when_subject_rewritten( self, api_client: APIClient, valid_jwt_token ): - """Test reply creates a new thread if the canonical subject differs.""" + """Test a reply stays in its thread even when its subject was rewritten. + + In-Reply-To points at a message we hold: that link is explicit and + wins over the subject, which participants routinely edit. + """ initial_subject = "Important Meeting" initial_mime_id = "meeting.789@example.com" initial_thread, initial_message = self._create_initial_message( @@ -868,6 +872,44 @@ class TestMTAInboundEmailThreading: HTTP_AUTHORIZATION=f"Bearer {token}", ) + assert response.status_code == status.HTTP_200_OK + assert models.Thread.objects.count() == 1 # No new thread created + assert models.Message.objects.count() == 2 + new_message = models.Message.objects.exclude(id=initial_message.id).first() + assert new_message is not None + assert new_message.thread == initial_thread + assert new_message.subject == reply_subject + + def test_references_only_creates_new_thread_different_subject( + self, api_client: APIClient, valid_jwt_token + ): + """Test a new topic built on a recycled References chain is split off. + + Without In-Reply-To, References alone is not proof of continuity: a + differing canonical subject means a new thread. + """ + initial_subject = "Important Meeting" + initial_mime_id = "meeting.790@example.com" + initial_thread, initial_message = self._create_initial_message( + initial_subject, initial_mime_id + ) + + reply_subject = "Completely Different Topic" # Subject changed + reply_email_bytes = self._create_reply_email( + self.recipient_email, reply_subject, references=[initial_mime_id] + ) + + token = valid_jwt_token( + reply_email_bytes, {"original_recipients": [self.recipient_email]} + ) + + response = api_client.post( + "/api/v1.0/inbound/mta/deliver/", + data=reply_email_bytes, + content_type="message/rfc822", + HTTP_AUTHORIZATION=f"Bearer {token}", + ) + assert response.status_code == status.HTTP_200_OK assert models.Thread.objects.count() == 2 # New thread created assert models.Message.objects.count() == 2 diff --git a/src/backend/core/tests/mda/test_inbound.py b/src/backend/core/tests/mda/test_inbound.py index 1240ad98..4146c282 100644 --- a/src/backend/core/tests/mda/test_inbound.py +++ b/src/backend/core/tests/mda/test_inbound.py @@ -15,7 +15,7 @@ from core.mda.inbound import deliver_inbound_message from core.mda.inbound_create import ( _create_message_from_inbound, _record_divergent_rcpt, - find_thread_for_inbound_message, + find_thread_for_message, ) @@ -65,7 +65,7 @@ class TestRecordDivergentRcpt: @pytest.mark.django_db class TestFindThread: - """Unit tests for the find_thread_for_inbound_message helper.""" + """Unit tests for the find_thread_for_message helper.""" mailbox = None @@ -102,7 +102,7 @@ class TestFindThread: "from": [{"email": "replier@a.com"}], } - found_thread = find_thread_for_inbound_message(parsed_reply, self.mailbox) + found_thread = find_thread_for_message(parsed_reply, self.mailbox) assert found_thread == initial_thread @pytest.mark.parametrize( @@ -132,7 +132,7 @@ class TestFindThread: "from": [{"email": "replier@a.com"}], } - found_thread = find_thread_for_inbound_message(parsed_reply, self.mailbox) + found_thread = find_thread_for_message(parsed_reply, self.mailbox) assert found_thread == initial_thread @pytest.mark.parametrize( @@ -142,8 +142,12 @@ class TestFindThread: enums.ThreadAccessRoleChoices.VIEWER, ], ) - def test_find_fallback_no_subject_match(self, role): - """Thread found via References header, falling back when subjects don't normalize.""" + def test_references_only_with_different_subject_returns_none(self, role): + """References alone + an unrelated subject → no thread. + + Some MUAs recycle the References chain when starting a new topic from + an old message, so that header on its own is not proof of continuity. + """ initial_subject = "Meeting Request" initial_mime_id = "meeting.abc@example.com" initial_thread = factories.ThreadFactory(subject=initial_subject) @@ -164,9 +168,64 @@ class TestFindThread: } # Create a new thread - found_thread = find_thread_for_inbound_message(parsed_reply, self.mailbox) + found_thread = find_thread_for_message(parsed_reply, self.mailbox) assert found_thread is None + def test_find_by_in_reply_to_with_rewritten_subject(self): + """In-Reply-To wins over a subject rewritten mid-conversation. + + Real-world case: the third message of a conversation replies to the + second one but its subject was edited, which used to start a brand + new thread on the delivery path. + """ + first_mime_id = "first.789@example.com" + second_mime_id = "second.789@example.com" + initial_thread = factories.ThreadFactory(subject="Original topic") + factories.ThreadAccessFactory( + mailbox=self.mailbox, + thread=initial_thread, + role=enums.ThreadAccessRoleChoices.EDITOR, + ) + factories.MessageFactory( + thread=initial_thread, mime_id=first_mime_id, subject="Original topic" + ) + factories.MessageFactory( + thread=initial_thread, mime_id=second_mime_id, subject="Re: Original topic" + ) + + parsed_reply = { + "subject": "Re: Renamed topic", + "inReplyTo": [second_mime_id], + "references": [first_mime_id, second_mime_id], + "from": [{"email": "replier@a.com"}], + } + + found_thread = find_thread_for_message(parsed_reply, self.mailbox) + assert found_thread == initial_thread + + def test_find_by_references_without_space_after_prefix(self): + """A ``Re:`` prefix with no space after the colon still canonicalizes.""" + initial_subject = "Quarterly report" + initial_mime_id = "report.001@example.com" + initial_thread = factories.ThreadFactory(subject=initial_subject) + factories.ThreadAccessFactory( + mailbox=self.mailbox, + thread=initial_thread, + role=enums.ThreadAccessRoleChoices.EDITOR, + ) + factories.MessageFactory( + thread=initial_thread, mime_id=initial_mime_id, subject=initial_subject + ) + + parsed_reply = { + "subject": f"Re:{initial_subject}", + "references": [initial_mime_id], + "from": [{"email": "replier@a.com"}], + } + + found_thread = find_thread_for_message(parsed_reply, self.mailbox) + assert found_thread == initial_thread + @pytest.mark.parametrize( "role", [ @@ -192,7 +251,7 @@ class TestFindThread: "from": [{"email": "replier@a.com"}], } - found_thread = find_thread_for_inbound_message(parsed_reply, self.mailbox) + found_thread = find_thread_for_message(parsed_reply, self.mailbox) assert found_thread is None @pytest.mark.parametrize( @@ -235,7 +294,7 @@ class TestFindThread: } # Should find the thread in *our* mailbox - found_thread = find_thread_for_inbound_message(parsed_reply, self.mailbox) + found_thread = find_thread_for_message(parsed_reply, self.mailbox) assert found_thread == initial_thread @pytest.mark.parametrize( @@ -259,7 +318,7 @@ class TestFindThread: # No In-Reply-To or References "from": [{"email": "new@a.com"}], } - found_thread = find_thread_for_inbound_message(parsed_new_email, self.mailbox) + found_thread = find_thread_for_message(parsed_new_email, self.mailbox) assert found_thread is None @@ -292,7 +351,7 @@ class TestDeliverInboundMessage: domain = factories.MailDomainFactory(name="deliver.test") return factories.MailboxFactory(local_part="recipient", domain=domain) - @patch("core.mda.inbound_create.find_thread_for_inbound_message") + @patch("core.mda.inbound_create.find_thread_for_message") def test_basic_delivery_new_thread( self, mock_find_thread, target_mailbox, sample_parsed_email, raw_email_data ): @@ -412,7 +471,7 @@ class TestDeliverInboundMessage: enums.ThreadAccessRoleChoices.VIEWER, ], ) - @patch("core.mda.inbound_create.find_thread_for_inbound_message") + @patch("core.mda.inbound_create.find_thread_for_message") def test_basic_delivery_existing_thread( self, mock_find_thread, @@ -685,6 +744,47 @@ class TestDeliverInboundMessage: assert thread2.subject == subject # Make sure the original subject is kept assert message3.mime_id == parsed_email_3["messageId"][0] + def test_smtp_exchange_single_thread_with_rewritten_subject(self, target_mailbox): + """A reply whose subject was edited still lands in the same thread. + + Delivery path only (no import): this is the case that used to split a + conversation in two, since the subject was required to match even when + In-Reply-To pointed at a message we already held. + """ + recipient_addr = f"{target_mailbox.local_part}@{target_mailbox.domain.name}" + + def _raw(subject: str, mime_id: str, headers: bytes = b"") -> bytes: + return ( + b"From: sender@test.com\r\n" + b"To: " + recipient_addr.encode() + b"\r\n" + b"Subject: " + subject.encode() + b"\r\n" + b"Message-ID: <" + mime_id.encode() + b">\r\n" + headers + b"\r\nbody" + ) + + raw_1 = _raw("Initial topic", "exchange.1@example.com") + assert deliver_inbound_message(recipient_addr, parse_email(raw_1), raw_1) + + raw_2 = _raw( + "Re: Initial topic", + "exchange.2@example.com", + b"In-Reply-To: \r\n" + b"References: \r\n", + ) + assert deliver_inbound_message(recipient_addr, parse_email(raw_2), raw_2) + + # Third message: same reply chain, but the subject was rewritten. + raw_3 = _raw( + "Re: Renamed topic", + "exchange.3@example.com", + b"In-Reply-To: \r\n" + b"References: \r\n", + ) + assert deliver_inbound_message(recipient_addr, parse_email(raw_3), raw_3) + + threads = models.Thread.objects.filter(accesses__mailbox=target_mailbox) + assert threads.count() == 1 + assert threads.get().messages.count() == 3 + def test_deliver_message_with_empty_subject(self, target_mailbox, raw_email_data): """Test delivery of message with empty subject.""" recipient_addr = f"{target_mailbox.local_part}@{target_mailbox.domain.name}"