Opened 8 years ago

Closed 8 years ago

Last modified 5 years ago

#6967 closed (fixed)

ModelForms doesn't validate CHOICES

Reported by: johannes.spielmann@… Owned by: Jacob
Component: Forms Version: master
Severity: Keywords:
Cc: simon@…, carl@…, ville@…, pat.j.anderson@…, web@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

A ModelForm with a CharField, that has CHOICES set, presents to the user only the specified CHOICES. But it does not _validate_ against the CHOICES, which allows a crafted response to write arbitrary values to the field.

Attachments (3)

choices-fix.patch (601 bytes) - added by Simon Willison 8 years ago.
6967-tests.diff (1.4 KB) - added by Matt McClanahan 8 years ago.
Some tests for choices on CharField and IntegerField.
6967-r8772-tests.diff (604 bytes) - added by Matt McClanahan 8 years ago.
New failing test

Download all attachments as: .zip

Change History (23)

Changed 8 years ago by Simon Willison

Attachment: choices-fix.patch added

comment:1 Changed 8 years ago by Simon Willison

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Attached patch appears to fix this, but needs tests.

comment:2 Changed 8 years ago by anonymous

Cc: simon@… added

comment:3 Changed 8 years ago by edgarsj

Triage Stage: UnreviewedAccepted

comment:4 Changed 8 years ago by Matt McClanahan

milestone: 1.0

Changed 8 years ago by Matt McClanahan

Attachment: 6967-tests.diff added

Some tests for choices on CharField and IntegerField.

comment:5 Changed 8 years ago by Jacob

Owner: changed from nobody to Jacob
Status: newassigned

comment:6 Changed 8 years ago by Jacob

(In [8771]) Added a TypedChoiceField which acts just like ChoiceField, except that it
returns a value coerced by some provided function. Refs #6967.

comment:7 Changed 8 years ago by Jacob

Resolution: fixed
Status: assignedclosed

(In [8772]) Fixed #6967: ModelForms now validate choices. Thanks, mattmcc -- the failing test helped quite a bit.

comment:8 Changed 8 years ago by Matt McClanahan

Resolution: fixed
Status: closedreopened

The test was insufficient, unfortunately. Field.formfield is going to need to cope with other model field types that set arbitrary arguments in their defaults dict which would make sense for the form field they expect to use, but won't make sense to TypedChoiceField. The only built-in examples of this are PositiveIntegerField and PositiveSmallIntegerField, which pass a min_value argument.

We could pop off min_value as max_length is being popped now, but it's just fixing the symptom. A custom model field could set anything.

Changed 8 years ago by Matt McClanahan

Attachment: 6967-r8772-tests.diff added

New failing test

comment:9 Changed 8 years ago by Chris Beaven

I fell across this just now too. For now I just changed my local copy of the pop() line to:

kwargs = kwargs.get('choices_kwargs', {})

Still not the nicest solution, but it isn't as bad as the current one.

comment:10 Changed 8 years ago by Antti Kaihola

Has patch: set

comment:11 Changed 8 years ago by Carl Meyer

Cc: carl@… added

comment:12 Changed 8 years ago by James Bennett

#8744 was a duplicate.

comment:13 Changed 8 years ago by Ville Säävuori

Cc: ville@… added

comment:14 Changed 8 years ago by anonymous

Cc: pat.j.anderson@… added

comment:15 Changed 8 years ago by anonymous

Cc: web@… added

comment:16 Changed 8 years ago by Jacob

BTW, in the future please open a new ticket for things of this nature.

comment:17 Changed 8 years ago by Jacob

Resolution: fixed
Status: reopenedclosed

(In [8806]) Repaired an oversight from [8772] that let made certain types of fields with choices fail. Fixes #6967 again.

comment:18 Changed 7 years ago by Stavros Korokithakis

I'm getting this bug as well (running latest trunk as of two minutes ago). Here is my traceback:

Validating models...
Unhandled exception in thread started by <function inner_run at 0x6aef70>
Traceback (most recent call last):
  File "/Library/Python/2.5/site-packages/django/core/management/commands/runserver.py", line 48, in inner_run
    self.validate(display_num_errors=True)
  File "/Library/Python/2.5/site-packages/django/core/management/base.py", line 249, in validate
    num_errors = get_validation_errors(s, app)
  File "/Library/Python/2.5/site-packages/django/core/management/validation.py", line 28, in get_validation_errors
    for (app_name, error) in get_app_errors().items():
  File "/Library/Python/2.5/site-packages/django/db/models/loading.py", line 131, in get_app_errors
    self._populate()
  File "/Library/Python/2.5/site-packages/django/db/models/loading.py", line 58, in _populate
    self.load_app(app_name, True)
  File "/Library/Python/2.5/site-packages/django/db/models/loading.py", line 74, in load_app
    models = import_module('.models', app_name)
  File "/Library/Python/2.5/site-packages/django/utils/importlib.py", line 35, in import_module
    __import__(name)
  File "/Users/stavros/Code/Django/head/main/models.py", line 215, in <module>
    class AdType(models.Model):
  File "/Users/stavros/Code/Django/head/main/models.py", line 221, in AdType
    price = models.DecimalField(_("price"), max_digits=6, decimal_places=2, min_value=0)
  File "/Library/Python/2.5/site-packages/django/db/models/fields/__init__.py", line 589, in __init__
    Field.__init__(self, verbose_name, name, **kwargs)
TypeError: __init__() got an unexpected keyword argument 'min_value'

I'm not sure what this means, but I can't launch my server.

comment:19 Changed 7 years ago by Stavros Korokithakis

I'm sorry, disregard my post above. I shouldn't post bug reports while sleepless...

comment:20 Changed 5 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

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