Cookie message storage does not set Vary: Cookie

[Carl Meyer]

The cookie storage backend for contrib.messages does not set Vary: Cookie on the response, which means that (in the absence of CSRF protection or sessions or other bits of Django that are likely to set Vary: Cookie) it is possible for users behind a cache to miss messages intended for them, or even for a cache to store a page with a message intended for a single user on it and then serve that page to other users.

In practice, it is quite likely that the cookie message store is used along with CSRF protection or contrib.session, in which case Vary: Cookie will be set on most responses anyway, rendering this issue moot.

A case could be made that the correct fix here is at an even deeper level; that any access of request.cookies should automatically trigger Vary: Cookie on the response. This follows the same logic as the current behavior of SessionMiddleware to set Vary: Cookie on any response where the session is accessed, but pushes that logic down to the common cookie layer where it really belongs, rather than duplicating it in the cookie message storage. If you are accessing cookies at all on the server side, presumably that means you are making some decision based on the cookie that might affect the response in some way, and if so Vary: Cookie must be set or the response might be cached incorrectly.

If we don't make the fix at the request.cookies layer, we are saying that it is the responsibility of the developer to ensure that if they use cookies, they set Vary: Cookie, in which case we ought to make that clear in the docs. Given that it's very difficult to construct a scenario in which it is correct to access cookies but not set Vary: Cookie, I don't see much reason to place this responsibility on the developer.

For this reason, I'm setting the component for this bug to "core" rather than "contrib.messages", even though the latter is the only place I know of where Django itself triggers the bug by making use of cookies without setting Vary: Cookie.

(Thanks to Florian for presenting the hypothetical case that exposed this bug.)

[Aymeric Augustin]

I confirm the bug.

Given my recent investigations in the area (#15201), I agree that any response that accesses cookies should "Vary: Cookie".

[bigkevmcd]

Adding the Vary: Cookie header when set_cookie is called on the response is trivial enough, connecting access to the request to changes in the response would be a lot more complex from what I can see.

I'm happy to put up a pull-request with test/fix for set_cookie if that's a start.

[Carl Meyer]

Replying to bigkevmcd:

Adding the Vary: Cookie header when set_cookie is called on the response is trivial enough, connecting access to the request to changes in the response would be a lot more complex from what I can see.

I'm happy to put up a pull-request with test/fix for set_cookie if that's a start.

Unfortunately that doesn't fix the problem. It's not just the response with the Set-Cookie header that needs Vary: Cookie, it's every subsequent response that makes decisions based on the cookie values that came in with the request.

Any real fix for this problem will need to involve setting Vary: Cookie on the response based on access to the request. Which means it will either need to happen in the request handler itself or in a response middleware, since that's where we have access to both request and response. The two choices I presented in the ticket description are either to have contrib.messages handle this for itself (in which case it would happen in the MessageMiddleware), or for Django to handle it generally, in which case there would be a design decision: either it would need to be hardcoded behavior in the request handler, or it could be handled in CommonMiddleware.

I still think that having Django handle it is the right approach. My gut feeling is that this is core HTTP behavior and thus it makes sense to bake it right into BaseHandler, since there is no guarantee people are using CommonMiddleware, but I could be talked out of that. I'd need to play around with an implementation and see how it looks.

[bigkevmcd]

CommonMiddleware seems reasonable to me...looking at what's in there, I can see a fairly straightforward approach.

I'll work on getting a patch together, and an I'm happy to push it as a discussion topic.

[bigkevmcd]

​https://github.com/bigkevmcd/django/tree/fix-for-19649

This approach uses CommonMiddleware, which seems reasonable to me, always adding headers implicitly doesn't sound great to me, but am open to being convinced :-)

[Tim Graham]

Patch looks good to me. It needs documentation added to the CommonMiddleware (with a .. versionchanged:: 1.8 annotation) and release notes in 1.8.

[Carl Meyer]

The request part of the patch looks good, but I think it is incorrect to set Vary: Cookie on any response which sets a cookie. Vary: Cookie indicates that the content of the response depended upon the value of cookies sent in the request. Whether or not the response itself sets a cookie is unrelated.

In other words, it is perfectly valid to have a view which doesn't care at all about the cookies it gets in the request, but always sets a cookie in the response - and there is no reason why the response from that view needs to be cached conditionally upon request cookies it never accessed.

So I think HttpResponse._cookies_accessed and related code should be removed from the patch.

I think the code in the sessions middleware that sets Vary: Cookie can and should be removed in this patch, as it now becomes redundant.

The documentation for the vary_on_cookie decorator should probably be updated to note that it should never be necessary (unless you're not using CommonMiddleware), since Django will always set Vary: Cookie if cookies are accessed.

I'm just slightly uneasy with adding this to CommonMiddleware, as it bundles this behavior together with other unrelated behaviors, making it difficult to turn them on or off independently. Maybe that's ok, since it's a behavior that you really shouldn't have any good reason to ever turn off. I guess the alternative would be a new VaryCookieMiddleware, added to the default middleware list. Not sure that's worth it - CommonMiddleware is probably fine.

Lastly, I think this patch's interaction with the CSRF middleware requires some attention. Currently the middleware always checks for the CSRF token cookie in the request before it does anything else, regardless of whether the request requires CSRF protection or not. This is so that it can stuff the CSRF token into request.META, making it available for access during construction of the response. The CSRF middleware then implements its own local version of the logic in this patch (via request.META['CSRF_TOKEN_USED']) in order to avoid Vary: Cookie on responses that did not ever actually use the CSRF token. But this patch would render that logic irrelevant, and there would be no way to ever avoid Vary: Cookie on a response if you use the CSRF middleware. So I think this patch needs to take one of two approaches:

1. Mostly leave CSRF middleware as-is, but allow it to use a private interface to access the CSRF cookie that avoids setting the _cookies_accessed flag, and then trust it to manage the Vary: Cookie header appropriately, using its existing technique. This is slightly ugly, but given that the CSRF middleware is in core, I think it's justifiable. And it's non-invasive and quite backwards-compatible.

2. Modify the CSRF middleware such that it accesses the CSRF cookie lazily. This looks doable (and should be doable in a way that is backwards-compatible), but would certainly be more invasive.