Prevent Browser Caching From Getting Involved With wp_redirect and wp_safe_redirect (Leaving the Browser to Purely Honor the Redirect Code Used)

[KZeni]

Currently, wp_redirect (and therefore wp_safe_redirect as well since that uses wp_redirect) can be cached by web browsers depending on a site's browser caching rules. For example, W3 Total Cache has browser caching that one would certainly want to utilize for improved site performance, but then doesn't really have the means to prevent this potential problem where a redirect then gets cached by the browser (even if it's a 302 redirect). W3TC is just one example, but browser caching can be implemented in any number of ways which this then can be a problem.

I came across this with a plugin and proposed a patch to the plugin per ​https://wordpress.org/support/topic/proposed-bugfix-prevent-login-redirect-from-browser-cache-rules/, but I'm wondering why the fix for this isn't part of WordPress' core so that plugins & developers in general don't need to specifically add this to prevent an issue that can come about given the right circumstances (with the fix being rather simple & straightforward, from what I can tell.)

In short, having nocache_headers(); before the redirect happens has the redirect still honored as it was set while removing the potential undesired effect of it being cached by the web browser (ex. trying to view a page that needs you logged in to view it has a plugin redirect to the login page, but then that 302 redirect was cached by the browser so the user is still redirected to the login when trying to visit that page even after they've already logged in, and it can really happen with any redirect that should be honored as being temporary that was then cached as the page's result by the browser when it shouldn't be [translation plugins have also encountered this in the past as well when switching between languages for pages via a redirect, and I'm sure there's more instances of this.])

---

Yes, currently developers can prevent this themselves by implementing nocache_headers(); before their wp_redirect and/or wp_safe_redirect (I've made sure to document this as such at ​https://developer.wordpress.org/reference/functions/wp_redirect/#comment-3973 and ​https://developer.wordpress.org/reference/functions/wp_safe_redirect/#comment-3974, respectively [which I imagine both of these notes should be removed if/when this gets implemented natively]), but I'm wondering why that should be necessary. Would there really be any downside of having wp_redirect trigger nocache_headers(); as part of that function natively so it just works more reliably without added code and/or developers potentially not knowing to do this which then results in problematic behavior?

At that point, things like the auth_redirect function (and possibly others) can be cleaned up to not need to include nocache_headers(); before their redirect. Meanwhile, wp_get_nocache_headers(); (used by nocache_headers();) checks to see if headers have already been sent or not & also honors the existing headers while just enforcing specific ones so those that have this applied twice (ex. after this is included natively as part of wp_redirect while they still have it in their own implementation before this was done) shouldn't have any negative effects.

---

- The Proposed Patch -
This simply has nocache_headers(); // Prevent browser caching of page with the redirect header (browser should still honor the redirect status code) before the header( "Location: $location", true, $status ); snippet in the wp_redirect function within wp-includes/pluggable.php, and then has a minor optimization of removing nocache_headers(); from the auth_redirect function in that same file since that's no longer needed (the wp_redirect later in the function now takes care of this. I've attached a patched version of pluggable.php from WP 5.4.2 with these 2 changes.

I filed this as an enhancement since it's not really a bug within the code being fixed as much as it is preventing bugs made by plugin & other developers (so it still is fixing bugs within the WordPress ecosystem [just going to the core of the problem rather than having to fix the issue with each instance where this came up as a result of the core code not taking care of this itself yet.])

[KZeni]

A patched version of pluggable.php from WP 5.4.2 with these 2 changes (add nocache_headers to wp_redirect and remove nocache_headers from auth_redirect [since it then uses wp_redirect that handles that now.])

Have wp_redirect use nocache_headers (then clean up auth_redirect since it then uses the updated wp_redirect & doesn't need to call nocache_headers itself.)

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

[KZeni]

Summary, why should browser caching be allowed to be potentially involved with a redirect when it should be up to the status code of the redirect to determine the permanence/behavior of the redirect? This just makes it so browser cache doesn't get in the way (be it a caching plugin, custom headers set by the site/server, etc.) and does so in a way that doesn't put it on plugin & other developers to account for this when it should be harmless (301 redirects should still be seen as such, little-to-no overhead, etc.) & only helpful (can help simplify code found elsewhere, prevent issues that may come up in certain circumstances, etc.) to have WordPress take care of it natively.

[KZeni]

Just want to give this a mention as it seems we have plugins like Profile Builder having manually implemented a precaution against this (per ​https://wordpress.org/support/topic/proposed-bugfix-prevent-login-redirect-from-browser-cache-rules/), but then there are others like that such as Subway (​https://wordpress.org/plugins/subway/) that might have this issue left unaddressed yet (going to be submitting a similar patch to them to fix it.)

Meanwhile, I'm thinking there are certainly other plugins experiencing this bug now and/or definitely in the future until they find out they need to manually account for this.

I think it really could/should be something WordPress itself just takes care of just as part of the redirect functions as proposed here so plugins aren't left to potentially having bugs with caching & redirects then not working properly and/or leaving the plugin developers to then implement a protection against it when it really shouldn't harm anything if WordPress did it for all developers just by using the built-in redirect functions.

[shoelaced]

Is there a reason this fix hasn't been/can't be implemented? Developing a plugin and running into this issue. I'll use nocache_headers() but it seems like the redirect functions should simply incorporate that (and it would have saved me a lot of time).

[KZeni]

Replying to shoelaced:

Is there a reason this fix hasn't been/can't be implemented? Developing a plugin and running into this issue. I'll use nocache_headers() but it seems like the redirect functions should simply incorporate that (and it would have saved me a lot of time).

Good question. I did what I could in the time I had available, but it somehow never got the proper attention of WordPress maintainers in the 5+ years since I reported it, gave a fix, gave updates, made sure it was properly represented in the GitHub issues as well, reached out again, etc. I'm surprised to hear this was just never addressed even from a different ticket, update, or anything (I had just made sure the plugins I use all implemented their own patch for this in the meantime.)

It really appears to be such a simple & harmless streamlining of this function that only makes things more reliable (and in some potentially important ways like with different redirects that may need to vary on the user's current permissions or some other temporary condition where this not being fixed makes even 302 temporary redirects be saved to cache [while this fix should still allow 301 redirects to be permanent/"cached" when used accordingly and so on.])

I guess they just kept relying on plugins to manually call the nocache_headers(); function anytime a redirect might need to make WordPress' less-than-ideal handling of this actually more reliable & security conscious (and that's assuming the plugin developer even knows about this bug they need to avoid & isn't just leading to however many plugins having open bugs/issues from this unexpected behavior.)

It really seems like the redirect status code one supplies when calling the redirect gives sufficient control over things that could assist with performance, if needed. Meanwhile, having WordPress potentially make something be saved & reused even though it was already told to be temporary doesn't seem like the correct way it should function. Though here I am going over potential concerns for a ticket/issue/patch awaiting review that just never got reviewed in these years for whatever reason.

Better late than never if this gets traction, I suppose. I hope it actually does get addressed at some point since the way it is now just has some counterintuitive behavior that can/has/does lead to potentially severe bugs on a given site / web app (I still just don't get why it currently makes temporary redirects potentially not temporary unless given additional code [as if one needs to specify temporary more than once & also needs to know about this whole other nocache_headers(); function to have it work as one would expect.])

[shoelaced]

Replying to KZeni:

It really appears to be such a simple & harmless streamlining of this function that only makes things more reliable

I agree — the only reason I discovered I was having the issue at all was purely by chance, because it worked fine without nocache_headers() in most cases while I was working on it. I could easily see someone having no idea there's an issue until someone complains, and at least in my case, I don't even remember how I found this solution.

In any case, I have it working now and hopefully I'll remember this the next time I use a redirect, but it does seem like a simple and sensible fix that I hope gets attention.

[tha_sun]

–1

Adding nocache_headers() to every redirect is not correct and should not be accepted.

The situation you are describing only applies to the special case of switching between anonymous and authenticated user sessions.

For most other redirects, it is actually advisable to let layers in front of the site cache the redirect response, so that the same response does not have to be regenerated all over again.

The current proposal won't fix in my opinion, because it would enforce a behavior that does not make sense for the vast majority of redirect use cases.

[shoelaced]

Replying to tha_sun:

The current proposal won't fix in my opinion, because it would enforce a behavior that does not make sense for the vast majority of redirect use cases.

In that case, perhaps it would make sense to add a $cache_headers parameter to the redirect functions, defaulting to true, that would simplify these less common cases? Obviously it would still require a conscious choice by the developer, but at least then it would be more visible and appear in IDE tools that display the documentation of WP functions.

Mainly my annoyance with this came from spending a lot of time debugging, and I did check the function's listing in the WP docs multiple times during that time to make sure I was using it correctly, but it wasn't until I finally noticed @KZeni 's note at the bottom that I learned about this issue and found this ticket. Being able to see the option in the parameters would have at least made me aware much sooner that this could be a potential issue and provided a way to disable it without yet more research.