Opened 6 years ago

Last modified 12 months ago

#11506 new Bug

session.flush should not delete the old session

Reported by: Glenn Owned by: nobody
Component: contrib.sessions Version: master
Severity: Normal Keywords:
Cc: glenn@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Flushing and cycling the session should empty the data in the session and create a new key, but should not delete the old key.

Scenario:

1: JS kicks off a periodic AJAX request to update something, which is delayed in transit.

2: User submits an AJAX login form, which calls auth.login, calling session.flush or session.cycle_key. The AJAX response sets a new session cookie for the user.

3: The async request from #1 makes it to the server. This still has the old cookie, since it started before #2 finished. contrib.session doesn't recognize the cookie, since the previous request deleted it. It thinks it's an expired or corrupt session cookie, and flushes the session again.

#2 logs the user in, then #3 logs the user back out. (I've seen this happen even without AJAX logins, when using django.views.static.serve in development.)

session.flush should leave the old session in the database, and just clear its data. That way, when #3 comes around, it won't be an unrecognized session, and it won't trigger a session flush. Let the old session row expire on its own, like any idle session.

This doesn't change the definition of the function: "Removes the current session data from the database and regenerates the key."

This patch also fixes and tests session.cycle_key() raising an error if no session already existed; accessing self._session_cache raises AttributeError. This was triggering while I was writing the main test.

Attachments (1)

django-session-flush.diff (5.5 KB) - added by Glenn 6 years ago.

Download all attachments as: .zip

Change History (9)

Changed 6 years ago by Glenn

comment:1 Changed 6 years ago by Glenn

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

As a followup: this will still fail if the delayed request modifies the session, since it'll refresh the cookie.

A fix would be to update the cookie only when the session hasn't been updated in over some timeout (say, a minute); this is long enough to avoid this race condition. This would have the nice side benefit of not sending a Set-Cookie header for each and every request that modifies the session.

It's harder to implement cleanly, though, since the session rows hold an expiry date, not a last-saved date. You can't reliably derive one from the other after the fact, since the session expiry setting might have changed. I'll leave this for further discussion.

comment:2 Changed 6 years ago by Glenn

  • Cc glenn@… added

The test case should probably use django.test.client, but until there's some indication of interest, I'll hold off on further updates.

comment:3 Changed 6 years ago by Alex

  • Triage Stage changed from Unreviewed to Design decision needed

comment:4 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:5 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:6 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:7 Changed 2 years ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

comment:8 Changed 12 months ago by oinopion

  • Patch needs improvement set

Patch no longer applies. As the ticket is 5 years old it would be worth checking that the problems still occurs.

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