Fix: bodies on everything by digitalresistor · Pull Request #283 · Pylons/webob

digitalresistor · 2016-09-29T06:51:49Z

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

mmerickel

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

mmerickel · 2016-09-29T17:34:32Z

webob/request.py

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


Expected fileobj but received bytes.

mmerickel · 2016-09-29T17:36:31Z

webob/request.py

-            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.


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

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

This was fixed.

mmerickel · 2016-09-29T17:37:01Z

webob/request.py

-    #is_body_readable = environ_getter('webob.is_body_readable', False)
-
-    def _is_body_readable__get(self):
+    @property


No need to indent docstrings past the quotation marks.

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

mmerickel · 2016-09-29T17:43:28Z

webob/request.py

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


Nice, this looks great.

mmerickel · 2016-09-29T17:44:23Z

webob/request.py

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


specify just "sending the body"

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

Nevermind, figured it out. Line numbers are off!

digitalresistor · 2016-09-29T18:21:37Z

webob/request.py

            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,)


specify just "sending the body" from @mmerickel

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.

Closes #276

digitalresistor · 2016-09-30T01:08:48Z

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

digitalresistor · 2016-09-30T06:13:39Z

Thanks @mmerickel!

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

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

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

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

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

digitalresistor added this to the 1.7.0 milestone Sep 29, 2016

digitalresistor mentioned this pull request Sep 29, 2016

request: read body from file if the method is DELETE #274

Closed

mmerickel requested changes Sep 29, 2016

View reviewed changes

digitalresistor commented Sep 29, 2016

View reviewed changes

digitalresistor added 15 commits September 29, 2016 19:07

Req.body_file no longer accepts bytes

f35a535

This has been deprecated since WebOb 1.2 and non-functional for a long time.

Set CONTENT_LENGTH in environ for tests

c4e7fac

Without it, there is no body and nothing is readable

Don't use DummyIO, use BytesIO

9f8e25b

Use UnseekableInput to test for no seek

91c5832

UnseekableInput now allows reads larger than length

9a5a1ca

Remove unused imports

b64bc42

Remove http_method_probably_has_body

0e6fcbf

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).

req.body gets some minor cleanups

50b7fc8

You can now set req.body for any request method.

Without Content-Length, we don't read body

64e3407

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.

Fix some test helpers

4cce6d1

Invalid indent

6f959d6

PEP8

8fde5e2

Add DeprecationWarning for FakeCGIBody

3b604da

Closes #276

Update docstring

cb9c0b4

digitalresistor force-pushed the fix/bodies_on_everything branch from 580d0f7 to cb9c0b4 Compare September 30, 2016 01:08

digitalresistor mentioned this pull request Sep 30, 2016

Update comment #275

Closed

mmerickel approved these changes Sep 30, 2016

View reviewed changes

digitalresistor merged commit ce1eed5 into master Sep 30, 2016

digitalresistor deleted the fix/bodies_on_everything branch November 18, 2016 07:13

pyup-bot mentioned this pull request Nov 3, 2017

Update webob to 1.7.3 tracim/tracim#434

Closed

pyup-bot mentioned this pull request Jan 26, 2018

Update webob to 1.7.4 DemocracyClub/yournextrepresentative#407

Closed

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

Conversation

digitalresistor commented Sep 29, 2016

Uh oh!

mmerickel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digitalresistor commented Sep 30, 2016

Uh oh!

digitalresistor commented Sep 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants