From 1661213e5b9ff478e19b024f5a599f50e86f2473 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 | 12 +++---- forms/Form.php | 32 ++++++++++------- forms/FormField.php | 9 +++-- forms/gridfield/GridFieldDetailForm.php | 18 +++++----- security/MemberLoginForm.php | 5 ++- tests/forms/FormTest.php | 46 +++++++++++++++++++++++++ tests/security/SecurityTest.php | 4 +-- 7 files changed, 90 insertions(+), 36 deletions(-) diff --git a/forms/ComplexTableField.php b/forms/ComplexTableField.php index 147674aa5..44c73c3d6 100644 --- a/forms/ComplexTableField.php +++ b/forms/ComplexTableField.php @@ -478,7 +478,7 @@ JS; try { $childData->write(); } catch(ValidationException $e) { - $form->sessionMessage($e->getResult()->message(), 'bad'); + $form->sessionMessage($e->getResult()->message(), 'bad', false); return Controller::curr()->redirectBack(); } @@ -493,16 +493,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 = _t( 'ComplexTableField.SUCCESSADD2', 'Added {name}', array('name' => $childData->singular_name()) ); - $message .= '' . $childData->Title . '' . $closeLink; - - $form->sessionMessage($message, 'good'); + $message .= '' . Convert::raw2xml($childData->Title) . '' . $closeLink; + $form->sessionMessage($message, 'good', false); + return Controller::curr()->redirectBack(); } } @@ -671,7 +671,7 @@ class ComplexTableField_ItemRequest extends TableListField_ItemRequest { $closeLink ); - $form->sessionMessage($message, 'good'); + $form->sessionMessage($message, 'good', false); return Controller::curr()->redirectBack(); } diff --git a/forms/Form.php b/forms/Form.php index 9f3e6361f..08827275d 100644 --- a/forms/Form.php +++ b/forms/Form.php @@ -144,10 +144,6 @@ class Form extends RequestHandler { */ protected $attributes = array(); - public static $casting = array( - 'Message' => 'Text' - ); - /** * Create a new form, with the given fields an action buttons. * @@ -446,10 +442,10 @@ class Form extends RequestHandler { * Add a plain text error message to a field on this form. It will be saved into the session * and used the next time this form is displayed. */ - public function addErrorMessage($fieldName, $message, $messageType) { + public 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, )); } @@ -1015,9 +1011,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. */ - public function setMessage($message, $type) { - $this->message = $message; + public function setMessage($message, $type, $escapeHtml = true) { + $this->message = ($escapeHtml) ? Convert::raw2xml($message) : $message; $this->messageType = $type; return $this; } @@ -1027,14 +1026,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. */ - public function sessionMessage($message, $type) { - Session::set("FormInfo.{$this->FormName()}.formError.message", $message); + public 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); } - public static function messageForForm( $formName, $message, $type ) { - Session::set("FormInfo.{$formName}.formError.message", $message); + public 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 e0468164f..ee825cb0e 100644 --- a/forms/FormField.php +++ b/forms/FormField.php @@ -93,10 +93,6 @@ class FormField extends RequestHandler { */ protected $attributes = array(); - public static $casting = array( - 'Message' => 'Text' - ); - /** * Takes a fieldname and converts camelcase to spaced * words. Also resolves combined fieldnames with dot syntax @@ -471,7 +467,10 @@ 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. */ public function setError($message, $messageType) { $this->message = $message; diff --git a/forms/gridfield/GridFieldDetailForm.php b/forms/gridfield/GridFieldDetailForm.php index 90bf9e077..d2515b62c 100644 --- a/forms/gridfield/GridFieldDetailForm.php +++ b/forms/gridfield/GridFieldDetailForm.php @@ -395,7 +395,7 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler { $this->record->write(); $this->gridField->getList()->add($this->record); } catch(ValidationException $e) { - $form->sessionMessage($e->getResult()->message(), 'bad'); + $form->sessionMessage($e->getResult()->message(), 'bad', false); $responseNegotiator = new PjaxResponseNegotiator(array( 'CurrentForm' => function() use(&$form) { return $form->forTemplate(); @@ -412,11 +412,9 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler { // TODO Save this item into the given relationship - // TODO Allow HTML in form messages - // $link = '"' - // . htmlspecialchars($this->record->Title, ENT_QUOTES) - // . '"'; - $link = '"' . $this->record->Title . '"'; + $link = '"' + . htmlspecialchars($this->record->Title, ENT_QUOTES) + . '"'; $message = _t( 'GridFieldDetailForm.Saved', 'Saved {name} {link}', @@ -426,7 +424,7 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler { ) ); - $form->sessionMessage($message, 'good'); + $form->sessionMessage($message, 'good', false); if($new_record) { return Controller::curr()->redirect($this->Link()); @@ -453,7 +451,7 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler { $this->record->delete(); } catch(ValidationException $e) { - $form->sessionMessage($e->getResult()->message(), 'bad'); + $form->sessionMessage($e->getResult()->message(), 'bad', false); return Controller::curr()->redirectBack(); } @@ -466,9 +464,9 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler { $toplevelController = $this->getToplevelController(); if($toplevelController && $toplevelController instanceof LeftAndMain) { $backForm = $toplevelController->getEditForm(); - $backForm->sessionMessage($message, 'good'); + $backForm->sessionMessage($message, 'good', false); } else { - $form->sessionMessage($message, 'good'); + $form->sessionMessage($message, 'good', false); } //when an item is deleted, redirect to the parent controller diff --git a/security/MemberLoginForm.php b/security/MemberLoginForm.php index 5ac7b7d86..40743fb27 100644 --- a/security/MemberLoginForm.php +++ b/security/MemberLoginForm.php @@ -105,7 +105,6 @@ JS * Get message from session */ protected function getMessageFromSession() { - parent::getMessageFromSession(); if(($member = Member::currentUser()) && !Session::get('MemberLoginForm.force_message')) { $this->message = _t( 'Member.LOGGEDINAS', @@ -114,6 +113,10 @@ JS ); } Session::set('MemberLoginForm.force_message', false); + + parent::getMessageFromSession(); + + return $this->message; } diff --git a/tests/forms/FormTest.php b/tests/forms/FormTest.php index dbba1aa03..6d25ca411 100644 --- a/tests/forms/FormTest.php +++ b/tests/forms/FormTest.php @@ -395,6 +395,52 @@ class FormTest extends FunctionalTest { $this->assertNotContains('two="2"', $form->getAttributesHTML('one', 'two')); $this->assertContains('three="3"', $form->getAttributesHTML('one', 'two')); } + + function testMessageEscapeHtml() { + $form = $this->getStubForm(); + $form->Controller()->handleRequest(new SS_HTTPRequest('GET', '/'), DataModel::inst()); // 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', '/'), DataModel::inst()); // 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', '/'), DataModel::inst()); // 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', '/'), DataModel::inst()); // 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( diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php index 3325d09f9..d0e681e9d 100644 --- a/tests/security/SecurityTest.php +++ b/tests/security/SecurityTest.php @@ -260,7 +260,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 */ @@ -301,7 +301,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);