'));
+ + Y.Escape.html(this.get('stacktrace'))+''));
if (M.cfg.developerdebug) {
content.all('.moodle-exception-param').removeClass('hidden');
}
@@ -425,6 +425,7 @@ Y.extend(AJAXEXCEPTION, DIALOGUE, {
reproductionlink : {
setter : function(link) {
if (link !== null) {
+ link = Y.Escape.html(link);
link = ''+link.replace(M.cfg.wwwroot, '')+'';
}
return link;
diff --git a/lib/yui/src/notification/meta/notification.json b/lib/yui/src/notification/meta/notification.json
index 739c8279ec7..620897294aa 100644
--- a/lib/yui/src/notification/meta/notification.json
+++ b/lib/yui/src/notification/meta/notification.json
@@ -5,6 +5,7 @@
"node",
"panel",
"event-key",
+ "escape",
"dd-plugin"
]
}
diff --git a/mod/forum/post_form.php b/mod/forum/post_form.php
index 20d6bf29607..73ea0dae68c 100644
--- a/mod/forum/post_form.php
+++ b/mod/forum/post_form.php
@@ -147,13 +147,21 @@ class mod_forum_post_form extends moodleform {
$mform->setConstants(array('timestart'=> 0, 'timeend'=>0));
}
- if (groups_get_activity_groupmode($cm, $course)) { // hack alert
+ if ($groupmode = groups_get_activity_groupmode($cm, $course)) { // hack alert
$groupdata = groups_get_activity_allowed_groups($cm);
$groupcount = count($groupdata);
+ $groupinfo = array();
$modulecontext = context_module::instance($cm->id);
- $contextcheck = has_capability('mod/forum:movediscussions', $modulecontext) && empty($post->parent) && $groupcount;
- if ($contextcheck) {
+
+ // Check whether the user has access to all groups in this forum from the accessallgroups cap.
+ if ($groupmode == VISIBLEGROUPS || has_capability('moodle/site:accessallgroups', $modulecontext)) {
+ // Only allow posting to all groups if the user has access to all groups.
$groupinfo = array('0' => get_string('allparticipants'));
+ $groupcount++;
+ }
+
+ $contextcheck = has_capability('mod/forum:movediscussions', $modulecontext) && empty($post->parent) && $groupcount > 1;
+ if ($contextcheck) {
foreach ($groupdata as $grouptemp) {
$groupinfo[$grouptemp->id] = $grouptemp->name;
}
diff --git a/mod/forum/tests/behat/separate_group_discussions.feature b/mod/forum/tests/behat/separate_group_discussions.feature
new file mode 100644
index 00000000000..3b5978f4441
--- /dev/null
+++ b/mod/forum/tests/behat/separate_group_discussions.feature
@@ -0,0 +1,84 @@
+@mod @mod_forum
+Feature: Posting to all groups in a separate group discussion is restricted to users with access to all groups
+ In order to post to all groups in a forum with separate groups
+ As a teacher
+ I need to have the accessallgroups capability or be a member of all of the groups
+
+ Background:
+ Given the following "users" exist:
+ | username | firstname | lastname | email |
+ | teacher1 | Teacher | 1 | teacher1@asd.com |
+ | noneditor1 | Non-editing teacher | 1 | noneditor1@asd.com |
+ | noneditor2 | Non-editing teacher | 2 | noneditor2@asd.com |
+ | student1 | Student | 1 | student1@asd.com |
+ And the following "courses" exist:
+ | fullname | shortname | category |
+ | Course 1 | C1 | 0 |
+ And the following "course enrolments" exist:
+ | user | course | role |
+ | teacher1 | C1 | editingteacher |
+ | noneditor1 | C1 | teacher |
+ | noneditor2 | C1 | teacher |
+ | student1 | C1 | student |
+ And the following "groups" exist:
+ | name | course | idnumber |
+ | Group A | C1 | G1 |
+ | Group B | C1 | G2 |
+ | Group C | C1 | G3 |
+ And the following "group members" exist:
+ | user | group |
+ | teacher1 | G1 |
+ | teacher1 | G2 |
+ | noneditor1 | G1 |
+ | noneditor1 | G2 |
+ | noneditor1 | G3 |
+ | noneditor2 | G1 |
+ | noneditor2 | G2 |
+ | student1 | G1 |
+ | student1 | G2 |
+ And I log in as "teacher1"
+ And I follow "Course 1"
+ And I turn editing mode on
+ And I add a "Forum" to section "1" and I fill the form with:
+ | Forum name | Standard forum name |
+ | Forum type | Standard forum for general use |
+ | Description | Standard forum description |
+ | Group mode | Separate groups |
+ And I log out
+
+ Scenario: Teacher with accessallgroups can post in all groups
+ Given I log in as "teacher1"
+ And I follow "Course 1"
+ And I follow "Standard forum name"
+ When I click on "Add a new discussion topic" "button"
+ Then the "Group" select box should contain "All participants"
+ And the "Group" select box should contain "Group A"
+ And the "Group" select box should contain "Group B"
+
+ @javascript
+ Scenario: Teacher in all groups but without accessallgroups can only post in their groups
+ And I log in as "admin"
+ And I set the following system permissions of "Non-editing teacher" role:
+ | moodle/site:accessallgroups | Prohibit |
+ And I log out
+ Given I log in as "noneditor1"
+ And I follow "Course 1"
+ And I follow "Standard forum name"
+ When I click on "Add a new discussion topic" "button"
+ Then the "Group" select box should not contain "All participants"
+ And the "Group" select box should contain "Group A"
+ And the "Group" select box should contain "Group B"
+
+ @javascript
+ Scenario: Teacher in some groups and without accessallgroups can only post in their groups
+ And I log in as "admin"
+ And I set the following system permissions of "Non-editing teacher" role:
+ | moodle/site:accessallgroups | Prohibit |
+ And I log out
+ Given I log in as "noneditor1"
+ And I follow "Course 1"
+ And I follow "Standard forum name"
+ When I click on "Add a new discussion topic" "button"
+ Then the "Group" select box should not contain "All participants"
+ And the "Group" select box should contain "Group A"
+ And the "Group" select box should contain "Group B"
diff --git a/mod/forum/tests/behat/separate_group_single_group_discussions.feature b/mod/forum/tests/behat/separate_group_single_group_discussions.feature
new file mode 100644
index 00000000000..d8745d11a77
--- /dev/null
+++ b/mod/forum/tests/behat/separate_group_single_group_discussions.feature
@@ -0,0 +1,84 @@
+@mod @mod_forum
+Feature: Posting to groups in a separate group discussion when restricted to groupings
+ In order to post to groups in a forum with separate groups and groupings
+ As a teacher
+ I need to have groups configured to post to a group
+
+ Background:
+ Given the following "users" exist:
+ | username | firstname | lastname | email |
+ | teacher1 | teacher1 | teacher1 | teacher1@asd.com |
+ And the following "courses" exist:
+ | fullname | shortname | category |
+ | Course 1 | C1 | 0 |
+ And the following "course enrolments" exist:
+ | user | course | role |
+ | teacher1 | C1 | teacher |
+ And the following "groups" exist:
+ | name | course | idnumber |
+ | G1G1 | C1 | G1G1 |
+ | G1G2 | C1 | G1G2 |
+ | G2G1 | C1 | G2G1 |
+ And the following "groupings" exist:
+ | name | course | idnumber |
+ | G1 | C1 | G1 |
+ | G2 | C1 | G2 |
+ And the following "group members" exist:
+ | user | group |
+ | teacher1 | G1G1 |
+ | teacher1 | G1G2 |
+ | teacher1 | G2G1 |
+ And the following "grouping groups" exist:
+ | grouping | group |
+ | G1 | G1G1 |
+ | G1 | G1G2 |
+ | G2 | G2G1 |
+ And I log in as "admin"
+ And I follow "Course 1"
+ And I turn editing mode on
+ And I add a "Forum" to section "1" and I fill the form with:
+ | Forum name | Multiple groups forum |
+ | Forum type | Standard forum for general use |
+ | Description | Standard forum description |
+ | Group mode | Separate groups |
+ | Grouping | G1 |
+ And I add a "Forum" to section "1" and I fill the form with:
+ | Forum name | Single groups forum |
+ | Forum type | Standard forum for general use |
+ | Description | Standard forum description |
+ | Group mode | Separate groups |
+ | Grouping | G2 |
+ And I log out
+
+ Scenario: Teacher with accessallgroups can post in all groups
+ Given I log in as "teacher1"
+ And I follow "Course 1"
+ And I follow "Multiple groups forum"
+ When I click on "Add a new discussion topic" "button"
+ Then the "Group" select box should contain "All participants"
+ And the "Group" select box should contain "G1G1"
+ And the "Group" select box should contain "G1G2"
+ And I follow "Course 1"
+ And I follow "Single groups forum"
+ And I click on "Add a new discussion topic" "button"
+ And the "Group" select box should contain "All participants"
+ And the "Group" select box should contain "G2G1"
+
+ @javascript
+ Scenario: Teacher in all groups but without accessallgroups can post in either group but not to All Participants
+ And I log in as "admin"
+ And I set the following system permissions of "Non-editing teacher" role:
+ | moodle/site:accessallgroups | Prohibit |
+ And I log out
+ Given I log in as "teacher1"
+ And I follow "Course 1"
+ And I follow "Multiple groups forum"
+ When I click on "Add a new discussion topic" "button"
+ Then the "Group" select box should not contain "All participants"
+ And the "Group" select box should contain "G1G1"
+ And the "Group" select box should contain "G1G2"
+ And I follow "Course 1"
+ And I follow "Single groups forum"
+ And I click on "Add a new discussion topic" "button"
+ And I should see "G2G1"
+ And "Group" "select" should not exist
diff --git a/mod/imscp/locallib.php b/mod/imscp/locallib.php
index b3dbafbf9a8..888802a648a 100644
--- a/mod/imscp/locallib.php
+++ b/mod/imscp/locallib.php
@@ -105,9 +105,11 @@ function imscp_parse_structure($imscp, $context) {
*/
function imscp_parse_manifestfile($manifestfilecontents, $imscp, $context) {
$doc = new DOMDocument();
+ $oldentities = libxml_disable_entity_loader(true);
if (!$doc->loadXML($manifestfilecontents, LIBXML_NONET)) {
return null;
}
+ libxml_disable_entity_loader($oldentities);
// we put this fake URL as base in order to detect path changes caused by xml:base attributes
$doc->documentURI = 'http://grrr/';
@@ -205,10 +207,14 @@ function imscp_recursive_href($manifestfilename, $imscp, $context) {
if (!$manifestfile = $fs->get_file($context->id, 'mod_imscp', 'content', $imscp->revision, $dirname, $filename)) {
return null;
}
+
$doc = new DOMDocument();
+ $oldentities = libxml_disable_entity_loader(true);
if (!$doc->loadXML($manifestfile->get_content(), LIBXML_NONET)) {
return null;
}
+ libxml_disable_entity_loader($oldentities);
+
$xmlresources = $doc->getElementsByTagName('resource');
foreach ($xmlresources as $res) {
if (!$href = $res->attributes->getNamedItem('href')) {
diff --git a/mod/lti/service.php b/mod/lti/service.php
index 60bcfa2caf8..d3645b4140c 100644
--- a/mod/lti/service.php
+++ b/mod/lti/service.php
@@ -53,7 +53,14 @@ if ($sharedsecret === false) {
throw new Exception('Message signature not valid');
}
-$xml = new SimpleXMLElement($rawbody);
+// TODO MDL-46023 Replace this code with a call to the new library.
+$origentity = libxml_disable_entity_loader(true);
+$xml = simplexml_load_string($rawbody);
+if (!$xml) {
+ libxml_disable_entity_loader($origentity);
+ throw new Exception('Invalid XML content');
+}
+libxml_disable_entity_loader($origentity);
$body = $xml->imsx_POXBody;
foreach ($body->children() as $child) {
diff --git a/notes/index.php b/notes/index.php
index 62c01b9979d..b2cfe36882b 100644
--- a/notes/index.php
+++ b/notes/index.php
@@ -15,6 +15,10 @@ $userid = optional_param('user', 0, PARAM_INT);
$filtertype = optional_param('filtertype', '', PARAM_ALPHA);
$filterselect = optional_param('filterselect', 0, PARAM_INT);
+if (empty($CFG->enablenotes)) {
+ print_error('notesdisabled', 'notes');
+}
+
$url = new moodle_url('/notes/index.php');
if ($courseid != SITEID) {
$url->param('course', $courseid);
@@ -61,11 +65,6 @@ if ($userid) {
/// require login to access notes
require_login($course);
-add_to_log($courseid, 'notes', 'view', 'index.php?course='.$courseid.'&user='.$userid, 'view notes');
-
-if (empty($CFG->enablenotes)) {
- print_error('notesdisabled', 'notes');
-}
/// output HTML
if ($course->id == SITEID) {
@@ -73,8 +72,11 @@ if ($course->id == SITEID) {
} else {
$coursecontext = context_course::instance($course->id); // Course context
}
+require_capability('moodle/notes:view', $coursecontext);
$systemcontext = context_system::instance(); // SYSTEM context
+add_to_log($courseid, 'notes', 'view', 'index.php?course='.$courseid.'&user='.$userid, 'view notes');
+
$strnotes = get_string('notes', 'notes');
if ($userid) {
$PAGE->set_context(context_user::instance($user->id));
diff --git a/question/format.php b/question/format.php
index 9520ae17d76..1b5cbf541bf 100644
--- a/question/format.php
+++ b/question/format.php
@@ -357,6 +357,7 @@ class qformat_default {
$count = 0;
foreach ($questions as $question) { // Process and store each question
+ $transaction = $DB->start_delegated_transaction();
// reset the php timeout
set_time_limit(0);
@@ -371,6 +372,7 @@ class qformat_default {
$this->category = $newcategory;
}
}
+ $transaction->allow_commit();
continue;
}
$question->context = $this->importcontext;
@@ -429,9 +431,14 @@ class qformat_default {
if (!empty($result->error)) {
echo $OUTPUT->notification($result->error);
+ // Can't use $transaction->rollback(); since it requires an exception,
+ // and I don't want to rewrite this code to change the error handling now.
+ $DB->force_transaction_rollback();
return false;
}
+ $transaction->allow_commit();
+
if (!empty($result->notice)) {
echo $OUTPUT->notification($result->notice);
return true;
diff --git a/question/format/webct/format.php b/question/format/webct/format.php
index 9bc4f3a4672..fd6c411b736 100644
--- a/question/format/webct/format.php
+++ b/question/format/webct/format.php
@@ -606,12 +606,9 @@ class qformat_webct extends qformat_default {
// Calculated Question.
$question = $this->defaultquestion();
$question->qtype = 'calculated';
- $question->answers = array(); // No problem as they go as :FORMULA: from webct.
+ $question->answer = array(); // No problem as they go as :FORMULA: from webct.
$question->units = array();
$question->dataset = array();
-
- // To make us pass the end-of-question sanity checks.
- $question->answer = array('dummy');
$question->fraction = array('1.0');
$question->feedback = array();
@@ -737,7 +734,7 @@ class qformat_webct extends qformat_default {
if (preg_match('~^:FORMULA:(.*)~i', $line, $webctoptions)) {
// Answer for a calculated question.
++$currentchoice;
- $question->answers[$currentchoice] =
+ $question->answer[$currentchoice] =
qformat_webct_convert_formula($webctoptions[1]);
// Default settings.
diff --git a/question/format/xml/format.php b/question/format/xml/format.php
index 1005f5a653d..60c0cef552c 100644
--- a/question/format/xml/format.php
+++ b/question/format/xml/format.php
@@ -780,7 +780,7 @@ class qformat_xml extends qformat_default {
// get answers array
$answers = $question['#']['answer'];
- $qo->answers = array();
+ $qo->answer = array();
$qo->feedback = array();
$qo->fraction = array();
$qo->tolerance = array();
@@ -794,7 +794,7 @@ class qformat_xml extends qformat_default {
if (empty($ans->answer['text'])) {
$ans->answer['text'] = '*';
}
- $qo->answers[] = $ans->answer;
+ $qo->answer[] = $ans->answer['text'];
$qo->feedback[] = $ans->feedback;
$qo->tolerance[] = $answer['#']['tolerance'][0]['#'];
// fraction as a tag is deprecated
diff --git a/question/type/calculated/edit_calculated_form.php b/question/type/calculated/edit_calculated_form.php
index d6d9f450e90..c3c02a90533 100644
--- a/question/type/calculated/edit_calculated_form.php
+++ b/question/type/calculated/edit_calculated_form.php
@@ -212,36 +212,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) {
+ $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);
- }
- 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);
+ $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/question.php b/question/type/calculated/question.php
index b3246855d70..d4a48726676 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;
@@ -341,6 +342,10 @@ class qtype_calculated_variable_substituter {
* @return float the computed result.
*/
public function calculate($expression) {
+ // Make sure no malicious code is present in the expression. Refer MDL-46148 for details.
+ if ($error = qtype_calculated_find_formula_errors($expression)) {
+ throw new moodle_exception('illegalformulasyntax', 'qtype_calculated', '', $error);
+ }
return $this->calculate_raw($this->substitute_values_for_eval($expression));
}
@@ -388,108 +393,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 = '-+/*%>:^\~=&|!'; /* */
- $operatorornumber = "[$safeoperatorchar.0-9eE]";
-
- while (preg_match("~(^|[$safeoperatorchar,(])([a-z0-9_]*)" .
- "\\(($operatorornumber+(,$operatorornumber+((,$operatorornumber+)+)?)?)?\\)~",
- $formula, $regs)) {
- switch ($regs[2]) {
- // Simple parenthesis.
- case '':
- if ((isset($regs[4]) && $regs[4]) || strlen($regs[3]) == 0) {
- return get_string('illegalformulasyntax', 'qtype_calculated', $regs[0]);
- }
- break;
-
- // Zero argument functions.
- case 'pi':
- if ($regs[3]) {
- return get_string('functiontakesnoargs', 'qtype_calculated', $regs[2]);
- }
- break;
-
- // Single argument functions (the most common case).
- case 'abs': case 'acos': case 'acosh': case 'asin': case 'asinh':
- case 'atan': case 'atanh': case 'bindec': case 'ceil': case 'cos':
- case 'cosh': case 'decbin': case 'decoct': case 'deg2rad':
- case 'exp': case 'expm1': case 'floor': case 'is_finite':
- case 'is_infinite': case 'is_nan': case 'log10': case 'log1p':
- case 'octdec': case 'rad2deg': case 'sin': case 'sinh': case 'sqrt':
- case 'tan': case 'tanh':
- if (!empty($regs[4]) || empty($regs[3])) {
- return get_string('functiontakesonearg', 'qtype_calculated', $regs[2]);
- }
- break;
-
- // Functions that take one or two arguments.
- case 'log': case 'round':
- if (!empty($regs[5]) || empty($regs[3])) {
- return get_string('functiontakesoneortwoargs', 'qtype_calculated',
- $regs[2]);
- }
- break;
-
- // Functions that must have two arguments.
- case 'atan2': case 'fmod': case 'pow':
- if (!empty($regs[5]) || empty($regs[4])) {
- return get_string('functiontakestwoargs', 'qtype_calculated', $regs[2]);
- }
- break;
-
- // Functions that take two or more arguments.
- case 'min': case 'max':
- if (empty($regs[4])) {
- return get_string('functiontakesatleasttwo', 'qtype_calculated', $regs[2]);
- }
- break;
-
- default:
- return get_string('unsupportedformulafunction', 'qtype_calculated', $regs[2]);
- }
-
- // Exchange the function call with '1' 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);
- } else {
- // The function call starts the formula.
- $formula = preg_replace("~^$regs[2]\\([^)]*\\)~", '1', $formula);
- }
- }
-
- if (preg_match("~[^$safeoperatorchar.0-9eE]+~", $formula, $regs)) {
- return get_string('illegalformulasyntax', 'qtype_calculated', $regs[0]);
- } else {
- // Formula just might be valid.
- return false;
- }
- }
-}
\ No newline at end of file
+}
diff --git a/question/type/calculated/questiontype.php b/question/type/calculated/questiontype.php
index 3e9143ebe94..5a369dcccc1 100644
--- a/question/type/calculated/questiontype.php
+++ b/question/type/calculated/questiontype.php
@@ -37,6 +37,9 @@ 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 MAX_DATASET_ITEMS = 100;
public $wizardpagesnumber = 3;
@@ -128,11 +131,13 @@ class qtype_calculated extends question_type {
public function save_question_options($question) {
global $CFG, $DB;
+
+ // Make it impossible to save bad formulas anywhere.
+ $this->validate_question_data($question);
+
// The code is used for calculated, calculatedsimple and calculatedmulti qtypes.
$context = $question->context;
- if (isset($question->answer) && !isset($question->answers)) {
- $question->answers = $question->answer;
- }
+
// Calculated options.
$update = true;
$options = $DB->get_record('question_calculated_options',
@@ -182,14 +187,7 @@ class qtype_calculated extends question_type {
$units = $result->units;
}
- // Insert all the new answers.
- if (isset($question->answer) && !isset($question->answers)) {
- $question->answers = $question->answer;
- }
- foreach ($question->answers as $key => $answerdata) {
- if (is_array($answerdata)) {
- $answerdata = $answerdata['text'];
- }
+ foreach ($question->answer as $key => $answerdata) {
if (trim($answerdata) == '') {
continue;
}
@@ -343,49 +341,6 @@ class qtype_calculated extends question_type {
$question->datasetloader = new qtype_calculated_dataset_loader($questiondata->id);
}
- public function validate_form($form) {
- switch($form->wizardpage) {
- case 'question':
- $calculatedmessages = array();
- if (empty($form->name)) {
- $calculatedmessages[] = get_string('missingname', 'qtype_calculated');
- }
- if (empty($form->questiontext)) {
- $calculatedmessages[] = get_string('missingquestiontext', 'qtype_calculated');
- }
- // Verify formulas.
- foreach ($form->answers as $key => $answer) {
- if ('' === trim($answer)) {
- $calculatedmessages[] = get_string(
- 'missingformula', 'qtype_calculated');
- }
- if ($formulaerrors = qtype_calculated_find_formula_errors($answer)) {
- $calculatedmessages[] = $formulaerrors;
- }
- if (! isset($form->tolerance[$key])) {
- $form->tolerance[$key] = 0.0;
- }
- if (! is_numeric($form->tolerance[$key])) {
- $calculatedmessages[] = get_string('xmustbenumeric', 'qtype_numerical',
- get_string('tolerance', 'qtype_calculated'));
- }
- }
-
- if (!empty($calculatedmessages)) {
- $errorstring = "The following errors were found: ";
- foreach ($calculatedmessages as $msg) {
- $errorstring .= $msg . ' ';
- }
- print_error($errorstring);
- }
-
- break;
- default:
- return parent::validate_form($form);
- break;
- }
- return true;
- }
public function finished_edit_wizard($form) {
return isset($form->savechanges);
}
@@ -481,6 +436,55 @@ 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);
+ }
+ }
+
+ /**
+ * Validate data before save.
+ * @param stdClass $question data from the form / import file.
+ */
+ protected function validate_question_data($question) {
+ $this->validate_text($question->questiontext); // Yes, really no ['text'].
+
+ if (isset($question->generalfeedback['text'])) {
+ $this->validate_text($question->generalfeedback['text']);
+ } else if (isset($question->generalfeedback)) {
+ $this->validate_text($question->generalfeedback); // Because question import is weird.
+ }
+
+ foreach ($question->answer as $key => $answer) {
+ $this->validate_answer($answer);
+ $this->validate_text($question->feedback[$key]['text']);
+ }
+ }
+
/**
* This method prepare the $datasets in a format similar to dadatesetdefinitions_form.php
* so that they can be saved
@@ -493,12 +497,13 @@ class qtype_calculated extends question_type {
* @param object $form
* @param int $questionfromid default = '0'
*/
- public function preparedatasets($form , $questionfromid = '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->answer as $key => $answer) {
$mandatorydatasets += $this->find_dataset_names($answer);
}
// If there are identical datasetdefs already saved in the original question
@@ -587,8 +592,9 @@ class qtype_calculated extends question_type {
*/
public function save_question($question, $form) {
global $DB;
+
if ($this->wizardpagesnumber() == 1 || $question->qtype == 'calculatedsimple') {
- $question = parent::save_question($question, $form);
+ $question = parent::save_question($question, $form);
return $question;
}
@@ -1058,10 +1064,14 @@ class qtype_calculated extends question_type {
}
$answers = fullclone($answers);
- $errors = '';
$delimiter = ': ';
$virtualqtype = $qtypeobj->get_virtual_qtype();
foreach ($answers as $key => $answer) {
+ $error = qtype_calculated_find_formula_errors($answer->answer);
+ if ($error) {
+ $comment->stranswers[$key] = $error;
+ continue;
+ }
$formula = $this->substitute_variables($answer->answer, $data);
$formattedanswer = qtype_calculated_calculate_answer(
$answer->answer, $data, $answer->tolerance,
@@ -1972,6 +1982,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
@@ -2000,7 +2015,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;
@@ -2061,3 +2076,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(
+ '