SessionAuthenticationMiddleware causes "Vary: Cookie" header no matter what

[Andrew Badr]

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.

[Tim Graham]

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?

[Carl Meyer]

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.

[Carl Meyer]

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.

[Tim Graham]

That approach is actually what I implemented originally in ​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?

[Carl Meyer]

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.

[Tim Graham]

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.

[Carl Meyer]

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.

[Tim Graham]

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. ​Pull request is up for review.

[Collin Anderson]

+1. This also should solve the "migrate is required for runserver" problem.

[Andrew Badr]

Nice patch @timgraham. Would it be easy to add the general test case mentioned in the ticket description and comment:4?

[Tim Graham]

Yes, I was going to do that as a separate commit.

[Tim Graham]

New pull requests:

• ​Moving session verification out of SessionAuthenticationMiddleware (without any deprecations)
• ​Test for response headers and default middleware

[Tim Graham]

I also created #23957 to discuss the idea of eventually requiring session verification and removing the SessionAuthenticationMiddleware stub.

[Tim Graham <timograham@…>]

In b06dfad88fb12a927c86a1eb23064201c9560fb1:

Fixed #23939 -- Moved session verification out of SessionAuthenticationMiddleware.

Thanks andrewbadr for the report and Carl Meyer for the review.

[Tim Graham <timograham@…>]

In 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

[Tim Graham <timograham@…>]

In 38960a5dd8dc2f3a6b78439ecabc99507656934d:

[1.7.x] Fixed assertion from refs #23939 test.

The assertion is different on master due to
393c0e24223c701edeb8ce7dc9d0f852f0c081ad.

[Tim Graham <timograham@…>]

In 5219a02fdadaaa76f1335f6a668c23dbf5790dac:

[1.7.x] Added a test to verify headers set by default middleware; refs #23939.

Backport of 50c1d8f24b0d04c813b3dd34720df86446091afa from master

[Tim Graham <timograham@…>]

In 50c1d8f24b0d04c813b3dd34720df86446091afa:

Added a test to verify headers set by default middleware; refs #23939.