File-based session never expire

[Edwin]

Unlike other session backends, session expiry is never checked for file-based session when loading a session. The consequence of this is that, although browser will not send an expired cookie with Django session ID, user can workaround this by modifying his system time so the browser still sends the expired cookie. Since Django doesn't check for the session expiry on the server, this expired session ID is seen as valid to Django.

I am running 1.3.1, but looking at 1.4 source, the bug is still there. Looking at the source code, this loading method should check for session expiry:

def load(self):                                                  
    session_data = {}
    try:
        session_file = open(self._key_to_file(), "rb")
        try:
            file_data = session_file.read()
            # Don't fail if there is no data in the session file.
            # We may have opened the empty placeholder file.
            if file_data:
                try:
                    session_data = self.decode(file_data)
                except (EOFError, SuspiciousOperation):
                    self.create()
        finally:
            session_file.close()
    except IOError:
        self.create()
    return session_data

The bug was first brought up in the mailing list:
​https://groups.google.com/forum/#!msg/django-developers/tB_F9rRx7Lg/y70AEdejud8J

[Rohan Jain]

Patch 1 - Use OS file info to expire sessions

[Rohan Jain]

Similar to expire_date field in loading from database, the patch - 1 checks the last modify time of the file for age. As far as analogy to updating expire_date is concerned, that will be automatically done with os.write.

Where are the existing tests for contrib.sessions? I was unable to find them for any layer of the app.

[Paul McMillan]

For contrib apps, the tests are stored alongside the apps themselves. In the case of sessions, that's here:

​https://github.com/django/django/blob/master/django/contrib/sessions/tests.py

[Rohan Jain]

Patch 2 - Added a test to represent the problem statement and a separate function to check file session expiry.

[Rohan Jain]

Patch 3 - Use the signing frework to handle session expiry and integrity

[Rohan Jain]

Cleanup management command support for file based sessions.

[Rohan Jain]

I made some attempts with this in file-session-server-expiry-{2,3}.patch. The 2nd uses files modification time to decide on expiry. The 3rd uses the signing framework's timed signer for handling the expiration. Also added a test which is supposed to emulate the ticket behaviour.

Moreover, sessions-cleanup-files-1.patch adds support for cleaning of file sessions with management cleanup command. It moves the session backend specific cleanup logic to their own modules and the management command invokes cleanup command according to the current backend. The base backend implements a cleanup class method which is supposed to be overridden by the backends.

[Tomáš Ehrlich]

Is it necessary to sign session data for all backends? I would define it for file-based sessions only. For example, signed-cookie backend cares about signing itself. I don't see any advantage in signing database data with TimestampSigner since expiration date is stored along with session data.

[Rohan Jain]

Linking this issue to my corresponding pull request over github: ​https://github.com/django/django/pull/453

Replying to Elvard:

Is it necessary to sign session data for all backends? I would define it for file-based sessions only. For example, signed-cookie backend cares about signing itself. I don't see any advantage in signing database data with TimestampSigner since expiration date is stored along with session data.

This makes sense, considering each backend all of the others have alternate ways of verifying expiry. Putting timed signer only in file session seems more appropriate. It'll also save us from compatibility issues for other backends.

Though in database backend the exist seems faulty. It doesn't check for expiry there. But its out of scope of this ticket.

Using modification time seems to me interesting (although simple signing would be useful here), but both solutions (TimestampSigner and modification time) have slight caveat: While in database backend we can specify exact expiration date and check that it's < timezone.now(), here we have modification date (or date of signing) and we check that it's < timezone.now() - SESSION_COOKIE_AGE. It could be solved with setting modification date (or date of signing) in future, but i'm not sure if it's allowed for every file system.

I also think using file system might be unreliable.

TimestampSigner unfortunatelly doesn't support change of time.

I am not sure if I get this point. As far as I know We can modify the time for TimeStameSigner while unsigning. It'll verify based on the second argument of signer.unsign:

signer.unsign(data, age)

