diff --git a/question/engine/datalib.php b/question/engine/datalib.php index 86af4021489..1464c7a8b9e 100644 --- a/question/engine/datalib.php +++ b/question/engine/datalib.php @@ -1674,13 +1674,13 @@ class question_file_saver implements question_response_files { * * @param int $draftitemid the draft area to save the files from. * @param string $component the component for the file area to save into. - * @param string $filearea the name of the file area to save into. + * @param string $uncleanedfilearea the name of the file area to save into - but before it has been cleaned up. * @param string $text optional content containing file links. */ - public function __construct($draftitemid, $component, $filearea, $text = null) { + public function __construct($draftitemid, $component, $uncleanedfilearea, $text = null) { $this->draftitemid = $draftitemid; $this->component = $component; - $this->filearea = $filearea; + $this->filearea = self::clean_file_area_name($uncleanedfilearea); $this->value = $this->compute_value($draftitemid, $text); } @@ -1744,6 +1744,29 @@ class question_file_saver implements question_response_files { $this->component, $this->filearea, $itemid); } + /** + * Clean up a possible file area name to ensure that it matches the required rules. + * + * @param string $uncleanedfilearea the proposed file area name (e.g. 'response_-attachments'). + * @return string a similar valid file area name. E.g: response_attachments. + */ + public static function clean_file_area_name(string $uncleanedfilearea): string { + $filearea = $uncleanedfilearea; + if ($filearea !== clean_param($filearea, PARAM_AREA)) { + // Only lowercase ascii letters, numbers and underscores are allowed. + // Remove the invalid character in the filearea string. + $filearea = preg_replace('~[^a-z0-9_]~', '', core_text::strtolower($filearea)); + // Replace multiple underscore to a single underscore. + $filearea = preg_replace('~_+~', '_', $filearea); + // If, after attempted cleaning, the filearea is not valid, throw a clear error to avoid subtle bugs. + if ($filearea !== clean_param($filearea, PARAM_AREA)) { + throw new coding_exception('Name ' . $filearea . + ' cannot be used with question_file_saver because it does not match the rules for file area names'); + } + } + return $filearea; + } + /** * Get the files that were submitted. * @return array of stored_files objects. diff --git a/question/engine/questionattempt.php b/question/engine/questionattempt.php index 31ba3235c6b..52422e9fa3d 100644 --- a/question/engine/questionattempt.php +++ b/question/engine/questionattempt.php @@ -622,7 +622,8 @@ class question_attempt { // No files yet. $draftid = 0; // Will be filled in by file_prepare_draft_area. - file_prepare_draft_area($draftid, $contextid, 'question', 'response_' . $name, null); + $filearea = question_file_saver::clean_file_area_name('response_' . $name); + file_prepare_draft_area($draftid, $contextid, 'question', $filearea, null); return $draftid; } diff --git a/question/engine/questionattemptstep.php b/question/engine/questionattemptstep.php index 4f198d1b768..7b9243e6b6f 100644 --- a/question/engine/questionattemptstep.php +++ b/question/engine/questionattemptstep.php @@ -248,6 +248,7 @@ class question_attempt_step { * type question_attempt::PARAM_FILES. * * @param string $name the name of the associated variable. + * @param int $contextid contextid of the question attempt * @return array of {@link stored_files}. */ public function get_qt_files($name, $contextid) { @@ -261,8 +262,9 @@ class question_attempt_step { } $fs = get_file_storage(); + $filearea = question_file_saver::clean_file_area_name('response_' . $name); $this->files[$name] = $fs->get_area_files($contextid, 'question', - 'response_' . $name, $this->id, 'sortorder', false); + $filearea, $this->id, 'sortorder', false); return $this->files[$name]; } @@ -287,13 +289,14 @@ class question_attempt_step { * * @param string $name the variable name the files belong to. * @param int $contextid the id of the context the quba belongs to. - * @param string $text the text to update the URLs in. + * @param string|null $text the text to update the URLs in. * @return array(int, string) the draft itemid and the text with URLs rewritten. */ public function prepare_response_files_draft_itemid_with_text($name, $contextid, $text) { + $filearea = question_file_saver::clean_file_area_name('response_' . $name); $draftid = 0; // Will be filled in by file_prepare_draft_area. $newtext = file_prepare_draft_area($draftid, $contextid, 'question', - 'response_' . $name, $this->id, null, $text); + $filearea, $this->id, null, $text); return array($draftid, $newtext); } @@ -306,12 +309,13 @@ class question_attempt_step { * @param string $text the text to update the URLs in. * @param int $contextid the id of the context the quba belongs to. * @param string $name the variable name the files belong to. - * @param array $extra extra file path components. + * @param array $extras extra file path components. * @return string the rewritten text. */ public function rewrite_response_pluginfile_urls($text, $contextid, $name, $extras) { + $filearea = question_file_saver::clean_file_area_name('response_' . $name); return question_rewrite_question_urls($text, 'pluginfile.php', $contextid, - 'question', 'response_' . $name, $extras, $this->id); + 'question', $filearea, $extras, $this->id); } /** diff --git a/question/engine/tests/datalib_test.php b/question/engine/tests/datalib_test.php index 192aa24f892..fdb7a702a4f 100644 --- a/question/engine/tests/datalib_test.php +++ b/question/engine/tests/datalib_test.php @@ -250,4 +250,31 @@ class datalib_test extends \qbehaviour_walkthrough_test_base { // Delete it. question_engine::delete_questions_usage_by_activity($quba->get_id()); } + + /** + * Test cases for {@see test_get_file_area_name()}. + * + * @return array test cases + */ + public function get_file_area_name_cases(): array { + return [ + 'simple variable' => ['response_attachments', 'response_attachments'], + 'behaviour variable' => ['response_5:answer', 'response_5answer'], + 'variable with special character' => ['response_5:answer', 'response_5answer'], + 'multiple underscores in different places' => ['response_weird____variable__name', 'response_weird_variable_name'], + ]; + } + + /** + * Test get_file_area_name. + * + * @covers \question_file_saver::clean_file_area_name + * @dataProvider get_file_area_name_cases + * + * @param string $uncleanedfilearea + * @param string $expectedfilearea + */ + public function test_clean_file_area_name(string $uncleanedfilearea, string $expectedfilearea): void { + $this->assertEquals($expectedfilearea, \question_file_saver::clean_file_area_name($uncleanedfilearea)); + } }