Redirection fails when redirect url has query string

I am changing the status to "needs work" since I am stuck in a redirect loop when one of the query variables key is 'destination'.

I am thinking that drupal_goto() is picking up the value of this variable and using it.

It's possible that the problem is originating from one of my custom module but I wanted to bring up this issue before this patch get merged.

Hi dashohoxha,

Yes, I have tested it and it's working well on my end but I do not agree that this patch does not change the functionality of the code. It does not seems to break any existing behaviors but it improves support for query string in the URI.

For example,
I am fetching the access token on a page where the url is node/1/update_favorites?token=23423rdfsgdfg

The registered path in drupal is:
node/%node/update_favorites

Everything works fine with my patch with the exception of when I used destination in the url.
For example:

1. node/1/update_favorites?token=23423rdfsgdfg works fine
2. node/1/update_favorites?token=23423rdfsgdfg&destination=node/1 does not

Without my patch, I am unable to fetch an access token because the user is redirected here:
node/1/update_favorites%3Ftoken%3D23423rdfsgdfg?code=234234234&state=2342342342342342

node/1/update_favorites%3Ftoken%3D23423rdfsgdfg is not a valid path in my site so it defaults to node/1.

I think that If I would try to fetch the access token on node/1, this would work since code and state are properly set. Since I am not, it's not working.

Here's a patch that works for me when the page is using $_GET['DESTINATION']. I needed to remove the drupal_goto().

Please ignore patch #12. It does not work well when $_GET['destination'] is missing. It encodes the query string before redirecting the user.

Basically, the idea here is:

Redirect to the current page if $_GET['destination'] is equal the path from drupal_get_destination() because if they match it mean that the current page will take care of the redirection.

For example,

user is on node/1 and then clicks on a link to node/1/addfavorites?destination=node/1&token=123123

The page callback for node/1/addfavorites tries to fetch an access token but not node/1.

Once the authentication and authorization is done on the server, we are redirected by the server to the redirect uri: 'oauth2/authorized'. This page should then redirect the user in saved path (node/1/addfavorites?destination=node/1&token=123123) and then this page should redirect the user to node/1.

We can also decide not to support this. I can work around this by using another key name for destination, i.e node/1/addfavorites?dest=node/1&token=123123.

I created a new patch that removes $query['destination'] if it's an external link, as this can be an attack vector.

I am also converting path with drupal_get_normal_path() to compare $destination['path'] == $query['destination'] on the same level.

I also created a new function called goto_location(). It's very similar to drupal_goto() but it does not override a destination from $_GET.

Renamed goto_location() to gotoLocation()

@dashohoxha, thanks for your patience!

I agree that we can simplify the gotoLocation() function. I am attaching a patch that does that.

I also added some comments to setRedirect(). I think these changes are necessary because drupal_get_destination() will return the path in the $_GET['destination'] parameter if it's set.

This is why I am fetching drupal_get_query_parameters() separately.

Added a bit more comments.

Rmoved an extra white line from the previous patch. Sorry for all the noise.

@dashohoxha,

how about we keep it simple and we do not commit the `gotoLocation()` function? I wanted to include this to support urls with $_GET['destination'] but I don't think this is the main focus of this bug. We could create a new issue to support $_GET['destination'] and deal with this issue separately.

I can work around this issue by setting my redirect path in a different _GET parameter, i.e $_GET['destination2'] and passing it to the drupal_goto($_GET['destination2']) function in the page callback of my custom module.

I think that we should fix the main issue which is removing the query string from $destination['destination'] before passing it to drupal_goto() because it will get encoded.

We can achieve that with drupal_parse_url().

Hi dashohoxha,

I was under the impression that the issue was not clear to you. I will be busy for the next week or so. I will try to comeback with a test for my patch.

The issue only happens if you try to fetch an access token on a page with a query string: node/1/favorites?token=dfsfdsf.

It's important to mention that this works fine if an access token already exist in $_SESSION but if the user is redirected to the authentication/authorization server, the redirect url is incorrect since we are passing node/1/favorites?token=dfsfdsf for the $path argument of drupal_goto().

Drupal_goto() first argument is $path and the second argument is $options. We should be setting query variables in $options['query']. This is what I am trying to achieve with my patch. I am using drupal_parse_url() to parse $destination['destination'] so that I can seperate the path and the query string.

The return value of drupal_parse_url() is :
array An associative array containing:

path: The path component of $url. If $url is an external URL, this includes the scheme, authority, and path.
query: An array of query parameters from $url, if they exist.
fragment: The fragment component from $url, if it exists.

This allows us to remove the query string from $path for drupal_goto()

Right now we are passing node/1/favorites?token=dfsfdsf in the $path argument of drupal_goto(). Drupal_goto redirects the user to node%2F1%2Ffavorites%3Ftoken%3Ddfsfdsf%20?code=23423423423&state=xxxx instead of node/1/favorites?token=dfsfdsf&code=23423423423&state=xxxx

Glad we were able to work this out together :)

Now that my patch is committed, we might be able to do:

if (isset($destination['query']['destination'])) unset($destination['query']['destination']);

I will open a new issue to address the destination problem so that we can discuss different solutions! Is that ok?

Thanks for doing this! I will review it.

I also updated my e-mail settings to start following all issues for this module.