Opened 7 months ago

Last modified 7 months ago

#29203 new Bug

Cached Session may cause losing a session, when it failed to connect to cache backend

Reported by: Kenial Sookyum Lee Owned by: nobody
Component: contrib.sessions Version: 1.11
Severity: Normal Keywords: session cookie, cached session
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Kenial Sookyum Lee)

Some cache backends (AFAIK Memcached and Redis) has a feature to ignore connection timeout exception, in order to ensure Django application working even if cache has failed. This can lead a subtle bug, deleting a session cookie. Following steps are reproduce this bug:

  • Set cached session for Django session (refer to https://docs.djangoproject.com/en/2.0/topics/http/sessions/#using-cached-sessions)
  • On Django app, try to log in or set a language in order to make a session cookie. At this step, cache nodes should be available.
  • To set cache nodes unavailable, by changing to wrong node name or turning the nodes off.
  • Reload a page on Django app. Django app responds back with this HTTP header, which deletes a session cookie:
    Set-Cookie:  sessionid=""; expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/
    

The problematic codes are at django/contrib/sessions/middleware.py:37 :

 def process_response(self, request, response):
        """
        If request.session was modified, or if the configuration is to save the
        session every time, save the changes and set a session cookie or delete
        the session cookie if the session has been emptied.
        """
        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
            if settings.SESSION_COOKIE_NAME in request.COOKIES and empty:     # NEED TO UPDATE!
                response.delete_cookie(
                    settings.SESSION_COOKIE_NAME,
                    path=settings.SESSION_COOKIE_PATH,
                    domain=settings.SESSION_COOKIE_DOMAIN,
                )
...

I guess the initial intention was to delete a session if there is no values in it. However, it happens that code execution reaches at :37 that code without exception, even if cache nodes are unavailable. The reason is, I already explained above, they just work that way even if the cache backend failed.

I first met this bug while I was testing failover of AWS Elasticache (Redis). I was in testing of failover scenario, but Django application got me logged out repeatedly, even though session data itself is remaining in the cache replica. (it should, because it was doing failover, not reboot)

IMHO, before checking empty of session data, there should be a logic to check cache backend is actually available. I found out request.session.cache_key can do that function, but it looks less explicit. Please show be a better way to do this, if you have one.

fyi. Configuration is: Django 1.11, MySQL 5.6.35, mysqlclient 1.3.12, Redis 3.2.7 (x64), and django-redis 4.9.0. I found out this bug on Django 1.11 though, but it has not been changed since, so this bug must happen on Django 2.x as well.

Attachments (1)

patch_cached_session_deleting_session_cookie.diff (1.5 KB) - added by Kenial Sookyum Lee 7 months ago.
workaround patch

Download all attachments as: .zip

Change History (12)

Changed 7 months ago by Kenial Sookyum Lee

workaround patch

comment:1 Changed 7 months ago by Kenial Sookyum Lee

Type: UncategorizedBug

comment:2 Changed 7 months ago by Kenial Sookyum Lee

Description: modified (diff)

comment:3 Changed 7 months ago by Kenial Sookyum Lee

Summary: Cached Session may cause losing a session, when it failed to connect backend cacheCached Session may cause losing a session, when it failed to connect to cache backend

comment:4 Changed 7 months ago by Kenial Sookyum Lee

Description: modified (diff)

comment:5 Changed 7 months ago by Kenial Sookyum Lee

Description: modified (diff)

comment:6 Changed 7 months ago by Kenial Sookyum Lee

When I tested the settings with SESSION_ENGINE = "django.contrib.sessions.backends.cached_db" (ie. 'write-through cache'), I didn't get through this bug. Configuration is: Django 1.11, MySQL 5.6.35, mysqlclient 1.3.12, Redis 3.2.7 (x64), django-redis 4.9.0.

comment:7 Changed 7 months ago by Kenial Sookyum Lee

Description: modified (diff)
Version: master1.11

comment:8 Changed 7 months ago by Kenial Sookyum Lee

Description: modified (diff)

comment:9 Changed 7 months ago by Tim Graham

Has patch: unset

I'm not sure if this is something that Django should implement. The patch doesn't look elegant because it's adding an implementation detail (cache_key) of a couple of the backends to code that's designed for all backends.

As far as I know, Django generally requires the cache to be available. It might be that we should document this requirement and discourage use of the "ignore connection timeout exception" option that you mentioned.

comment:10 in reply to:  9 Changed 7 months ago by Kenial Sookyum Lee

Replying to Tim Graham:

I'm not sure if this is something that Django should implement. The patch doesn't look elegant because it's adding an implementation detail (cache_key) of a couple of the backends to code that's designed for all backends.

As long as there is another way to check if cache backend is available, it's okay. Any idea?

As far as I know, Django generally requires the cache to be available. It might be that we should document this requirement and discourage use of the "ignore connection timeout exception" option that you mentioned.

Agreed. But, the thing is, cache downtime itself is inevitable. As I told you, I found out this bug when I tested failover scenario of AWS Elasticache - it means, this can happen during any kind of cache backend failover scenario. If "CAUTION: failover of cache backend might cause termination of Django sessions unintentionally" is in documentation, well... It sounds awkward.

Last edited 7 months ago by Kenial Sookyum Lee (previous) (diff)

comment:11 Changed 7 months ago by Tim Graham

Triage Stage: UnreviewedAccepted

I'm not sure what to do, but your report seems credible.

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