Problem/Motivation

Going to a URL like http://example.com/foo?destination=http://example.com/bar does not result in going to http://example.com/bar

This is purposely prevented by the code in drupal_goto() for security:

<?php
 
// We do not allow absolute URLs to be passed via $_GET, as this can be an attack vector.
 
if (isset($_GET['destination']) && !url_is_external($_GET['destination'])) {
?>

Proposed resolution

I'm proposing that we allow absolute URLs in $_GET['destination'] if and only if the domain is equal to our website's domain.

Remaining tasks

Commit #68 to 7.x.

User interface changes

None.

API changes

Going to a URL like http://example.com/foo?destination=http://example.com/bar would result in going to http://example.com/bar

Original report by fizk

Going to a URL like http://example.com/foo?destination=http://example.com/bar does not result in going to http://example.com/bar

This is purposely prevented by the code in drupal_goto() for security:

<?php
 
// We do not allow absolute URLs to be passed via $_GET, as this can be an attack vector.
 
if (isset($_GET['destination']) && !url_is_external($_GET['destination'])) {
?>

I'm proposing that we allow absolute URLs in $_GET['destination'] if and only if the domain is equal to our website's domain.

Files: 
CommentFileSizeAuthor
#97 interdiff.txt665 bytesDavid_Rothstein
#97 736172-drupal-goto-same-domain-97.patch5.92 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 41,035 pass(es).
[ View ]
#68 736172-drupal-goto-same-domain-3-d7.patch5.73 KBfizk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 736172-drupal-goto-same-domain-3-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#68 736172-drupal-goto-same-domain-3-d7-tests-only.patch3.7 KBfizk
FAILED: [[SimpleTest]]: [MySQL] 40,216 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#55 736172-drupal-goto-same-domain-8-interdiff.patch558 bytesfizk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 736172-drupal-goto-same-domain-8-interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#54 736172-drupal-goto-same-domain-8.patch4.93 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 50,658 pass(es).
[ View ]
#54 736172-drupal-goto-same-domain-8-test-only.patch3.24 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 50,547 pass(es).
[ View ]
#46 736172-drupal-goto-same-domain-7.patch4.93 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 47,580 pass(es).
[ View ]
#46 736172-drupal-goto-same-domain-7-test-only.patch3.24 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 47,598 pass(es).
[ View ]
#39 736172-drupal-goto-same-domain-1-d7.patch5.39 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 39,551 pass(es).
[ View ]
#39 736172-drupal-goto-same-domain-1-test-only-d7.patch3.65 KBfizk
FAILED: [[SimpleTest]]: [MySQL] 39,550 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#31 736172-drupal-goto-same-domain-6.patch5.76 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 46,207 pass(es).
[ View ]
#31 736172-drupal-goto-same-domain-6-interdiff.patch2.67 KBfizk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 736172-drupal-goto-same-domain-6-interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#31 736172-drupal-goto-same-domain-6-test-only.patch3.81 KBfizk
FAILED: [[SimpleTest]]: [MySQL] 46,193 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#28 736172-drupal-goto-same-domain-5.patch5.67 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 42,815 pass(es).
[ View ]
#27 736172-drupal-goto-same-domain-4.patch1.88 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 42,693 pass(es).
[ View ]
#26 736172-drupal-goto-same-domain-3.patch1.86 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 42,598 pass(es).
[ View ]
#24 736172-drupal-goto-same-domain-2.patch1.88 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 42,606 pass(es).
[ View ]
#11 736172-drupal-goto-same-domain.patch1.07 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 42,576 pass(es).
[ View ]

Comments

fizk’s picture

Title:drupal_goto() should allow absolute URLs that are within the same domain» drupal_goto() should allow absolute destinations that are within the same domain
fizk’s picture

This issue could also be titled, "?destination" query can't redirect forms to a secure path

harish.salian05’s picture

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.

valthebald’s picture

Status:Active» Closed (works as designed)

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

<?php
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

fizk’s picture

Status:Closed (works as designed)» Active

The drupal_goto() function has not changed at all in 7.x and 6.x.

valthebald’s picture

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

fizk’s picture

Going to a URL like http://example.com/foo?destination=http://example.com/bar does not result in going to http://example.com/bar

This is purposely prevented by the code in drupal_goto() for security:

<?php
 
// We do not allow absolute URLs to be passed via $_GET, as this can be an attack vector.
 
if (isset($_GET['destination']) && !url_is_external($_GET['destination'])) {
?>

I'm proposing that we allow absolute URLs in $_GET['destination'] if and only if the domain is equal to our website's domain.

valthebald’s picture

  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
greggles’s picture

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.

fizk’s picture

Version:6.16» 8.x-dev

Marking 8.x-dev as per #8 and #9.

fizk’s picture

StatusFileSize
new1.07 KB
PASSED: [[SimpleTest]]: [MySQL] 42,576 pass(es).
[ View ]

Patch for 8.x-dev.

fizk’s picture

Status:Active» Needs review
valthebald’s picture

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’s picture

valthebald, if you mean http://my-domain.com/node/1?destination=http://other-domain.com, then this would be an unsafe open redirect as greggles mentioned and shouldn't be allowed.

greggles’s picture

@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.

fizk’s picture

@greggles, no problem, was hoping to get your input on the patch before I started on the tests.

valthebald’s picture

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

fizk’s picture

Yes, with this patch, http://example.com/subfolder1/node/1?destination=http://example.com/subfolder2/node/2 will work.

I see what you mean now. Perhaps we should not only match the base_url, but also the base_path.

valthebald’s picture

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

fizk’s picture

I think you're right, I'll update the patch to match the base_url and base_path.

greggles’s picture

Status:Needs review» Needs work

@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.

greggles’s picture

x-post, but status change makes sense :)

Pancho’s picture

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?

fizk’s picture

Status:Needs work» Needs review
StatusFileSize
new1.88 KB
PASSED: [[SimpleTest]]: [MySQL] 42,606 pass(es).
[ View ]

We now check the domain and base_path.

fizk’s picture

Pancho,

Why would this be a bug regarding core functionality rather than a new core feature?

IMHO it's a bug because we were trying to do something but didn't do it quite right the first time - i.e. there is no security benefit to preventing absolute URLs that actually point to your Drupal site.

How do we cope with language subdomains that need to work transparently just as language prefixes?

I haven't worked with Drupal in non-English languages. Could you give me some example URLs?

fizk’s picture

StatusFileSize
new1.86 KB
PASSED: [[SimpleTest]]: [MySQL] 42,598 pass(es).
[ View ]

Fix a mistake - helper function _external_url_is_local() didn't use its argument.

fizk’s picture

StatusFileSize
new1.88 KB
PASSED: [[SimpleTest]]: [MySQL] 42,693 pass(es).
[ View ]

Slight tweak - only grab the host part when parsing base_url.

I'm going to add some simple tests next.

fizk’s picture

StatusFileSize
new5.67 KB
PASSED: [[SimpleTest]]: [MySQL] 42,815 pass(es).
[ View ]

Patch + simple tests.

valthebald’s picture

Status:Needs review» Reviewed & tested by the community

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

xjm’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Novice

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:

  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.

fizk’s picture

Status:Needs work» Needs review
StatusFileSize
new3.81 KB
FAILED: [[SimpleTest]]: [MySQL] 46,193 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 736172-drupal-goto-same-domain-6-interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new5.76 KB
PASSED: [[SimpleTest]]: [MySQL] 46,207 pass(es).
[ View ]

Thanks @xjm, here are the new patches.

@everyone, Please review and mark RTBC.

Status:Needs review» Needs work

The last submitted patch, 736172-drupal-goto-same-domain-6-test-only.patch, failed testing.

fizk’s picture

Status:Needs work» Reviewed & tested by the community

Since no functionality has been changed, and the tests pass and fail as expected, I'm re-marking as RTBC.

valthebald’s picture

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

Crell’s picture

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.

fizk’s picture

Thanks Crell. #1668866: Replace drupal_goto() with RedirectResponse seems to have stalled for the last two months and still needs more work. This patch is ready to go today, so I don't see why committing this now would pose a problem.

catch’s picture

Status:Reviewed & tested by the community» Fixed

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

fizk’s picture

First core patch committed - w00t!! :P

Thanks everyone.

fizk’s picture

Version:8.x-dev» 7.x-dev
Status:Fixed» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new3.65 KB
FAILED: [[SimpleTest]]: [MySQL] 39,550 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new5.39 KB
PASSED: [[SimpleTest]]: [MySQL] 39,551 pass(es).
[ View ]

Backporting to D7.

Status:Needs review» Needs work

The last submitted patch, 736172-drupal-goto-same-domain-1-test-only-d7.patch, failed testing.

fizk’s picture

Status:Needs work» Needs review

@valthebald, Can you please review and mark as RTBC :)

valthebald’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-needs backport to D7

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

valthebald’s picture

@fizk, welcome to the club!

fizk’s picture

@catch, can you please commit the D7 backport in #39? Thanks.

David_Rothstein’s picture

Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs work

@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!

fizk’s picture

Status:Needs work» Needs review
StatusFileSize
new3.24 KB
PASSED: [[SimpleTest]]: [MySQL] 47,598 pass(es).
[ View ]
new4.93 KB
PASSED: [[SimpleTest]]: [MySQL] 47,580 pass(es).
[ View ]

@David_Rothstein, Thanks, updated patch includes changes 1-4 for 8.x-dev.

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?

I had the same thought at first, but I strongly believe it makes the code much easier to understand using a new menu item.

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']?

You're right, good catch. I've added a if (!isset($url_parts['path'])) test to the patch to check for this.

fizk’s picture

@valthebald, RTBC? :)

