From bcf67dd1cb667a630c54f13778cbb47067bcfd4b Mon Sep 17 00:00:00 2001 From: David Woloszyn Date: Wed, 26 Nov 2025 13:14:44 +1100 Subject: [PATCH] MDL-72744 dataformat: Escape formulas when exporting spreadsheets Co-authored-by: Huong Nguyen --- lib/classes/dataformat.php | 35 ++++++++++++ lib/classes/dataformat/spout_base.php | 6 ++- lib/table/tests/tablelib_test.php | 29 ++++++++++ lib/tests/dataformat_test.php | 76 +++++++++++++++++++++++++++ 4 files changed, 145 insertions(+), 1 deletion(-) diff --git a/lib/classes/dataformat.php b/lib/classes/dataformat.php index a86e39d5122..acd1d37caf6 100644 --- a/lib/classes/dataformat.php +++ b/lib/classes/dataformat.php @@ -168,4 +168,39 @@ 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 mixed $value Value to check. + * @return string|null Return escaped formula if detected. + */ + public static function escape_spreadsheet_formula(mixed $value): ?string { + // Allow mixed input; only process strings. + 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; + } + + $formulacharacters = ['=', '+', '-', '@']; + // If trimmed version starts with formula character, escape it. + if (in_array($trimmed[0], $formulacharacters, true)) { + // Prepend single quote if value starts with a formula character. + return "'" . $value; + } + return $value; + } } diff --git a/lib/classes/dataformat/spout_base.php b/lib/classes/dataformat/spout_base.php index fbe55cff70d..fb5ec828bc9 100644 --- a/lib/classes/dataformat/spout_base.php +++ b/lib/classes/dataformat/spout_base.php @@ -123,7 +123,11 @@ abstract class spout_base extends \core\dataformat\base { * @param int $rownum */ public function write_record($record, $rownum) { - $row = Row::fromValues($this->format_record($record)); + $rowvalues = $this->format_record($record); + foreach ($rowvalues as $key => $value) { + $rowvalues[$key] = \core\dataformat::escape_spreadsheet_formula($value); + } + $row = Row::fromValues($rowvalues); $this->writer->addRow($row); } diff --git a/lib/table/tests/tablelib_test.php b/lib/table/tests/tablelib_test.php index f2ff315d594..66ed326e4ef 100644 --- a/lib/table/tests/tablelib_test.php +++ b/lib/table/tests/tablelib_test.php @@ -826,4 +826,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)); + } } diff --git a/lib/tests/dataformat_test.php b/lib/tests/dataformat_test.php index 27dc12ef4da..2fb53641dc3 100644 --- a/lib/tests/dataformat_test.php +++ b/lib/tests/dataformat_test.php @@ -122,4 +122,80 @@ final 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); + } }