Opened 7 years ago

Closed 6 years ago

Last modified 4 years ago

#8746 closed (fixed)

Data entered in raw_id_fields needs better checking

Reported by: kmtracey Owned by: nobody
Component: contrib.admin Version: master
Severity: Keywords:
Cc: dgouldin@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I stumbled across this while trying to verify the patch for #8648. If you have a ForeignKey listed in raw_id_fields and enter an incorrect value in it, the admin code will likely thrown an exception. In the case I ran across in #8648 the incorrect value is an integer with no associated related object. The admin code attempts to re-display the form with an error message about the value being invalid, but the raw id widget in an attempt to be helpful and display the printable representation of the object referred to in the form generates an exception when it assumes it can get the associated object.

A 2nd way to cause a (different) exception is to enter something that isn't an integer at all. In this case I got:

Environment:

Request Method: POST
Request URL: http://lbox:8000/admin/crossword/clues/2518/
Django Version: 1.0-beta_2-SVN-8769
Python Version: 2.5.1
Installed Applications:
['django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.admin',
 'django.contrib.sites',
 'django.contrib.humanize',
 'xword.crossword']
Installed Middleware:
('django.middleware.common.CommonMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.middleware.doc.XViewMiddleware')


Traceback:
File "/home/kmt/tmp/django/trunk/django/core/handlers/base.py" in get_response
  86.                 response = callback(request, *callback_args, **callback_kwargs)
File "/home/kmt/tmp/django/trunk/django/contrib/admin/sites.py" in root
  173.                 return self.model_page(request, *url.split('/', 2))
File "/home/kmt/tmp/django/trunk/django/views/decorators/cache.py" in _wrapped_view_func
  44.         response = view_func(request, *args, **kwargs)
File "/home/kmt/tmp/django/trunk/django/contrib/admin/sites.py" in model_page
  192.         return admin_obj(request, rest_of_url)
File "/home/kmt/tmp/django/trunk/django/contrib/admin/options.py" in __call__
  196.             return self.change_view(request, unquote(url))
File "/home/kmt/tmp/django/trunk/django/db/transaction.py" in _commit_on_success
  238.                 res = func(*args, **kw)
File "/home/kmt/tmp/django/trunk/django/contrib/admin/options.py" in change_view
  571.             if form.is_valid():
File "/home/kmt/tmp/django/trunk/django/forms/forms.py" in is_valid
  120.         return self.is_bound and not bool(self.errors)
File "/home/kmt/tmp/django/trunk/django/forms/forms.py" in _get_errors
  111.             self.full_clean()
File "/home/kmt/tmp/django/trunk/django/forms/forms.py" in full_clean
  218.                     value = field.clean(value)
File "/home/kmt/tmp/django/trunk/django/forms/models.py" in clean
  527.             value = self.queryset.get(pk=value)
File "/home/kmt/tmp/django/trunk/django/db/models/query.py" in get
  295.         clone = self.filter(*args, **kwargs)
File "/home/kmt/tmp/django/trunk/django/db/models/query.py" in filter
  481.         return self._filter_or_exclude(False, *args, **kwargs)
File "/home/kmt/tmp/django/trunk/django/db/models/query.py" in _filter_or_exclude
  499.             clone.query.add_q(Q(*args, **kwargs))
File "/home/kmt/tmp/django/trunk/django/db/models/sql/query.py" in add_q
  1191.                             can_reuse=used_aliases)
File "/home/kmt/tmp/django/trunk/django/db/models/sql/query.py" in add_filter
  1135.         self.where.add((alias, col, field, lookup_type, value), connector)
File "/home/kmt/tmp/django/trunk/django/db/models/sql/where.py" in add
  48.                 params = field.get_db_prep_lookup(lookup_type, value)
File "/home/kmt/tmp/django/trunk/django/db/models/fields/__init__.py" in get_db_prep_lookup
  202.             return [self.get_db_prep_value(value)]
File "/home/kmt/tmp/django/trunk/django/db/models/fields/__init__.py" in get_db_prep_value
  337.         return int(value)

