From a338e608b8798d1d00d63114d0981bb8c9989160 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Tue, 24 Sep 2013 13:58:32 +0200 Subject: [PATCH 1/3] API Escape form validation messages (SS-2013-008) --- forms/Form.php | 6 +++++- forms/FormField.php | 4 ++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/forms/Form.php b/forms/Form.php index 4dd0f84f2..30186998c 100644 --- a/forms/Form.php +++ b/forms/Form.php @@ -155,6 +155,10 @@ class Form extends RequestHandler { 'forTemplate', ); + private static $casting = array( + 'Message' => 'Text' + ); + /** * Create a new form, with the given fields an action buttons. * @@ -489,7 +493,7 @@ class Form extends RequestHandler { } /** - * Add an error message to a field on this form. It will be saved into the session + * 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) { diff --git a/forms/FormField.php b/forms/FormField.php index 87b73e63c..76c9d5948 100644 --- a/forms/FormField.php +++ b/forms/FormField.php @@ -93,6 +93,10 @@ 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 From d8d07d971e67e5774172791a7687e0b0bff94e4c Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Tue, 24 Sep 2013 12:59:05 +0200 Subject: [PATCH 2/3] FIX Auto-escape titles in TreeDropdownField Related to SS-2013-009. While the default "TreeTitle" was escaped within the SiteTree->TreeTitle() getter, other properties like SiteTree->Title weren't escaped. The new logic uses the underlying casting helpers on the processed objects. --- forms/TreeDropdownField.php | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/forms/TreeDropdownField.php b/forms/TreeDropdownField.php index c793b9549..abffad185 100644 --- a/forms/TreeDropdownField.php +++ b/forms/TreeDropdownField.php @@ -265,9 +265,23 @@ class TreeDropdownField extends FormField { $obj->markToExpose($this->objectForKey($value)); } } - $eval = '"
  • getName() . '-{$child->' . $this->keyField . '}\" data-id=\"$child->' - . $this->keyField . '\" class=\"class-$child->class"' - . ' . $child->markingClasses() . "\">ID\">" . $child->' . $this->labelField . ' . ""'; + + $self = $this; + $escapeLabelField = ($obj->escapeTypeForField($this->labelField) != 'xml'); + $titleFn = function(&$child) use(&$self, $escapeLabelField) { + $keyField = $self->keyField; + $labelField = $self->labelField; + return sprintf( + '
  • %s', + Convert::raw2xml($self->getName()), + Convert::raw2xml($child->$keyField), + Convert::raw2xml($child->$keyField), + Convert::raw2xml($child->class), + Convert::raw2xml($child->markingClasses()), + (int)$child->ID, + $escapeLabelField ? Convert::raw2xml($child->$labelField) : $child->$labelField + ); + }; // Limit the amount of nodes shown for performance reasons. // Skip the check if we're filtering the tree, since its not clear how many children will @@ -290,7 +304,7 @@ class TreeDropdownField extends FormField { if($isSubTree) { $html = $obj->getChildrenAsUL( "", - $eval, + $titleFn, null, true, $this->childrenMethod, @@ -303,7 +317,7 @@ class TreeDropdownField extends FormField { } else { $html = $obj->getChildrenAsUL( 'class="tree"', - $eval, + $titleFn, null, true, $this->childrenMethod, From 298de5a67ddaaf416157555fe9c6743a57da5578 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Tue, 24 Sep 2013 10:45:55 +0200 Subject: [PATCH 3/3] FIX Escape breadcrumbs in SecurityAdmin (SS-2013-007) --- admin/code/SecurityAdmin.php | 2 +- docs/en/changelogs/rc/3.1.0-rc3.md | 3 +-- forms/gridfield/GridFieldDataColumns.php | 7 ++++++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/admin/code/SecurityAdmin.php b/admin/code/SecurityAdmin.php index 317c12e5e..0a1a8e58b 100755 --- a/admin/code/SecurityAdmin.php +++ b/admin/code/SecurityAdmin.php @@ -83,7 +83,7 @@ class SecurityAdmin extends LeftAndMain implements PermissionProvider { )); $columns->setFieldFormatting(array( 'Breadcrumbs' => function($val, $item) { - return $item->getBreadcrumbs(' > '); + return Convert::raw2xml($item->getBreadcrumbs(' > ')); } )); diff --git a/docs/en/changelogs/rc/3.1.0-rc3.md b/docs/en/changelogs/rc/3.1.0-rc3.md index 80b1b40e5..0f1ec4f80 100644 --- a/docs/en/changelogs/rc/3.1.0-rc3.md +++ b/docs/en/changelogs/rc/3.1.0-rc3.md @@ -1,11 +1,10 @@ # 3.1.0-rc3 -# Overview +## Overview ### Security: XSS in CMS "Security" section (SS-2013-007) See [announcement](http://www.silverstripe.org/ss-2013-007-xss-in-cms-security-section/) - ### Security: XSS in form validation errors (SS-2013-008) See [announcement](http://www.silverstripe.org/ss-2013-008-xss-in-numericfield-validation/) diff --git a/forms/gridfield/GridFieldDataColumns.php b/forms/gridfield/GridFieldDataColumns.php index 0749ac898..f3ae0f4f2 100644 --- a/forms/gridfield/GridFieldDataColumns.php +++ b/forms/gridfield/GridFieldDataColumns.php @@ -95,10 +95,15 @@ class GridFieldDataColumns implements GridField_ColumnProvider { /** * Specify custom formatting for fields, e.g. to render a link instead of pure text. + * * Caution: Make sure to escape special php-characters like in a normal php-statement. * Example: "myFieldName" => '$ID'. + * * Alternatively, pass a anonymous function, which takes two parameters: - * The value and the original list item. + * The value and the original list item. + * + * Formatting is applied after field casting, so if you're modifying the string + * to include further data through custom formatting, ensure it's correctly escaped. * * @param array $formatting */