MDL-72744 dataformat: Escape formulas when exporting spreadsheets
Co-authored-by: Huong Nguyen <[email protected]>
This commit is contained in:
David Woloszyn
committed by
Huong Nguyen
Huong Nguyen
co-authored by
Huong Nguyen
Huong Nguyen
parent
191365e09f
commit
347e10c764
@@ -167,4 +167,44 @@ class dataformat {
|
||||
|
||||
return get_file_storage()->create_file_from_pathname($filerecord, $filepath);
|
||||
}
|
||||
|
||||
/**
|
||||
* Escape formula spreadsheet values.
|
||||
*
|
||||
* Check values being used in spreadsheets and make them safe for inclusion.
|
||||
* Following OWASP recommendations {@link https://owasp.org/www-community/attacks/CSV_Injection}.
|
||||
*
|
||||
* @param string|null $value Value to check.
|
||||
* @return string|null Return escaped formula if detected.
|
||||
*/
|
||||
public static function escape_spreadsheet_formula($value): ?string {
|
||||
// Only escape strings; leave numbers and other types unchanged.
|
||||
if (!is_string($value)) {
|
||||
return $value;
|
||||
}
|
||||
|
||||
// Moodle's null placeholder: exactly one dash.
|
||||
if ($value === '-') {
|
||||
return $value;
|
||||
}
|
||||
|
||||
// Trim only for checking, not for modifying output.
|
||||
$trimmed = ltrim($value);
|
||||
|
||||
if ($trimmed === '') {
|
||||
return $value;
|
||||
}
|
||||
|
||||
// Characters that trigger formula parsing in Excel/Sheets.
|
||||
$formulacharacters = ['=', '+', '-', '@'];
|
||||
|
||||
// If trimmed version starts with formula character, escape it.
|
||||
if (in_array($trimmed[0], $formulacharacters, true)) {
|
||||
// Preserve original whitespace. Do not alter actual content.
|
||||
return "'" . $value;
|
||||
}
|
||||
|
||||
return $value;
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
@@ -120,7 +120,11 @@ abstract class spout_base extends \core\dataformat\base {
|
||||
* @param int $rownum
|
||||
*/
|
||||
public function write_record($record, $rownum) {
|
||||
$row = \Box\Spout\Writer\Common\Creator\WriterEntityFactory::createRowFromArray($this->format_record($record));
|
||||
$rowvalues = $this->format_record($record);
|
||||
foreach ($rowvalues as $key => $value) {
|
||||
$rowvalues[$key] = \core\dataformat::escape_spreadsheet_formula($value);
|
||||
}
|
||||
$row = \Box\Spout\Writer\Common\Creator\WriterEntityFactory::createRowFromArray($rowvalues);
|
||||
$this->writer->addRow($row);
|
||||
}
|
||||
|
||||
|
||||
@@ -112,4 +112,80 @@ class dataformat_test extends \advanced_testcase {
|
||||
$this->assertStringStartsWith($filerecord['filename'], $file->get_filename());
|
||||
$this->assertGreaterThan(0, $file->get_filesize());
|
||||
}
|
||||
|
||||
/**
|
||||
* Data provider for test_escape_spreadsheet_formula.
|
||||
*
|
||||
* @return array
|
||||
*/
|
||||
public static function escape_spreadsheet_formula_provider(): array {
|
||||
return [
|
||||
'null stays null' => [
|
||||
null,
|
||||
null,
|
||||
],
|
||||
'empty string stays empty' => [
|
||||
'',
|
||||
'',
|
||||
],
|
||||
'Formula with tab' => [
|
||||
'value' => "\t=SUM(1+1)",
|
||||
'expected' => "'\t=SUM(1+1)",
|
||||
],
|
||||
'Formula with carriage return' => [
|
||||
'value' => "\r=SUM(1+1)",
|
||||
'expected' => "'\r=SUM(1+1)",
|
||||
],
|
||||
'Formula with new line' => [
|
||||
'value' => "\n=SUM(1+1)",
|
||||
'expected' => "'\n=SUM(1+1)",
|
||||
],
|
||||
'Formula starting with "="' => [
|
||||
'value' => "=SUM(1+1)",
|
||||
'expected' => "'=SUM(1+1)",
|
||||
],
|
||||
'Formula starting with "+"' => [
|
||||
'value' => "+1+1",
|
||||
'expected' => "'+1+1",
|
||||
],
|
||||
'Formula starting with "-"' => [
|
||||
'value' => "-1+1",
|
||||
'expected' => "'-1+1",
|
||||
],
|
||||
'Formula starting with "@"' => [
|
||||
'value' => "@A5",
|
||||
'expected' => "'@A5",
|
||||
],
|
||||
'Null placeholder' => [
|
||||
'value' => "-",
|
||||
'expected' => "-",
|
||||
],
|
||||
'dash with leading space is not placeholder, so escaped' => [
|
||||
' -',
|
||||
"' -",
|
||||
],
|
||||
'dash with trailing space is not placeholder, so escaped' => [
|
||||
'- ',
|
||||
"'- ",
|
||||
],
|
||||
'Non-formula' => [
|
||||
'value' => "Hello there",
|
||||
'expected' => "Hello there",
|
||||
],
|
||||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* Test escape_spreadsheet_formula.
|
||||
*
|
||||
* @dataProvider escape_spreadsheet_formula_provider
|
||||
* @param string|null $value The value to test.
|
||||
* @param string|null $expected The expected value after escaping.
|
||||
*/
|
||||
public function test_escape_spreadsheet_formula(?string $value, ?string $expected): void {
|
||||
$this->resetAfterTest();
|
||||
|
||||
$escapedvalue = dataformat::escape_spreadsheet_formula($value);
|
||||
$this->assertEquals($expected, $escapedvalue);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -829,4 +829,33 @@ final class tablelib_test extends \advanced_testcase {
|
||||
$this->expectOutputRegex('/' . '<caption class="inline">' . $caption . '<\/caption>' . '/');
|
||||
}
|
||||
|
||||
/**
|
||||
* Test formulas are escaped in exported tables.
|
||||
*/
|
||||
public function test_table_exports_escaped_formulas(): void {
|
||||
$table = new flexible_table('tablelib_test_export');
|
||||
$table->define_baseurl('/invalid.php');
|
||||
$table->define_columns(['c1', 'c2', 'c3', 'c4', 'c5', 'c6', 'c7', 'c8']);
|
||||
|
||||
ob_start();
|
||||
$table->is_downloadable(true);
|
||||
$table->is_downloading('csv');
|
||||
|
||||
$table->setup();
|
||||
$table->add_data([
|
||||
'column0' => "\t=SUM(1+1)", // With tab.
|
||||
'column1' => "\r=SUM(1+1)", // With carriage return.
|
||||
'column2' => "\n=SUM(1+1)", // With new line.
|
||||
'column3' => "=SUM(1+1)",
|
||||
'column4' => "=1+1",
|
||||
'column5' => "+1+1",
|
||||
'column6' => "-1+1",
|
||||
'column7' => "@A1",
|
||||
'column8' => "-", // Single dash (should not be escaped).
|
||||
]);
|
||||
$output = ob_get_contents();
|
||||
ob_end_clean();
|
||||
|
||||
$this->assertEquals("\n'=SUM(1+1),'=SUM(1+1),'=SUM(1+1),'=SUM(1+1),'=1+1,'+1+1,'-1+1,'@A1,-\n", substr($output, 3));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user