From 17097a4d11274b157eadf64f32708acef204d510 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Thu, 27 Oct 2016 16:03:25 +0100 Subject: [PATCH 1/5] [SS-2016-016] FIX Properly escape backURL for template injection --- security/CMSSecurity.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/CMSSecurity.php b/security/CMSSecurity.php index 545111ed5..e34efb7df 100644 --- a/security/CMSSecurity.php +++ b/security/CMSSecurity.php @@ -204,7 +204,7 @@ PHP '

Login success. If you are not automatically redirected '. 'click here

', 'Login message displayed in the cms popup once a user has re-authenticated themselves', - array('link' => $backURL) + array('link' => Convert::raw2att($backURL)) ) )); From 61e4055bdb13e37df6aa0d8edca0bf5d9345dc7e Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Fri, 11 Nov 2016 15:00:15 +0000 Subject: [PATCH 2/5] [SS-2016-010] FIX Cast FormField values as Text to prevent readonly fields embeding rogue HTML --- forms/FormField.php | 8 ++++++++ forms/HtmlEditorField.php | 8 ++++++++ forms/ReadonlyField.php | 16 +++++++++++++++- forms/TextareaField.php | 7 ------- tests/forms/TextareaFieldTest.php | 10 ---------- 5 files changed, 31 insertions(+), 18 deletions(-) diff --git a/forms/FormField.php b/forms/FormField.php index 0d0ebe525..787569cb1 100644 --- a/forms/FormField.php +++ b/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. diff --git a/forms/HtmlEditorField.php b/forms/HtmlEditorField.php index 246603e83..11faa5ac1 100644 --- a/forms/HtmlEditorField.php +++ b/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; /** diff --git a/forms/ReadonlyField.php b/forms/ReadonlyField.php index a2bd5888b..87aa37e63 100644 --- a/forms/ReadonlyField.php +++ b/forms/ReadonlyField.php @@ -49,10 +49,24 @@ 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 '(' . _t('FormField.NONE', 'none') . ')'; } + /** + * 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 ($field == 'Value' && $this->dontEscape) { + return 'HTMLText'; + } + return parent::castingHelper($field); + } + public function getAttributes() { return array_merge( parent::getAttributes(), diff --git a/forms/TextareaField.php b/forms/TextareaField.php index f907d21fc..d40edee8c 100644 --- a/forms/TextareaField.php +++ b/forms/TextareaField.php @@ -85,11 +85,4 @@ class TextareaField extends FormField { return $parent; } - - /** - * @return string - */ - public function Value() { - return htmlentities($this->value, ENT_COMPAT, 'UTF-8'); - } } diff --git a/tests/forms/TextareaFieldTest.php b/tests/forms/TextareaFieldTest.php index 1847754fc..103fa269e 100644 --- a/tests/forms/TextareaFieldTest.php +++ b/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. */ From 4440b887304fe80ca77366800457cbc2ac705654 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Fri, 11 Nov 2016 15:36:56 +0000 Subject: [PATCH 3/5] [SS-2016-010] FIX Form@httpSubmission will no longer load submitted data to disabled or readonly fields --- forms/Form.php | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/forms/Form.php b/forms/Form.php index 769e6ece6..e21d9a6a0 100644 --- a/forms/Form.php +++ b/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(); From 6136cf85023a4ca50a1e19c7c5ff498cb890bd58 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Mon, 14 Nov 2016 19:02:56 +0000 Subject: [PATCH 4/5] DOCS Update PHPDoc for SS_HTTPResponse --- control/HTTPResponse.php | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/control/HTTPResponse.php b/control/HTTPResponse.php index c952c9306..0b7f4c06f 100644 --- a/control/HTTPResponse.php +++ b/control/HTTPResponse.php @@ -66,12 +66,12 @@ class SS_HTTPResponse { ); /** - * @var Int + * @var int */ protected $statusCode = 200; /** - * @var String + * @var string */ protected $statusDescription = "OK"; @@ -93,9 +93,9 @@ class SS_HTTPResponse { /** * Create a new HTTP response * - * @param $body The body of the response - * @param $statusCode The numeric status code - 200, 404, etc - * @param $statusDescription The text to be given alongside the status code. + * @param string $body The body of the response + * @param int $statusCode The numeric status code - 200, 404, etc + * @param string $statusDescription The text to be given alongside the status code. * See {@link setStatusCode()} for more information. */ public function __construct($body = null, $statusCode = null, $statusDescription = null) { @@ -104,12 +104,12 @@ class SS_HTTPResponse { } /** - * @param String $code - * @param String $description Optional. See {@link setStatusDescription()}. + * @param int $code + * @param string $description Optional. See {@link setStatusDescription()}. * No newlines are allowed in the description. * If omitted, will default to the standard HTTP description * for the given $code value (see {@link $status_codes}). - * @return SS_HTTPRequest $this + * @return $this */ public function setStatusCode($code, $description = null) { if(isset(self::$status_codes[$code])) $this->statusCode = $code; @@ -124,8 +124,8 @@ class SS_HTTPResponse { * The text to be given alongside the status code ("reason phrase"). * Caution: Will be overwritten by {@link setStatusCode()}. * - * @param String $description - * @return SS_HTTPRequest $this + * @param string $description + * @return $this */ public function setStatusDescription($description) { $this->statusDescription = $description; @@ -133,7 +133,7 @@ class SS_HTTPResponse { } /** - * @return Int + * @return int */ public function getStatusCode() { return $this->statusCode; @@ -157,7 +157,7 @@ class SS_HTTPResponse { /** * @param string $body - * @return SS_HTTPRequest $this + * @return $this */ public function setBody($body) { $this->body = $body ? (string) $body : $body; // Don't type-cast false-ish values, eg null is null not '' @@ -176,7 +176,7 @@ class SS_HTTPResponse { * * @param string $header Example: "Content-Type" * @param string $value Example: "text/xml" - * @return SS_HTTPRequest $this + * @return $this */ public function addHeader($header, $value) { $this->headers[$header] = $value; @@ -206,7 +206,7 @@ class SS_HTTPResponse { * e.g. "Content-Type". * * @param string $header - * @return SS_HTTPRequest $this + * @return $this */ public function removeHeader($header) { if(isset($this->headers[$header])) unset($this->headers[$header]); @@ -216,7 +216,7 @@ class SS_HTTPResponse { /** * @param string $dest * @param int $code - * @return SS_HTTPRequest $this + * @return $this */ public function redirect($dest, $code=302) { if(!in_array($code, self::$redirect_codes)) $code = 302; @@ -310,12 +310,17 @@ EOT */ class SS_HTTPResponse_Exception extends Exception { + /** + * @var SS_HTTPResponse + */ protected $response; /** * @param string|SS_HTTPResponse body Either the plaintext content of the error message, or an SS_HTTPResponse * object representing it. In either case, the $statusCode and * $statusDescription will be the HTTP status of the resulting response. + * @param int $statusCode + * @param string $statusDescription * @see SS_HTTPResponse::__construct(); */ public function __construct($body = null, $statusCode = null, $statusDescription = null) { From cc9d17063a3ce3e9966ef54b4c0978c2258f0d2f Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 15 Nov 2016 11:55:48 +1300 Subject: [PATCH 5/5] Add tests for FormField submission behaviour Fix ReadonlyField casting with empty values Restore Value() behaviour for TextareaField --- forms/ReadonlyField.php | 12 ++- forms/TextareaField.php | 12 +++ tests/forms/FormTest.php | 154 +++++++++++++++++++++++---------------- 3 files changed, 112 insertions(+), 66 deletions(-) diff --git a/forms/ReadonlyField.php b/forms/ReadonlyField.php index 87aa37e63..ee8231b77 100644 --- a/forms/ReadonlyField.php +++ b/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) { @@ -61,7 +61,11 @@ class ReadonlyField extends FormField { * @return string */ public function castingHelper($field) { - if ($field == 'Value' && $this->dontEscape) { + if ( + (strcasecmp($field, 'Value') === 0) + && ($this->dontEscape || empty($this->value)) + ) { + // Value is either empty, or unescaped return 'HTMLText'; } return parent::castingHelper($field); diff --git a/forms/TextareaField.php b/forms/TextareaField.php index d40edee8c..bbb88c4e2 100644 --- a/forms/TextareaField.php +++ b/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. * @@ -85,4 +90,11 @@ class TextareaField extends FormField { return $parent; } + + /** + * @return string + */ + public function Value() { + return htmlentities($this->value, ENT_COMPAT, 'UTF-8'); + } } diff --git a/tests/forms/FormTest.php b/tests/forms/FormTest.php index c893979cf..f754ae12d 100644 --- a/tests/forms/FormTest.php +++ b/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' => '' + // leaving out "Required" field + ) + ); + + // Number field updates its value + $this->assertContains('getBody()); + + + // Readonly field remains + $this->assertContains( + '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 "_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
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 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();