Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#13149 closed (fixed)

ForeignKeyRawIdWidget doesn't handle invalid values

Reported by: acdha Owned by: acdha
Component: contrib.admin Version: 1.2
Severity: Keywords: raw_id_fields, sprintSep2010
Cc: chris@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

#11465 added a check for values which do not exist but does not help in the case where a user or fuzzer enters a non-integer value into a raw id field. Ticket #9209 tracks the change to ModelChoiceField to add a validation error but ForeignKeyRawIdWidget.label_for_value will trigger another attempting to label the invalid value. The attached patch or http://github.com/acdha/django/commit/fb87e1a3bba33fd9fae7a0a0fe3a380d7b25d05c addresses this as well.

Attachments (2)

ForeignKeyRawIdWidget-invalid-value.patch (1021 bytes) - added by acdha 5 years ago.
Patch against Django 1.1.1
django-13149.patch (3.7 KB) - added by jbronn 4 years ago.

Download all attachments as: .zip

Change History (15)

Changed 5 years ago by acdha

Patch against Django 1.1.1

comment:1 Changed 5 years ago by russellm

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 years ago by acdha

  • Cc chris@… added

comment:3 Changed 5 years ago by petrikuittinen@…

Sorry for opening a new ticket (#13606). This is the same thing.
HOWEVER this problem still persists in Django-1.2.1. The above patch solves
the problem in Django-1.2-beta 1, but it still gives a ValueError in Django-1.2.1.
We need a better patch.

comment:4 Changed 5 years ago by petrikuittinen@…

  • Patch needs improvement set
  • Version changed from 1.1 to 1.2

Django 1.2+ needs to have patches in two files to get this issue fixed.

changes to django/forms/models.py from line 984 (catch ValueError):

    def to_python(self, value):
        if value in EMPTY_VALUES:
            return None
        try:
            key = self.to_field_name or 'pk'
            value = self.queryset.get(**{key: value})
        except (ValueError, self.queryset.model.DoesNotExist):
            raise ValidationError(self.error_messages['invalid_choice'])
        return value

And django/contrib/admin/widgets.py (catch ValueError as well):

    def label_for_value(self, value):
        key = self.rel.get_related_field().name
        try:
            obj = self.rel.to._default_manager.using(self.db).get(**{key:
value})
            return '&nbsp;<strong>%s</strong>' %
escape(truncate_words(obj, 14))
        except (ValueError, self.rel.to.DoesNotExist):
            return ''

Applying both of these patches seems to fix the issue. It would be nice to
have this fixed in the next Django 1.2.2 release.

comment:5 Changed 5 years ago by acdha

This problem appears to be fixed completely by Django 1.2.1. I can no longer reproduce errors by either using non-int values or non-existent primary keys. I recommend closing this ticket unless petrikuittinen@… or someone else can come up with a scenario which still reproduces.

comment:6 Changed 5 years ago by acdha

A similar problem happens if someone enters a non-ascii values in a raw id field:

@@ -169,7 +169,7 @@ class ManyToManyRawIdWidget(ForeignKeyRawIdWidget):
     def render(self, name, value, attrs=None):
         attrs['class'] = 'vManyToManyRawIdAdminField'
         if value:
-            value = ','.join([str(v) for v in value])
+            value = ','.join([unicode(v) for v in value])

comment:7 Changed 5 years ago by kmtracey

I do not believe this has been fixed. I still (current trunk) see the ValueError (e.g. "invalid literal for int() with base 10: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'") when entering a non-integer value in an integer PK raw-id field.

It would help to move this forward if the proposed patches were attached as diffs to the ticket rather than inline, and if they included tests.

comment:8 Changed 4 years ago by acdha

  • Keywords sprintSep2010 added
  • Owner changed from nobody to acdha

comment:9 Changed 4 years ago by acdha

  • Patch needs improvement unset

I confirmed my previous message was indeed in error (test environment had some workaround code enabled by mistake - obviously this needs real tests).

Here's a branch against 1.2.1:

http://github.com/acdha/django/compare/django-13149

and the complete patch:

http://github.com/acdha/django/compare/django-13149.patch

comment:11 Changed 4 years ago by mtredinnick

  • Triage Stage changed from Accepted to Ready for checkin

Yeah, I'll buy this. Looks good. Thanks.

Changed 4 years ago by jbronn

comment:12 Changed 4 years ago by jbronn

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

(In [13751]) Fixed #13149 -- The admin ForeignKeyRawIdWidget now properly handles non-integer values. Thanks, Chris Adams.

comment:13 Changed 4 years ago by jbronn

(In [13752]) [1.2.X] Fixed #13149 -- The admin ForeignKeyRawIdWidget now properly handles non-integer values. Thanks, Chris Adams.

Backport of r13751 from trunk.

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