Make WordPress Core

Opened 11 years ago

Closed 3 months ago

Last modified 9 days ago

#21938 closed enhancement (fixed)

Add "no-store" to Cache-Control header to prevent history caching of admin resources

Reported by: soulseekah's profile soulseekah Owned by: johnbillion's profile johnbillion
Milestone: 6.3 Priority: normal
Severity: normal Version: 3.4
Component: Administration Keywords: has-patch has-unit-tests has-dev-note
Focuses: performance, privacy Cc:

Description

The current implementation of wp_get_nocache_headers does not take into account history caching, which results in a browser serving a cached copy of pages from history (by pressing the Back button) even if the user has long logged out.

RFC 2616 14.9.2 no-store describes this cache directive.

To repoduce: login to dashboard, logout, press the back button.
Expected: the login screen.
Reality: a copy of the previous page.

By adding the "no-store" directive to all non-cachable resources the behavior was mitigated successfully in Chrome 21, Firefox 15. Fails on Opera 12 (they chose to disregard "no-store" when applied to history, RFC allows this).

Attachments (1)

21938.patch (509 bytes) - added by soulseekah 11 years ago.
"no-store" please

Download all attachments as: .zip

Change History (50)

@soulseekah
11 years ago

"no-store" please

#1 @kovshenin
11 years ago

  • Cc kovshenin added

#2 @ocean90
11 years ago

  • Cc ocean90 added

#3 follow-up: @toscho
11 years ago

  • Cc info@… added

That needs good tests. For example the behavior after POST requests can be quite annoying. Performance might be an issue too.

Last edited 11 years ago by toscho (previous) (diff)

#4 in reply to: ↑ 3 @soulseekah
11 years ago

Replying to toscho:

That needs good tests. For example the behavior after POST requests can be quite annoying. Performance might be an issue too.

Agreed, under certain circumstances this could be a nuisance, especially if users are logged in and the admin bar is shown. As for performance, it seems that all of twitter is served with "no-store" and it does appear (to me) to be quite jagged navigation-wise.

Without a proper step-by-step testing plan in mind, I guess I'll use the patch in production for a bit to see if it causes any unexpected issues in general.

#5 @nacin
11 years ago

The other issue is sometimes you want the back button to be where you were last, without a complete reload. How many times have you lost a comment or some other content by clicking away (in WP or not) and scrambled to recover it?

#6 @SergeyBiryukov
11 years ago

  • Version changed from trunk to 3.4

#8 @nevma
9 years ago

It seems that Firefox actually needs the no-store as well, in order to not cache a page. Otherwise, it serves it from the BFCache. The no-store should be added to the wp_get_nocache_headers function in the Cache-Control header. This is what the MDN documentation in Using Firefox Caching mentions, too.

#9 @chriscct7
8 years ago

  • Keywords needs-patch added; has-patch removed
  • Severity changed from trivial to minor

#10 @dingo_bastard
5 years ago

Is this something that will be implemented or not? Since there exists a nocache_headers filter, if user wants to modify the Cache-Control all he/she has to do is unset it and then add no-store in it with the filter.

IMO this ticket can be closed.

#11 @vikram6
2 years ago

Hi. Are there any plans to fix this or should we workaround it on our side?

#12 @johnbillion
7 months ago

  • Focuses privacy added
  • Milestone changed from Awaiting Review to 6.3
  • Owner set to johnbillion
  • Status changed from new to accepted

Moving to 6.3 along with #57627

#13 @johnbillion
6 months ago

#57938 was marked as a duplicate.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


4 months ago

This ticket was mentioned in Slack in #core by oglekler. View the logs.


4 months ago

#16 @luehrsen
4 months ago

  • Keywords needs-refresh added

This ticket was mentioned in PR #4569 on WordPress/wordpress-develop by @luehrsen.


4 months ago
#17

  • Keywords has-patch added; needs-patch needs-refresh removed

Add the no-store keyword to the default nocache headers.

Trac ticket: #21938

#19 @johnbillion
4 months ago

Thanks for the PR @luehrsen! I've been working on this in combination with #57627 and the PR at https://github.com/WordPress/wordpress-develop/pull/4570 should address both of these tickets by conditionally adding the no-store and private directives when the user is logged in. Would you like to do some testing?

@luehrsen commented on PR #4570:


4 months ago
#20

While logged in, visiting the frontend: Cache-Control: no-cache, must-revalidate, max-age=0, no-store, private

While logged in, visiting the backend: Cache-Control: no-cache, must-revalidate, max-age=0, no-store, private

While logged out, visiting the frontend: No Cache-Control header is present.

While logged out, visiting the backend and redirected to wp-login: Cache-Control: no-cache, must-revalidate, max-age=0

