tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-06-29 07:59:31 +02:00
Code Issues Projects Releases Wiki Activity

Merge pull request #51 from silverstripe-security/pulls/4.0/ss-2017-007

Browse Source 
[ss-2017-007] Ensure xls formulae are safely sanitised on output (4.0)
This commit is contained in:
Damian Mooyman 2017-12-06 18:22:11 +13:00 committed by GitHub
commit 99e772b361
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 117 additions and 68 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/Dev/CSVParser.php
View File

@ -260,7 +260,9 @@ class CSVParser 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;

19
src/Forms/GridField/GridFieldExportButton.php
View File

@ -4,6 +4,7 @@ namespace SilverStripe\Forms\GridField;
use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPRequest;
use SilverStripe\Control\HTTPResponse; use SilverStripe\Control\HTTPResponse;
use SilverStripe\Core\Config\Config;
use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataObject;
/** /**
@ -11,7 +12,6 @@ use SilverStripe\ORM\DataObject;
*/ */
class GridFieldExportButton implements GridField_HTMLProvider, GridField_ActionProvider, GridField_URLHandler 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. * @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. * 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; 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
@ -170,7 +179,7 @@ class GridFieldExportButton implements GridField_HTMLProvider, GridField_ActionP
} }
//Remove GridFieldPaginator as we're going to export the entire list. //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(); $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; $columnData[] = $value;
} }

87
tests/php/Dev/CSVParserTest.php
View File

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

6
tests/php/Dev/CsvBulkLoaderTest.php
View File

@ -50,7 +50,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( $obj = DataObject::get_one(
@ -76,7 +76,7 @@ class CsvBulkLoaderTest extends SapphireTest
$filepath = $this->csvPath . 'PlayersWithHeader.csv'; $filepath = $this->csvPath . '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); $results2 = $loader->load($filepath);
@ -84,7 +84,7 @@ class CsvBulkLoaderTest extends SapphireTest
$resultDataObject = DataObject::get(Player::class); $resultDataObject = DataObject::get(Player::class);
$this->assertEquals( $this->assertEquals(
4, 5,
$resultDataObject->count(), $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/php/Dev/CsvBulkLoaderTest/csv/PlayersWithHeader.csv
View File

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

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

68
tests/php/Forms/GridField/GridFieldExportButtonTest.php
View File

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