Skip to content

Commit 88ec9f3

Browse files
timhuntdanpoltawski
timhunt
authored and
danpoltawski
committed
MDL-46148 qtype_calculated: validate formulas everywhere.
1 parent 155bc75 commit 88ec9f3

File tree

4 files changed

+104
-62
lines changed

4 files changed

+104
-62
lines changed

Diff for: ‎question/type/calculated/edit_calculated_form.php

+24-21
Original file line numberDiff line numberDiff line change
@@ -216,36 +216,39 @@ public function qtype() {
216216
return 'calculated';
217217
}
218218

219-
public function validation($data, $files) {
220-
221-
// Verifying for errors in {=...} in question text.
222-
$qtext = "";
223-
$qtextremaining = $data['questiontext']['text'];
224-
$possibledatasets = $this->qtypeobj->find_dataset_names($data['questiontext']['text']);
225-
foreach ($possibledatasets as $name => $value) {
226-
$qtextremaining = str_replace('{'.$name.'}', '1', $qtextremaining);
227-
}
228-
while (preg_match('~\{=([^[:space:]}]*)}~', $qtextremaining, $regs1)) {
229-
$qtextsplits = explode($regs1[0], $qtextremaining, 2);
230-
$qtext = $qtext.$qtextsplits[0];
231-
$qtextremaining = $qtextsplits[1];
232-
if (!empty($regs1[1]) && $formulaerrors =
233-
qtype_calculated_find_formula_errors($regs1[1])) {
234-
if (!isset($errors['questiontext'])) {
235-
$errors['questiontext'] = $formulaerrors.':'.$regs1[1];
236-
} else {
237-
$errors['questiontext'] .= '<br/>'.$formulaerrors.':'.$regs1[1];
238-
}
239-
}
219+
/**
220+
* Validate the equations in the some question content.
221+
* @param array $errors where errors are being accumulated.
222+
* @param string $field the field being validated.
223+
* @param string $text the content of that field.
224+
* @return array the updated $errors array.
225+
*/
226+
protected function validate_text($errors, $field, $text) {
227+
$problems = qtype_calculated_find_formula_errors_in_text($text);
228+
if ($problems) {
229+
$errors[$field] = $problems;
240230
}
231+
return $errors;
232+
}
241233

234+
public function validation($data, $files) {
242235
$errors = parent::validation($data, $files);
243236

237+
// Verifying for errors in {=...} in question text.
238+
$errors = $this->validate_text($errors, 'questiontext', $data['questiontext']['text']);
239+
$errors = $this->validate_text($errors, 'generalfeedback', $data['generalfeedback']['text']);
240+
244241
// Check that the answers use datasets.
245242
$answers = $data['answer'];
246243
$mandatorydatasets = array();
247244
foreach ($answers as $key => $answer) {
245+
$problems = qtype_calculated_find_formula_errors($answer);
246+
if ($problems) {
247+
$errors['answeroptions['.$key.']'] = $problems;
248+
}
248249
$mandatorydatasets += $this->qtypeobj->find_dataset_names($answer);
250+
$errors = $this->validate_text($errors, 'feedback[' . $key . ']',
251+
$data['feedback'][$key]['text']);
249252
}
250253
if (empty($mandatorydatasets)) {
251254
foreach ($answers as $key => $answer) {

Diff for: ‎question/type/calculated/questiontype.php

+48-2
Original file line numberDiff line numberDiff line change
@@ -484,6 +484,36 @@ public function display_question_editing_page($mform, $question, $wizardnow) {
484484
$mform->display();
485485
}
486486

487+
/**
488+
* Verify that the equations in part of the question are OK.
489+
* We throw an exception here because this should have already been validated
490+
* by the form. This is just a last line of defence to prevent a question
491+
* being stored in the database if it has bad formulas. This saves us from,
492+
* for example, malicious imports.
493+
* @param string $text containing equations.
494+
*/
495+
protected function validate_text($text) {
496+
$error = qtype_calculated_find_formula_errors_in_text($text);
497+
if ($error) {
498+
throw new coding_exception($error);
499+
}
500+
}
501+
502+
/**
503+
* Verify that an answer is OK.
504+
* We throw an exception here because this should have already been validated
505+
* by the form. This is just a last line of defence to prevent a question
506+
* being stored in the database if it has bad formulas. This saves us from,
507+
* for example, malicious imports.
508+
* @param string $text containing equations.
509+
*/
510+
protected function validate_answer($answer) {
511+
$error = qtype_calculated_find_formula_errors($answer);
512+
if ($error) {
513+
throw new coding_exception($error);
514+
}
515+
}
516+
487517
/**
488518
* This method prepare the $datasets in a format similar to dadatesetdefinitions_form.php
489519
* so that they can be saved
@@ -497,11 +527,12 @@ public function display_question_editing_page($mform, $question, $wizardnow) {
497527
* @param int $questionfromid default = '0'
498528
*/
499529
public function preparedatasets($form , $questionfromid = '0') {
530+
500531
// The dataset names present in the edit_question_form and edit_calculated_form
501532
// are retrieved.
502533
$possibledatasets = $this->find_dataset_names($form->questiontext);
503534
$mandatorydatasets = array();
504-
foreach ($form->answers as $answer) {
535+
foreach ($form->answers as $key => $answer) {
505536
$mandatorydatasets += $this->find_dataset_names($answer);
506537
}
507538
// If there are identical datasetdefs already saved in the original question
@@ -590,8 +621,15 @@ public function addnamecategory(&$question) {
590621
*/
591622
public function save_question($question, $form) {
592623
global $DB;
624+
625+
if (isset($form->correctfeedback)) {
626+
$this->validate_text($form->correctfeedback['text']);
627+
$this->validate_text($form->partiallycorrectfeedback['text']);
628+
$this->validate_text($form->incorrectfeedback['text']);
629+
}
630+
593631
if ($this->wizardpagesnumber() == 1 || $question->qtype == 'calculatedsimple') {
594-
$question = parent::save_question($question, $form);
632+
$question = parent::save_question($question, $form);
595633
return $question;
596634
}
597635

@@ -610,6 +648,14 @@ public function save_question($question, $form) {
610648
case '' :
611649
case 'question': // Coming from the first page, creating the second.
612650
if (empty($form->id)) { // or a new question $form->id is empty.
651+
// Make it impossible to save bad formulas anywhere.
652+
$this->validate_text($form->questiontext['text']);
653+
$this->validate_text($form->generalfeedback['text']);
654+
foreach ($form->answer as $key => $answer) {
655+
$this->validate_answer($answer);
656+
$this->validate_text($form->feedback[$key]['text']);
657+
}
658+
613659
$question = parent::save_question($question, $form);
614660
// Prepare the datasets using default $questionfromid.
615661
$this->preparedatasets($form);

Diff for: ‎question/type/calculatedmulti/edit_calculatedmulti_form.php

+25-39
Original file line numberDiff line numberDiff line change
@@ -232,30 +232,31 @@ protected function data_preprocessing_answers($question, $withanswerfiles = fals
232232
return $question;
233233
}
234234

235+
/**
236+
* Validate the equations in the some question content.
237+
* @param array $errors where errors are being accumulated.
238+
* @param string $field the field being validated.
239+
* @param string $text the content of that field.
240+
* @return array the updated $errors array.
241+
*/
242+
protected function validate_text($errors, $field, $text) {
243+
$problems = qtype_calculated_find_formula_errors_in_text($text);
244+
if ($problems) {
245+
$errors[$field] = $problems;
246+
}
247+
return $errors;
248+
}
249+
235250
public function validation($data, $files) {
236251
$errors = parent::validation($data, $files);
237252

238253
// Verifying for errors in {=...} in question text.
239-
$qtext = '';
240-
$qtextremaining = $data['questiontext']['text'];
241-
$possibledatasets = $this->qtypeobj->find_dataset_names($data['questiontext']['text']);
242-
foreach ($possibledatasets as $name => $value) {
243-
$qtextremaining = str_replace('{'.$name.'}', '1', $qtextremaining);
244-
}
254+
$errors = $this->validate_text($errors, 'questiontext', $data['questiontext']['text']);
255+
$errors = $this->validate_text($errors, 'generalfeedback', $data['generalfeedback']['text']);
256+
$errors = $this->validate_text($errors, 'correctfeedback', $data['correctfeedback']['text']);
257+
$errors = $this->validate_text($errors, 'partiallycorrectfeedback', $data['partiallycorrectfeedback']['text']);
258+
$errors = $this->validate_text($errors, 'incorrectfeedback', $data['incorrectfeedback']['text']);
245259

246-
while (preg_match('~\{=([^[:space:]}]*)}~', $qtextremaining, $regs1)) {
247-
$qtextsplits = explode($regs1[0], $qtextremaining, 2);
248-
$qtext = $qtext.$qtextsplits[0];
249-
$qtextremaining = $qtextsplits[1];
250-
if (!empty($regs1[1]) && $formulaerrors =
251-
qtype_calculated_find_formula_errors($regs1[1])) {
252-
if (!isset($errors['questiontext'])) {
253-
$errors['questiontext'] = $formulaerrors.':'.$regs1[1];
254-
} else {
255-
$errors['questiontext'] .= '<br/>'.$formulaerrors.':'.$regs1[1];
256-
}
257-
}
258-
}
259260
$answers = $data['answer'];
260261
$answercount = 0;
261262
$maxgrade = false;
@@ -270,6 +271,7 @@ public function validation($data, $files) {
270271
get_string('atleastonewildcard', 'qtype_calculated');
271272
}
272273
}
274+
273275
$totalfraction = 0;
274276
$maxfraction = -1;
275277
foreach ($answers as $key => $answer) {
@@ -283,27 +285,11 @@ public function validation($data, $files) {
283285
}
284286
if ($trimmedanswer != '' || $answercount == 0) {
285287
// Verifying for errors in {=...} in answer text.
286-
$qanswer = '';
287-
$qanswerremaining = $trimmedanswer;
288-
$possibledatasets = $this->qtypeobj->find_dataset_names($trimmedanswer);
289-
foreach ($possibledatasets as $name => $value) {
290-
$qanswerremaining = str_replace('{'.$name.'}', '1', $qanswerremaining);
291-
}
292-
293-
while (preg_match('~\{=([^[:space:]}]*)}~', $qanswerremaining, $regs1)) {
294-
$qanswersplits = explode($regs1[0], $qanswerremaining, 2);
295-
$qanswer = $qanswer . $qanswersplits[0];
296-
$qanswerremaining = $qanswersplits[1];
297-
if (!empty($regs1[1]) && $formulaerrors =
298-
qtype_calculated_find_formula_errors($regs1[1])) {
299-
if (!isset($errors['answeroptions['.$key.']'])) {
300-
$errors['answeroptions['.$key.']'] = $formulaerrors.':'.$regs1[1];
301-
} else {
302-
$errors['answeroptions['.$key.']'] .= '<br/>'.$formulaerrors.':'.$regs1[1];
303-
}
304-
}
305-
}
288+
$errors = $this->validate_text($errors, 'answeroptions[' . $key . ']', $answer);
289+
$errors = $this->validate_text($errors, 'feedback[' . $key . ']',
290+
$data['feedback'][$key]['text']);
306291
}
292+
307293
if ($trimmedanswer != '') {
308294
if ('2' == $data['correctanswerformat'][$key] &&
309295
'0' == $data['correctanswerlength'][$key]) {

Diff for: ‎question/type/calculatedmulti/questiontype.php

+7
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,13 @@ public function save_question_options($question) {
156156
return true;
157157
}
158158

159+
protected function validate_answer($answer) {
160+
$error = qtype_calculated_find_formula_errors_in_text($answer);
161+
if ($error) {
162+
throw new coding_exception($error);
163+
}
164+
}
165+
159166
protected function make_question_instance($questiondata) {
160167
question_bank::load_question_definition_classes($this->name());
161168
if ($questiondata->options->single) {

0 commit comments

Comments
 (0)