Opened 6 years ago

Closed 4 years ago

Last modified 3 years ago

#11555 closed Bug (fixed)

Undocumented contrib.sessions behaviour

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


When new session starts (sessionid not passed in cookies) and request.session.session_key is accessed before request.session["abc"], then:

1) request.session.session_key is generated,

2) first access to request.session[ "abc" ] calls request.session.load(), which overrides (by calling create()) request.session.session_key because the session_key cannot be found in DB.

This can be manually fixed by calling request.session.load() before using request.session.session_key.

I'm not sure if this is a bug or undocumented feature - I can send relevant patch depending on the answer.

Attachments (1)

11555.diff (4.7 KB) - added by aaugustin 5 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 6 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 years ago by thejaswi_puthraya

  • Component changed from Uncategorized to django.contrib.sessions

comment:3 Changed 5 years ago by aaugustin

  • Cc aaugustin added

When no session exists, create will generate a new session key, regardless of the existing session key; this is an expected behavior, used for key regeneration and checked by the tests. It should not be changed.

So, one way to solve the OP's problem is to create the session when session.session_key is accessed. That makes sense, since the only way to know if a session key is usable is to create a session with that key (that is what the backends do).

Accessing session.session_key either returns an existing session key or creates a new session key with _get_new_session_key and returns it. Furthermore, all create methods in the backends begin by calling _get_new_session_key. So the following patch looks reasonable.

Index: django/contrib/sessions/backends/
--- django/contrib/sessions/backends/	(revision 15655)
+++ django/contrib/sessions/backends/	(working copy)
@@ -172,7 +172,7 @@
         if self._session_key:
             return self._session_key
-            self._session_key = self._get_new_session_key()
+            self.create()
             return self._session_key
     def _set_session_key(self, session_key):

Unfortunately, it breaks a seemingly unrelated test (DatabaseSessionTests.test_session_get_decoded) in a way I do not understand.

Looking at the problem differently, creating a session key is a real task; it should not occur as a side effect of accessing a public attribute (e.g. writing session.session_key).

So another possible fix is to return session._session_key when session.session_key is called; returning None would mean that there is no session key at this time (possibly because the session is not loaded or created yet). From the user's point of view, returning None instead of a wrong session key is probably better, although it might break code that relies on getting a string.

Then, a get_or_create_session_key method should be created and used instead of session_key wherever it is expected that a new session key may be generated.

That is a rather big change, so I'd like to hear from someone else before working on it.

comment:4 Changed 5 years ago by aaugustin

Session key randomization is also an important security feature: see #13478.

That is one more reason to make the public API for session_key read-only.

Changed 5 years ago by aaugustin

comment:5 Changed 5 years ago by aaugustin

  • Has patch set

Attached patch implements the second solution described above.

comment:6 Changed 5 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:7 Changed 4 years ago by ptone

  • Easy pickings unset
  • Patch needs improvement set
  • UI/UX unset

patch not applying cleanly on 16843 - looks like minor update needed

comment:8 Changed 4 years ago by aaugustin

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

In [17155]:

(The changeset message doesn't reference this ticket)

comment:5 Changed 3 years ago by aaugustin

In [17911]:

Clarified that Django randomizes session keys. Refs #11555, #13478, #18128.

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