Fixed #21231 -- Enforced a max size for GET/POST values read into memory.

[timgraham]

Updated from #3852.
https://code.djangoproject.com/ticket/21231

[edevil]

LGTM

Thanks for the help!

[timgraham]

@edevil, what do you think about also limiting the reads for request.body as described on django-developers? (Not sure if you're following that thread.) Thanks.

[edevil]

@timgraham That check was present in the patch, but I was asked to remove it. I can't find the commit anymore since I have consolidated commits several times already. In my opinion that check should be there.

[lovelydinosaur]

Is dropping the initial '' argument to QueryDict here intentional? Does it have any effect?

[timgraham]

No behavior change since d68987a. For clarity, I can change all instances in Django in a separate commit.

[lovelydinosaur]

I'd agree that a check on body also seems reasonable. (A check on read() or limiting the underlying stream would not tho')

[lovelydinosaur]

I'd probably rather see us do read_size = settings.DATA_UPLOAD_MAX_MEMORY_SIZE + 1 - self._data_size and then drop the field_stream.read(1). I think that might be very marginally clearer. Either way reasonable tho'.

[timgraham]

Done this although it seems there might be a missing test as none fail if removing the "+ 1" (or the field_stream.read(1) in the earlier version of the patch).

[timgraham]

@edevil, do you understand what needs to be done for the body check? If so, could you send a PR to this branch? I guess it's not what is implemented here in the previous PR: 620ab2a

[edevil]

@timgraham @tomchristie I don't see the problem with the previous patch 620ab2a. The check was not on read() or the underlying stream, it was done when accessing the body property, which seems the correct place for it to be.

[lovelydinosaur]

This one would still make marginally more sense as a variable in the parse method, right?

[timgraham]

Was the complaint about the request.body check on this thread from @epandurski?

[edevil]

It was, and @apollo13 also weighed in:

Florian Apolloner <notifications@github.com>...

In django/http/request.py:

> @@ -230,6 +288,17 @@ def body(self):
>          if not hasattr(self, '_body'):
>              if self._read_started:
>                  raise RawPostDataException("You cannot access body after reading from request's data stream")
> +
> +            # Limit the maximum request data size that will be handled in-memory.
> +            try:
> +                content_length = int(self.META.get('HTTP_CONTENT_LENGTH', self.META.get('CONTENT_LENGTH', 0)))
> +            except (ValueError, TypeError):
> +                content_length = 0
> +
> +            if (settings.DATA_UPLOAD_MAX_MEMORY_SIZE is not None
I agree with @epandurski -- this check is wrong in the sense that it includes files.

—
Reply to this email directly or view it on GitHub.

I can't find this comment on github anymore, probably because the history was re-written.

[timgraham]

I'm also not seeing how that includes file uploads. I added print(self._body) in HttpRequest.body and didn't see any file upload data when running the test suite.

[lovelydinosaur]

Likewise. MultipartParser uses the stream interface so the body check seems valid to me too.

[timgraham]

The latest commit moves the check back to request.body.

[timgraham]

@edevil, do you have time to investigate this TODO?

[edevil]

I don't remember anymore the specific details of this... It seems to me that the idea was to read 1 more than the max so that the max would still be valid. The test bellow is: data_size > settings.DATA_UPLOAD_MAX_MEMORY_SIZE. But since we're already adding len(field_name) + 2 on top of what we read this seems pointless now.

[lovelydinosaur]

But since we're already adding len(field_name) + 2 on top of what we read this seems pointless now.

Good point.

[timgraham]

Removed. Ready for merge now?

[lovelydinosaur]

Having this around L152 would be very marginally cleaner from my POV. (Ie. it's similar to num_post_keys so makes sense for it to get initialized at same point in the codebase)

[lovelydinosaur]

One inconsequential style tweak noted above. Otherwise I've no remaining concerns.

[edevil]

Woah! Finally. Thanks for the work!