From 6d708765fe2b55b625c295a7dc83ae160e69034e Mon Sep 17 00:00:00 2001
From: Ingo Schommer <ingo@silverstripe.com>
Date: Wed, 11 Feb 2009 21:08:28 +0000
Subject: [PATCH] BUGFIX Fixed redirection to external URLs through
 Security/login with BackURL parameter

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@71708 467b73ca-7a2a-4603-9d3b-597d59a354a9
---
 security/MemberLoginForm.php    | 15 +++++++++----
 tests/security/SecurityTest.php | 38 +++++++++++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/security/MemberLoginForm.php b/security/MemberLoginForm.php
index 39ea5c5ce..e0b4fc927 100644
--- a/security/MemberLoginForm.php
+++ b/security/MemberLoginForm.php
@@ -111,7 +111,6 @@ class MemberLoginForm extends LoginForm {
 		if($this->performLogin($data)) {
 			Session::clear('SessionForms.MemberLoginForm.Email');
 			Session::clear('SessionForms.MemberLoginForm.Remember');
-			
 			if(Member::currentUser()->isPasswordExpired()) {
 				if(isset($_REQUEST['BackURL']) && $backURL = $_REQUEST['BackURL']) {
 					Session::set('BackURL', $backURL);
@@ -121,9 +120,17 @@ class MemberLoginForm extends LoginForm {
 				$cp->sessionMessage('Your password has expired.  Please choose a new one.', 'good');
 				
 				Director::redirect('Security/changepassword');
-			} elseif(isset($_REQUEST['BackURL']) && $backURL = $_REQUEST['BackURL']) {
-				Session::clear("BackURL");
-				Director::redirect($backURL);
+			} elseif(
+				isset($_REQUEST['BackURL']) 
+				&& $_REQUEST['BackURL'] 
+				&& (
+					// absolute redirection URLs may cause spoofing 
+					!Director::is_absolute_url($_REQUEST['BackURL'])
+					// absolute URLs on the current domain are allowed
+					|| strpos($_REQUEST['BackURL'], Director::absoluteBaseURL()) !== FALSE
+				)
+			) {
+				Director::redirect($_REQUEST['BackURL']);
 			} else {
 				$member = Member::currentUser();
 				if($member) {
diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php
index 1a7e78c2e..08898310b 100644
--- a/tests/security/SecurityTest.php
+++ b/tests/security/SecurityTest.php
@@ -37,6 +37,40 @@ class SecurityTest extends FunctionalTest {
 		parent::tearDown();
 	}
 	
+	function testExternalBackUrlRedirectionDisallowed() {
+		$page = new SiteTree();
+		$page->URLSegment = 'testpage';
+		$page->Title = 'Testpage';
+		$page->write();
+		$page->publish('Stage','Live');
+		
+		// Test internal relative redirect
+		$response = $this->doTestLoginForm('noexpiry@silverstripe.com', '1nitialPassword', 'testpage');
+		$this->assertEquals(302, $response->getStatusCode());
+		$this->assertRegExp('/testpage/', $response->getHeader('Location'),
+			"Internal relative BackURLs work when passed through to login form"
+		);
+		// Log the user out
+		$this->session()->inst_set('loggedInAs', null);
+		
+		// Test internal absolute redirect
+		$response = $this->doTestLoginForm('noexpiry@silverstripe.com', '1nitialPassword', Director::absoluteBaseURL() . 'testpage');
+		// for some reason the redirect happens to a relative URL
+		$this->assertRegExp('/^' . preg_quote(Director::absoluteBaseURL(), '/') . 'testpage/', $response->getHeader('Location'),
+			"Internal absolute BackURLs work when passed through to login form"
+		);
+		// Log the user out
+		$this->session()->inst_set('loggedInAs', null);
+		
+		// Test external redirect
+		$response = $this->doTestLoginForm('noexpiry@silverstripe.com', '1nitialPassword', 'http://myspoofedhost.com');
+		$this->assertNotRegExp('/^' . preg_quote('http://myspoofedhost.com', '/') . '/', $response->getHeader('Location'),
+			"Redirection to external links in login form BackURL gets prevented as a measure against spoofing attacks"
+		);
+		// Log the user out
+		$this->session()->inst_set('loggedInAs', null);
+	}
+	
 	/**
 	 * Test that the login form redirects to the change password form after logging in with an expired password
 	 */
@@ -183,8 +217,8 @@ class SecurityTest extends FunctionalTest {
 	 * Execute a log-in form using Director::test().
 	 * Helper method for the tests above
 	 */
-	function doTestLoginForm($email, $password) {
-		$this->session()->inst_set('BackURL', 'test/link');
+	function doTestLoginForm($email, $password, $backURL = 'test/link') {
+		$this->session()->inst_set('BackURL', $backURL);
 		$this->get('Security/login');
 		
 		return $this->submitForm(