Opened 3 years ago

Last modified 3 years ago

#18543 new Bug

Non image file can be saved to ImageField

Reported by: johnsmith Owned by: deprince
Component: Database layer (models, ORM) Version: 1.4
Severity: Normal Keywords:
Cc: niwi@…, deprince@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

https://docs.djangoproject.com/en/dev/ref/models/fields/#imagefield

Inherits all attributes and methods from FileField, but also validates that the uploaded object is a valid image.

I was trying to find out the exact exception that would be raised if a file is not an image and it appears that no such image validation is done. So I did a test, with a model such as this

class Image(models.Model):
    caption = models.Charfield(max_length=60)
    image = models.ImageField(upload_to='somewhere')

I tried this

f = django.core.files.base.ContentFile('not an image')
i = myapp.models.Image()
i.caption("This should throw an error but doesn't")
i.image.save('bar.jpg', f)
i.save()

I don't get any errors and the file is saved. Maybe I'm doing something wrong but from what the docs say I'm expecting to be prevented from doing this.

From pip freeze
Django==1.4
PIL==1.1.7

Change History (5)

comment:1 Changed 3 years ago by niwi

  • Cc niwi@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I tried to reproduce the bug with this test:

@skipUnless(test_images, "PIL not installed")
def test_model_image_field(self):
    from django.core.files.base import ContentFile
    f = ContentFile(b'not an image')
    with open(os.path.join(os.path.dirname(__file__), "test.png"), 'rb') as fp:
        image_data = fp.read()

    instance = ImageFile()
    instance.image.save("foo.png", SimpleUploadedFile("foo.png", image_data))

    instance2 = ImageFile()
    instance2.image.save("foo.png", f)

And I get is this:

======================================================================
ERROR: test_model_image_field (modeltests.model_forms.tests.OldFormForXTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/niwi/django/tests/modeltests/model_forms/tests.py", line 1250, in test_model_image_field
    instance2.image.save("foo.png", f)
  File "../django/db/models/fields/files.py", line 348, in save
    super(ImageFieldFile, self).save(name, content, save)
  File "../django/db/models/fields/files.py", line 89, in save
    setattr(self.instance, self.field.name, self.name)
  File "../django/db/models/fields/files.py", line 334, in __set__
    self.field.update_dimension_fields(instance, force=True)
  File "../django/db/models/fields/files.py", line 412, in update_dimension_fields
    width = file.width
  File "../django/core/files/images.py", line 15, in _get_width
    return self._get_image_dimensions()[0]
TypeError: 'NoneType' object has no attribute '__getitem__'

----------------------------------------------------------------------

comment:2 Changed 3 years ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

Currently, the verification that the content is an image is done by the ImageField form field (see its to_python method), not in the model field.

Accepted on the base that we should at least:

  • Improve the error message as shown in the previous comment.
  • Clarify the documentation

Validating that the file is really an image file at the model level might be something to evaluate, but with great care, because of the performance issue involved by this operation. Opinions/design decision needed here.

comment:3 Changed 3 years ago by Adam DePrince <deprince@…>

What constitutes a valid image will depend on how the image is processed --- Imagemagick, netpnm and PIL all disagree slightly on the definition of "an image". We should also consider what it means to "validate" the image --- basic magic number checking will catch the common cases where a user uploads something decidedly not an image, like an mp3 file, but we should stress in our documentation that just because the image passed our "validation" does not mean what was uploaded is actually an image or even safe to process.

What if ImageField accepted anything for which "file -b --mime-type" responds with a match to "$image/" ?

comment:4 Changed 3 years ago by Adam DePrince <deprince@…>

  • Cc deprince@… added

comment:5 Changed 3 years ago by Adam DePrince <deprince@…>

  • Owner changed from nobody to deprince

A quick survey shows that the python options for file seem to be lib magic based. I don't think we really want to see yet another c dependencey in django so I'll spend the rest of my djangocon 2012 sprint time banging out a pure python port.

Note: See TracTickets for help on using tickets.
Back to Top