Problem / Motivation

When using the page cache, we will generate the following HTTP headers when we need to by-pass the cache:

Cache-Control: must-revalidate, no-cache, private

This appears to be the intended behavior per https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/page_...

This is also what happens when you use the page cache kill switch: \Drupal::service('page_cache_kill_switch')->trigger();. You can also see this in action by inspecting the headers of https://dri.es/status.

The problem is that this is a bad way to tell a browser and any proxy cache not to cache a page:

  • must-revalidate means that the cache must not use the resource after it becomes stale. It needs to first revalidate the request with the origin server.
  • no-cache means the resource can be cached, but that it must be revalidated each time before using it. The name of the header is somewhat counter-intuitive.
  • private means that the resource can't be cached by proxies (e.g. Varnish, CDN), but that it is ok for a browser to cache the resource.

In other words, these headers are at odds with one another:

  1. no-cache allows caching by proxies but requires revalidation. This makes must-revalidate redundant.
  2. private disallows caching by proxies (only the browser can cache). This is at odds with no-cache which allows proxies to cache the resource, as long they always revalidate first.

Proposed Resolution

It is better to use the following header:

Cache-Control: no-store

no-store means that the resource can't be stored by any cache, including the browser's cache.

There appear to be two parts to the fix:

  1. Change behavior of FinishResponseSubscriber::setCacheControlNoCache() and added a new function FinishResponseSubscriber::setCacheControlNoStore(). setCacheControlNoCache() is no longer used in core, but given that it is a protected method, it might be used by contributed modules. I don't think we can remove it. An alternative solution is to change the implementation of setCacheControlNoCache(), and not introduce setCacheControlNoStore().
  2. private and no-cache are set at the same time because $response->headers->set() appends headers by default. This is fixed by setting the 3rd parameter to TRUE.

Behavior Changes

Question: When this fix is applied, what will change about how Drupal is cached?

Answer: This will improve the browser's back button behavior for authenticated users: after the user logs out, its no longer possible to see browsers cached content with back-button. This is true for Chrome, Edge, Firefox. Safari has an bug on handling Cache-Control headers and there behavior does not change. If we find a work-a-round, we can implement it on #1912514: Using the back button after logging out shows you pages from the authenticated user's session

CommentFileSizeAuthor
#25 interdiff_22-25.txt1.73 KBweekbeforenext
#25 better-cache-headers-3130912-25.patch5.22 KBweekbeforenext
#22 interdiff_20-22.txt1.76 KBweekbeforenext
#22 better-cache-headers-3130912-22.patch5.19 KBweekbeforenext
#20 3130912-20.patch3.29 KB_utsavsharma
#20 interdiff_19-20.txt934 bytes_utsavsharma
#19 interdiff_4-19.txt1.51 KBweekbeforenext
#19 better-cache-headers-3130912-19.patch3.29 KBweekbeforenext
#14 3130912_d7.patch636 bytesmpp
#5 better-cache-headers-3130912-4.patch1.78 KBkirkkala
#3 kill-switch-example.png490.45 KBdries
better-cache-headers.patch1.81 KBdries

Issue fork drupal-3130912

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Dries created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Incorrect Cache-Control headers » Incorrect Cache-Control headers for authenticated users
Component: cache system » page_cache.module

no-cache, must-revalidate originates from Drupal 7: https://git.drupalcode.org/project/drupal/-/blob/7.x/includes/bootstrap..... That's also what \Drupal\Core\EventSubscriber\FinishResponseSubscriber::setCacheControlNoCache() in D8/D9 does.

private is AFAICT only added for authenticated users. The test expectations prove this, and you even quote that at the beginning of the issue summary. So, expanding the issue title.

It is better to use the following header:

Cache-Control: no-store

In my experience, it's almost never quite this simple. There's a lot of dark magic and obscure knowledge in Cache-Control response directive land. I did some digging and found:

IOW: no-store breaks the browser's "back button cache".

Based on my digging, this seems accurate though:

no-cache allows caching by proxies but requires revalidation. This makes must-revalidate redundant.

Curious what Page Cache maintainer @znerol thinks!

dries’s picture

dries commented
StatusFileSize
new kill-switch-example.png490.45 KB

I believe my current patch only affects the kill switch, \Drupal::service('page_cache_kill_switch')->trigger(). The kill switch is applied to anonymous users, so it's not strictly for authenticated users. As you can see from the screenshot private get added to Cache-Control, even for anonymous visitors. Try it at https://dri.es/status.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

kirkkala’s picture

kirkkala
He/him
Primary language Finnish
Location Helsinki
commented
StatusFileSize
new better-cache-headers-3130912-4.patch1.78 KB

Thanks, we needed this header change after a security audit finding.

The original patch though has `docroot/` in paths which causes it to fail from applying. Re-adding with fixed paths.

