Merge pull request #29 from silverstripe-security/patch/3.1/SS-2016-010

[SS-2016-010] Prevent readonly fields loading submitted data

forms/Form.php

@@ -319,8 +319,21 @@ class Form extends RequestHandler {
 			$vars = $request->requestVars();
 		}
 
+		// construct an array of allowed fields that can be populated from request data.
+		// readonly or disabled fields should not be loading data from requests
+		$allowedFields = array();
+		$dataFields = $this->Fields()->dataFields();
+		if ($dataFields) {
+			/** @var FormField $field */
+			foreach ($this->Fields()->dataFields() as $name => $field) {
+				if (!$field->isReadonly() && !$field->isDisabled()) {
+					$allowedFields[] = $name;
+				}
+			}
+		}
+
 		// Populate the form
-		$this->loadDataFrom($vars, true);
+		$this->loadDataFrom($vars, true, $allowedFields);
 
 		// Protection against CSRF attacks
 		$token = $this->getSecurityToken();

forms/FormField.php

@@ -157,6 +157,14 @@ class FormField extends RequestHandler {
 	 */
 	protected $attributes = array();
 
+	/**
+	 * @config
+	 * @var array
+	 */
+	private static $casting = array(
+		'Value' => 'Text',
+	);
+
 	/**
 	 * Takes a field name and converts camelcase to spaced words. Also resolves combined field
 	 * names with dot syntax to spaced words.

forms/HtmlEditorField.php

@@ -26,6 +26,14 @@ class HtmlEditorField extends TextareaField {
 	 */
 	private static $sanitise_server_side = false;
 
+	/**
+	 * @config
+	 * @var array
+	 */
+	private static $casting = array(
+		'Value' => 'HTMLText',
+	);
+
 	protected $rows = 30;
 	
 	/**

forms/ReadonlyField.php

@@ -3,7 +3,7 @@
  * Read-only field to display a non-editable value with a label.
  * Consider using an {@link LabelField} if you just need a label-less
  * value display.
- * 
+ *
  * @package forms
  * @subpackage fields-basic
  */
@@ -19,13 +19,13 @@ class ReadonlyField extends FormField {
 
 	/**
 	 * If true, a hidden field will be included in the HTML for the readonly field.
-	 * 
+	 *
 	 * This can be useful if you need to pass the data through on the form submission, as
 	 * long as it's okay than an attacker could change the data before it's submitted.
 	 *
 	 * This is disabled by default as it can introduce security holes if the data is not
 	 * allowed to be modified by the user.
-	 * 
+	 *
 	 * @param boolean $includeHiddenField
 	 */
 	public function setIncludeHiddenField($includeHiddenField) {
@@ -49,10 +49,28 @@ class ReadonlyField extends FormField {
 	}
 
 	public function Value() {
-		if($this->value) return $this->dontEscape ? $this->value : Convert::raw2xml($this->value);
+		if($this->value) return $this->value;
 		else return '<i>(' . _t('FormField.NONE', 'none') . ')</i>';
 	}
 
+	/**
+	 * This is a legacy fix to ensure that the `dontEscape` flag has an impact on readonly fields
+	 * now that we've moved to casting template values more rigidly
+	 *
+	 * @param string $field
+	 * @return string
+	 */
+	public function castingHelper($field) {
+		if (
+			(strcasecmp($field, 'Value') === 0)
+			&& ($this->dontEscape || empty($this->value))
+		) {
+			// Value is either empty, or unescaped
+			return 'HTMLText';
+		}
+		return parent::castingHelper($field);
+	}
+
 	public function getAttributes() {
 		return array_merge(
 			parent::getAttributes(),

forms/TextareaField.php

@@ -18,6 +18,11 @@
  * @subpackage fields-basic
  */
 class TextareaField extends FormField {
+
+	private static $casting = array(
+		'Value' => 'HTMLText',
+	);
+
 	/**
 	 * Visible number of text lines.
 	 *

tests/forms/FormTest.php

@@ -4,14 +4,14 @@
  * @subpackage tests
  */
 class FormTest extends FunctionalTest {
-	
+
 	protected static $fixture_file = 'FormTest.yml';
 
 	protected $extraDataObjects = array(
 		'FormTest_Player',
 		'FormTest_Team',
 	);
-	
+
 	public function testLoadDataFromRequest() {
 		$form = new Form(
 			new Controller(),
@@ -24,7 +24,7 @@ class FormTest extends FunctionalTest {
 			),
 			new FieldList()
 		);
-		
+
 		// url would be ?key1=val1&namespace[key2]=val2&namespace[key3][key4]=val4&othernamespace[key5][key6][key7]=val7
 		$requestData = array(
 			'key1' => 'val1',
@@ -42,16 +42,43 @@ class FormTest extends FunctionalTest {
 				)
 			)
 		);
-		
+
 		$form->loadDataFrom($requestData);
-		
+
 		$fields = $form->Fields();
 		$this->assertEquals($fields->fieldByName('key1')->Value(), 'val1');
 		$this->assertEquals($fields->fieldByName('namespace[key2]')->Value(), 'val2');
 		$this->assertEquals($fields->fieldByName('namespace[key3][key4]')->Value(), 'val4');
 		$this->assertEquals($fields->fieldByName('othernamespace[key5][key6][key7]')->Value(), 'val7');
 	}
-	
+
+	public function testSubmitReadonlyFields() {
+		$this->get('FormTest_Controller');
+
+		// Submitting a value for a readonly field should be ignored
+		$response = $this->post(
+			'FormTest_Controller/Form',
+			array(
+				'Email' => 'invalid',
+				'Number' => '888',
+				'ReadonlyField' => '<script>alert("hacxzored")</script>'
+				// leaving out "Required" field
+			)
+		);
+
+		// Number field updates its value
+		$this->assertContains('<input type="text" name="Number" value="888"', $response->getBody());
+
+
+		// Readonly field remains
+		$this->assertContains(
+			'<input type="text" name="ReadonlyField" value="This value is readonly"',
+			$response->getBody()
+		);
+
+		$this->assertNotContains('hacxzored', $response->getBody());
+	}
+
 	public function testLoadDataFromUnchangedHandling() {
 		$form = new Form(
 			new Controller(),
@@ -68,7 +95,7 @@ class FormTest extends FunctionalTest {
 			'key2_unchanged' => '1'
 		));
 		$this->assertEquals(
-			$form->getData(), 
+			$form->getData(),
 			array(
 				'key1' => 'save',
 				'key2' => null,
@@ -76,7 +103,7 @@ class FormTest extends FunctionalTest {
 			'loadDataFrom() doesnt save a field if a matching "<fieldname>_unchanged" flag is set'
 		);
 	}
-	
+
 	public function testLoadDataFromObject() {
 		$form = new Form(
 			new Controller(),
@@ -90,16 +117,16 @@ class FormTest extends FunctionalTest {
 			),
 			new FieldList()
 		);
-		
+
 		$captainWithDetails = $this->objFromFixture('FormTest_Player', 'captainWithDetails');
 		$form->loadDataFrom($captainWithDetails);
 		$this->assertEquals(
-			$form->getData(), 
+			$form->getData(),
 			array(
 				'Name' => 'Captain Details',
 				'Biography' => 'Bio 1',
-				'Birthday' => '1982-01-01', 
-				'BirthdayYear' => '1982', 
+				'Birthday' => '1982-01-01',
+				'BirthdayYear' => '1982',
 			),
 			'LoadDataFrom() loads simple fields and dynamic getters'
 		);
@@ -107,17 +134,17 @@ class FormTest extends FunctionalTest {
 		$captainNoDetails = $this->objFromFixture('FormTest_Player', 'captainNoDetails');
 		$form->loadDataFrom($captainNoDetails);
 		$this->assertEquals(
-			$form->getData(), 
+			$form->getData(),
 			array(
 				'Name' => 'Captain No Details',
 				'Biography' => null,
-				'Birthday' => null, 
-				'BirthdayYear' => 0, 
+				'Birthday' => null,
+				'BirthdayYear' => 0,
 			),
 			'LoadNonBlankDataFrom() loads only fields with values, and doesnt overwrite existing values'
 		);
 	}
-	
+
 	public function testLoadDataFromClearMissingFields() {
 		$form = new Form(
 			new Controller(),
@@ -134,33 +161,33 @@ class FormTest extends FunctionalTest {
 			new FieldList()
 		);
 		$unrelatedField->setValue("random value");
-		
+
 		$captainWithDetails = $this->objFromFixture('FormTest_Player', 'captainWithDetails');
 		$captainNoDetails = $this->objFromFixture('FormTest_Player', 'captainNoDetails');
 		$form->loadDataFrom($captainWithDetails);
 		$this->assertEquals(
-			$form->getData(), 
+			$form->getData(),
 			array(
 				'Name' => 'Captain Details',
 				'Biography' => 'Bio 1',
-				'Birthday' => '1982-01-01', 
+				'Birthday' => '1982-01-01',
 				'BirthdayYear' => '1982',
 				'UnrelatedFormField' => 'random value',
 			),
 			'LoadDataFrom() doesnt overwrite fields not found in the object'
 		);
-		
+
 		$captainWithDetails = $this->objFromFixture('FormTest_Player', 'captainNoDetails');
 		$team2 = $this->objFromFixture('FormTest_Team', 'team2');
 		$form->loadDataFrom($captainWithDetails);
 		$form->loadDataFrom($team2, Form::MERGE_CLEAR_MISSING);
 		$this->assertEquals(
-			$form->getData(), 
+			$form->getData(),
 			array(
 				'Name' => 'Team 2',
 				'Biography' => '',
-				'Birthday' => '', 
-				'BirthdayYear' => 0, 
+				'Birthday' => '',
+				'BirthdayYear' => 0,
 				'UnrelatedFormField' => null,
 			),
 			'LoadDataFrom() overwrites fields not found in the object with $clearMissingFields=true'
@@ -199,7 +226,7 @@ class FormTest extends FunctionalTest {
 		$form = $this->getStubForm();
 		$form->setFormMethod('GET');
 		$this->assertNull($form->Fields()->dataFieldByName('_method'));
-		
+
 		$form = $this->getStubForm();
 		$form->setFormMethod('PUT');
 		$this->assertEquals($form->Fields()->dataFieldByName('_method')->Value(), 'put',
@@ -208,7 +235,7 @@ class FormTest extends FunctionalTest {
 		$this->assertEquals($form->FormMethod(), 'post',
 			'PUT override in forms has POST in <form> tag'
 		);
-		
+
 		$form = $this->getStubForm();
 		$form->setFormMethod('DELETE');
 		$this->assertEquals($form->Fields()->dataFieldByName('_method')->Value(), 'delete',
@@ -218,10 +245,10 @@ class FormTest extends FunctionalTest {
 			'PUT override in forms has POST in <form> tag'
 		);
 	}
-	
+
 	public function testSessionValidationMessage() {
 		$this->get('FormTest_Controller');
-		
+
 		$response = $this->post(
 			'FormTest_Controller/Form',
 			array(
@@ -256,10 +283,10 @@ class FormTest extends FunctionalTest {
 			"Unsafe content is not emitted directly inside the response body"
 		);
 	}
-	
+
 	public function testSessionSuccessMessage() {
 		$this->get('FormTest_Controller');
-		
+
 		$response = $this->post(
 			'FormTest_Controller/Form',
 			array(
@@ -275,31 +302,31 @@ class FormTest extends FunctionalTest {
 			'Form->sessionMessage() shows up after reloading the form'
 		);
 	}
-	
+
 	public function testGloballyDisabledSecurityTokenInheritsToNewForm() {
 		SecurityToken::enable();
-		
+
 		$form1 = $this->getStubForm();
 		$this->assertInstanceOf('SecurityToken', $form1->getSecurityToken());
-		
+
 		SecurityToken::disable();
-		
+
 		$form2 = $this->getStubForm();
 		$this->assertInstanceOf('NullSecurityToken', $form2->getSecurityToken());
-		
+
 		SecurityToken::enable();
 	}
-	
+
 	public function testDisableSecurityTokenDoesntAddTokenFormField() {
 		SecurityToken::enable();
-		
+
 		$formWithToken = $this->getStubForm();
 		$this->assertInstanceOf(
 			'HiddenField',
 			$formWithToken->Fields()->fieldByName(SecurityToken::get_default_name()),
 			'Token field added by default'
 		);
-		
+
 		$formWithoutToken = $this->getStubForm();
 		$formWithoutToken->disableSecurityToken();
 		$this->assertNull(
@@ -307,11 +334,11 @@ class FormTest extends FunctionalTest {
 			'Token field not added if disableSecurityToken() is set'
 		);
 	}
-	
+
 	public function testDisableSecurityTokenAcceptsSubmissionWithoutToken() {
 		SecurityToken::enable();
 		$expectedToken = SecurityToken::inst()->getValue();
-		
+
 		$response = $this->get('FormTest_ControllerWithSecurityToken');
 		// can't use submitForm() as it'll automatically insert SecurityID into the POST data
 		$response = $this->post(
@@ -356,8 +383,8 @@ class FormTest extends FunctionalTest {
 		$response = $this->get('FormTest_ControllerWithSecurityToken');
 		$tokenEls = $this->cssParser()->getBySelector('#Form_Form_SecurityID');
 		$this->assertEquals(
-			1, 
-			count($tokenEls), 
+			1,
+			count($tokenEls),
 			'Token form field added for controller without disableSecurityToken()'
 		);
 		$token = (string)$tokenEls[0];
@@ -389,24 +416,24 @@ class FormTest extends FunctionalTest {
 		);
 		$this->assertEquals(200, $response->getStatusCode(), 'Submission succeeds with correct method');
 	}
-	
+
 	public function testEnableSecurityToken() {
 		SecurityToken::disable();
 		$form = $this->getStubForm();
 		$this->assertFalse($form->getSecurityToken()->isEnabled());
 		$form->enableSecurityToken();
 		$this->assertTrue($form->getSecurityToken()->isEnabled());
-		
+
 		SecurityToken::disable(); // restore original
 	}
-	
+
 	public function testDisableSecurityToken() {
 		SecurityToken::enable();
 		$form = $this->getStubForm();
 		$this->assertTrue($form->getSecurityToken()->isEnabled());
 		$form->disableSecurityToken();
 		$this->assertFalse($form->getSecurityToken()->isEnabled());
-		
+
 		SecurityToken::disable(); // restore original
 	}
 
@@ -575,7 +602,7 @@ class FormTest extends FunctionalTest {
         $formData = $form->getData();
         $this->assertEmpty($formData['ExtraFieldCheckbox']);
     }
-	
+
 	protected function getStubForm() {
 		return new Form(
 			new FormTest_Controller(),
@@ -584,7 +611,7 @@ class FormTest extends FunctionalTest {
 			new FieldList()
 		);
 	}
-	
+
 }
 
 class FormTest_Player extends DataObject implements TestOnly {
@@ -593,19 +620,19 @@ class FormTest_Player extends DataObject implements TestOnly {
 		'Biography' => 'Text',
 		'Birthday' => 'Date'
 	);
-	
+
 	private static $belongs_many_many = array(
 		'Teams' => 'FormTest_Team'
 	);
-	
+
 	private static $has_one = array(
-		'FavouriteTeam' => 'FormTest_Team', 
+		'FavouriteTeam' => 'FormTest_Team',
 	);
-	
+
 	public function getBirthdayYear() {
 		return ($this->Birthday) ? date('Y', strtotime($this->Birthday)) : null;
 	}
-	
+
 }
 
 class FormTest_Team extends DataObject implements TestOnly {
@@ -613,7 +640,7 @@ class FormTest_Team extends DataObject implements TestOnly {
 		'Name' => 'Varchar',
 		'Region' => 'Varchar',
 	);
-	
+
 	private static $many_many = array(
 		'Players' => 'FormTest_Player'
 	);
@@ -628,12 +655,12 @@ class FormTest_Controller extends Controller implements TestOnly {
 	);
 
 	protected $template = 'BlankPage';
-	
+
 	public function Link($action = null) {
 		return Controller::join_links('FormTest_Controller', $this->request->latestParam('Action'),
 			$this->request->latestParam('ID'), $action);
 	}
-	
+
 	public function Form() {
 		$form = new Form(
 			$this,
@@ -642,7 +669,10 @@ class FormTest_Controller extends Controller implements TestOnly {
 				new EmailField('Email'),
 				new TextField('SomeRequiredField'),
 				new CheckboxSetField('Boxes', null, array('1'=>'one','2'=>'two')),
-				new NumericField('Number')
+				new NumericField('Number'),
+				TextField::create('ReadonlyField')
+					->setReadonly(true)
+					->setValue('This value is readonly')
 			),
 			new FieldList(
 				new FormAction('doSubmit')
@@ -653,10 +683,10 @@ class FormTest_Controller extends Controller implements TestOnly {
 			)
 		);
 		$form->disableSecurityToken(); // Disable CSRF protection for easier form submission handling
-		
+
 		return $form;
 	}
-	
+
 	public function doSubmit($data, $form, $request) {
 		$form->sessionMessage('Test save was successful', 'good');
 		return $this->redirectBack();
@@ -669,7 +699,7 @@ class FormTest_Controller extends Controller implements TestOnly {
 }
 
 class FormTest_ControllerWithSecurityToken extends Controller implements TestOnly {
-	
+
 	private static $allowed_actions = array('Form');
 
 	private static $url_handlers = array(
@@ -677,12 +707,12 @@ class FormTest_ControllerWithSecurityToken extends Controller implements TestOnl
 	);
 
 	protected $template = 'BlankPage';
-	
+
 	public function Link($action = null) {
 		return Controller::join_links('FormTest_ControllerWithSecurityToken', $this->request->latestParam('Action'),
 			$this->request->latestParam('ID'), $action);
 	}
-	
+
 	public function Form() {
 		$form = new Form(
 			$this,
@@ -697,7 +727,7 @@ class FormTest_ControllerWithSecurityToken extends Controller implements TestOnl
 
 		return $form;
 	}
-	
+
 	public function doSubmit($data, $form, $request) {
 		$form->sessionMessage('Test save was successful', 'good');
 		return $this->redirectBack();

tests/forms/TextareaFieldTest.php

@@ -2,16 +2,6 @@
 
 class TextareaFieldTest extends SapphireTest {
 
-	/**
-	 * Quick smoke test to ensure that text is being encoded properly.
-	 */
-	public function testTextEncoding() {
-		$inputText = "These are some unicodes: äöü";
-		$field = new TextareaField("Test", "Test");
-		$field->setValue($inputText);
-		$this->assertContains('These are some unicodes: äöü', $field->Field());
-	}
-
 	/**
 	 * Quick smoke test to ensure that text with unicodes is being displayed properly in readonly fields.
 	 */