Code

Opened 22 months ago

Last modified 20 months 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

Attachments (0)

Change History (5)

comment:1 Changed 22 months 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 22 months 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 20 months 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 20 months ago by Adam DePrince <deprince@…>

  • Cc deprince@… added

comment:5 Changed 20 months 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from deprince to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.