Merge pull request #7517 from open-sausages/pulls/4.0/fix-tree-readonly-encoding

BUG Ensure readonly tree dropdown is safely encoded

src/Forms/LookupField.php

@@ -2,10 +2,10 @@
 
 namespace SilverStripe\Forms;
 
-use SilverStripe\ORM\ArrayLib;
-use SilverStripe\ORM\FieldType\DBField;
-use SilverStripe\ORM\DataObjectInterface;
 use SilverStripe\Core\Convert;
+use SilverStripe\ORM\ArrayLib;
+use SilverStripe\ORM\DataObjectInterface;
+use SilverStripe\ORM\FieldType\DBField;
 
 /**
  * Read-only complement of {@link DropdownField}.
@@ -36,23 +36,22 @@ class LookupField extends MultiSelectField
         $values = $this->getValueArray();
 
         // Get selected values
-        $mapped = array();
+        $mapped = [];
         foreach ($values as $value) {
             if (isset($source[$value])) {
-                $mapped[] = $source[$value];
+                $mapped[] = Convert::raw2xml($source[$value]);
             }
         }
 
         // Don't check if string arguments are matching against the source,
         // as they might be generated HTML diff views instead of the actual values
         if ($this->value && is_string($this->value) && empty($mapped)) {
-            $mapped = array(trim($this->value));
+            $mapped[] = Convert::raw2xml(trim($this->value));
-            $values = array();
+            $values = [];
         }
 
         if ($mapped) {
             $attrValue = implode(', ', array_values($mapped));
-
             $inputValue = implode(', ', array_values($values));
         } else {
             $attrValue = '<i>('._t('SilverStripe\\Forms\\FormField.NONE', 'none').')</i>';

src/Forms/TreeDropdownField.php

@@ -2,18 +2,16 @@
 
 namespace SilverStripe\Forms;
 
+use Exception;
+use InvalidArgumentException;
 use SilverStripe\Assets\Folder;
 use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Control\HTTPResponse;
-use SilverStripe\Core\Convert;
 use SilverStripe\ORM\DataList;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\ORM\FieldType\DBDatetime;
 use SilverStripe\ORM\Hierarchy\Hierarchy;
 use SilverStripe\ORM\Hierarchy\MarkedSet;
-use SilverStripe\View\ViewableData;
-use Exception;
-use InvalidArgumentException;
 
 /**
  * Dropdown-like field that allows you to select an item from a hierarchical
@@ -425,39 +423,6 @@ class TreeDropdownField extends FormField
         return $this;
     }
 
-    /**
-     * @param array $properties
-     * @return string
-     */
-    public function Field($properties = array())
-    {
-        $record = $this->Value() ? $this->objectForKey($this->Value()) : null;
-        if ($record instanceof ViewableData) {
-            $title = $record->obj($this->getLabelField())->forTemplate();
-        } elseif ($record) {
-            $title = Convert::raw2xml($record->{$this->getLabelField()});
-        } else {
-            $title = $this->getEmptyString();
-        }
-
-        // TODO Implement for TreeMultiSelectField
-        $metadata = array(
-            'id' => $record ? $record->ID : null,
-            'ClassName' => $record ? $record->ClassName : $this->getSourceObject()
-        );
-
-        $properties = array_merge(
-            $properties,
-            array(
-                'Title' => $title,
-                'EmptyTitle' => $this->getEmptyString(),
-                'Metadata' => ($metadata) ? Convert::raw2json($metadata) : null,
-            )
-        );
-
-        return parent::Field($properties);
-    }
-
     public function extraClass()
     {
         return implode(' ', array(parent::extraClass(), ($this->getShowSearch() ? "searchable" : null)));
@@ -633,6 +598,9 @@ class TreeDropdownField extends FormField
     }
 
     /**
+     * HTML-encoded label for this node, including css classes and other markup.
+     *
+     * @deprecated 4.0...5.0 Use setTitleField()
      * @param string $field
      * @return $this
      */
@@ -643,6 +611,9 @@ class TreeDropdownField extends FormField
     }
 
     /**
+     * HTML-encoded label for this node, including css classes and other markup.
+     *
+     * @deprecated 4.0...5.0 Use getTitleField()
      * @return string
      */
     public function getLabelField()
@@ -651,7 +622,7 @@ class TreeDropdownField extends FormField
     }
 
     /**
-     * Field to use for item titles
+     * Field to use for plain text item titles.
      *
      * @return string
      */
@@ -795,14 +766,16 @@ class TreeDropdownField extends FormField
 
         $sourceObject = $this->getSourceObject();
         $filters = array();
