Re: [Django] #15855: cache_page decorator bypasses any Vary headers set in middleware

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Core (Cache system)
Version: | Severity: Normal
Resolution: | Keywords:
Triage Stage: Design | Has patch: 1
decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
Changes (by idangazit):

* has_patch: 0 => 1
* easy: => 0

Comment:

I just got bitten by this; the docs about mixing CSRF and per-view caching
are misleading. The `vary_on_cookie` decorator doesn't actually solve the
fact that te CSRF token which is placed in the template of a cached view.

Having sat with Russ, Alex, and Jannis on the matter, the conclusion is
that the docs are simply wrong, and bear updating. To that end, see
attached docs patch.

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Core (Cache system)
Version: | Severity: Normal
Resolution: | Keywords:
Triage Stage: Design | Has patch: 1
decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |

UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by carljm):

* ui_ux: => 0

Comment:

Replying to [comment:2 idangazit]:

> I just got bitten by this; the docs about mixing CSRF and per-view
caching are misleading. The `vary_on_cookie` decorator doesn't actually
solve the fact that te CSRF token which is placed in the template of a
cached view.
>
> Having sat with Russ, Alex, and Jannis on the matter, the conclusion is
that the docs are simply wrong, and bear updating. To that end, see
attached docs patch.

I'm not clear on the issue here - if vary_on_cookie doesn't solve the
problem, then how is it solved by using the cache middleware rather than
per-page caching? What, exactly, is the issue?

In any case, if vary_on_cookie doesn't solve this problem, then I think
this problem deserves its own separate ticket and doesn't belong in this
ticket, which is specifically about Vary header issues with the cache_page
decorator. Even if the attached doc patch is correct, which is not yet
clear to me, applying it would not be a fix for this ticket.

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:3>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Core (Cache system)
Version: | Severity: Normal
Resolution: | Keywords:
Triage Stage: Design | Has patch: 1
decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------

Comment (by russellm):

There's two problems here:

1. The subject of this ticket -- that cache_page has problems because it
caches content too early in the process. This is the root problem.
2. The fact that the docs currently suggest that @vary_on_header and
@cache_page can be used on CSRF pages.

The second problem will be resolved when/if we fix the first problem, but
in the interim, the existence of the suggestion is pretty misleading.
Idan's patch looks good for that purpose.

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:4>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Core (Cache system)
Version: | Severity: Normal
Resolution: | Keywords:
Triage Stage: Design | Has patch: 1
decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------

Comment (by lukeplant):

It still hasn't been explained **why** `@vary_on_cookie` and `@cache_page`
don't work with CSRF pages. Idan had a sentence that looked like it was
about to explain it and then stopped. I'm guessing it is do with the
cookie being set by the middleware **after** the page has been cached.
Would the documentation be fixed by adding `@csrf_protect` into the stack
of decorators?

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:5>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Core (Cache system)
Version: | Severity: Normal
Resolution: | Keywords:
Triage Stage: Design | Has patch: 1
decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------

Comment (by carljm):

Replying to [comment:5 lukeplant]:

> It still hasn't been explained **why** `@vary_on_cookie` and
`@cache_page` don't work with CSRF pages. Idan had a sentence that looked
like it was about to explain it and then stopped. I'm guessing it is do
with the cookie being set by the middleware **after** the page has been
cached. Would the documentation be fixed by adding `@csrf_protect` into
the stack of decorators?

Which is why a new ticket should be created, so the actual problem can be
clearly identified and dealt with appropriately, without muddying the
waters of this ticket. As far as **this** ticket goes, the current
workaround documented in the CSRF docs **is** correct, and clearly so:
vary_on_cookie does in fact add the Vary: Cookie header, which
unequivocally fixes the problem of the response not having the Vary:
Cookie header. And that's the only problem this ticket is concerned about.

If there is a different problem with the current CSRF docs that needs
fixing, then it is a CSRF-specific problem unrelated to this ticket (even
if it seems superficially related because it involves the cache_page
header).

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:6>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Core (Cache system)
Version: | Severity: Normal
Resolution: | Keywords:
Triage Stage: Design | Has patch: 1
decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------

Comment (by russellm):

The reason why @vary_on_cookie and @cache_page doesn't work with CSRF
pages is exactly what this ticket describes. The new-style CSRF page puts
a CSRF token in the HTML. The page needs to be marked @vary_on_cookie
because the CSRF token, and thus the cached page content, varies on a per-
user basis. However, the @cache_page decorator caches the page before the
CSRF middleware has a chance to set the CSRF cookie. As a result,
@vary_on_cookie doesn't actually cause anything to vary -- there isn't a
cookie to vary on at the time the @cache_page decorator determines the
content to be cached.

This wasn't an issue with the old-style CSRF code because the page content
was modified by the CSRF middleware. The current docs were accurate for
the old CSRF approach, but not for the new, explicitly inserted {%
csrf_token %}.

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:7>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Core (Cache system)
Version: | Severity: Normal
Resolution: | Keywords:
Triage Stage: Design | Has patch: 1
decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------

Comment (by lukeplant):

Thanks Russell for the clarification. I'm pretty sure it can be fixed by
use of `@csrf_protect`:
{{{
#!python
@cache_page(60 * 15)
@vary_on_cookie
@csrf_protect
def my_view(request):
pass

}}}

But however the docs are fixed on this issue, it doesn't technically
address this bug, just one symptom of it.

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:8>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Core (Cache system)
Version: | Severity: Normal
Resolution: | Keywords:
Triage Stage: Design | Has patch: 1
decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------

Comment (by carljm):

