drupal_goto() should allow absolute destinations that are within the same site

Try this it may work for u all,

I have used some conditions to get my desired output the redirect string also may contain ? in the string for e.g. index.php?test=test

function drupal_goto($path = '', $query = NULL, $fragment = NULL, $http_response_code = 302) {

$destination = FALSE;
$append='';
$url_append='';
$inc='';
if (isset($_REQUEST['destination'])) {
$destination = $_REQUEST['destination'];
// THIS IS THE CONDITION THAT I HAVE APPENDED
// STARTS HERE
if(isset($_GET['redirect']))
{
$destination = $_REQUEST['redirect'];
if(isset($_REQUEST['redr']) && $_REQUEST['redr']==1)
{
// I HAVE PASSED THE ENCODED STRING
$destination = base64_decode($_REQUEST['dest']); //DECODED HERE
if(strpos($destination,'?')===TRUE)
{
$inc=1;
$exp=explode('?',$destination);
$url_append=$exp[1];

}
}

$append="|| !preg_match('![/&#]!', substr($destination, 0, $colonpos))";
}
// ENDS HERE

}
else if (isset($_REQUEST['edit']['destination'])) {
$destination = $_REQUEST['edit']['destination'];
}
if ($destination) {
// Do not redirect to an absolute URL originating from user input.
$colonpos = strpos($destination, ':');

// The $append is appended the following condition
$absolute = ($colonpos !== FALSE && (!preg_match('![/?#]!', substr($destination, 0, $colonpos)) .$append));
if (!$absolute) {
extract(parse_url(urldecode($destination)));
}
}

$url = url($path, array('query' => $query, 'fragment' => $fragment, 'absolute' => TRUE));
// Remove newlines from the URL to avoid header injection attacks.
$url = str_replace(array("\n", "\r"), '', $url);

// Allow modules to react to the end of the page request before redirecting.
// We do not want this while running update.php.
if (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update') {
module_invoke_all('exit', $url);
}

// Even though session_write_close() is registered as a shutdown function, we
// need all session data written to the database before redirecting.
session_write_close();
global $base_url;
// Last and finally redirect
if($inc==1)
{
header('Location: '. $url.$url_append, TRUE, $http_response_code);
}
else

{
header('Location: '. $url, TRUE, $http_response_code);
}

// The "Location" header sends a redirect status code to the HTTP daemon. In
// some cases this can be wrong, so we make sure none of the code below the
// drupal_goto() call gets executed upon redirection.
exit();
}

Thanks.

Since the last change, drupal_goto() has significantly changed.
I have used the following code to test redirect to the same domain:

function test_menu() {
  $items['test_form'] = array(
    'page callback' => 'drupal_get_form',
    'page arguments' => array('test_form1'),
    'access arguments' => array('access content'),
  );
  return $items;
}

function test_form1(&$form_state) {
  $form['text_el'] = array(
    '#type' => 'textfield',
    '#title' => 'Text field',
  );
  $form['button'] = array(
    '#type' => 'submit',
    '#value' => 'Submit',
  );
  return $form;
}

function test_form1_submit($form, &$form_state) {
  drupal_set_message('Button callback called');
  drupal_goto("http://{$_SERVER['HTTP_HOST']}/help");
}

to see if drupal_goto accepts absolute URLs to the current domain.
Same code worked correctly (redirect to help) for 8.x, 7.x and 6.x

Then can you please describe what's wrong with it? The code in my previous comment works as expected

1. This can't be clarified as bug report, probably feature request fits better
2. If at all, issue should be implemented in 8.x first, and then backported
3. I personally have deep doubts this would pass security team

I think this proposal is fine. Open redirects is about getting redirected to somewhere else on the internet. If the site is the exact same domain I think it's fine. We probably shouldn't allow redirects to subdomains/parent domains.

I don't know that this is a bug or a feature, but my sense is that it should be fixed in 8 first, then 7, then 6 as is the normal flow.

I think patch should also handle situation with 2 sites with different base_paths, and one redirects to another.
This should be treated as external host (as with subdomains), right?

@fizk - it would be great if you could add some tests to help confirm that the protection against redirection to external domains is working properly.

#14: I meant redirect from example.com/subfolder1/node/1 to example.com/subfolder2/node/2

#18: Right, probably redirect to subfolder2 should be treated as external domain (just IMO, not 100% in that)

@fizk - actually, I think #18 is a problem that would need protection against (which is what I think valthebald meant in #13). We need to prevent redirection from one Drupal site to another whether that site is on the same host or not.

x-post, but status change makes sense :)

