[Django] #29427: RequestDataTooBig raised in request.py prevents Middleware from returning a valid response
Django
valid response
-----------------------------------------+------------------------
Reporter: S. Paquette | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
This is effectively a request to re-open #28106, which was closed because
the original author never replied to a request for more information.
We need a way to return a response from a Middleware which is handling the
RequestDataTooBig exception, but Middlewares intercepting this exception
never generate a valid response. This seems due to how the exception
causes self._body to never be created in request.py.
In django.http.request, a check of the content length is done against
settings.DATA_UPLOAD_MAX_MEMORY_SIZE at line 267.
{{{
# Limit the maximum request data size that will be handled in-
memory.
if (settings.DATA_UPLOAD_MAX_MEMORY_SIZE is not None and
int(self.META.get('CONTENT_LENGTH') or 0) >
settings.DATA_UPLOAD_MAX_MEMORY_SIZE):
raise RequestDataTooBig('Request body exceeded
settings.DATA_UPLOAD_MAX_MEMORY_SIZE.')
}}}
If the content length exceeds DATA_UPLOAD_MAX_MEMORY_SIZE, no request body
is generated, and a RequestDataTooBig exception is raised.
In order to detect this error and return a useful response to our users'
browsers, we created a Middleware to catch the exception and supply an
informative JsonResponse. However, despite the status setting correctly,
the response itself was never being returned. Our Middleware:
{{{
from django.http import JsonResponse
from django.core.exceptions import RequestDataTooBig
class CheckSize(object):
def __init__(self, get_response):
self.get_response = get_response
def __call__(self, request):
try:
body = request.body
except RequestDataTooBig:
return JsonResponse({"msg": "The file provided is too large.
Please reduce its size and try again."}, status=400)
response = self.get_response(request)
return response
}}}
We tried placing the Middleware anywhere in the chain, and making it the
only Middleware, but nothing worked.
Per the author of #28106, we then added in an empty body to the request
when the exception is raised, and that solved the problem:
{{{
# Limit the maximum request data size that will be handled in-
memory.
if (settings.DATA_UPLOAD_MAX_MEMORY_SIZE is not None and
int(self.META.get('CONTENT_LENGTH') or 0) >
settings.DATA_UPLOAD_MAX_MEMORY_SIZE):
self._body = self.read(None)
raise RequestDataTooBig('Request body exceeded
settings.DATA_UPLOAD_MAX_MEMORY_SIZE.')
}}}
After doing this, our response is returned. This can be reproduced on
Django 1.11.
--
Ticket URL: <https://code.djangoproject.com/ticket/29427>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
valid response
Reporter: S. Paquette | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* stage: Unreviewed => Accepted
Comment:
Seems legitimate. Would you be able to write a failing test for the Django
test suite?
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:1>
Django
valid response
Reporter: S. Paquette | Owner: oliver
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by oliver):
* owner: nobody => oliver
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:2>
Django
valid response
Reporter: S. Paquette | Owner: (none)
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by oliver):
* owner: oliver => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:3>
Django
valid response
Reporter: S. Paquette | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by S. Paquette):
Replying to [comment:1 Claude Paroz]:
> Seems legitimate. Would you be able to write a failing test for the
Django test suite?
Sure thing--how's this look for django/tests/requests/tests.py?
{{{
from django.core.exceptions import RequestDataTooBig
def test_req_body_exists_after_size_exceeded(self):
"""
If a CONTENT_LENGTH > DATA_UPLOAD_MAX_MEMORY_SIZE is encountered,
an empty
_body attribute should still be generated in the request
"""
with override_settings(DATA_UPLOAD_MAX_MEMORY_SIZE=12):
payload = FakePayload('a=1&a=2;a=3\r\n')
request = WSGIRequest({
'REQUEST_METHOD': 'POST',
'CONTENT_TYPE': 'application/x-www-form-
urlencoded',
'CONTENT_LENGTH': len(payload),
'wsgi.input': payload,
})
with self.assertRaises(RequestDataTooBig):
request.body
self.assertTrue(hasattr(request,'_body'))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:2>
Django
valid response
Reporter: S. Paquette | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Herbert Fortes):
* cc: Herbert Fortes (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:3>
Django
valid response
Reporter: S. Paquette | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Josh Schneier):
Is there any reason that this can't be handled using an exception handling
middleware?
{{{#!python
from django.http import JsonResponse
from django.core.exceptions import RequestDataTooBig
class HandleDataTooBigMiddleware:
def __init__(self, get_response):
self.get_response = get_response
def __call__(self, request):
return self.get_response(request)
def process_exception(self, request, exception):
if isinstance(exception, RequestDataTooBig):
return JsonResponse({'info': 'File too big'}, status=400)
return None
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:4>
Django
valid response
Reporter: S. Paquette | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Herbert Fortes):
I liked [https://docs.djangoproject.com/en/2.0/topics/http/middleware
/#process-exception process-exception] proposed by Josh Schneier. I did
not test it though.
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:5>
Django
valid response
Reporter: S. Paquette | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Herbert Fortes):
Replying to [comment:2 S. Paquette]:
> Replying to [comment:1 Claude Paroz]:
> > Seems legitimate. Would you be able to write a failing test for the
Django test suite?
>
> Sure thing--how's this look for django/tests/requests/tests.py?
As I see, only the PR is missing. Test and fix (self._body) are known.
I ran the test.
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:6>
Django
valid response
Reporter: S. Paquette | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by S. Paquette):
Replying to [comment:4 Josh Schneier]:
> Is there any reason that this can't be handled using an exception
handling middleware?
>
> {{{#!python
> from django.http import JsonResponse
> from django.core.exceptions import RequestDataTooBig
>
> class HandleDataTooBigMiddleware:
> def __init__(self, get_response):
> self.get_response = get_response
>
> def __call__(self, request):
> return self.get_response(request)
>
> def process_exception(self, request, exception):
> if isinstance(exception, RequestDataTooBig):
> return JsonResponse({'info': 'File too big'}, status=400)
> return None
> }}}
Per the author of #28106, this also doesn't work if the body isn't set; in
fact that's how their Middleware is structured. (The one I pasted here is
another option I tried when the exception handling Middleware wouldn't
work.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:7>
Django
valid response
Reporter: S. Paquette | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Herbert Fortes):
S. Paquette,
Can you do the PR? You did the test. And if the test needs
adjustments about the style you can do it.
If it is the first time you do a PR to Django project, there is an
official [https://docs.djangoproject.com/en/dev/intro/contributing/, how
to write a patch].
Regards
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:8>
Django
valid response
Reporter: S. Paquette | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/11601 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:9>
Django
valid response
Reporter: S. Paquette | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* Attachment "issue_29427.zip" added.
Failing to reproduce...
Django
valid response
Reporter: S. Paquette | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Claude's patch looks fine but doesn't seem to address the underlying
issue. (Why is a response not returned when/if the request doesn't have a
body set.)
I attached a project using the `HandleDataTooBigMiddleware` but failing to
reproduce the issue.
`DEBUG=TRUE`: Expected debug error page.
`DEBUG=False`: Expected JSON response.
As the project is `DEBUG=False`. `./manage.py runserver`, then:
{{{
~ $ curl -X "POST" "http://127.0.0.1:8000/reproduce/" \
> -H 'Content-Type: application/x-www-form-urlencoded; charset=utf-8'
\
> --data-urlencode "testing=123456789"
{"info": "File too big"}
}}}
(Testing against `master`, pre-3.0 and `stable/1.11.x` — same results.)
It would be nice to pin down the underlying issue here.
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:10>
Django
valid response
Reporter: S. Paquette | Owner: Claude Paroz
Type: Bug | Status: assigned
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => assigned
* owner: nobody => Claude Paroz
* version: 1.11 => master
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:11>
Django
valid response
Reporter: S. Paquette | Owner: Claude Paroz
Type: Bug | Status: assigned
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
As Carlton wasn't able to reproduce, it would be great if reporters could
provide some sample project code to reproduce.
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:12>
Django
valid response
Reporter: S. Paquette | Owner: Claude Paroz
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* status: assigned => closed
* resolution: => needsinfo
--
Ticket URL: <https://code.djangoproject.com/ticket/29427#comment:13>