Exception Type: ValueError at /admin/crossword/clues/2518/
Exception Value: invalid literal for int() with base 10: 'akdakdf'

Not sure whether this should be one or multiple tickets, but I'll start with one under the assumption that "making admin safe from bad input placed in a raw id widget" can be viewed as one task, even if bad input today can lead to different errors depending on what flavor of 'bad' you feed to admin.

Specifically not marking this 1.0 since an easy workaround is to just not enter invalid values in these fields. Might be nice to fix it post-1.0, though, to save people from surprising results due to typos and such.

Attachments (2)

raw_id_field_more_tolerant.diff (546 bytes) - added by jfw 7 years ago.
8746.diff (1.6 KB) - added by dgouldin 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by slowik

  • Component changed from django.contrib.admin to Forms
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

This bug is not case of admin site but form validating. I receive sort of this when I try submit ModelForm with choices to integer field like this

birth_begin=models.PositiveSmallIntegerField('',choices=gen_birth_choices(),null=True)

with no choice of birth

error:

/python/django/core/handlers/base.py in get_response 
...
                response = callback(request, *callback_args, **callback_kwargs) ...
▶ Local vars 
/python/django/contrib/auth/decorators.py in __call__ 
...
            return self.view_func(request, *args, **kwargs) ...
▶ Local vars 
/apps/users/views.py in search_for 
...
                form.save() ...
▶ Local vars 
/python/django/forms/models.py in save 
...
        return save_instance(self, self.instance, self._meta.fields, fail_message, commit) ...
▶ Local vars 
/python/django/forms/models.py in save_instance 
...
        instance.save() ...
▶ Local vars 
/python/django/db/models/base.py in save 
...
        self.save_base(force_insert=force_insert, force_update=force_update) ...
▶ Local vars 
/python/django/db/models/base.py in save_base 
...
                    values = [(f, None, f.get_db_prep_save(raw and getattr(self, f.attname) or f.pre_save(self, False))) for f in non_pks] ...
▶ Local vars 
/python/django/db/models/fields/__init__.py in get_db_prep_save 
...
        return self.get_db_prep_value(value) ...
▶ Local vars 
/python/django/db/models/fields/__init__.py in get_db_prep_value 
...
        return int(value) 
Exception Type:	ValueError
Exception Value:	invalid literal for int() with base 10: ''
Exception Location:	/python/django/db/models/fields/__init__.py in get_db_prep_value, line 671
Python Executable:	/usr/bin/python
Python Version:	2.5.2

comment:2 Changed 7 years ago by kmtracey

  • Component changed from Forms to django.contrib.admin

I am not convinced you are running into the same problem this ticket was opened to resolve. You don't give enough details of what you are doing, but the error you are seeing could be the result of having blank=True but not null=True on your model field. django-users would be a better place to try to resolve what you are seeing, this ticket was really opened to address what I believe is specifically an admin problem, not a general forms validation problem.

comment:3 Changed 7 years ago by adroffne@…

I just discovered this raw_id_fields bug and also believe it is lacking Form validation. The raw-id field should raise a ValidationError when the contents is not the right data type.

I have a ManyToMany model that has a through model. An Inline admin class links the through model to the parent model. In my case, the primary key is declared to be a CharField, a US ZIP code, not a plain integer key.

The raw_id_fields fix must determine the primary key datatype of the model, type(instance.pk), first. Then, its Form field raises ValidationError when TypeError is thrown.

comment:4 Changed 7 years ago by kmtracey

#9484 was a dup.

comment:5 Changed 7 years ago by jfw

I had a similar issue with raw_id_fields: if users type into the field, sometimes they'll leave extra commas or spaces. Extra spaces are fine, though should be stripped--extra commas makes the query die by trying to convert " " or "" to an int. Attached is a patch to at least fix that. I agree that it should have better checking as to whether or not the values are valid for the particular key, but I'm not sure (yet) how to do that without assuming that the key is always an integer.

Changed 7 years ago by jfw