Status: Needs review » Needs work

The last submitted patch, 5: better-cache-headers-3130912-4.patch, failed testing. View results

josephdpurcell’s picture

josephdpurcell commented
Related issues: +#1912514: Using the back button after logging out shows you pages from the authenticated user's session

I know this ticket is technical in its focus, but I'm curious if there was a functional issue that prompted this ticket? I found this when digging into #1912514: Using the back button after logging out shows you pages from the authenticated user's session. I'm going to set this ticket as related, in the event that no-store affects the other ticket.

josephdpurcell’s picture

josephdpurcell commented
Issue summary: View changes

Testing results on Drupal 8.9.1 using Firefox 78.0.2.

Note: when testing locally, ensure you do not have "Vary: Cookie" as a header in the server response.

Test scenario: logged in then logged out, "Back" does not show authenticated cache

Before applying patch #5

* Go to login page: GET request at /user/login has header: cache-control: max-age=1800, public
* Attempt login with invalid credentials: POST /user/login has header: cache-control: must-revalidate, no-cache, private
* Attempt login with valid credentials: POST /user/login has header: cache-control: must-revalidate, no-cache, private
* User is redirected on login: GET /user/123 has header: cache-control: must-revalidate, no-cache, private
* Logout: GET /user/logout has header: cache-control: must-revalidate, no-cache, private
* User is redirected on logout: GET / has header: cache-control: max-age=1800, public
* Click the "Back" button in the browser: GET /user/123 is served from browser cache and shows the user is logged in (e.g. Drupal admin toolbar is present)

After applying patch #5

* Go to login page: GET request at /user/login has header: cache-control: max-age=1800, public
* Attempt login with invalid credentials: POST /user/login has header: cache-control: no-store, private
* Attempt login with valid credentials: POST /user/login has header: cache-control: no-store, private
* User is redirected on login: GET /user/123 has header: cache-control: no-store, private
* Logout: GET /user/logout has header: cache-control: no-store, private
* User is redirected on logout: GET / has header: cache-control: max-age=1800, public
* Click the "Back" button in the browser: GET /user/123 returns a 403 forbidden

Test scenario: logged out then logged in, "Back" does not show anonymous cache

Assume there is an about us page at /about-us.

Before applying patch #5

* Go to about page: GET request at /about-us has header: cache-control: max-age=1800, public
* Go to login page: GET request at /user/login has header: cache-control: max-age=1800, public
* Attempt login with valid credentials: POST /user/login has header: cache-control: no-store, private
* User is redirected on login: GET /user/123 has header: cache-control: no-store, private
* Click the "Back" button in the browser and select About page: GET /about-us is served from cache and shows the user is not logged in (e.g. Drupal admin toolbar is missing)

After applying patch #5

Same results.

My interest in testing is for #1912514. It appears this patch #5 is a potential solution for it. Thanks for submitting!

I did not test on a fresh Drupal install. Given how severe of an impact this could have, it would be good to see automated tests and perhaps some anecdotes from anyone using this approach on a live site. It would also be good to get more input on the cache-control choice, based on comment #2 it sounds like it might not be clear what the header value should be for certain? Also, it sounds like this change would affect some behavior of Drupal which should be documented.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Related issues: +#3205494: Prevent re-using the password reset confirmation form

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

cilefen’s picture

cilefen commented
Related issues: +#2593887: Follow-up: Do not cache private or no-store responses in PageCache

mpp made their first commit to this issue’s fork.

mpp’s picture

mpp commented
StatusFileSize
new 3130912_d7.patch636 bytes

Drupal 7 has the same conflicting cache directives.

Replaced must-revalidate (force browser to revalidate cache) by no-store (avoid caching in the browser).
From a security perspective, this is an important difference: we should avoid caching personal data in the browser.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

johnwebdev’s picture

johnwebdev commented

Regarding the behavior changes, #3209204-8: Adding existing translation throws "Invalid translation language specified" illustrates an example where that bug behaves differently depending on the browser you use. With this patch change, the Chrome behavior works as the Edge one, which IMO is the right behavior.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

weekbeforenext’s picture

weekbeforenext
Primary language English
Location Asheville, NC
commented
Status: Needs work » Needs review
StatusFileSize
new better-cache-headers-3130912-19.patch3.29 KB
new interdiff_4-19.txt1.51 KB

The patch from comment #5 worked for me in 9.4.9. I created a new patch to include fixes to the failing tests (hopefully).

_utsavsharma’s picture

_utsavsharma commented
StatusFileSize
new interdiff_19-20.txt934 bytes
new 3130912-20.patch3.29 KB

Fixed CCF for #19.

Status: Needs review » Needs work

The last submitted patch, 20: 3130912-20.patch, failed testing. View results

weekbeforenext’s picture

