From 0bae1826bbf1c2abb1d0ee89efb5532d5e7b2e1c Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Fri, 31 Jan 2014 10:38:34 +1300 Subject: [PATCH] FIX Opt-out pf form message escaping (fixes #2796) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a limitation introduced through http://www.silverstripe.org/ss-2013-008-xss-in-numericfield-validation/. Form messages used to accept HTML, now they’re escaped by default, effectively removing the ability to pass in HTML and take care of escaping manually. We pass through HTML to message in core through the CTF system, so this needs to be fixed. It’s an alternative fix to https://github.com/silverstripe/silverstripe-framework/pull/2803. --- forms/ComplexTableField.php | 10 +++---- forms/Form.php | 34 ++++++++++++++--------- forms/FormField.php | 11 ++++---- security/MemberLoginForm.php | 10 +++++-- tests/forms/FormTest.php | 48 ++++++++++++++++++++++++++++++++- tests/security/SecurityTest.php | 4 +-- 6 files changed, 88 insertions(+), 29 deletions(-) diff --git a/forms/ComplexTableField.php b/forms/ComplexTableField.php index 8c7aa8fb3..64a63975d 100755 --- a/forms/ComplexTableField.php +++ b/forms/ComplexTableField.php @@ -642,7 +642,7 @@ JS; try { $childData->write(); } catch(ValidationException $e) { - $form->sessionMessage($e->getResult()->message(), 'bad'); + $form->sessionMessage($e->getResult()->message(), 'bad', false); return Director::redirectBack(); } @@ -670,16 +670,16 @@ JS; _t('ComplexTableField.CLOSEPOPUP', 'Close Popup') ); - $editLink = Controller::join_links($this->Link(), 'item/' . $childData->ID . '/edit'); + $editLink = Controller::join_links($this->Link(), 'item/' . (int)$childData->ID . '/edit'); $message = sprintf( _t('ComplexTableField.SUCCESSADD', 'Added %s %s %s'), $childData->singular_name(), - '' . $childData->Title . '', + '' . Convert::raw2xml($childData->Title) . '', $closeLink ); - $form->sessionMessage($message, 'good'); + $form->sessionMessage($message, 'good', false); Director::redirectBack(); } @@ -851,7 +851,7 @@ class ComplexTableField_ItemRequest extends TableListField_ItemRequest { $closeLink ); - $form->sessionMessage($message, 'good'); + $form->sessionMessage($message, 'good', false); Director::redirectBack(); } diff --git a/forms/Form.php b/forms/Form.php index f457352ad..86a8cd3b8 100755 --- a/forms/Form.php +++ b/forms/Form.php @@ -137,10 +137,6 @@ class Form extends RequestHandler { */ protected $extraClasses = array(); - public static $casting = array( - 'Message' => 'Text' - ); - /** * Create a new form, with the given fields an action buttons. * @@ -393,10 +389,10 @@ class Form extends RequestHandler { * Add an error message to a field on this form. It will be saved into the session * and used the next time this form is displayed. */ - function addErrorMessage($fieldName, $message, $messageType) { + function addErrorMessage($fieldName, $message, $messageType, $escapeHtml = true) { Session::add_to_array("FormInfo.{$this->FormName()}.errors", array( 'fieldName' => $fieldName, - 'message' => $message, + 'message' => $escapeHtml ? Convert::raw2xml($message) : $message, 'messageType' => $messageType, )); } @@ -799,6 +795,7 @@ class Form extends RequestHandler { $this->getMessageFromSession(); $message = $this->message; $this->clearMessage(); + return $message; } @@ -816,7 +813,6 @@ class Form extends RequestHandler { }else{ $this->message = Session::get("FormInfo.{$this->FormName()}.formError.message"); $this->messageType = Session::get("FormInfo.{$this->FormName()}.formError.type"); - Session::clear("FormInfo.{$this->FormName()}"); } } @@ -826,9 +822,12 @@ class Form extends RequestHandler { * * @param message the text of the message * @param type Should be set to good, bad, or warning. + * @param boolean $escapeHtml Automatically sanitize the message. Set to FALSE if the message contains HTML. + * In that case, you might want to use {@link Convert::raw2xml()} to escape any + * user supplied data in the message. */ - function setMessage($message, $type) { - $this->message = $message; + function setMessage($message, $type, $escapeHtml = true) { + $this->message = ($escapeHtml) ? Convert::raw2xml($message) : $message; $this->messageType = $type; } @@ -837,14 +836,23 @@ class Form extends RequestHandler { * * @param message the text of the message * @param type Should be set to good, bad, or warning. + * @param boolean $escapeHtml Automatically sanitize the message. Set to FALSE if the message contains HTML. + * In that case, you might want to use {@link Convert::raw2xml()} to escape any + * user supplied data in the message. */ - function sessionMessage($message, $type) { - Session::set("FormInfo.{$this->FormName()}.formError.message", $message); + function sessionMessage($message, $type, $escapeHtml = true) { + Session::set( + "FormInfo.{$this->FormName()}.formError.message", + $escapeHtml ? Convert::raw2xml($message) : $message + ); Session::set("FormInfo.{$this->FormName()}.formError.type", $type); } - static function messageForForm( $formName, $message, $type ) { - Session::set("FormInfo.{$formName}.formError.message", $message); + static function messageForForm( $formName, $message, $type, $escapeHtml = true) { + Session::set( + "FormInfo.{$formName}.formError.message", + $escapeHtml ? Convert::raw2xml($message) : $message + ); Session::set("FormInfo.{$formName}.formError.type", $type); } diff --git a/forms/FormField.php b/forms/FormField.php index cc62c3748..7d41c162f 100755 --- a/forms/FormField.php +++ b/forms/FormField.php @@ -72,10 +72,6 @@ class FormField extends RequestHandler { * @var Custom Validation Message for the Field */ protected $customValidationMessage = ""; - - public static $casting = array( - 'Message' => 'Text' - ); /** * Create a new field. @@ -330,9 +326,12 @@ class FormField extends RequestHandler { /** * Sets the error message to be displayed on the form field - * Set by php validation of the form + * Set by php validation of the form. + * + * @param string $message Message to show to the user. Allows HTML content, + * which means you need to use Convert::raw2xml() for any user supplied data. */ - function setError($message,$messageType){ + function setError($message, $messageType){ $this->message = $message; $this->messageType = $messageType; } diff --git a/security/MemberLoginForm.php b/security/MemberLoginForm.php index 6e67e1dd4..b0bb75f2c 100644 --- a/security/MemberLoginForm.php +++ b/security/MemberLoginForm.php @@ -106,11 +106,17 @@ JS * Get message from session */ protected function getMessageFromSession() { - parent::getMessageFromSession(); if(($member = Member::currentUser()) && !Session::get('MemberLoginForm.force_message')) { - $this->message = sprintf(_t('Member.LOGGEDINAS', "You're logged in as %s."), $member->{$this->loggedInAsField}); + $this->sessionMessage( + sprintf(_t('Member.LOGGEDINAS', "You're logged in as %s."), $member->{$this->loggedInAsField}), + 'notice' + ); } Session::set('MemberLoginForm.force_message', false); + + parent::getMessageFromSession(); + + return $this->message; } diff --git a/tests/forms/FormTest.php b/tests/forms/FormTest.php index e45aba6e4..118b8649a 100755 --- a/tests/forms/FormTest.php +++ b/tests/forms/FormTest.php @@ -324,10 +324,56 @@ class FormTest extends FunctionalTest { SecurityToken::disable(); // restore original } + + function testMessageEscapeHtml() { + $form = $this->getStubForm(); + $form->Controller()->handleRequest(new SS_HTTPRequest('GET', '/')); // stub out request + $form->sessionMessage('Escaped HTML', 'good', true); + $parser = new CSSContentParser($form->forTemplate()); + $messageEls = $parser->getBySelector('.message'); + $this->assertContains( + '<em>Escaped HTML</em>', + $messageEls[0]->asXML() + ); + + $form = $this->getStubForm(); + $form->Controller()->handleRequest(new SS_HTTPRequest('GET', '/')); // stub out request + $form->sessionMessage('Unescaped HTML', 'good', false); + $parser = new CSSContentParser($form->forTemplate()); + $messageEls = $parser->getBySelector('.message'); + $this->assertContains( + 'Unescaped HTML', + $messageEls[0]->asXML() + ); + } + + function testFieldMessageEscapeHtml() { + $form = $this->getStubForm(); + $form->Controller()->handleRequest(new SS_HTTPRequest('GET', '/')); // stub out request + $form->addErrorMessage('key1', 'Escaped HTML', 'good', true); + $form->setupFormErrors(); + $parser = new CSSContentParser($form->forTemplate()); + $messageEls = $parser->getBySelector('#key1 .message'); + $this->assertContains( + '<em>Escaped HTML</em>', + $messageEls[0]->asXML() + ); + + $form = $this->getStubForm(); + $form->Controller()->handleRequest(new SS_HTTPRequest('GET', '/')); // stub out request + $form->addErrorMessage('key1', 'Unescaped HTML', 'good', false); + $form->setupFormErrors(); + $parser = new CSSContentParser($form->forTemplate()); + $messageEls = $parser->getBySelector('#key1 .message'); + $this->assertContains( + 'Unescaped HTML', + $messageEls[0]->asXML() + ); + } protected function getStubForm() { return new Form( - new Controller(), + new FormTest_Controller(), 'Form', new FieldSet(new TextField('key1')), new FieldSet() diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php index c8bdbe221..055fd26b4 100644 --- a/tests/security/SecurityTest.php +++ b/tests/security/SecurityTest.php @@ -231,7 +231,7 @@ class SecurityTest extends FunctionalTest { /* THE FIRST 4 TIMES, THE MEMBER SHOULDN'T BE LOCKED OUT */ if($i < 5) { $this->assertNull($member->LockedOutUntil); - $this->assertContains($this->loginErrorMessage(), _t('Member.ERRORWRONGCRED')); + $this->assertContains($this->loginErrorMessage(), Convert::raw2xml(_t('Member.ERRORWRONGCRED'))); } /* AFTER THAT THE USER IS LOCKED OUT FOR 15 MINUTES */ @@ -272,7 +272,7 @@ class SecurityTest extends FunctionalTest { $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); $this->assertNull($this->session()->inst_get('loggedInAs')); - $this->assertTrue(false !== stripos($this->loginErrorMessage(), _t('Member.ERRORWRONGCRED'))); + $this->assertContains($this->loginErrorMessage(), Convert::raw2xml(_t('Member.ERRORWRONGCRED'))); $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); $this->assertEquals($this->session()->inst_get('loggedInAs'), $member->ID);