Formsets should sign/verify max_num

[Jacob]

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.

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

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

[Carl Meyer]

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.

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

[Carl Meyer <carl@…>]

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.