-        if (singleton($sourceObject)->hasDatabaseField($this->getLabelField())) {
+        $sourceObjectInstance = DataObject::singleton($sourceObject);
-            $filters["{$this->getLabelField()}:PartialMatch"]  = $this->search;
+        $candidates = array_unique([
-        } else {
+            $this->getLabelField(),
-            if (singleton($sourceObject)->hasDatabaseField('Title')) {
+            $this->getTitleField(),
-                $filters["Title:PartialMatch"] = $this->search;
+            'Title',
-            }
+            'Name'
-            if (singleton($sourceObject)->hasDatabaseField('Name')) {
+        ]);
-                $filters["Name:PartialMatch"] = $this->search;
+        foreach ($candidates as $candidate) {
+            if ($sourceObjectInstance->hasDatabaseField($candidate)) {
+                $filters["{$candidate}:PartialMatch"] = $this->search;
             }
         }
 
@@ -810,7 +783,7 @@ class TreeDropdownField extends FormField
             throw new InvalidArgumentException(sprintf(
                 'Cannot query by %s.%s, not a valid database column',
                 $sourceObject,
-                $this->getLabelField()
+                $this->getTitleField()
             ));
         }
         return DataObject::get($this->getSourceObject())->filterAny($filters);

src/Forms/TreeDropdownField_Readonly.php

@@ -8,18 +8,15 @@ class TreeDropdownField_Readonly extends TreeDropdownField
 
     public function Field($properties = array())
     {
-        $fieldName = $this->getLabelField();
+        $fieldName = $this->getTitleField();
         if ($this->value) {
             $keyObj = $this->objectForKey($this->value);
-            $obj = $keyObj ? $keyObj->$fieldName : '';
+            $title = $keyObj ? $keyObj->$fieldName : '';
         } else {
-            $obj = null;
+            $title = null;
         }
 
-        $source = array(
+        $source = [ $this->value => $title ];
-            $this->value => $obj
-        );
-
         $field = new LookupField($this->name, $this->title, $source);
         $field->setValue($this->value);
         $field->setForm($this->form);

src/Forms/TreeMultiselectField_Readonly.php

@@ -9,27 +9,28 @@ class TreeMultiselectField_Readonly extends TreeMultiselectField
 
     public function Field($properties = array())
     {
-        $titleArray = $itemIDs = array();
+        // Build list of titles
-        $titleList = $itemIDsList = "";
+        $titleField = $this->getTitleField();
-        if ($items = $this->getItems()) {
+        $items = $this->getItems();
-            foreach ($items as $item) {
+        $titleArray = [];
-                $titleArray[] = $item->Title;
+        foreach ($items as $item) {
-            }
+            $titleArray[] = $item->$titleField;
-            foreach ($items as $item) {
-                $itemIDs[] = $item->ID;
-            }
-            if ($titleArray) {
-                $titleList = implode(", ", $titleArray);
-            }
-            if ($itemIDs) {
-                $itemIDsList = implode(",", $itemIDs);
-            }
         }
+        $titleList = implode(", ", $titleArray);
 
+        // Build list of values
+        $itemIDs = [];
+        foreach ($items as $item) {
+            $itemIDs[] = $item->ID;
+        }
+        $itemIDsList = implode(",", $itemIDs);
+
+        // Readonly field for display
         $field = new ReadonlyField($this->name . '_ReadonlyValue', $this->title);
         $field->setValue($titleList);
         $field->setForm($this->form);
 
+        // Store values to hidden field
         $valueField = new HiddenField($this->name);
         $valueField->setValue($itemIDsList);
         $valueField->setForm($this->form);

tests/php/Forms/TreeDropdownFieldTest.php

@@ -194,4 +194,19 @@ class TreeDropdownFieldTest extends SapphireTest
             $file3->Name.' is not found'
         );
     }
+
+    public function testReadonly()
+    {
+        $field = new TreeDropdownField('TestTree', 'Test tree', File::class);
+        $asdf = $this->objFromFixture(File::class, 'asdf');
+        $field->setValue($asdf->ID);
+        $readonlyField = $field->performReadonlyTransformation();
+        $this->assertEquals(
+            <<<"HTML"
+<span class="readonly" id="TestTree">&lt;Special &amp; characters&gt;</span><input type="hidden" name="TestTree" value="{$asdf->ID}" />
+HTML
+            ,
+            (string)$readonlyField->Field()
+        );
+    }
 }

tests/php/Forms/TreeDropdownFieldTest.yml

@@ -11,6 +11,7 @@ SilverStripe\Assets\Folder:
 SilverStripe\Assets\File:
   asdf:
     Filename: assets/FileTest.txt
+    Title: '<Special & characters>'
   subfolderfile1:
     Filename: assets/FileTest-subfolder/TestFile1InSubfolder.txt
     Name: TestFile1InSubfolder

tests/php/Forms/TreeMultiselectFieldTest.php

@@ -0,0 +1,31 @@
+<?php
+
+namespace SilverStripe\Forms\Tests;
+
+use SilverStripe\Assets\File;
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Forms\TreeMultiselectField;
+
+class TreeMultiselectFieldTest extends SapphireTest
+{
+    protected static $fixture_file = 'TreeDropdownFieldTest.yml';
+
+    public function testReadonly()
+    {
+        $field = new TreeMultiselectField('TestTree', 'Test tree', File::class);
+        $asdf = $this->objFromFixture(File::class, 'asdf');
+        $subfolderfile1 = $this->objFromFixture(File::class, 'subfolderfile1');
+        $field->setValue(implode(',', [$asdf->ID, $subfolderfile1->ID]));
+
+        $readonlyField = $field->performReadonlyTransformation();
+        $this->assertEquals(
+            <<<"HTML"
+<span id="TestTree_ReadonlyValue" class="readonly">
+	&lt;Special &amp; characters&gt;, TestFile1InSubfolder
+</span><input type="hidden" name="TestTree" value="{$asdf->ID},{$subfolderfile1->ID}" class="hidden" id="TestTree" />
+HTML
+            ,
+            (string)$readonlyField->Field()
+        );
+    }
+}