diff --git a/question/type/calculated/edit_calculated_form.php b/question/type/calculated/edit_calculated_form.php index 8b3fbdddf0b..bddf2065865 100644 --- a/question/type/calculated/edit_calculated_form.php +++ b/question/type/calculated/edit_calculated_form.php @@ -189,36 +189,39 @@ class qtype_calculated_edit_form extends qtype_numerical_edit_form { return 'calculated'; } + /** + * Validate the equations in the some question content. + * @param array $errors where errors are being accumulated. + * @param string $field the field being validated. + * @param string $text the content of that field. + * @return array the updated $errors array. + */ + protected function validate_text($errors, $field, $text) { + $problems = qtype_calculated_find_formula_errors_in_text($text); + if ($problems) { + $errors[$field] = $problems; + } + return $errors; + } + public function validation($data, $files) { - - // verifying for errors in {=...} in question text; - $qtext = ""; - $qtextremaining = $data['questiontext']['text']; - $possibledatasets = $this->qtypeobj->find_dataset_names($data['questiontext']['text']); - foreach ($possibledatasets as $name => $value) { - $qtextremaining = str_replace('{'.$name.'}', '1', $qtextremaining); - } - while (preg_match('~\{=([^[:space:]}]*)}~', $qtextremaining, $regs1)) { - $qtextsplits = explode($regs1[0], $qtextremaining, 2); - $qtext = $qtext.$qtextsplits[0]; - $qtextremaining = $qtextsplits[1]; - if (!empty($regs1[1]) && $formulaerrors = - qtype_calculated_find_formula_errors($regs1[1])) { - if (!isset($errors['questiontext'])) { - $errors['questiontext'] = $formulaerrors.':'.$regs1[1]; - } else { - $errors['questiontext'] .= '
'.$formulaerrors.':'.$regs1[1]; - } - } - } - $errors = parent::validation($data, $files); + // Verifying for errors in {=...} in question text. + $errors = $this->validate_text($errors, 'questiontext', $data['questiontext']['text']); + $errors = $this->validate_text($errors, 'generalfeedback', $data['generalfeedback']['text']); + // Check that the answers use datasets. $answers = $data['answer']; $mandatorydatasets = array(); foreach ($answers as $key => $answer) { + $problems = qtype_calculated_find_formula_errors($answer); + if ($problems) { + $errors['answer['.$key.']'] = $problems; + } $mandatorydatasets += $this->qtypeobj->find_dataset_names($answer); + $errors = $this->validate_text($errors, 'feedback[' . $key . ']', + $data['feedback'][$key]['text']); } if (empty($mandatorydatasets)) { foreach ($answers as $key => $answer) { diff --git a/question/type/calculated/question.php b/question/type/calculated/question.php index 3ecbc9d0f7b..76811f13fa6 100644 --- a/question/type/calculated/question.php +++ b/question/type/calculated/question.php @@ -259,6 +259,7 @@ class qtype_calculated_dataset_loader { * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class qtype_calculated_variable_substituter { + /** @var array variable name => value */ protected $values; @@ -388,108 +389,11 @@ class qtype_calculated_variable_substituter { * @return string the text with values substituted. */ 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('~\{=([^{}]*(?:\{[^{}]+}[^{}]*)*)}~', + $vs = $this; // Can't use $this in a PHP closure. + $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); return $this->substitute_values_pretty($text); } - - /** - * Return an array describing any problems there are with an expression. - * Returns false if the expression is fine. - * @param string $formula an expression. - * @return array|false list of problems, or false if the exression is OK. - */ - public function get_formula_errors($formula) { - // 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 empty space and lowercase it - $formula = strtolower(str_replace(' ', '', $formula)); - - $safeoperatorchar = '-+/*%>:^\~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 MAX_DATASET_ITEMS = 100; public $wizardpagesnumber = 3; @@ -481,6 +484,36 @@ class qtype_calculated extends question_type { $mform->display(); } + /** + * Verify that the equations in part of the question are OK. + * We throw an exception here because this should have already been validated + * by the form. This is just a last line of defence to prevent a question + * being stored in the database if it has bad formulas. This saves us from, + * for example, malicious imports. + * @param string $text containing equations. + */ + protected function validate_text($text) { + $error = qtype_calculated_find_formula_errors_in_text($text); + if ($error) { + throw new coding_exception($error); + } + } + + /** + * Verify that an answer is OK. + * We throw an exception here because this should have already been validated + * by the form. This is just a last line of defence to prevent a question + * being stored in the database if it has bad formulas. This saves us from, + * for example, malicious imports. + * @param string $text containing equations. + */ + protected function validate_answer($answer) { + $error = qtype_calculated_find_formula_errors($answer); + if ($error) { + throw new coding_exception($error); + } + } + /** * This method prepare the $datasets in a format similar to dadatesetdefinitions_form.php * so that they can be saved @@ -498,7 +531,7 @@ class qtype_calculated extends question_type { // are retrieved $possibledatasets = $this->find_dataset_names($form->questiontext); $mandatorydatasets = array(); - foreach ($form->answers as $answer) { + foreach ($form->answers as $key => $answer) { $mandatorydatasets += $this->find_dataset_names($answer); } // if there are identical datasetdefs already saved in the original question. @@ -587,8 +620,15 @@ class qtype_calculated extends question_type { */ public function save_question($question, $form) { global $DB; + + if (isset($form->correctfeedback)) { + $this->validate_text($form->correctfeedback['text']); + $this->validate_text($form->partiallycorrectfeedback['text']); + $this->validate_text($form->incorrectfeedback['text']); + } + if ($this->wizardpagesnumber() == 1 || $question->qtype == 'calculatedsimple') { - $question = parent::save_question($question, $form); + $question = parent::save_question($question, $form); return $question; } @@ -607,6 +647,14 @@ class qtype_calculated extends question_type { case '' : case 'question': // coming from the first page, creating the second if (empty($form->id)) { // for a new question $form->id is empty + // Make it impossible to save bad formulas anywhere. + $this->validate_text($form->questiontext['text']); + $this->validate_text($form->generalfeedback['text']); + foreach ($form->answer as $key => $answer) { + $this->validate_answer($answer); + $this->validate_text($form->feedback[$key]['text']); + } + $question = parent::save_question($question, $form); //prepare the datasets using default $questionfromid $this->preparedatasets($form); @@ -1973,6 +2021,11 @@ function qtype_calculated_calculate_answer($formula, $individualdata, } +/** + * Validate a forumula. + * @param string $formula the formula to validate. + * @return string|boolean false if there are no problems. Otherwise a string error message. + */ function qtype_calculated_find_formula_errors($formula) { // Validates the formula submitted from the question edit page. // Returns false if everything is alright. @@ -2001,7 +2054,7 @@ function qtype_calculated_find_formula_errors($formula) { // Zero argument functions case 'pi': - if ($regs[3]) { + if (array_key_exists(3, $regs)) { return get_string('functiontakesnoargs', 'qtype_calculated', $regs[2]); } break; @@ -2062,3 +2115,26 @@ function qtype_calculated_find_formula_errors($formula) { return false; } } + +/** + * Validate all the forumulas in a bit of text. + * @param string $text the text in which to validate the formulas. + * @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); + + $errors = array(); + foreach ($matches[1] as $match) { + $error = qtype_calculated_find_formula_errors($match); + if ($error) { + $errors[] = $error; + } + } + + if ($errors) { + return implode(' ', $errors); + } + + return false; +} diff --git a/question/type/calculated/tests/formula_validation_test.php b/question/type/calculated/tests/formula_validation_test.php new file mode 100644 index 00000000000..d7ff76ed18e --- /dev/null +++ b/question/type/calculated/tests/formula_validation_test.php @@ -0,0 +1,105 @@ +. + +/** + * Unit tests for formula validation code. + * + * @package qtype_calculated + * @copyright 2014 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/question/type/calculated/questiontype.php'); + + +/** + * Unit tests for formula validation code. + * + * @copyright 2014 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class qtype_calculated_formula_validation_testcase extends basic_testcase { + protected function assert_nonempty_string($actual) { + $this->assertInternalType('string', $actual); + $this->assertNotEquals('', $actual); + } + + public function test_simple_equations_ok() { + $this->assertFalse(qtype_calculated_find_formula_errors(1)); + $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}')); + } + + public function test_safe_functions_ok() { + $this->assertFalse(qtype_calculated_find_formula_errors('abs(-1)')); + $this->assertFalse(qtype_calculated_find_formula_errors('tan(pi())')); + $this->assertFalse(qtype_calculated_find_formula_errors('log(10)')); + $this->assertFalse(qtype_calculated_find_formula_errors('log(64, 2)')); + $this->assertFalse(qtype_calculated_find_formula_errors('atan2(1.0, 1.0)')); + $this->assertFalse(qtype_calculated_find_formula_errors('max(1.0, 1.0)')); + $this->assertFalse(qtype_calculated_find_formula_errors('max(1.0, 1.0, 2.0)')); + $this->assertFalse(qtype_calculated_find_formula_errors('max(1.0, 1.0, 2, 3)')); + } + + 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)')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('base64_decode(1)')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('unserialize(1)')); + + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('cos(tan(1) + abs(cos(eval)) * pi())')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('eval (CONSTANTREADASSTRING)')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors("eval \t ()")); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('"eval"()')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('?>assert_nonempty_string(qtype_calculated_find_formula_errors('?>assert_nonempty_string(qtype_calculated_find_formula_errors('abs(-1, 1)')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('abs()')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('pi(1)')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('log()')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('log(64, 2, 3)')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('atan2(1.0)')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('atan2(1.0, 1.0, 2.0)')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors('max(1.0)')); + } + + public function test_validation_of_formulas_in_text_ok() { + $this->assertFalse(qtype_calculated_find_formula_errors_in_text( + '

Look no equations.

')); + $this->assertFalse(qtype_calculated_find_formula_errors_in_text( + '

Simple variable: {x}.

')); + $this->assertFalse(qtype_calculated_find_formula_errors_in_text( + '

This is an equation: {=1+1}, as is this: {={x}+{y}}.

' . + '

Here is a more complex one: {=sin(2*pi()*{theta})}.

')); + } + + public function test_validation_of_formulas_in_text_bad_function() { + $this->assert_nonempty_string(qtype_calculated_find_formula_errors_in_text( + '

This is an equation: {=eval(1)}.

')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors_in_text( + '

Good: {=1+1}, bad: {=eval(1)}, good: {={x}+{y}}.

')); + $this->assert_nonempty_string(qtype_calculated_find_formula_errors_in_text( + '

Bad: {=eval(1)}, bad: {=system(1)}.

')); + } +} diff --git a/question/type/calculatedmulti/edit_calculatedmulti_form.php b/question/type/calculatedmulti/edit_calculatedmulti_form.php index 7a802eeb8d0..4e9369a26b3 100644 --- a/question/type/calculatedmulti/edit_calculatedmulti_form.php +++ b/question/type/calculatedmulti/edit_calculatedmulti_form.php @@ -218,30 +218,31 @@ class qtype_calculatedmulti_edit_form extends question_edit_form { return $question; } + /** + * Validate the equations in the some question content. + * @param array $errors where errors are being accumulated. + * @param string $field the field being validated. + * @param string $text the content of that field. + * @return array the updated $errors array. + */ + protected function validate_text($errors, $field, $text) { + $problems = qtype_calculated_find_formula_errors_in_text($text); + if ($problems) { + $errors[$field] = $problems; + } + return $errors; + } + public function validation($data, $files) { $errors = parent::validation($data, $files); //verifying for errors in {=...} in question text; - $qtext = ''; - $qtextremaining = $data['questiontext']['text']; - $possibledatasets = $this->qtypeobj->find_dataset_names($data['questiontext']['text']); - foreach ($possibledatasets as $name => $value) { - $qtextremaining = str_replace('{'.$name.'}', '1', $qtextremaining); - } + $errors = $this->validate_text($errors, 'questiontext', $data['questiontext']['text']); + $errors = $this->validate_text($errors, 'generalfeedback', $data['generalfeedback']['text']); + $errors = $this->validate_text($errors, 'correctfeedback', $data['correctfeedback']['text']); + $errors = $this->validate_text($errors, 'partiallycorrectfeedback', $data['partiallycorrectfeedback']['text']); + $errors = $this->validate_text($errors, 'incorrectfeedback', $data['incorrectfeedback']['text']); - while (preg_match('~\{=([^[:space:]}]*)}~', $qtextremaining, $regs1)) { - $qtextsplits = explode($regs1[0], $qtextremaining, 2); - $qtext = $qtext.$qtextsplits[0]; - $qtextremaining = $qtextsplits[1]; - if (!empty($regs1[1]) && $formulaerrors = - qtype_calculated_find_formula_errors($regs1[1])) { - if (!isset($errors['questiontext'])) { - $errors['questiontext'] = $formulaerrors.':'.$regs1[1]; - } else { - $errors['questiontext'] .= '
'.$formulaerrors.':'.$regs1[1]; - } - } - } $answers = $data['answer']; $answercount = 0; $maxgrade = false; @@ -256,6 +257,7 @@ class qtype_calculatedmulti_edit_form extends question_edit_form { get_string('atleastonewildcard', 'qtype_calculated'); } } + $totalfraction = 0; $maxfraction = -1; foreach ($answers as $key => $answer) { @@ -268,28 +270,12 @@ class qtype_calculatedmulti_edit_form extends question_edit_form { $errors['fraction['.$key.']'] = get_string('errgradesetanswerblank', 'qtype_multichoice'); } if ($trimmedanswer != '' || $answercount == 0) { - //verifying for errors in {=...} in answer text; - $qanswer = ''; - $qanswerremaining = $trimmedanswer; - $possibledatasets = $this->qtypeobj->find_dataset_names($trimmedanswer); - foreach ($possibledatasets as $name => $value) { - $qanswerremaining = str_replace('{'.$name.'}', '1', $qanswerremaining); - } - - while (preg_match('~\{=([^[:space:]}]*)}~', $qanswerremaining, $regs1)) { - $qanswersplits = explode($regs1[0], $qanswerremaining, 2); - $qanswer = $qanswer . $qanswersplits[0]; - $qanswerremaining = $qanswersplits[1]; - if (!empty($regs1[1]) && $formulaerrors = - qtype_calculated_find_formula_errors($regs1[1])) { - if (!isset($errors['answer['.$key.']'])) { - $errors['answer['.$key.']'] = $formulaerrors.':'.$regs1[1]; - } else { - $errors['answer['.$key.']'] .= '
'.$formulaerrors.':'.$regs1[1]; - } - } - } + // Verifying for errors in {=...} in answer text. + $errors = $this->validate_text($errors, 'answeroptions[' . $key . ']', $answer); + $errors = $this->validate_text($errors, 'feedback[' . $key . ']', + $data['feedback'][$key]['text']); } + if ($trimmedanswer != '') { if ('2' == $data['correctanswerformat'][$key] && '0' == $data['correctanswerlength'][$key]) { diff --git a/question/type/calculatedmulti/questiontype.php b/question/type/calculatedmulti/questiontype.php index 220d1bcaf8f..5c0179fab95 100644 --- a/question/type/calculatedmulti/questiontype.php +++ b/question/type/calculatedmulti/questiontype.php @@ -156,6 +156,13 @@ class qtype_calculatedmulti extends qtype_calculated { return true; } + protected function validate_answer($answer) { + $error = qtype_calculated_find_formula_errors_in_text($answer); + if ($error) { + throw new coding_exception($error); + } + } + protected function make_question_instance($questiondata) { question_bank::load_question_definition_classes($this->name()); if ($questiondata->options->single) {