From 0c8568037168f95da455d21febcbce1945169064 Mon Sep 17 00:00:00 2001 From: Joel Marcey Date: Mon, 3 Feb 2014 15:32:58 -0800 Subject: [PATCH 1/2] Support PHPUnit 3.8+ compatibility Summary: PHPUnit 3.8+ adds a method to its PHPUnit_Framework_TestListener called addRiskyTest(). Need to stub it out to avoid "must implement this interface method" fatals when using 3.8+ Test Plan: Reviewers: CC: Task ID: # Blame Rev: --- dev/SapphireTestReporter.php | 12 ++++++++++++ dev/SilverStripeListener.php | 12 ++++++++++++ dev/TeamCityListener.php | 12 ++++++++++++ dev/TestListener.php | 12 ++++++++++++ 4 files changed, 48 insertions(+) diff --git a/dev/SapphireTestReporter.php b/dev/SapphireTestReporter.php index 84da76c72..602520d0e 100644 --- a/dev/SapphireTestReporter.php +++ b/dev/SapphireTestReporter.php @@ -300,6 +300,18 @@ class SapphireTestReporter implements PHPUnit_Framework_TestListener { } } + /** + * Risky test. + * + * @param PHPUnit_Framework_Test $test + * @param Exception $e + * @param float $time + * @since Method available since Release 3.8.0 + */ + public function addRiskyTest(PHPUnit_Framework_Test $test, Exception $e, $time) { + // Stub out to support PHPUnit 3.8 + } + /** * Trys to get the original exception thrown by the test on failure/error * to enable us to give a bit more detail about the failure/error diff --git a/dev/SilverStripeListener.php b/dev/SilverStripeListener.php index 009591f82..0d6b7ceb8 100644 --- a/dev/SilverStripeListener.php +++ b/dev/SilverStripeListener.php @@ -42,4 +42,16 @@ class SilverStripeListener implements PHPUnit_Framework_TestListener { public function addSkippedTest(PHPUnit_Framework_Test $test, Exception $e, $time) { } + + /** + * Risky test. + * + * @param PHPUnit_Framework_Test $test + * @param Exception $e + * @param float $time + * @since Method available since Release 3.8.0 + */ + public function addRiskyTest(PHPUnit_Framework_Test $test, Exception $e, $time) { + // Stub out to support PHPUnit 3.8 + } } diff --git a/dev/TeamCityListener.php b/dev/TeamCityListener.php index 7af8a6c52..48e017594 100644 --- a/dev/TeamCityListener.php +++ b/dev/TeamCityListener.php @@ -58,4 +58,16 @@ class TeamCityListener implements PHPUnit_Framework_TestListener { $message = $this->escape($e->getMessage()); echo "##teamcity[testIgnored name='{$class}.{$test->getName()}' message='$message']\n"; } + + /** + * Risky test. + * + * @param PHPUnit_Framework_Test $test + * @param Exception $e + * @param float $time + * @since Method available since Release 3.8.0 + */ + public function addRiskyTest(PHPUnit_Framework_Test $test, Exception $e, $time) { + // Stub out to support PHPUnit 3.8 + } } diff --git a/dev/TestListener.php b/dev/TestListener.php index 5494dfd89..4c0115004 100644 --- a/dev/TestListener.php +++ b/dev/TestListener.php @@ -38,6 +38,18 @@ class SS_TestListener implements PHPUnit_Framework_TestListener { $this->class->tearDownOnce(); } + /** + * Risky test. + * + * @param PHPUnit_Framework_Test $test + * @param Exception $e + * @param float $time + * @since Method available since Release 3.8.0 + */ + public function addRiskyTest(PHPUnit_Framework_Test $test, Exception $e, $time) { + // Stub out to support PHPUnit 3.8 + } + /** * @param String Classname * @return boolean From 1661213e5b9ff478e19b024f5a599f50e86f2473 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Fri, 31 Jan 2014 10:38:34 +1300 Subject: [PATCH 2/2] 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);