Opened 6 years ago
Closed 5 years ago
#29427 closed Bug (needsinfo)
RequestDataTooBig raised in request.py prevents Middleware from returning a valid response
Reported by: | S. Paquette | Owned by: | Claude Paroz |
---|---|---|---|
Component: | HTTP handling | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Herbert Fortes | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
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.
Attachments (1)
Change History (14)
follow-up: 2 comment:1 by , 6 years ago
Triage Stage: | Unreviewed → Accepted |
---|
follow-up: 6 comment:2 by , 6 years ago
Replying to 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'))
comment:3 by , 6 years ago
Cc: | added |
---|
follow-up: 7 comment:4 by , 6 years ago
Is there any reason that this can't be handled using an exception handling middleware?
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
comment:5 by , 6 years ago
I liked process-exception proposed by Josh Schneier. I did not test it though.
comment:6 by , 6 years ago
Replying to S. Paquette:
Replying to 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.
comment:7 by , 6 years ago
Replying to Josh Schneier:
Is there any reason that this can't be handled using an exception handling middleware?
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.)
comment:8 by , 6 years ago
- 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 how to write a patch.
Regards
comment:10 by , 5 years ago
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?)
...Middlewares intercepting this exception never generate a valid response.
I attached a project using the HandleDataTooBigMiddleware
but failing to reproduce the issue. i.e. a valid response is generated.
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"}
Tested against master
, pre-3.0, and stable/1.11.x
— same results. (1.11 needs a from django.conf.urls import url
adjustment in urls.py
...)
It would be nice to pin down the underlying issue here.
comment:11 by , 5 years ago
Owner: | changed from | to
---|---|
Patch needs improvement: | set |
Status: | new → assigned |
Version: | 1.11 → master |
comment:12 by , 5 years ago
As Carlton wasn't able to reproduce, it would be great if reporters could provide some sample project code to reproduce.
comment:13 by , 5 years ago
Resolution: | → needsinfo |
---|---|
Status: | assigned → closed |
Seems legitimate. Would you be able to write a failing test for the Django test suite?