Verified that Luke's fix works on both 1.3.X and trunk. In fact,
vary_on_cookie isn't even needed, since csrf_protect patches the vary
header -- the docs can just replace vary_on_cookie with csrf_protect.

Went a little ways down the path of an automated test to verify that this
works, but since we have tests for all the component pieces already, the
test ends up just being a verification that Python does indeed apply
decorators in the expected order. Which is kind of a stupid thing to test.

Applying the doc fix to 1.3.X and trunk.

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:9>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Core (Cache system)
Version: | Severity: Normal
Resolution: | Keywords:
Triage Stage: Design | Has patch: 1
decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------

Comment (by carljm):

In [16361]:
{{{
#!CommitTicketReference repository="" revision="16361"
Refs #15855 -- Recommended the csrf_protect decorator rather than
vary_on_cookie as workaround for cache_page caching the response before it
gets to middleware.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:10>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Core (Cache system)
Version: | Severity: Normal
Resolution: | Keywords:
Triage Stage: Design | Has patch: 1
decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------

Comment (by carljm):

In [16362]:
{{{
#!CommitTicketReference repository="" revision="16362"
[1.3.X] Refs #15855 -- Recommended the csrf_protect decorator rather than

vary_on_cookie as workaround for cache_page caching the response before it
gets to middleware.

Backport of r16361 from trunk.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:11>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Core (Cache system)
Version: | Severity: Normal
Resolution: | Keywords:

Triage Stage: Design | Has patch: 0

decision needed | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------

Changes (by carljm):

* has_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:12>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new

Component: Core (Cache system) | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Design
Has patch: 0 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by jedie):

* cc: django@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:13>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Design
Has patch: 0 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by inactivist):

* cc: inactivist@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:14>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Design
Has patch: 0 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by carljm):

Accepting this, since we're trying to eliminate the DDN state. There are
still significant design decisions needed in terms of how this should be
fixed, but I don't think there's any question it should be fixed (or else
the `cache_page` decorator should be removed). There's no scenario where
you want full-response caching that omits key Vary headers.

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:15>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+------------------------------------

Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version:
Severity: Normal | Resolution:

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 carljm):

* stage: Design decision needed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:16>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------

Comment (by carljm):

Just ran into Pyramid's
[http://docs.pylonsproject.org/projects/pyramid/en/1.3-branch/narr/hooks.html
#using-response-callbacks response callbacks] concept, and I think we
could consider introducing something like that as a fix for this bug. It
would be a new method on `HttpResponse` (in Pyramid it's on the request,
but that's only because you often don't have access to the actual response
yet; in Django you generally do) to register a callback to be executed
right before the response is sent out (i.e. after all middleware). So the
`cache_page` decorator would just register a response callback to do the
actual caching, thus the response that would be cached would be the final
one, after it's been processed by all middleware. There may be other uses
for this feature, but initially it could just be a private API until those
uses become clearer.

Presuming `cache_page` continued to be generated via
`decorator_from_middleware` from the cache middlewares, that would imply
`UpdateCacheMiddleware` would also register a response callback rather
than doing the caching immediately. This would be a change in behavior for
anyone who does not have `UpdateCacheMiddleware` first in their
middlewares (as [https://docs.djangoproject.com/en/dev/topics/cache/#the-
per-site-cache recommended in the docs]).

Actually, this would also obviate the need for the
`UpdateCacheMiddleware`/`FetchFromCacheMiddleware` split; we could just go
back to recommending the use of a single unified `CacheMiddleware` placed
last in `MIDDLEWARE_CLASSES`, and the actual cached response would still
always be the final about-to-be-sent-out response.

Thoughts?

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:17>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+------------------------------------
Reporter: carljm | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version:
Severity: Normal | Resolution:
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 syphar):

* cc: denis.cornehl@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:18>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+------------------------------------

Reporter: carljm | Owner: renskiy
Type: Bug | Status: assigned

Component: Core (Cache system) | Version:
Severity: Normal | Resolution:
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 renskiy):

* owner: nobody => renskiy
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:19>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+------------------------------------
Reporter: carljm | Owner: renskiy
Type: Bug | Status: assigned
Component: Core (Cache system) | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------

Changes (by charettes):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/7229 PR]

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:20>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware

-------------------------------------+-------------------------------------
Reporter: Carl Meyer | Owner: Rinat
| Khabibiev

Type: Bug | Status: assigned
Component: Core (Cache system) | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 1

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Alexandre Hajjar):

* needs_docs: 0 => 1

Comment:

Reviewed PR which seems fine: Vary headers from cache_page decorator &
other middlewares are taken into account by cachemiddleware, reg tests ok,
new tests added.

Added comments for minor improvements in PR. Needs update in docs IMHO.

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:21>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: Carl Meyer | Owner: Rinat
| Khabibiev
Type: Bug | Status: assigned
Component: Core (Cache system) | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Evstifeev Roman):

* cc: someuniquename@… (added)

Comment:

add someuni...@gmail.com to CC list

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:22>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+-------------------------------------
Reporter: Carl Meyer | Owner: Rinat
| Khabibiev
Type: Bug | Status: assigned
Component: Core (Cache system) | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Fernando Gutiérrez):

* cc: Fernando Gutiérrez (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:23>

[Django]

#15855: cache_page decorator bypasses any Vary headers set in middleware
-------------------------------------+------------------------------------

Reporter: Carl Meyer | Owner: (none)
Type: Bug | Status: new

Component: Core (Cache system) | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------

Changes (by Mariusz Felisiak):

* owner: Rinat Khabibiev => (none)
* status: assigned => new

--
Ticket URL: <https://code.djangoproject.com/ticket/15855#comment:24>