[Django] #23939: SessionAuthenticationMiddleware causes "Vary: Cookie" header no matter what
Django
------------------------------+---------------------
Reporter: andrewbadr | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.7
Severity: Normal | Keywords: cookies
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+---------------------
Setting a "Vary: Cookie" header when unnecessary is bad for reasons
described in e.g. #3586, #6552. It seems that the recently-introduced and
on-by-default SessionAuthenticationMiddleware causes this header to always
be set. This seems to be caused by the `hasattr(user,
'get_session_auth_hash')` check here:
https://github.com/django/django/blob/1.7.1/django/contrib/auth/middleware.py#L34.
To reproduce: start a new empty project with django-admin.py, request the
index page, and see that the Vary: Cookie header is present. Commenting-
out the middleware's line in settings.py causes the header to no longer be
sent.
It might be good to add a test verifying that the above steps never set a
Vary: Cookie header.
--
Ticket URL: <https://code.djangoproject.com/ticket/23939>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: andrewbadr | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.7
Keywords: cookies | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by andrewbadr):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
> Setting a "Vary: Cookie" header when unnecessary is bad for reasons
> described in e.g. #3586, #6552. It seems that the recently-introduced and
> on-by-default SessionAuthenticationMiddleware causes this header to
> always be set. This seems to be caused by the `hasattr(user,
> 'get_session_auth_hash')` check here:
> https://github.com/django/django/blob/1.7.1/django/contrib/auth/middleware.py#L34.
>
> To reproduce: start a new empty project with django-admin.py, request the
> index page, and see that the Vary: Cookie header is present. Commenting-
> out the middleware's line in settings.py causes the header to no longer
> be sent.
>
> It might be good to add a test verifying that the above steps never set a
> Vary: Cookie header.
New description:
Setting a "Vary: Cookie" header when unnecessary is bad for reasons
described in e.g. #3586, #6552. It seems that the recently-introduced and
on-by-default SessionAuthenticationMiddleware causes this header to always
be set. This seems to be caused by the `hasattr(user,
'get_session_auth_hash')` check here:
https://github.com/django/django/blob/1.7.1/django/contrib/auth/middleware.py#L34.
To reproduce: start a new empty project with django-admin.py, request the
index page, and see that the Vary: Cookie header is present. Commenting-
out the middleware's line in settings.py causes the header to disappear.
It might be good to add a general test case verifying that the above steps
never set a Vary: Cookie header.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:1>
Django
Reporter: andrewbadr | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.7
Keywords: cookies | Triage Stage: Unreviewed
Easy pickings: 0 | UI/UX: 0
Description changed by andrewbadr:
Old description:
> Setting a "Vary: Cookie" header when unnecessary is bad for reasons
> described in e.g. #3586, #6552. It seems that the recently-introduced and
> on-by-default SessionAuthenticationMiddleware causes this header to
> always be set. This seems to be caused by the `hasattr(user,
> 'get_session_auth_hash')` check here:
> https://github.com/django/django/blob/1.7.1/django/contrib/auth/middleware.py#L34.
>
> To reproduce: start a new empty project with django-admin.py, request the
> index page, and see that the Vary: Cookie header is present. Commenting-
> out the middleware's line in settings.py causes the header to disappear.
>
> It might be good to add a general test case verifying that the above
> steps never set a Vary: Cookie header.
New description:
Setting a "Vary: Cookie" header when unnecessary is bad for reasons
described in e.g. #3586, #6552. It seems that the recently-introduced and
on-by-default SessionAuthenticationMiddleware causes this header to always
be set. This seems to be caused by the `hasattr(user,
'get_session_auth_hash')` call at
https://github.com/django/django/blob/1.7.1/django/contrib/auth/middleware.py#L34.
To reproduce: start a new empty project with django-admin.py, request the
index page, and see that the Vary: Cookie header is present. Commenting-
out the middleware's line in settings.py causes the header to disappear.
It might be good to add a general test case verifying that the default
page never sets a Vary: Cookie header.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:2>
Django
Reporter: andrewbadr | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.7
Keywords: cookies | Triage Stage: Unreviewed
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I am not sure how to address this other than to document the limitation.
We cannot perform the middleware's purpose of verifying the session
without accessing the session. Do you have any suggestions?
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:3>
Django
Reporter: andrewbadr | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.7
Keywords: cookies | Triage Stage: Unreviewed
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
I don't think "document the limitation" is an acceptable answer here.
Adding `Vary: Cookie` unconditionally to all requests has massive
implications for caching of requests that otherwise would not vary per-
user. I know some large sites work hard to avoid `Vary: Cookie` on certain
responses in order to make them cacheable without having to cache
separately per-client.
I think `SessionAuthenticationMiddleware` should be implemented quite
differently, probably not as a middleware at all, but rather as code that
runs lazily when `request.user` is first used (that is, in the `get_user`
helper function).
We should also add a simple test somewhere in the test suite that sets up
a view which never accesses the session (or `request.user`, which
implicitly accesses the session), and then calls that view with an end-to-
end test passing through all the default middleware, and asserts that
`Vary: Cookie' is not added to its responses. These bugs are way too easy
to introduce, and we need a general test like that to prevent regressions
of this type.
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:4>
Django
Reporter: andrewbadr | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.7
Keywords: cookies | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
Marking as release blocker -- this is a serious regression in 1.7 which
should be backported to 1.7.x. Unfortunately, we'll have to keep
`SessionAuthenticationMiddleware` around as a no-op middleware, deprecated
in 1.8.
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:5>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: assigned
Component: contrib.auth | Version: 1.7
Severity: Release blocker | Resolution:
Keywords: cookies | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* owner: nobody => timgraham
* status: new => assigned
Comment:
That approach is actually what I implemented originally in
[https://github.com/django/django/pull/2113 this pull request]. In #21649,
however, the concern about users losing their session when Django was
upgraded was raised. If we revert to the original approach, it seems like
we may need a setting to disable the behavior for users who don't want it.
Any other ideas?
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:6>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: cookies | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
A couple ideas, but I don't think either one is better than adding a
setting:
1. Could have a new version of `AuthenticationMiddleware` which does the
session validation, and deprecate the current `AuthenticationMiddleware`.
But this is more churn for everyone, and means the new
`AuthenticationMiddleware` would have a less obvious name. Bad idea.
2. Could keep `SessionAuthenticationMiddleware`, and have it re-wrap
`request.user` in a second level of lazy object. But - yuck.
The problem with just reverting to your original approach and adding a
setting to enable it, is that it would have the effect of silently
disabling session validation for all projects created using already-
released versions of Django 1.7 (since they wouldn't have that setting).
Is there a use case for a long-term simple way to disable this behavior?
Or is it just a way to preserve sessions across the upgrade that we need?
Personally I think we should be on a deprecation path to making this
always-on; I think it's fine if you have to write your own
`AuthenticationMiddleware` if you don't want it.
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:7>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: cookies | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I don't know of a use case to disable the behavior in the long-term.
For backwards compatibility, we could guard the new verification logic in
`auth.get_user()` with `if
'django.contrib.auth.middleware.SessionAuthenticationMiddleware' in
settings.MIDDLEWARE_CLASSES` or `settings.AUTH_VERIFY_SESSION`. The new
`settings.AUTH_VERIFY_SESSION` defaults to `False` and goes through the
normal deprecation cycle (set it to `True` to disable the warning). As
sessions naturally expire and are recreated, they are "upgraded" to
include the verification hash, so presumably you could flip the setting
after some time of running 1.7 in production and lose a minimal amount of
sessions, if any.
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:8>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: cookies | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
That proposal sounds right to me; I don't have a better plan. Presumably
there would be two deprecation warnings, one for
`SessionAuthenticationMiddleware` (which would be a no-op, other than
being checked for as an alternative to `settings.AUTH_VERIFY_SESSION`),
and one for `settings.AUTH_VERIFY_SESSION = False`.
Do we have precedent for adding deprecations in a minor version? Maybe the
deprecation warnings should be added only in master, and not in the
backport? I guess adding the warnings in the backport gets people aware of
the issue earlier, but it seems slightly backwards-incompatible to add
deprecation warnings in the middle of a release series.
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:9>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: cookies | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
Comment:
Since it's a flawed new feature and should be trivial to migrate to the
fix (change some settings), my preference is to deprecate
`SessionAuthenticationMiddleware` in 1.7.2 and remove it in 1.8.
[https://github.com/django/django/pull/3667 Pull request] is up for
review.
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:10>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: cookies | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by collinanderson):
+1. This also should solve the "migrate is required for runserver"
problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:11>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: cookies | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewbadr):
Nice patch @timgraham. Would it be easy to add the general test case
mentioned in the ticket description and comment:4?
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:12>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: cookies | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Yes, I was going to do that as a separate commit.
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:13>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: cookies | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
New pull requests:
* [https://github.com/django/django/pull/3672 Moving session verification
out of SessionAuthenticationMiddleware] (without any deprecations)
* [https://github.com/django/django/pull/3673 Test for response headers
and default middleware]
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:14>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: cookies | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I also created #23957 to discuss the idea of eventually requiring session
verification and removing the `SessionAuthenticationMiddleware` stub.
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:15>
Django
Reporter: andrewbadr | Owner: timgraham
Component: contrib.auth | Version: 1.7
Severity: Release blocker | Resolution: fixed
Keywords: cookies | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"b06dfad88fb12a927c86a1eb23064201c9560fb1"]:
{{{
#!CommitTicketReference repository=""
revision="b06dfad88fb12a927c86a1eb23064201c9560fb1"
Fixed #23939 -- Moved session verification out of
SessionAuthenticationMiddleware.
Thanks andrewbadr for the report and Carl Meyer for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:16>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: cookies | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"99c0cc5300be21bd067aa9903b567ce64258942f"]:
{{{
#!CommitTicketReference repository=""
revision="99c0cc5300be21bd067aa9903b567ce64258942f"
[1.7.x] Fixed #23939 -- Moved session verification out of
SessionAuthenticationMiddleware.
Thanks andrewbadr for the report and Carl Meyer for the review.
Backport of b06dfad88fb12a927c86a1eb23064201c9560fb1 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:17>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: cookies | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"38960a5dd8dc2f3a6b78439ecabc99507656934d"]:
{{{
#!CommitTicketReference repository=""
revision="38960a5dd8dc2f3a6b78439ecabc99507656934d"
[1.7.x] Fixed assertion from refs #23939 test.
The assertion is different on master due to
393c0e24223c701edeb8ce7dc9d0f852f0c081ad.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:18>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: cookies | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"50c1d8f24b0d04c813b3dd34720df86446091afa"]:
{{{
#!CommitTicketReference repository=""
revision="50c1d8f24b0d04c813b3dd34720df86446091afa"
Added a test to verify headers set by default middleware; refs #23939.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:20>
Django
Reporter: andrewbadr | Owner: timgraham
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: cookies | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"5219a02fdadaaa76f1335f6a668c23dbf5790dac"]:
{{{
#!CommitTicketReference repository=""
revision="5219a02fdadaaa76f1335f6a668c23dbf5790dac"
[1.7.x] Added a test to verify headers set by default middleware; refs
#23939.
Backport of 50c1d8f24b0d04c813b3dd34720df86446091afa from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23939#comment:19>