Merge pull request #401 from chillu/is-absolute-url

Director::is_absolute_url() security fixes

control/Director.php

@@ -509,21 +509,42 @@ class Director implements TemplateGlobalProvider {
 	
 	/**
 	 * 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);
+		$colonPosition = strpos($url, ':');
-		// remove all query strings to avoid parse_url choking on URLs like 'test.com?url=http://test.com'
+	  return (
-		$url = preg_replace('/(.*)\?.*/', '$1', $url);
+	  	// Base check for existence of a host on a compliant URL
-		$parsed = parse_url($url);
+	  	parse_url($url, PHP_URL_HOST)
-		return (isset($parsed['scheme']));
+	  	// 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
+	 * Checks if a given URL is relative by checking {@link is_absolute_url()}.
-	 * {@link is_absolute_url()}.
 	 * 
 	 * @param string $url
 	 * @return boolean
@@ -533,8 +554,10 @@ class Director implements TemplateGlobalProvider {
 	}
 	
 	/**
-	 * Checks if the given URL is belonging to this "site",
+	 * Checks if the given URL is belonging to this "site" (not an external link).
-	 * as defined by {@link makeRelative()} and {@link absoluteBaseUrl()}.
+	 * 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.
@@ -543,8 +566,13 @@ class Director implements TemplateGlobalProvider {
 	 * @return boolean
 	 */
 	public static function is_site_url($url) {
-		$relativeUrl = Director::makeRelative($url);
+		$urlHost = parse_url($url, PHP_URL_HOST);
-		return (bool)self::is_relative_url($relativeUrl);
+		$actualHost = parse_url(self::protocolAndHost(), PHP_URL_HOST);
+		if($urlHost && $actualHost && $urlHost == $actualHost) {
+			return true;
+		} else {
+			return self::is_relative_url($url);
+		}
 	}
 
 	/**

dev/TestSession.php

@@ -43,7 +43,7 @@ class TestSession {
 	 */
 	function get($url, $session = null, $headers = null, $cookies = null) {
 		$headers = (array) $headers;
-		if($this->lastUrl) $headers['Referer'] = $this->lastUrl;
+		if($this->lastUrl && !isset($headers['Referer'])) $headers['Referer'] = $this->lastUrl;
 		$this->lastResponse = Director::test($url, null, $session ? $session : $this->session, null, null, $headers, $cookies);
 		$this->lastUrl = $url;
 		if(!$this->lastResponse) user_error("Director::test($url) returned null", E_USER_WARNING);
@@ -56,7 +56,7 @@ class TestSession {
 	 */
 	function post($url, $data, $headers = null, $session = null, $body = null, $cookies = null) {
 		$headers = (array) $headers;
-		if($this->lastUrl) $headers['Referer'] = $this->lastUrl;
+		if($this->lastUrl && !isset($headers['Referer'])) $headers['Referer'] = $this->lastUrl;
 		$this->lastResponse = Director::test($url, $data, $session ? $session : $this->session, null, $body, $headers, $cookies);
 		$this->lastUrl = $url;
 		if(!$this->lastResponse) user_error("Director::test($url) returned null", E_USER_WARNING);

tests/control/ControllerTest.php

@@ -1,8 +1,11 @@
 <?php
 
 class ControllerTest extends FunctionalTest {
+	
 	static $fixture_file = 'ControllerTest.yml';
 
+	protected $autoFollowRedirection = false;
+	
 	function testDefaultAction() {
 		/* For a controller with a template, the default action will simple run that template. */
 		$response = $this->get("ControllerTest_Controller/");
@@ -139,6 +142,50 @@ class ControllerTest extends FunctionalTest {
 		$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?BackURL=' . 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?BackURL=' . 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?BackURL=' . urlencode($externalAbsoluteUrl));
+		$this->assertEquals(200, $response->getStatusCode(),
+			"Doesn't redirect on external URLs"
+		);
+	}
 }
 
 /**
@@ -156,6 +203,10 @@ class ControllerTest_Controller extends Controller implements TestOnly {
 	function stringaction() {
 		return "stringaction was called.";
 	}
+
+	function redirectbacktest() {
+		return $this->redirectBack();
+	}
 }
 
 /**

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('http://test.com/testpage'));
-		$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('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("https://test.com/?url=http://foo.com"));
-		$this->assertTrue(Director::is_absolute_url('http://test.com/?url=http://test.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->assertTrue(Director::is_relative_url('/relative/?url=http://test.com'));
-		$this->assertFalse(Director::is_relative_url('http://test.com/?url=' . $siteUrl));
 	}
 	
 	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();
+		$this->assertFalse(Director::is_site_url("http://test.com"));
-		$siteUrlNoProtocol = preg_replace('/https?:\/\//', '', $siteUrl);
+		$this->assertTrue(Director::is_site_url(Director::absoluteBaseURL()));
-		$this->assertTrue(Director::is_site_url($siteUrl));
+		$this->assertFalse(Director::is_site_url("http://test.com?url=" . Director::absoluteBaseURL()));
-		$this->assertTrue(Director::is_site_url("$siteUrl/testpage"));
+		$this->assertFalse(Director::is_site_url("http://test.com?url=" . urlencode(Director::absoluteBaseURL())));
-		$this->assertTrue(Director::is_site_url("   $siteUrl/testpage   "));
+		$this->assertFalse(Director::is_site_url("//test.com?url=" . Director::absoluteBaseURL()));
-		$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"));
 	}
 	
 	public function testResetGlobalsAfterTestRequest() {