Fixed #21608 -- Prevented logged out sessions being resurrected by concurrent requests.

Thanks Simon Charette for the review.

AUTHORS

@@ -720,6 +720,7 @@ answer newbie questions, and generally made Django that much better:
     Tom Insam
     Tommy Beadle <tbeadle@gmail.com>
     Tom Tobin
+    Tore Lundqvist <tore.lundqvist@gmail.com>
     torne-django@wolfpuppy.org.uk
     Travis Cline <travis.cline@gmail.com>
     Travis Pinney

django/contrib/sessions/backends/base.py

@@ -28,6 +28,13 @@ class CreateError(Exception):
     pass
 
 
+class UpdateError(Exception):
+    """
+    Occurs if Django tries to update a session that was deleted.
+    """
+    pass
+
+
 class SessionBase(object):
     """
     Base class for all Session classes.
@@ -301,7 +308,8 @@ class SessionBase(object):
         key = self.session_key
         self.create()
         self._session_cache = data
-        self.delete(key)
+        if key:
+            self.delete(key)
 
     # Methods that child classes must implement.
 
@@ -323,7 +331,8 @@ class SessionBase(object):
         """
         Saves the session data. If 'must_create' is True, a new session object
         is created (otherwise a CreateError exception is raised). Otherwise,
-        save() can update an existing object with the same key.
+        save() only updates an existing object and does not create one
+        (an UpdateError is raised).
         """
         raise NotImplementedError('subclasses of SessionBase must provide a save() method')
 

django/contrib/sessions/backends/cache.py

 from django.conf import settings
-from django.contrib.sessions.backends.base import CreateError, SessionBase
+from django.contrib.sessions.backends.base import (
+    CreateError, SessionBase, UpdateError,
+)
 from django.core.cache import caches
 from django.utils.six.moves import range
 
@@ -55,8 +57,10 @@ class SessionStore(SessionBase):
             return self.create()
         if must_create:
             func = self._cache.add
-        else:
+        elif self._cache.get(self.session_key) is not None:
             func = self._cache.set
+        else:
+            raise UpdateError
         result = func(self.cache_key,
                       self._get_session(no_load=must_create),
                       self.get_expiry_age())

django/contrib/sessions/backends/db.py

 import logging
 
-from django.contrib.sessions.backends.base import CreateError, SessionBase
+from django.contrib.sessions.backends.base import (
+    CreateError, SessionBase, UpdateError,
+)
 from django.core.exceptions import SuspiciousOperation
-from django.db import IntegrityError, router, transaction
+from django.db import DatabaseError, IntegrityError, router, transaction
 from django.utils import timezone
 from django.utils.encoding import force_text
 from django.utils.functional import cached_property
@@ -83,11 +85,15 @@ class SessionStore(SessionBase):
         using = router.db_for_write(self.model, instance=obj)
         try:
             with transaction.atomic(using=using):
-                obj.save(force_insert=must_create, using=using)
+                obj.save(force_insert=must_create, force_update=not must_create, using=using)
         except IntegrityError:
             if must_create:
                 raise CreateError
             raise
+        except DatabaseError:
+            if not must_create:
+                raise UpdateError
+            raise
 
     def delete(self, session_key=None):
         if session_key is None:

django/contrib/sessions/backends/file.py

@@ -7,7 +7,7 @@ import tempfile
 
 from django.conf import settings
 from django.contrib.sessions.backends.base import (
-    VALID_KEY_CHARS, CreateError, SessionBase,
+    VALID_KEY_CHARS, CreateError, SessionBase, UpdateError,
 )
 from django.contrib.sessions.exceptions import InvalidSessionKey
 from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation
@@ -129,15 +129,17 @@ class SessionStore(SessionBase):
         try:
             # Make sure the file exists.  If it does not already exist, an
             # empty placeholder file is created.
-            flags = os.O_WRONLY | os.O_CREAT | getattr(os, 'O_BINARY', 0)
+            flags = os.O_WRONLY | getattr(os, 'O_BINARY', 0)
             if must_create:
-                flags |= os.O_EXCL
+                flags |= os.O_EXCL | os.O_CREAT
             fd = os.open(session_file_name, flags)
             os.close(fd)
 
         except OSError as e:
             if must_create and e.errno == errno.EEXIST:
                 raise CreateError
+            if not must_create and e.errno == errno.ENOENT:
+                raise UpdateError
             raise
 
         # Write the session file without interfering with other threads

django/contrib/sessions/middleware.py

@@ -2,6 +2,8 @@ import time
 from importlib import import_module
 
 from django.conf import settings
+from django.contrib.sessions.backends.base import UpdateError
+from django.shortcuts import redirect
 from django.utils.cache import patch_vary_headers
 from django.utils.http import cookie_date
 
@@ -47,7 +49,13 @@ class SessionMiddleware(object):
                     # Save the session data and refresh the client cookie.
                     # Skip session save for 500 responses, refs #3881.
                     if response.status_code != 500:
-                        request.session.save()
+                        try:
+                            request.session.save()
+                        except UpdateError:
+                            # The user is now logged out; redirecting to same
+                            # page will result in a redirect to the login page
+                            # if required.
+                            return redirect(request.path)
                         response.set_cookie(settings.SESSION_COOKIE_NAME,
                                 request.session.session_key, max_age=max_age,
                                 expires=expires, domain=settings.SESSION_COOKIE_DOMAIN,

tests/defer_regress/tests.py

@@ -131,7 +131,7 @@ class DeferRegressionTest(TestCase):
         s = SessionStore(SESSION_KEY)
         s.clear()
         s["item"] = item
-        s.save()
+        s.save(must_create=True)
 
         s = SessionStore(SESSION_KEY)
         s.modified = True

tests/sessions_tests/tests.py

@@ -8,6 +8,7 @@ import unittest
 from datetime import timedelta
 
 from django.conf import settings
+from django.contrib.sessions.backends.base import UpdateError
 from django.contrib.sessions.backends.cache import SessionStore as CacheSession
 from django.contrib.sessions.backends.cached_db import \
     SessionStore as CacheDBSession
@@ -174,6 +175,7 @@ class SessionTestsMixin(object):
         prev_key = self.session.session_key
         prev_data = list(self.session.items())
         self.session.cycle_key()
+        self.assertFalse(self.session.exists(prev_key))
         self.assertNotEqual(self.session.session_key, prev_key)
         self.assertEqual(list(self.session.items()), prev_data)
 
@@ -349,6 +351,28 @@ class SessionTestsMixin(object):
         # provided unknown key was cycled, not reused
         self.assertNotEqual(session.session_key, 'someunknownkey')
 
+    def test_session_save_does_not_resurrect_session_logged_out_in_other_context(self):
+        """
+        Sessions shouldn't be resurrected by a concurrent request.
+        """
+        # Create new session.
+        s1 = self.backend()
+        s1['test_data'] = 'value1'
+        s1.save(must_create=True)
+
+        # Logout in another context.
+        s2 = self.backend(s1.session_key)
+        s2.delete()
+
+        # Modify session in first context.
+        s1['test_data'] = 'value2'
+        with self.assertRaises(UpdateError):
+            # This should throw an exception as the session is deleted, not
+            # resurrect the session.
+            s1.save()
+
+        self.assertEqual(s1.load(), {})
+
 
 class DatabaseSessionTests(SessionTestsMixin, TestCase):
 
@@ -657,6 +681,25 @@ class SessionMiddlewareTests(TestCase):
         # Check that the value wasn't saved above.
         self.assertNotIn('hello', request.session.load())
 
+    def test_session_update_error_redirect(self):
+        path = '/foo/'
+        request = RequestFactory().get(path)
+        response = HttpResponse()
+        middleware = SessionMiddleware()
+
+        request.session = DatabaseSession()
+        request.session.save(must_create=True)
+        request.session.delete()
+
+        # Handle the response through the middleware. It will try to save the
+        # deleted session which will cause an UpdateError that's caught and
+        # results in a redirect to the original page.
+        response = middleware.process_response(request, response)
+
+        # Check that the response is a redirect.
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response['Location'], path)
+
     def test_session_delete_on_end(self):
         request = RequestFactory().get('/')
         response = HttpResponse('Session test')
@@ -808,3 +851,7 @@ class CookieSessionTests(SessionTestsMixin, unittest.TestCase):
     @unittest.skip("Cookie backend doesn't have an external store to create records in.")
     def test_session_load_does_not_create_record(self):
         pass
+
+    @unittest.skip("CookieSession is stored in the client and there is no way to query it.")
+    def test_session_save_does_not_resurrect_session_logged_out_in_other_context(self):
+        pass