Redirect loop when serving cached pages to anonymous users

I've actually seen this too, but why would it redirect back again to the first?

Interesting. You probably want to mention this in the core issue as well: #2641118: Route normalizer: Global Redirect in core

It might not be reproducible on all environments but might be possible to write a unit test for this. Would be awesome to have that to avoid regressions on this in the future.

Hi !
I also have the same behavior happening with anonymous users on the same kind of URL:
You can see it in action here: https://eureka-game.com/reservations?utm_source=escapegamefrance&utm_medium=page&utm_campaign=escapegamefrance

Using the logger snippet at #9, I got :
Normalizing request: https://eureka-game.com/reservations?utm_campaign=escapegamefrance&utm_medium=page&utm_source=escapegamefrance as https://eureka-game.com/reservations?utm_source=escapegamefrance&utm_medium=page&utm_campaign=escapegamefrance.
Surprisingly, the arguments are the same but have been "mixed up" in the redirection.

I also don't know if normal, but the 301 redirection loops over and over while I only have one lign logged and not "an infinite" number as I would expect.

Also when logged as admin, the log is the same (with the url argument mixed up) and yet the 301 does not loop over.

Last point, #4 had no effects.

@tran_tien: did you change something in .htaccess
I am not yet 100% certain, but I have changed my htaccess to move http to https via:

# Redirect to HTTPS
RewriteCond %{HTTPS} off
RewriteCond %{HTTP:X-Forwarded-Proto} !https
RewriteRule ^(.*)$ https://%{HTTP_HOST}%{REQUEST_URI} [L,R=301]

and I had also uncommented:

# To redirect all users to access the site WITHOUT the 'www.' prefix,
# (http://www.example.com/foo will be redirected to http://example.com/foo)
# uncomment the following:
RewriteCond %{HTTP_HOST} ^www\.(.+)$ [NC]
RewriteRule ^ http%{ENV:protossl}://%1%{REQUEST_URI} [L,R=301]

When commenting back the www => http redirection, then the issue seems to disappear (needs more tests for definitive confirmation)
This might thus happens that you have also some .htaccess that could lead to a loop condition somehow.

We're also seeing this behavior, although we started noticing it following the alpha2 upgrade. We've been unable to reproduce it locally, although it's happening consistently on the production site.

We encountered a similar issue in #2761639-7: PageCache should not use $request->getUri() with redirect loops for URLs with query strings, but only when page caching was enabled.

The patch in #2761639: PageCache should not use $request->getUri() resolved the issue for us. Could that be relevant here as well?

Using 8.x-1.0-alpha4 - appending an empty query string causes a redirect loop on the homepage only. Same issue as @stella in #2761639-11: PageCache should not use $request->getUri() (not sure if this belongs here or there).

www.example.com/? redirects to www.example.com/ and loops from there.

A Drupal cache clear (drush cr all) recovers from the loop.

Closing the loop on this (sorry, too easy) - when Redirect is disabled, the homepage redirect loop does not occur.

Redirect module enabled (default config) with no active redirects - the issue occurs.

In the settings page (/admin/config/search/redirect/settings) inside the "Global Redirects" fieldset - unchecking "Enforce clean and canonical URLs" avoids the issue. So something about the cleaning process doesn't like query strings...

What's the status here? We need at least tests to cover this.

Could we just ignore query arguments completely and only look at the path? I'm not sure there is a valid use case for redirecting because of different query arguments?

We'll, we'd still include it to generate the route, but we'd only compare the path. So example.org/foo/bar?amp and example.org/foo/bar would be the same path, and we wouldn't redirect from one to the other.

As per related issue #2857169-2: Period in query string converted to underscore, $request->query->all() won't return the original query string so it can't be used to generate the redirect URL for comparing with the original URL as the basis for whether to redirect.

Attached is a patch which uses the original query string to compare the original URL and redirect URL.

Also added an extra test in GlobalRedirectTest to check that the original query string is preserved.

@tran_tien and @SchnWalter, please review when you get a chance.

Another scenario I just had is a redirect loop with just some/path?, without any actual arguments. That was cached without ? and redirected to some/path, then it looped.

Hmm. Is there any way to prevent the page cache from kicking in other than \Drupal::service('page_cache_kill_switch')->trigger(); ?

Setting do not cache type headers on RedirectResponse seems to have no effect.

Just want to register I've also encountered the redirect loop issue quite a lot recently (after the last core update) and I've finally stumbled upon this d.o. issue. As a manual workaround I managed to fix the issue either by saving the node again (e.g. when modifying the body field and have cache tags kicking in, I guess) or by rebuilding caches.

Here's another stab which preserves the query string even when there are no actual query parameters and disables the page cache. This solves the case in #31.

Also added an extra test for that case with caching enabled and moved this and the other new test further up so they run for anonymous users. Still not entirely sure this is the right approach, or if this is what the normalization is supposed to be doing, but it seems to work.

