Skip to content

Comments

Adding FORM_PART_LIMIT for overriding MultipartParser.part_limit#1251

Merged
drfho merged 5 commits intomasterfrom
fix_form_part_limit
Feb 18, 2025
Merged

Adding FORM_PART_LIMIT for overriding MultipartParser.part_limit#1251
drfho merged 5 commits intomasterfrom
fix_form_part_limit

Conversation

@drfho
Copy link
Contributor

@drfho drfho commented Feb 9, 2025

Complex modelling GUIs may have lot of form values in a grid. With the introduction of multipart==1.2.1
https://github.com/zopefoundation/Zope/blame/a2cbf86fca81836aac194b1075ed0728e8c0c79b/requirements-full.txt#L27
some weeks ago the arbitrary limit of 128 form parameters is getting relevant. Using the new requirement ZPublisher blocks bigger forms. I would like to suggest handling the parts_limit it similar to the other form limits and add a corresponding variable

FORM_PART_LIMIT = 2 ** 10     # limit for individual form parts

that can override the 128 limitation.

Ref: zms-publishing/ZMS#334

@drfho drfho requested a review from d-maurer February 9, 2025 18:46
@dataflake
Copy link
Member

I think that's a good idea but not ready for merging. This needs tests and it should be configurable in zope.conf under the dos_protection section as well, like the other limits. I may have some time over the next few days.

@dataflake
Copy link
Member

@drfho I'm currently working on making this configurable. Question: I'm not a frontend developer so I don't know how big those forms can get. Would it not be better to choose a lower default (or even the multipart default of 128) as a safer starting value? My best guess would be that this excessive number of form parameters would be the exception rather than the rule. IMHO an administrator should have to explicitly take action and configure the higher limit if they really need it.

@drfho
Copy link
Contributor Author

drfho commented Feb 17, 2025

@drfho 128 as a safer starting value?

128 is really low; 512 would be good and 1024 quite safe. I think the authors of multiparts are not adressing production GUIs but 3rd view front-ends that are published to the public. Supposingly they didn't have administrative GUIs like Zope, Plone etc in mind.

drfho added a commit to zms-publishing/ZMS that referenced this pull request Feb 17, 2025
@dataflake
Copy link
Member

128 is really low; 512 would be good and 1024 quite safe. I think the authors of multiparts are not adressing production GUIs but 3rd view front-ends that are published to the public. Supposingly they didn't have administrative GUIs like Zope, Plone etc in mind.

Yes you're right, I didn't think about mile long folder contents lists in the ZMI.

@davisagli
Copy link
Member

And the grid of checkboxes in manage_access...

Copy link
Member

@dataflake dataflake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to go I think

@drfho
Copy link
Contributor Author

drfho commented Feb 18, 2025

ok, looks perfect, many thanks!
Merge is done by @dataflake, right?

@dataflake
Copy link
Member

No, it is done by the person who opened the PR, unless this is an unknown new developer.

@drfho drfho merged commit 8744127 into master Feb 18, 2025
28 checks passed
@drfho drfho deleted the fix_form_part_limit branch February 18, 2025 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants