L-electron-Rare/moodle
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

MDL-62275 qtype_calc*: improve validation of formulae

Browse Source 
Many thanks to Marina Glancy for helping with this.
This commit is contained in:
Tim Hunt
2018-05-11 01:33:54 +02:00
committed by Eloy Lafuente (stronk7)
parent de464f7105
commit 9d1bc62c35
9 changed files with 125 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files
question/type/calculated/datasetitems_form.php
+1 -1
View File
@@ -313,7 +313,7 @@ class question_dataset_dependent_items_form extends question_wizard_form {
// 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
+2 -4
View File
@@ -58,10 +58,8 @@ class qtype_calculated_edit_form extends qtype_numerical_edit_form {
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
+1
View File
@@ -28,6 +28,7 @@ defined('MOODLE_INTERNAL') || die();
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
+58 -24
View File
@@ -38,8 +38,20 @@ require_once($CFG->dirroot . '/question/type/numerical/question.php');
* @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 @@ class qtype_calculated extends question_type {
}
}
/**
* 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 @@ class qtype_calculated extends question_type {
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.
@@ -1530,15 +1547,28 @@ class qtype_calculated extends question_type {
: '');
}
/**
* 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]);
}
/**
@@ -1921,13 +1951,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));
@@ -1991,14 +2025,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);
}
}
@@ -2016,10 +2050,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
+17
View File
@@ -46,6 +46,14 @@ class qtype_calculated_formula_validation_testcase extends basic_testcase {
$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 @@ class qtype_calculated_formula_validation_testcase extends basic_testcase {
$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
+33
View File
@@ -127,4 +127,37 @@ class qtype_calculated_test extends advanced_testcase {
$this->assertEquals("Lorem ipsum dolor...", $this->qtype->get_short_question_name($longquestionname, 20));
$this->assertEquals("Lorem ipsum", $this->qtype->get_short_question_name($shortquestionname, 20));
}
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
+1 -1
View File
@@ -317,7 +317,7 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u
*/
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
+2 -4
View File
@@ -54,10 +54,8 @@ class qtype_calculatedmulti_edit_form extends question_edit_form {
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
+10 -19
View File
@@ -231,36 +231,27 @@ class qtype_calculatedmulti extends qtype_calculated {
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);