From 33bd0be19d408176831712b83c0579ea958cff71 Mon Sep 17 00:00:00 2001 From: Dan Poltawski Date: Thu, 3 Jul 2014 15:52:31 +0100 Subject: [PATCH 01/19] MDL-43848 messages: do not display message content in popups The user is not in control of when these popups show up and it may not be appropiate to have part of this content display whilst in the presence of other users. --- lib/moodlelib.php | 38 +------------------------------------- 1 file changed, 1 insertion(+), 37 deletions(-) diff --git a/lib/moodlelib.php b/lib/moodlelib.php index 2db916f619add..0059dc8feb366 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -10314,43 +10314,7 @@ function message_popup_window() { //if we have new messages to notify the user about if (!empty($message_users)) { - $strmessages = ''; - if (count($message_users)>1) { - $strmessages = get_string('unreadnewmessages', 'message', count($message_users)); - } else { - $message_users = reset($message_users); - - //show who the message is from if its not a notification - if (!$message_users->notification) { - $strmessages = get_string('unreadnewmessage', 'message', fullname($message_users) ); - } - - //try to display the small version of the message - $smallmessage = null; - if (!empty($message_users->smallmessage)) { - //display the first 200 chars of the message in the popup - $smallmessage = null; - if (textlib::strlen($message_users->smallmessage) > 200) { - $smallmessage = textlib::substr($message_users->smallmessage,0,200).'...'; - } else { - $smallmessage = $message_users->smallmessage; - } - - //prevent html symbols being displayed - if ($message_users->fullmessageformat == FORMAT_HTML) { - $smallmessage = html_to_text($smallmessage); - } else { - $smallmessage = s($smallmessage); - } - } else if ($message_users->notification) { - //its a notification with no smallmessage so just say they have a notification - $smallmessage = get_string('unreadnewnotification', 'message'); - } - if (!empty($smallmessage)) { - $strmessages .= '
'.s($smallmessage).'
'; - } - } - + $strmessages = get_string('unreadnewmessages', 'message', count($message_users)); $strgomessage = get_string('gotomessages', 'message'); $strstaymessage = get_string('ignore','admin'); From a5abafcde27764a2e9f4ae3a7ed30b60f2546268 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Fri, 16 May 2014 15:26:25 -0700 Subject: [PATCH 02/19] MDL-45485 auth_shibboleth: User taking over other user's session Replacing code with call to complete_user_login() since it calls session_regenerate_id(). --- auth/shibboleth/index.php | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/auth/shibboleth/index.php b/auth/shibboleth/index.php index 61f6ba877f841..c5fef866dd273 100644 --- a/auth/shibboleth/index.php +++ b/auth/shibboleth/index.php @@ -47,21 +47,7 @@ if ($shibbolethauth->user_login($frm->username, $frm->password) && $user = authenticate_user_login($frm->username, $frm->password)) { - enrol_check_plugins($user); - session_set_user($user); - - $USER->loggedin = true; - $USER->site = $CFG->wwwroot; // for added security, store the site in the - - update_user_login_times(); - - // Don't show previous shibboleth username on login page - - set_login_session_preferences(); - - unset($SESSION->lang); - $SESSION->justloggedin = true; - + complete_user_login($user); add_to_log(SITEID, 'user', 'login', "view.php?id=$USER->id&course=".SITEID, $USER->id, 0, $USER->id); if (user_not_fully_set_up($USER)) { From cb2b42aed8d9ce3c9840ad825f2e0e7e81bfad91 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Wed, 25 Jun 2014 14:40:27 +0800 Subject: [PATCH 03/19] MDL-45616 repositories: more clearly distinguish when we use source and when reference Function repository::get_moodle_file() should always be called on packed reference and not on the source received from user. Also added phpdocs to some other methods that were confusing source and reference --- repository/filepicker.php | 2 +- repository/lib.php | 23 +++++++++++++---------- repository/repository_ajax.php | 2 +- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/repository/filepicker.php b/repository/filepicker.php index bd19ef7f73be7..20d5b872bb608 100644 --- a/repository/filepicker.php +++ b/repository/filepicker.php @@ -293,7 +293,7 @@ // note that in this case user may not have permission to access the source file directly // so no file_browser/file_info can be used below if ($repo->has_moodle_files()) { - $file = repository::get_moodle_file($fileurl); + $file = repository::get_moodle_file($reference); if ($file && $file->is_external_file()) { $sourcefield = $file->get_source(); // remember the original source $record->source = $repo::build_source_field($sourcefield); diff --git a/repository/lib.php b/repository/lib.php index e9781e20bccba..3bec676b00e82 100644 --- a/repository/lib.php +++ b/repository/lib.php @@ -717,13 +717,14 @@ public static function draftfile_exists($itemid, $filepath, $filename) { } /** - * Parses the 'source' returned by moodle repositories and returns an instance of stored_file + * Parses the moodle file reference and returns an instance of stored_file * - * @param string $source + * @param string $reference reference to the moodle internal file as retruned by + * {@link repository::get_file_reference()} or {@link file_storage::pack_reference()} * @return stored_file|null */ - public static function get_moodle_file($source) { - $params = file_storage::unpack_reference($source, true); + public static function get_moodle_file($reference) { + $params = file_storage::unpack_reference($reference, true); $fs = get_file_storage(); return $fs->get_file($params['contextid'], $params['component'], $params['filearea'], $params['itemid'], $params['filepath'], $params['filename']); @@ -735,13 +736,14 @@ public static function get_moodle_file($source) { * This is checked when user tries to pick the file from repository to deal with * potential parameter substitutions is request * - * @param string $source + * @param string $source source of the file, returned by repository as 'source' and received back from user (not cleaned) * @return bool whether the file is accessible by current user */ public function file_is_accessible($source) { if ($this->has_moodle_files()) { + $reference = $this->get_file_reference($source); try { - $params = file_storage::unpack_reference($source, true); + $params = file_storage::unpack_reference($reference, true); } catch (file_reference_exception $e) { return false; } @@ -1336,12 +1338,13 @@ public function get_file_by_reference($reference) { * again to another file area (also as a copy or as a reference), the value of * files.source is copied. * - * @param string $source the value that repository returned in listing as 'source' + * @param string $source source of the file, returned by repository as 'source' and received back from user (not cleaned) * @return string|null */ public function get_file_source_info($source) { if ($this->has_moodle_files()) { - return $this->get_reference_details($source, 0); + $reference = $this->get_file_reference($source); + return $this->get_reference_details($reference, 0); } return $source; } @@ -1621,11 +1624,11 @@ public static function display_instances_list($context, $typename = null) { /** * Prepare file reference information * - * @param string $source + * @param string $source source of the file, returned by repository as 'source' and received back from user (not cleaned) * @return string file referece */ public function get_file_reference($source) { - if ($this->has_moodle_files() && ($this->supported_returntypes() & FILE_REFERENCE)) { + if ($source && $this->has_moodle_files()) { $params = file_storage::unpack_reference($source); if (!is_array($params)) { throw new repository_exception('invalidparams', 'repository'); diff --git a/repository/repository_ajax.php b/repository/repository_ajax.php index fa8f488523fc1..8b5fe04ad7a60 100644 --- a/repository/repository_ajax.php +++ b/repository/repository_ajax.php @@ -239,7 +239,7 @@ // note that in this case user may not have permission to access the source file directly // so no file_browser/file_info can be used below if ($repo->has_moodle_files()) { - $file = repository::get_moodle_file($source); + $file = repository::get_moodle_file($reference); if ($file && $file->is_external_file()) { $sourcefield = $file->get_source(); // remember the original source $record->source = $repo::build_source_field($sourcefield); From 5c4ef26c39d3106315f74c26cdcca779ba74254c Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Thu, 26 Jun 2014 08:53:25 +0800 Subject: [PATCH 04/19] MDL-45616 repositories: use json encoding instead of serialization --- repository/coursefiles/lib.php | 10 +++++----- repository/equella/callback.php | 2 +- repository/equella/lib.php | 15 ++++++++++----- repository/lib.php | 26 +++++++++++++++++++++++--- repository/local/lib.php | 6 +++--- repository/recent/lib.php | 5 +++-- repository/user/lib.php | 8 ++++---- 7 files changed, 49 insertions(+), 23 deletions(-) diff --git a/repository/coursefiles/lib.php b/repository/coursefiles/lib.php index b81904827b25d..9fb102f7a0159 100644 --- a/repository/coursefiles/lib.php +++ b/repository/coursefiles/lib.php @@ -63,7 +63,7 @@ public function get_listing($encodedpath = '', $page = '') { $browser = get_file_browser(); if (!empty($encodedpath)) { - $params = unserialize(base64_decode($encodedpath)); + $params = json_decode(base64_decode($encodedpath), true); if (is_array($params)) { $filepath = is_null($params['filepath']) ? NULL : clean_param($params['filepath'], PARAM_PATH);; $filename = is_null($params['filename']) ? NULL : clean_param($params['filename'], PARAM_FILE); @@ -80,12 +80,12 @@ public function get_listing($encodedpath = '', $page = '') { if ($fileinfo = $browser->get_file_info($context, $component, $filearea, $itemid, $filepath, $filename)) { // build path navigation $pathnodes = array(); - $encodedpath = base64_encode(serialize($fileinfo->get_params())); + $encodedpath = base64_encode(json_encode($fileinfo->get_params())); $pathnodes[] = array('name'=>$fileinfo->get_visible_name(), 'path'=>$encodedpath); $level = $fileinfo->get_parent(); while ($level) { $params = $level->get_params(); - $encodedpath = base64_encode(serialize($params)); + $encodedpath = base64_encode(json_encode($params)); if ($params['contextid'] != $context->id) { break; } @@ -102,7 +102,7 @@ public function get_listing($encodedpath = '', $page = '') { if ($child->is_directory()) { $params = $child->get_params(); $subdir_children = $child->get_children(); - $encodedpath = base64_encode(serialize($params)); + $encodedpath = base64_encode(json_encode($params)); $node = array( 'title' => $child->get_visible_name(), 'datemodified' => $child->get_timemodified(), @@ -113,7 +113,7 @@ public function get_listing($encodedpath = '', $page = '') { ); $list[] = $node; } else { - $encodedpath = base64_encode(serialize($child->get_params())); + $encodedpath = base64_encode(json_encode($child->get_params())); $node = array( 'title' => $child->get_visible_name(), 'size' => $child->get_filesize(), diff --git a/repository/equella/callback.php b/repository/equella/callback.php index 01fc27dc7f3f7..f1c044d4001b1 100644 --- a/repository/equella/callback.php +++ b/repository/equella/callback.php @@ -55,7 +55,7 @@ $license = s(clean_param($info->license, PARAM_ALPHAEXT)); } -$source = base64_encode(serialize((object)array('url'=>$url,'filename'=>$filename))); +$source = base64_encode(json_encode(array('url'=>$url,'filename'=>$filename))); $js =<< diff --git a/repository/equella/lib.php b/repository/equella/lib.php index 14afc6ec20a11..f4bfe944f5ba4 100644 --- a/repository/equella/lib.php +++ b/repository/equella/lib.php @@ -119,7 +119,11 @@ public function supported_returntypes() { * @return string file referece */ public function get_file_reference($source) { - return $source; + // Internally we store serialized value but user input is json-encoded for security reasons. + $ref = json_decode(base64_decode($source)); + $filename = clean_param($ref->filename, PARAM_FILE); + $url = clean_param($ref->url, PARAM_URL); + return base64_encode(serialize((object)array('url' => $url, 'filename' => $filename))); } /** @@ -407,12 +411,13 @@ private static function to_mime_type($value) { /** * Return the source information * - * @param stdClass $url + * @param string $source * @return string|null */ - public function get_file_source_info($url) { - $ref = unserialize(base64_decode($url)); - return 'EQUELLA: ' . $ref->filename; + public function get_file_source_info($source) { + $ref = json_decode(base64_decode($source)); + $filename = clean_param($ref->filename, PARAM_FILE); + return 'EQUELLA: ' . $filename; } /** diff --git a/repository/lib.php b/repository/lib.php index 3bec676b00e82..5656a460d2f22 100644 --- a/repository/lib.php +++ b/repository/lib.php @@ -1625,12 +1625,32 @@ public static function display_instances_list($context, $typename = null) { * Prepare file reference information * * @param string $source source of the file, returned by repository as 'source' and received back from user (not cleaned) - * @return string file referece + * @return string file reference, ready to be stored */ public function get_file_reference($source) { if ($source && $this->has_moodle_files()) { - $params = file_storage::unpack_reference($source); - if (!is_array($params)) { + $params = @json_decode(base64_decode($source), true); + if (!$params && !in_array($this->get_typename(), array('recent', 'user', 'local', 'coursefiles'))) { + // IMPORTANT! Since default format for moodle files was changed in the minor release as a security fix + // we maintain an old code here in order not to break 3rd party repositories that deal + // with moodle files. Repositories are strongly encouraged to be upgraded, see MDL-45616. + // In Moodle 2.8 this fallback will be removed. + $params = file_storage::unpack_reference($source, true); + return file_storage::pack_reference($params); + } + if (!is_array($params) || empty($params['contextid'])) { + throw new repository_exception('invalidparams', 'repository'); + } + $params = array( + 'component' => empty($params['component']) ? '' : clean_param($params['component'], PARAM_COMPONENT), + 'filearea' => empty($params['filearea']) ? '' : clean_param($params['filearea'], PARAM_AREA), + 'itemid' => empty($params['itemid']) ? 0 : clean_param($params['itemid'], PARAM_INT), + 'filename' => empty($params['filename']) ? null : clean_param($params['filename'], PARAM_FILE), + 'filepath' => empty($params['filepath']) ? null : clean_param($params['filepath'], PARAM_PATH), + 'contextid' => clean_param($params['contextid'], PARAM_INT) + ); + // Check if context exists. + if (!context::instance_by_id($params['contextid'], IGNORE_MISSING)) { throw new repository_exception('invalidparams', 'repository'); } return file_storage::pack_reference($params); diff --git a/repository/local/lib.php b/repository/local/lib.php index f3be15674970c..75f01e1883712 100644 --- a/repository/local/lib.php +++ b/repository/local/lib.php @@ -56,7 +56,7 @@ public function get_listing($encodedpath = '', $page = '') { $component = null; if (!empty($encodedpath)) { - $params = unserialize(base64_decode($encodedpath)); + $params = json_decode(base64_decode($encodedpath), true); if (is_array($params) && isset($params['contextid'])) { $component = is_null($params['component']) ? NULL : clean_param($params['component'], PARAM_COMPONENT); $filearea = is_null($params['filearea']) ? NULL : clean_param($params['filearea'], PARAM_AREA); @@ -216,7 +216,7 @@ private function can_skip(file_info $fileinfo, $extensions, $parent = -1) { */ private function get_node(file_info $fileinfo) { global $OUTPUT; - $encodedpath = base64_encode(serialize($fileinfo->get_params())); + $encodedpath = base64_encode(json_encode($fileinfo->get_params())); $node = array( 'title' => $fileinfo->get_visible_name(), 'datemodified' => $fileinfo->get_timemodified(), @@ -256,7 +256,7 @@ private function get_node(file_info $fileinfo) { * @return array */ private function get_node_path(file_info $fileinfo) { - $encodedpath = base64_encode(serialize($fileinfo->get_params())); + $encodedpath = base64_encode(json_encode($fileinfo->get_params())); return array( 'path' => $encodedpath, 'name' => $fileinfo->get_visible_name() diff --git a/repository/recent/lib.php b/repository/recent/lib.php index 76cdb69e2a8b8..55dedde34b5ec 100644 --- a/repository/recent/lib.php +++ b/repository/recent/lib.php @@ -126,7 +126,7 @@ public function get_listing($encodedpath = '', $page = '') { $fileinfo = $browser->get_file_info($context, $file['component'], $file['filearea'], $file['itemid'], $file['filepath'], $file['filename']); if ($fileinfo) { - $params = base64_encode(serialize($file)); + $params = base64_encode(json_encode($file)); $node = array( 'title' => $fileinfo->get_visible_name(), 'size' => $fileinfo->get_filesize(), @@ -191,7 +191,8 @@ public function supported_returntypes() { */ public function file_is_accessible($source) { global $USER; - $file = self::get_moodle_file($source); + $reference = $this->get_file_reference($source); + $file = self::get_moodle_file($reference); return (!empty($file) && $file->get_userid() == $USER->id); } diff --git a/repository/user/lib.php b/repository/user/lib.php index b2f4f1414e7b5..0147c3d838ed7 100644 --- a/repository/user/lib.php +++ b/repository/user/lib.php @@ -61,7 +61,7 @@ public function get_listing($encodedpath = '', $page = '') { $list = array(); if (!empty($encodedpath)) { - $params = unserialize(base64_decode($encodedpath)); + $params = json_decode(base64_decode($encodedpath), true); if (is_array($params)) { $filepath = clean_param($params['filepath'], PARAM_PATH);; $filename = clean_param($params['filename'], PARAM_FILE); @@ -84,7 +84,7 @@ public function get_listing($encodedpath = '', $page = '') { $level = $fileinfo; $params = $fileinfo->get_params(); while ($level && $params['component'] == 'user' && $params['filearea'] == 'private') { - $encodedpath = base64_encode(serialize($level->get_params())); + $encodedpath = base64_encode(json_encode($level->get_params())); $pathnodes[] = array('name'=>$level->get_visible_name(), 'path'=>$encodedpath); $level = $level->get_parent(); $params = $level->get_params(); @@ -95,7 +95,7 @@ public function get_listing($encodedpath = '', $page = '') { $children = $fileinfo->get_children(); foreach ($children as $child) { if ($child->is_directory()) { - $encodedpath = base64_encode(serialize($child->get_params())); + $encodedpath = base64_encode(json_encode($child->get_params())); $node = array( 'title' => $child->get_visible_name(), 'datemodified' => $child->get_timemodified(), @@ -106,7 +106,7 @@ public function get_listing($encodedpath = '', $page = '') { ); $list[] = $node; } else { - $encodedpath = base64_encode(serialize($child->get_params())); + $encodedpath = base64_encode(json_encode($child->get_params())); $node = array( 'title' => $child->get_visible_name(), 'size' => $child->get_filesize(), From 3b68d1b23a57325126125176634aeaa54a99de53 Mon Sep 17 00:00:00 2001 From: Frederic Massart Date: Tue, 10 Jun 2014 12:53:43 +0800 Subject: [PATCH 05/19] MDL-45463 mod_lti: Prevent XML entity injections from provider --- mod/lti/service.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/mod/lti/service.php b/mod/lti/service.php index 60bcfa2caf87d..d3645b4140c68 100644 --- a/mod/lti/service.php +++ b/mod/lti/service.php @@ -53,7 +53,14 @@ 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) { From 9b60a086cfdc35c53f38ad678b9bef1dd656348c Mon Sep 17 00:00:00 2001 From: Frederic Massart Date: Fri, 13 Jun 2014 15:40:15 +0800 Subject: [PATCH 06/19] MDL-45417 mod_imscp: Prevent entity injections from package content --- mod/imscp/locallib.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mod/imscp/locallib.php b/mod/imscp/locallib.php index d51ecf7a690c1..edf55d1deded1 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) { $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/'; From a2bf5412a7a4daa2961ae6484cd5af90fd74e4e1 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Thu, 26 Jun 2014 10:48:02 +0100 Subject: [PATCH 07/19] MDL-46148 qtype_calculated: fix formula validation. --- .../type/calculated/edit_calculated_form.php | 45 ++++---- question/type/calculated/question.php | 104 +---------------- question/type/calculated/questiontype.php | 82 +++++++++++++- .../tests/formula_validation_test.php | 105 ++++++++++++++++++ .../edit_calculatedmulti_form.php | 66 +++++------ .../type/calculatedmulti/questiontype.php | 7 ++ 6 files changed, 245 insertions(+), 164 deletions(-) create mode 100644 question/type/calculated/tests/formula_validation_test.php diff --git a/question/type/calculated/edit_calculated_form.php b/question/type/calculated/edit_calculated_form.php index 8b3fbdddf0bf2..bddf20658657c 100644 --- a/question/type/calculated/edit_calculated_form.php +++ b/question/type/calculated/edit_calculated_form.php @@ -189,36 +189,39 @@ public function qtype() { 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['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 3ecbc9d0f7be8..76811f13fa6a4 100644 --- a/question/type/calculated/question.php +++ b/question/type/calculated/question.php @@ -259,6 +259,7 @@ public function datasets_are_synchronised($category) { * @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 @@ protected function substitute_values_pretty($text) { * @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 = '-+/*%>:^\~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 @@ public function preparedatasets($form , $questionfromid = '0') { // 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 @@ public function addnamecategory(&$question) { */ 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 @@ public function save_question($question, $form) { 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 0000000000000..d7ff76ed18e04 --- /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 7a802eeb8d07e..4e9369a26b336 100644 --- a/question/type/calculatedmulti/edit_calculatedmulti_form.php +++ b/question/type/calculatedmulti/edit_calculatedmulti_form.php @@ -218,30 +218,31 @@ protected function data_preprocessing_answers($question, $withanswerfiles = fals 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 @@ public function validation($data, $files) { get_string('atleastonewildcard', 'qtype_calculated'); } } + $totalfraction = 0; $maxfraction = -1; foreach ($answers as $key => $answer) { @@ -268,28 +270,12 @@ public function validation($data, $files) { $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 220d1bcaf8f68..5c0179fab95c6 100644 --- a/question/type/calculatedmulti/questiontype.php +++ b/question/type/calculatedmulti/questiontype.php @@ -156,6 +156,13 @@ public function save_question_options($question) { 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) { From 8f7d596058a18c60b795b4677b59cf074c56de39 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Fri, 4 Jul 2014 10:40:13 +0800 Subject: [PATCH 08/19] MDL-45760 make sure to check permission before setting header --- notes/index.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/notes/index.php b/notes/index.php index 62c01b9979d6f..b2cfe36882b30 100644 --- a/notes/index.php +++ b/notes/index.php @@ -15,6 +15,10 @@ $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 @@ /// 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 @@ } 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)); From f25f472be425d6ef8aa587648dafda1bd4d1c5d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Mon, 7 Jul 2014 06:38:47 +0200 Subject: [PATCH 09/19] MDL-46223 Improve the rubric output --- grade/grading/form/rubric/js/rubriceditor.js | 4 ++-- grade/grading/form/rubric/renderer.php | 12 ++++++------ grade/grading/form/rubric/rubriceditor.php | 1 + 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/grade/grading/form/rubric/js/rubriceditor.js b/grade/grading/form/rubric/js/rubriceditor.js index 1d5d5248d87cb..f13492de432dc 100644 --- a/grade/grading/form/rubric/js/rubriceditor.js +++ b/grade/grading/form/rubric/js/rubriceditor.js @@ -93,8 +93,8 @@ M.gradingform_rubriceditor.editmode = function(el, editmode, focustb) { value = (el.hasClass('level')) ? M.str.gradingform_rubric.levelempty : M.str.gradingform_rubric.criterionempty taplain.addClass('empty') } - taplain.one('.textvalue').set('innerHTML', value) - if (tb) tbplain.one('.textvalue').set('innerHTML', tb.get('value')) + taplain.one('.textvalue').set('innerHTML', Y.Escape.html(value)); + if (tb) tbplain.one('.textvalue').set('innerHTML', Y.Escape.html(tb.get('value'))); // hide/display textarea, textbox and plaintexts taplain.removeClass('hiddenelement') ta.addClass('hiddenelement') diff --git a/grade/grading/form/rubric/renderer.php b/grade/grading/form/rubric/renderer.php index c269c0afba9ff..0095c9a2d8535 100644 --- a/grade/grading/form/rubric/renderer.php +++ b/grade/grading/form/rubric/renderer.php @@ -74,13 +74,13 @@ public function criterion_template($mode, $options, $elementname = '{NAME}', $cr } $criteriontemplate .= html_writer::end_tag('td'); // .controls $criteriontemplate .= html_writer::empty_tag('input', array('type' => 'hidden', 'name' => '{NAME}[criteria][{CRITERION-id}][sortorder]', 'value' => $criterion['sortorder'])); - $description = html_writer::tag('textarea', htmlspecialchars($criterion['description']), array('name' => '{NAME}[criteria][{CRITERION-id}][description]', 'cols' => '10', 'rows' => '5')); + $description = html_writer::tag('textarea', s($criterion['description']), array('name' => '{NAME}[criteria][{CRITERION-id}][description]', 'cols' => '10', 'rows' => '5')); } else { if ($mode == gradingform_rubric_controller::DISPLAY_EDIT_FROZEN) { $criteriontemplate .= html_writer::empty_tag('input', array('type' => 'hidden', 'name' => '{NAME}[criteria][{CRITERION-id}][sortorder]', 'value' => $criterion['sortorder'])); $criteriontemplate .= html_writer::empty_tag('input', array('type' => 'hidden', 'name' => '{NAME}[criteria][{CRITERION-id}][description]', 'value' => $criterion['description'])); } - $description = $criterion['description']; + $description = s($criterion['description']); } $descriptionclass = 'description'; if (isset($criterion['error_description'])) { @@ -106,12 +106,12 @@ public function criterion_template($mode, $options, $elementname = '{NAME}', $cr $currentremark = $value['remark']; } if ($mode == gradingform_rubric_controller::DISPLAY_EVAL) { - $input = html_writer::tag('textarea', htmlspecialchars($currentremark), array('name' => '{NAME}[criteria][{CRITERION-id}][remark]', 'cols' => '10', 'rows' => '5')); + $input = html_writer::tag('textarea', s($currentremark), array('name' => '{NAME}[criteria][{CRITERION-id}][remark]', 'cols' => '10', 'rows' => '5')); $criteriontemplate .= html_writer::tag('td', $input, array('class' => 'remark')); } else if ($mode == gradingform_rubric_controller::DISPLAY_EVAL_FROZEN) { $criteriontemplate .= html_writer::empty_tag('input', array('type' => 'hidden', 'name' => '{NAME}[criteria][{CRITERION-id}][remark]', 'value' => $currentremark)); }else if ($mode == gradingform_rubric_controller::DISPLAY_REVIEW || $mode == gradingform_rubric_controller::DISPLAY_VIEW) { - $criteriontemplate .= html_writer::tag('td', $currentremark, array('class' => 'remark')); // TODO maybe some prefix here like 'Teacher remark:' + $criteriontemplate .= html_writer::tag('td', s($currentremark), array('class' => 'remark')); } } $criteriontemplate .= html_writer::end_tag('tr'); // .criterion @@ -163,7 +163,7 @@ public function level_template($mode, $options, $elementname = '{NAME}', $criter $leveltemplate = html_writer::start_tag('td', $tdattributes); $leveltemplate .= html_writer::start_tag('div', array('class' => 'level-wrapper')); if ($mode == gradingform_rubric_controller::DISPLAY_EDIT_FULL) { - $definition = html_writer::tag('textarea', htmlspecialchars($level['definition']), array('name' => '{NAME}[criteria][{CRITERION-id}][levels][{LEVEL-id}][definition]', 'cols' => '10', 'rows' => '4')); + $definition = html_writer::tag('textarea', s($level['definition']), array('name' => '{NAME}[criteria][{CRITERION-id}][levels][{LEVEL-id}][definition]', 'cols' => '10', 'rows' => '4')); $score = html_writer::label(get_string('criterionempty', 'gradingform_rubric'), '{NAME}criteria{CRITERION-id}levels{LEVEL-id}', false, array('class' => 'accesshide')); $score .= html_writer::empty_tag('input', array('type' => 'text','id' => '{NAME}criteria{CRITERION-id}levels{LEVEL-id}', 'name' => '{NAME}[criteria][{CRITERION-id}][levels][{LEVEL-id}][score]', 'size' => '3', 'value' => $level['score'])); } else { @@ -171,7 +171,7 @@ public function level_template($mode, $options, $elementname = '{NAME}', $criter $leveltemplate .= html_writer::empty_tag('input', array('type' => 'hidden', 'name' => '{NAME}[criteria][{CRITERION-id}][levels][{LEVEL-id}][definition]', 'value' => $level['definition'])); $leveltemplate .= html_writer::empty_tag('input', array('type' => 'hidden', 'name' => '{NAME}[criteria][{CRITERION-id}][levels][{LEVEL-id}][score]', 'value' => $level['score'])); } - $definition = $level['definition']; + $definition = s($level['definition']); $score = $level['score']; } if ($mode == gradingform_rubric_controller::DISPLAY_EVAL) { diff --git a/grade/grading/form/rubric/rubriceditor.php b/grade/grading/form/rubric/rubriceditor.php index 61d8307f1794e..537274b7fee13 100644 --- a/grade/grading/form/rubric/rubriceditor.php +++ b/grade/grading/form/rubric/rubriceditor.php @@ -85,6 +85,7 @@ public function toHtml() { if (!$this->_flagFrozen) { $mode = gradingform_rubric_controller::DISPLAY_EDIT_FULL; $module = array('name'=>'gradingform_rubriceditor', 'fullpath'=>'/grade/grading/form/rubric/js/rubriceditor.js', + 'requires' => array('base', 'dom', 'event', 'event-touch', 'escape'), 'strings' => array(array('confirmdeletecriterion', 'gradingform_rubric'), array('confirmdeletelevel', 'gradingform_rubric'), array('criterionempty', 'gradingform_rubric'), array('levelempty', 'gradingform_rubric') )); From 470a466d7f1e0aef030ad2178bbef5a81765c42e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Mon, 7 Jul 2014 09:52:37 +0200 Subject: [PATCH 10/19] MDL-46223 Improve the marking guide output --- grade/grading/form/guide/guideeditor.php | 1 + grade/grading/form/guide/js/guideeditor.js | 4 +-- grade/grading/form/guide/renderer.php | 30 +++++++++++----------- 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/grade/grading/form/guide/guideeditor.php b/grade/grading/form/guide/guideeditor.php index aebb9822fcab2..da430ec902747 100644 --- a/grade/grading/form/guide/guideeditor.php +++ b/grade/grading/form/guide/guideeditor.php @@ -100,6 +100,7 @@ public function toHtml() { $mode = gradingform_guide_controller::DISPLAY_EDIT_FULL; $module = array('name'=>'gradingform_guideeditor', 'fullpath'=>'/grade/grading/form/guide/js/guideeditor.js', + 'requires' => array('base', 'dom', 'event', 'event-touch', 'escape'), 'strings' => array( array('confirmdeletecriterion', 'gradingform_guide'), array('clicktoedit', 'gradingform_guide'), diff --git a/grade/grading/form/guide/js/guideeditor.js b/grade/grading/form/guide/js/guideeditor.js index 3aeb83be91f23..94c1e157fab3b 100644 --- a/grade/grading/form/guide/js/guideeditor.js +++ b/grade/grading/form/guide/js/guideeditor.js @@ -111,9 +111,9 @@ M.gradingform_guideeditor.editmode = function(el, editmode) { value = M.str.gradingform_guide.clicktoedit taplain.addClass('empty') } - taplain.one('.textvalue').set('innerHTML', value) + taplain.one('.textvalue').set('innerHTML', Y.Escape.html(value)) if (tb) { - tbplain.one('.textvalue').set('innerHTML', tb.get('value')) + tbplain.one('.textvalue').set('innerHTML', Y.Escape.html(tb.get('value'))) } // hide/display textarea, textbox and plaintexts taplain.removeClass('hiddenelement') diff --git a/grade/grading/form/guide/renderer.php b/grade/grading/form/guide/renderer.php index 360e1b9e5020c..ac30ab8108d87 100644 --- a/grade/grading/form/guide/renderer.php +++ b/grade/grading/form/guide/renderer.php @@ -93,20 +93,20 @@ public function criterion_template($mode, $options, $elementname = '{NAME}', $cr 'name' => '{NAME}[criteria][{CRITERION-id}][sortorder]', 'value' => $criterion['sortorder'])); $shortname = html_writer::empty_tag('input', array('type'=> 'text', - 'name' => '{NAME}[criteria][{CRITERION-id}][shortname]', 'value' => htmlspecialchars($criterion['shortname']), + 'name' => '{NAME}[criteria][{CRITERION-id}][shortname]', 'value' => $criterion['shortname'], 'id ' => '{NAME}[criteria][{CRITERION-id}][shortname]')); $shortname = html_writer::tag('div', $shortname, array('class'=>'criterionname')); - $description = html_writer::tag('textarea', htmlspecialchars($criterion['description']), + $description = html_writer::tag('textarea', s($criterion['description']), array('name' => '{NAME}[criteria][{CRITERION-id}][description]', 'cols' => '65', 'rows' => '5')); $description = html_writer::tag('div', $description, array('class'=>'criteriondesc')); - $descriptionmarkers = html_writer::tag('textarea', htmlspecialchars($criterion['descriptionmarkers']), + $descriptionmarkers = html_writer::tag('textarea', s($criterion['descriptionmarkers']), array('name' => '{NAME}[criteria][{CRITERION-id}][descriptionmarkers]', 'cols' => '65', 'rows' => '5')); $descriptionmarkers = html_writer::tag('div', $descriptionmarkers, array('class'=>'criteriondescmarkers')); $maxscore = html_writer::empty_tag('input', array('type'=> 'text', 'name' => '{NAME}[criteria][{CRITERION-id}][maxscore]', 'size' => '3', - 'value' => htmlspecialchars($criterion['maxscore']), + 'value' => $criterion['maxscore'], 'id' => '{NAME}[criteria][{CRITERION-id}][maxscore]')); $maxscore = html_writer::tag('div', $maxscore, array('class'=>'criterionmaxscore')); } else { @@ -125,7 +125,7 @@ public function criterion_template($mode, $options, $elementname = '{NAME}', $cr $mode == gradingform_guide_controller::DISPLAY_VIEW) { $descriptionclass = 'descriptionreadonly'; } - $shortname = html_writer::tag('div', $criterion['shortname'], + $shortname = html_writer::tag('div', s($criterion['shortname']), array('class'=>'criterionshortname', 'name' => '{NAME}[criteria][{CRITERION-id}][shortname]')); $descmarkerclass = ''; $descstudentclass = ''; @@ -137,13 +137,13 @@ public function criterion_template($mode, $options, $elementname = '{NAME}', $cr $descstudentclass = ' hide'; } } - $description = html_writer::tag('div', $criterion['description'], + $description = html_writer::tag('div', s($criterion['description']), array('class'=>'criteriondescription'.$descstudentclass, 'name' => '{NAME}[criteria][{CRITERION-id}][descriptionmarkers]')); - $descriptionmarkers = html_writer::tag('div', $criterion['descriptionmarkers'], + $descriptionmarkers = html_writer::tag('div', s($criterion['descriptionmarkers']), array('class'=>'criteriondescriptionmarkers'.$descmarkerclass, 'name' => '{NAME}[criteria][{CRITERION-id}][descriptionmarkers]')); - $maxscore = html_writer::tag('div', $criterion['maxscore'], + $maxscore = html_writer::tag('div', s($criterion['maxscore']), array('class'=>'criteriondescriptionscore', 'name' => '{NAME}[criteria][{CRITERION-id}][maxscore]')); } @@ -188,7 +188,7 @@ public function criterion_template($mode, $options, $elementname = '{NAME}', $cr $scoreclass = 'error'; $currentscore = $validationerrors[$criterion['id']]['score']; // Show invalid score in form. } - $input = html_writer::tag('textarea', htmlspecialchars($currentremark), + $input = html_writer::tag('textarea', s($currentremark), array('name' => '{NAME}[criteria][{CRITERION-id}][remark]', 'cols' => '65', 'rows' => '5', 'class' => 'markingguideremark')); $criteriontemplate .= html_writer::tag('td', $input, array('class' => 'remark')); @@ -197,7 +197,7 @@ public function criterion_template($mode, $options, $elementname = '{NAME}', $cr $score .= html_writer::empty_tag('input', array('type'=> 'text', 'name' => '{NAME}[criteria][{CRITERION-id}][score]', 'class' => $scoreclass, 'id' => '{NAME}[criteria][{CRITERION-id}][score]', - 'size' => '3', 'value' => htmlspecialchars($currentscore))); + 'size' => '3', 'value' => $currentscore)); $score .= '/'.$maxscore; $criteriontemplate .= html_writer::tag('td', $score, array('class' => 'score')); @@ -206,9 +206,9 @@ public function criterion_template($mode, $options, $elementname = '{NAME}', $cr 'name' => '{NAME}[criteria][{CRITERION-id}][remark]', 'value' => $currentremark)); } else if ($mode == gradingform_guide_controller::DISPLAY_REVIEW || $mode == gradingform_guide_controller::DISPLAY_VIEW) { - $criteriontemplate .= html_writer::tag('td', $currentremark, array('class' => 'remark')); + $criteriontemplate .= html_writer::tag('td', s($currentremark), array('class' => 'remark')); if (!empty($options['showmarkspercriterionstudents'])) { - $criteriontemplate .= html_writer::tag('td', htmlspecialchars($currentscore). ' / '.$maxscore, + $criteriontemplate .= html_writer::tag('td', s($currentscore). ' / '.$maxscore, array('class' => 'score')); } } @@ -267,7 +267,7 @@ public function comment_template($mode, $elementname = '{NAME}', $comment = null $criteriontemplate .= html_writer::end_tag('td'); // Controls. $criteriontemplate .= html_writer::empty_tag('input', array('type' => 'hidden', 'name' => '{NAME}[comments][{COMMENT-id}][sortorder]', 'value' => $comment['sortorder'])); - $description = html_writer::tag('textarea', htmlspecialchars($comment['description']), + $description = html_writer::tag('textarea', s($comment['description']), array('name' => '{NAME}[comments][{COMMENT-id}][description]', 'cols' => '65', 'rows' => '5')); $description = html_writer::tag('div', $description, array('class'=>'criteriondesc')); } else { @@ -278,12 +278,12 @@ public function comment_template($mode, $elementname = '{NAME}', $comment = null 'name' => '{NAME}[comments][{COMMENT-id}][description]', 'value' => $comment['description'])); } if ($mode == gradingform_guide_controller::DISPLAY_EVAL) { - $description = html_writer::tag('span', htmlspecialchars($comment['description']), + $description = html_writer::tag('span', s($comment['description']), array('name' => '{NAME}[comments][{COMMENT-id}][description]', 'title' => get_string('clicktocopy', 'gradingform_guide'), 'id' => '{NAME}[comments][{COMMENT-id}]', 'class'=>'markingguidecomment')); } else { - $description = $comment['description']; + $description = s($comment['description']); } } $descriptionclass = 'description'; From a99ad20d94e16967e72c5ea681515af70f06dcd1 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Tue, 8 Jul 2014 10:11:40 +0800 Subject: [PATCH 11/19] MDL-43948 forms: setting types to filemanager and editor elements --- lib/form/editor.php | 22 +++++++++++++++++++++- lib/form/filemanager.php | 22 ++++++++++++++++++++-- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/lib/form/editor.php b/lib/form/editor.php index bd04b222f3601..d97762844a071 100644 --- a/lib/form/editor.php +++ b/lib/form/editor.php @@ -94,6 +94,25 @@ function MoodleQuickForm_editor($elementName=null, $elementLabel=null, $attribut editors_head_setup(); } + /** + * Called by HTML_QuickForm whenever form event is made on this element + * + * @param string $event Name of event + * @param mixed $arg event arguments + * @param object $caller calling object + * @return bool + */ + function onQuickFormEvent($event, $arg, &$caller) + { + switch ($event) { + case 'createElement': + $caller->setType($arg[0] . '[format]', PARAM_ALPHANUM); + $caller->setType($arg[0] . '[itemid]', PARAM_INT); + break; + } + return parent::onQuickFormEvent($event, $arg, $caller); + } + /** * Sets name of editor * @@ -403,7 +422,8 @@ function toHtml() { if (!during_initial_install() && empty($CFG->adminsetuppending)) { // 0 means no files, -1 unlimited if ($maxfiles != 0 ) { - $str .= ''; + $str .= html_writer::empty_tag('input', array('type' => 'hidden', 'name' => $elname.'[itemid]', + 'value' => $draftitemid)); // used by non js editor only $editorurl = new moodle_url("$CFG->wwwroot/repository/draftfiles_manager.php", array( diff --git a/lib/form/filemanager.php b/lib/form/filemanager.php index 77ee16903e549..8140fec153d50 100644 --- a/lib/form/filemanager.php +++ b/lib/form/filemanager.php @@ -79,6 +79,24 @@ function MoodleQuickForm_filemanager($elementName=null, $elementLabel=null, $att parent::HTML_QuickForm_element($elementName, $elementLabel, $attributes); } + /** + * Called by HTML_QuickForm whenever form event is made on this element + * + * @param string $event Name of event + * @param mixed $arg event arguments + * @param object $caller calling object + * @return bool + */ + function onQuickFormEvent($event, $arg, &$caller) + { + switch ($event) { + case 'createElement': + $caller->setType($arg[0], PARAM_INT); + break; + } + return parent::onQuickFormEvent($event, $arg, $caller); + } + /** * Sets name of filemanager * @@ -263,9 +281,9 @@ function toHtml() { $output = $PAGE->get_renderer('core', 'files'); $html .= $output->render($fm); - $html .= ''; + $html .= html_writer::empty_tag('input', array('value' => $draftitemid, 'name' => $elname, 'type' => 'hidden')); // label element needs 'for' attribute work - $html .= ''; + $html .= html_writer::empty_tag('input', array('value' => '', 'id' => 'id_'.$elname, 'type' => 'hidden')); return $html; } From a1ae35173b54ed0c2c3736dfa78cad9899a55d4e Mon Sep 17 00:00:00 2001 From: Frederic Massart Date: Thu, 8 May 2014 16:12:52 +0800 Subject: [PATCH 12/19] MDL-45471 javascript: Escape content of exception dialogs --- lib/yui/notification/notification.js | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/lib/yui/notification/notification.js b/lib/yui/notification/notification.js index 2cda0bcd2f70e..07a9a4eefe93f 100644 --- a/lib/yui/notification/notification.js +++ b/lib/yui/notification/notification.js @@ -254,11 +254,11 @@ Y.extend(EXCEPTION, DIALOGUE, { _keypress : null, initializer : function(config) { this.get(BASE).addClass('moodle-dialogue-exception'); - this.setStdModContent(Y.WidgetStdMod.HEADER, '

' + config.name + '

', Y.WidgetStdMod.REPLACE); + this.setStdModContent(Y.WidgetStdMod.HEADER, '

' + Y.Escape.html(config.name) + '

', Y.WidgetStdMod.REPLACE); var content = C('
') - .append(C('
'+this.get('message')+'
')) - .append(C('')) - .append(C('')) + .append(C('
'+Y.Escape.html(this.get('message'))+'
')) + .append(C('')) + .append(C('')) .append(C('')); if (M.cfg.developerdebug) { content.all('.moodle-exception-param').removeClass('hidden'); @@ -300,7 +300,7 @@ Y.extend(EXCEPTION, DIALOGUE, { }, stack : { setter : function(str) { - var lines = str.split("\n"); + var lines = Y.Escape.html(str).split("\n"); var pattern = new RegExp('^(.+)@('+M.cfg.wwwroot+')?(.{0,75}).*:(\\d+)$'); for (var i in lines) { lines[i] = lines[i].replace(pattern, "
ln: $4
$3
$1
"); @@ -325,12 +325,12 @@ Y.extend(AJAXEXCEPTION, DIALOGUE, { _keypress : null, initializer : function(config) { this.get(BASE).addClass('moodle-dialogue-exception'); - this.setStdModContent(Y.WidgetStdMod.HEADER, '

' + config.name + '

', Y.WidgetStdMod.REPLACE); + this.setStdModContent(Y.WidgetStdMod.HEADER, '

' + Y.Escape.html(config.name) + '

', Y.WidgetStdMod.REPLACE); var content = C('
') - .append(C('
'+this.get('error')+'
')) + .append(C('
'+Y.Escape.html(this.get('error'))+'
')) .append(C('')) - .append(C('')) - .append(C('')); + .append(C('')) + .append(C('')); if (M.cfg.developerdebug) { content.all('.moodle-exception-param').removeClass('hidden'); } @@ -369,6 +369,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; @@ -389,4 +390,4 @@ M.core.confirm = CONFIRM; M.core.exception = EXCEPTION; M.core.ajaxException = AJAXEXCEPTION; -}, '@VERSION@', {requires:['base','node','panel','event-key', 'moodle-core-notification-skin', 'dd-plugin']}); +}, '@VERSION@', {requires:['base','node','panel','escape','event-key', 'moodle-core-notification-skin', 'dd-plugin']}); From 8380722bb11f36d33308580aee169e161d3f2c14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Wed, 9 Jul 2014 10:21:16 +0200 Subject: [PATCH 13/19] MDL-46223 Improve the display of marking guide validation message --- grade/grading/form/guide/lib.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grade/grading/form/guide/lib.php b/grade/grading/form/guide/lib.php index 33686a8a62c6b..6b20f4c3686dc 100644 --- a/grade/grading/form/guide/lib.php +++ b/grade/grading/form/guide/lib.php @@ -845,7 +845,7 @@ public function render_grading_element($page, $gradingformelement) { if (!empty($this->validationerrors)) { foreach ($this->validationerrors as $id => $err) { $a = new stdClass(); - $a->criterianame = $criteria[$id]['shortname']; + $a->criterianame = s($criteria[$id]['shortname']); $a->maxscore = $criteria[$id]['maxscore']; $html .= html_writer::tag('div', get_string('err_scoreinvalid', 'gradingform_guide', $a), array('class' => 'gradingform_guide-error')); From b5dacb548800ee10d4940c8ebeca48c3c2ae0512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Wed, 9 Jul 2014 10:28:56 +0200 Subject: [PATCH 14/19] MDL-46223 Fix frequently used comments in the marking guide When the frequently used comment contains a character like >, re-use it directly instead of its HTML entitiy. --- grade/grading/form/guide/js/guide.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grade/grading/form/guide/js/guide.js b/grade/grading/form/guide/js/guide.js index 1f1cd909d7d4f..09da8c4512b2a 100644 --- a/grade/grading/form/guide/js/guide.js +++ b/grade/grading/form/guide/js/guide.js @@ -10,7 +10,7 @@ M.gradingform_guide.init = function(Y, options) { currentfocus = e.currentTarget; }); Y.all('.markingguidecomment').on('click', function(e) { - currentfocus.set('value', currentfocus.get('value') + '\n' + e.currentTarget.get('innerHTML')); + currentfocus.set('value', currentfocus.get('value') + '\n' + e.currentTarget.get('text')); currentfocus.focus(); }); From 64d9b08a999fd49b19b12a4a9a3a68624bde18b8 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 9 Jul 2014 12:56:18 +0100 Subject: [PATCH 15/19] MDL-46148 qtype_calculated: removed unused method. --- question/type/calculated/questiontype.php | 43 ----------------------- 1 file changed, 43 deletions(-) diff --git a/question/type/calculated/questiontype.php b/question/type/calculated/questiontype.php index 735aff8ce0f0e..6947463ddbdec 100644 --- a/question/type/calculated/questiontype.php +++ b/question/type/calculated/questiontype.php @@ -346,49 +346,6 @@ protected function initialise_question_instance(question_definition $question, $ $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( - 'tolerancemustbenumeric', '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); } From c8beaf6d70228396a1a6fa8cc96f837be24505c9 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 9 Jul 2014 13:35:09 +0100 Subject: [PATCH 16/19] MDL-46148 qtype_calculated: fix validation when importing. In order to do this in a sane way, I cleaned up a lot of old mess, inclduing: 1. Previously, qtype_calcuated used ->answeres when importing, and ->answer when saving the form. This was crazy, so I fixed it, and stripped out the code that made the alternative variable name work. 2. Similarly, it could handle ->answer being either an array, such as you would get form the HTML editor, or a simple string, which is what you get form the form. I simplified that too. 3. Finally, I made import use a transaction around saving each question, so we don't get half questions in the database when an error occurs. --- question/format.php | 6 ++ question/format/webct/format.php | 7 +- question/format/xml/format.php | 4 +- question/type/calculated/questiontype.php | 67 ++++++++++--------- .../type/calculatedmulti/questiontype.php | 19 ++++-- .../type/calculatedsimple/questiontype.php | 13 ++-- 6 files changed, 61 insertions(+), 55 deletions(-) diff --git a/question/format.php b/question/format.php index cb3564a1c9b40..4a9e19548d5b6 100644 --- a/question/format.php +++ b/question/format.php @@ -357,6 +357,7 @@ public function importprocess($category) { $count = 0; foreach ($questions as $question) { // Process and store each question + $transaction = $DB->start_delegated_transaction(); // reset the php timeout set_time_limit(0); @@ -429,9 +430,14 @@ public function importprocess($category) { 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 7f36acccea01e..b12c0618c42f0 100644 --- a/question/format/webct/format.php +++ b/question/format/webct/format.php @@ -606,12 +606,9 @@ public function readquestions ($lines) { // 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(); @@ -736,7 +733,7 @@ public function readquestions ($lines) { 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 b713595cc6188..128085802e739 100644 --- a/question/format/xml/format.php +++ b/question/format/xml/format.php @@ -774,7 +774,7 @@ public function import_calculated($question) { // get answers array $answers = $question['#']['answer']; - $qo->answers = array(); + $qo->answer = array(); $qo->feedback = array(); $qo->fraction = array(); $qo->tolerance = array(); @@ -788,7 +788,7 @@ public function import_calculated($question) { 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/questiontype.php b/question/type/calculated/questiontype.php index 6947463ddbdec..82792426309f7 100644 --- a/question/type/calculated/questiontype.php +++ b/question/type/calculated/questiontype.php @@ -131,12 +131,14 @@ public function get_datasets_for_export($question) { public function save_question_options($question) { global $CFG, $DB; - // the code is used for calculated, calculatedsimple and calculatedmulti qtypes + + // 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 + + // Calculated options. $update = true; $options = $DB->get_record('question_calculated_options', array('question' => $question->id)); @@ -185,14 +187,7 @@ public function save_question_options($question) { $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; } @@ -471,6 +466,25 @@ protected function validate_answer($answer) { } } + /** + * 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 @@ -483,12 +497,13 @@ protected function validate_answer($answer) { * @param object $form * @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 + 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 $key => $answer) { + foreach ($form->answer as $key => $answer) { $mandatorydatasets += $this->find_dataset_names($answer); } // if there are identical datasetdefs already saved in the original question. @@ -578,12 +593,6 @@ public function addnamecategory(&$question) { 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); return $question; @@ -602,16 +611,8 @@ public function save_question($question, $form) { // See where we're coming from switch($wizardnow) { 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']); - } - + case 'question': // Coming from the first page, creating the second. + if (empty($form->id)) { // or a new question $form->id is empty. $question = parent::save_question($question, $form); //prepare the datasets using default $questionfromid $this->preparedatasets($form); diff --git a/question/type/calculatedmulti/questiontype.php b/question/type/calculatedmulti/questiontype.php index 5c0179fab95c6..50b2ed406cddd 100644 --- a/question/type/calculatedmulti/questiontype.php +++ b/question/type/calculatedmulti/questiontype.php @@ -42,7 +42,10 @@ public function save_question_options($question) { global $CFG, $DB; $context = $question->context; - // Calculated options + // Make it impossible to save bad formulas anywhere. + $this->validate_question_data($question); + + // Calculated options. $update = true; $options = $DB->get_record('question_calculated_options', array('question' => $question->id)); @@ -71,11 +74,8 @@ public function save_question_options($question) { $oldoptions = array(); } - // Insert all the new answers - if (isset($question->answer) && !isset($question->answers)) { - $question->answers = $question->answer; - } - foreach ($question->answers as $key => $answerdata) { + // Insert all the new answers. + foreach ($question->answer as $key => $answerdata) { if (is_array($answerdata)) { $answerdata = $answerdata['text']; } @@ -163,6 +163,13 @@ protected function validate_answer($answer) { } } + protected function validate_question_data($question) { + parent::validate_question_data($question); + $this->validate_text($question->correctfeedback['text']); + $this->validate_text($question->partiallycorrectfeedback['text']); + $this->validate_text($question->incorrectfeedback['text']); + } + protected function make_question_instance($questiondata) { question_bank::load_question_definition_classes($this->name()); if ($questiondata->options->single) { diff --git a/question/type/calculatedsimple/questiontype.php b/question/type/calculatedsimple/questiontype.php index 035df5cd2dd5c..1f46d8c548318 100644 --- a/question/type/calculatedsimple/questiontype.php +++ b/question/type/calculatedsimple/questiontype.php @@ -43,11 +43,9 @@ class qtype_calculatedsimple extends qtype_calculated { public function save_question_options($question) { global $CFG, $DB; $context = $question->context; - // Get old answers: - if (isset($question->answer) && !isset($question->answers)) { - $question->answers = $question->answer; - } + // Make it impossible to save bad formulas anywhere. + $this->validate_question_data($question); // Get old versions of the objects if (!$oldanswers = $DB->get_records('question_answers', @@ -68,11 +66,8 @@ public function save_question_options($question) { } else { $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) { + // Insert all the new answers. + foreach ($question->answer as $key => $answerdata) { if (is_array($answerdata)) { $answerdata = $answerdata['text']; } From 2576939e4daa048cfc5b9b0a5a6fd81b65306822 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 9 Jul 2014 13:47:40 +0100 Subject: [PATCH 17/19] MDL-46148 qtype_calculatedsimple: fix notice ... when adding a dataset with a formula error. --- question/type/calculated/questiontype.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/question/type/calculated/questiontype.php b/question/type/calculated/questiontype.php index 82792426309f7..ceca8be695de0 100644 --- a/question/type/calculated/questiontype.php +++ b/question/type/calculated/questiontype.php @@ -1063,10 +1063,14 @@ public function comment_on_datasetitems($qtypeobj, $questionid, $questiontext, } $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, From bc4fd2776863822a6a9a3880e22717e05ce0cbe1 Mon Sep 17 00:00:00 2001 From: Damyon Wiese Date: Thu, 10 Jul 2014 11:35:47 +0800 Subject: [PATCH 18/19] MDL-46148 questions: Added missing allow_commit for transaction --- question/format.php | 1 + 1 file changed, 1 insertion(+) diff --git a/question/format.php b/question/format.php index 4a9e19548d5b6..716a48ea39ed9 100644 --- a/question/format.php +++ b/question/format.php @@ -372,6 +372,7 @@ public function importprocess($category) { $this->category = $newcategory; } } + $transaction->allow_commit(); continue; } $question->context = $this->importcontext; From c110815236e57d468a2c974ad06f12e7186ce5d7 Mon Sep 17 00:00:00 2001 From: Ankit Agarwal Date: Thu, 10 Jul 2014 17:44:59 +0800 Subject: [PATCH 19/19] MDL-46148 qtype_calculated: low-level defence against bad formulas This catches things like: * Malicious equations coming from backup files. * Malicious equations in old questions in the database. --- question/type/calculated/question.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/question/type/calculated/question.php b/question/type/calculated/question.php index 76811f13fa6a4..64ee4f19150d6 100644 --- a/question/type/calculated/question.php +++ b/question/type/calculated/question.php @@ -342,6 +342,10 @@ public function get_values() { * @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)); }