MDL-62275 qtype_calc*: improve validation of formulae

Many thanks to Marina Glancy for helping with this.

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');

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);

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.

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;

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)'));

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('
+                        <p>If called with $a = {a} and $b = {b}, what does this PHP function return?</p>
+                        <pre>
+                        /**
+                         * What does this do?
+                         */
+                        function mystery($a, $b) {
+                            return {c}*$a + $b;
+                        }
+                        </pre>
+                        '));
+    }
 }

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);

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);

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.'<br/>'.$anstext;
         }
         return fullclone($comment);