tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-10-22 14:05:37 +02:00
Code Issues Projects Releases Wiki Activity

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

Browse Source 
CSVParser now strips leading tabs on cells
This commit is contained in:
Damian Mooyman 2017-11-29 15:27:36 +13:00
parent b31b22ac8e
commit 22ccf3e2f9
No known key found for this signature in database
GPG Key ID: 78B823A10DE27D1A
6 changed files with 59 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
dev/CSVParser.php
View File

@ -247,7 +247,9 @@ class CSVParser extends Object implements Iterator {
array($this->enclosure, $this->delimiter), array($this->enclosure, $this->delimiter),
$value $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(array_key_exists($i, $this->headerRow)) {
if($this->headerRow[$i]) { if($this->headerRow[$i]) {
$row[$this->headerRow[$i]] = $value; $row[$this->headerRow[$i]] = $value;

16
forms/gridfield/GridFieldExportButton.php
View File

@ -30,6 +30,15 @@ class GridFieldExportButton implements GridField_HTMLProvider, GridField_ActionP
*/ */
protected $targetFragment; 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 string $targetFragment The HTML fragment to write the button into
* @param array $exportColumns The columns to include in the export * @param array $exportColumns The columns to include in the export
@ -174,6 +183,13 @@ class GridFieldExportButton implements GridField_HTMLProvider, GridField_ActionP
} }
$value = str_replace(array("\r", "\n"), "\n", $value); $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) . '"'; $columnData[] = '"' . str_replace('"', '""', $value) . '"';
} }

28
tests/dev/CSVParserTest.php
View File

@ -21,16 +21,18 @@ class CSVParserTest extends SapphireTest {
$registered[] = $record['IsRegistered']; $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( $this->assertEquals(array(
"He's a good guy", "He's a good guy",
"She is awesome." . PHP_EOL "She is awesome." . PHP_EOL
. "So awesome that she gets multiple rows and \"escaped\" strings in her biography", . "So awesome that she gets multiple rows and \"escaped\" strings in her biography",
"Pretty old, with an escaped comma", "Pretty old, with an escaped comma",
"Unicode FTW"), $biographies); "Unicode FTW",
$this->assertEquals(array("31/01/1988","31/01/1982","31/01/1882","31/06/1982"), $birthdays); "Likes leading tabs in his biography",
$this->assertEquals(array('1', '0', '1', '1'), $registered); ), $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() { public function testParsingWithHeadersAndColumnMap() {
@ -54,15 +56,16 @@ class CSVParserTest extends SapphireTest {
$registered[] = $record['IsRegistered']; $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( $this->assertEquals(array(
"He's a good guy", "He's a good guy",
"She is awesome." "She is awesome."
. PHP_EOL . "So awesome that she gets multiple rows and \"escaped\" strings in her biography", . PHP_EOL . "So awesome that she gets multiple rows and \"escaped\" strings in her biography",
"Pretty old, with an escaped comma", "Pretty old, with an escaped comma",
"Unicode FTW"), $biographies); "Unicode FTW",
$this->assertEquals(array("31/01/1988","31/01/1982","31/01/1882","31/06/1982"), $birthdays); "Likes leading tabs in his biography"), $biographies);
$this->assertEquals(array('1', '0', '1', '1'), $registered); $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() { public function testParsingWithExplicitHeaderRow() {
@ -82,15 +85,16 @@ class CSVParserTest extends SapphireTest {
} }
/* And the first row will be returned in the data */ /* 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( $this->assertEquals(array(
'Biography', 'Biography',
"He's a good guy", "He's a good guy",
"She is awesome." . PHP_EOL "She is awesome." . PHP_EOL
. "So awesome that she gets multiple rows and \"escaped\" strings in her biography", . "So awesome that she gets multiple rows and \"escaped\" strings in her biography",
"Pretty old, with an escaped comma", "Pretty old, with an escaped comma",
"Unicode FTW"), $biographies); "Unicode FTW",
$this->assertEquals(array("Birthday","31/01/1988","31/01/1982","31/01/1882","31/06/1982"), $birthdays); "Likes leading tabs in his biography"), $biographies);
$this->assertEquals(array('IsRegistered', '1', '0', '1', '1'), $registered); $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);
} }
} }

6
tests/dev/CsvBulkLoaderTest.php
View File

@ -27,7 +27,7 @@ class CsvBulkLoaderTest extends SapphireTest {
$results = $loader->load($filepath); $results = $loader->load($filepath);
// Test that right amount of columns was imported // 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 // Test that columns were correctly imported
$obj = DataObject::get_one("CsvBulkLoaderTest_Player", array( $obj = DataObject::get_one("CsvBulkLoaderTest_Player", array(
@ -49,14 +49,14 @@ class CsvBulkLoaderTest extends SapphireTest {
$filepath = $this->getCurrentAbsolutePath() . '/CsvBulkLoaderTest_PlayersWithHeader.csv'; $filepath = $this->getCurrentAbsolutePath() . '/CsvBulkLoaderTest_PlayersWithHeader.csv';
$loader->deleteExistingRecords = true; $loader->deleteExistingRecords = true;
$results1 = $loader->load($filepath); $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 //delete existing data before doing second CSV import
$results2 = $loader->load($filepath, '512MB', true); $results2 = $loader->load($filepath, '512MB', true);
//get all instances of the loaded DataObject from the database and count them //get all instances of the loaded DataObject from the database and count them
$resultDataObject = DataObject::get('CsvBulkLoaderTest_Player'); $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'); 'Test if existing data is deleted before new data is added');
} }

1
tests/dev/CsvBulkLoaderTest_PlayersWithHeader.csv
View File

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

Can't render this file because it contains an unexpected character in line 4 and column 45.

16
tests/forms/gridfield/GridFieldExportButtonTest.php
View File

@ -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() { public function testGenerateFileDataAnonymousFunctionField() {
$button = new GridFieldExportButton(); $button = new GridFieldExportButton();
$button->setExportColumns(array( $button->setExportColumns(array(