From 62a0611ddb9fea821724928f8f5979f04042115c Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Thu, 11 Jul 2024 18:46:33 +0000 Subject: [PATCH] emoji: Pass down content-type, rather than guessing from extension. --- zerver/actions/realm_emoji.py | 4 ++-- zerver/lib/transfer.py | 11 ++++++++++- zerver/lib/upload/__init__.py | 17 +++++++++++------ zerver/tests/test_audit_log.py | 6 +++++- zerver/tests/test_events.py | 4 +++- zerver/tests/test_import_export.py | 14 +++++++++++--- zerver/tests/test_realm_emoji.py | 13 +++++++++++-- zerver/tests/test_transfer.py | 8 ++++++-- zerver/tests/test_upload_local.py | 6 +++--- zerver/tests/test_upload_s3.py | 2 +- zerver/views/realm_emoji.py | 4 +++- zilencer/management/commands/populate_db.py | 4 +++- 12 files changed, 69 insertions(+), 24 deletions(-) diff --git a/zerver/actions/realm_emoji.py b/zerver/actions/realm_emoji.py index e94109dfb8..e860e1f837 100644 --- a/zerver/actions/realm_emoji.py +++ b/zerver/actions/realm_emoji.py @@ -21,7 +21,7 @@ def notify_realm_emoji(realm: Realm, realm_emoji: Dict[str, EmojiInfo]) -> None: def check_add_realm_emoji( - realm: Realm, name: str, author: UserProfile, image_file: IO[bytes] + realm: Realm, name: str, author: UserProfile, image_file: IO[bytes], content_type: str ) -> RealmEmoji: try: realm_emoji = RealmEmoji(realm=realm, name=name, author=author) @@ -36,7 +36,7 @@ def check_add_realm_emoji( emoji_uploaded_successfully = False is_animated = False try: - is_animated = upload_emoji_image(image_file, emoji_file_name, author) + is_animated = upload_emoji_image(image_file, emoji_file_name, author, content_type) emoji_uploaded_successfully = True finally: if not emoji_uploaded_successfully: diff --git a/zerver/lib/transfer.py b/zerver/lib/transfer.py index 60b8e65005..600639cf48 100644 --- a/zerver/lib/transfer.py +++ b/zerver/lib/transfer.py @@ -10,6 +10,7 @@ from django.db import connection from zerver.lib.avatar_hash import user_avatar_path from zerver.lib.mime_types import guess_type +from zerver.lib.thumbnail import BadImageError from zerver.lib.upload import upload_emoji_image, write_avatar_images from zerver.lib.upload.s3 import S3UploadBackend, upload_image_to_s3 from zerver.models import Attachment, RealmEmoji, UserProfile @@ -113,13 +114,21 @@ def _transfer_emoji_to_s3(realm_emoji: RealmEmoji) -> None: ) assert settings.LOCAL_UPLOADS_DIR is not None assert settings.LOCAL_AVATARS_DIR is not None + content_type = guess_type(emoji_path)[0] emoji_path = os.path.join(settings.LOCAL_AVATARS_DIR, emoji_path) + ".original" + if content_type is None: # nocoverage + logging.error("Emoji %d has no recognizable file extension", realm_emoji.id) + return try: with open(emoji_path, "rb") as f: - upload_emoji_image(f, realm_emoji.file_name, realm_emoji.author, backend=s3backend) + upload_emoji_image( + f, realm_emoji.file_name, realm_emoji.author, content_type, backend=s3backend + ) logging.info("Uploaded emoji file in path %s", emoji_path) except FileNotFoundError: # nocoverage logging.error("Emoji %d could not be loaded from local disk", realm_emoji.id) + except BadImageError as e: + logging.error("Emoji %d is invalid: %s", realm_emoji.id, e) def transfer_emoji_to_s3(processes: int) -> None: diff --git a/zerver/lib/upload/__init__.py b/zerver/lib/upload/__init__.py index 857baed141..2004a5295d 100644 --- a/zerver/lib/upload/__init__.py +++ b/zerver/lib/upload/__init__.py @@ -334,11 +334,12 @@ def upload_emoji_image( emoji_file: IO[bytes], emoji_file_name: str, user_profile: UserProfile, + content_type: str, backend: Optional[ZulipUploadBackend] = None, ) -> bool: if backend is None: backend = upload_backend - content_type = guess_type(emoji_file_name)[0] + emoji_path = RealmEmoji.PATH_ID_TEMPLATE.format( realm_id=user_profile.realm_id, emoji_file_name=emoji_file_name, @@ -368,21 +369,21 @@ def upload_emoji_image( def get_emoji_file_content( session: OutgoingSession, emoji_url: str, emoji_id: int, logger: logging.Logger -) -> bytes: # nocoverage +) -> Tuple[bytes, str]: # nocoverage original_emoji_url = emoji_url + ".original" logger.info("Downloading %s", original_emoji_url) response = session.get(original_emoji_url) if response.status_code == 200: assert isinstance(response.content, bytes) - return response.content + return response.content, response.headers["Content-Type"] logger.info("Error fetching emoji from URL %s", original_emoji_url) logger.info("Trying %s instead", emoji_url) response = session.get(emoji_url) if response.status_code == 200: assert isinstance(response.content, bytes) - return response.content + return response.content, response.headers["Content-Type"] logger.info("Error fetching emoji from URL %s", emoji_url) logger.error("Could not fetch emoji %s", emoji_id) raise AssertionError(f"Could not fetch emoji {emoji_id}") @@ -403,7 +404,9 @@ def handle_reupload_emojis_event(realm: Realm, logger: logging.Logger) -> None: if emoji_url.startswith("/"): emoji_url = urljoin(realm_emoji.realm.url, emoji_url) - emoji_file_content = get_emoji_file_content(session, emoji_url, realm_emoji.id, logger) + emoji_file_content, content_type = get_emoji_file_content( + session, emoji_url, realm_emoji.id, logger + ) emoji_bytes_io = io.BytesIO(emoji_file_content) @@ -412,7 +415,9 @@ def handle_reupload_emojis_event(realm: Realm, logger: logging.Logger) -> None: assert user_profile is not None logger.info("Reuploading emoji %s", realm_emoji.id) - realm_emoji.is_animated = upload_emoji_image(emoji_bytes_io, emoji_filename, user_profile) + realm_emoji.is_animated = upload_emoji_image( + emoji_bytes_io, emoji_filename, user_profile, content_type + ) realm_emoji.save(update_fields=["is_animated"]) diff --git a/zerver/tests/test_audit_log.py b/zerver/tests/test_audit_log.py index 11bedf7f17..1f92f3a7e5 100644 --- a/zerver/tests/test_audit_log.py +++ b/zerver/tests/test_audit_log.py @@ -1029,7 +1029,11 @@ class TestRealmAuditLog(ZulipTestCase): # check in upload_emoji, we need to make this request via # that helper rather than via the API. realm_emoji = check_add_realm_emoji( - realm=user.realm, name="test_emoji", author=user, image_file=img_file + realm=user.realm, + name="test_emoji", + author=user, + image_file=img_file, + content_type="image/png", ) added_emoji = EmojiInfo( diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 4c3bd8939a..f10d908bfb 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -2665,7 +2665,9 @@ class NormalActionsTest(BaseAction): author = self.example_user("iago") with get_test_image_file("img.png") as img_file: with self.verify_action() as events: - check_add_realm_emoji(self.user_profile.realm, "my_emoji", author, img_file) + check_add_realm_emoji( + self.user_profile.realm, "my_emoji", author, img_file, "image/png" + ) check_realm_emoji_update("events[0]", events[0]) diff --git a/zerver/tests/test_import_export.py b/zerver/tests/test_import_export.py index a9bd1345d0..3e15ffc4f6 100644 --- a/zerver/tests/test_import_export.py +++ b/zerver/tests/test_import_export.py @@ -181,7 +181,7 @@ class ExportFile(ZulipTestCase): realm = user_profile.realm with get_test_image_file("img.png") as img_file: - check_add_realm_emoji(realm, emoji_name, user_profile, img_file) + check_add_realm_emoji(realm, emoji_name, user_profile, img_file, "image/png") def upload_files_for_realm(self, user_profile: UserProfile) -> None: realm = user_profile.realm @@ -775,7 +775,11 @@ class RealmImportExportTest(ExportFile): with get_test_image_file("img.png") as img_file: realm_emoji = check_add_realm_emoji( - realm=hamlet.realm, name="hawaii", author=hamlet, image_file=img_file + realm=hamlet.realm, + name="hawaii", + author=hamlet, + image_file=img_file, + content_type="image/png", ) self.assertEqual(realm_emoji.name, "hawaii") @@ -931,7 +935,11 @@ class RealmImportExportTest(ExportFile): # to test that upon import that gets fixed. with get_test_image_file("img.png") as img_file: new_realm_emoji = check_add_realm_emoji( - realm=hamlet.realm, name="hawaii2", author=hamlet, image_file=img_file + realm=hamlet.realm, + name="hawaii2", + author=hamlet, + image_file=img_file, + content_type="image/png", ) assert new_realm_emoji is not None original_realm_emoji_count = RealmEmoji.objects.count() diff --git a/zerver/tests/test_realm_emoji.py b/zerver/tests/test_realm_emoji.py index 01a7aab44b..e60f84401d 100644 --- a/zerver/tests/test_realm_emoji.py +++ b/zerver/tests/test_realm_emoji.py @@ -17,7 +17,11 @@ class RealmEmojiTest(ZulipTestCase): def create_test_emoji(self, name: str, author: UserProfile) -> RealmEmoji: with get_test_image_file("img.png") as img_file: realm_emoji = check_add_realm_emoji( - realm=author.realm, name=name, author=author, image_file=img_file + realm=author.realm, + name=name, + author=author, + image_file=img_file, + content_type="image/png", ) if realm_emoji is None: raise Exception("Error creating test emoji.") # nocoverage @@ -395,7 +399,11 @@ class RealmEmojiTest(ZulipTestCase): # check in upload_emoji, we need to make this request via # that helper rather than via the API. check_add_realm_emoji( - realm=emoji_author.realm, name=emoji_name, author=emoji_author, image_file=img_file + realm=emoji_author.realm, + name=emoji_name, + author=emoji_author, + image_file=img_file, + content_type="image/png", ) with self.assertRaises(JsonableError): check_add_realm_emoji( @@ -403,4 +411,5 @@ class RealmEmojiTest(ZulipTestCase): name=emoji_name, author=emoji_author, image_file=img_file, + content_type="image/png", ) diff --git a/zerver/tests/test_transfer.py b/zerver/tests/test_transfer.py index 93bfec9913..d673a52208 100644 --- a/zerver/tests/test_transfer.py +++ b/zerver/tests/test_transfer.py @@ -88,7 +88,9 @@ class TransferUploadsToS3Test(ZulipTestCase): emoji_name = "emoji.png" with get_test_image_file("img.png") as image_file: - emoji = check_add_realm_emoji(othello.realm, emoji_name, othello, image_file) + emoji = check_add_realm_emoji( + othello.realm, emoji_name, othello, image_file, "image/png" + ) emoji_path = RealmEmoji.PATH_ID_TEMPLATE.format( realm_id=othello.realm_id, @@ -112,7 +114,9 @@ class TransferUploadsToS3Test(ZulipTestCase): emoji_name = "emoji2.png" with get_test_image_file("animated_img.gif") as image_file: - emoji = check_add_realm_emoji(othello.realm, emoji_name, othello, image_file) + emoji = check_add_realm_emoji( + othello.realm, emoji_name, othello, image_file, "image/gif" + ) emoji_path = RealmEmoji.PATH_ID_TEMPLATE.format( realm_id=othello.realm_id, diff --git a/zerver/tests/test_upload_local.py b/zerver/tests/test_upload_local.py index 8c711a33aa..ccf2451cc8 100644 --- a/zerver/tests/test_upload_local.py +++ b/zerver/tests/test_upload_local.py @@ -174,7 +174,7 @@ class LocalStorageTest(UploadSerializeMixin, ZulipTestCase): file_name = "emoji.png" with get_test_image_file("img.png") as image_file: - upload_emoji_image(image_file, file_name, user_profile) + upload_emoji_image(image_file, file_name, user_profile, "image/png") url = zerver.lib.upload.upload_backend.get_emoji_url(file_name, user_profile.realm_id) emoji_path = RealmEmoji.PATH_ID_TEMPLATE.format( @@ -186,7 +186,7 @@ class LocalStorageTest(UploadSerializeMixin, ZulipTestCase): file_name = "emoji.gif" with get_test_image_file("animated_img.gif") as image_file: - upload_emoji_image(image_file, file_name, user_profile) + upload_emoji_image(image_file, file_name, user_profile, "image/png") url = zerver.lib.upload.upload_backend.get_emoji_url(file_name, user_profile.realm_id) still_url = zerver.lib.upload.upload_backend.get_emoji_url( file_name, user_profile.realm_id, still=True @@ -211,7 +211,7 @@ class LocalStorageTest(UploadSerializeMixin, ZulipTestCase): file_name = "emoji.png" with get_test_image_file("img.png") as image_file: - upload_emoji_image(image_file, file_name, user_profile) + upload_emoji_image(image_file, file_name, user_profile, "image/png") emoji_path = RealmEmoji.PATH_ID_TEMPLATE.format( realm_id=user_profile.realm_id, diff --git a/zerver/tests/test_upload_s3.py b/zerver/tests/test_upload_s3.py index 9aa22b0f65..e45ead1aed 100644 --- a/zerver/tests/test_upload_s3.py +++ b/zerver/tests/test_upload_s3.py @@ -478,7 +478,7 @@ class S3Test(ZulipTestCase): user_profile = self.example_user("hamlet") emoji_name = "emoji.png" with get_test_image_file("img.png") as image_file: - zerver.lib.upload.upload_emoji_image(image_file, emoji_name, user_profile) + zerver.lib.upload.upload_emoji_image(image_file, emoji_name, user_profile, "image/png") emoji_path = RealmEmoji.PATH_ID_TEMPLATE.format( realm_id=user_profile.realm_id, diff --git a/zerver/views/realm_emoji.py b/zerver/views/realm_emoji.py index 95faab4b74..be99e9c066 100644 --- a/zerver/views/realm_emoji.py +++ b/zerver/views/realm_emoji.py @@ -9,6 +9,7 @@ from zerver.lib.emoji import check_remove_custom_emoji, check_valid_emoji_name, from zerver.lib.exceptions import JsonableError, ResourceNotFoundError from zerver.lib.request import REQ, has_request_variables from zerver.lib.response import json_success +from zerver.lib.upload import get_file_info from zerver.models import RealmEmoji, UserProfile from zerver.models.realm_emoji import get_all_custom_emoji_for_realm @@ -51,7 +52,8 @@ def upload_emoji( ) ) - check_add_realm_emoji(user_profile.realm, emoji_name, user_profile, emoji_file) + _filename, content_type = get_file_info(emoji_file) + check_add_realm_emoji(user_profile.realm, emoji_name, user_profile, emoji_file, content_type) return json_success(request) diff --git a/zilencer/management/commands/populate_db.py b/zilencer/management/commands/populate_db.py index 14df8a66f8..8838f49b1e 100644 --- a/zilencer/management/commands/populate_db.py +++ b/zilencer/management/commands/populate_db.py @@ -913,7 +913,9 @@ class Command(ZulipBaseCommand): # Create a test realm emoji. IMAGE_FILE_PATH = static_path("images/test-images/checkbox.png") with open(IMAGE_FILE_PATH, "rb") as fp: - check_add_realm_emoji(zulip_realm, "green_tick", iago, File(fp, name="checkbox.png")) + check_add_realm_emoji( + zulip_realm, "green_tick", iago, File(fp, name="checkbox.png"), "image/png" + ) if not options["test_suite"]: # Populate users with some bar data