We can have settings.SESSION_COOKIE_AGE in place of age parameter.

[Aymeric Augustin]

Related: #18978

[Tomáš Ehrlich]

Replying to crodjer:

TimestampSigner unfortunatelly doesn't support change of time.

I am not sure if I get this point. As far as I know We can modify the time for TimeStameSigner while unsigning. It'll verify based on the second argument of signer.unsign:

signer.unsign(data, age)

We can have settings.SESSION_COOKIE_AGE in place of age parameter.

It's probably just a detail. DB backend stores datetime of expiration, while file-based backend stores datetime of signing.

In database backend I can do:

session = SessionBase()
session[...] = ...  # set data
session.set_expiry(arbitraty_time_in_future)
session.save()

Now we have database record with expiration_date set to 'arbitrary_time_in_future' (which doesn't depend on SESSION_COOKIE_AGE).

While in file-based backend, using TimestampSigner, I can't set arbitrary expiration_date independent on SESSION_COOKIE_AGE because I can't store expiration_date along with data. Whole file containing data is signed and current timestamp is stored.

Probably it's not a big issue. I've came across it when I was trying to write unit tests.

[Aymeric Augustin]

I've worked on this ticket today. Here's an analysis of how session expiration is handled by django.contrib.sessions, and specifically by the built-in backends.

Expiration is handled both client-side (by setting cookie expiry) and server-side (because we don't want to keep stale data forever).

This ticket is about synchronizing server-side expiration with client-side expiration, especially for the file backend. Historically Django hasn't paid much attention to server-side expiration.

---

The default session expiration policy is:

• client-side: expire after settings.SESSION_COOKIE_AGE seconds if settings.SESSION_EXPIRE_AT_BROWSER_CLOSE = False (default), at browser close otherwise
• server-side: expire after settings.SESSION_COOKIE_AGE (no matter the value of settings.SESSION_EXPIRE_AT_BROWSER_CLOSE)

When a non-default expiration is set with session.set_expiry(...), it is saved in the session under the _session_expiry key. The semantic of this value is:

• if it's missing or None: use the default expiration policy described above
• if it's 0: expire at browser close (client side), after settings.SESSION_COOKIE_AGE (server side)
• if it's an int: expire in that many seconds (both sides)
• if it's a datetime: expire at that time (both sides)

This analysis is based on the implementation of SessionMiddleware, session.get_expire_at_browser_close(), session.get_expiry_age() (which controls client-side expiry) and session.get_expiry_date() (which controls server-side expiry).

tl;dr The expiry date of a session is always defined by session.get_expiry_date() / session.get_expiry_age() at the time the session is saved — because it relies on timezone.now() in some cases.

---

How do the various backends deal with that?

• cache: the expiry age is computed and passed to the cache engine. The cache engine is responsible for not returning stale data. Since correct expiry is a major feature for a cache, I think we can rely on cache engine to enforce expiry properly. There's no need to clear expired sessions, the cache does it by itself.

• cached_db: there's a bug here — expiry age is hardcoded to settings.SESSION_COOKIE_AGE instead of session.get_expiry_age(). Otherwise it should work like cache and db. EDIT: fixed in 04b00b668d0d56c37460cbed19671f4b1b5916c3.

• db: the session expiry date is computed and stored alongside the session data when the session is saved. Only sessions whose expiry dates are in the future can be re-loaded. Sessions whose expiry dates are in the past can be cleared.

• file: the session expiry date isn't stored. It can be rebuilt with the algorithm of session.get_expiry_age(), by substituting the file's last modification time to timezone.now(). The patches above attempt to do that in order to clear expired sessions.

• signed_cookies: server-side expiration is provided by timestamping and signing the cookies. However non-default expiry dates aren't handled; the maximum expiration time is hardcoded at settings.SESSION_COOKIE_AGE. There's no need to clear expired sessions because they're stored client-side. Edit: could be fixed in the context of #19201

---

That was a bit long but necessary to frame this ticket :)

Now there are two actions:
1) Clear expired sessions with the file backend. The patches above are on the right track. This depends on #18978.
2) In the longer term, always store the expiration date in the session and refuse to load an expired session in SessionBase.load. This is already how signed cookies work. It overlaps significantly with _session_expiry as described above. It's a major refactor of django.contrib.sessions that will require backwards-compatibility.

