[Django] #19649: Cookie message storage does not set Vary: Cookie
Django
----------------------------------------+------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.4
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
----------------------------------------+------------------------
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.messages, 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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/19649>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.4
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by carljm:
Old description:
New description:
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.)
--
--
Ticket URL: <https://code.djangoproject.com/ticket/19649#comment:1>
Django
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.4
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* stage: Unreviewed => Accepted
Comment:
I confirm the bug.
Given my recent investigations in the area (#15201), I agree that any
response that accesses cookies should "Vary: Cookie".
--
Ticket URL: <https://code.djangoproject.com/ticket/19649#comment:2>
Django
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* component: Core (Other) => HTTP handling
--
Ticket URL: <https://code.djangoproject.com/ticket/19649#comment:3>
Django
Reporter: carljm | Owner: bigkevmcd
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bigkevmcd):
* status: new => assigned
* owner: nobody => bigkevmcd
--
Ticket URL: <https://code.djangoproject.com/ticket/19649#comment:4>
Django
Reporter: carljm | Owner: bigkevmcd
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19649#comment:5>
Django
Reporter: carljm | Owner: bigkevmcd
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
Replying to [comment:5 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19649#comment:6>
Django
Reporter: carljm | Owner: bigkevmcd
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19649#comment:7>
Django
Reporter: carljm | Owner: bigkevmcd
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19649#comment:8>
Django
Reporter: carljm | Owner: bigkevmcd
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bigkevmcd):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/19649#comment:9>
Django
Reporter: carljm | Owner: bigkevmcd
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_docs: 0 => 1
Comment:
Patch looks good to me. It needs documentation added to the
`CommonMiddleware` (with a `.. versionchanged:: 1.8` annotation) and
release notes in 1.8.
--
Ticket URL: <https://code.djangoproject.com/ticket/19649#comment:10>
Django
Reporter: carljm | Owner: bigkevmcd
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (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.
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19649#comment:11>
Django
Reporter: carljm | Owner: bigkevmcd
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/19649#comment:12>
Django
Reporter: carljm | Owner:
Type: Bug | Status: new
Component: HTTP handling | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by bigkevmcd):
* status: assigned => new
* owner: bigkevmcd =>
--
Ticket URL: <https://code.djangoproject.com/ticket/19649#comment:13>