wp-media / wp-rocket

Performance optimization plugin for WordPress
https://wp-rocket.me
GNU General Public License v2.0
687 stars 215 forks source link

WP Rocket prevents WordPress redirection when multiple /// are added at the end of the URL #5868

Closed alfonso100 closed 2 months ago

alfonso100 commented 1 year ago

Before submitting an issue please check that you’ve completed the following steps: Yes - Made sure you’re on the latest version Yes - Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug When WP Rocket is enabled, if you add multiple /// at the end of any URL, prevents WordPress from redirecting to the correct URL. If you disable WP Rocket, you are redirected

Adding (.*)// using the filter rocket_cache_reject_uri (via helper plugin) solves the issue allowing the redirect.

To Reproduce Steps to reproduce the behavior:

  1. Go to any site using WP Rocket, http://example.com/////
  2. You are not redirected to http://example.com
  3. Disable WP Rocket and visit http://example.com/////
  4. You are redirected to http://example.com

Expected behavior We should not stop that redirection, but it has to be done using a regex that takes query strings into consideration

Additional context Ticket: https://secure.helpscout.net/conversation/2205173476/412303?folderId=377611 Slack Thread: https://wp-media.slack.com/archives/C43T1AYMQ/p1681320714992399

Backlog Grooming (for WP Media dev team use only)

webtrainingwheels commented 1 year ago

Update: This solution works for the inner pages of the site, but not for the homepage.

DahmaniAdame commented 1 year ago

Putting this before our htaccess directives fixes the issue on Apache-enabled servers:

<IfModule mod_rewrite.c>
RewriteEngine On
RewriteCond %{THE_REQUEST} //
RewriteRule ^(.*)$ /$1 [R=301,L]
</IfModule>
CrochetFeve0251 commented 1 year ago

Reproduce the problem

I succeeded to reproduce the issue with the information given.

Identify the root cause

The root code is that we are serving the file using htaccess so the WordPress redirection can't be executed.

Scope a solution

A solution would be to insert the redirection @DahmaniAdame gaved at this position https://github.com/wp-media/wp-rocket/blob/82b29629790c2cc222917e3dd32bc2a126015837/inc/functions/htaccess.php#L247. It will redirect the user when he has more than one slash at the end of the URL.

We will also need to reload the .htaccess on update.

Estimate the effort

Effort S

piotrbak commented 1 year ago

@CrochetFeve0251 It's happening on all server types, not only the htaccess ones.

mostafa-hisham commented 1 year ago

Reproduce the problem

I succeeded to reproduce the issue with the information given.

Identify the root cause

I think we have a problem with this function https://github.com/wp-media/wp-rocket/blob/57649bd9aadaba6149cd475f2eef0331c985afc6/inc/classes/Buffer/class-cache.php#L761

this will always prevent the redirect if the last char in the permalink and the request_uri are the same

Scope a solution

I think we need to check if the request uri ends with multi slashes / first and if it does allow the wp redirect we cannot use get_raw_request_uri and get_request_uri_base because they trim extra slashes https://github.com/wp-media/wp-rocket/blob/5b9b3f30b3aefa615d2d005f5b5b94ffc3424965/inc/classes/Buffer/class-tests.php#L982-L1009

we can create a new function

    public function get_raw_base_request_uri() {
        $raw_request_uri =  $this->config->get_server_input( 'REQUEST_URI' );
        if ( ! $raw_request_uri ) {
            return '';
        }

        $raw_request_uri = explode( '?', $raw_request_uri );

        return reset( $raw_request_uri );
    }

and use regex to check if raw_base_request_uri ends with multi-slashes and returns true at the beginning of the maybe_allow_wp_redirect function

        $re = '#/{2,}$#m';
        if (preg_match($re, $x)){
            return true;
        }

Estimate the effort

Effort S

engahmeds3ed commented 1 year ago

Good catch @mostafa-hisham

The issue happens only with sites that have trailingslash permalink because of this condition

https://github.com/wp-media/wp-rocket/blob/8682be1010d2bf50e851698bb5572a9126496459/inc/classes/Buffer/class-cache.php#L800

this returns false because both last characters are / so this will continue to find the cache file in filesystem and will not let WP loads to do the redirection.

So here:

https://github.com/wp-media/wp-rocket/blob/8682be1010d2bf50e851698bb5572a9126496459/inc/classes/Buffer/class-cache.php#L799

we need to add else block (to handle the case when the permalink has trailingslash only because the other case is already handled as mentioned above) to return true as Mostafa mentioned above using the regex.

PS: This is very risky part to touch as it may affect the whole caching system so we need to test it thoroughly and carefully.

piotrbak commented 2 months ago

This is not something that can be implemented soon. We might reopen this in the future.