tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-10-22 14:05:37 +02:00
Code Issues Projects Releases Wiki Activity

FIX Opt-out pf form message escaping (fixes #2796)

Browse Source 
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.
This commit is contained in:
Ingo Schommer 2014-01-31 10:38:34 +13:00 committed by Sean Harvey
parent 0c85680371
commit 1661213e5b
7 changed files with 90 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
forms/ComplexTableField.php
View File

@ -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 .= '<a href="' . $editLink . '">' . $childData->Title . '</a>' . $closeLink;
$form->sessionMessage($message, 'good');
$message .= '<a href="' . $editLink . '">' . Convert::raw2xml($childData->Title) . '</a>' . $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();
}

32
forms/Form.php
View File

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

9
forms/FormField.php
View File

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

18
forms/gridfield/GridFieldDetailForm.php
View File

@ -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 = '<a href="' . $this->Link('edit') . '">"'
// . htmlspecialchars($this->record->Title, ENT_QUOTES)
// . '"</a>';
$link = '"' . $this->record->Title . '"';
$link = '<a href="' . $this->Link('edit') . '">"'
. htmlspecialchars($this->record->Title, ENT_QUOTES)
. '"</a>';
$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

5
security/MemberLoginForm.php
View File

@ -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;
}

46
tests/forms/FormTest.php
View File

@ -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('<em>Escaped HTML</em>', 'good', true);
$parser = new CSSContentParser($form->forTemplate());
$messageEls = $parser->getBySelector('.message');
$this->assertContains(
'&lt;em&gt;Escaped HTML&lt;/em&gt;',
$messageEls[0]->asXML()
);
$form = $this->getStubForm();
$form->Controller()->handleRequest(new SS_HTTPRequest('GET', '/'), DataModel::inst()); // stub out request
$form->sessionMessage('<em>Unescaped HTML</em>', 'good', false);
$parser = new CSSContentParser($form->forTemplate());
$messageEls = $parser->getBySelector('.message');
$this->assertContains(
'<em>Unescaped HTML</em>',
$messageEls[0]->asXML()
);
}
function testFieldMessageEscapeHtml() {
$form = $this->getStubForm();
$form->Controller()->handleRequest(new SS_HTTPRequest('GET', '/'), DataModel::inst()); // stub out request
$form->addErrorMessage('key1', '<em>Escaped HTML</em>', 'good', true);
$form->setupFormErrors();
$parser = new CSSContentParser($form->forTemplate());
$messageEls = $parser->getBySelector('#key1 .message');
$this->assertContains(
'&lt;em&gt;Escaped HTML&lt;/em&gt;',
$messageEls[0]->asXML()
);
$form = $this->getStubForm();
$form->Controller()->handleRequest(new SS_HTTPRequest('GET', '/'), DataModel::inst()); // stub out request
$form->addErrorMessage('key1', '<em>Unescaped HTML</em>', 'good', false);
$form->setupFormErrors();
$parser = new CSSContentParser($form->forTemplate());
$messageEls = $parser->getBySelector('#key1 .message');
$this->assertContains(
'<em>Unescaped HTML</em>',
$messageEls[0]->asXML()
);
}
protected function getStubForm() {
return new Form(

4
tests/security/SecurityTest.php
View File

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