Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#13149 closed (fixed)

ForeignKeyRawIdWidget doesn't handle invalid values

Reported by: Chris Adams Owned by: Chris Adams
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: no UI/UX: no

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 Chris Adams 14 years ago.
Patch against Django 1.1.1
django-13149.patch (3.7 KB ) - added by jbronn 14 years ago.

Download all attachments as: .zip

Change History (15)

by Chris Adams, 14 years ago

Patch against Django 1.1.1

comment:1 by Russell Keith-Magee, 14 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:2 by Chris Adams, 14 years ago

Cc: chris@… added

comment:3 by petrikuittinen@…, 14 years ago

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 by petrikuittinen@…, 14 years ago

Patch needs improvement: set
Version: 1.11.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 by Chris Adams, 14 years ago

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 by Chris Adams, 14 years ago

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 by Karen Tracey, 14 years ago

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 by Chris Adams, 14 years ago

Keywords: sprintSep2010 added
Owner: changed from nobody to Chris Adams

comment:9 by Chris Adams, 14 years ago

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 by Malcolm Tredinnick, 14 years ago

Triage Stage: AcceptedReady for checkin

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

by jbronn, 14 years ago

Attachment: django-13149.patch added

comment:12 by jbronn, 14 years ago

Resolution: fixed
Status: newclosed

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

comment:13 by jbronn, 14 years ago

(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