Opened 4 years ago

Closed 4 years ago

#31962 closed Cleanup/optimization (fixed)

Raise a more appriopriate exception when session cannot be updated.

Reported by: Alex Vandiver Owned by: Hasan Ramezani
Component: contrib.sessions Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

django/contrib/sessions/middleware.py contains the only non-test place where a bare SuspiciousOperation is raised; all other locations use one of its more specific subclasses. Making this suspicious operation that involves a session into a raise SuspiciousSession would remove this outlier, and make the exception more specific.

This is also notable because every SuspiciousOperation subclass gets its own logger, so this is the only place that is logged via the django.security.SuspiciousOperation logger.

This should only require:

diff --git django/contrib/sessions/middleware.py django/contrib/sessions/middleware.py
index cb8c1ff45b..f8386a21ce 100644
--- django/contrib/sessions/middleware.py
+++ 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 SuspiciousSession
 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 SuspiciousSession(
                             "The request's session was deleted before the "
                             "request completed. The user may have logged "
                             "out in a concurrent request, for example."

Change History (7)

comment:1 by Mariusz Felisiak, 4 years ago

Easy pickings: unset
Has patch: unset
Summary: Raise of SuspiciousOperation should use more specific SuspiciousSession classRaise a more appriopriate exception when session cannot be updated.
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

SuspiciousSession is better than SuspiciousOperation, but it's not a big improvement because this error is not really caused by user and there is nothing suspicious in it (see #27363). I think we should use a new exception class as proposed in the original PR.

comment:2 by Hasan Ramezani, 4 years ago

Owner: changed from nobody to Hasan Ramezani
Status: newassigned

@felixxm

I think we should use a new exception class

What do you think about SessionInterrupted as new exception class name?

comment:3 by Hasan Ramezani, 4 years ago

Has patch: set

comment:4 by Mariusz Felisiak, 4 years ago

Needs tests: set
Patch needs improvement: set

comment:5 by Hasan Ramezani, 4 years ago

Needs tests: unset
Patch needs improvement: unset

comment:6 by Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 2808cdc:

Fixed #31962 -- Made SessionMiddleware raise SessionInterrupted when session destroyed while request is processing.

Note: See TracTickets for help on using tickets.
Back to Top