Make WordPress Core

Opened 7 months ago

Closed 11 days ago

Last modified 11 days ago

#61711 closed defect (bug) (fixed)

Password-protected pages lacking appropriate 'Cache-Control' request header

Reported by: brevilo's profile brevilo Owned by: johnbillion's profile johnbillion
Milestone: 6.8 Priority: normal
Severity: normal Version: 2.0.5
Component: Posts, Post Types Keywords: has-patch has-testing-info has-unit-tests 2nd-opinion
Focuses: Cc:

Description

Over the years there have been recurring reports of users having issues with password-protected pages which simply redirected to the password prompt again, instead of the actually protected page (after successful login), e.g.:

The tickets got closed with "worksforme" and the discussions kept looking at the WP config, plugins or client-side issues, but so far without reliable solutions. Since I myself ran into the same problem as so many others before, I now analyzed it and found the (well, at least a) culprit.

I narrowed it down to be a caching problem at my hosting provider, seemingly using Nginx as a caching reverse-proxy! This could also explain the "worksforme" closures above: if your provider doesn't use such caching (or a CDN for that matter), you won't reproduce it.

In my case the response headers (after the successful) redirect contained:
X-Cache-Status: HIT (see this FAQ)

As a workround I now send the appropriate header tailored for this situation, as part of my theme:

<?php
header('Cache-Control: private');

For me, and probably a lot of others (reverse-proxy caching or CDNs are widely used) the solution would be to do this for all password-protected pages. Remember that header() must be called before any actual output is sent.

Thanks

Attachments (3)

61711.diff (591 bytes) - added by haozi 3 months ago.
61711-unit-test.diff (866 bytes) - added by ironprogrammer 2 months ago.
Unit test for protected post Cache-Control header
61711-2.diff (591 bytes) - added by haozi 2 weeks ago.

Download all attachments as: .zip

Change History (32)

#1 @brevilo
7 months ago

Here's one of many examples where troubled users switched their provider and the problem was gone. To me this also hints at a reverse-proxy setup. This fix could really positively affect as lot of installations, I reckon.

Last edited 7 months ago by brevilo (previous) (diff)

#2 @brevilo
7 months ago

  • Component changed from General to Security

#3 @hellofromTonya
5 months ago

  • Version changed from 6.6 to 2.0.5

Changing the Version from 6.6 to 2.0.5.

#4 @johnbillion
4 months ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

Thanks for the report @brevilo! Would you be willing to work on a PR or patch to get this fixed?

#5 @brevilo
4 months ago

Willing, sure, but I've never seen this particular codebase before and I don't know your dev process. Given that the fix should be trivial (a one-liner in the right place), I'm sure an existing dev can do a much better and faster job than me. I don't care about authorship credit, so please feel free to just fix it :)

Cheers

#6 @johnbillion
3 months ago

  • Milestone changed from Future Release to 6.8

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


3 months ago
#7

  • Keywords has-patch added; needs-patch removed

This PR add no-cache headers for password protected posts to avoid reverse-proxy or CDNs caching.

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

@haozi
3 months ago

#8 follow-up: @haozi
3 months ago

@brevilo I added a patch for this, could you test it?

#9 @haozi
3 months ago

  • Keywords needs-testing added; good-first-bug removed

#10 in reply to: ↑ 8 @brevilo
3 months ago

Replying to haozi:

@brevilo I added a patch for this, could you test it?

Thanks for the patch! I'll try to test it in the coming days. Stay tuned.

Cheers

brevilo commented on PR #7858:


3 months ago
#11

Just adding myself as the reporter of the proposed fix 😇 ...

@brevilo commented on PR #7858:


3 months ago
#12

Linked accounts (sorry for the noise)...

@brevilo commented on PR #7858:


3 months ago
#13

I reviewed and tested the patch against WP 6.7.1 and can confirm it to be working as expected.

Thanks 👍

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


2 months ago

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


2 months ago

#16 @johnbillion
2 months ago

  • Owner set to johnbillion
  • Status changed from new to accepted

