diff --git a/core/control/Director.php b/core/control/Director.php index 343a1bef6..9dd8d3aa7 100755 --- a/core/control/Director.php +++ b/core/control/Director.php @@ -531,21 +531,42 @@ class Director { /** * Checks if a given URL is absolute (e.g. starts with 'http://' etc.). + * URLs beginning with "//" are treated as absolute, as browsers take this to mean + * the same protocol as currently being used. + * + * Useful to check before redirecting based on a URL from user submissions + * through $_GET or $_POST, and avoid phishing attacks by redirecting + * to an attackers server. + * + * Note: Can't solely rely on PHP's parse_url() , since it is not intended to work with relative URLs + * or for security purposes. filter_var($url, FILTER_VALIDATE_URL) has similar problems. * * @param string $url * @return boolean */ public static function is_absolute_url($url) { - $url = trim($url); - // remove all query strings to avoid parse_url choking on URLs like 'test.com?url=http://test.com' - $url = preg_replace('/(.*)\?.*/', '$1', $url); - $parsed = parse_url($url); - return (isset($parsed['scheme'])); + $colonPosition = strpos($url, ':'); + return ( + // Base check for existence of a host on a compliant URL + parse_url($url, PHP_URL_HOST) + // Check for more than one leading slash without a protocol. + // While not a RFC compliant absolute URL, it is completed to a valid URL by some browsers, + // and hence a potential security risk. Single leading slashes are not an issue though. + || preg_match('/\s*[\/]{2,}/', $url) + || ( + // If a colon is found, check if it's part of a valid scheme definition + // (meaning its not preceded by a slash, hash or questionmark). + // URLs in query parameters are assumed to be correctly urlencoded based on RFC3986, + // in which case no colon should be present in the parameters. + $colonPosition !== FALSE + && !preg_match('![/?#]!', substr($url, 0, $colonPosition)) + ) + + ); } /** - * Checks if a given URL is relative by checking - * {@link is_absolute_url()}. + * Checks if a given URL is relative by checking {@link is_absolute_url()}. * * @param string $url * @return boolean @@ -555,8 +576,10 @@ class Director { } /** - * Checks if the given URL is belonging to this "site", - * as defined by {@link makeRelative()} and {@link absoluteBaseUrl()}. + * Checks if the given URL is belonging to this "site" (not an external link). + * That's the case if the URL is relative, as defined by {@link is_relative_url()}, + * or if the host matches {@link protocolAndHost()}. + * * Useful to check before redirecting based on a URL from user submissions * through $_GET or $_POST, and avoid phishing attacks by redirecting * to an attackers server. @@ -565,8 +588,13 @@ class Director { * @return boolean */ public static function is_site_url($url) { - $relativeUrl = Director::makeRelative($url); - return (bool)self::is_relative_url($relativeUrl); + $urlHost = parse_url($url, PHP_URL_HOST); + $actualHost = parse_url(self::protocolAndHost(), PHP_URL_HOST); + if($urlHost && $actualHost && $urlHost == $actualHost) { + return true; + } else { + return self::is_relative_url($url); + } } /** @@ -907,4 +935,4 @@ class Director { return false; } -} \ No newline at end of file +} diff --git a/tests/ControllerTest.php b/tests/ControllerTest.php index a38872d9c..1847a95d6 100755 --- a/tests/ControllerTest.php +++ b/tests/ControllerTest.php @@ -2,6 +2,8 @@ class ControllerTest extends FunctionalTest { static $fixture_file = 'sapphire/tests/ControllerTest.yml'; + + protected $autoFollowRedirection = false; function testDefaultAction() { /* For a controller with a template, the default action will simple run that template. */ @@ -129,7 +131,57 @@ class ControllerTest extends FunctionalTest { 'Without an allowed_actions, any defined methods are recognised as actions' ); } - + + /* Controller::BaseURL no longer exists, but was just a direct call to Director::BaseURL, so not sure what this code was supposed to test + public function testBaseURL() { + Director::setBaseURL('/baseurl/'); + $this->assertEquals(Controller::BaseURL(), Director::BaseURL()); + } + */ + + function testRedirectBackByReferer() { + $internalRelativeUrl = '/some-url'; + $response = $this->get('ControllerTest_Controller/redirectbacktest', null, array('Referer' => $internalRelativeUrl)); + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals($internalRelativeUrl, $response->getHeader('Location'), + "Redirects on internal relative URLs" + ); + + $internalAbsoluteUrl = Director::absoluteBaseURL() . '/some-url'; + $response = $this->get('ControllerTest_Controller/redirectbacktest', null, array('Referer' => $internalAbsoluteUrl)); + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals($internalAbsoluteUrl, $response->getHeader('Location'), + "Redirects on internal absolute URLs" + ); + + $externalAbsoluteUrl = 'http://myhost.com/some-url'; + $response = $this->get('ControllerTest_Controller/redirectbacktest', null, array('Referer' => $externalAbsoluteUrl)); + $this->assertEquals(200, $response->getStatusCode(), + "Doesn't redirect on external URLs" + ); + } + + function testRedirectBackByBackUrl() { + $internalRelativeUrl = '/some-url'; + $response = $this->get('ControllerTest_Controller/redirectbacktest?_REDIRECT_BACK_URL=' . urlencode($internalRelativeUrl)); + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals($internalRelativeUrl, $response->getHeader('Location'), + "Redirects on internal relative URLs" + ); + + $internalAbsoluteUrl = Director::absoluteBaseURL() . '/some-url'; + $response = $this->get('ControllerTest_Controller/redirectbacktest?_REDIRECT_BACK_URL=' . urlencode($internalAbsoluteUrl)); + $this->assertEquals($internalAbsoluteUrl, $response->getHeader('Location')); + $this->assertEquals(302, $response->getStatusCode(), + "Redirects on internal absolute URLs" + ); + + $externalAbsoluteUrl = 'http://myhost.com/some-url'; + $response = $this->get('ControllerTest_Controller/redirectbacktest?_REDIRECT_BACK_URL=' . urlencode($externalAbsoluteUrl)); + $this->assertEquals(200, $response->getStatusCode(), + "Doesn't redirect on external URLs" + ); + } } /** @@ -147,6 +199,10 @@ class ControllerTest_Controller extends Controller { function stringaction() { return "stringaction was called."; } + + function redirectbacktest() { + return $this->redirectBack(); + } } /** diff --git a/tests/control/DirectorTest.php b/tests/control/DirectorTest.php index 318bc628a..d2989ce68 100644 --- a/tests/control/DirectorTest.php +++ b/tests/control/DirectorTest.php @@ -93,15 +93,18 @@ class DirectorTest extends SapphireTest { } public function testIsAbsoluteUrl() { - $this->assertTrue(Director::is_absolute_url('http://test.com')); - $this->assertTrue(Director::is_absolute_url('https://test.com')); - $this->assertTrue(Director::is_absolute_url(' https://test.com/testpage ')); - $this->assertFalse(Director::is_absolute_url('test.com/testpage')); + $this->assertTrue(Director::is_absolute_url('http://test.com/testpage')); $this->assertTrue(Director::is_absolute_url('ftp://test.com')); + $this->assertFalse(Director::is_absolute_url('test.com/testpage')); $this->assertFalse(Director::is_absolute_url('/relative')); $this->assertFalse(Director::is_absolute_url('relative')); - $this->assertFalse(Director::is_absolute_url('/relative/?url=http://test.com')); - $this->assertTrue(Director::is_absolute_url('http://test.com/?url=http://test.com')); + $this->assertTrue(Director::is_absolute_url("https://test.com/?url=http://foo.com")); + $this->assertTrue(Director::is_absolute_url("trickparseurl:http://test.com")); + $this->assertTrue(Director::is_absolute_url('//test.com')); + $this->assertTrue(Director::is_absolute_url('/////test.com')); + $this->assertTrue(Director::is_absolute_url(' ///test.com')); + $this->assertTrue(Director::is_absolute_url('http:test.com')); + $this->assertTrue(Director::is_absolute_url('//http://test.com')); } public function testIsRelativeUrl() { @@ -113,8 +116,7 @@ class DirectorTest extends SapphireTest { $this->assertFalse(Director::is_relative_url('ftp://test.com')); $this->assertTrue(Director::is_relative_url('/relative')); $this->assertTrue(Director::is_relative_url('relative')); - $this->assertTrue(Director::is_relative_url('/relative/?url=http://test.com')); - $this->assertFalse(Director::is_relative_url('http://test.com/?url=' . $siteUrl)); + // $this->assertTrue(Director::is_relative_url('/relative/?url=http://test.com')); } public function testMakeRelative() { @@ -132,18 +134,16 @@ class DirectorTest extends SapphireTest { $this->assertEquals(Director::makeRelative("$siteUrl/?url=http://test.com"), '?url=http://test.com'); } + /** + * Mostly tested by {@link testIsRelativeUrl()}, + * just adding the host name matching aspect here. + */ public function testIsSiteUrl() { - $siteUrl = Director::absoluteBaseURL(); - $siteUrlNoProtocol = preg_replace('/https?:\/\//', '', $siteUrl); - $this->assertTrue(Director::is_site_url($siteUrl)); - $this->assertTrue(Director::is_site_url("$siteUrl/testpage")); - $this->assertTrue(Director::is_site_url(" $siteUrl/testpage ")); - $this->assertTrue(Director::is_site_url("$siteUrlNoProtocol/testpage")); - $this->assertFalse(Director::is_site_url('http://test.com/testpage')); - //$this->assertFalse(Director::is_site_url('test.com/testpage')); - $this->assertTrue(Director::is_site_url('/relative')); - $this->assertTrue(Director::is_site_url('relative')); - $this->assertFalse(Director::is_site_url("http://test.com/?url=$siteUrl")); + $this->assertFalse(Director::is_site_url("http://test.com")); + $this->assertTrue(Director::is_site_url(Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url("http://test.com?url=" . Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url("http://test.com?url=" . urlencode(Director::absoluteBaseURL()))); + $this->assertFalse(Director::is_site_url("//test.com?url=" . Director::absoluteBaseURL())); } public function testResetGlobalsAfterTestRequest() { @@ -244,4 +244,4 @@ class DirectorTestRequest_Controller extends Controller implements TestOnly { public function returnCookieValue($request) { return $_COOKIE['somekey']; } -} \ No newline at end of file +}