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

extractCredentials: do not fail when the request is too large to read. #1726

Closed

Conversation

mauritsvanrees
Copy link
Sponsor Member

This is a problem since at least Zope 5.8.4, introduced in Plone 6.0.7.
See plone/Products.CMFPlone#3848 and zopefoundation/Zope#1180.

This PR makes sure that a too big request body does not throw an error when extracting credentials. This makes large image upload work again in ClassicUI with plone.restapi installed.

Uploading a file or image larger than 1MB using plone.restapi still likely fails, unless you have increased the form-memory-limit in zope.conf. Fixing this may need a change along these lines, but that would be a more far reaching change that needs careful consideration.

@mister-roboto

This comment was marked as resolved.

@mauritsvanrees
Copy link
Sponsor Member Author

@jenkins-plone-org please run jobs

@mauritsvanrees
Copy link
Sponsor Member Author

As alternative, or as extra, we could do this before calling creds = deserializer.json_body(request):

if request.get_header("Content-Type") != "application/json":
    return

This was suggested by @davisagli in plone/Products.CMFPlone#3848 (comment)

In my case in ClassicUI during an image upload, the content type is something like multipart/form-data; boundary=.... When just browsing the site, the content type is None. But then, when I visit http://localhost:8080/Plone/++api++/ in the browser (so not called by javascript), the Content-Type request header is also None. So that check might not be enough.

Copy link

netlify bot commented Oct 31, 2023

Deploy Preview for plone-restapi canceled.

Name Link
🔨 Latest commit 3a3770b
🔍 Latest deploy log https://app.netlify.com/sites/plone-restapi/deploys/6540e2249f1fa2000840ff29

@davisagli
Copy link
Sponsor Member

davisagli commented Oct 31, 2023

@mauritsvanrees In both of those cases, the request does not have a JSON body, and we don't want to waste effort in trying to parse it as JSON. The only problem would be if there is a client which is sending a JSON body but not sending the Content-Type: application/json header. That is a buggy client not following the documented API.

It should be a conditional around the code that tries to get login & password from the JSON body though, not a return out of the function. We still need to run the code after that looks for the token in the Authorization header, regardless of request content type.

@davisagli
Copy link
Sponsor Member

Hmm, the whole chunk of code looking for login and password in the request body seems misplaced. Shouldn't that be done only in the login service?

@mauritsvanrees
Copy link
Sponsor Member Author

Hmm, the whole chunk of code looking for login and password in the request body seems misplaced. Shouldn't that be done only in the login service?

I think you are right. Theoretically this chunk gets a dict with login and password, but would then be passed on to authenticateCredentials a few lines down, and this only checks for a token. So extracting login and password indeed seems useless.

@mauritsvanrees
Copy link
Sponsor Member Author

Do note this remark in extractCredentials:

Prefer any credentials in a JSON POST request under the assumption that any
such requested sent when a JWT token is already in the Authorization header
is intended to change or update the logged in user.

But as said, that should not have any effect.

I will keep this PR, as it should be okay, but then create another one where I remove these lines.

mauritsvanrees added a commit that referenced this pull request Oct 31, 2023
The result was never used, and it may fail when the request is too large to read.
This is a problem since at least Zope 5.8.4, introduced in Plone 6.0.7.
See plone/Products.CMFPlone#3848 and zopefoundation/Zope#1180.

This PR is an alternative to #1726.  See discussion there.
@mauritsvanrees
Copy link
Sponsor Member Author

Closed in favour of PR #1728.

@mauritsvanrees mauritsvanrees deleted the maurits-pas-extract-credentials-catch-badrequest branch November 2, 2023 10:39
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.

None yet

3 participants