Prevent uploads of non images for user photo and collection icon

src/olympia/amo/tests/images/non-image.png

@@ -0,0 +1 @@
+this is not an image.

src/olympia/bandwagon/forms.py

@@ -10,7 +10,8 @@ from django_statsd.clients import statsd
 import olympia.core.logger
 
 from olympia import amo
-from olympia.amo.utils import clean_nl, has_links, slug_validator, slugify
+from olympia.amo.utils import (
+    clean_nl, has_links, ImageCheck, slug_validator, slugify)
 from olympia.lib import happyforms
 from olympia.translations.widgets import (
     TranslationTextarea, TranslationTextInput)
@@ -195,10 +196,15 @@ class CollectionForm(happyforms.ModelForm):
         icon = self.cleaned_data['icon']
         if not icon:
             return
-        if icon.content_type not in ('image/png', 'image/jpeg'):
+        icon_check = ImageCheck(icon)
+        if (icon.content_type not in ('image/png', 'image/jpeg') or
+                not icon_check.is_image()):
             raise forms.ValidationError(
                 ugettext('Icons must be either PNG or JPG.'))
 
+        if icon_check.is_animated():
+            raise forms.ValidationError(ugettext('Icons cannot be animated.'))
+
         if icon.size > settings.MAX_ICON_UPLOAD_SIZE:
             size_in_mb = settings.MAX_ICON_UPLOAD_SIZE / 1024 / 1024 - 1
             raise forms.ValidationError(
@@ -206,32 +212,33 @@ class CollectionForm(happyforms.ModelForm):
         return icon
 
     def save(self, default_locale=None):
-        c = super(CollectionForm, self).save(commit=False)
+        collection = super(CollectionForm, self).save(commit=False)
-        c.author = self.initial['author']
+        collection.author = self.initial['author']
-        c.application = self.initial['application']
+        collection.application = self.initial['application']
         icon = self.cleaned_data.get('icon')
 
         if default_locale:
-            c.default_locale = default_locale
+            collection.default_locale = default_locale
 
         if icon:
-            c.icontype = 'image/png'
+            collection.icontype = 'image/png'
 
-        c.save()
+        collection.save()
 
         if icon:
-            dirname = c.get_img_dir()
+            dirname = collection.get_img_dir()
 
-            destination = os.path.join(dirname, '%d.png' % c.id)
+            destination = os.path.join(dirname, '%d.png' % collection.id)
-            tmp_destination = os.path.join(dirname,
+            tmp_destination = os.path.join(
-                                           '%d.png__unconverted' % c.id)
+                dirname, '%d.png__unconverted' % collection.id)
+            icon.seek(0)
             with storage.open(tmp_destination, 'w') as fh:
                 for chunk in icon.chunks():
                     fh.write(chunk)
             tasks.resize_icon.delay(tmp_destination, destination,
-                                    set_modified_on=[c])
+                                    set_modified_on=[collection])
 
-        return c
+        return collection
 
     class Meta:
         model = Collection

src/olympia/bandwagon/tests/test_views.py

@@ -1183,6 +1183,49 @@ class TestCollectionForm(TestCase):
         form.save()
         assert update_mock.called
 
+    def test_icon_invalid_though_content_type_is_correct(self):
+        collection = Collection.objects.get(pk=57181)
+        # This file is not an image at all, but we'll try to upload it with an
+        # image mime type. It should not work.
+        fake_image = get_uploaded_file('non-image.png')
+        assert fake_image.content_type == 'image/png'
+        form = forms.CollectionForm(
+            {'listed': collection.listed,
+             'slug': collection.slug,
+             'name': collection.name},
+            instance=collection,
+            files={'icon': fake_image},
+            initial={'author': collection.author,
+                     'application': collection.application})
+        assert not form.is_valid()
+        assert form.errors == {'icon': [u'Icons must be either PNG or JPG.']}
+
+    def test_icon_invalid_gif(self):
+        collection = Collection.objects.get(pk=57181)
+        form = forms.CollectionForm(
+            {'listed': collection.listed,
+             'slug': collection.slug,
+             'name': collection.name},
+            instance=collection,
+            files={'icon': get_uploaded_file('animated.gif')},
+            initial={'author': collection.author,
+                     'application': collection.application})
+        assert not form.is_valid()
+        assert form.errors == {'icon': [u'Icons must be either PNG or JPG.']}
+
+    def test_icon_invalid_animated(self):
+        collection = Collection.objects.get(pk=57181)
+        form = forms.CollectionForm(
+            {'listed': collection.listed,
+             'slug': collection.slug,
+             'name': collection.name},
+            instance=collection,
+            files={'icon': get_uploaded_file('animated.png')},
+            initial={'author': collection.author,
+                     'application': collection.application})
+        assert not form.is_valid()
+        assert form.errors == {'icon': [u'Icons cannot be animated.']}
+
     def test_denied_name(self):
         form = forms.CollectionForm()
         form.cleaned_data = {'name': 'IE6Fan'}

