Improved/corrected documentation for the DOS protection options

[d-maurer]

#1177 has shown that Zope's DOS protection options documentation is partially wrong and difficult to understand.

This PR significantly extends the documentation by a description of the process influenced by the options and a precise specification of their imposed limits.

@mauritsvanrees #1177 might indicate that Plone needs a higher form-memory-limit default (currently 1MB). What is your experience?

[d-maurer]

The failing "Windows py3.7" test is:

Failure in test test_scored_search (Products.ZCatalog.tests.test_catalog.TestScoring)
...
  File "d:\a\zope\zope\.tox\py37\eggs\products.zcatalog-7.0-py3.7.egg\products\ZCatalog\tests\test_catalog.py", line 1120, in test_scored_search
    self.assertEqual(brains[0].title, '111')
...
AssertionError: '11' != '111'

We know that some ZCatalog tests are not robust wrt. load conditions and therefore can occasionally fail. But, I have not yet known that the above test belongs to them. I will restart the tests and see what happens.

[mauritsvanrees]

The updated documentation LGTM, thanks.

Dieter Maurer wrote:

@mauritsvanrees #1177 might indicate that Plone needs a higher form-memory-limit default (currently 1MB). What is your experience?

In my experience, only sites with plone.restapi installed (this includes all sites using the default Volto frontend) are affected. In such a site, if you try to upload a file or image larger than 1MB, you get an error:

zExceptions.BadRequest: data exceeds memory limit

See also plone/Products.CMFPlone#3848.

On a few sites I have increased the form-memory-limit to 10MB, which at least seems fair for images. I wondered about doing a patch in Products.CMFPlone to update this limit, but then we would have to be careful to not override a manual setting in zope.conf. I would be happy with a higher limit in Zope.

In a pure Plone ClassicUI site without plone.restapi installed, with the default settings I can upload a 22MB file without problem. There the uploaded file is read in a way that does not trigger the limits.

With plone.restapi the error already happens when calling extractCredentials on the PAS plugin that comes with the package. The json_body function that is used there should probably be changed along the lines of your suggestions in #1177 (comment). This function is used all over the place though, so this could have unwanted side effects.

I think I will create a PR for plone.restapi that catches the BadRequest error in the PAS plugin. That fixes the basic error for ClassicUI at least.

[d-maurer]

In my experience, only sites with plone.restapi installed (this includes all sites using the default Volto frontend) are affected. In such a site, if you try to upload a file or image larger than 1MB, you get an error:

zExceptions.BadRequest: data exceeds memory limit

@mauritsvanrees
If Plone (or plone.restapi) reads whole file contents into memory, it does something wrong. We would not want to have the content of a large (e.g. 1 GB) file in memory - it would be there not once but several times (e.g. once as part of the complete body and once as value in the JSON object).
Files (and to a smaller extend images) should be transferred via multipart/form-data requests. They represent files via file like objects which can be efficiently (i.e. without high memory requirement) transformed into blobs.

I suggest to rework the plone.restapi API. API requests should consist of a JSON object and optional attachments. The JSON object is for small information, the attachments carry potentially large information. Without attachments, the JSON object could be transferred in an application/json request (which would ensure backward compatibility); requests with attachments would use mutipart/form-data requests. This would ensure that attachments are represented as file like objects and not be read as a whole into memory.

[mauritsvanrees]

Based on the feedback from @mauritsvanrees, the PR now also increases the default for form-memory-limit from 1 MB to 8 MB. Hopefully, this will avoid the need for Plone to configure this option.

Thanks!

[mauritsvanrees]

I am happy with this. @d-maurer Can you merge?

@dataflake @icemac A Zope release with this would be good. Would that be possible today or tomorrow? I am trying to release Plone 6.0.8 and want to have the increased limit in there, either with a new Zope version or with a temporary patch in CMFPlone.

[dataflake]