While logged in, visiting the backend, performing a logout, pressing the BACK button in the browser: No admin page is shown, but the login page.

LGTM!

#21 @johnbillion
4 months ago

  • Keywords needs-testing added

#22 @Dharm1025
4 months ago

  • Keywords needs-testing removed

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/4570

Environment

  • OS: macOS Ventura 13.0
  • Web Server: nginx/1.25.0
  • PHP: 7.4.33
  • WordPress: 6.3-alpha-55505-src
  • Browser: Chrome Version 113.0.5672.126 (Official Build) (arm64)
  • Theme: Twenty Twenty-Three
  • Active Plugins: -

Test Results

✅ Works as expected with a patch.

I have tested the patch as per testing instructions and it works as expected.

Before Patch:

Cache-Control Header:

  1. Front-end (logged in): Cache-Control: no-cache, must-revalidate, max-age=0
  2. Front-end (not logged in): No Cache-Control present
  3. Back-end (logged in): Cache-Control: no-cache, must-revalidate, max-age=0
  4. wp-login.php page: Cache-Control: no-cache, must-revalidate, max-age=0

Login to wp-admin, then logout and press the back button in the browser shows the previous wp-admin page.

After Patch:

Cache-Control Header:

  1. Front-end (logged in): Cache-Control: no-cache, must-revalidate, max-age=0, no-store, private
  2. Front-end (not logged in): No Cache-Control present
  3. Back-end (logged in): Cache-Control: no-cache, must-revalidate, max-age=0, no-store, private
  4. wp-login.php Page: Cache-Control: no-cache, must-revalidate, max-age=0

Login to wp-admin, then logout and press the back button in the browser shows the wp-login page.

Thanks

#23 @oglekler
3 months ago

  • Keywords has-testing-info added

@paulkevan commented on PR #4570:


3 months ago
#24

Change looks good - looking at the spec it seems to make sense to have both.

#25 @johnbillion
3 months ago

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

In 55968:

Administration: Add the no-store and private directives to the Cache-Control header when preventing caching for logged in users.

The intention behind this change is to prevent sensitive data in responses for logged in users being cached and available to others, for example via the browser history after the user logs out.

The no-store directive instructs caches in the browser or within proxies not to store the response in the cache. This is subtly different from the no-cache directive which means the response can be cached but must be revalidated before re-use. WordPress does not use ETag headers by default therefore this does not achieve the same result.

The private directive complements the no-store directive by specifying that the response contains private information that should not be stored in a public cache. Som
e proxy caches may ignore the no-store directive but respect the private directive, thus it is included.

The existing Cache-Control header for users who are not logged in remains unchanged, and the existing cache prevention directives remain in place for backwards compatib
ility.

Props soulseekah, luehrsen, Dharm1025, markdoliner, rutviksavsani, ayeshrajans, paulkevan, clorith, andy786, johnbillion

Fixes #21938, Fixes #57627

#28 @johnbillion
3 months ago

  • Keywords needs-dev-note added

#29 @noisysocks
3 months ago

@soulseekah @johnbillion: Do either of you have any guesses as to why no-cache would affect E2E tests in WordPress and Gutenberg?

See:
https://core.trac.wordpress.org/ticket/58592
https://github.com/WordPress/gutenberg/pull/51826

#30 @soulseekah
3 months ago

@noisysocks the issues outlined in the pull seem to be timeout-related. Most probably, due to the no-cache calls caching has been disabled and load has been significantly increased (especially if tests are being run in parallel). I'm unfortunately unable to provide any more insight as I'm not familiar with how the E2E tests are developed and run.

#31 @johnbillion
3 months ago

  • Focuses performance added
  • Keywords needs-testing added; has-patch has-testing-info removed
  • Resolution fixed deleted
  • Severity changed from minor to normal
  • Status changed from closed to reopened

The load should not increase significantly during tests because the no-store directive affects whether a response can be fetched from the browser cache when navigating the browser history. If the tests aren't making use of the browser history then there should be no change to the load on the server. The browser should still continue to cache CSS and JS files that are loaded on the page.

There could well be a bug in the test driver or test suite which means this is not the case and additional load is being placed on the server, therefore increasing response times and causing timeouts. I'm also not familiar enough with the e2e test suite to know for sure.

I'm going to reopen this ticket and tag it with the performance focus in case there is a degradation or regression that's gone unnoticed.

Performance testing steps

  • Request multiple successive pages in the wp-admin area
  • Ensure that JS, CSS, and image assets loaded on the pages remain cached appropriately between successive requests
  • Ensure the browser back button causes the page to be re-requested from the server and not served from the cache, but that JS, CSS, and image assets are served from the cache
  • Test both with SCRIPT_DEBUG enabled and disabled