Since 1.5 is now feature-frozen, let's restrict this ticket to 1) and create a new ticket for 2).

[Aymeric Augustin]

I've created #19200 to track the bug in cached_db and signed_cookies and #19201 for proposal 2).

I'm now going to attempt to fix 1) which is still marked as a release blocker for 1.5.

[Aymeric Augustin]

This branch fixes this ticket as described above: ​https://github.com/aaugustin/django/compare/session-fixes

Review appreciated :)

[Aymeric Augustin <aymeric.augustin@…>]

In cd17a24083f3ef17cf4c40a41c9d03c250d817c6:

Added optional kwargs to get_expiry_age/date.

This change allows for cleaner tests: we can test the exact output.

Refs #18194: this change makes it possible to compute session expiry
dates at times other than when the session is saved.

Fixed #18458: the existence of the modification kwarg implies that you
must pass it to get_expiry_age/date if you call these functions outside
of a short request - response cycle (the intended use case).

[Aymeric Augustin <aymeric.augustin@…>]

In 882c47cd405cfd29194f2e968678a5aa1d6ec75f:

Improved tests introduced in 04b00b6.

These tests are expected to fail for the file session backend because it
doesn't handle expiry properly. They didn't because of an error in the
test setup sequence.

Refs #19200, #18194.

[Aymeric Augustin <aymeric.augustin@…>]

In 5fec97b9df6ea075483276de159e522a29437773:

Fixed #18194 -- Expiration of file-based sessions

• Prevented stale session files from being loaded
• Added removal of stale session files in django-admin.py clearsessions

Thanks ej for the report, crodjer and Elvard for their inputs.

[Aymeric Augustin <aymeric.augustin@…>]

In 845d8408e7aa09a63c98f160b2c6bc9fa6dbb20f:

[1.5.x] Added optional kwargs to get_expiry_age/date.

This change allows for cleaner tests: we can test the exact output.

Refs #18194: this change makes it possible to compute session expiry
dates at times other than when the session is saved.

Fixed #18458: the existence of the modification kwarg implies that you
must pass it to get_expiry_age/date if you call these functions outside
of a short request - response cycle (the intended use case).

Backport of cd17a24 from master.

[Aymeric Augustin <aymeric.augustin@…>]

In e6b0ee768c46368a41a5b278180d1d1ecbd3d5c6:

[1.5.x] Improved tests introduced in 04b00b6.

These tests are expected to fail for the file session backend because it
doesn't handle expiry properly. They didn't because of an error in the
test setup sequence.

Refs #19200, #18194.

Backport of 882c47c from master.

[Aymeric Augustin <aymeric.augustin@…>]

In 39082494e62d70be62cde1eb10620548fd62dfb6:

[1.5.x] Fixed #18194 -- Expiration of file-based sessions

• Prevented stale session files from being loaded
• Added removal of stale session files in django-admin.py clearsessions

Thanks ej for the report, crodjer and Elvard for their inputs.

Backport of 5fec97b from master.

[Aymeric Augustin <aymeric.augustin@…>]

In 93e0ec553dae73a2a86cb23b058414d8f9ae3bd6:

[1.5.x] Fixed #19254 -- Bug in SESSION_FILE_PATH handling.

Thanks simonb for the report.

Refs #18194.

Backport of 11fd00c from master.

[Aymeric Augustin <aymeric.augustin@…>]

In 11fd00c46e2826ce5852e096487762a96909b717:

Fixed #19254 -- Bug in SESSION_FILE_PATH handling.

Thanks simonb for the report.

Refs #18194.