[Django] #18543: Non image file can be saved to ImageField
Django
----------------------------------------------+--------------------
Reporter: johnsmith | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.4
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/18543>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by niwi):
* cc: niwi@… (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I tried to reproduce the bug with this test:
{{{
#!python
@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__'
----------------------------------------------------------------------
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18543#comment:1>
Django
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by claudep):
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18543#comment:2>
Django
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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/" ?
--
Ticket URL: <https://code.djangoproject.com/ticket/18543#comment:3>
Django
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: deprince@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/18543#comment:4>
Django
Reporter: johnsmith | Owner: deprince
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by Adam DePrince <deprince@…>):
* owner: nobody => deprince
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18543#comment:5>
Django
Reporter: johnsmith | Owner: (none)
Type: Bug | Status: assigned
Component: Database layer | Version: 1.4
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: deprince => (none)
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/18543#comment:6>
Django
Reporter: johnsmith | Owner: (none)
Component: Database layer | Version: 1.4
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/18543#comment:7>
Django
Reporter: johnsmith | Owner: (none)
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Comment (by jburns6789):
I would like to work on this, it is my first attempt at a bug fix.
I will need extra time, Im going to replicate the bug, come up with a
proposal
and communicate my ideas.
--
Ticket URL: <https://code.djangoproject.com/ticket/18543#comment:8>
Django
Reporter: johnsmith | Owner:
| jburns6789
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by jburns6789):
* owner: (none) => jburns6789
Ticket URL: <https://code.djangoproject.com/ticket/18543#comment:9>
Django
Reporter: johnsmith | Owner:
| jburns6789
Type: Bug | Status: assigned
Component: Database layer | Version: 1.4
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Comment (by jburns6789):
Hi, from what I can deduce from the previous comments, In this context,
the Django image.field is used to validate the uploaded image file and no
other third party libraries. The fix in this case would be to have the
error message refer users to the ImageField documentation needing pillow
to be installed?
--
Ticket URL: <https://code.djangoproject.com/ticket/18543#comment:10>
Django
Reporter: johnsmith | Owner: (none)
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by jburns6789):
* owner: jburns6789 => (none)
Ticket URL: <https://code.djangoproject.com/ticket/18543#comment:11>