From cfe1d4f481bf53ea8da2b8608a563e207d923df9 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 29 Nov 2017 16:07:30 +1300 Subject: [PATCH] [ss-2017-007] Ensure xls formulae are safely sanitised on output CSVParser now strips leading tabs on cells --- src/Dev/CSVParser.php | 4 +- src/Forms/GridField/GridFieldExportButton.php | 19 +++- tests/php/Dev/CSVParserTest.php | 87 +++++++++++-------- tests/php/Dev/CsvBulkLoaderTest.php | 6 +- .../csv/PlayersWithHeader.csv | 1 + .../GridField/GridFieldExportButtonTest.php | 68 +++++++++------ 6 files changed, 117 insertions(+), 68 deletions(-) diff --git a/src/Dev/CSVParser.php b/src/Dev/CSVParser.php index f03ccffde..b16333445 100644 --- a/src/Dev/CSVParser.php +++ b/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; diff --git a/src/Forms/GridField/GridFieldExportButton.php b/src/Forms/GridField/GridFieldExportButton.php index 5254070e3..c9d65e036 100644 --- a/src/Forms/GridField/GridFieldExportButton.php +++ b/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; } diff --git a/tests/php/Dev/CSVParserTest.php b/tests/php/Dev/CSVParserTest.php index 0e06b0363..6d596ae0d 100644 --- a/tests/php/Dev/CSVParserTest.php +++ b/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); } } diff --git a/tests/php/Dev/CsvBulkLoaderTest.php b/tests/php/Dev/CsvBulkLoaderTest.php index 7e4ff93e1..e6cd461c1 100644 --- a/tests/php/Dev/CsvBulkLoaderTest.php +++ b/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' ); diff --git a/tests/php/Dev/CsvBulkLoaderTest/csv/PlayersWithHeader.csv b/tests/php/Dev/CsvBulkLoaderTest/csv/PlayersWithHeader.csv index 25a868cb8..604d9541e 100644 --- a/tests/php/Dev/CsvBulkLoaderTest/csv/PlayersWithHeader.csv +++ b/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" diff --git a/tests/php/Forms/GridField/GridFieldExportButtonTest.php b/tests/php/Forms/GridField/GridFieldExportButtonTest.php index be46e1702..0f6b42323 100644 --- a/tests/php/Forms/GridField/GridFieldExportButtonTest.php +++ b/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",