Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

BUGFIX Avoid potential referer leaking in Security->changepassword() …

…form by storing Member->AutoLoginHash in session instead of 'h' GET parameter

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@114758 467b73ca-7a2a-4603-9d3b-597d59a354a9
  • Loading branch information...
commit 4b2c64c843093f2f08557ded108a5379b122097e 1 parent e4a786e
@chillu chillu authored
View
15 security/ChangePasswordForm.php
@@ -5,7 +5,7 @@
* @subpackage security
*/
class ChangePasswordForm extends Form {
-
+
/**
* Constructor
*
@@ -28,7 +28,11 @@ 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")));
}
@@ -93,10 +97,9 @@ function doChangePassword(array $data) {
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'])
View
20 security/Security.php
@@ -519,7 +519,14 @@ public static function getPasswordResetLink($autoLoginHash) {
}
/**
- * 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 @@ public function changepassword() {
$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 @@ public function changepassword() {
'</p>',
'Form' => $this->ChangePasswordForm(),
));
-
} elseif(Member::currentUser()) {
// let a logged in user change his password
$customisedController = $controller->customise(array(
@@ -573,7 +584,6 @@ public function changepassword() {
}
}
- //Controller::$currentController = $controller;
return $customisedController->renderWith(array('Security_changepassword', 'Security', $this->stat('template_main'), 'ContentController'));
}
View
31 tests/security/SecurityTest.php
@@ -165,7 +165,7 @@ function testExpiredPassword() {
$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 @@ function testChangePassword() {
$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();
Please sign in to comment.
Something went wrong with that request. Please try again.