Code

Opened 20 months ago

Last modified 16 months ago

#19324 new Bug

invalid session keys cause unnecessary empty records in django_session table

Reported by: liangrubo@… Owned by: nobody
Component: contrib.sessions Version: 1.4
Severity: Normal 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

db session store calls self.create when no record is found for the session key, which causes an empty record inserted. Is this necessary? This gives chance to user to fill the session table with empty records by sending invalid session keys.

is it more appropriate to set session_key to be None in this case?

current implementation:

    def load(self):
        try:
            s = Session.objects.get(
                session_key=self.session_key,
                expire_date__gt=timezone.now()
            )
            return self.decode(s.session_data)
        except (Session.DoesNotExist, SuspiciousOperation):
            self.create()
            return {}

suggested implementation:

    def load(self):
        try:
            s = Session.objects.get(
                session_key=self.session_key,
                expire_date__gt=timezone.now()
            )
            return self.decode(s.session_data)
        except (Session.DoesNotExist, SuspiciousOperation):
            self.session_key = None
            return {}

Attachments (0)

Change History (2)

comment:1 Changed 20 months ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

You probably meant: self._session_key = None.

I don't immediately see how this could allow session fixation attacks — but that doesn't prove anything :)

As is, this change causes two test failures:

Creating test database for alias 'default'...
Creating test database for alias 'other'...
........................................................................................x................................................F.............................F..........................................
======================================================================
FAIL: test_save (django.contrib.sessions.tests.DatabaseSessionTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/aaugustin/Documents/dev/django/django/contrib/sessions/tests.py", line 143, in test_save
    self.assertTrue(self.session.exists(self.session.session_key))
AssertionError: False is not true

======================================================================
FAIL: test_save (django.contrib.sessions.tests.DatabaseSessionWithTimeZoneTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/aaugustin/Documents/dev/django/django/contrib/sessions/tests.py", line 143, in test_save
    self.assertTrue(self.session.exists(self.session.session_key))
AssertionError: False is not true

----------------------------------------------------------------------
Ran 210 tests in 0.353s

FAILED (failures=2, expected failures=1)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...

This could probably be resolved in save(), though.


In fact, this change would cause save() to be called instead of create(). Currently the roles of these two functions overlap: save() even has a must_create argument! See also #18344.

To sum up, the behavior described exists, but it has a very low impact, and even with the proposed change it's easy to cause the cache to fill up. I suspect this ticket should be closed in favor of a ticket describing a refactoring of the sessions API to eliminate the redundancy between save() and create().

comment:2 Changed 16 months ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.