From 23f4cde91cbd99de245b7ed1890f052bde33d1c6 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Tue, 24 Jan 2023 15:45:49 -0500 Subject: [PATCH] email_mirror: Ensure that attachments get space to be included. The content of a message is truncated to `MAX_MESSAGE_LENGTH`, which is 1000 characters. Since the email gateway places attachments at the very end of the extracted body, that means that they are the first thing to get truncated off. That is, if an incoming email message contains 1000 `a`s and an image attachment, the link that attaches the attachment to the message will get truncated off, leaving it dangling in the database. Truncate the message body content separately from the attachment links which are included at the end of the body. --- zerver/lib/email_mirror.py | 23 +++++++--- zerver/tests/test_email_mirror.py | 70 ++++++++++++++++++++++++++----- 2 files changed, 78 insertions(+), 15 deletions(-) diff --git a/zerver/lib/email_mirror.py b/zerver/lib/email_mirror.py index ab19d0becb..6c6ba5974e 100644 --- a/zerver/lib/email_mirror.py +++ b/zerver/lib/email_mirror.py @@ -21,7 +21,7 @@ from zerver.lib.email_mirror_helpers import ( ) from zerver.lib.email_notifications import convert_html_to_markdown from zerver.lib.exceptions import JsonableError, RateLimitedError -from zerver.lib.message import normalize_body, truncate_topic +from zerver.lib.message import normalize_body, truncate_content, truncate_topic from zerver.lib.queue import queue_json_publish from zerver.lib.rate_limiter import RateLimitedObject from zerver.lib.send_email import FromAddress @@ -147,7 +147,9 @@ def create_missed_message_address(user_profile: UserProfile, message: Message) - return FromAddress.NOREPLY mm_address = MissedMessageEmailAddress.objects.create( - message=message, user_profile=user_profile, email_token=generate_missed_message_token() + message=message, + user_profile=user_profile, + email_token=generate_missed_message_token(), ) return str(mm_address) @@ -170,15 +172,26 @@ def construct_zulip_body( if not body.endswith("\n"): body += "\n" - body += extract_and_upload_attachments(message, realm, sender) if not body.rstrip(): body = "(No email body)" + preamble = "" if show_sender: from_address = str(message.get("From", "")) - body = f"From: {from_address}\n{body}" + preamble = f"From: {from_address}\n" - return body + postamble = extract_and_upload_attachments(message, realm, sender) + if postamble != "": + postamble = "\n" + postamble + + # Truncate the content ourselves, to ensure that the attachments + # all make it into the body-as-posted + body = truncate_content( + body, + settings.MAX_MESSAGE_LENGTH - len(preamble) - len(postamble), + "\n[message truncated]", + ) + return preamble + body + postamble ## Sending the Zulip ## diff --git a/zerver/tests/test_email_mirror.py b/zerver/tests/test_email_mirror.py index 1bcb6578df..1751f84d8b 100644 --- a/zerver/tests/test_email_mirror.py +++ b/zerver/tests/test_email_mirror.py @@ -334,7 +334,9 @@ class TestStreamEmailMessagesSuccess(ZulipTestCase): self.assertEqual(get_display_recipient(message.recipient), stream.name) self.assertEqual(message.topic_name(), "(no topic)") - def test_receive_stream_email_messages_subject_with_nonprintable_chars(self) -> None: + def test_receive_stream_email_messages_subject_with_nonprintable_chars( + self, + ) -> None: user_profile = self.example_user("hamlet") self.login_user(user_profile) self.subscribe(user_profile, "Denmark") @@ -600,7 +602,8 @@ class TestEmailMirrorMessagesWithAttachments(ZulipTestCase): incoming_valid_message["Reply-to"] = self.example_email("othello") with mock.patch( - "zerver.lib.email_mirror.upload_message_file", return_value="https://test_url" + "zerver.lib.email_mirror.upload_message_file", + return_value="https://test_url", ) as upload_message_file: process_message(incoming_valid_message) upload_message_file.assert_called_with( @@ -613,7 +616,7 @@ class TestEmailMirrorMessagesWithAttachments(ZulipTestCase): ) message = most_recent_message(user_profile) - self.assertEqual(message.content, "Test body\n[image.png](https://test_url)") + self.assertEqual(message.content, "Test body\n\n[image.png](https://test_url)") def test_message_with_valid_attachment_model_attributes_set_correctly(self) -> None: """ @@ -656,6 +659,48 @@ class TestEmailMirrorMessagesWithAttachments(ZulipTestCase): self.assertEqual(attachment.realm, stream.realm) self.assertEqual(attachment.is_realm_public, True) + def test_message_with_attachment_long_body(self) -> None: + user_profile = self.example_user("hamlet") + self.login_user(user_profile) + self.subscribe(user_profile, "Denmark") + stream = get_stream("Denmark", user_profile.realm) + stream_to_address = encode_email_address(stream) + + incoming_valid_message = EmailMessage() + incoming_valid_message.set_content("a" * settings.MAX_MESSAGE_LENGTH) + with open( + os.path.join(settings.DEPLOY_ROOT, "static/images/default-avatar.png"), "rb" + ) as f: + image_bytes = f.read() + + incoming_valid_message.add_attachment( + image_bytes, + maintype="image", + subtype="png", + filename="image.png", + ) + + incoming_valid_message["Subject"] = "TestStreamEmailMessages subject" + incoming_valid_message["From"] = self.example_email("hamlet") + incoming_valid_message["To"] = stream_to_address + incoming_valid_message["Reply-to"] = self.example_email("othello") + + process_message(incoming_valid_message) + + message = most_recent_message(user_profile) + attachment = Attachment.objects.last() + assert attachment is not None + self.assertEqual(list(attachment.messages.values_list("id", flat=True)), [message.id]) + self.assertEqual( + message.sender, get_system_bot(settings.EMAIL_GATEWAY_BOT, stream.realm_id) + ) + self.assertEqual(attachment.realm, stream.realm) + self.assertEqual(attachment.is_realm_public, True) + + assert message.content.endswith( + f"aaaaaa\n[message truncated]\n[image.png](/user_uploads/{attachment.path_id})" + ) + def test_message_with_attachment_utf8_filename(self) -> None: user_profile = self.example_user("hamlet") self.login_user(user_profile) @@ -684,7 +729,8 @@ class TestEmailMirrorMessagesWithAttachments(ZulipTestCase): incoming_valid_message["Reply-to"] = self.example_email("othello") with mock.patch( - "zerver.lib.email_mirror.upload_message_file", return_value="https://test_url" + "zerver.lib.email_mirror.upload_message_file", + return_value="https://test_url", ) as upload_message_file: process_message(incoming_valid_message) upload_message_file.assert_called_with( @@ -697,7 +743,7 @@ class TestEmailMirrorMessagesWithAttachments(ZulipTestCase): ) message = most_recent_message(user_profile) - self.assertEqual(message.content, f"Test body\n[{utf8_filename}](https://test_url)") + self.assertEqual(message.content, f"Test body\n\n[{utf8_filename}](https://test_url)") def test_message_with_valid_nested_attachment(self) -> None: user_profile = self.example_user("hamlet") @@ -730,7 +776,8 @@ class TestEmailMirrorMessagesWithAttachments(ZulipTestCase): incoming_valid_message["Reply-to"] = self.example_email("othello") with mock.patch( - "zerver.lib.email_mirror.upload_message_file", return_value="https://test_url" + "zerver.lib.email_mirror.upload_message_file", + return_value="https://test_url", ) as upload_message_file: process_message(incoming_valid_message) upload_message_file.assert_called_with( @@ -743,7 +790,7 @@ class TestEmailMirrorMessagesWithAttachments(ZulipTestCase): ) message = most_recent_message(user_profile) - self.assertEqual(message.content, "Test body\n[image.png](https://test_url)") + self.assertEqual(message.content, "Test body\n\n[image.png](https://test_url)") def test_message_with_invalid_attachment(self) -> None: user_profile = self.example_user("hamlet") @@ -894,7 +941,8 @@ class TestStreamEmailMessagesEmptyBody(ZulipTestCase): with self.assertLogs(logger_name, level="INFO") as m: process_message(incoming_valid_message) self.assertEqual( - m.output, [f"INFO:{logger_name}:Email has no nonempty body sections; ignoring."] + m.output, + [f"INFO:{logger_name}:Email has no nonempty body sections; ignoring."], ) def test_receive_stream_email_messages_no_textual_body(self) -> None: @@ -1134,7 +1182,8 @@ class TestMissedMessageEmailMessages(ZulipTestCase): "Error sending message to stream announce via message notification email reply:\nOnly organization administrators can send to this stream.", ) self.assertEqual( - message.sender, get_system_bot(settings.NOTIFICATION_BOT, user_profile.realm_id) + message.sender, + get_system_bot(settings.NOTIFICATION_BOT, user_profile.realm_id), ) def test_missed_stream_message_email_response_tracks_topic_change(self) -> None: @@ -1488,7 +1537,8 @@ class TestEmailMirrorTornadoView(ZulipTestCase): MirrorWorker().consume(event) self.assertEqual( - self.get_last_message().content, "This is a plain-text message for testing Zulip." + self.get_last_message().content, + "This is a plain-text message for testing Zulip.", ) post_data = {