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);