src/olympia/devhub/views.py

@@ -1007,8 +1007,8 @@ def ajax_upload_image(request, upload_type, addon_id=None):
         is_persona = upload_type.startswith('persona_')
 
         check = amo_utils.ImageCheck(upload_preview)
-        if (not check.is_image() or
+        if (upload_preview.content_type not in amo.IMG_TYPES or
-                upload_preview.content_type not in amo.IMG_TYPES):
+                not check.is_image()):
             if is_icon:
                 errors.append(ugettext('Icons must be either PNG or JPG.'))
             else:

src/olympia/users/forms.py

@@ -12,7 +12,8 @@ from olympia import amo
 from olympia.accounts.views import fxa_error_message
 from olympia.activity.models import ActivityLog
 from olympia.amo.fields import HttpHttpsOnlyURLField
-from olympia.amo.utils import clean_nl, has_links, slug_validator
+from olympia.amo.utils import (
+    clean_nl, has_links, ImageCheck, slug_validator)
 from olympia.lib import happyforms
 from olympia.users import notifications
 
@@ -176,10 +177,15 @@ class UserEditForm(happyforms.ModelForm):
         if not photo:
             return
 
-        if photo.content_type not in ('image/png', 'image/jpeg'):
+        image_check = ImageCheck(photo)
+        if (photo.content_type not in ('image/png', 'image/jpeg') or
+                not image_check.is_image()):
             raise forms.ValidationError(
                 ugettext('Images must be either PNG or JPG.'))
 
+        if image_check.is_animated():
+            raise forms.ValidationError(ugettext('Images cannot be animated.'))
+
         if photo.size > settings.MAX_PHOTO_UPLOAD_SIZE:
             msg = ugettext('Please use images smaller than %dMB.')
             size_in_mb = settings.MAX_PHOTO_UPLOAD_SIZE / 1024 / 1024 - 1

src/olympia/users/tests/test_forms.py

@@ -115,18 +115,53 @@ class TestUserEditForm(UserFormBase):
 
     @patch('olympia.amo.models.ModelBase.update')
     def test_photo_modified(self, update_mock):
-        dummy = Mock()
+        request = Mock()
-        dummy.user = self.user
+        request.user = self.user
-
         data = {'username': self.user_profile.username,
                 'email': self.user_profile.email}
         files = {'photo': get_uploaded_file('transparent.png')}
         form = UserEditForm(data, files=files, instance=self.user_profile,
-                            request=dummy)
+                            request=request)
         assert form.is_valid()
         form.save()
         assert update_mock.called
 
+    def test_photo_invalid_though_content_type_is_correct(self):
+        request = Mock()
+        request.user = self.user
+        data = {'username': self.user_profile.username,
+                'email': self.user_profile.email}
+        files = {'photo': get_uploaded_file('non-image.png')}
+        form = UserEditForm(data, files=files, instance=self.user_profile,
+                            request=request)
+
+        assert not form.is_valid()
+        assert form.errors == {'photo': [u'Images must be either PNG or JPG.']}
+
+    def test_photo_invalid_gif(self):
+        request = Mock()
+        request.user = self.user
+        data = {'username': self.user_profile.username,
+                'email': self.user_profile.email}
+        files = {'photo': get_uploaded_file('animated.gif')}
+        form = UserEditForm(data, files=files, instance=self.user_profile,
+                            request=request)
+
+        assert not form.is_valid()
+        assert form.errors == {'photo': [u'Images must be either PNG or JPG.']}
+
+    def test_photo_invalid_animated(self):
+        request = Mock()
+        request.user = self.user
+        data = {'username': self.user_profile.username,
+                'email': self.user_profile.email}
+        files = {'photo': get_uploaded_file('animated.png')}
+        form = UserEditForm(data, files=files, instance=self.user_profile,
+                            request=request)
+
+        assert not form.is_valid()
+        assert form.errors == {'photo': [u'Images cannot be animated.']}
+
     def test_cannot_change_email(self):
         self.user.update(fxa_id='1a2b3c', email='me@example.com')
         form = UserEditForm(