Opened 12 years ago
Closed 12 years ago
#20084 closed Bug (fixed)
Formsets should sign/verify max_num
Reported by: | Jacob | Owned by: | andrewsg |
---|---|---|---|
Component: | Forms | Version: | 1.5 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Originally reported in 2011 by Miloslav Pojman:
The problem is that formsets accept its max_num from data submitted by the user and ignore a value set in the code. It means that user can bypass any formset max_num check. For example: a user has paid for two persons so I will offer him formsets with max_num=2 in order to make an order. If he tampers the form data he can send orders for any number of persons. In case of model formsets it means that any number of orders will be saved to a database despite the max_num value.
We should sign and verify max_num.
Change History (6)
comment:1 by , 12 years ago
follow-up: 4 comment:2 by , 12 years ago
It looks like not only is max_num not trustworthy, but it's also not checked during form validation/cleaning at all. Nor is the default maximum of 1000. It only affects the output of the form and not the response. So, step one is to implement checking of max_num (as opposed to just total) and then to sign it so it's also trustworthy.
comment:3 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
follow-up: 5 comment:4 by , 12 years ago
Replying to anonymous:
It looks like not only is max_num not trustworthy, but it's also not checked during form validation/cleaning at all. Nor is the default maximum of 1000. It only affects the output of the form and not the response. So, step one is to implement checking of max_num (as opposed to just total) and then to sign it so it's also trustworthy.
To be clear, max_num
is not checked during form validation/cleaning, but absolute_max
(which is the higher value of 1000 or max_num
) is an absolute maximum for the number of forms created, during either form display or validation (see _construct_forms
).
This doesn't change the conclusion, though - this ticket does require both signing max_num
so it can be trusted, and checking it during validation.
IIRC from when I looked into this earlier, there's currently some odd inconsistency in behavior between BaseModelFormSet
and BaseFormSet
in terms of how max_num
is handled, which hopefully can be resolved by the fix for this ticket.
comment:5 by , 12 years ago
Replying to carljm:
Replying to anonymous:
It looks like not only is max_num not trustworthy, but it's also not checked during form validation/cleaning at all. Nor is the default maximum of 1000. It only affects the output of the form and not the response. So, step one is to implement checking of max_num (as opposed to just total) and then to sign it so it's also trustworthy.
To be clear,
max_num
is not checked during form validation/cleaning, butabsolute_max
(which is the higher value of 1000 ormax_num
) is an absolute maximum for the number of forms created, during either form display or validation (see_construct_forms
).
This doesn't change the conclusion, though - this ticket does require both signing
max_num
so it can be trusted, and checking it during validation.
IIRC from when I looked into this earlier, there's currently some odd inconsistency in behavior between
BaseModelFormSet
andBaseFormSet
in terms of howmax_num
is handled, which hopefully can be resolved by the fix for this ticket.
I have been testing BaseFormSet in various permutations and with various sets of forged POST data, and I haven't been able to get the formset to limit the number of created objects to the defined max_num or the default max_num. absolute_max does seem to affect the number of forms rendered on the page, but it doesn't seem to affect the number of objects returned from the formset.cleaned_data property even if the POST data is forged to contain a very large number.
(Edit: I talked to carljm in person here at pycon and discovered that the absolute_max limit of 1000 is actually working, but only in that it throws an IndexError. I had observed that earlier but thought the IndexError was due to something else. Ideally the 500 error result would be improved upon in any patch for this issue.)
comment:6 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Just a thought while Andrew (who's looking into this) was explaining this ticket to me at PyCon.
Javascript in the client might want to read max_num. So if signing makes it unreadable, this could break the client code.