Remove session fallback cookie (#11673)

We set the fallback cookie to work around some old browsers rejecting cookies with `SameSite=None`
https://www.chromium.org/updates/same-site/incompatible-clients/.
That note is from 2019, I think we should be fine without supporting those old browsers,
and browsers from supported devices are already patched to support that cookie.

This is adding an additional cookie that takes some space.

Author: stsewd

readthedocs/core/middleware.py

@@ -1,17 +1,9 @@
-import time
-
 import structlog
 from django.conf import settings
-from django.contrib.sessions.backends.base import SessionBase, UpdateError
+from django.contrib.sessions.backends.base import SessionBase
 from django.contrib.sessions.middleware import SessionMiddleware
-from django.core.exceptions import (
-    ImproperlyConfigured,
-    MiddlewareNotUsed,
-    SuspiciousOperation,
-)
+from django.core.exceptions import ImproperlyConfigured, MiddlewareNotUsed
 from django.http import HttpResponse
-from django.utils.cache import patch_vary_headers
-from django.utils.http import http_date
 
 log = structlog.get_logger(__name__)
 
@@ -22,17 +14,6 @@ class ReadTheDocsSessionMiddleware(SessionMiddleware):
     An overridden session middleware with a few changes.
 
     - Doesn't create a session on logged out doc views.
-    - Uses a fallback cookie for browsers that reject SameSite=None cookies
-    - Modifies Django's behavior of treating SESSION_COOKIE_SAMESITE=None
-      to mean SameSite unset and instead makes it mean SameSite=None
-
-    This overrides and replaces Django's built-in SessionMiddleware completely.
-    Much of this middleware is duplicated from Django 2.2's SessionMiddleware.
-
-    In Django 3.1, Django will fully support SameSite=None cookies.
-    However, we may still need this middleware to support browsers that reject
-    SameSite=None cookies.
-    https://www.chromium.org/updates/same-site/incompatible-clients
     """
 
     # Don't set a session cookie on these URLs unless the cookie is already set
@@ -42,125 +23,27 @@ class ReadTheDocsSessionMiddleware(SessionMiddleware):
         "/sustainability/click",
     ]
 
-    # This is a fallback cookie for the regular session cookie
-    # It is only used by clients that reject cookies with `SameSite=None`
-    cookie_name_fallback = f"{settings.SESSION_COOKIE_NAME}-samesiteunset"
-
     def process_request(self, request):
         for url in self.IGNORE_URLS:
             if (
                 request.path_info.startswith(url)
                 and settings.SESSION_COOKIE_NAME not in request.COOKIES
-                and self.cookie_name_fallback not in request.COOKIES
             ):
                 # Hack request.session otherwise the Authentication middleware complains.
                 request.session = SessionBase()  # create an empty session
                 return
 
-        if settings.SESSION_COOKIE_SAMESITE == "None":
-            if settings.SESSION_COOKIE_NAME in request.COOKIES:
-                session_key = request.COOKIES.get(settings.SESSION_COOKIE_NAME)
-            else:
-                session_key = request.COOKIES.get(self.cookie_name_fallback)
-
-            request.session = self.SessionStore(session_key)
-        else:
-            super().process_request(request)
+        super().process_request(request)
 
     def process_response(self, request, response):
         for url in self.IGNORE_URLS:
             if (
                 request.path_info.startswith(url)
                 and settings.SESSION_COOKIE_NAME not in request.COOKIES
-                and self.cookie_name_fallback not in request.COOKIES
             ):
                 return response
 
-        # Most of the code below is taken directly from Django's SessionMiddleware.
-        # Some changes (marked with NOTE:) were added to support the fallback cookie.
-
-        try:
-            accessed = request.session.accessed
-            modified = request.session.modified
-            empty = request.session.is_empty()
-        except AttributeError:
-            pass
-        else:
-            # First check if we need to delete this cookie.
-            # The session should be deleted only if the session is entirely empty
-            # NOTE: This was changed to support both cookies
-            if (
-                settings.SESSION_COOKIE_NAME in request.COOKIES
-                or self.cookie_name_fallback in request.COOKIES
-            ) and empty:
-                for cookie_name in (
-                    settings.SESSION_COOKIE_NAME,
-                    self.cookie_name_fallback,
-                ):
-                    if cookie_name in request.COOKIES:
-                        response.delete_cookie(
-                            cookie_name,
-                            path=settings.SESSION_COOKIE_PATH,
-                            domain=settings.SESSION_COOKIE_DOMAIN,
-                        )
-            else:
-                if accessed:
-                    patch_vary_headers(response, ("Cookie",))
-                if (modified or settings.SESSION_SAVE_EVERY_REQUEST) and not empty:
-                    if request.session.get_expire_at_browser_close():
-                        max_age = None
-                        expires = None
-                    else:
-                        max_age = request.session.get_expiry_age()
-                        expires_time = time.time() + max_age
-                        expires = http_date(expires_time)
-                    # Save the session data and refresh the client cookie.
-                    # Skip session save for 500 responses, refs #3881.
-                    if response.status_code != 500:
-                        try:
-                            request.session.save()
-                        except UpdateError:
-                            raise SuspiciousOperation(
-                                "The request's session was deleted before the "
-                                "request completed. The user may have logged "
-                                "out in a concurrent request, for example."
-                            )
-
-                        response.set_cookie(
-                            settings.SESSION_COOKIE_NAME,
-                            request.session.session_key,
-                            max_age=max_age,
-                            expires=expires,
-                            domain=settings.SESSION_COOKIE_DOMAIN,
-                            path=settings.SESSION_COOKIE_PATH,
-                            secure=settings.SESSION_COOKIE_SECURE or None,
-                            httponly=settings.SESSION_COOKIE_HTTPONLY or None,
-                            samesite=settings.SESSION_COOKIE_SAMESITE,
-                        )
-
-                        # NOTE: This was added to support the fallback cookie
-                        if settings.SESSION_COOKIE_SAMESITE == "None":
-                            # Forcibly set the session cookie to SameSite=None
-                            # This isn't supported in Django<3.1
-                            # https://github.com/django/django/pull/11894
-                            response.cookies[settings.SESSION_COOKIE_NAME][
-                                "samesite"
-                            ] = "None"
-
-                            # Set the fallback cookie in case the above cookie is rejected
-                            response.set_cookie(
-                                self.cookie_name_fallback,
-                                request.session.session_key,
-                                max_age=max_age,
-                                expires=expires,
-                                domain=settings.SESSION_COOKIE_DOMAIN,
-                                path=settings.SESSION_COOKIE_PATH,
-                                secure=settings.SESSION_COOKIE_SECURE or None,
-                                httponly=settings.SESSION_COOKIE_HTTPONLY or None,
-                                # Use browser default in case SameSite=None is rejected.
-                                samesite=False,
-                            )
-        return response
+        return super().process_response(request, response)
 
 
 class ReferrerPolicyMiddleware:

readthedocs/rtd_tests/tests/test_middleware.py

@@ -250,17 +250,6 @@ def setUp(self):
 
         self.user = create_user(username="owner", password="test")
 
-    @override_settings(SESSION_COOKIE_SAMESITE="None")
-    def test_fallback_cookie(self):
-        request = self.factory.get("/")
-        response = HttpResponse()
-        self.middleware.process_request(request)
-        request.session["test"] = "value"
-        response = self.middleware.process_response(request, response)
-
-        self.assertTrue(settings.SESSION_COOKIE_NAME in response.cookies)
-        self.assertTrue(self.middleware.cookie_name_fallback in response.cookies)
-
     @override_settings(SESSION_COOKIE_SAMESITE="None")
     def test_main_cookie_samesite_none(self):
         request = self.factory.get("/")
@@ -272,9 +261,6 @@ def test_main_cookie_samesite_none(self):
         self.assertEqual(
             response.cookies[settings.SESSION_COOKIE_NAME]["samesite"], "None"
         )
-        self.assertEqual(
-            response.cookies[self.middleware.cookie_name_fallback]["samesite"], ""
-        )
 
     def test_main_cookie_samesite_lax(self):
         request = self.factory.get("/")
@@ -286,7 +272,6 @@ def test_main_cookie_samesite_lax(self):
         self.assertEqual(
             response.cookies[settings.SESSION_COOKIE_NAME]["samesite"], "Lax"
         )
-        self.assertTrue(self.test_main_cookie_samesite_none not in response.cookies)
 
 
 class TestNullCharactersMiddleware(TestCase):