Keep confirm_human from overwriting concurrent session writes

.github/workflows/tests.yml

@@ -12,7 +12,9 @@ jobs:
     strategy:
       max-parallel: 5
       matrix:
-        python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', '3.12', 'pypy-3.8', 'pypy-3.9', 'pypy-3.10']
+        # 3.7 is gone from the ubuntu-latest runner images (setup-python:
+        #   "Version 3.7 with arch x64 not found").
+        python-version: ['3.8', '3.9', '3.10', '3.11', '3.12', '3.13', 'pypy-3.8', 'pypy-3.9', 'pypy-3.10']
       fail-fast: false
 
     steps:

experiments/apps.py

@@ -4,6 +4,9 @@
 class ExperimentsConfig(AppConfig):
     name = 'experiments'
     label = 'experiments'
+    # Django >= 6 defaults new projects to BigAutoField; pin the historical
+    # AutoField so existing installations are not asked for an id migration.
+    default_auto_field = 'django.db.models.AutoField'
 
     def ready(self):
         from django.contrib.auth.signals import user_logged_in, user_logged_out

experiments/tests/test_views.py

@@ -0,0 +1,104 @@
+from __future__ import absolute_import
+
+from django.contrib.sessions.backends.db import SessionStore as DatabaseSession
+from django.test import TestCase, override_settings
+from django.urls import reverse
+
+from experiments import conf
+from experiments.utils import WebUser
+
+from mock import patch
+
+OAUTH_STATE_KEY = 'oauth_state'
+OAUTH_STATE = 'state-written-by-a-concurrent-request'
+
+
+class ConfirmHumanViewTest(TestCase):
+    def setUp(self):
+        """Store a session with a seeded key and point the test client at it."""
+        session = DatabaseSession()
+        session['seeded'] = 'before'
+        session.save()
+        self.session_key = session.session_key
+        self.client.cookies['sessionid'] = self.session_key
+        self.url = reverse('experiment_confirm_human')
+
+    def stored(self):
+        """Re-read the session from the store, bypassing the request copy."""
+        return DatabaseSession(session_key=self.session_key)
+
+    def test_get_is_rejected(self):
+        """The endpoint stays POST-only."""
+        response = self.client.get(self.url)
+        self.assertEqual(response.status_code, 405)
+
+    def test_post_marks_the_session_as_human(self):
+        """The primary effect: the confirmed-human flag lands in the session."""
+        response = self.client.post(self.url)
+        self.assertEqual(response.status_code, 204)
+        self.assertTrue(self.stored().get(conf.CONFIRM_HUMAN_SESSION_KEY))
+
+    def test_leaves_untouched_keys_alone(self):
+        """Keys confirm_human never touched keep their stored values."""
+        self.client.post(self.url)
+        self.assertEqual(self.stored().get('seeded'), 'before')
+
+    def test_does_not_drop_a_concurrent_session_write(self):
+        """A write landing while confirm_human replays counters must survive.
+
+        confirm_human() can spend seconds replaying enrollments and goals to
+        the counter store. A concurrent request writing to the same session in
+        that window (an OAuth login storing its state, for example) used to be
+        overwritten when this request's stale session snapshot was saved at the
+        end of the request. The concurrent write is injected inside
+        confirm_human(), which is exactly where such requests land.
+        """
+        original = WebUser.confirm_human
+
+        def concurrent_write_then_confirm(user):
+            concurrent = DatabaseSession(session_key=self.session_key)
+            concurrent[OAUTH_STATE_KEY] = OAUTH_STATE
+            concurrent.save()
+            return original(user)
+
+        with patch.object(WebUser, 'confirm_human', concurrent_write_then_confirm):
+            response = self.client.post(self.url)
+
+        self.assertEqual(response.status_code, 204)
+        stored = self.stored()
+        self.assertEqual(stored.get(OAUTH_STATE_KEY), OAUTH_STATE)
+        self.assertTrue(stored.get(conf.CONFIRM_HUMAN_SESSION_KEY))
+
+    def test_repeat_ping_makes_no_session_write(self):
+        """A ping that changes nothing must not write the session at all."""
+        self.client.post(self.url)
+        with patch.object(DatabaseSession, 'save') as save:
+            self.client.post(self.url)
+        save.assert_not_called()
+
+    def test_without_a_session_cookie_it_still_answers(self):
+        """A cookieless request (bot, first hit) is answered, not crashed."""
+        del self.client.cookies['sessionid']
+        response = self.client.post(self.url)
+        self.assertEqual(response.status_code, 204)
+
+    def test_concurrent_write_survives_save_every_request(self):
+        """SESSION_SAVE_EVERY_REQUEST makes the middleware save even an
+        unmodified session, so the merged state must also be what the request's
+        own session object holds by the time the middleware runs."""
+        original = WebUser.confirm_human
+
+        def concurrent_write_then_confirm(user):
+            concurrent = DatabaseSession(session_key=self.session_key)
+            concurrent[OAUTH_STATE_KEY] = OAUTH_STATE
+            concurrent.save()
+            return original(user)
+
+        with override_settings(SESSION_SAVE_EVERY_REQUEST=True):
+            with patch.object(WebUser, 'confirm_human', concurrent_write_then_confirm):
+                response = self.client.post(self.url)
+
+        self.assertEqual(response.status_code, 204)
+        stored = self.stored()
+        self.assertEqual(stored.get(OAUTH_STATE_KEY), OAUTH_STATE)
+        self.assertTrue(stored.get(conf.CONFIRM_HUMAN_SESSION_KEY))