I was able to reproduce the problem of the redirect loop as follows:

1) clear the render cache table

2) curl -I 'http://mysite.dev/?'

The curl request will get changed by the Normalizer Subscriber (or rather the core functions that it calls) to 'http://mysite.dev/' and the redirect will then be stored and served to all people requesting the homepage.

I believe the patch goes in the right direction, but I think we should keep the caching.

Upgrading severity.

I've verified that the patch solves the problem. The test failures seem to be due to test for redirect_404 being run, too.

There's one issue with the patch: it no longer does all of the cleanup that is advertised in the description since it does no longer pass the query param through core. I can live with that.

Adding the patch. Only change is that it keeps the caching. It still solves the problem.

fixed patch

A collaborator has pointed out that the actual issue is that the redirect is stored with the wrong cache ID. Ie the redirect that is stored, should have the cache ID for 'http://mysite.dev/?' not 'http://mysite.dev/'

It may be possible to fix that instead.

That might be #2761639: PageCache should not use $request->getUri(), see discussion there.

@@ -175,6 +175,18 @@ class GlobalRedirectTest extends WebTestBase {
+
+    // Enable page cache.
+    $config = $this->config('system.performance');
+    $config->set('cache.page.max_age', 300);
+    $config->save();
+

this is not page cache, just the max-age header and actually not relevant here. page cache is only determined by having or not having the page_cache module enabled, which is hte case by default.

Can you provide a test-only patch as well?

Thanks for the feedback. Have updated the test to remove the page cache bit and split that into its own patch.

Without the disabling of the page cache, the test now fails.

For example, in manual testing, I see the following:

/Test-node? correctly redirects to /test-node?
Following that /Test-node then incorrectly redirects to /test-node?

However, with the patch from #2761639-2: PageCache should not use $request->getUri(), the test passes so it does look we will need that fix in core for this patch to pass.

Ok, if we end up with the reverse redirect then I'm fine with disabling caching with a @todo and follow-up issue on that core issue. Better to not have caching than have broken redirects.

1. +++ b/src/EventSubscriber/RouteNormalizerRequestSubscriber.php
   @@ -98,12 +98,15 @@ class RouteNormalizerRequestSubscriber implements EventSubscriberInterface {
   +      // Append the original query string.
   +      $query_string = $request->server->get('QUERY_STRING');
   +      if ($query_string || substr($request->getRequestUri(), -1) == '?') {
   +        $redirect_uri .= '?' . $query_string;
   +      }
   

   Here you are checking if there is a query string and append it to the uri accordingly. But the uri may already have a query string and then the new one needs is appended aswell '?'.
   The Fix404RedirectUITest is failing because of that, it already has a query string and appends a second one with '?'.
   You can cover this case with an if statement that checks if there is already a query string.

2. +++ b/src/Tests/GlobalRedirectTest.php
   @@ -175,6 +175,13 @@ class GlobalRedirectTest extends WebTestBase {
   +    $this->assertRedirect('Test-node?', 'test-node?');
   

   Here, we should assert 'test-node' instead I think (the trailing ? should dissappear after normalization

Also, you can provide two patches: 1 test only, and 1 combined (test and fix).

And you can add some test coverage on few cases in the GlobalRedirectTest:
1. Alias normalization without trailing '?'
2. Disable caching, then with trailing '?'.

+++ b/src/EventSubscriber/RouteNormalizerRequestSubscriber.php
@@ -98,12 +98,15 @@ class RouteNormalizerRequestSubscriber implements EventSubscriberInterface {
+      // Append the original query string.
+      $query_string = $request->server->get('QUERY_STRING');
+      if ($query_string || substr($request->getRequestUri(), -1) == '?') {

IMHO it would be super helpful to explain why we have to do this here ...

@dawehener, that is my attempt to construct the redirect_uri when there is a query string or a trailing slash without any query parameters, but agree it needs explanation. @Ginovski has mentioned /foo? should redirect to /foo so we no longer need to check for a trailing ? and the substr bit can be ditched. Still working on a new patch.

@Ginovski has highlighted the current approach of constructing the redirect_uri will not work for routes (like the one in Redirect 404) that declare a CSRF token.

In these cases, the CSRF token, will be on the redirect_uri generated from the route name but that same CSRF token may not be in the original_uri.

In the case of Redirect 404, the query_string also has another/different CSRF token so we can't construct the normalized url simply by adding back the query_string.

Instead we need to remove the "token=XXXX" in the redirect_uri before we append back the query string.

In my local testing this makes Fix404RedirectUITest::testIgnorePages pass, but are there other cases (perhaps AMP module?) besides a route which declares _csrf_token where we can end up with a query string in the URL when no query parameter in the options parameter is present?

What is the right approach here?
- Skip normalization if route has CRSF token, or
- Ignore/strip all query parameters from redirect_uri before adding back the query_string, or
- Use a different method than generateFromRoute (does one exist?) so the redirect_uri won't include the CSRF token?

Here's another attempt which

- strips off any query parameters added by the route before adding back the query string from the request. This makes Redirect 404 happy but might be defeating the purpose of normalization.
- redirects /foo? to /foo
- clears cache in test before testing alias normalization with query strings.

Thanks for working on this @hitfactory. As @Ginovski said, the idea is to have a test-only patch and a *combined* patch, otherwise we don't know if the test is actually passing with the fixes. Also, always upload the test-only patch first, then the issue isn't set to needs work when they fail and the combined one passes.

I tried out quite a few things, to improve when and what we cache, but as long as the core issue isn't fixed, all kinds of unpredictable things happen. So, lets keep that like that, just improved the docs a bit and remove the cache clear, that's not necessary when we don't cache those redirect anyway.

Committed.

Excellent! Are you planning on a new alpha release?

EDIT: https://www.drupal.org/project/redirect/releases/8.x-1.0-alpha5 is here.

I can't tell yet if it was this patch, but since updating to the latest redirect version about 15 minutes ago, almost every page on my site is causing a redirect loop now that was not there before. Including the Redirect module's settings page itself.

I end up with a browser URL like this:

http://[domain]/admin/config/search/redirect/settings?q=admin/config/search/redirect/settings&q=admin/config/search/redirect/settings&q=admin/config/search/redirect/settings&q=admin/config/search/redirect/settings&q=admin/config/search/redirect/settings&q=admin/config/search/redirect/settings.... (it just keeps going)

Not sure, but Drupal 8 doesn't use ?q anymore, you should update your ngninx/drush/... configuration and no longer populate that.

This seems to be some sort of Kalabox issue I'm running into that's causing the ?q= to be output on every page, it just wasn't causing an infinite redirect loop until the new version of redirect was installed today. I suppose I'll roll back redirect until the issue in Kalabox is figured out. Thanks for your assistance.

If you can figure what exactly is happening and how to prevent it then I'm happy to add a workaround. Lots of people reported ?q=something being added already in the previous release, just not in a loop. Maybe now all those will see that loop. But I can't really explain why that would happen and can't debug it myself.

I think I've discovered the issue.

Kalabox, and previously even Pantheon, were apparently using the older '?q=' style of rewriting for Drupal 8. Pantheon fixed their config presumably to resolve this issue, but Kalabox had not changed it yet and that's where I was seeing it.

So basically, if nginx is set up for the old style, like this: try_files $uri $uri/ @cleanurl; then the ?q= problem occurs, and this redirect loop seems to occur.

The fix was to update my Kalabox nginx drupal8.conf file so the line read try_files $uri $uri/ /index.php?$args; (and I submitted a PR to Kalabox).

For the record, @cleanurl in the nginx config is pointing to the following code on Pantheon/Kalabox:

  location @cleanurl {
      rewrite ^/(.*)$ /index.php?q=$1 last;
  }

Not everyone who uses the old style of rewriting has this issue. I don't know what presents this issue by showing the ?q= string in the browser in some cases, while not in others, but redirect settings could possibly be coming into play there.

Thanks bmcclure! That fixed the issue for me. Hopefully Kalabox can update this so we don't have to remember to change this for each local app instance.

No prob! And agreed, I did speak to pirog and submit a PR to Kalabox (https://github.com/kalabox/kalabox-app-pantheon/pull/196) so hopefully that will get merged in the near future.

Thanks bmcclure! I'm in the same boat -- Kalabox, used to only add a single ?q= to the URL, now adding multiple times. Only when redirect module is enabled on a clean install of 8.3.2. I'm not sure whether the trigger was an update to redirect or the update from 8.3.1. Since I'm on Pantheon it's harder to roll back, and since everything works on Pantheon it's possible to not notice until it's too late (especially if you're lazy like me and only check the homepage after updating).

Thanks @bmcclure! You solved my problem too.

Kalabox was the issue for me, too. The pull request linked to in #66 solved it. Thanks!

Where is this committed in the latest dev and beta this is not applied.

lathan: If you're referring to the fix I mentioned, it's not actually in this module, but in the nginx configuration on the web server running the site. In my case, this was in Kalabox. FWIW, I'm using Lando now instead of Kalabox and it worked out of the box.

@bmcclure
tnx for your solution on NGINX. I now have this config. The try_files $uri $uri/ /index.php?$args; didn't work in the block location for the frontpage, i had to made 2 blocks (location / and location @cleanurl.
Not sure if i can remove location @rewrite ..

location / {
# try_files $uri @rewrite; # For Drupal <= 6
try_files $uri /index.php?$query_string; # For Drupal >= 7
# try_files $uri $uri/ /index.php?$args;
}

location @rewrite {
rewrite ^/(.*)$ /index.php?q=$1;
}

location @cleanurl {
try_files $uri $uri/ /index.php?$args;
}