comment:6 Changed 7 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:7 Changed 7 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:8 Changed 7 years ago by jacob

  • milestone set to 1.1

Changed 6 years ago by dgouldin

comment:9 Changed 6 years ago by dgouldin

  • Cc dgouldin@… added
  • Has patch set

comment:10 Changed 6 years ago by jacob

  • Triage Stage changed from Accepted to Ready for checkin

comment:11 Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from new to closed

(In [10233]) Fixed #8746: Check data in raw_id_fields more closely. Thanks, dgouldin

comment:12 Changed 6 years ago by silviogutierrez

  • Resolution fixed deleted
  • Status changed from closed to reopened

Seems like only M2M fields were fixed. Someone submitted #9484 reporting this, but the ticket was closed and linked to this fix. ForeignKey fields with raw_id_fields enabled will produce an exception if anything but an int, or no value, is entered.

Error:

Django Version: 1.1 beta 1 SVN-10934

Traceback:
File "/var/lib/python-support/python2.6/django/core/handlers/base.py" in get_response
  92.                 response = callback(request, *callback_args, **callback_kwargs)
File "/var/lib/python-support/python2.6/django/contrib/admin/sites.py" in root
  480.                 return self.model_page(request, *url.split('/', 2))
File "/var/lib/python-support/python2.6/django/views/decorators/cache.py" in _wrapped_view_func
  44.         response = view_func(request, *args, **kwargs)
File "/var/lib/python-support/python2.6/django/contrib/admin/sites.py" in model_page
  499.         return admin_obj(request, rest_of_url)
File "/var/lib/python-support/python2.6/django/contrib/admin/options.py" in __call__
  1088.             return self.add_view(request)
File "/var/lib/python-support/python2.6/django/db/transaction.py" in _commit_on_success
  240.                 res = func(*args, **kw)
File "/var/lib/python-support/python2.6/django/contrib/admin/options.py" in add_view
  715.             if form.is_valid():
File "/var/lib/python-support/python2.6/django/forms/forms.py" in is_valid
  120.         return self.is_bound and not bool(self.errors)
File "/var/lib/python-support/python2.6/django/forms/forms.py" in _get_errors
  111.             self.full_clean()
File "/var/lib/python-support/python2.6/django/forms/forms.py" in full_clean
  240.                     value = field.clean(value)
File "/var/lib/python-support/python2.6/django/forms/models.py" in clean
  981.             value = self.queryset.get(**{key: value})
File "/var/lib/python-support/python2.6/django/db/models/query.py" in get
  299.         clone = self.filter(*args, **kwargs)
File "/var/lib/python-support/python2.6/django/db/models/query.py" in filter
  498.         return self._filter_or_exclude(False, *args, **kwargs)
File "/var/lib/python-support/python2.6/django/db/models/query.py" in _filter_or_exclude
  516.             clone.query.add_q(Q(*args, **kwargs))
File "/var/lib/python-support/python2.6/django/db/models/sql/query.py" in add_q
  1675.                             can_reuse=used_aliases)
File "/var/lib/python-support/python2.6/django/db/models/sql/query.py" in add_filter
  1614.                 connector)
File "/var/lib/python-support/python2.6/django/db/models/sql/where.py" in add
  56.                 obj, params = obj.process(lookup_type, value)
File "/var/lib/python-support/python2.6/django/db/models/sql/where.py" in process
  269.                 params = self.field.get_db_prep_lookup(lookup_type, value)
File "/var/lib/python-support/python2.6/django/db/models/fields/__init__.py" in get_db_prep_lookup
  210.             return [self.get_db_prep_value(value)]
File "/var/lib/python-support/python2.6/django/db/models/fields/__init__.py" in get_db_prep_value
  361.         return int(value)

Exception Type: ValueError at /admin/articles/video/add/
Exception Value: invalid literal for int() with base 10: 'adfas'

comment:13 Changed 6 years ago by Alex

  • Resolution set to fixed
  • Status changed from reopened to closed

Please file a new ticket for this.

comment:14 Changed 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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