weekbeforenext
Primary language English
Location Asheville, NC
commented
StatusFileSize
new better-cache-headers-3130912-22.patch5.19 KB
new interdiff_20-22.txt1.76 KB

Updated the patch to fix more test failures.

weekbeforenext’s picture

weekbeforenext
Primary language English
Location Asheville, NC
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: better-cache-headers-3130912-22.patch, failed testing. View results

weekbeforenext’s picture

weekbeforenext
Primary language English
Location Asheville, NC
commented
Status: Needs work » Needs review
StatusFileSize
new better-cache-headers-3130912-25.patch5.22 KB
new interdiff_22-25.txt1.73 KB

Fixing test failures... again.

weekbeforenext’s picture

weekbeforenext
Primary language English
Location Asheville, NC
commented
Issue tags: +fldc23
znerol’s picture

znerol commented
Status: Needs review » Postponed

It is better to use the following header:

Cache-Control: no-store

I disagree for the reasons already stated in #2. The approach taken here is very probably overzealous and has the potential to badly affect UX.

I propose to postpone this and then reevaluate after the Vary header has been fixed in #1912514: Using the back button after logging out shows you pages from the authenticated user's session.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

johnv’s picture

johnv
Primary language Dutch
commented
Component: page_cache.module » cache system

As I understand, the page_cache module is only intended for anonymous users, so moving to other component.

sokru made their first commit to this issue’s fork.

sokru opened merge request !7288

sokru’s picture

sokru commented
Component: cache system » request processing system
Issue summary: View changes
Status: Postponed » Needs review
Issue tags: -page cache

On Slack @catch suggested moving this to "request processing system" since this is not cache subsystem issue. Cleaned the tags based on that.

The scope of this issue should be making sure the private information is not stored in browser disk. This will resolve the security scanner reports mention on #5. Changing the Cache-Control header from must-revalidate, no-cache into no-store fixes the #1912514: Using the back button after logging out shows you pages from the authenticated user's session on Firefox, Chrome, Edge, but not on Safari, see https://discussions.apple.com/thread/251817133. I'd suggest leaving that issue to solve issue with Safari.

sokru changed the visibility of the branch 10.0.x to hidden.

sokru changed the visibility of the branch 7.x to hidden.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

but not on Safari, see https://discussions.apple.com/thread/251817133.

hm, does Safari also ignores must-understand Cache control value? :O

znerol’s picture

znerol commented

Serving responses with Cache-Control: no-store has been the source of wicked UX issues in the past. This needs careful manual evaluation of multiple scenarios across popular browsers. Some scenarios are described in this rather old atlassion blog post.

One scenario based on Drupal core alone could use the contact form together with a misconfigured e-mail transport. This accurately simulates a situation on a production site where a mail server isn't reachable for a short time. In this case an error message is displayed after the submit button has been pressed. My reaction as a user of this site would be to press the back button in order to get back to the text I've just written, either to save it to a file or to retry the form submission. On a site with Cache-Control: no-store, I fear that the browser will render an empty contact form and that all text is gone.

My hunch is that the Cache-Control: no-store response header should be used on specific routes, i.e., on pages which display sensitive / confidential data. Examples of this type of information would be social security number, personal health information or credit-card data. Forms which accept and pages which display this kind of data probably should supply their own Cache-Control: no-store header. And security audits will rightfully flag such pages if that isn't the case.

Also please note that results of any automated reporting tools (including security scanners) need to be interpreted by people with knowledge in that particular field. Comment #5 gives no details about the type of application, the scope of the audit and the reason for the suggested change.

sokru’s picture

sokru commented

hm, does Safari also ignores must-understand Cache control value? :O

Yes, Safari does ignore must-understand with back-button, Safari behavior is similar to other browser if the "disable cache" is selected on developer toolbar.

Serving responses with Cache-Control: no-store has been the source of wicked UX issues in the past. This needs careful manual evaluation of multiple scenarios across popular browsers. Some scenarios are described in this rather old atlassion blog post.

It would be very beneficial to get these weird UX issues documented with repro steps. On core's /contact/feedback contact form I was not able to find any difference between no-store and must-revalidate, no-cache, using browsers back & forward buttons.

My hunch is that the Cache-Control: no-store response header should be used on specific routes, i.e., on pages which display sensitive / confidential data

Or we could make Drupal more secure by default using no-store and if people need better caching, they could change the response header. Many times even the unpublished node title could contain confidential data.

But I acknowledge there are risks of making this change, so it might be best just to close this issue and introduce a contrib module with eventSubscriber using Symfony's HeaderBag to get a desired Cache-Control header.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs manual testing

I think we need to manually test and document browser behaviour in the issue summary, marking needs work for that.

fskreuz made their first commit to this issue’s fork.

diegodz changed the visibility of the branch 10.0.x to active.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.