parse_url in drupal_goto breaks destination url

I just ran into the same issue with logintoboggan for D6 - it's a core bug IMO; drupal_goto should not use parse_url on a relative URL if php.net says it doesn't work.

This would have to be fixed in D7 first and backported, I think. I will try to provide a patch soon if nobody else does.

Here's the beginnings of one idea. Needs a test.

Does this only ever need to be run in drupal_goto(), if there's other use cases, should we make drupal_parse_url() or similar?

Hmm, good point. A quick grep of core reveals 8 more references to parse_url(). Most of them are probably legitimate (i.e. expect a full URL), but the one in menu.admin.inc looks suspicious -- and sure enough, try to add a menu link to node/add/page?test=foo:/:bar and you get an error "The path '/add/page' is either invalid or you do not have access to it."

Hmm, one unintended consequence of this is that drupal_goto will now happily redirect to an absolute URL in the destination. Not sure if that's a good thing or a bad thing.

@John Morahan, I think its a good thing and I haven't heard any reason so far it'd be a problem. This dev list thread is relevant: http://lists.drupal.org/pipermail/development/2008-May/029772.html

#107830: Support external sites in destination request in drupal_goto was marked a duplicate of this.

+1 I would like to see this!

suspected test bot failure

I have also just encountered this bug in Drupal 6 using the Menu module. Unable to create a link to a URL with a colon in it. +1 from me!

preg_match('/^([^?#]*)(?:\?([^#]*))?(?:#(.*))?$/', $url, $matches);
$parts['path'] = $matches[1];

You can't execute a regular expression and then use the match groups without first checking if the expression matched. If the expression didn't match, you will see notices about $matches[1] not being defined.

What you need is

if (preg_match('/^([^?#]*)(?:\?([^#]*))?(?:#(.*))?$/', $url, $matches)) {
  $parts['path'] = $matches[1];
  // ...
}
else {
  // error-handling - maybe return null or something.
}

How would it not match?

/^([^?#]*)(?:\?([^#]*))?(?:#(.*))?$/

Oh, whoops. I misread a part of it (regexes are my personal nemesis). Yes, it seems that this pattern should match any string, so a check probably isn't required.

Hmm...
http://googlewebmastercentral.blogspot.com/2009/01/open-redirect-urls-is...
would we need to worry about this?

See #553262: Make getAbsoluteURL() standard compliant that suggests a new function, drupal_parse_url(), that supports parsing relative URLs.

#203571: Custom logout redirects to external sites depends on this issue. Seems like we could solve the open redirect issue by adding an argument $external = FALSE to drupal_goto(), which was only overridden to permit external redirects in suitable contexts.

I believe this has been fixed in #578520: Make $query in url() only accept an array.

I'm not sure how this was fixed by #578520: Make $query in url() only accept an array.

I'm trying to use login_destination to redirect from an SSL login page to a NON-SSL front page. Using "http://www.example.com" isn't working because of this bug.

patches above didn't work for me, at least not with drupal 6.22 so made this one that does based on the one in comment 2

Yes, this still seems to be an issue in Drupal 7 as it is noted in drupal_parse_url() that 'relative URL "foo/bar:1" isn't properly parsed'.

#6: drupal_goto-parse_url.patch queued for re-testing.

Just in case anyone still needs to fix this issue when using Faceted Search in Drupal 6, just appending a dot to the destination url on login where there is just one taxonomy term on the url does it. So you could do this (a bit of a hack really):

<?php

/**
 * Implementation of hook_user
 *
 */
function mymodule_user($op, &$edit, &$account, $category = NULL) {
  if ($op == 'login') {
    if (preg_match('@^.+/taxonomy:[0-9]+$@', $_REQUEST['destination'])) {
      $_REQUEST['destination'] = $_REQUEST['destination'] . '.';
    }
  }
}



?>