diff --git a/lib/classes/dataformat.php b/lib/classes/dataformat.php index 70bdcec5a6b..ba33494989c 100644 --- a/lib/classes/dataformat.php +++ b/lib/classes/dataformat.php @@ -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; + } + } diff --git a/lib/classes/dataformat/spout_base.php b/lib/classes/dataformat/spout_base.php index 4c582707ec3..ead5b879d61 100644 --- a/lib/classes/dataformat/spout_base.php +++ b/lib/classes/dataformat/spout_base.php @@ -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); } diff --git a/lib/tests/dataformat_test.php b/lib/tests/dataformat_test.php index ebd10b0910a..f48997ebeec 100644 --- a/lib/tests/dataformat_test.php +++ b/lib/tests/dataformat_test.php @@ -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); + } } diff --git a/lib/tests/tablelib_test.php b/lib/tests/tablelib_test.php index 7f3676434d9..c41bcaf9b44 100644 --- a/lib/tests/tablelib_test.php +++ b/lib/tests/tablelib_test.php @@ -829,4 +829,33 @@ final class tablelib_test extends \advanced_testcase { $this->expectOutputRegex('/' . '' . $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)); + } }