#32 @oglekler
3 months ago

  • Milestone changed from 6.3 to 6.4

Right now we don't have more time for testing, so, because there are doubts and this ticket still needs testing, I am moving this into 6.4. Let's plan it handling before Beta 1 of the next release, or better — much sooner.

#33 @johnbillion
3 months ago

This has already been committed for 6.3. The change itself is working as expected, I added the testing notes above in case an anomaly was found in a certain browser configuration or in the e2e test runner.

I'd like to keep this open for now and in the 6.3 milestone while the investigations into the e2e test failures are ongoing.

@oglekler Any objection to moving this back to 6.3? If so I'll have to revert.

#34 @oglekler
3 months ago

  • Milestone changed from 6.4 to 6.3

@johnbillion sorry, from the whole your comment it looked like it was not ready, and I didn't notice that changes weren't reverted. But I am not sure that this is the right way to open a ticket for watching if some side effect will happen, or we need to do something additional. Possibly for such things, we need to open another one or make and additional status for ticket. I believe that all tickets needs to be rechecked and tested after the Beta 1, but it looks like a pink pony world. I also believe that people who are checking that things are working after ticket is gone into trunk needs also be appreciated. This should be a normal practice - if you don't find any problem, if it doesn't mean that your efforts are not valuable. But this question it out of the scope of this ticket 😅

#35 @johnbillion
3 months ago

Ok I am going to open a PR with this change reverted and see what happens with the e2e tests. Stand by.

This ticket was mentioned in PR #4723 on WordPress/wordpress-develop by @johnbillion.


3 months ago
#36

  • Keywords has-patch added

#37 @johnbillion
3 months ago

  • Keywords needs-testing removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Ok the test run on https://github.com/WordPress/wordpress-develop/actions/runs/5392522701 has produced the same e2e test failures as we have in trunk. Therefore it appears that the introduction of the no-store cache control directive is a red herring.

I'll re-close this ticket as fixed for 6.3.

This ticket was mentioned in PR #4785 on WordPress/wordpress-develop by joemcgill.


3 months ago
#38

  • Keywords has-unit-tests added

This adjusts the "No private directive present in cache control when user not logged in" test so they won't fail if the headers don't contain a cache-control key at all.

While trying to debug why E2E tests have been flaky, I noticed that the logged out test for cache control headers was often failing locally because the response headers don't have a cache-control key at all, resulting in a Matcher error: received value must not be null nor undefined failure. This adjusts the tests to use expect.not.objectContaining() instead.

Trac ticket: https://core.trac.wordpress.org/ticket/21938

#39 @joemcgill
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#40 @joemcgill
3 months ago

Reopening to consider adjusting the unit tests. The main functionality that has already been committed still applies.

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


3 months ago

@johnbillion commented on PR #4785:


3 months ago
#42

The tests are running fine for me locally with this change, but CI still isn't happy. I'll get this in anyway.

#43 @johnbillion
3 months ago

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

In 56134:

Administration: Adjust the "No private directive present in cache control when user not logged in" test so the assertions won't fail if the headers don't contain a cache-control key at all.

Props joemcgill

Fixes #21938

joemcgill commented on PR #4785:


3 months ago
#45

Thanks @johnbillion. Agree with you that this doesn't fix the timeouts that we're seeing in CI, but this does make it easier to run these tests locally while debugging.

#46 @stevenlinx
2 months ago

  • Keywords has-dev-note added; needs-dev-note removed

#47 @westonruter
4 weeks ago

Coincidentally, I've been looking into removing use of the unload event (#55491) because Chrome intends to deprecate it, and more importantly because it prevents bfcache. But something else that blocks bfcache is Cache-Control: no-store, which this ticket is all about. Adding no-store wouldn't have caused any performance regression in the admin in 6.3 because wp-heartbeat uses the unload event. However, with this removed, the introduction of no-store holds back the performance of page navigations in the admin and the frontend by disabling bfcache.

The question I have is whether the increase to security/privacy by disabling bfcache for logged-in users is worth the performance hit for the 80% of users. If not, perhaps adding no-store should be a privacy/security enhancement that site owners install via a plugin when a site is accessed by users who use shared computers? Alternatively, perhaps no-store should only be used by default when a user does not check the "remember me" checkbox when logging-in? POC plugin: https://gist.github.com/westonruter/8c19d87a80a36e8f24db910750162628

Last edited 4 weeks ago by westonruter (previous) (diff)

This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.


3 weeks ago

This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.


9 days ago

Note: See TracTickets for help on using tickets.