BUGFIX Avoid potential referer leaking in Security->changepassword() form by storing Member->AutoLoginHash in session instead of 'h' GET parameter (from r114758)

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/branches/2.3@114763 467b73ca-7a2a-4603-9d3b-597d59a354a9

security/ChangePasswordForm.php

@@ -5,7 +5,7 @@
  * @subpackage security
  */
 class ChangePasswordForm extends Form {
-
+	
 	/**
 	 * Constructor
 	 *
@@ -22,7 +22,11 @@ class ChangePasswordForm extends Form {
 	function __construct($controller, $name, $fields = null, $actions = null) {
 		if(!$fields) {
 			$fields = new FieldSet();
-			if(Member::currentUser() && (!isset($_REQUEST['h']) || !Member::member_from_autologinhash($_REQUEST['h']))) {
+			
+			// Security/changepassword?h=XXX redirects to Security/changepassword
+			// without GET parameter to avoid potential HTTP referer leakage.
+			// In this case, a user is not logged in, and no 'old password' should be necessary.
+			if(Member::currentUser()) {
 				$fields->push(new PasswordField("OldPassword",_t('Member.YOUROLDPASSWORD', "Your old password")));
 			}
 
@@ -75,10 +79,9 @@ class ChangePasswordForm extends Form {
 		if($data['NewPassword1'] == $data['NewPassword2']) {
 			$isValid = $member->changePassword($data['NewPassword1']);
 			if($isValid->valid()) {
-				$this->clearMessage();
+				$member->logIn();
-				$this->sessionMessage(
+				
-					_t('Member.PASSWORDCHANGED', "Your password has been changed, and a copy emailed to you."),
+				// TODO Add confirmation message to login redirect
-					"good");
 				Session::clear('AutoLoginHash');
 				$redirectURL = HTTP::setGetVar('BackURL', urlencode(Director::absoluteBaseURL()), Security::Link('login'));
 				Director::redirect($redirectURL);

security/Security.php

@@ -471,7 +471,14 @@ class Security extends Controller {
 	}
 	
 	/**
-	 * Show the "change password" page
+	 * Show the "change password" page.
+	 * This page can either be called directly by logged-in users
+	 * (in which case they need to provide their old password),
+	 * or through a link emailed through {@link lostpassword()}.
+	 * In this case no old password is required, authentication is ensured
+	 * through the Member.AutoLoginHash property.
+	 * 
+	 * @see ChangePasswordForm
 	 *
 	 * @return string Returns the "change password" page as HTML code.
 	 */
@@ -482,10 +489,15 @@ class Security extends Controller {
 		$controller = new Page_Controller($tmpPage);
 		$controller->init();
 
+		// First load with hash: Redirect to same URL without hash to avoid referer leakage
 		if(isset($_REQUEST['h']) && Member::member_from_autologinhash($_REQUEST['h'])) {
-			// The auto login hash is valid, store it for the change password form
+			// The auto login hash is valid, store it for the change password form.
+			// Temporary value, unset in ChangePasswordForm
 			Session::set('AutoLoginHash', $_REQUEST['h']);
-
+			
+			return $this->redirect($this->Link('changepassword'));
+		// Redirection target after "First load with hash"
+		} elseif(Session::get('AutoLoginHash')) {
 			$customisedController = $controller->customise(array(
 				'Content' =>
 					'<p>' . 
@@ -493,7 +505,6 @@ class Security extends Controller {
 					'</p>',
 				'Form' => $this->ChangePasswordForm(),
 			));
-
 		} elseif(Member::currentUser()) {
 			// let a logged in user change his password
 			$customisedController = $controller->customise(array(
@@ -524,7 +535,6 @@ class Security extends Controller {
 			}
 		}
 
-		//Controller::$currentController = $controller;
 		return $customisedController->renderWith(array('Security_changepassword', 'Security', $this->stat('template_main')));
 	}
 	

tests/security/SecurityTest.php

@@ -142,6 +142,51 @@ class SecurityTest extends FunctionalTest {
 		$this->assertEquals($this->idFromFixture('Member', 'expiredpassword'), $this->session()->inst_get('loggedInAs'));
 	}
 	
+
+	function testChangePasswordForLoggedInUsers() {
+		$goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword');
+		
+		// Change the password
+		$this->get('Security/changepassword');
+		$changedResponse = $this->doTestChangepasswordForm('1nitialPassword', 'changedPassword');
+		$this->assertEquals(302, $changedResponse->getStatusCode());
+		$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($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs'));
+	}
+	
+	function testChangePasswordFromLostPassword() {
+		$admin = $this->objFromFixture('Member', 'test');
+
+		$this->assertNull($admin->AutoLoginHash, 'Hash is empty before lost password');
+		
+		// Request new password by email
+		$response = $this->get('Security/lostpassword');
+		$response = $this->submitForm('MemberLoginForm_LostPasswordForm', null, array('Email' => 'sam@silverstripe.com'));
+		
+		$this->assertEmailSent('sam@silverstripe.com');
+		
+		// Load password link from email
+		$admin = DataObject::get_by_id('Member', $admin->ID);
+		$this->assertNotNull($admin->AutoLoginHash, 'Hash has been written after lost password');
+		$response = $this->get('Security/changepassword/?h=' . $admin->AutoLoginHash);
+		$this->assertEquals(302, $response->getStatusCode());
+		$this->assertEquals(Director::baseUrl() . 'Security/changepassword', $response->getHeader('Location'));
+		
+		// Follow redirection to form without hash in GET parameter
+		$response = $this->get('Security/changepassword');
+		$changedResponse = $this->doTestChangepasswordForm('1nitialPassword', 'changedPassword');
+		$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($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs'));
+	}
+		
 	function testRepeatedLoginAttemptsLockingPeopleOut() {
 		Member::lock_out_after_incorrect_logins(5);
 		
@@ -283,6 +328,19 @@ class SecurityTest extends FunctionalTest {
 		); 
 	}
 	
+	function doTestChangepasswordForm($oldPassword, $newPassword) {
+		return $this->submitForm(
+			"ChangePasswordForm_ChangePasswordForm", 
+			null,
+			array(
+				'OldPassword' => $oldPassword, 
+				'NewPassword1' => $newPassword, 
+				'NewPassword2' => $newPassword,
+				'action_doChangePassword' => 1,
+			)
+		);
+	}
+	
 	/**
 	 * Get the error message on the login form
 	 */