Opened 3 years ago

Closed 3 years ago

#17810 closed Bug (fixed)

Switching from cookie-based sessions to memcached-based sessions raises exception

Reported by: gabrielhurley Owned by: nobody
Component: contrib.sessions Version: 1.4-beta-1
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When using the cookie-based session backend, the entire session cookie is stuffed into the session name. If you then switch the session backend to a backend with a length limit (such as memcached, or some DBs) you'll get very obtuse errors such as "Key too long" from memcached.

While arbitrarily switching session backends isn't generally supported behavior, this could be a huge problem for a production site if they ever decided cookie-based sessions weren't ideal for their uses and switched. It would immediately break for every user who had an existing session on the site.

Ideally, if the session backend is unable to retrieve the session due to an error during retrieval, a new session should be generated.

Attachments (1)

17810-catch-all.patch (1.6 KB) - added by aaugustin 3 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 3 years ago by PaulM

  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.3 to 1.4-beta-1

Marking as accepted, and a release blocker. We don't need to support transitioning data between one session database and another, but switching from one session backend to another should never completely block users from accessing a website until they clear their cookies.

Last edited 3 years ago by PaulM (previous) (diff)

comment:2 Changed 3 years ago by jezdez

  • Severity changed from Release blocker to Normal

This is exactly that: "arbitrarily switching session backends isn't generally supported behavior". Removing the release blocker again.

comment:3 Changed 3 years ago by PaulM

  • Resolution set to fixed
  • Status changed from new to closed

In [17795]:

Fixed #17810. Catch session key errors.

Catches memcached session key errors related to overly long session keys.
This is a long-standing bug, but severity was exacerbated by the addition
of cookie-backed session storage, which generates long session values. If
an installation switched from cookie-backed session store to memcached,
users would not be able to log in because of the server error from overly
long memcached keys.

comment:4 Changed 3 years ago by aaugustin

Since this is a bit sensitive and we're very close to 1.4 final, I confirmed manually that the fix works.

I started with a website running on Django 1.4c2.

I added this code to a view to generate a very long cookie:

    if request.user.is_superuser:   # mess only with myself, not with users
        with open('/dev/urandom') as stuff:
            request.session['stuff'] = stuff.read(1024)

I set SESSION_ENGINE = 'django.contrib.sessions.backends.signed_cookies' and restarted the server. The value for sessionid in the cookie is 1658 characters long after encoding.

I set SESSION_ENGINE = 'django.contrib.sessions.backends.cache' and restarted the server again (my cache backend is memcached).

I encountered an exception: MemcachedKeyLengthError: Key length is > 250.

I updated to trunk, performed the same steps and didn't encounter the exception.

comment:5 Changed 3 years ago by aaugustin

  • Resolution fixed deleted
  • Status changed from closed to reopened

Note that two things are wrong in the patch:

  • the except clause doesn't work under Python 2.5 — this is trivial to fix);
  • checking an exception by name is ugly, I'm reopening the ticket to figure out a better solution (if possible) after 1.4 is out.

comment:6 Changed 3 years ago by jezdez

In [17796]:

Fixed an incompatibility with Python 2.5 in the changes done in r17795. Refs #17810.

comment:7 Changed 3 years ago by aaugustin

I tried with 'django.core.cache.backends.memcached.PyLibMCCache' as cache backend instead of 'django.core.cache.backends.memcached.MemcachedCache' and the problem still exists: ValueError: key too long, max is 251.

MemcachedKeyLengthError inherits MemcachedKeyError, which inherits Exception, so the first shared ancestor is Exception :(

comment:8 Changed 3 years ago by aaugustin

  • Severity changed from Normal to Release blocker

Changed 3 years ago by aaugustin

comment:9 Changed 3 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from reopened to closed

In [17797]:

Fixed #17810 (again). Catch session key errors.

The previous commit didn't work with PyLibMC.
This solution appears to be the best compromise
at this point in the 1.4 release cycle.

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