From 2808cdc8fb15ad27f83af3e62db69f5ea7ced29e Mon Sep 17 00:00:00 2001 From: Hasan Ramezani Date: Mon, 7 Sep 2020 13:33:47 +0200 Subject: [PATCH] Fixed #31962 -- Made SessionMiddleware raise SessionInterrupted when session destroyed while request is processing. --- django/contrib/sessions/exceptions.py | 7 ++++++- django/contrib/sessions/middleware.py | 4 ++-- django/core/exceptions.py | 5 +++++ django/core/handlers/exception.py | 13 +++++++++++- docs/ref/exceptions.txt | 30 +++++++++++++++++++++++++++ docs/releases/3.2.txt | 5 +++++ tests/handlers/tests.py | 8 +++++++ tests/handlers/urls.py | 1 + tests/handlers/views.py | 6 +++++- tests/sessions_tests/tests.py | 10 +++++---- tests/view_tests/tests/test_debug.py | 20 ++++++++++++++++++ tests/view_tests/urls.py | 1 + tests/view_tests/views.py | 8 ++++++- 13 files changed, 108 insertions(+), 10 deletions(-) diff --git a/django/contrib/sessions/exceptions.py b/django/contrib/sessions/exceptions.py index 4f4dc6b048..8dbca3d04f 100644 --- a/django/contrib/sessions/exceptions.py +++ b/django/contrib/sessions/exceptions.py @@ -1,4 +1,4 @@ -from django.core.exceptions import SuspiciousOperation +from django.core.exceptions import BadRequest, SuspiciousOperation class InvalidSessionKey(SuspiciousOperation): @@ -9,3 +9,8 @@ class InvalidSessionKey(SuspiciousOperation): class SuspiciousSession(SuspiciousOperation): """The session may be tampered with""" pass + + +class SessionInterrupted(BadRequest): + """The session was interrupted.""" + pass diff --git a/django/contrib/sessions/middleware.py b/django/contrib/sessions/middleware.py index cb8c1ff45b..50627a663b 100644 --- a/django/contrib/sessions/middleware.py +++ b/django/contrib/sessions/middleware.py @@ -3,7 +3,7 @@ from importlib import import_module from django.conf import settings from django.contrib.sessions.backends.base import UpdateError -from django.core.exceptions import SuspiciousOperation +from django.contrib.sessions.exceptions import SessionInterrupted from django.utils.cache import patch_vary_headers from django.utils.deprecation import MiddlewareMixin from django.utils.http import http_date @@ -60,7 +60,7 @@ class SessionMiddleware(MiddlewareMixin): try: request.session.save() except UpdateError: - raise SuspiciousOperation( + raise SessionInterrupted( "The request's session was deleted before the " "request completed. The user may have logged " "out in a concurrent request, for example." diff --git a/django/core/exceptions.py b/django/core/exceptions.py index 2b3e55fe49..5780ffdb4a 100644 --- a/django/core/exceptions.py +++ b/django/core/exceptions.py @@ -71,6 +71,11 @@ class RequestAborted(Exception): pass +class BadRequest(Exception): + """The request is malformed and cannot be processed.""" + pass + + class PermissionDenied(Exception): """The user did not have permission to do that""" pass diff --git a/django/core/handlers/exception.py b/django/core/handlers/exception.py index 98fb46083a..70b23edb62 100644 --- a/django/core/handlers/exception.py +++ b/django/core/handlers/exception.py @@ -8,7 +8,7 @@ from asgiref.sync import sync_to_async from django.conf import settings from django.core import signals from django.core.exceptions import ( - PermissionDenied, RequestDataTooBig, SuspiciousOperation, + BadRequest, PermissionDenied, RequestDataTooBig, SuspiciousOperation, TooManyFieldsSent, ) from django.http import Http404 @@ -76,6 +76,17 @@ def response_for_exception(request, exc): exc_info=sys.exc_info(), ) + elif isinstance(exc, BadRequest): + if settings.DEBUG: + response = debug.technical_500_response(request, *sys.exc_info(), status_code=400) + else: + response = get_exception_response(request, get_resolver(get_urlconf()), 400, exc) + log_response( + '%s: %s', str(exc), request.path, + response=response, + request=request, + exc_info=sys.exc_info(), + ) elif isinstance(exc, SuspiciousOperation): if isinstance(exc, (RequestDataTooBig, TooManyFieldsSent)): # POST data can't be accessed again, otherwise the original diff --git a/docs/ref/exceptions.txt b/docs/ref/exceptions.txt index ae2180ff9f..d4af1de79d 100644 --- a/docs/ref/exceptions.txt +++ b/docs/ref/exceptions.txt @@ -162,6 +162,18 @@ or model are classified as ``NON_FIELD_ERRORS``. This constant is used as a key in dictionaries that otherwise map fields to their respective list of errors. +``BadRequest`` +-------------- + +.. exception:: BadRequest + + .. versionadded:: 3.2 + + The :exc:`BadRequest` exception is raised when the request cannot be + processed due to a client error. If a ``BadRequest`` exception reaches the + ASGI/WSGI handler level it results in a + :class:`~django.http.HttpResponseBadRequest`. + ``RequestAborted`` ------------------ @@ -271,6 +283,24 @@ Http exceptions may be imported from ``django.http``. :exc:`UnreadablePostError` is raised when a user cancels an upload. +.. currentmodule:: django.contrib.sessions.exceptions + +Sessions Exceptions +=================== + +Sessions exceptions are defined in ``django.contrib.sessions.exceptions``. + +``SessionInterrupted`` +---------------------- + +.. exception:: SessionInterrupted + + .. versionadded:: 3.2 + + :exc:`SessionInterrupted` is raised when a session is destroyed in a + concurrent request. It's a subclass of + :exc:`~django.core.exceptions.BadRequest`. + Transaction Exceptions ====================== diff --git a/docs/releases/3.2.txt b/docs/releases/3.2.txt index a7bd5575d1..63d6f0700d 100644 --- a/docs/releases/3.2.txt +++ b/docs/releases/3.2.txt @@ -489,6 +489,11 @@ Miscellaneous :py:meth:`~unittest.TestCase.setUp` method are called before ``TestContextDecorator.disable()``. +* ``SessionMiddleware`` now raises a + :exc:`~django.contrib.sessions.exceptions.SessionInterrupted` exception + instead of :exc:`~django.core.exceptions.SuspiciousOperation` when a session + is destroyed in a concurrent request. + .. _deprecated-features-3.2: Features deprecated in 3.2 diff --git a/tests/handlers/tests.py b/tests/handlers/tests.py index 3b655a91c2..1d445cd38c 100644 --- a/tests/handlers/tests.py +++ b/tests/handlers/tests.py @@ -176,6 +176,10 @@ class HandlerRequestTests(SimpleTestCase): response = self.client.get('/suspicious/') self.assertEqual(response.status_code, 400) + def test_bad_request_in_view_returns_400(self): + response = self.client.get('/bad_request/') + self.assertEqual(response.status_code, 400) + def test_invalid_urls(self): response = self.client.get('~%A9helloworld') self.assertEqual(response.status_code, 404) @@ -259,6 +263,10 @@ class AsyncHandlerRequestTests(SimpleTestCase): response = await self.async_client.get('/suspicious/') self.assertEqual(response.status_code, 400) + async def test_bad_request_in_view_returns_400(self): + response = await self.async_client.get('/bad_request/') + self.assertEqual(response.status_code, 400) + async def test_no_response(self): msg = ( "The view handlers.views.no_response didn't return an " diff --git a/tests/handlers/urls.py b/tests/handlers/urls.py index a438da55b4..e287e3734e 100644 --- a/tests/handlers/urls.py +++ b/tests/handlers/urls.py @@ -10,6 +10,7 @@ urlpatterns = [ path('streaming/', views.streaming), path('in_transaction/', views.in_transaction), path('not_in_transaction/', views.not_in_transaction), + path('bad_request/', views.bad_request), path('suspicious/', views.suspicious), path('malformed_post/', views.malformed_post), path('httpstatus_enum/', views.httpstatus_enum), diff --git a/tests/handlers/views.py b/tests/handlers/views.py index 6d573c5df8..739aa2de40 100644 --- a/tests/handlers/views.py +++ b/tests/handlers/views.py @@ -1,7 +1,7 @@ import asyncio from http import HTTPStatus -from django.core.exceptions import SuspiciousOperation +from django.core.exceptions import BadRequest, SuspiciousOperation from django.db import connection, transaction from django.http import HttpResponse, StreamingHttpResponse from django.views.decorators.csrf import csrf_exempt @@ -33,6 +33,10 @@ def not_in_transaction(request): return HttpResponse(str(connection.in_atomic_block)) +def bad_request(request): + raise BadRequest() + + def suspicious(request): raise SuspiciousOperation('dubious') diff --git a/tests/sessions_tests/tests.py b/tests/sessions_tests/tests.py index 29b58073e1..11adef9d75 100644 --- a/tests/sessions_tests/tests.py +++ b/tests/sessions_tests/tests.py @@ -19,7 +19,9 @@ from django.contrib.sessions.backends.file import SessionStore as FileSession from django.contrib.sessions.backends.signed_cookies import ( SessionStore as CookieSession, ) -from django.contrib.sessions.exceptions import InvalidSessionKey +from django.contrib.sessions.exceptions import ( + InvalidSessionKey, SessionInterrupted, +) from django.contrib.sessions.middleware import SessionMiddleware from django.contrib.sessions.models import Session from django.contrib.sessions.serializers import ( @@ -28,7 +30,7 @@ from django.contrib.sessions.serializers import ( from django.core import management from django.core.cache import caches from django.core.cache.backends.base import InvalidCacheBackendError -from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation +from django.core.exceptions import ImproperlyConfigured from django.http import HttpResponse from django.test import ( RequestFactory, SimpleTestCase, TestCase, ignore_warnings, @@ -746,10 +748,10 @@ class SessionMiddlewareTests(TestCase): "The request's session was deleted before the request completed. " "The user may have logged out in a concurrent request, for example." ) - with self.assertRaisesMessage(SuspiciousOperation, msg): + with self.assertRaisesMessage(SessionInterrupted, msg): # Handle the response through the middleware. It will try to save # the deleted session which will cause an UpdateError that's caught - # and raised as a SuspiciousOperation. + # and raised as a SessionInterrupted. middleware(request) def test_session_delete_on_end(self): diff --git a/tests/view_tests/tests/test_debug.py b/tests/view_tests/tests/test_debug.py index 6d1b3e6f48..2337d3ed3d 100644 --- a/tests/view_tests/tests/test_debug.py +++ b/tests/view_tests/tests/test_debug.py @@ -86,6 +86,16 @@ class DebugViewTests(SimpleTestCase): response = self.client.get('/raises400/') self.assertContains(response, '