HttpRequest.read(NUM_BYTES) can read beyond the end of wsgi.input stream. (Violation of WSGI spec & under-defined behaviour)

[Tom Christie]

Okay, this is the underlying cause behind #15762 and #15763.
I've marked those as duplicates of this ticket.

Please see discussion on this bug here: ​https://groups.google.com/forum/#!topic/django-developers/VG1ueWTSs_g

The problem is now that HttpRequest exposes a read() method, user code can do something like:

request_content = json.load(request)

at the moment that will:

• break the wsgi spec, as the client app is contracted not to attempt to read more than CONTENT-LENGTH bytes from wsgi.input
• result in under-defined behaviour, although it appears to work right now.
• break when used in the test client, as per #15762

I've attached a patch with tests for this issue, which:

• Changes WSGIRequest._stream to be a property that is (always) instantiated as a LimitedStream when first accessed.
• Removes some redundant code in HttpRequest and MultiPartParser.
• Fixes some minor bugs in tests/regressiontests/requests/tests.py
• Adds two tests for MultiPartParser to check graceful behaviour on truncated or empty multipart requests.
• Adds a test for TestClient request.read(LARGE_BUFFER) behaviour.

[Tom Christie]

Updated patch (limited_stream_lazy.diff)
Thanks to Ivan's testing it turns out the defered creation of the LimitedStream isn't necessary.
This should do the job nicely.

[Ivan Sagalaev]

Tom, since you're hacking on it so actively :-) how about adding another test somewhere near read_limited_stream that would test reading from a GET request that doesn't have a Content-length header? It would test the situation with some generic view code always calling request.read() irregardless of the request type. The sensible thing to do here is not to break and just give it an empty string.

[Tom Christie]

Good plan, I'll get on to that.
I'm assuming it's cool to drop this ticket into 'Accepted' now.

[Tom Christie]

Added some extra tests and cleaned up some test code to also include the test for #14753, which has similar logic.

I've added this as limited_stream_lazy_plus_extra_tests.2.diff.

As a final sanity check I reverted, applied the tests patches only, these two fail:

• test_read_numbytes_from_empty_request
• test_read_numbytes_from_nonempty_request

applied the code patches, checked they passed.

Ivan, could you review, I'm guessing this should be able to go into 'Ready for checkin' now.

Ta! :)

[Ivan Sagalaev]

Couple of comments, small nits, actually:

+        r = {
+            'CONTENT_LENGTH': len(payload),
+            'CONTENT_TYPE':   client.MULTIPART_CONTENT,
+            'PATH_INFO':      "/file_uploads/echo/",
+            'REQUEST_METHOD': 'POST',
+            'wsgi.input':     client.FakePayload(payload),
+        }

Django uses a different style in such cases: values should not be vertically aligned but separated from keys with a colon and a single space.

+        try:
+            content_length = int(self.environ.get('CONTENT_LENGTH', 0))
+        except (ValueError, TypeError):
+            content_length = 0

I don't see how TypeError can be raised here. Also, the default value is defined in two places. I'd do it like this instead:

try:
  content_length = int(self.environ.get('CONTENT_LENGTH'))
except ValueError:
  content_length = 0

[Graham Dumpleton]

If you are going to do:

try:
  content_length = int(self.environ.get('CONTENT_LENGTH'))
except ValueError:
  content_length = 0

then you must catch TypeError as lack of CONTENT_LENGTH would return None which yields a TypeError when int() is applied to it.

So yes, TypeError shouldn't have been needed in what they originally had, but is for what you have suggested.

[Ivan Sagalaev]

Oh, good catch, thanks!

[anonymous]

Coolio, I've updated the patch to reflect those points.

It's my first contribution, so thanks for the feedback and helping this along...

[Ivan Sagalaev]

This now looks to me ready for checkin. Thanks, Tom!

[vung]

This patch would also fix #15890

[Jannis Leidel]

Not sure what's wrong, but one test doesn't pass anymore:

Creating test database for alias 'default'...
Creating test database for alias 'other'...
Destroying old test database 'other'...
....F...........
======================================================================
FAIL: test_empty_multipart_raises_error (regressiontests.file_uploads.tests.FileUploadTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jezdez/Code/git/django/tests/regressiontests/file_uploads/tests.py", line 217, in test_empty_multipart_raises_error
    self.assertRaises(MultiPartParserError, lambda r: self.client.request(**r), r)
AssertionError: MultiPartParserError not raised

----------------------------------------------------------------------
Ran 16 tests in 1.351s

FAILED (failures=1)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...

[Tom Christie]

The expected behavior of MultiPartParser when given a Content-Length of 0 has changed slightly since this ticket/patch was submitted.
Please see ticket #16201 for details of how & why.

I've updated the test_empty_multipart_raises_error() test to instead be test_empty_multipart_handled_gracefully(), and test that it returns an empty QueryDict, rather than test that it throws a MultiPartParseError.

I've verified that the patch still applies okay to the latest revision, and that the tests are now all passing again.

Patch attached as limited_stream_lazy_plus_extra_tests.4.diff

[Jannis Leidel]

In [16479]:

Fixed #15785 -- Stopped HttpRequest.read() from reading beyond the end of a wsgi.input stream and removed some redundant code in the multipartparser. Thanks, tomchristie, grahamd and isagalaev.

[anonymous]

Doesn't this revert the changes made in [14453]?

[Jannis Leidel]

It reverts it, but it's not needed any more since we wrap the wsgi.input in a LimitedStream in any case, whether development server or not.

[Jacob]

Milestone 1.3 deleted