Merge pull request #66 from silverstripe-security/pulls/4.0/security-password-fix

SECURITY: Remove password text from session data on failed submission

src/Forms/FormRequestHandler.php

@@ -157,7 +157,6 @@ class FormRequestHandler extends RequestHandler
                         "SilverStripe\\Forms\\Form.CSRF_EXPIRED_MESSAGE",
                         "Your session has expired. Please re-submit the form."
                     ));
-
                 // Return the user
                 return $this->redirectBack();
             }

src/Forms/PasswordField.php

@@ -19,6 +19,12 @@ class PasswordField extends TextField
 
     protected $inputType = 'password';
 
+    /**
+     * If true, the field can accept a value attribute, e.g. from posted form data
+     * @var bool
+     */
+    protected $allowValuePostback = false;
+
     /**
      * Returns an input field.
      *
@@ -39,12 +45,35 @@ 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}
      */
     public function getAttributes()
     {
-        $attributes = array();
+        $attributes = [];
+
+        if (!$this->getAllowValuePostback()) {
+            $attributes['value'] = null;
+        }
 
         $autocomplete = $this->config()->get('autocomplete');
 

tests/php/Forms/FormRequestHandlerTest.php

@@ -9,8 +9,10 @@ use SilverStripe\Dev\SapphireTest;
 use SilverStripe\Forms\FieldList;
 use SilverStripe\Forms\FormAction;
 use SilverStripe\Forms\FormRequestHandler;
+use SilverStripe\Forms\PasswordField;
 use SilverStripe\Forms\Tests\FormRequestHandlerTest\TestForm;
 use SilverStripe\Forms\Tests\FormRequestHandlerTest\TestFormRequestHandler;
+use SilverStripe\Forms\TextField;
 
 /**
  * @skipUpgrade

tests/php/Forms/FormTest.php

@@ -3,6 +3,8 @@
 namespace SilverStripe\Forms\Tests;
 
 use SilverStripe\Control\Session;
+use SilverStripe\Core\Config\Config;
+use SilverStripe\Forms\PasswordField;
 use SilverStripe\Forms\Tests\FormTest\TestController;
 use SilverStripe\Forms\Tests\FormTest\ControllerWithSecurityToken;
 use SilverStripe\Forms\Tests\FormTest\ControllerWithStrictPostCheck;
@@ -10,6 +12,7 @@ use SilverStripe\Forms\Tests\FormTest\Player;
 use SilverStripe\Forms\Tests\FormTest\Team;
 use SilverStripe\ORM\ValidationResult;
 use SilverStripe\Security\NullSecurityToken;
+use SilverStripe\Security\Security;
 use SilverStripe\Security\SecurityToken;
 use SilverStripe\Security\RandomGenerator;
 use SilverStripe\Dev\CSSContentParser;
@@ -59,6 +62,17 @@ class FormTest extends FunctionalTest
         );
     }
 
+    /**
+     * @return array
+     */
+    public function boolDataProvider()
+    {
+        return [
+            [false],
+            [true],
+        ];
+    }
+
     public function testLoadDataFromRequest()
     {
         $form = new Form(
@@ -915,6 +929,46 @@ class FormTest extends FunctionalTest
         $this->assertEmpty($formData['ExtraFieldCheckbox']);
     }
 
+    /**
+     * @dataProvider boolDataProvider
+     * @param bool $allow
+     */
+    public function testPasswordPostback($allow)
+    {
+        $form = $this->getStubForm();
+        $form->enableSecurityToken();
+        $form->Fields()->push(
+            PasswordField::create('Password')
+                ->setAllowValuePostback($allow)
+        );
+        $form->Actions()->push(FormAction::create('doSubmit'));
+        $request = new HTTPRequest(
+            'POST',
+            'FormTest_Controller/Form',
+            [],
+            [
+                'key1' => 'foo',
+                'Password' => 'hidden',
+                SecurityToken::inst()->getName() => 'fail',
+                'action_doSubmit' => 1,
+            ]
+        );
+        $form->getRequestHandler()->httpSubmission($request);
+        $parser = new CSSContentParser($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('hidden', $attrs['value']);
+        } else {
+            $this->assertArrayNotHasKey('value', $attrs);
+        }
+    }
+
     protected function getStubForm()
     {
         return new Form(

tests/php/Forms/PasswordFieldTest.php

@@ -0,0 +1,46 @@
+<?php
+
+namespace SilverStripe\Forms\Tests;
+
+use SilverStripe\Core\Config\Config;
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Forms\PasswordField;
+
+class PasswordFieldTest extends SapphireTest
+{
+    public function boolDataProvider()
+    {
+        return [
+            [false],
+            [true]
+        ];
+    }
+
+    /**
+     * @dataProvider boolDataProvider
+     * @param bool $bool
+     */
+    public function testAutocomplete($bool)
+    {
+        Config::modify()->set(PasswordField::class, '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 = (new PasswordField('test', 'test', 'password'))
+            ->setAllowValuePostback($bool);
+        $attrs = $field->getAttributes();
+
+        $this->assertArrayHasKey('value', $attrs);
+        $this->assertEquals($bool ? 'password' : '', $attrs['value']);
+    }
+}