add protection for image uploads (#7295)

* add more protection for image uploads

* gracefully handle file-size errors in GroupProfileAdmin

* create validate_avatar_size utility function

Author: escattone

kitsune/gallery/tests/test_models.py

@@ -8,7 +8,7 @@
 from kitsune.upload.tasks import generate_thumbnail
 
 
-@patch("kitsune.upload.tasks.create_image_thumbnail")
+@patch("kitsune.upload.utils.create_image_thumbnail")
 class ImageTestCase(TestCase):
     def tearDown(self):
         Image.objects.all().delete()

kitsune/groups/admin.py

@@ -5,11 +5,19 @@
 from treebeard.forms import movenodeform_factory
 
 from kitsune.groups.models import GroupProfile
-from kitsune.upload.tasks import create_image_thumbnail
+from kitsune.upload.utils import create_image_thumbnail, validate_avatar_size
+
+
+class GroupProfileAdminForm(movenodeform_factory(GroupProfile)):  # type: ignore[misc]
+    def clean_avatar(self):
+        avatar = self.cleaned_data.get("avatar")
+        if avatar and hasattr(avatar, "size"):
+            validate_avatar_size(avatar)
+        return avatar
 
 
 class GroupProfileAdmin(TreeAdmin):
-    form = movenodeform_factory(GroupProfile)
+    form = GroupProfileAdminForm
     list_display = ["slug", "group", "visibility", "depth", "numchild"]
     list_filter = ["visibility"]
     raw_id_fields = ["leaders"]

kitsune/groups/forms.py

@@ -7,7 +7,7 @@
 from kitsune.sumo.form_fields import MultiUsernameField
 from kitsune.sumo.widgets import ImageWidget
 from kitsune.upload.forms import LimitedImageField
-from kitsune.upload.utils import FileTooLargeError, check_file_size
+from kitsune.upload.utils import validate_avatar_size
 
 
 class GroupProfileForm(forms.ModelForm):
@@ -41,11 +41,7 @@ def clean_avatar(self):
             avatar = self.request.FILES.get("avatar")
             if not avatar:
                 raise forms.ValidationError(_("You have not selected an image to upload."))
-            # Validate file size
-            try:
-                check_file_size(avatar, settings.MAX_AVATAR_FILE_SIZE)
-            except FileTooLargeError as e:
-                raise forms.ValidationError(e.args[0])
+            validate_avatar_size(avatar)
 
         return self.cleaned_data["avatar"]
 

kitsune/groups/tests/test_admin.py

@@ -1,11 +1,15 @@
+from unittest import mock
+
 from django import forms
 from django.contrib.admin.sites import AdminSite
 from django.contrib.auth.models import Group
+from django.core.files.uploadedfile import SimpleUploadedFile
 from django.test import RequestFactory
 
-from kitsune.groups.admin import GroupProfileAdmin
+from kitsune.groups.admin import GroupProfileAdmin, GroupProfileAdminForm
 from kitsune.groups.models import GroupProfile
 from kitsune.sumo.tests import TestCase
+from kitsune.upload.utils import FileTooLargeError
 from kitsune.users.tests import UserFactory
 
 
@@ -94,3 +98,41 @@ def test_subgroup_can_have_no_leaders(self):
         self.admin.save_related(mock_request, mock_form, [], change=True)
 
         self.assertEqual(sub_profile.leaders.count(), 0)
+
+    @mock.patch("kitsune.upload.utils.open_as_pil_image")
+    def test_avatar_too_large_pixels(self, mock_open):
+        """Uploading an avatar that exceeds pixel limits shows a form validation error."""
+        mock_open.side_effect = FileTooLargeError("Image exceeds the maximum allowed size.")
+
+        with open("kitsune/upload/tests/media/test.jpg", "rb") as f:
+            avatar = SimpleUploadedFile("test.jpg", f.read(), content_type="image/jpeg")
+        group = Group.objects.create(name="Avatar Test Group")
+        profile = GroupProfile.add_root(group=group, slug="avatar-test-group")
+
+        form = GroupProfileAdminForm(
+            data={"group": group.pk, "slug": "avatar-test-group"},
+            files={"avatar": avatar},
+            instance=profile,
+        )
+        self.assertFalse(form.is_valid())
+        self.assertIn("avatar", form.errors)
+        self.assertIn("Image exceeds the maximum allowed size.", form.errors["avatar"][0])
+
+    @mock.patch("kitsune.upload.utils.check_file_size")
+    def test_avatar_too_large_file_size(self, mock_check):
+        """Uploading an avatar that exceeds file size limits shows a form validation error."""
+        mock_check.side_effect = FileTooLargeError("File is too large.")
+
+        with open("kitsune/upload/tests/media/test.jpg", "rb") as f:
+            avatar = SimpleUploadedFile("test.jpg", f.read(), content_type="image/jpeg")
+        group = Group.objects.create(name="Size Test Group")
+        profile = GroupProfile.add_root(group=group, slug="size-test-group")
+
+        form = GroupProfileAdminForm(
+            data={"group": group.pk, "slug": "size-test-group"},
+            files={"avatar": avatar},
+            instance=profile,
+        )
+        self.assertFalse(form.is_valid())
+        self.assertIn("avatar", form.errors)
+        self.assertIn("File is too large.", form.errors["avatar"][0])

kitsune/groups/tests/test_views.py

@@ -5,7 +5,7 @@
 from django.contrib.auth.models import Group
 from django.core.files import File
 from django.http import HttpResponse
-from django.test import RequestFactory
+from django.test import RequestFactory, override_settings
 from pyquery import PyQuery as pq
 
 from kitsune.groups.models import GroupProfile
@@ -127,6 +127,14 @@ def test_upload_avatar(self):
         self.assertEqual(url, r["location"])
         assert not os.path.exists(old_path), "Old avatar was not removed."
 
+    @override_settings(IMAGE_MAX_PIXELS=1)
+    def test_upload_avatar_too_large(self):
+        """Uploading an avatar that exceeds IMAGE_MAX_PIXELS fails form validation."""
+        url = reverse("groups.edit_avatar", locale="en-US", args=[self.group_profile.slug])
+        with open("kitsune/upload/tests/media/test.jpg", "rb") as f:
+            r = self.client.post(url, {"avatar": f})
+        self.assertEqual(200, r.status_code)
+
     def test_delete_avatar(self):
         """Delete a group avatar."""
         self.test_upload_avatar()

kitsune/groups/views.py

@@ -15,7 +15,7 @@
 from kitsune.groups.models import GroupProfile
 from kitsune.sumo.urlresolvers import reverse
 from kitsune.sumo.utils import get_next_url, paginate
-from kitsune.upload.tasks import create_image_thumbnail
+from kitsune.upload.utils import create_image_thumbnail
 
 
 def _remove_group_member(profile, user, request, remove_from_group=True):

kitsune/settings.py

@@ -840,6 +840,7 @@ def JINJA_CONFIG():
 WIKI_VIDEO_HEIGHT = 480
 
 IMAGE_MAX_FILESIZE = 10485760  # 10 megabytes, in bytes
+IMAGE_MAX_PIXELS = 20_000_000  # 20 megapixels
 THUMBNAIL_SIZE = 120  # Thumbnail size, in pixels
 THUMBNAIL_UPLOAD_PATH = "uploads/images/thumbnails/"
 IMAGE_UPLOAD_PATH = "uploads/images/"

kitsune/upload/tasks.py

@@ -1,4 +1,3 @@
-import io
 import logging
 import os
 import subprocess
@@ -9,7 +8,6 @@
 from django.conf import settings
 from django.core.files.base import ContentFile
 from django.core.files.storage import default_storage
-from PIL import Image
 
 log = logging.getLogger("k.task")
 
@@ -23,6 +21,7 @@ def generate_thumbnail(
     optionally the "max_size" of its longest side. The model name must be in the form of
     "<app>.<model-name>", so for example, "gallery.Image" or "upload.ImageAttachment".
     """
+    from kitsune.upload.utils import create_image_thumbnail
 
     # Get the model of the object, and then get the object itself.
     model = apps.get_model(obj_model_name)
@@ -51,37 +50,6 @@ def generate_thumbnail(
     obj.update(**{to_field: to_.name})
 
 
-def create_image_thumbnail(fileobj, longest_side=settings.THUMBNAIL_SIZE, pad=False):
-    """
-    Returns a thumbnail file with a set longest side.
-    """
-    original_image = Image.open(fileobj)
-    original_image = original_image.convert("RGBA")
-    file_width, file_height = original_image.size
-
-    width, height = _scale_dimensions(file_width, file_height, longest_side)
-    resized_image = original_image.resize((width, height), Image.LANCZOS)
-
-    data = io.BytesIO()
-
-    if pad:
-        padded_image = _make_image_square(resized_image, longest_side)
-        padded_image.save(data, "PNG")
-    else:
-        resized_image.save(data, "PNG")
-
-    return ContentFile(data.getvalue())
-
-
-def _make_image_square(source_image, side=settings.THUMBNAIL_SIZE):
-    """Pads a rectangular image with transparency to make it square."""
-    square_image = Image.new("RGBA", (side, side), (255, 255, 255, 0))
-    width = (side - source_image.size[0]) // 2
-    height = (side - source_image.size[1]) // 2
-    square_image.paste(source_image, (width, height))
-    return square_image
-
-
 def _scale_dimensions(width, height, longest_side=settings.THUMBNAIL_SIZE):
     """
     Returns a tuple (width, height), both smaller than longest side, and

kitsune/upload/tests/__init__.py

@@ -1,12 +1,9 @@
 import factory
-from django.conf import settings
-from django.core.files import File
 
 from kitsune.questions.tests import QuestionFactory
 from kitsune.sumo.tests import TestCase
 from kitsune.upload.models import ImageAttachment
 from kitsune.upload.storage import RenameFileStorage
-from kitsune.upload.utils import FileTooLargeError, check_file_size, create_imageattachment
 from kitsune.users.tests import UserFactory
 
 
@@ -32,103 +29,3 @@ def check_file_info(file_info, name, width, height, delete_url, url, thumbnail_u
 def get_file_name(name):
     storage = RenameFileStorage()
     return storage.get_available_name(name)
-
-
-class CheckFileSizeTestCase(TestCase):
-    """Tests for check_file_size"""
-
-    def test_check_file_size_under(self):
-        """No exception should be raised"""
-        with open("kitsune/upload/tests/media/test.jpg", "rb") as f:
-            up_file = File(f)
-            check_file_size(up_file, settings.IMAGE_MAX_FILESIZE)
-
-    def test_check_file_size_over(self):
-        """FileTooLargeError should be raised"""
-        with self.assertRaises(FileTooLargeError):
-            with open("kitsune/upload/tests/media/test.jpg", "rb") as f:
-                up_file = File(f)
-                # This should raise
-                check_file_size(up_file, 0)
-
-
-class CreateImageAttachmentTestCase(TestCase):
-    def setUp(self):
-        super().setUp()
-        self.user = UserFactory()
-        self.obj = QuestionFactory()
-
-    def tearDown(self):
-        ImageAttachment.objects.all().delete()
-        super().tearDown()
-
-    def test_create_imageattachment(self):
-        """
-        An image attachment is created from an uploaded file.
-
-        Verifies all appropriate fields are correctly set.
-        """
-        with open("kitsune/upload/tests/media/test.jpg", "rb") as f:
-            up_file = File(f)
-            file_info = create_imageattachment({"image": up_file}, self.user, self.obj)
-
-        image = ImageAttachment.objects.all()[0]
-        check_file_info(
-            file_info,
-            name="test.png",
-            width=90,
-            height=120,
-            delete_url=image.get_delete_url(),
-            url=image.get_absolute_url(),
-            thumbnail_url=image.thumbnail.url,
-        )
-
-    def test_create_imageattachment_when_animated(self):
-        """
-        An image attachment is created from an uploaded animated GIF file.
-
-        Verifies all appropriate fields are correctly set.
-        """
-        filepath = "kitsune/upload/tests/media/animated.gif"
-        with open(filepath, "rb") as f:
-            up_file = File(f)
-            file_info = create_imageattachment({"image": up_file}, self.user, self.obj)
-
-        image = ImageAttachment.objects.all()[0]
-        check_file_info(
-            file_info,
-            name=filepath,
-            width=120,
-            height=120,
-            delete_url=image.get_delete_url(),
-            url=image.get_absolute_url(),
-            thumbnail_url=image.thumbnail.url,
-        )
-
-
-class FileNameTestCase(TestCase):
-    def _match_file_name(self, name, name_end):
-        assert name.endswith(name_end), '"{}" does not end with "{}"'.format(name, name_end)
-
-    def test_empty_file_name(self):
-        self._match_file_name("", "")
-
-    def test_empty_file_name_with_extension(self):
-        self._match_file_name(get_file_name(".wtf"), "3f8242")
-
-    def test_ascii(self):
-        self._match_file_name(get_file_name("some ascii.jpg"), "5959e0.jpg")
-
-    def test_ascii_dir(self):
-        self._match_file_name(get_file_name("dir1/dir2/some ascii.jpg"), "5959e0.jpg")
-
-    def test_low_unicode(self):
-        self._match_file_name(get_file_name("157d9383e6aeba7180378fd8c1d46f80.gif"), "bdaf1a.gif")
-
-    def test_high_unicode(self):
-        self._match_file_name(get_file_name("\u6709\u52b9.jpeg"), "ce1518.jpeg")
-
-    def test_full_mixed(self):
-        self._match_file_name(
-            get_file_name("123\xe5\xe5\xee\xe9\xf8\xe7\u6709\u52b9.png"), "686c11.png"
-        )

kitsune/upload/tests/test_tasks.py

@@ -13,9 +13,9 @@
 from kitsune.upload.tasks import (
     _scale_dimensions,
     compress_image,
-    create_image_thumbnail,
     generate_thumbnail,
 )
+from kitsune.upload.utils import FileTooLargeError, create_image_thumbnail
 from kitsune.users.tests import UserFactory
 
 
@@ -74,6 +74,12 @@ def test_create_image_thumbnail_default(self):
         self.assertEqual(expected_thumb.width, actual_thumb.width)
         self.assertEqual(expected_thumb.height, actual_thumb.height)
 
+    @override_settings(IMAGE_MAX_PIXELS=1)
+    def test_image_too_large(self):
+        """create_image_thumbnail raises FileTooLargeError for an oversized image."""
+        with self.assertRaises(FileTooLargeError):
+            create_image_thumbnail("kitsune/upload/tests/media/test.jpg")
+
     def test_create_image_thumbnail_avatar(self):
         """An avatar is created from an image file."""
         thumb_content = create_image_thumbnail(