From 22ccf3e2f9092f51e7f7288ce108598c6f17b49c Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 29 Nov 2017 15:27:36 +1300 Subject: [PATCH] [ss-2017-007] Ensure xls formulae are safely sanitised on output CSVParser now strips leading tabs on cells --- dev/CSVParser.php | 4 ++- forms/gridfield/GridFieldExportButton.php | 24 +++++++++++++--- tests/dev/CSVParserTest.php | 28 +++++++++++-------- tests/dev/CsvBulkLoaderTest.php | 6 ++-- .../CsvBulkLoaderTest_PlayersWithHeader.csv | 1 + .../gridfield/GridFieldExportButtonTest.php | 16 +++++++++++ 6 files changed, 59 insertions(+), 20 deletions(-) diff --git a/dev/CSVParser.php b/dev/CSVParser.php index 158d9d863..4c1fdf3ba 100644 --- a/dev/CSVParser.php +++ b/dev/CSVParser.php @@ -247,7 +247,9 @@ class CSVParser extends Object 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/forms/gridfield/GridFieldExportButton.php b/forms/gridfield/GridFieldExportButton.php index 30df9f01f..9c865b6a1 100644 --- a/forms/gridfield/GridFieldExportButton.php +++ b/forms/gridfield/GridFieldExportButton.php @@ -30,6 +30,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 @@ -91,12 +100,12 @@ class GridFieldExportButton implements GridField_HTMLProvider, GridField_ActionP return SS_HTTPRequest::send_file($fileData, $fileName, 'text/csv'); } } - + /** * Return the columns to export - * - * @param GridField $gridField - * + * + * @param GridField $gridField + * * @return array */ protected function getExportColumnsForGridField(GridField $gridField) { @@ -174,6 +183,13 @@ class GridFieldExportButton implements GridField_HTMLProvider, GridField_ActionP } $value = str_replace(array("\r", "\n"), "\n", $value); + + // [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[] = '"' . str_replace('"', '""', $value) . '"'; } diff --git a/tests/dev/CSVParserTest.php b/tests/dev/CSVParserTest.php index c895b2f8b..512717521 100644 --- a/tests/dev/CSVParserTest.php +++ b/tests/dev/CSVParserTest.php @@ -21,16 +21,18 @@ class CSVParserTest extends SapphireTest { $registered[] = $record['IsRegistered']; } - $this->assertEquals(array('John','Jane','Jamie','Järg'), $firstNames); + $this->assertEquals(array('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"), $biographies); - $this->assertEquals(array("31/01/1988","31/01/1982","31/01/1882","31/06/1982"), $birthdays); - $this->assertEquals(array('1', '0', '1', '1'), $registered); + "Unicode FTW", + "Likes leading tabs in his biography", + ), $biographies); + $this->assertEquals(array("31/01/1988","31/01/1982","31/01/1882","31/06/1982","31/4/2000"), $birthdays); + $this->assertEquals(array('1', '0', '1', '1', '0'), $registered); } public function testParsingWithHeadersAndColumnMap() { @@ -54,15 +56,16 @@ class CSVParserTest extends SapphireTest { $registered[] = $record['IsRegistered']; } - $this->assertEquals(array('John','Jane','Jamie','Järg'), $firstNames); + $this->assertEquals(array('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"), $biographies); - $this->assertEquals(array("31/01/1988","31/01/1982","31/01/1882","31/06/1982"), $birthdays); - $this->assertEquals(array('1', '0', '1', '1'), $registered); + "Unicode FTW", + "Likes leading tabs in his biography"), $biographies); + $this->assertEquals(array("31/01/1988","31/01/1982","31/01/1882","31/06/1982","31/4/2000"), $birthdays); + $this->assertEquals(array('1', '0', '1', '1', '0'), $registered); } public function testParsingWithExplicitHeaderRow() { @@ -82,15 +85,16 @@ 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(array('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"), $biographies); - $this->assertEquals(array("Birthday","31/01/1988","31/01/1982","31/01/1882","31/06/1982"), $birthdays); - $this->assertEquals(array('IsRegistered', '1', '0', '1', '1'), $registered); + "Unicode FTW", + "Likes leading tabs in his biography"), $biographies); + $this->assertEquals(array("Birthday","31/01/1988","31/01/1982","31/01/1882","31/06/1982","31/4/2000"), $birthdays); + $this->assertEquals(array('IsRegistered', '1', '0', '1', '1', '0'), $registered); } } diff --git a/tests/dev/CsvBulkLoaderTest.php b/tests/dev/CsvBulkLoaderTest.php index 7b1d15a5b..bb21b98f2 100644 --- a/tests/dev/CsvBulkLoaderTest.php +++ b/tests/dev/CsvBulkLoaderTest.php @@ -27,7 +27,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("CsvBulkLoaderTest_Player", array( @@ -49,14 +49,14 @@ class CsvBulkLoaderTest extends SapphireTest { $filepath = $this->getCurrentAbsolutePath() . '/CsvBulkLoaderTest_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, '512MB', true); //get all instances of the loaded DataObject from the database and count them $resultDataObject = DataObject::get('CsvBulkLoaderTest_Player'); - $this->assertEquals(4, $resultDataObject->Count(), + $this->assertEquals(5, $resultDataObject->Count(), 'Test if existing data is deleted before new data is added'); } diff --git a/tests/dev/CsvBulkLoaderTest_PlayersWithHeader.csv b/tests/dev/CsvBulkLoaderTest_PlayersWithHeader.csv index 2536266fc..f8e101f08 100644 --- a/tests/dev/CsvBulkLoaderTest_PlayersWithHeader.csv +++ b/tests/dev/CsvBulkLoaderTest_PlayersWithHeader.csv @@ -4,3 +4,4 @@ So awesome that she gets multiple rows and \"escaped\" strings in her biography","31/01/1982","0" "Jamie","Pretty old\, with an escaped comma","31/01/1882","1" "Järg","Unicode FTW","31/06/1982","1" +"Jacob"," Likes leading tabs in his biography","31/4/2000","0" diff --git a/tests/forms/gridfield/GridFieldExportButtonTest.php b/tests/forms/gridfield/GridFieldExportButtonTest.php index 42ef28e22..516a2bc13 100644 --- a/tests/forms/gridfield/GridFieldExportButtonTest.php +++ b/tests/forms/gridfield/GridFieldExportButtonTest.php @@ -53,6 +53,22 @@ class GridFieldExportButtonTest extends SapphireTest { ); } + public function testXLSSanitisation() { + // Create risky object + $object = new GridFieldExportButtonTest_Team(); + $object->Name = '=SUM(1, 2)'; + $object->write(); + + // Export + $button = new GridFieldExportButton(); + $button->setExportColumns(array('Name' => 'My Name')); + + $this->assertEquals( + "\"My Name\"\n\"\t=SUM(1, 2)\"\n\"Test\"\n\"Test2\"\n", + $button->generateExportFileData($this->gridField) + ); + } + public function testGenerateFileDataAnonymousFunctionField() { $button = new GridFieldExportButton(); $button->setExportColumns(array(