From 7650b5a972d0d87d712e025cf44d4560a021095a Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Wed, 9 Mar 2022 01:41:27 +0000 Subject: [PATCH] session: Enforce that changes cannot happen in a transaction. --- tools/test-backend | 2 ++ zerver/lib/cache_helpers.py | 2 +- zerver/lib/safe_session_cached_db.py | 25 +++++++++++++++++++++++++ zproject/computed_settings.py | 2 +- 4 files changed, 29 insertions(+), 2 deletions(-) create mode 100644 zerver/lib/safe_session_cached_db.py diff --git a/tools/test-backend b/tools/test-backend index dc9d797520..6de73db69a 100755 --- a/tools/test-backend +++ b/tools/test-backend @@ -140,6 +140,8 @@ not_yet_fully_covered = [ "zerver/webhooks/teamcity/view.py", "zerver/webhooks/travis/view.py", "zerver/webhooks/zapier/view.py", + # Cannot have coverage, as tests run in a transaction + "zerver/lib/safe_session_cached_db.py", ] enforce_fully_covered = sorted( diff --git a/zerver/lib/cache_helpers.py b/zerver/lib/cache_helpers.py index 405fd61a89..e0244fe48e 100644 --- a/zerver/lib/cache_helpers.py +++ b/zerver/lib/cache_helpers.py @@ -56,7 +56,7 @@ def huddle_cache_items(items_for_remote_cache: Dict[str, Tuple[Huddle]], huddle: def session_cache_items(items_for_remote_cache: Dict[str, str], session: Session) -> None: - if settings.SESSION_ENGINE != "django.contrib.sessions.backends.cached_db": + if settings.SESSION_ENGINE != "zerver.lib.safe_session_cached_db": # If we're not using the cached_db session engine, we there # will be no store.cache_key attribute, and in any case we # don't need to fill the cache, since it won't exist. diff --git a/zerver/lib/safe_session_cached_db.py b/zerver/lib/safe_session_cached_db.py new file mode 100644 index 0000000000..1d4bd5a667 --- /dev/null +++ b/zerver/lib/safe_session_cached_db.py @@ -0,0 +1,25 @@ +from typing import Optional + +from django.contrib.sessions.backends.cached_db import SessionStore as CachedDbSessionStore +from django.db.transaction import get_connection + + +class SessionStore(CachedDbSessionStore): + """Caching session object which does not leak into the cache. + + django.contrib.sessions.backends.cached_db does write-through to + the cache and the backing database. If the database is in a + transaction, this may leak not-yet-committed changes to the cache, + which can lead to inconsistent state. This class wraps changes to + the session in assertions which enforce that the database cannot + be in a transaction before writing. + + """ + + def save(self, must_create: bool = False) -> None: + assert not get_connection().in_atomic_block + super().save(must_create) + + def delete(self, session_key: Optional[str] = None) -> None: + assert not get_connection().in_atomic_block + super().delete(session_key) diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index f908a7d2c2..36ba2935da 100644 --- a/zproject/computed_settings.py +++ b/zproject/computed_settings.py @@ -334,7 +334,7 @@ RABBITMQ_PASSWORD = get_secret("rabbitmq_password") # CACHING CONFIGURATION ######################################################################## -SESSION_ENGINE = "django.contrib.sessions.backends.cached_db" +SESSION_ENGINE = "zerver.lib.safe_session_cached_db" MEMCACHED_PASSWORD = get_secret("memcached_password")