Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#50422 new enhancement

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

Reported by: kzeni's profile KZeni Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 5.4.2
Component: General Keywords: needs-testing has-patch
Focuses: Cc:

Description

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.])

Attachments (1)

pluggable.php (99.0 KB) - added by KZeni 4 years ago.
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.])

Download all attachments as: .zip

Change History (4)

@KZeni
4 years ago

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.])

This ticket was mentioned in PR #340 on WordPress/wordpress-develop by KZeni.


4 years ago
#1

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

#2 @KZeni
4 years ago

In 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.

Last edited 4 years ago by KZeni (previous) (diff)

#3 @KZeni
3 years ago

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.

Note: See TracTickets for help on using tickets.