greggles’s picture

Title:drupal_goto() should allow absolute destinations that are within the same domain» drupal_goto() should allow absolute destinations that are within the same site

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

fizk’s picture

The patch in #46 looks good to me. Waiting for someone else to RTBC.

valthebald’s picture

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

fizk’s picture

The code committed to 8.x needs to be fixed. I can backport to 7.x after that.

fizk’s picture

Status:Needs review» Reviewed & tested by the community

Seems this isn't moving forward due to the lack of RTBC status, so I'm changing the status. The patch hasn't changed too much anyway.

catch’s picture

Status:Reviewed & tested by the community» Needs work

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.

fizk’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new3.24 KB
PASSED: [[SimpleTest]]: [MySQL] 50,547 pass(es).
[ View ]
new4.93 KB
PASSED: [[SimpleTest]]: [MySQL] 50,658 pass(es).
[ View ]

Thanks catch! Here's the fixed patch against 8.x-dev as of today.

fizk’s picture

StatusFileSize
new558 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 736172-drupal-goto-same-domain-8-interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Attaching interdiff.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 736172-drupal-goto-same-domain-8-interdiff.patch, failed testing.

Crell’s picture

Status:Needs work» Reviewed & tested by the community

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

catch’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)
Issue tags:+needs backport to D7