experiments/utils.py

@@ -54,7 +54,7 @@ def clear_participant_cache(request):
 def _get_participant(request, session, user):
     if request and hasattr(request, 'user') and not user:
         user = request.user
-    if request and hasattr(request, 'session') and not session:
+    if request and hasattr(request, 'session') and session is None:
         session = request.session
 
     if request and conf.BOT_REGEX.search(request.META.get("HTTP_USER_AGENT", "")):
@@ -64,7 +64,10 @@ def _get_participant(request, session, user):
             return WebUser(user=user, request=request)
         else:
             return DummyUser()
-    elif session:
+    elif session is not None:
+        # Truthiness is not identity here: since Django 6.1 an *empty* session
+        # is falsy, and a fresh visitor's empty session must still get a
+        # WebUser, not a DummyUser.
         return WebUser(session=session, request=request)
     else:
         return DummyUser()
@@ -409,7 +412,10 @@ def _is_verified_human(self):
 
     @property
     def _session_key(self):
-        if not self.session:
+        # `is None`, not truthiness: since Django 6.1 an *empty* session is
+        # falsy, and returning None for every fresh visitor would key all of
+        # their enrollments and counters to the same identifier.
+        if self.session is None:
             return None
         if 'experiments_session_key' not in self.session:
             if not self.session.session_key:

experiments/views.py

@@ -1,3 +1,4 @@
+from django.contrib.sessions.backends.signed_cookies import SessionStore as SignedCookiesStore
 from django.http import HttpResponse, HttpResponseBadRequest
 from django.views.decorators.cache import never_cache
 from django.shortcuts import get_object_or_404
@@ -19,15 +20,71 @@
  "\x00\x49\x45\x4e\x44\xae\x42\x60\x82\x00")
 
 
+_MISSING = object()
+
+
 @never_cache
 @require_POST
 def confirm_human(request):
-    if conf.CONFIRM_HUMAN:
-        experiment_user = participant(request)
-        experiment_user.confirm_human()
+    """Mark the session as belonging to a human, without clobbering the session.
+
+    See _save_only_confirm_human_changes for why the session handling is not
+    left to the middleware.
+    """
+    if not conf.CONFIRM_HUMAN:
+        return HttpResponse(status=204)
+
+    session = getattr(request, 'session', None)
+    before = dict(session.items()) if session is not None else {}
+
+    experiment_user = participant(request)
+    experiment_user.confirm_human()
+
+    _save_only_confirm_human_changes(session, before)
     return HttpResponse(status=204)
 
 
+def _save_only_confirm_human_changes(session, before):
+    """Persist this request's session changes without dropping concurrent ones.
+
+    confirm_human() replays the participant's enrollments and goals - a counter
+    round trip each - between writing its session flag and the session being
+    saved at the end of the request. That can take seconds, and the middleware
+    then writes back the whole session dict as it looked when this request
+    loaded it: a concurrent request that wrote to the same session in the
+    meantime (an OAuth login storing its state, for example) is silently
+    overwritten by this request's stale snapshot.
+
+    Instead, re-read the stored session, write only the keys confirm_human
+    changed, and keep the middleware from saving the stale snapshot.
+    """
+    if session is None or not session.session_key:
+        # No stored session yet, so there is nothing to race with - let the
+        # middleware create and save the session as it normally would.
+        return
+    if isinstance(session, SignedCookiesStore):
+        # Cookie-backed sessions have no server-side store to race on;
+        # persistence is the response cookie the middleware writes.
+        return
+
+    changed = {key: value for key, value in session.items() if before.get(key, _MISSING) != value}
+
+    fresh = type(session)(session_key=session.session_key)
+    if changed:
+        for key, value in changed.items():
+            fresh[key] = value
+        fresh.save()
+
+    # The middleware must not write this request's stale snapshot over the
+    # merge above. With SESSION_SAVE_EVERY_REQUEST=True it saves even an
+    # unmodified session, so the request's session object is also pointed at
+    # the merged state - whatever the middleware does, it persists that.
+    # (_session_cache is the only way to replace the contents without
+    # marking the session dirty.)
+    session._session_cache = dict(fresh.items())
+    session.modified = False
+
+
 @never_cache
 def record_experiment_goal(request, goal_name, cache_buster=None):
     participant(request).goal(goal_name)

tox.ini

@@ -17,6 +17,8 @@ envlist =
     {py,pypy}{38,39,310,311}-django{4.1}
     {py,pypy}{38,39,310,311,312}-django{4.2}
     {py,pypy}{310,311,312}-django{5.0}
+    {py,pypy}{310,311,312,313}-django{5.1,5.2}
+    py{312,313}-django{6.0,6.1}
 
 [gh-actions]
 python =
@@ -27,6 +29,8 @@ python =
     3.9: py39
     3.10: py310
     3.11: py311
+    3.12: py312
+    3.13: py313
     pypy-3.8: pypy38
     pypy-3.9: pypy39
     pypy-3.10: pypy310
@@ -51,3 +55,7 @@ deps =
     django4.1: Django==4.1.*
     django4.2: Django==4.2.*
     django5.0: Django==5.0.*
+    django5.1: Django==5.1.*
+    django5.2: Django==5.2.*
+    django6.0: Django==6.0.*
+    django6.1: Django==6.1.*