Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#23939 closed Bug (fixed)

SessionAuthenticationMiddleware causes "Vary: Cookie" header no matter what

Reported by: Andrew Badr Owned by: Tim Graham
Component: contrib.auth Version: 1.7
Severity: Release blocker Keywords: cookies
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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.

Change History (20)

comment:1 by Andrew Badr, 10 years ago

Description: modified (diff)

comment:2 by Andrew Badr, 10 years ago

Description: modified (diff)

comment:3 by Tim Graham, 10 years ago

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?

comment:4 by Carl Meyer, 10 years ago

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.

comment:5 by Carl Meyer, 10 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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.

comment:6 by Tim Graham, 10 years ago

Owner: changed from nobody to Tim Graham
Status: newassigned

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?

comment:7 by Carl Meyer, 10 years ago

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.
  1. 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.

comment:8 by Tim Graham, 10 years ago

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.

comment:9 by Carl Meyer, 10 years ago

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.

comment:10 by Tim Graham, 10 years ago

Has patch: set

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.

comment:11 by Collin Anderson, 10 years ago

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

comment:12 by Andrew Badr, 10 years ago

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

comment:13 by Tim Graham, 10 years ago

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

comment:15 by Tim Graham, 10 years ago

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

comment:16 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In b06dfad88fb12a927c86a1eb23064201c9560fb1:

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

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

comment:17 by Tim Graham <timograham@…>, 10 years ago

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

comment:18 by Tim Graham <timograham@…>, 10 years ago

In 38960a5dd8dc2f3a6b78439ecabc99507656934d:

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

The assertion is different on master due to
393c0e24223c701edeb8ce7dc9d0f852f0c081ad.

comment:19 by Tim Graham <timograham@…>, 10 years ago

In 5219a02fdadaaa76f1335f6a668c23dbf5790dac:

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

Backport of 50c1d8f24b0d04c813b3dd34720df86446091afa from master

comment:20 by Tim Graham <timograham@…>, 10 years ago

In 50c1d8f24b0d04c813b3dd34720df86446091afa:

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

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