OK. Committed/pushed to 8.x.

This looks backportable.

fizk’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new3.7 KB
FAILED: [[SimpleTest]]: [MySQL] 39,654 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new5.77 KB
PASSED: [[SimpleTest]]: [MySQL] 39,755 pass(es).
[ View ]

Updated the D7 patch using the corrections made to the D8 patch. No new features or tests.

Status:Needs review» Needs work

The last submitted patch, 736172-drupal-goto-same-domain-2-d7-test-only.patch, failed testing.

fizk’s picture

Status:Needs work» Needs review

Test only patch failed as expected.

fizk’s picture

Status:Needs review» Reviewed & tested by the community

@David, I think this is good to go.

fizk’s picture

@David Do you see any remaining issues here?

David_Rothstein’s picture

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!

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs review

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.

fizk’s picture

Yes, that shouldn't be allowed, but I don't see how the patch in #59 would allow that?

David_Rothstein’s picture

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.

fizk’s picture

StatusFileSize
new3.7 KB
FAILED: [[SimpleTest]]: [MySQL] 40,216 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new5.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 736172-drupal-goto-same-domain-3-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Gah, thanks for catching that. I've changed the if statement to:

+  if (!isset($url_parts['path'])) {
+    $url_parts['path'] = '/';
+  }
+
+  // When comparing base paths, we need a trailing slash to make sure a
+  // partial URL match isn't occuring. Since base_path() always returns with
+  // a trailing slash, we don't need to add the trailing slash here.
+  return ($url_parts['host'] == $base_host && stripos($url_parts['path'], base_path()) === 0);
fizk’s picture

