[ss-2017-007] Ensure xls formulae are safely sanitised on output

CSVParser now strips leading tabs on cells

src/Dev/CSVParser.php

@@ -260,7 +260,9 @@ class CSVParser implements Iterator
                     array($this->enclosure, $this->delimiter),
                     $value
                 );
-
+                // Trim leading tab
+                // [SS-2017-007] Ensure all cells with leading [@=+] have a leading tab
+                $value = ltrim($value, "\t");
                 if (array_key_exists($i, $this->headerRow)) {
                     if ($this->headerRow[$i]) {
                         $row[$this->headerRow[$i]] = $value;

src/Forms/GridField/GridFieldExportButton.php

@@ -4,6 +4,7 @@ namespace SilverStripe\Forms\GridField;
 
 use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Control\HTTPResponse;
+use SilverStripe\Core\Config\Config;
 use SilverStripe\ORM\DataObject;
 
 /**
@@ -11,7 +12,6 @@ use SilverStripe\ORM\DataObject;
  */
 class GridFieldExportButton implements GridField_HTMLProvider, GridField_ActionProvider, GridField_URLHandler
 {
-
     /**
      * @var array Map of a property name on the exported objects, with values being the column title in the CSV file.
      * Note that titles are only used when {@link $csvHasHeader} is set to TRUE.
@@ -38,6 +38,15 @@ class GridFieldExportButton implements GridField_HTMLProvider, GridField_ActionP
      */
     protected $targetFragment;
 
+    /**
+     * Set to true to disable XLS sanitisation
+     * [SS-2017-007] Ensure all cells with leading [@=+] have a leading tab
+     *
+     * @config
+     * @var bool
+     */
+    private static $xls_export_disabled = false;
+
     /**
      * @param string $targetFragment The HTML fragment to write the button into
      * @param array $exportColumns The columns to include in the export
@@ -170,7 +179,7 @@ class GridFieldExportButton implements GridField_HTMLProvider, GridField_ActionP
         }
 
         //Remove GridFieldPaginator as we're going to export the entire list.
-        $gridField->getConfig()->removeComponentsByType('SilverStripe\\Forms\\GridField\\GridFieldPaginator');
+        $gridField->getConfig()->removeComponentsByType(GridFieldPaginator::class);
 
         $items = $gridField->getManipulatedList();
 
@@ -203,6 +212,12 @@ class GridFieldExportButton implements GridField_HTMLProvider, GridField_ActionP
                         }
                     }
 
+                    // [SS-2017-007] Sanitise XLS executable column values with a leading tab
+                    if (!Config::inst()->get(get_class($this), 'xls_export_disabled')
+                        && preg_match('/^[-@=+].*/', $value)
+                    ) {
+                        $value = "\t" . $value;
+                    }
                     $columnData[] = $value;
                 }
 

tests/php/Dev/CSVParserTest.php

@@ -29,31 +29,44 @@ class CSVParserTest extends SapphireTest
         $firstNames = $birthdays = $biographies = $registered = array();
         foreach ($csv as $record) {
             /* Each row in the CSV file will be keyed with the header row */
-            $this->assertEquals(array('FirstName','Biography','Birthday','IsRegistered'), array_keys($record));
+            $this->assertEquals(
+                ['FirstName','Biography','Birthday','IsRegistered'],
+                array_keys($record)
+            );
             $firstNames[] = $record['FirstName'];
             $biographies[] = $record['Biography'];
             $birthdays[] = $record['Birthday'];
             $registered[] = $record['IsRegistered'];
         }
 
-        $this->assertEquals(array('John','Jane','Jamie','Järg'), $firstNames);
+        $this->assertEquals(
+            ['John','Jane','Jamie','Järg','Jacob'],
+            $firstNames
+        );
 
         $this->assertEquals(
-            array(
-            "He's a good guy",
-            "She is awesome." . PHP_EOL
-                . "So awesome that she gets multiple rows and \"escaped\" strings in her biography",
-            "Pretty old, with an escaped comma",
-            "Unicode FTW"),
+            [
+                "He's a good guy",
+                "She is awesome."
+                    . PHP_EOL
+                    . "So awesome that she gets multiple rows and \"escaped\" strings in her biography",
+                "Pretty old, with an escaped comma",
+                "Unicode FTW",
+                "Likes leading tabs in his biography",
+            ],
             $biographies
         );
         $this->assertEquals([
             "1988-01-31",
             "1982-01-31",
             "1882-01-31",
-            "1982-06-30"
+            "1982-06-30",
+            "2000-04-30",
         ], $birthdays);
-        $this->assertEquals(array('1', '0', '1', '1'), $registered);
+        $this->assertEquals(
+            ['1', '0', '1', '1', '0'],
+            $registered
+        );
     }
 
     public function testParsingWithHeadersAndColumnMap()
@@ -62,41 +75,43 @@ class CSVParserTest extends SapphireTest
         $csv = new CSVParser($this->csvPath . 'PlayersWithHeader.csv');
 
         /* We can set up column remapping.  The keys are case-insensitive. */
