Setting 403 or 404 handler to a page that redirects leads to endless loop

Also, Search 404.
Not closing because /search usually goes to /search/node

This happens because the page not found handler (drupal_deliver_html_page > MENU_NOT_FOUND) adds the original path to $_GET['destination']. The search module needs a module specified (search/x), so it redirects to search/node by default (see the search_view function). That redirect is accomplished via drupal_goto, which of course redirects to $_GET['destination'], rather than the intended search/node.

Here's a patch that removes $_GET['destination'] before the redirect in search_view. I think this is a reasonable approach, since we shouldn't need to worry about form submissions or other uses of 'destination'.

At DrupalCon Chicago, this is my first patch that I am testing. The patch applies cleanly and works fine with the latest D7. I have re-rolled this patch adding a simpletest to check for this use case.

The patch says at the end:

\ No newline at end of file

Aside from this very very minor problem, I think this is excellent work and should be immediately committed to d7 and d8. Can you please reroll with a newline at the end of the search.test file? Thanks!

I just tested and this is still a problem in Drupal 8.

We need this patch to be rerolled so that it will work in the current Drupal 8, and the test will need to be put into a different file. Then we can consider backporting to Drupal 7.

I just tested this again in D8 with the patch to #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords applied.

Steps:
a) Go to admin/config/system/site-information
b) In the Error Pages section, enter "search" for both 403 and 404 pages.
c) Go to example.com/foo (a 404 page). The site endlessly tries to redirect, but it fails as described here.

So this is still a problem.

The solution is not so obvious. The redirect from /search to /search/(page URL) is set up in \Drupal\search\Routing\SearchPageRoutes::routes(), and then handled in \Drupal\search\Controller\SearchController::redirectSearchPage() now, with the default search page entity being passed in as a parameter. The D7 patch here will not work as it is, and I personally do not know how to make it work in D8.

However, as suggested in #1, if you make the 403/404 handler page not be /search but instead something like /search/node, you will not have this problem.

As per comment #5 reroll patch.

Thanks! Note that the patch in #8 is for Drupal 7. For Drupal 8, we should wait until #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords is done as it is overhauling all the search redirection stuff (but as noted in #7, even with that patch this is still a bug).

Discussed this in IRC today... This is not really a problem with the search module per se. It is a problem with entering anything as your 403/404 page that is actually a redirect to another page.

The path "search" redirects to search/node (or whatever your default search page is), so it is a special case of this problem.

So I'm moving this into the Routing component to see if the maintainers of that component think it's a bug we should address or a "won't fix" issue, or maybe that we should tell people to be careful not to put redirecting pages in as their 403/404 page handlers?

Interestingly I am also seeing endless redirects when trying to issue a redirect on a URL that happens to be a 403 or 404 page. I get the redirect meta tag as the body, and 403 or 404 as the status code. I may be doing it wrong (TM) - if I figure it out I'll update this comment :) UPDATE: My working code for redirecting from a possibly 403/404 page is the secureRedirect() method here: http://cgit.drupalcode.org/securelogin/tree/src/SecureLoginManager.php?h...

So I'm moving this into the Routing component to see if the maintainers of that component think it's a bug we should address or a "won't fix" issue, or maybe that we should tell people to be careful not to put redirecting pages in as their 403/404 page handlers?

So do I understand it correctly is that the destination itself points to the same path as the 404 page? In that case we could prevent it and unset it.
@jhodgdon Do you have a better insight in what is going on here?

Not quite.

The original bug reported here is that if you configure the 403 or 404 handler page to be path "search", that doesn't work -- you end up in an endless redirect loop. So for instance you try URL "example.com/foobar", which doesn't exist. The 404 handler says to use path "search", but in the process of trying to redirect to "search", the route handler says "Oh, actually redirect to search/node". The redirect doesn't work due to some technicalities in how redirects are done. See comment #3 above for more explanation of this (although the patch is very outdated compared to the current code). It looks like #11 is another case of this same problem not related to configuration of the 403/404 handler, by the way.

So for this issue the question is whether:
a) We should fix the bug and make it possible to enter a path that itself redirects for the 403/404 handler.
or
b) We should tell people not to enter paths that themselves redirect to other paths in the 403/404 handler configuration.

And incidentally I just tested this and it is still a problem in a D8 build from last week at least; I would assume nothing has changed since then.

This code is actually in system.module

I just tested this in 8.0.x (dev branch). It is still not working right, but the behavior is different now in 8 and 7. Updated summary. Still a bug.

There is a very common need that we should check if user is logged in when showing 403 page.
But if you redirect the anonymous user to user/login page, it will end up with infinite loop.

Strange behaviour.

Test with 8.5.0.
I set the Default 404 (not found) page like the follow.
/search

Access a nonexistent page it is redirects the home page.

I ran into this issue today on a site that had the 403 page set as /user.

As an anonymous user, going to a route without access redirects to /user/login
As an authenticated user, going to a route without access results in an endless redirect.

The test I added will probably timeout the test bot due to the endless redirect.

I opted to create a separate test class because AccessDeniedTest seemed kind of big, but it could move the test into AccessDeniedTest.

And there we are.

BrowserTestBase really doesn't handle infinite redirects very well.

Testing Drupal\Tests\system\Functional\System\AccessDeniedAsAuthenticatedUserTest
E 1 / 1 (100%)unserialize(): Error at offset 7661404121 of 7661404160 bytes

I tried to follow the steps to reproduce against 11.x but getting slightly different results. For a start, going to /admin as an anonymous user just shows an Access Denied message. That's fine, but doing the same after logging in as an authenticated user results in a redirect loop. Also not seeing the same thing for /foobar, I'm redirected to the front page instead. Can anyone confirm the same?

Re #33:

BrowserTestBase really doesn't handle infinite redirects very well

I think instead we can use the guzzle http client to make requests to the site. There is an option not to follow redirects, so we can simply inspect the headers. There is also a max redirects option, so we could use that too.