@mauritsvanrees I don't mind doing releases but I would really appreciate more warning than "today or tomorrow". I am sure releasing Plone 6.0.8 didn't occur to you just today, right?

[d-maurer]

Maurits van Rees wrote at 2023-10-31 04:02 -0700:

On a few sites I have increased the form-memory-limit to 10MB, which at least seems fair for images. I wondered about doing a patch in `Products.CMFPlone` to update this limit, but then we would have to be careful to not override a manual setting in `zope.conf`. I would be happy with a higher limit in Zope.

@mauritsvanrees The parameter protects against potential denial of service attacks by overloading a system's RAM resources. I meanwhile learned that `plone.restapi` wants to load complete file contents into RAM (a design fault in my view). Should you want to keep this (hopefully only temporarily), you could override `ZPublisher.HTTPRequest.VALUE_LIMIT` (currently `Global("FORM_MEMORY_LIMIT")`) by `None`. This would allow arbitrary large request bodies accessed via `request["BODY"])`.

[mauritsvanrees]

@dataflake Sorry for not communicating earlier.

The problem with the low memory limit was already known (to Plone people) since shortly after the Plone 6.0.7 release from 21 September. Since then some fixes were done to counter this (in plone.recipe.zope2instance and plone/cookiecutter-zope-instance), but they still required a configuration change. I had hoped that someone (most likely a Plone core developer) would have picked this up already, no matter if that would be by a change in Zope or in Plone. But that did not happen yet. So much for wishful thinking...

Meanwhile there actually is already a Plone 6.0.8 release candidate since last Thursday. I wanted to do the final release two days ago. But finally I decided we should not ship with this low default limit, so I postponed the release (also for one other, totally unrelated problem).

[d-maurer]

Maurits van Rees wrote at 2023-11-2 03:44 -0700:

I am happy with this. @d-maurer Can you merge?

I would like to still wait a bit. The current state will still not make all Plone admins happy as only uploads up to 8 MB will be possible without reconfiguration. The current state of `plone.restapi` requires that access to `request["BODY"]` does not have a size protection (or a very large one (in the order of GB)). We could remove this size protection in `Zope` or you could do this in `Products.CMFPlone`. Or we could add an additional parameter (e.g. `REQUEST_BODY_MEMORY_LIMIT`) with a quite large default. We should find consensus about those options before the PR gets merged.

[mauritsvanrees]

plone.restapi definitely needs some changes along the lines you have indicated here and elsewhere.

As a quick fix, I think I will open a PR in plone.restapi to override FORM_MEMORY_LIMIT with a larger value (maybe 16MB) if it is at the current default of 1MB. Then only users of plone.restapi are affected, and the configuration options are still all there.

[d-maurer]

As a quick fix, I think I will open a PR in plone.restapi to override FORM_MEMORY_LIMIT with a larger value (maybe 16MB) if it is at the current default of 1MB. Then only users of plone.restapi are affected, and the configuration options are still all there.

Maybe, overriding ZPublisher.HTTPRequest.ZopeFieldStorage.VALUE_LIMIT is a better option. If plone.restapi sets it to None, then there is no longer a size limitation for request["BODY"] accesses.

[d-maurer]

I removed the default change for form_memory_limit again as @mauritsvanrees plans to resolve the Plone related problems elsewhere.

[mauritsvanrees]

Maybe, overriding ZPublisher.HTTPRequest.ZopeFieldStorage.VALUE_LIMIT is a better option. If plone.restapi sets it to None, then there is no longer a size limitation for request["BODY"] accesses.

Okay, let's go with that after all. Thanks.
See plone/plone.restapi#1729

[dataflake]

@d-maurer OK, I suggest merging now

@mauritsvanrees Do you still need a Zope 5.8.7 release?

[mauritsvanrees]

@dataflake No need for a Zope release, the plone.restapi change will be enough for now. Thanks anyway.

FWIW, I tried the Zope master branch and its versions earlier this week, and all Plone tests passed.