-        $csv->mapColumns(
-            array(
+        $csv->mapColumns([
             'FirstName' => '__fn',
             'bIoGrApHy' => '__BG',
-            )
-        );
+        ]);
 
         $firstNames = $birthdays = $biographies = $registered = array();
         foreach ($csv as $record) {
             /* Each row in the CSV file will be keyed with the renamed columns.  Any unmapped column names will be
             * left as-is. */
-            $this->assertEquals(array('__fn','__BG','Birthday','IsRegistered'), array_keys($record));
+            $this->assertEquals(['__fn','__BG','Birthday','IsRegistered'], array_keys($record));
             $firstNames[] = $record['__fn'];
             $biographies[] = $record['__BG'];
             $birthdays[] = $record['Birthday'];
             $registered[] = $record['IsRegistered'];
         }
 
-        $this->assertEquals(array('John','Jane','Jamie','Järg'), $firstNames);
+        $this->assertEquals(['John','Jane','Jamie','Järg','Jacob'], $firstNames);
         $this->assertEquals(
-            array(
-            "He's a good guy",
-            "She is awesome."
-                . PHP_EOL . "So awesome that she gets multiple rows and \"escaped\" strings in her biography",
-            "Pretty old, with an escaped comma",
-            "Unicode FTW"),
+            [
+                "He's a good guy",
+                "She is awesome."
+                    . PHP_EOL
+                    . "So awesome that she gets multiple rows and \"escaped\" strings in her biography",
+                "Pretty old, with an escaped comma",
+                "Unicode FTW",
+                "Likes leading tabs in his biography",
+            ],
             $biographies
         );
         $this->assertEquals([
             "1988-01-31",
             "1982-01-31",
             "1882-01-31",
-            "1982-06-30"
+            "1982-06-30",
+            "2000-04-30",
         ], $birthdays);
-        $this->assertEquals(array('1', '0', '1', '1'), $registered);
+        $this->assertEquals(array('1', '0', '1', '1', '0'), $registered);
     }
 
     public function testParsingWithExplicitHeaderRow()
@@ -117,15 +132,18 @@ class CSVParserTest extends SapphireTest
         }
 
         /* And the first row will be returned in the data */
-        $this->assertEquals(array('FirstName','John','Jane','Jamie','Järg'), $firstNames);
+        $this->assertEquals(['FirstName','John','Jane','Jamie','Järg','Jacob'], $firstNames);
         $this->assertEquals(
-            array(
-            'Biography',
-            "He's a good guy",
-            "She is awesome." . PHP_EOL
-                . "So awesome that she gets multiple rows and \"escaped\" strings in her biography",
-            "Pretty old, with an escaped comma",
-            "Unicode FTW"),
+            [
+                'Biography',
+                "He's a good guy",
+                "She is awesome."
+                    . PHP_EOL
+                    . "So awesome that she gets multiple rows and \"escaped\" strings in her biography",
+                "Pretty old, with an escaped comma",
+                "Unicode FTW",
+                "Likes leading tabs in his biography"
+            ],
             $biographies
         );
         $this->assertEquals([
@@ -133,8 +151,9 @@ class CSVParserTest extends SapphireTest
             "1988-01-31",
             "1982-01-31",
             "1882-01-31",
-            "1982-06-30"
+            "1982-06-30",
+            "2000-04-30",
         ], $birthdays);
-        $this->assertEquals(array('IsRegistered', '1', '0', '1', '1'), $registered);
+        $this->assertEquals(['IsRegistered', '1', '0', '1', '1', '0'], $registered);
     }
 }

tests/php/Dev/CsvBulkLoaderTest.php

@@ -50,7 +50,7 @@ class CsvBulkLoaderTest extends SapphireTest
         $results = $loader->load($filepath);
 
         // Test that right amount of columns was imported
