Code

Opened 18 months ago

Last modified 8 months ago

#19649 assigned Bug

Cookie message storage does not set Vary: Cookie

Reported by: carljm Owned by: bigkevmcd
Component: HTTP handling Version: 1.4
Severity: Normal Keywords:
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 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.)

Attachments (0)

Change History (9)

comment:1 Changed 18 months ago by carljm

  • Description modified (diff)

comment:2 Changed 18 months 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 16 months ago by aaugustin

  • Component changed from Core (Other) to HTTP handling

comment:4 Changed 10 months ago by bigkevmcd

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

comment:5 follow-up: Changed 10 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 10 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 10 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 10 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 10 months ago by bigkevmcd (next)

comment:9 Changed 8 months ago by bigkevmcd

  • Has patch set

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from bigkevmcd to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.