Setting destination on 403 or 404 response can result in redirect loop

Problem/Motivation

Steps to reproduce the bug

• basic page request (without a destination argument) that results in 403 or 404 response
• default error page whose execution results in a call to drupal_goto()

Expected behavior: redirect to default error page
Actual behavior: redirect loop

This results in browser displaying something like the following:

The page isn't redirecting properly
Browser has detected that the server is redirecting the request for this address in a way that will never complete.

The code trail is as follows (for a 403 response):

menu_execute_active_handler()
- $router_item['access'] = FALSE;
- $page_callback_result = MENU_ACCESS_DENIED;
drupal_deliver_page()
drupal_deliver_html_page()
- case MENU_ACCESS_DENIED:
- $_GET['destination'] = $_GET['q'];
- above saves the invalid path to a key used later in drupal_goto()
- $path = drupal_get_normal_path(variable_get('site_403', ''));
- this $path is the default error page
- $return = menu_execute_active_handler($path, FALSE);
menu_execute_active_handler()
- drupal_alter('menu_site_status', $page_callback_result, $read_only_path);
- for the site that motivated this issue the error path is 'user/login'
user_menu_site_status_alter()
- because the user was logged in
- drupal_goto('user');
drupal_goto()
- if (isset($_GET['destination']) && !url_is_external($_GET['destination']))
- $destination = drupal_parse_url($_GET['destination']);
- $path = $destination['path'];
- and so we redirect to a known invalid path

Because drupal_deliver_html_page() set $_GET['destination'] to a page known to result in a 403 or 404 response, this causes a redirect loop. Why exactly would we want this code? Is there a use case for such? The comment on this statement reads:

Keep old path for reference, and to allow forms to redirect to it.

Why would a form want to redirect to a page known to result in a dead end? If we would like the path for reference, then put it in a different key in $_GET, not one that gets picked up by later code to hijack the whole error page handling process.

And lest someone question why the error path on a site is 'user/login' please do not let this interfere with understanding the bug in general sense (see "Steps to reproduce the bug" above).

Proposed resolution

Two choices:

• Remove the conditional block in drupal_deliver_html_page() that sets $_GET['destination'] to the known bad path.
• Set a different key in the $_GET array if the invalid path is truly needed for reference and let a contrib or custom module deal with it if desired in the call to drupal_alter('drupal_goto', $path);.

Remaining tasks

Review, a test could be written, but not clear it is necessary.

User interface changes

None.

API changes

None.