D8 needs this fix too. I'll attach a D8 patch after the #68 tests are done.

Status:Needs review» Needs work

The last submitted patch, 736172-drupal-goto-same-domain-3-d7-tests-only.patch, failed testing.

fizk’s picture

Status:Needs work» Needs review
StatusFileSize
new1.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 736172-drupal-goto-same-domain-3-d8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, 736172-drupal-goto-same-domain-3-d8.patch, failed testing.

fizk’s picture

Status:Needs work» Needs review
fizk’s picture

@David_Rothstein, thanks for pointing out that previous fix. I think we should be good to go now.

Pancho’s picture

Title:drupal_goto() should allow absolute destinations that are within the same site» [Followup] drupal_goto() should allow absolute destinations that are within the same site
Version:7.x-dev» 8.x-dev
Priority:Normal» Major
Issue tags:-Novice

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.)

Pancho’s picture

Issue tags:+Needs change record

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

Pancho’s picture

Pancho’s picture

Title:[Followup] drupal_goto() should allow absolute destinations that are within the same site» Change notice: drupal_goto() should allow absolute destinations that are within the same site
Priority:Major» Critical
Status:Needs review» Needs work

Change notices are critical.

Pancho’s picture

Title:Change notice: drupal_goto() should allow absolute destinations that are within the same site» [Followup] drupal_goto() should allow absolute destinations that are within the same site
Priority:Critical» Major
Status:Needs work» Needs review

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

dlu’s picture

Component:base system» routing system

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...).

xjm’s picture

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

xjm’s picture

Issue summary:View changes

Clarify the description.

xjm’s picture

Status:Needs review» Needs work

The last submitted patch, 71: 736172-drupal-goto-same-domain-3-d8.patch, failed testing.

fizk’s picture

Issue summary:View changes

@xjm, Thanks for coming back to this. I don't think we should work on this patch until we have a commitment from someone with commit access that this will be committed as soon as the patch applies cleanly. This patch has gone through a lot of work and should have been committed a while ago.

xjm’s picture

@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.

xjm’s picture

greendemiurge’s picture

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

fizk’s picture

Version:8.x-dev» 7.x-dev

greendemiurge, thanks for the heads up.

It looks like drupal_goto() was removed in this commit:

http://cgit.drupalcode.org/drupal/commit/?id=00cb147e146d24c87bc31462e9f...

Changing this issue to 7.x.

fizk’s picture

Issue summary:View changes
Status:Needs work» Reviewed & tested by the community
Issue tags:-needs backport to D7, -Needs reroll, -Needs issue summary update

Marking the 7.x. patch RTBC, since it's been here a while, tested, and still applies cleanly against 7.x-dev.

fizk’s picture

Issue summary:View changes
David_Rothstein’s picture

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.

David_Rothstein’s picture

Priority:Major» Normal

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

The last submitted patch, 68: 736172-drupal-goto-same-domain-3-d7.patch, failed testing.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs work

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.

fizk’s picture

Thanks for coming back to this David, I appreciate your efforts. Let's make it our goal to focus on this patch for this week. We probably won't need much more time than that.

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...

This shouldn't be an issue. If an attacker can give you a URL to follow like https://example.com?destination=http://example.com , then they can also simply give you http://example.com . HTTPS isn't enforced by not having HTTP links on the page - although that helps - the site or web server needs to actively redirect HTTP requests to the HTTPS equivalent.

David_Rothstein’s picture

Title:[Followup] drupal_goto() should allow absolute destinations that are within the same site» drupal_goto() should allow absolute destinations that are within the same site
Status:Needs work» Needs review
StatusFileSize
new5.92 KB
PASSED: [[SimpleTest]]: [MySQL] 41,035 pass(es).
[ View ]
new665 bytes

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.

fizk’s picture

Thanks for the new patch.

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,

I'll add some code to parse the "/../" parent paths in _external_url_is_local().

By definition this patch allows you to redirect from http://example.com to http://example.com/drupal

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.

David_Rothstein’s picture

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!