Yep. Must certainly be checked against base_path.
Furthermore: Why would this be a bug regarding core functionality rather than a new core feature?
And if it's indeed a feature: what are the use cases for adding this?
Finally: how do we cope with language subdomains that need to work transparently just as language prefixes?

Patch looks fine for me, and covered by new test, which are ok for the test bot.
Marking RTBC

Thanks @fizk and @valthebald!

I found a few things to clean up, one of which is missing minimum documentation (see http://drupal.org/core-gates#documentation-block-requirements), so marking needs work.

1. +++ b/core/includes/common.incundefined
   @@ -683,8 +683,9 @@ function drupal_encode_path($path) {
   +  // We do not allow absolute URLs to be passed via $_GET, as this can be an attack vector, with the following exception:
   +  // - absolute URLs that point to this site (i.e. same base URL and base path) are allowed
   
   +++ b/core/modules/system/lib/Drupal/system/Tests/Common/GotoTest.phpundefined
   @@ -47,12 +47,26 @@ function testDrupalGoto() {
   +    // Test that drupal_goto() fails to respect ?destination=xxx with an absolute URL
   +    // that does not point to this Drupal installation.
   

   These comments should be rewrapped so that the lines do not go over 80 characters. Reference: http://drupal.org/node/1354#general

2. +++ b/core/includes/common.incundefined
   @@ -707,6 +708,22 @@ function drupal_goto($path = '', array $options = array(), $http_response_code =
   + * Helper function for determining if an external URL points to this Drupal installation.
   + *
   + * @return
   + *   TRUE if the URL has the same domain and base path
   

   Few things with this docblock:

   • It needs a @param documenting the URL, and datatypes for the @param and @return. Reference: http://drupal.org/core-gates#documentation-block-requirements
   • The one-line summary should be fewer than 80 characters and start with a verb. I'd suggest:
     

     Determines if an external URL points to this Drupal installation.

     Reference: http://drupal.org/node/1354#functions

   • The parameter description should end in a period.

3. +++ b/core/includes/common.incundefined
   @@ -707,6 +708,22 @@ function drupal_goto($path = '', array $options = array(), $http_response_code =
   +    // When comparing base path's, we need a trailing slash to make sure a
   

   "paths" should not have an apostrophe.

When uploading a new patch, please include an interdiff, and also a test-only patch (which we expect to fail) to expose the test's coverage of the fix.

It's usually not a good idea for patch author to mark issue RTBC... but I was going to do this anyway :)

Catch pinged me on this issue. drupal_goto() is slated for removal and must be removed before we ship: #1668866: Replace drupal_goto() with RedirectResponse.

That issue isn't commitable yet, though, so I defer to catch on what order he wants to do things in. But this code will get rewritten before we finish.

OK then, this seems reasonable so I've gone ahead and committed/pushed to 8.x

Confirm that patch from #39 works the same for D7, as #31 for D8.
Also, test addition fails without patch, and passes with patch

@fizk, welcome to the club!

@catch is the Drupal 8 maintainer, not Drupal 7 :)

I was going to commit this (as one of the last patches before the Drupal 7.17 release) but I think it could use a little more work. The URL matching seems a little iffy... for example, won't a URL like http://drupal.dev/node?destination=http://drupal.dev result in a PHP notice due to an undefined $url_parts['path']?

Also, it could use another review or two for code style issues. Some I noticed quickly:

1. +  // - absolute URLs that point to this site (i.e. same base URL and
   +  //   base path) are allowed
   

   This should be a complete sentence with a capital letter and a period.

   (And by the way, why wasn't this comment in the Drupal 7 version of the patch?)

2. +function _external_url_is_local($url) {
   +    $url_parts = parse_url($url);
   

   This function has the wrong indentation throughout (should be two spaces only).

3. +    $destination = 'http://pagedoesnotexist';
   

   Maybe just use http://example.com?

4. +  $items['common-test/drupal_goto/alt'] = array(
   +    'title' => 'Drupal Goto',
   +    'page callback' => 'common_test_drupal_goto_land_alt',
   

   I'm not sure what "alt" refers to here, and normally we wouldn't use abbreviated words like this (although I see the surrounding code is full of them already, so whatever :)

   In any case, do we really need this new menu item at all; couldn't the existing common-test/drupal_goto path be used in the tests instead?

The patch looks quite good otherwise, and I might have committed it with the code style issues, but especially given the PHP notices/functionality issue I mentioned above, I think it would be good to have one more round of work before getting this into Drupal 7...

Thanks!

Slight title tweak since it's not just about domain, but actually the whole site (including subdomains or subfolders).

Why does it refer to 8.x? I think patch was already committed to 8.x branch, so it should be backported now

Looks fine to me except for one code style issue:

+  if (!isset($url_parts['path'])) {
+    return ($url_parts['host'] == $base_host);
+  } else {

else should be on a new line.

Well of course the interdiff failed, but the real patch worked. :-) (First patch in #54.)

OK. Committed/pushed to 8.x.

This looks backportable.

Well, you marked your own patch RTBC which is generally discouraged (especially for a non-trivial patch that has security implications) :)

I know there are other people interested in this issue so I'm surprised that in the last couple months no one else has reviewed it. In general, I really don't like to be the only person to review a patch before committing it. However, I know this one has gone through several rounds of reviews already so maybe it's OK (looks fine on a quick glance at any rate). I'll try to get a final review on this done and hopefully commit it soon.

Thanks!

Hm, is it a problem that with this patch, a URL like http://example.com/drupal/node?destination=http://example.com will successfully redirect you from the site at http://example.com/drupal to the site at http://example.com?

That's a little different than the examples I remember being discussed above (e.g. two different subfolders on the same domain) and seems less likely to be a problem than those, but I'm not 100% sure it's OK.

Also (comparitively minor): It looks like there will still be PHP notices if $url_parts['host'] is undefined, although I guess unlike $url_parts['path'] you'd need to construct a pathological example to run into that one.

When I tried it out locally it seemed to (specifically with http://localhost/drupal/node?destination=http://localhost).

I think it's because of this part:

+  if (!isset($url_parts['path'])) {
+    return ($url_parts['host'] == $base_host);
+  }

When the URL being redirected to in 'destination' doesn't have a path at all, it only cares about comparing the hosts.

Raising priority to major per #65 and #67.

Also switching back to D8, so the testbot can successfully apply the followup patch in #71. (Unfortunately the testbot isn't smart enough to figure out if a patch is D7 or D8 even if accordingly named.)

Documented behaviour of drupal_goto was changed, so we need a change notification.

#71: 736172-drupal-goto-same-domain-3-d8.patch queued for re-testing.

Change notices are critical.

Sorry, I was too fast. The followup is not yet committed.

Moved to routing system per #2050763-16: Refine "base system" component (notes on refactoring of "base system" category here: https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AusehVccVSq2dF...).

Alright, so we'll write the change notice once the followup is in. Thanks!

Clarify the description.

71: 736172-drupal-goto-same-domain-3-d8.patch queued for re-testing.

@fizk, the patch was never marked "Reviewed & tested by the community", so that is why it was never committed. Core committers look at every patch that is marked RTBC within 1-2 weeks, and much sooner than that typically for majors and criticals. What was missed was someone to review and RTBC the followup.

One thing that makes it harder to review the issue here is that there was already one commit and there is a different followup patch, so it is confusing. An update to the issue summary to clearly explain the situation would help as well. And a possible way to find reviewers is to reach out on #drupal-contribute in IRC once there's an updated patch and maybe an issue summary update.

Drupal_goto() has been removed, so the current patch is no longer relevant.

I tried the latest D7 patch and it still seems a tiny bit fragile - for a site at http://example.com/drupal, http://example.com/drupal/node/1?destination=http://example.com/drupal/ works but http://example.com/drupal/node/1?destination=http://example.com/drupal doesn't?

I also noticed that this allows an attacker to force a redirect from HTTPS to HTTP. I am not sure if that's a problem or not...

It also looks like the patch might need a reroll - retesting now.

I think this was raised to Major priority for the D8 followup, which is no longer relevant. Restoring the original priority.

OK, needs work - at the very least because the patch doesn't apply, but also see above.

Sorry. I feel bad about how long this issue is taking to get in, but it seems like there aren't many people interested in reviewing this particular patch.

Yeah, I think you're right - if a site requires HTTPS for security it really needs to be forcing it, so there's no issue there.

OK, here's a reroll (the conflict in the test file was trivial). I have also added some code to fix the trailing slash case I mentioned in #91.

Unfortunately I discovered another problem while doing that. There is still a way to redirect from http://example.com/drupal to http://example.com. You can do it with a URL like this:

http://example.com/drupal/node/1?destination=http://example.com/drupal/../

That is probably possible to fix, but worse, what about the other way around? By definition this patch allows you to redirect from http://example.com to http://example.com/drupal because it does not know that http://example.com/drupal is a separate site (rather than a page on the parent site)... So if the goal really is as stated in #21, "We need to prevent redirection from one Drupal site to another whether that site is on the same host or not" then unfortunately I'm not sure this is even possible?

Guess that needs some discussion. I'm setting needs review now for the testbot.

You can redirect from http://example.com to http://example.com/drupal without this patch using http://example.com/user?destination=drupal , so we're not introducing a new security issue.

That is a very good point :) D'oh!