From b7bbe9fd9466036a84fe7bc4619b376f05b7049f Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Mon, 30 Apr 2018 19:45:47 +0100 Subject: [PATCH] MDL-62275 qtype_calc*: improve validation of formulae Many thanks to Marina Glancy for helping with this. --- .../type/calculated/datasetitems_form.php | 2 +- .../type/calculated/edit_calculated_form.php | 6 +- question/type/calculated/question.php | 1 + question/type/calculated/questiontype.php | 82 +++++++++++++------ .../tests/formula_validation_test.php | 17 ++++ .../calculated/tests/questiontype_test.php | 33 ++++++++ .../type/calculatedmulti/db/upgradelib.php | 2 +- .../edit_calculatedmulti_form.php | 6 +- .../type/calculatedmulti/questiontype.php | 29 +++---- 9 files changed, 125 insertions(+), 53 deletions(-) diff --git a/question/type/calculated/datasetitems_form.php b/question/type/calculated/datasetitems_form.php index 348289420e632..08b8d56dca097 100644 --- a/question/type/calculated/datasetitems_form.php +++ b/question/type/calculated/datasetitems_form.php @@ -313,7 +313,7 @@ protected function definition() { // Decode equations in question text. $qtext = $this->qtypeobj->substitute_variables( $this->question->questiontext, $data); - $textequations = $this->qtypeobj->find_math_equations($qtext); + $textequations = $this->qtypeobj->find_formulas($qtext); if ($textequations != '' && count($textequations) > 0 ) { $mform->addElement('static', "divider1[{$j}]", '', 'Formulas {=..} in question text'); diff --git a/question/type/calculated/edit_calculated_form.php b/question/type/calculated/edit_calculated_form.php index dcc48a10ef1b2..bc76205b12d96 100644 --- a/question/type/calculated/edit_calculated_form.php +++ b/question/type/calculated/edit_calculated_form.php @@ -58,10 +58,8 @@ public function __construct($submiturl, $question, $category, $contexts, if (isset($this->question->id)) { // Remove prefix #{..}# if exists. $this->initialname = $question->name; - $regs= array(); - if (preg_match('~#\{([^[:space:]]*)#~', $question->name , $regs)) { - $question->name = str_replace($regs[0], '', $question->name); - }; + $question->name = question_bank::get_qtype($this->qtype()) + ->clean_technical_prefix_from_question_name($question->name); } } parent::__construct($submiturl, $question, $category, $contexts, $formeditable); diff --git a/question/type/calculated/question.php b/question/type/calculated/question.php index 99cd17120f7ef..6fb1cf1216623 100644 --- a/question/type/calculated/question.php +++ b/question/type/calculated/question.php @@ -28,6 +28,7 @@ require_once($CFG->dirroot . '/question/type/questionbase.php'); require_once($CFG->dirroot . '/question/type/numerical/question.php'); +require_once($CFG->dirroot . '/question/type/calculated/questiontype.php'); /** * Represents a calculated question. diff --git a/question/type/calculated/questiontype.php b/question/type/calculated/questiontype.php index 4426c6205e61d..d8e4165f4d4d2 100644 --- a/question/type/calculated/questiontype.php +++ b/question/type/calculated/questiontype.php @@ -38,8 +38,20 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class qtype_calculated extends question_type { - /** Regular expression that finds the formulas in content. */ - const FORMULAS_IN_TEXT_REGEX = '~\{=([^{}]*(?:\{[^{}]+}[^{}]*)*)\}~'; + /** + * @const string a placeholder is a letter, followed by almost any characters. (This should probably be restricted more.) + */ + const PLACEHOLDER_REGEX_PART = '[[:alpha:]][^>} <`{"\']*'; + + /** + * @const string REGEXP for a placeholder, wrapped in its {...} delimiters, with capturing brackets around the name. + */ + const PLACEHODLER_REGEX = '~\{(' . self::PLACEHOLDER_REGEX_PART . ')\}~'; + + /** + * @const string Regular expression that finds the formulas in content, with capturing brackets to get the forumlas. + */ + const FORMULAS_IN_TEXT_REGEX = '~\{=([^{}]*(?:\{' . self::PLACEHOLDER_REGEX_PART . '\}[^{}]*)*)\}~'; const MAX_DATASET_ITEMS = 100; @@ -486,6 +498,15 @@ protected function validate_question_data($question) { } } + /** + * Remove prefix #{..}# if exists. + * @param $name a question name, + * @return string the cleaned up question name. + */ + public function clean_technical_prefix_from_question_name($name) { + return preg_replace('~#\{([^[:space:]]*)#~', '', $name); + } + /** * This method prepare the $datasets in a format similar to dadatesetdefinitions_form.php * so that they can be saved @@ -553,11 +574,7 @@ public function addnamecategory(&$question) { AND a.category != 0 AND b.question = ? ORDER BY a.name ", array($question->id)); - $questionname = $question->name; - $regs= array(); - if (preg_match('~#\{([^[:space:]]*)#~', $questionname , $regs)) { - $questionname = str_replace($regs[0], '', $questionname); - }; + $questionname = $this->clean_technical_prefix_from_question_name($question->name); if (!empty($categorydatasetdefs)) { // There is at least one with the same name. @@ -1527,15 +1544,28 @@ public function dataset_options_from_database($form, $name, $prefix = '', : ''); } + /** + * Find the names of all datasets mentioned in a piece of question content like the question text. + * @param $text the text to analyse. + * @return array with dataset name for both key and value. + */ public function find_dataset_names($text) { - // Returns the possible dataset names found in the text as an array. - // The array has the dataset name for both key and value. - $datasetnames = array(); - while (preg_match('~\\{([[:alpha:]][^>} <{"\']*)\\}~', $text, $regs)) { - $datasetnames[$regs[1]] = $regs[1]; - $text = str_replace($regs[0], '', $text); - } - return $datasetnames; + preg_match_all(self::PLACEHODLER_REGEX, $text, $matches); + return array_combine($matches[1], $matches[1]); + } + + /** + * Find all the formulas in a bit of text. + * + * For example, called with "What is {a} plus {b}? (Hint, it is not {={a}*{b}}.)" this + * returns ['{a}*{b}']. + * + * @param $text text to analyse. + * @return array where they keys an values are the formulas. + */ + public function find_formulas($text) { + preg_match_all(self::FORMULAS_IN_TEXT_REGEX, $text, $matches); + return array_combine($matches[1], $matches[1]); } /** @@ -1908,13 +1938,17 @@ function qtype_calculated_calculate_answer($formula, $individualdata, * @return string|boolean false if there are no problems. Otherwise a string error message. */ function qtype_calculated_find_formula_errors($formula) { + foreach (['//', '/*', '#', ''] as $commentstart) { + if (strpos($formula, $commentstart) !== false) { + return get_string('illegalformulasyntax', 'qtype_calculated', $commentstart); + } + } + // Validates the formula submitted from the question edit page. // Returns false if everything is alright // otherwise it constructs an error message. - // Strip away dataset names. - while (preg_match('~\\{[[:alpha:]][^>} <{"\']*\\}~', $formula, $regs)) { - $formula = str_replace($regs[0], '1', $formula); - } + // Strip away dataset names. Use 1.0 to catch illegal concatenation like {a}{b}. + $formula = preg_replace(qtype_calculated::PLACEHODLER_REGEX, '1.0', $formula); // Strip away empty space and lowercase it. $formula = strtolower(str_replace(' ', '', $formula)); @@ -1978,14 +2012,14 @@ function qtype_calculated_find_formula_errors($formula) { return get_string('unsupportedformulafunction', 'qtype_calculated', $regs[2]); } - // Exchange the function call with '1' and then check for + // Exchange the function call with '1.0' and then check for // another function call... if ($regs[1]) { // The function call is proceeded by an operator. - $formula = str_replace($regs[0], $regs[1] . '1', $formula); + $formula = str_replace($regs[0], $regs[1] . '1.0', $formula); } else { // The function call starts the formula. - $formula = preg_replace("~^{$regs[2]}\\([^)]*\\)~", '1', $formula); + $formula = preg_replace('~^' . preg_quote($regs[2], '~') . '\([^)]*\)~', '1.0', $formula); } } @@ -2003,10 +2037,10 @@ function qtype_calculated_find_formula_errors($formula) { * @return string|boolean false if there are no problems. Otherwise a string error message. */ function qtype_calculated_find_formula_errors_in_text($text) { - preg_match_all(qtype_calculated::FORMULAS_IN_TEXT_REGEX, $text, $matches); + $formulas = question_bank::get_qtype('calculated')->find_formulas($text); $errors = array(); - foreach ($matches[1] as $match) { + foreach ($formulas as $match) { $error = qtype_calculated_find_formula_errors($match); if ($error) { $errors[] = $error; diff --git a/question/type/calculated/tests/formula_validation_test.php b/question/type/calculated/tests/formula_validation_test.php index d7ff76ed18e04..4f6cb4ebb69b0 100644 --- a/question/type/calculated/tests/formula_validation_test.php +++ b/question/type/calculated/tests/formula_validation_test.php @@ -46,6 +46,14 @@ public function test_simple_equations_ok() { $this->assertFalse(qtype_calculated_find_formula_errors('1 + 1')); $this->assertFalse(qtype_calculated_find_formula_errors('{x} + {y}')); $this->assertFalse(qtype_calculated_find_formula_errors('{x}*{y}')); + $this->assertFalse(qtype_calculated_find_formula_errors('{x}*({y}+1)')); + } + + public function test_simple_equations_errors() { + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('{a{b}}')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('{a{b}}')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('{a}({b})')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('2({b})')); } public function test_safe_functions_ok() { @@ -59,6 +67,15 @@ public function test_safe_functions_ok() { $this->assertFalse(qtype_calculated_find_formula_errors('max(1.0, 1.0, 2, 3)')); } + public function test_php_comments_blocked() { + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('# No need for this.')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('/* Also blocked. */')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('1 + 1 /* Blocked too. */')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('// As is this.')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('1/*2')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('/*{a*///{x}}')); + } + public function test_dangerous_functions_blocked() { $this->assert_nonempty_string(qtype_calculated_find_formula_errors('eval(1)')); $this->assert_nonempty_string(qtype_calculated_find_formula_errors('system(1)')); diff --git a/question/type/calculated/tests/questiontype_test.php b/question/type/calculated/tests/questiontype_test.php index a233c2f3a241e..d6d924febb8e8 100644 --- a/question/type/calculated/tests/questiontype_test.php +++ b/question/type/calculated/tests/questiontype_test.php @@ -106,4 +106,37 @@ public function test_get_possible_responses_no_star() { ), ), $this->qtype->get_possible_responses($q)); } + + public function test_placehodler_regex() { + preg_match_all(qtype_calculated::PLACEHODLER_REGEX, '= {={a} + {b}}', $matches); + $this->assertEquals([['{a}', '{b}'], ['a', 'b']], $matches); + } + + public function test_formulas_in_text_regex() { + preg_match_all(qtype_calculated::FORMULAS_IN_TEXT_REGEX, '= {={a} + {b}}', $matches); + $this->assertEquals([['{={a} + {b}}'], ['{a} + {b}']], $matches); + } + + public function test_find_dataset_names() { + $this->assertEquals([], $this->qtype->find_dataset_names('Frog.')); + + $this->assertEquals(['a' => 'a', 'b' => 'b'], + $this->qtype->find_dataset_names('= {={a} + {b}}')); + + $this->assertEquals(['a' => 'a', 'b' => 'b'], + $this->qtype->find_dataset_names('What is {a} plus {b}? (Hint, it is not {={a}*{b}}.)')); + + $this->assertEquals(['a' => 'a', 'b' => 'b', 'c' => 'c'], + $this->qtype->find_dataset_names(' +

If called with $a = {a} and $b = {b}, what does this PHP function return?

+ 
+                        /**
+                         * What does this do?
+                         */
+                        function mystery($a, $b) {
+                            return {c}*$a + $b;
+                        }
+
+ ')); + } } diff --git a/question/type/calculatedmulti/db/upgradelib.php b/question/type/calculatedmulti/db/upgradelib.php index ed4cb25bee1ab..d17d43c1aa688 100644 --- a/question/type/calculatedmulti/db/upgradelib.php +++ b/question/type/calculatedmulti/db/upgradelib.php @@ -317,7 +317,7 @@ protected function substitute_values_pretty($text) { */ public function replace_expressions_in_text($text, $length = null, $format = null) { $vs = $this; // Can't see to use $this in a PHP closure. - $text = preg_replace_callback('~\{=([^{}]*(?:\{[^{}]+}[^{}]*)*)}~', + $text = preg_replace_callback(qtype_calculated::FORMULAS_IN_TEXT_REGEX, function ($matches) use ($vs, $format, $length) { return $vs->format_float($vs->calculate($matches[1]), $length, $format); }, $text); diff --git a/question/type/calculatedmulti/edit_calculatedmulti_form.php b/question/type/calculatedmulti/edit_calculatedmulti_form.php index 5801fa6aa5ced..31db47fb44497 100644 --- a/question/type/calculatedmulti/edit_calculatedmulti_form.php +++ b/question/type/calculatedmulti/edit_calculatedmulti_form.php @@ -54,10 +54,8 @@ public function __construct($submiturl, $question, $category, if (isset($this->question->id)) { // Remove prefix #{..}# if exists. $this->initialname = $question->name; - $regs= array(); - if (preg_match('~#\{([^[:space:]]*)#~', $question->name , $regs)) { - $question->name = str_replace($regs[0], '', $question->name); - }; + $question->name = question_bank::get_qtype('calculated') + ->clean_technical_prefix_from_question_name($question->name); } } parent::__construct($submiturl, $question, $category, $contexts, $formeditable); diff --git a/question/type/calculatedmulti/questiontype.php b/question/type/calculatedmulti/questiontype.php index 72d6306763e46..377ae22a1e8f9 100644 --- a/question/type/calculatedmulti/questiontype.php +++ b/question/type/calculatedmulti/questiontype.php @@ -231,36 +231,27 @@ public function comment_header($question) { public function comment_on_datasetitems($qtypeobj, $questionid, $questiontext, $answers, $data, $number) { - global $DB; + $comment = new stdClass(); $comment->stranswers = array(); $comment->outsidelimit = false; $comment->answers = array(); $answers = fullclone($answers); - $errors = ''; - $delimiter = ': '; foreach ($answers as $key => $answer) { - $anssubstituted = $this->substitute_variables($answer->answer, $data); // Evaluate the equations i.e {=5+4). - $anstext = ''; - $anstextremaining = $anssubstituted; - while (preg_match('~\{=([^[:space:]}]*)}~', $anstextremaining, $regs1)) { - $anstextsplits = explode($regs1[0], $anstextremaining, 2); - $anstext =$anstext.$anstextsplits[0]; - $anstextremaining = $anstextsplits[1]; - if (empty($regs1[1])) { - $str = ''; + $anssubstituted = $this->substitute_variables($answer->answer, $data); + $formulas = $this->find_formulas($anssubstituted); + $replaces = []; + foreach ($formulas as $formula) { + if ($formulaerrors = qtype_calculated_find_formula_errors($formula)) { + $str = $formulaerrors; } else { - if ($formulaerrors = qtype_calculated_find_formula_errors($regs1[1])) { - $str=$formulaerrors; - } else { - eval('$str = '.$regs1[1].';'); - } + eval('$str = ' . $formula . ';'); } - $anstext = $anstext.$str; + $replaces[$formula] = $str; } - $anstext .= $anstextremaining; + $anstext = str_replace(arary_keys($replaces), arary_values($replaces), $anssubstituted); $comment->stranswers[$key] = $anssubstituted.'
'.$anstext; } return fullclone($comment);