Code

#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.

Attachments (0)

Change History (6)

comment:1 Changed 16 months ago by tobych

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.

comment:2 follow-up: Changed 16 months ago by 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.

comment:3 Changed 16 months ago by andrewsg

  • Owner changed from nobody to andrewsg
  • Status changed from new to assigned

comment:4 in reply to: ↑ 2 ; follow-up: Changed 16 months ago by 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, 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.

Last edited 16 months ago by carljm (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 16 months ago by andrewsg

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, 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.

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.)

Last edited 16 months ago by andrewsg (previous) (diff)

comment:6 Changed 16 months ago by Carl Meyer <carl@…>

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

In f9ab543720532400e8b0d490cdbe67ea09ae9c17:

Fixed #20084 -- Provided option to validate formset max_num on server.

This is provided as a new "validate_max" formset_factory option defaulting to
False, since the non-validating behavior of max_num is longstanding, and there
is certainly code relying on it. (In fact, even the Django admin relies on it
for the case where there are more existing inlines than the given max_num). It
may be that at some point we want to deprecate validate_max=False and
eventually remove the option, but this commit takes no steps in that direction.

This also fixes the DoS-prevention absolute_max enforcement so that it causes a
form validation error rather than an IndexError, and ensures that absolute_max
is always 1000 more than max_num, to prevent surprising changes in behavior
with max_num close to absolute_max.

Lastly, this commit fixes the previous inconsistency between a regular formset
and a model formset in the precedence of max_num and initial data. Previously
in a regular formset, if the provided initial data was longer than max_num, it
was truncated; in a model formset, all initial forms would be displayed
regardless of max_num. Now regular formsets are the same as model formsets; all
initial forms are displayed, even if more than max_num. (But if validate_max is
True, submitting these forms will result in a "too many forms" validation
error!) This combination of behaviors was chosen to keep the max_num validation
simple and consistent, and avoid silent data loss due to truncation of initial
data.

Thanks to Preston for discussion of the design choices.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.