Merge pull request #72 from silverstripe-security/pulls/3/security-password-fix

SECURITY: Ensure passwords do not get added to session on submission failure

forms/PasswordField.php

@@ -7,6 +7,7 @@
  * @subpackage fields-formattedinput
  */
 class PasswordField extends TextField {
+
 	/**
 	 * Controls the autocomplete attribute on the field.
 	 *
@@ -15,6 +16,13 @@ class PasswordField extends TextField {
 	 */
 	private static $autocomplete;
 
+    /**
+     * If true, the field can accept a value attribute, e.g. from posted form data
+     * @var bool
+     */
+    protected $allowValuePostback = false;
+	
+
 	/**
 	 * Returns an input field.
 	 *
@@ -33,6 +41,24 @@ class PasswordField extends TextField {
 		parent::__construct($name, $title, $value);
 	}
 
+    /**
+     * @param bool $bool
+     * @return $this
+     */
+    public function setAllowValuePostback($bool) {
+        $this->allowValuePostback = (bool) $bool;
+
+        return $this;
+    }
+
+    /**
+     * @return bool
+     */
+    public function getAllowValuePostback() {
+        return $this->allowValuePostback;
+    }
+
+
 	/**
 	 * {@inheritdoc}
 	 */
@@ -41,6 +67,11 @@ class PasswordField extends TextField {
 			'type' => 'password',
 		);
 
+
+        if (!$this->getAllowValuePostback()) {
+            $attributes['value'] = null;
+        }
+
 		$autocomplete = Config::inst()->get('PasswordField', 'autocomplete');
 
 		if($autocomplete) {

tests/forms/FormTest.php

@@ -24,6 +24,13 @@ class FormTest extends FunctionalTest {
 		Config::inst()->remove('SSViewer', 'theme');
 	}
 
+	public function boolDataProvider() {
+		return array(
+			array(false),
+			array(true),
+		);
+	}
+
 	public function testLoadDataFromRequest() {
 		$form = new Form(
 			new Controller(),
@@ -728,6 +735,57 @@ class FormTest extends FunctionalTest {
         $this->assertEmpty($formData['ExtraFieldCheckbox']);
     }
 
+
+    /**
+     * @dataProvider boolDataProvider
+     * @param bool $allow
+     */
+    public function testPasswordRemovedFromResponseData($allow) {		
+		$form = $this->getStubForm();
+		$form->enableSecurityToken();
+		$form->Fields()->push(
+			new TextField('Username')
+		);
+		$form->Fields()->push(
+			PasswordField::create('Password')
+				->setAllowValuePostback($allow)
+		);
+		$form->Actions()->push(
+			new FormAction('submit')
+		);
+		$form->handleRequest(
+			new SS_HTTPRequest(
+				'POST',
+				'/',
+				array(),
+				array(
+					'Username' => 'uncle',
+					'Password' => 'cheese',
+					'SecurityID' => 'FAIL',
+					'action_submit' => 1
+				)
+			),
+			DataModel::inst()
+		);
+
+		$parser = new CSSContentParser($result = $form->forTemplate());
+        $passwords = $parser->getBySelector('input#Password');
+        $this->assertNotNull($passwords);
+        $this->assertCount(1, $passwords);
+        /* @var \SimpleXMLElement $password */
+        $password = $passwords[0];
+        $attrs = iterator_to_array($password->attributes());
+
+        if ($allow) {
+            $this->assertArrayHasKey('value', $attrs);
+            $this->assertEquals('cheese', $attrs['value']);
+        } else {
+            $this->assertArrayNotHasKey('value', $attrs);
+        }
+
+    }    
+
+
 	protected function getStubForm() {
 		return new Form(
 			new FormTest_Controller(),

tests/forms/PasswordFieldTest.php

@@ -0,0 +1,38 @@
+<?php
+
+
+class PasswordFieldTest extends SapphireTest
+{
+    public function boolDataProvider() {
+        return array(
+            array(false),
+            array(true)
+        );
+    }
+
+    /**
+     * @dataProvider boolDataProvider
+     * @param bool $bool
+     */
+    public function testAutocomplete($bool) {
+        Config::inst()->update('PasswordField', 'autocomplete', $bool);
+        $field = new PasswordField('test');
+        $attrs = $field->getAttributes();
+
+        $this->assertArrayHasKey('autocomplete', $attrs);
+        $this->assertEquals($bool ? 'on' : 'off', $attrs['autocomplete']);
+    }
+
+    /**
+     * @dataProvider boolDataProvider
+     * @param bool $bool
+     */
+    public function testValuePostback($bool) {        
+        $field = PasswordField::create('test', 'test', 'password')
+        	->setAllowValuePostback($bool);
+        $attrs = $field->getAttributes();
+
+        $this->assertArrayHasKey('value', $attrs);
+        $this->assertEquals($bool ? 'password' : '', $attrs['value']);
+    }
+}