diff --git a/dev/SapphireTestReporter.php b/dev/SapphireTestReporter.php index 33a86755b..c30b56757 100644 --- a/dev/SapphireTestReporter.php +++ b/dev/SapphireTestReporter.php @@ -299,7 +299,7 @@ class SapphireTestReporter implements PHPUnit_Framework_TestListener { $this->endCurrentTestSuite(); } } - + /** * Risky test. * diff --git a/dev/TestListener.php b/dev/TestListener.php index c94e42f3b..d423bc06a 100644 --- a/dev/TestListener.php +++ b/dev/TestListener.php @@ -37,7 +37,7 @@ class SS_TestListener implements PHPUnit_Framework_TestListener { $this->class->tearDownOnce(); } - + /** * Risky test. * diff --git a/forms/Form.php b/forms/Form.php index 513f709ac..89b6dcdde 100644 --- a/forms/Form.php +++ b/forms/Form.php @@ -155,10 +155,6 @@ class Form extends RequestHandler { 'forTemplate', ); - private static $casting = array( - 'Message' => 'Text' - ); - /** * Create a new form, with the given fields an action buttons. * @@ -508,10 +504,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, )); } @@ -1035,9 +1031,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; } @@ -1047,14 +1046,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 94b2d5f2f..40fdba1c2 100644 --- a/forms/FormField.php +++ b/forms/FormField.php @@ -93,10 +93,6 @@ class FormField extends RequestHandler { */ protected $attributes = array(); - private static $casting = array( - 'Message' => 'Text' - ); - /** * Takes a fieldname and converts camelcase to spaced * words. Also resolves combined fieldnames with dot syntax @@ -475,7 +471,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 34de0e08c..dcaed3bfc 100644 --- a/forms/gridfield/GridFieldDetailForm.php +++ b/forms/gridfield/GridFieldDetailForm.php @@ -527,7 +527,7 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler { $this->record->write(); $list->add($this->record, $extraData); } 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(); @@ -544,11 +544,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}', @@ -558,7 +556,7 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler { ) ); - $form->sessionMessage($message, 'good'); + $form->sessionMessage($message, 'good', false); if($new_record) { return $controller->redirect($this->Link()); @@ -585,7 +583,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 $this->getToplevelController()->redirectBack(); } @@ -598,9 +596,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 9f6fc6e62..263ea0473 100644 --- a/security/MemberLoginForm.php +++ b/security/MemberLoginForm.php @@ -134,7 +134,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', @@ -143,6 +142,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 5ac98719e..8226f783a 100644 --- a/tests/forms/FormTest.php +++ b/tests/forms/FormTest.php @@ -483,6 +483,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 3d3ce1066..0f0fbc137 100644 --- a/tests/security/SecurityTest.php +++ b/tests/security/SecurityTest.php @@ -299,7 +299,7 @@ class SecurityTest extends FunctionalTest { $member->LockedOutUntil, 'User does not have a lockout time set if under threshold for failed attempts' ); - $this->assertContains($this->loginErrorMessage(), _t('Member.ERRORWRONGCRED')); + $this->assertContains($this->loginErrorMessage(), Convert::raw2xml(_t('Member.ERRORWRONGCRED'))); } else { // Fuzzy matching for time to avoid side effects from slow running tests $this->assertGreaterThan( @@ -337,7 +337,7 @@ class SecurityTest extends FunctionalTest { $member->ID, 'After lockout expires, the user can login again' ); - + // Log the user out $this->session()->inst_set('loggedInAs', null); @@ -346,11 +346,12 @@ class SecurityTest extends FunctionalTest { $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')), 'The user can retry with a wrong password after the lockout expires' ); - + $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); $this->assertEquals( $this->session()->inst_get('loggedInAs'),