tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-10-22 14:05:37 +02:00
Code Issues Projects Releases Wiki Activity

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

Browse Source 
git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@114758 467b73ca-7a2a-4603-9d3b-597d59a354a9
This commit is contained in:
Ingo Schommer 2010-12-09 21:18:49 +00:00
parent e4a786eb1a
commit 4b2c64c843
3 changed files with 54 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
security/ChangePasswordForm.php
View File

@ -5,7 +5,7 @@
* @subpackage security
*/
class ChangePasswordForm extends Form {
/**
* Constructor
*
@ -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();
$this->sessionMessage(
_t('Member.PASSWORDCHANGED', "Your password has been changed, and a copy emailed to you."),
"good");
$member->logIn();
// TODO Add confirmation message to login redirect
Session::clear('AutoLoginHash');
if (isset($_REQUEST['BackURL'])

20
security/Security.php
View File

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

31
tests/security/SecurityTest.php
View File

@ -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
@ -181,6 +181,35 @@ class SecurityTest extends FunctionalTest {
$this->assertEquals(Director::baseURL() . 'test/link', $goodResponse->getHeader('Location'));
$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();