Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#25489 closed Cleanup/optimization (fixed)

SESSION_SAVE_EVERY_REQUEST Appears to have broke in 1.8.4

Reported by: Damon Timm Owned by: nobody
Component: Documentation Version: 1.8
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

After upgrading to Django 1.8.4 the request.session.session_key value always returns None for a non-authenticated user even when SESSION_SAVE_EVERY_REQUEST is set to True.

I was able to reproduce this by creating a new project and app using a very basic view:

# settings.py
SESSION_SAVE_EVERY_REQUEST = True

# views.py
from django.http import HttpResponse

def index(request):
    return HttpResponse('Session Key is: %s' % request.session.session_key)

#urls.py
from django.conf.urls import url

from test_app import views

urlpatterns = [
    url(r'^$', views.index, name="index"),
]

In Django <=1.8.3 this view will start to return a session key after the first page load (you have to refresh is a second time, of course, but you will get a key!).

But with Django 1.8.4 it continues to return None even after refreshing the browser. (Be sure to remove your site cookies during testing because the key can persist between downgrading and upgrading of Django.)

First time submitting a ticket ... I hope this is helpful.

Change History (11)

comment:1 by Tim Graham, 9 years ago

This is due to the security fix in 8cc41ce7a7a8f6bebfdd89d5ab276cd0109f4fc5. Do you really require the creation of empty sessions (and do you accept the DoS possibilities that it entails)? We should at least update the documentation to better clarify the expected behavior.

comment:2 by Damon Timm, 9 years ago

Well ... I am certainly no expert on security and would defer to a wiser cohort! Smile.

From my standpoint, it seems like the expected behavior of SESSION_SAVE_EVERY_REQUEST has changed. The outcome it was producing was what I had come to expect. By adding the and not Empty in that patch doesn't it kind of negate the setting itself (because I thought the setting was supposed to force it to save even if it was empty) ... or am I misunderstanding that?

comment:3 by Tim Graham, 9 years ago

I'm not sure what the intended use case of SESSION_SAVE_EVERY_REQUEST is -- that doesn't seem to be described in the original commit where it was added: 3895a825a9696b58db1a0a2f6f30b1b023d58050. Could you describe why enabling the setting is useful for you?

comment:4 by Carl Meyer, 9 years ago

The one time I used SESSION_SAVE_EVERY_REQUEST was because there was a security requirement for authenticated sessions to expire quickly (a matter of minutes), but that expiry time needed to be updated on each request (so the expiry would only occur after inactivity, not in the middle of usage). Currently SESSION_SAVE_EVERY_REQUEST is the only way to get Django to update the session cookie expiry time on each request.

That use case didn't have any need for saving empty sessions for unauthenticated users, though.

comment:5 by Damon Timm, 9 years ago

So I used it in django-hitcount as one of the methods to identify a unique visitor as part of the view logic -- it was just the low hanging fruit at the time. Smile. If that setting is going away it may just be time to look into how to do it a little better.

https://github.com/thornomad/django-hitcount

comment:6 by Carl Meyer, 9 years ago

Component: contrib.sessionsDocumentation
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

The setting isn't going away, but the behavior of creating empty session records for all visitors isn't coming back, I don't think.

Probably worth a mention in the docs for SESSION_SAVE_EVERY_REQUEST, though.

comment:7 by Tim Graham, 9 years ago

Has patch: set

comment:8 by Carl Meyer, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In abf5ccc2:

Fixed #25489 -- Documented that SESSION_SAVE_EVERY_REQUEST doesn't create empty sessions.

comment:10 by Tim Graham <timograham@…>, 9 years ago

In adc9fa83:

[1.9.x] Fixed #25489 -- Documented that SESSION_SAVE_EVERY_REQUEST doesn't create empty sessions.

Backport of abf5ccc29c45d53ec17541179bb5f0a75b28915d from master

comment:11 by Tim Graham <timograham@…>, 9 years ago

In 12f4db23:

[1.8.x] Fixed #25489 -- Documented that SESSION_SAVE_EVERY_REQUEST doesn't create empty sessions.

Backport of abf5ccc29c45d53ec17541179bb5f0a75b28915d from master

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