-        $this->assertEquals(4, $results->Count(), 'Test correct count of imported data');
+        $this->assertEquals(5, $results->Count(), 'Test correct count of imported data');
 
         // Test that columns were correctly imported
         $obj = DataObject::get_one(
@@ -76,7 +76,7 @@ class CsvBulkLoaderTest extends SapphireTest
         $filepath = $this->csvPath . 'PlayersWithHeader.csv';
         $loader->deleteExistingRecords = true;
         $results1 = $loader->load($filepath);
-        $this->assertEquals(4, $results1->Count(), 'Test correct count of imported data on first load');
+        $this->assertEquals(5, $results1->Count(), 'Test correct count of imported data on first load');
 
         //delete existing data before doing second CSV import
         $results2 = $loader->load($filepath);
@@ -84,7 +84,7 @@ class CsvBulkLoaderTest extends SapphireTest
         $resultDataObject = DataObject::get(Player::class);
 
         $this->assertEquals(
-            4,
+            5,
             $resultDataObject->count(),
             'Test if existing data is deleted before new data is added'
         );

tests/php/Dev/CsvBulkLoaderTest/csv/PlayersWithHeader.csv

@@ -4,3 +4,4 @@
 So awesome that she gets multiple rows and \"escaped\" strings in her biography","1982-01-31","0"
 "Jamie","Pretty old\, with an escaped comma","1882-01-31","1"
 "Järg","Unicode FTW","1982-06-30","1"
+"Jacob","	Likes leading tabs in his biography","2000-04-30","0"

tests/php/Forms/GridField/GridFieldExportButtonTest.php

@@ -12,22 +12,27 @@ use SilverStripe\Forms\GridField\GridFieldConfig;
 use SilverStripe\Forms\GridField\GridFieldExportButton;
 use SilverStripe\Forms\GridField\GridField;
 use SilverStripe\Forms\GridField\GridFieldPaginator;
+use SilverStripe\ORM\FieldType\DBField;
 
 class GridFieldExportButtonTest extends SapphireTest
 {
 
+    /**
+     * @var DataList
+     */
     protected $list;
 
+    /**
+     * @var GridField
+     */
     protected $gridField;
 
-    protected $form;
-
     protected static $fixture_file = 'GridFieldExportButtonTest.yml';
 
-    protected static $extra_dataobjects = array(
+    protected static $extra_dataobjects = [
         Team::class,
         NoView::class,
-    );
+    ];
 
     protected function setUp()
     {
@@ -44,7 +49,7 @@ class GridFieldExportButtonTest extends SapphireTest
         $list = new DataList(NoView::class);
 
         $button = new GridFieldExportButton();
-        $button->setExportColumns(array('Name' => 'My Name'));
+        $button->setExportColumns(['Name' => 'My Name']);
 
         $config = GridFieldConfig::create()->addComponent(new GridFieldExportButton());
         $gridField = new GridField('testfield', 'testfield', $list, $config);
@@ -58,7 +63,7 @@ class GridFieldExportButtonTest extends SapphireTest
     public function testGenerateFileDataBasicFields()
     {
         $button = new GridFieldExportButton();
-        $button->setExportColumns(array('Name' => 'My Name'));
+        $button->setExportColumns(['Name' => 'My Name']);
 
         $this->assertEquals(
             '"My Name"'."\n".
@@ -68,17 +73,32 @@ class GridFieldExportButtonTest extends SapphireTest
         );
     }
 
+    public function testXLSSanitisation()
+    {
+        // Create risky object
+        $object = new Team();
+        $object->Name = '=SUM(1, 2)';
+        $object->write();
+
+        // Export
+        $button = new GridFieldExportButton();
+        $button->setExportColumns(['Name' => 'My Name']);
+
+        $this->assertEquals(
+            "\"My Name\"\n\"\t=SUM(1, 2)\"\nTest\nTest2\n",
+            $button->generateExportFileData($this->gridField)
+        );
+    }
+
     public function testGenerateFileDataAnonymousFunctionField()
     {
         $button = new GridFieldExportButton();
-        $button->setExportColumns(
-            array(
+        $button->setExportColumns([
             'Name' => 'Name',
-            'City' => function ($obj) {
+            'City' => function (DBField $obj) {
                 return $obj->getValue() . ' city';
             }
-            )
-        );
+        ]);
 
         $this->assertEquals(
             'Name,City'."\n".
@@ -91,12 +111,10 @@ class GridFieldExportButtonTest extends SapphireTest
     public function testBuiltInFunctionNameCanBeUsedAsHeader()
     {
         $button = new GridFieldExportButton();
-        $button->setExportColumns(
-            array(
+        $button->setExportColumns([
             'Name' => 'Name',
-            'City' => 'strtolower'
-            )
-        );
+            'City' => 'strtolower',
+        ]);
 
         $this->assertEquals(
             'Name,strtolower'."\n".
@@ -109,12 +127,10 @@ class GridFieldExportButtonTest extends SapphireTest
     public function testNoCsvHeaders()
     {
         $button = new GridFieldExportButton();
-        $button->setExportColumns(
-            array(
+        $button->setExportColumns([
             'Name' => 'Name',
-            'City' => 'City'
-            )
-        );
+            'City' => 'City',
+        ]);
         $button->setCsvHasHeader(false);
 
         $this->assertEquals(
@@ -132,9 +148,7 @@ class GridFieldExportButtonTest extends SapphireTest
         //Create an ArrayList 1 greater the Paginator's default 15 rows
         $arrayList = new ArrayList();
         for ($i = 1; $i <= 16; $i++) {
-            $dataobject = new DataObject(
-                array ( 'ID' => $i )
-            );
+            $dataobject = new DataObject(['ID' => $i]);
             $arrayList->add($dataobject);
         }
         $this->gridField->setList($arrayList);
@@ -164,11 +178,9 @@ class GridFieldExportButtonTest extends SapphireTest
     public function testZeroValue()
     {
         $button = new GridFieldExportButton();
-        $button->setExportColumns(
-            array(
+        $button->setExportColumns([
             'RugbyTeamNumber' => 'Rugby Team Number'
-            )
-        );
+        ]);
 
         $this->assertEquals(
             "\"Rugby Team Number\"\n2\n0\n",