From 88ec9f308da6a4bc7a735458cdf72648357d501d Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Thu, 26 Jun 2014 15:37:33 +0100 Subject: [PATCH] MDL-46148 qtype_calculated: validate formulas everywhere. --- .../type/calculated/edit_calculated_form.php | 45 +++++++------ question/type/calculated/questiontype.php | 50 ++++++++++++++- .../edit_calculatedmulti_form.php | 64 ++++++++----------- .../type/calculatedmulti/questiontype.php | 7 ++ 4 files changed, 104 insertions(+), 62 deletions(-) diff --git a/question/type/calculated/edit_calculated_form.php b/question/type/calculated/edit_calculated_form.php index 853030d383c..532f056de9e 100644 --- a/question/type/calculated/edit_calculated_form.php +++ b/question/type/calculated/edit_calculated_form.php @@ -216,36 +216,39 @@ class qtype_calculated_edit_form extends qtype_numerical_edit_form { return 'calculated'; } - 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]; - } - } + /** + * 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. + $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['answeroptions['.$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/questiontype.php b/question/type/calculated/questiontype.php index 1ce68e8fe5e..414470de734 100644 --- a/question/type/calculated/questiontype.php +++ b/question/type/calculated/questiontype.php @@ -484,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 @@ -497,11 +527,12 @@ class qtype_calculated extends question_type { * @param int $questionfromid default = '0' */ public function preparedatasets($form , $questionfromid = '0') { + // The dataset names present in the edit_question_form and edit_calculated_form // 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 @@ -590,8 +621,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; } @@ -610,6 +648,14 @@ class qtype_calculated extends question_type { case '' : case 'question': // Coming from the first page, creating the second. if (empty($form->id)) { // or 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); diff --git a/question/type/calculatedmulti/edit_calculatedmulti_form.php b/question/type/calculatedmulti/edit_calculatedmulti_form.php index 400a1dfad6d..bd0af8269ba 100644 --- a/question/type/calculatedmulti/edit_calculatedmulti_form.php +++ b/question/type/calculatedmulti/edit_calculatedmulti_form.php @@ -232,30 +232,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; @@ -270,6 +271,7 @@ class qtype_calculatedmulti_edit_form extends question_edit_form { get_string('atleastonewildcard', 'qtype_calculated'); } } + $totalfraction = 0; $maxfraction = -1; foreach ($answers as $key => $answer) { @@ -283,27 +285,11 @@ class qtype_calculatedmulti_edit_form extends question_edit_form { } 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['answeroptions['.$key.']'])) { - $errors['answeroptions['.$key.']'] = $formulaerrors.':'.$regs1[1]; - } else { - $errors['answeroptions['.$key.']'] .= '
'.$formulaerrors.':'.$regs1[1]; - } - } - } + $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 3522e0d7d71..283ccd4b663 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) { -- 2.36.1