@narenin commented on PR #7858:


2 months ago
#17

@devhaozi The PR looks good to me.

#18 @narenin
2 months ago

  • Keywords has-testing-info added

Test Report

Patch Tested: https://github.com/WordPress/wordpress-develop/pull/7858

Environment
PHP: 8.0
WordPress: 6.7.1
Active Plugins: None

Actual Results:

✅ The patch is working as expected. This will add appropriate Cache-Control to the Header.

#19 @ironprogrammer
2 months ago

Thanks for the patch and testing, folks!

Test Report

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

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 14.7.1
  • Browser: Safari 18.1.1
  • Server: nginx/1.27.3
  • PHP: 8.4.1 (gd)
  • MySQL: 8.0.27
  • WordPress: 6.8-alpha-59274-src / WP-CLI 2.11.0

Actual Results

When not logged in:

  • ✅ On a password-protected post (before entering a password), the headers include Cache-Control: no-cache, must-revalidate, max-age=0.

Additional Notes

  • After entering the password on the post, the Cache-Control header is no longer present.
  • The no-store, private directives are additionally added for logged-in users.

@ironprogrammer
2 months ago

Unit test for protected post Cache-Control header

#20 @ironprogrammer
2 months ago

  • Keywords has-unit-tests added

I've attached a unit test to validate this update. I don't think we need a reverse test, as omitting this header is already the default for non-logged-in requests.

#21 @haozi
2 months ago

  • Keywords needs-testing removed

@johnbillion Can you help merge this?

#22 @johnbillion
3 weeks ago

  • Component changed from Security to Posts, Post Types

#23 @johnbillion
3 weeks ago

  • Keywords 2nd-opinion added

Given the reverse proxy configuration from the ticket description, I'm concerned that the proposed change will have the side effect of allowing the password protected page content to be written to the page cache after the password has been entered by a visitor.

Reasoning: If the request after the redirect can return a cache read HIT despite the presence of a wp-postpass_COOKIEHASH cookie (causing the bug reported in this ticket), then it's reasonable to assume it can also write to the cache with the current page contents when there isn't a hit. This enables a form of cache poisoning that facilitates exposing the password protected content to visitors who have not entered the password but who are served the cached response that was written after a visitor entered the correct post password.

I think the complete solution here is to always send no-cache headers for password protected posts, regardless of whether the visitor has entered a password. This prevents caching of either the password input form or the password protected content. I think that is what @brevilo's code in their theme is doing, but the patch on this ticket took a different approach.

@haozi @ironprogrammer @brevilo What do you think?

#24 @brevilo
2 weeks ago

Thanks John, you might be right there. I opted for private in my proposed fix as I wanted to take the reverse proxy cache entirely out of the equation, to be on the safe side with regards to fixing the problem I saw.

I didn't question the alternative approach by haozi since it also fixed the problem in my tests and my experience with reverse proxy caching is limited.

So, yes, private is less efficient, but that's probably justified for protected content. The same is probably true for when to send that header. I'd expect that to be required for the password entry page as well as for the protected pages themselves.

Cheers

@haozi
2 weeks ago

#25 @haozi
2 weeks ago

For this case, maybe can check post_password directly.

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


11 days ago
#26

#27 @johnbillion
11 days ago

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

In 59728:

Posts, Post Types: Add no-cache headers to password protected posts.

This instructs an intermediate cache, for example a proxy server, to not cache a password protected post both before and after a visitor has entered a password.

Props brevilo, haozi, ironprogrammer, narenin

Fixes #61711

#28 @brevilo
11 days ago

Thanks everyone involved! If my historic assessment is correct, this will make a lot of people very happy! If only we could get someone to update this discussion a final time to let them know that a solution is likely finally on the way...

That said, the release target is still 6.8, right?

Cheers (literally!)

#29 @ironprogrammer
11 days ago

@brevilo, you might be able to get someone in the #forums channel to add a comment about this being addressed as of 6.8. It might be helpful to write a brief synopsis for them that they can use to link here.

Note: See TracTickets for help on using tickets.