Opened 2 years ago

Last modified 11 months ago

#19649 new Bug

Cookie message storage does not set Vary: Cookie

Reported by: carljm Owned by:
Component: HTTP handling Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by carljm)

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

Change History (13)

comment:1 Changed 2 years ago by carljm

  • Description modified (diff)

comment:2 Changed 2 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

I confirm the bug.

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

comment:3 Changed 2 years ago by aaugustin

  • Component changed from Core (Other) to HTTP handling

comment:4 Changed 22 months ago by bigkevmcd

  • Owner changed from nobody to bigkevmcd
  • Status changed from new to assigned

comment:5 follow-up: Changed 22 months ago by 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.

comment:6 in reply to: ↑ 5 Changed 22 months ago by carljm

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.

comment:7 Changed 22 months ago by 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.

comment:8 Changed 22 months ago by 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.

Version 0, edited 22 months ago by bigkevmcd (next)

comment:9 Changed 20 months ago by bigkevmcd

  • Has patch set

comment:10 Changed 11 months ago by timgraham

  • Needs documentation set

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

comment:11 Changed 11 months ago by carljm

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

comment:12 Changed 11 months ago by carljm

  • Patch needs improvement set

comment:13 Changed 11 months ago by bigkevmcd

  • Owner bigkevmcd deleted
  • Status changed from assigned to new
Note: See TracTickets for help on using tickets.
Back to Top