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.4@114760 467b73ca-7a2a-4603-9d3b-597d59a354a9

security/ChangePasswordForm.php

@@ -28,7 +28,11 @@ class ChangePasswordForm extends Form {
 		
 		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")));
 			}
 
@@ -93,10 +97,9 @@ class ChangePasswordForm extends Form {
 		else 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');
 				
 				if (isset($_REQUEST['BackURL']) 

security/Security.php

@@ -512,7 +512,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.
 	 */
@@ -524,10 +531,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>' . 
@@ -535,7 +547,6 @@ class Security extends Controller {
 					'</p>',
 				'Form' => $this->ChangePasswordForm(),
 			));
-
 		} elseif(Member::currentUser()) {
 			// let a logged in user change his password
 			$customisedController = $controller->customise(array(
@@ -566,7 +577,6 @@ class Security extends Controller {
 			}
 		}
 
-		//Controller::$currentController = $controller;
 		return $customisedController->renderWith(array('Security_changepassword', 'Security', $this->stat('template_main'), 'ContentController'));
 	}
 	

tests/security/SecurityTest.php

@@ -165,7 +165,7 @@ class SecurityTest extends FunctionalTest {
 		$this->assertEquals(Director::baseURL() . 'test/link', $changedResponse->getHeader('Location'));
 	}
 	
-	function testChangePassword() {
+	function testChangePasswordForLoggedInUsers() {
 		$goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword');
 		
 		// Change the password
@@ -182,6 +182,35 @@ class SecurityTest extends FunctionalTest {
 		$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() {
 		$local = i18n::get_locale();
 		i18n::set_locale('en_US');