[Django] #29971: Sessions setting vary cookie without providing a cookie
Django
-------------------------------------+-------------------------------------
Reporter: Jd | Owner: Jd Collins
Collins |
Type: | Status: assigned
Uncategorized |
Component: | Version: 2.1
contrib.sessions | Keywords: Sessions Vary
Severity: Normal | Header Cookie
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
While reviewing process_response I believe the code is trying to
accomplish the following things:
1. Determine if the session has been accessed.
2. Determine if the session has been modified.
3. Determine if the session is empty.
4. If the session cookie is in the cookies but is empty remove the session
cookie.
5. Else if not 4. then if the session has been accessed then patch the
vary header to add Cookie.
6. If the session has been modified or settings dictate that we always
save the session and the session is not empty then we setup some cookie
attributes, save the session, and set the cookie in the response.
7. Return the response.
Possible issues in the current behavior:
1. As reported in 29471, step 4 in the current behavior is responding in a
way that sets the cookie to a blank value and expired but not setting the
vary on cookie so this response can be cached for a requests causing
authenticated sessions to be logged out.
2. Step 5 in the current behavior, accessed is always True when using auth
as a result of: django/contrib/auth/middleware.py in get_user. I am not
sure how accessing the session alone is enough reason to vary on cookie
regardless of why it was accessed?
3. Step 5 in the current behavior, adds the vary cookie header even if the
session is empty and will not be setting a cookie. This is problematic for
caching since you are requiring a unique session based cache even if the
session is empty.
Proposed behavior:
1. Determine if the session has been accessed. (Not sure if this is
providing much value for a vary header perspective but leaving it).
2. Determine if the session has been modified.
3. Determine if the session is empty.
4. If the session cookie is in the cookies but is empty remove the session
cookie. Also set the vary cookie to address potential issue #1.
5. Else if the session has been modified or settings dictate that we
always save the session and the session is not empty then we setup some
cookie attributes, save the session, and set the cookie in the response.
Also set the vary cookie.
6. Else if the session is not empty the set the vary cookie.
7. Return response.
Related Ticket(s)
https://code.djangoproject.com/ticket/29471
--
Ticket URL: <https://code.djangoproject.com/ticket/29971>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Collins
Cleanup/optimization | Status: assigned
Component: contrib.sessions | Version: 2.1
Severity: Normal | Resolution:
Keywords: Sessions Vary | Triage Stage:
Header Cookie | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Uncategorized => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/29971#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Collins
Cleanup/optimization | Status: assigned
Component: contrib.sessions | Version: 2.1
Severity: Normal | Resolution:
Keywords: Sessions Vary | Triage Stage:
Header Cookie | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> Related Ticket(s)
> https://code.djangoproject.com/ticket/29471
New description:
While reviewing process_response in django/contrib/sessions/middleware.py
Proposed behavior:
Related Ticket(s)
https://code.djangoproject.com/ticket/29471
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29971#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Collins
Cleanup/optimization | Status: assigned
Component: contrib.sessions | Version: 2.1
Severity: Normal | Resolution:
Keywords: Sessions Vary | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Someday/Maybe
Comment:
It looks like you intend to try a patch. You might find answers to your
questions if there are test failures with your approach. You can either
send a pull request or close this ticket depending on the outcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/29971#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Collins
Cleanup/optimization | Status: assigned
Component: contrib.sessions | Version: 2.1
Severity: Normal | Resolution:
Keywords: Sessions Vary | Triage Stage:
Header Cookie | Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "patch.diff" added.
Django
-------------------------------------+-------------------------------------
Type: | Collins
Cleanup/optimization | Status: assigned
Component: contrib.sessions | Version: 2.1
Severity: Normal | Resolution:
Keywords: Sessions Vary | Triage Stage:
Header Cookie | Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jd Collins):
Yes I did plan to try a patch, and did create one. After creating the
patch I ran the tests and found one failing test. Before continuing on at
this point I decided to think about the importance of the test and the
impact my solution could have before writing tests that fit my approach. I
came to the conclusion that it may be logical to always provide Vary:
Cookie when using sessions and auth even if the session is not
authenticated and empty. The problem with trying to not provide this
header if the session is anonymous and empty is the cached response would
also then apply to authenticated and non empty sessions which is likely
worse and less manageable result. Although Vary: Cookie has limitless
potential to diminish the effectiveness of caching since the cookie can be
unique for so many reasons beyond Django's use of sessions/cookies. It
seems a better approach would be to do some normalization at the cache
layer or maybe https://github.com/rory/django-dont-vary-on. I have decided
to close this ticket and provided my patch for completeness or anything it
might offer to ticket #29471.
--
Ticket URL: <https://code.djangoproject.com/ticket/29971#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Collins
Component: contrib.sessions | Version: 2.1
Severity: Normal | Resolution: wontfix
Keywords: Sessions Vary | Triage Stage:
Header Cookie | Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => wontfix
--
Ticket URL: <https://code.djangoproject.com/ticket/29971#comment:5>