Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#29471 closed Bug (fixed)

Set-Cookie response is cached for deleting invalid session cookies

Reported by: Duane Hutchins Owned by: birthdaysgift
Component: contrib.sessions Version: 2.0
Severity: Normal Keywords: empty session cache
Cc: Herbert Fortes, Martin H. 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

Regarding: django.contrib.sessions.middleware.SessionMiddleware
and django.utils.cache

Using the SessionMiddleware and caching, if an invalid sessionid (i.e. an empty session) is passed via cookie, the SessionMiddleware responds by deleting the cookie, and this response is cached without Vary: Cookie, so all subsequent authenticated requests also receive that delete-session-cookie header.

The end result is that if a cache-enabled page receives a request using an invalid session id, that page will log out all users who visit that page until that cache expires/purges.

To duplicate:

  • Enable caching and sessions as normal.
  • Visit a cache-enabled page which shows different content for users who are logged in vs not logged in.
  • Log in via browser
  • Change your browser's sessionid cookie value via Chrome Developer Tools or whatever.
  • Refresh your browser
  • The browser is told to delete the invalid cookie (correct).
  • Log in again. Refresh again (if necessary).
  • The login is successful, however, the browser is instructed to delete the session cookie (incorrect).
  • The user is logged out
  • The same logout happens for all users visit that page -- until cache expires

If I decorate the aforementioned page view with @vary_on_cookie (which inserts the Vary:Cookie header), then the issue is resolved.

Possible solutions I can think of:

  • SessionMiddleware adds Vary:Cookie when deleting invalid session cookies
  • Cache Util assumes Vary:Cookie when response includes Set-Cookie header.

Change History (10)

comment:1 by Tim Graham, 6 years ago

Isn't the solution to use @vary_on_cookie as you said? If you have "a cache-enabled page which shows different content for users who are logged in vs not logged in" then you need to use the Vary: Cookie header, don't you?

in reply to:  1 comment:2 by Duane Hutchins, 6 years ago

Replying to Tim Graham:

Isn't the solution to use @vary_on_cookie as you said? If you have "a cache-enabled page which shows different content for users who are logged in vs not logged in" then you need to use the Vary: Cookie header, don't you?

The SessionMiddleware sets Vary: Cookie automatically if the session is accessed. The bug is that SessionMiddleware is not doing that if the session cookie is invalid.

When accessing the session object:

  • No session cookie => Vary: Cookie is set
  • Valid session cookie => Vary: Cookie is set
  • Invalid session cookie => Vary: Cookie is not set

Examining the source code, you can confirm this as true because the patch_vary_headers only happens if the session cookie is valid or missing.

            # First check if we need to delete this cookie.
            # The session should be deleted only if the session is entirely empty
            if settings.SESSION_COOKIE_NAME in request.COOKIES and empty:
                response.delete_cookie(
                    settings.SESSION_COOKIE_NAME,
                    path=settings.SESSION_COOKIE_PATH,
                    domain=settings.SESSION_COOKIE_DOMAIN,
                )
            else:
                if accessed:
                    patch_vary_headers(response, ('Cookie',))

The end result is that this causes the cache handler to cache invalid-session responses without Vary: Cookie. So, if the cached invalid-session response was to delete the session cookie, then the cookie is always deleted on that page -- until the cache is updated.

comment:3 by Tim Graham, 6 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Herbert Fortes, 6 years ago

Cc: Herbert Fortes added

comment:5 by birthdaysgift, 6 years ago

Owner: changed from nobody to birthdaysgift
Status: newassigned

comment:6 by birthdaysgift, 6 years ago

Has patch: set
Last edited 6 years ago by Tim Graham (previous) (diff)

comment:7 by Simon Charette, 6 years ago

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

In dc740dd:

Fixed #29471 -- Added 'Vary: Cookie' to invalid/empty session cookie responses.

comment:9 by Martin H., 5 years ago

Cc: Martin H. added

AFAICS, this bug is already present in the 2.x versions. Shouldn't it be patched there as well?

comment:10 by Mariusz Felisiak, 5 years ago

This patch doesn't qualify for a backport, because it's not a regression or a new feature in Django 2.2.

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