BUG Fix malformed urls redirecting to external sites

control/Controller.php

@@ -497,6 +497,7 @@ class Controller extends RequestHandler implements TemplateGlobalProvider {
 
 		// absolute redirection URLs not located on this site may cause phishing
 		if(Director::is_site_url($url)) {
+			$url = Director::absoluteURL($url, true);
 			return $this->redirect($url);
 		} else {
 			return false;

forms/Form.php

@@ -349,6 +349,7 @@ class Form extends RequestHandler {
 						if(Director::is_site_url($pageURL)) {
 							// Remove existing pragmas
 							$pageURL = preg_replace('/(#.*)/', '', $pageURL);
+							$pageURL = Director::absoluteURL($pageURL, true);
 							return $this->controller->redirect($pageURL . '#' . $this->FormName());
 						}
 					}

security/ChangePasswordForm.php

@@ -102,12 +102,12 @@ class ChangePasswordForm extends Form {
 				// TODO Add confirmation message to login redirect
 				Session::clear('AutoLoginHash');
 				
-				if (isset($_REQUEST['BackURL']) 
+				if (!empty($_REQUEST['BackURL'])
-					&& $_REQUEST['BackURL'] 
 					// absolute redirection URLs may cause spoofing 
 					&& Director::is_site_url($_REQUEST['BackURL'])
 				) {
-					$this->controller->redirect($_REQUEST['BackURL']);
+					$url = Director::absoluteURL($_REQUEST['BackURL']);
+					$this->controller->redirect($url);
 				}
 				else {
 					// Redirect to default location - the login form saying "You are logged in as..."

security/MemberLoginForm.php

@@ -165,7 +165,7 @@ JS
 	 * )
 	 *
 	 * @param array $data
-	 * @return void
+	 * @return SS_HTTPResponse
 	 */
 	protected function logInUserAndRedirect($data) {
 		Session::clear('SessionForms.MemberLoginForm.Email');
@@ -181,18 +181,21 @@ JS
 		}
 		
 		// Absolute redirection URLs may cause spoofing
-		if(isset($_REQUEST['BackURL']) && $_REQUEST['BackURL'] && Director::is_site_url($_REQUEST['BackURL']) ) {
+		if(!empty($_REQUEST['BackURL'])) {
-			return $this->controller->redirect($_REQUEST['BackURL']);
+			$url = $_REQUEST['BackURL'];
-		}
+			if(Director::is_site_url($url) ) {
-
+				$url = Director::absoluteURL($url);
-		// Spoofing attack, redirect to homepage instead of spoofing url
+			} else {
-		if(isset($_REQUEST['BackURL']) && $_REQUEST['BackURL'] && !Director::is_site_url($_REQUEST['BackURL'])) {
+				// Spoofing attack, redirect to homepage instead of spoofing url
-			return $this->controller->redirect(Director::absoluteBaseURL());
+				$url = Director::absoluteBaseURL();
+			}
+			return $this->controller->redirect($url);
 		}
 
 		// If a default login dest has been set, redirect to that.
-		if (Security::default_login_dest()) {
+		if ($url = Security::default_login_dest()) {
-			return $this->controller->redirect(Director::absoluteBaseURL() . Security::default_login_dest());
+			$url = Controller::join_links(Director::absoluteBaseURL(), $url);
+			return $this->controller->redirect($url);
 		}
 
 		// Redirect the user to the page where he came from

tests/control/ControllerTest.php

@@ -201,14 +201,15 @@ class ControllerTest extends FunctionalTest {
 
 	public function testRedirectBackByReferer() {
 		$internalRelativeUrl = '/some-url';
+		$internalAbsoluteUrl = Controller::join_links(Director::absoluteBaseURL(), '/some-url');
+		
 		$response = $this->get('ControllerTest_Controller/redirectbacktest', null,
 			array('Referer' => $internalRelativeUrl));
 		$this->assertEquals(302, $response->getStatusCode());
-		$this->assertEquals($internalRelativeUrl, $response->getHeader('Location'),
+		$this->assertEquals($internalAbsoluteUrl, $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());
@@ -226,9 +227,11 @@ class ControllerTest extends FunctionalTest {
 
 	public function testRedirectBackByBackUrl() {
 		$internalRelativeUrl = '/some-url';
+		$internalAbsoluteUrl = Controller::join_links(Director::absoluteBaseURL(), '/some-url');
+		
 		$response = $this->get('ControllerTest_Controller/redirectbacktest?BackURL=' . urlencode($internalRelativeUrl));
 		$this->assertEquals(302, $response->getStatusCode());
-		$this->assertEquals($internalRelativeUrl, $response->getHeader('Location'),
+		$this->assertEquals($internalAbsoluteUrl, $response->getHeader('Location'),
 			"Redirects on internal relative URLs"
 		);
 

tests/core/startup/ParameterConfirmationTokenTest.php

@@ -31,9 +31,12 @@ class ParameterConfirmationTokenTest extends SapphireTest {
 
 		return array($answer, $slash);
 	}
+
+	protected $oldHost = null;
 	
 	public function setUp() {
 		parent::setUp();
+		$this->oldHost = $_SERVER['HTTP_HOST'];
 		$_GET['parameterconfirmationtokentest_notoken'] = 'value';
 		$_GET['parameterconfirmationtokentest_empty'] = '';
 		$_GET['parameterconfirmationtokentest_withtoken'] = '1';
@@ -48,6 +51,7 @@ class ParameterConfirmationTokenTest extends SapphireTest {
 		foreach($_GET as $param) {
 			if(stripos($param, 'parameterconfirmationtokentest_') === 0) unset($_GET[$param]);
 		}
+		$_SERVER['HTTP_HOST'] = $this->oldHost;
 		parent::tearDown();
 	}
 	

tests/security/SecurityTest.php

@@ -177,13 +177,19 @@ class SecurityTest extends FunctionalTest {
 		/* UNEXPIRED PASSWORD GO THROUGH WITHOUT A HITCH */
 		$goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword');
 		$this->assertEquals(302, $goodResponse->getStatusCode());
-		$this->assertEquals(Director::baseURL() . 'test/link', $goodResponse->getHeader('Location'));
+		$this->assertEquals(
+			Controller::join_links(Director::absoluteBaseURL(), 'test/link'),
+			$goodResponse->getHeader('Location')
+		);
 		$this->assertEquals($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs'));
 		
 		/* EXPIRED PASSWORDS ARE SENT TO THE CHANGE PASSWORD FORM */
 		$expiredResponse = $this->doTestLoginForm('expired@silverstripe.com' , '1nitialPassword');
 		$this->assertEquals(302, $expiredResponse->getStatusCode());
-		$this->assertEquals(Director::baseURL() . 'Security/changepassword', $expiredResponse->getHeader('Location'));
+		$this->assertEquals(
+			Controller::join_links(Director::baseURL(), 'Security/changepassword'),
+			$expiredResponse->getHeader('Location')
+		);
 		$this->assertEquals($this->idFromFixture('Member', 'expiredpassword'), 
 			$this->session()->inst_get('loggedInAs'));
 
@@ -191,7 +197,10 @@ class SecurityTest extends FunctionalTest {
 		$this->mainSession->followRedirection();
 		$changedResponse = $this->doTestChangepasswordForm('1nitialPassword', 'changedPassword');
 		$this->assertEquals(302, $changedResponse->getStatusCode());
-		$this->assertEquals(Director::baseURL() . 'test/link', $changedResponse->getHeader('Location'));
+		$this->assertEquals(
+			Controller::join_links(Director::absoluteBaseURL(), 'test/link'),
+			$changedResponse->getHeader('Location')
+		);
 	}
 	
 	public function testChangePasswordForLoggedInUsers() {
@@ -201,13 +210,19 @@ class SecurityTest extends FunctionalTest {
 		$this->get('Security/changepassword?BackURL=test/back');
 		$changedResponse = $this->doTestChangepasswordForm('1nitialPassword', 'changedPassword');
 		$this->assertEquals(302, $changedResponse->getStatusCode());
-		$this->assertEquals(Director::baseURL() . 'test/back', $changedResponse->getHeader('Location'));
+		$this->assertEquals(
+			Controller::join_links(Director::absoluteBaseURL(), 'test/back'),
+			$changedResponse->getHeader('Location')
+		);
 		$this->assertEquals($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs'));
 		
 		// Check if we can login with the new password
 		$goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , 'changedPassword');
 		$this->assertEquals(302, $goodResponse->getStatusCode());
-		$this->assertEquals(Director::baseURL() . 'test/link', $goodResponse->getHeader('Location'));
+		$this->assertEquals(
+			Controller::join_links(Director::absoluteBaseURL(), 'test/link'),
+			$goodResponse->getHeader('Location')
+		);
 		$this->assertEquals($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs'));
 	}
 	

thirdparty/simpletest/url.php

@@ -410,7 +410,7 @@ class SimpleUrl {
      */
     function asString() {
         $path = $this->_path;
-        $scheme = $identity = $host = $encoded = $fragment = '';
+        $scheme = $identity = $host = $port = $encoded = $fragment = '';
         if ($this->_username && $this->_password) {
             $identity = $this->_username . ':' . $this->_password . '@';
         }
@@ -419,13 +419,16 @@ class SimpleUrl {
             $scheme .= "://";
             $host = $this->getHost();
         }
+        if ($this->getPort() && $this->getPort() != 80 ) {
+            $port = ':'.$this->getPort();
+        }
         if (substr($this->_path, 0, 1) == '/') {
             $path = $this->normalisePath($this->_path);
         }
         $encoded = $this->getEncodedRequest();
         $fragment = $this->getFragment() ? '#'. $this->getFragment() : '';
         $coords = $this->getX() === false ? '' : '?' . $this->getX() . ',' . $this->getY();
-        return "$scheme$identity$host$path$encoded$fragment$coords";
+        return "$scheme$identity$host$port$path$encoded$fragment$coords";
     }
     
     /**