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

Fix: bodies on everything #283

Merged
merged 15 commits into from Sep 30, 2016
Merged

Fix: bodies on everything #283

merged 15 commits into from Sep 30, 2016

Conversation

@bertjwregeer
Copy link
Member

@bertjwregeer bertjwregeer commented Sep 29, 2016

This removes the implicit support of Chunked Encoding within WebOb. WebOb now treats all Requests as body-less unless the Content-Length header is set. This also means that every single HTTP verb (and unknown ones if you'd like :-P) are now allowed to have a body on it, and using wsgiref no longer hangs on non-existent bodies.

from wsgiref.simple_server import make_server
from webob import Request, Response

class MyApp(object):
    def __call__(self, environ, start_response):
        request = Request(environ)
        response = Response()
        print(request.method)
        print(request.body)
        response.body = b'Received: ' + request.body
        return response(environ, start_response)

httpd = make_server('localhost', 8080, MyApp())
try:
    httpd.serve_forever()
except KeyboardInterrupt:
    print('^C')

test with:

curl -X DELETE --data 'mydata' http://localhost:8080/
curl -X POST http://localhost:8080/ # No longer hangs
curl -X DELETE http://localhost:8080/
curl -X GET --data 'otherdata' http://localhost:8080/

The proposal in #278 has not yet been implemented.

Closes: #279, #233, #116
Supersedes: #274

Copy link
Member

@mmerickel mmerickel left a comment

Only had a few nitpicks. This really looks great, nicely done.

if isinstance(value, bytes):
warn_deprecation(
"Please use req.body = b'bytes' or req.body_file = fileobj",

This comment has been minimized.

@mmerickel

mmerickel Sep 29, 2016
Member

Expected fileobj but received bytes.

This comment has been minimized.

@bertjwregeer

bertjwregeer Sep 30, 2016
Author Member

Done.

chunked encoding in requests.
For background see https://bitbucket.org/ianb/webob/issue/6
webob.is_body_readable is a flag that tells us that we can read the
input stream even though CONTENT_LENGTH is missing.

This comment has been minimized.

@mmerickel

mmerickel Sep 29, 2016
Member

You are falling back to webob.is_body_readable if content_length == 0, is that really what you want?

This comment has been minimized.

@bertjwregeer

bertjwregeer Sep 29, 2016
Author Member

Ah... that's probably not a good idea.

This comment has been minimized.

@bertjwregeer

bertjwregeer Sep 30, 2016
Author Member

This was fixed.

#is_body_readable = environ_getter('webob.is_body_readable', False)

def _is_body_readable__get(self):
@property

This comment has been minimized.

@mmerickel

mmerickel Sep 29, 2016
Member

No need to indent docstrings past the quotation marks.

This comment has been minimized.

@bertjwregeer

bertjwregeer Sep 29, 2016
Author Member

It was already like that, didn't change it. Will fix that.

This comment has been minimized.

@bertjwregeer

bertjwregeer Sep 30, 2016
Author Member

Fixed.


if fileobj:
# We apparently had enough data to need a file

This comment has been minimized.

@mmerickel

mmerickel Sep 29, 2016
Member

Nice, this looks great.

@@ -1479,7 +1492,7 @@ def environ_add_POST(env, data, content_type=None):
data = url_encode(data)

This comment has been minimized.

@mmerickel

mmerickel Sep 29, 2016
Member

specify just "sending the body"

This comment has been minimized.

@bertjwregeer

bertjwregeer Sep 29, 2016
Author Member

I'm sorry, I am not sure what you are commenting on here...

This comment has been minimized.

@bertjwregeer

bertjwregeer Sep 29, 2016
Author Member

Nevermind, figured it out. Line numbers are off!

This comment has been minimized.

@bertjwregeer

bertjwregeer Sep 30, 2016
Author Member

Fixed.

if sz < sz0 and self.remaining:
raise DisconnectionError(
"The client disconnected while sending the POST/PUT body "
+ "(%d more bytes were expected)" % self.remaining
"(%d more bytes were expected)" % (self.remaining,)

This comment has been minimized.

@bertjwregeer

bertjwregeer Sep 29, 2016
Author Member

specify just "sending the body" from @mmerickel

bertjwregeer added 15 commits Sep 29, 2016
This has been deprecated since WebOb 1.2 and non-functional for a long
time.
Without it, there is no body and nothing is readable
This also changes req.from_file in that now it will attempt to read the
entire file for the body if no Content-Length is set, even if the
request method generally does not have a body (such as GET/DELETE).
You can now set req.body for any request method.
copy_body() used to treat chunked encoding (or at least reading from
wsgi.input without a valid Content-Length, could be user provided
body_file_raw and didn't set Content-Length) with no consideration for
the amount of data that could be on the other end.

This could potentially allow for a VERY large python bytes() object to
be allocated followed by a very large BytesIO(), which then one line
later it would copy to a temporary file, damage already done.

The new changes will read up to Content-Length if it is available, if
not but the body is still marked readable, it will attempt to read from
the input until it is exhausted.

Up to req.request_body_tempfile_limit will be stored in a BytesIO, after
that it will instead write it to a temporary file. So even if the other
end is sending 100's of MB worth of data it won't blow up your Python
process.
If there is no Content-Length then we don't read the body, thus the
tests that used to be able to assume True are now no longer True.
@bertjwregeer bertjwregeer force-pushed the fix/bodies_on_everything branch from 580d0f7 to cb9c0b4 Sep 30, 2016
@bertjwregeer
Copy link
Member Author

@bertjwregeer bertjwregeer commented Sep 30, 2016

@mmerickel Fixed up the issue you saw, and rebased them into previous commits. Let me know if this fixes all your concerns :-)

@bertjwregeer bertjwregeer mentioned this pull request Sep 30, 2016
@bertjwregeer bertjwregeer merged commit ce1eed5 into master Sep 30, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@bertjwregeer
Copy link
Member Author

@bertjwregeer bertjwregeer commented Sep 30, 2016

Thanks @mmerickel!

@bertjwregeer bertjwregeer deleted the fix/bodies_on_everything branch Nov 18, 2016
openstack-gerrit added a commit to openstack/openstack that referenced this pull request Mar 29, 2017
Project: openstack/glance  327682e8528bf4effa6fb16e8cabf744f18a55a1

Fix incompatibilities with WebOb 1.7

WebOb 1.7 changed [0] how request bodies are determined to be
readable. Prior to version 1.7, the following is how WebOb
determined if a request body is readable:
  #1 Request method is one of POST, PUT or PATCH
  #2 ``content_length`` length is set
  #3 Special flag ``webob.is_body_readable`` is set

The special flag ``webob.is_body_readable`` was used to signal
WebOb to consider a request body readable despite the content length
not being set. #1 above is how ``chunked`` Transfer Encoding was
supported implicitly in WebOb < 1.7.

Now with WebOb 1.7, a request body is considered readable only if
``content_length`` is set and it's non-zero [1]. So, we are only left
with #2 and #3 now. This drops implicit support for ``chunked``
Transfer Encoding Glance relied on. Hence, to emulate #1, Glance must
set the the special flag upon checking the HTTP methods that may have
bodies. This is precisely what this patch attemps to do.

[0] Pylons/webob#283
[1] https://github.com/Pylons/webob/pull/283/files#diff-706d71e82f473a3b61d95c2c0d833b60R894

Closes-bug: #1657459
Closes-bug: #1657452
Co-Authored-By: Hemanth Makkapati <hemanth.makkapati@rackspace.com>
Change-Id: I19f15165a3d664d5f3a361f29ad7000ba2465a85
openstack-gerrit pushed a commit to openstack/glance that referenced this pull request Mar 29, 2017
WebOb 1.7 changed [0] how request bodies are determined to be
readable. Prior to version 1.7, the following is how WebOb
determined if a request body is readable:
  #1 Request method is one of POST, PUT or PATCH
  #2 ``content_length`` length is set
  #3 Special flag ``webob.is_body_readable`` is set

The special flag ``webob.is_body_readable`` was used to signal
WebOb to consider a request body readable despite the content length
not being set. #1 above is how ``chunked`` Transfer Encoding was
supported implicitly in WebOb < 1.7.

Now with WebOb 1.7, a request body is considered readable only if
``content_length`` is set and it's non-zero [1]. So, we are only left
with #2 and #3 now. This drops implicit support for ``chunked``
Transfer Encoding Glance relied on. Hence, to emulate #1, Glance must
set the the special flag upon checking the HTTP methods that may have
bodies. This is precisely what this patch attemps to do.

[0] Pylons/webob#283
[1] https://github.com/Pylons/webob/pull/283/files#diff-706d71e82f473a3b61d95c2c0d833b60R894

Closes-bug: #1657459
Closes-bug: #1657452
Co-Authored-By: Hemanth Makkapati <hemanth.makkapati@rackspace.com>
Change-Id: I19f15165a3d664d5f3a361f29ad7000ba2465a85
openstack-gerrit added a commit to openstack/openstack that referenced this pull request Mar 29, 2017
Project: openstack/glance  327682e8528bf4effa6fb16e8cabf744f18a55a1

Fix incompatibilities with WebOb 1.7

WebOb 1.7 changed [0] how request bodies are determined to be
readable. Prior to version 1.7, the following is how WebOb
determined if a request body is readable:
  #1 Request method is one of POST, PUT or PATCH
  #2 ``content_length`` length is set
  #3 Special flag ``webob.is_body_readable`` is set

The special flag ``webob.is_body_readable`` was used to signal
WebOb to consider a request body readable despite the content length
not being set. #1 above is how ``chunked`` Transfer Encoding was
supported implicitly in WebOb < 1.7.

Now with WebOb 1.7, a request body is considered readable only if
``content_length`` is set and it's non-zero [1]. So, we are only left
with #2 and #3 now. This drops implicit support for ``chunked``
Transfer Encoding Glance relied on. Hence, to emulate #1, Glance must
set the the special flag upon checking the HTTP methods that may have
bodies. This is precisely what this patch attemps to do.

[0] Pylons/webob#283
[1] https://github.com/Pylons/webob/pull/283/files#diff-706d71e82f473a3b61d95c2c0d833b60R894

Closes-bug: #1657459
Closes-bug: #1657452
Co-Authored-By: Hemanth Makkapati <hemanth.makkapati@rackspace.com>
Change-Id: I19f15165a3d664d5f3a361f29ad7000ba2465a85
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants