Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

json_body should not read entire request BODY #1730

Open
mauritsvanrees opened this issue Nov 2, 2023 · 5 comments · May be fixed by #1731
Open

json_body should not read entire request BODY #1730

mauritsvanrees opened this issue Nov 2, 2023 · 5 comments · May be fixed by #1731

Comments

@mauritsvanrees
Copy link
Member

So this line is bad:

data = json.loads(request.get("BODY") or "{}")

With Zope 5.8.4+ this fails when the request BODY is larger than 1MB.
See discussion in plone/Products.CMFPlone#3848 and zopefoundation/Zope#1142.
See my summary of the history and the steps taken so far.

Temporarily fixed in #1729 by disabling a Zope form memory limit check.

@gogobd
Copy link

gogobd commented Nov 7, 2023

I was patching this like this:

…/plone/restapi/deserializer/__init__.py

from plone.restapi.exceptions import DeserializationError

import json


def json_body(request):
    try:
        if hasattr(request, "BODYFILE"):
            data = request.get('BODYFILE').read()
        else:
            data = json.loads(request.get("BODY") or "{}")
    except ValueError:
        raise DeserializationError("No JSON object could be decoded")
    if not isinstance(data, dict):
        raise DeserializationError("Malformed body")
    return data


def boolean_value(value):
    """

    Args:
        value: a value representing a boolean which can be
               a string, a boolean or an integer
                   (usually a string from a GET parameter).

    Returns: a boolean

    """
    return value not in {False, "false", "False", "0", 0}
 

But as @mauritsvanrees pointed out: "We should not read the complete request BODY in memory." and so that's probably not helping. I wasn't able to come up with a test that would actually push a request through Zope's machinery to address this issue. I also think that a "fix" like the one I show here would not really eliminate the D.O.S. threat that potentially was the reason for introducing the limits in the first place. The only "good" fix would be to have every request body be a stream in all cases, but I wonder if that's feasible. Can we use a data structure that is a stream and behaves like "in memory data"?

@gogobd
Copy link

gogobd commented Nov 7, 2023

Additionally we "fixed" the problem by upping the limits in zope.conf like this:

<dos_protection>
    form-memory-limit 1GB
    form-disk-limit 1GB
    form-memfile-limit 4KB
</dos_protection>

@gogobd
Copy link

gogobd commented Nov 7, 2023

... maybe at one point we will have to live with "huge" data in memory.

@wesleybl
Copy link
Member

wesleybl commented Nov 7, 2023

Can this problem cause high memory usage on portals that upload large files?

@davisagli
Copy link
Member

json_body's job is to return an entire data structure deserialized from JSON. If we give it a very large JSON document, it can't do much about that.

It might be possible to create a new function which returns a specific key/value from a JSON document without reading the entire structure into memory, using something like https://pypi.org/project/ijson/ -- but you need to look at the specific places where json_body is used to see if that is helpful.

For the use case of file uploads in particular, I think the current approach that volto uses (base64-encoded file data in a JSON body) is not optimal. I think we should explore adding support for a different serialization format in plone.restapi, with a multipart/form-data body that includes both the content as JSON and separate files as attachments (and some convention for referencing the files from within the JSON structure).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants