Opened 5 years ago

Closed 4 months ago

#13008 closed Bug (fixed)

@never_cache decorator should add 'no-cache' & 'no-store'

Reported by: harm Owned by: mbertheau
Component: Core (Cache system) Version: master
Severity: Normal Keywords:
Cc: obeattie, net147 Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I noticed that the never_cache django/views/decorators/cache.py does not add all the proper http headers to prevent that a page is cached.
It just adds a max-age=0. This does _not_ prevent browsers to actually cache that page.
For example when hitting back, firefox will happily show such a page if the connection to the server disappeared.

proposal, let never_cache() act as advertised: never cache. Let it add 'no-cache, must-revalidate' to the http response.

something like:

--- virtualenv/src/django/django/utils/cache.py 2010-03-02 15:11:54.000000000 +0100
+++ /tmp/cache.py       2010-03-02 15:11:48.000000000 +0100
@@ -104,6 +104,8 @@
         cache_timeout = settings.CACHE_MIDDLEWARE_SECONDS
     if cache_timeout < 0:
         cache_timeout = 0 # Can't have max-age negative
+        patch_cache_control(response, no_cache=True)
+        patch_cache_control(response, must_revalidate=True)
     if not response.has_header('ETag'):
         response['ETag'] = '"%s"' % md5_constructor(response.content).hexdigest()
     if not response.has_header('Last-Modified'):

Attachments (1)

13008.diff (507 bytes) - added by SmileyChris 3 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 5 years ago by russellm

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 follow-up: Changed 5 years ago by lukeplant

Personally, I really hate it when I press the back button and the browser tells me I can't view that page any longer. Adding this behaviour to all the admin pages could be a pain in the neck, especially in the common work flow of "list of objects -> click one to edit/view -> go back to the list because you don't need to change it". Oon the other hand I am probably a 'power user' who knows when I am likely to be dealing with old data, and I can always use new tabs. But I do think we should think about the usability impact on the admin, which has 'never_cache' applied everywhere.

comment:3 in reply to: ↑ 2 Changed 5 years ago by kmtracey

Replying to lukeplant:

Personally, I really hate it when I press the back button and the browser tells me I can't view that page any longer.

Me too. But I don't see that behavior (from Firefox 3.5.7 or 3.6, or Chrome 4.0) if I run with the proposed change. In fact I don't even see either of those browsers re-requesting the page on using the back and forward buttons if I run with the proposed change, so I'm not entirely sure the proposed change accomplishes what it was intended to.

I do see IE8 re-request the page with the proposed change. However, for the others I have to also add: patch_cache_control(response, no_store=True), before they will start re-fetching the page when accessed via the back/forward buttons. But when they refetch, they all do it automatically and not with any error message about not being able to access the page anymore (unless I stop the server and then I get the standard can't establish connection message).

comment:4 Changed 5 years ago by obeattie

  • Cc obeattie added

comment:5 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

@kmtracey - I just realised, we don't see those annoying pages about not being able to view the page because they happen when the page has been set to never cache/store AND was the result of a POST. Since we always re-direct after a POST in Django admin, we don't see those pages, and the web browser is happy to silently re-request the page, since the pages in history are always the result of GET requests. Well done us!

comment:6 Changed 4 years ago by harm

Besides the no_cache and must_revalidate this also needs no_store. (For Firefox)

+          patch_cache_control(response, no_store=True)

comment:7 Changed 4 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:8 Changed 4 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:9 Changed 3 years ago by eykd

Shouldn't this also set the Pragma: no-cache header? (See http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9)

Last edited 3 years ago by eykd (previous) (diff)

comment:10 Changed 3 years ago by harm

I think it should not be needed. Pragma: no-cache is intended for http requests, not http responses. (See http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9)

comment:11 Changed 3 years ago by SmileyChris

More explicitly, http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.32 says:

Note: because the meaning of "Pragma: no-cache as a response
header field is not actually specified, it does not provide a
reliable replacement for "Cache-Control: no-cache" in a response

I can't see must-revalidate being necessary, either. But maybe I'm overlooking something.

Changed 3 years ago by SmileyChris

comment:12 Changed 3 years ago by net147

  • Cc net147 added

comment:13 Changed 2 years ago by harm

confirmed, must-revalidate is not necessary.

tested adding cache control headers with

@cache_control(max_age=0, no_cache=True, no_store=True)

on FF 19.0 + FF 19.02 + chrome 25 (on mac)
A 'back' button in the browser, does NOT reload a view.

So I think your patch 13008.diff is correct.

Last edited 2 years ago by harm (previous) (diff)

comment:14 Changed 2 years ago by harm

  • Has patch set

comment:15 Changed 23 months ago by susan

  • Owner changed from nobody to susan
  • Status changed from new to assigned
  • Summary changed from @never_cache decorator should add 'no-cache' & 'must-revalidate' to @never_cache decorator should add 'no-cache' & 'no-store'

comment:16 Changed 23 months ago by susan

Where should I write a test? Should it be in cache/tests.py? The function add_never_cache_headers is never tested in thetests/ directory.

comment:17 Changed 10 months ago by thenewguy

This issue is related to https://code.djangoproject.com/ticket/23755 (patch_cache_control should special case "no-cache")

comment:18 Changed 4 months ago by mbertheau

  • Owner changed from susan to mbertheau

comment:19 Changed 4 months ago by MarkusH

  • Needs tests unset
  • Version changed from 1.2-beta to master

comment:20 Changed 4 months ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

The patch looks rather good. Tim, a last review?

comment:21 Changed 4 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 4a438e4:

Fixed #13008 -- Added more Cache-Control headers to never_cache() decorator.

Note: See TracTickets for help on using tickets.
Back to Top