From 2ca3bffa959a6c2621b2b075b73b4bcbcd9b445e Mon Sep 17 00:00:00 2001 From: Petr Skoda Date: Wed, 17 Aug 2011 16:26:32 +0200 Subject: [PATCH 1/5] MDL-26796 do not accept arrays in optional_param() and required_param(), add new functions --- lib/moodlelib.php | 164 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 143 insertions(+), 21 deletions(-) diff --git a/lib/moodlelib.php b/lib/moodlelib.php index e797bf67fed..c651300bb23 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -448,17 +448,15 @@ define('MOODLE_OFFICIAL_MOBILE_SERVICE', 'moodle_mobile_app'); * used like this: * $id = required_param('id', PARAM_INT); * - * Please note the $type parameter is now required, - * for now PARAM_CLEAN is used for backwards compatibility only. + * Please note the $type parameter is now required and the value can not be array. * * @param string $parname the name of the page parameter we want * @param string $type expected type of parameter * @return mixed */ function required_param($parname, $type) { - if (!isset($type)) { - debugging('required_param() requires $type to be specified.'); - $type = PARAM_CLEAN; // for now let's use this deprecated type + if (func_num_args() != 2 or empty($parname) or empty($type)) { + throw new coding_exception('required_param() requires $parname and $type to be specified (parameter: '.$parname.')'); } if (isset($_POST[$parname])) { // POST has precedence $param = $_POST[$parname]; @@ -468,7 +466,57 @@ function required_param($parname, $type) { print_error('missingparam', '', '', $parname); } - return clean_param($param, $type); + if (is_array($param)) { + debugging('Invalid array parameter detected in required_param(): '.$parname); + // TODO: switch to fatal error in Moodle 2.3 + //print_error('missingparam', '', '', $parname); + return required_param_array($parname, $type); + } + + return clean_param($param, $type, false); +} + +/** + * Returns a particular array value for the named variable, taken from + * POST or GET. If the parameter doesn't exist then an error is + * thrown because we require this variable. + * + * This function should be used to initialise all required values + * in a script that are based on parameters. Usually it will be + * used like this: + * $ids = required_param_array('ids', PARAM_INT); + * + * Note: arrays of arrays are not supported, only alphanumeric keys with _ and - are supported + * + * @param string $parname the name of the page parameter we want + * @param string $type expected type of parameter + * @return array + */ +function required_param_array($parname, $type) { + if (func_num_args() != 2 or empty($parname) or empty($type)) { + throw new coding_exception('required_param_array() requires $parname and $type to be specified (parameter: '.$parname.')'); + } + if (isset($_POST[$parname])) { // POST has precedence + $param = $_POST[$parname]; + } else if (isset($_GET[$parname])) { + $param = $_GET[$parname]; + } else { + print_error('missingparam', '', '', $parname); + } + if (!is_array($param)) { + print_error('missingparam', '', '', $parname); + } + + $result = array(); + foreach($param as $key=>$value) { + if (!preg_match('/^[a-z0-9_-]+$/i', $key)) { + debugging('Invalid key name in required_param_array() detected: '.$key.', parameter: '.$parname); + continue; + } + $result[$key] = clean_param($value, $type, false); + } + + return $result; } /** @@ -480,8 +528,7 @@ function required_param($parname, $type) { * used like this: * $name = optional_param('name', 'Fred', PARAM_TEXT); * - * Please note $default and $type parameters are now required, - * for now PARAM_CLEAN is used for backwards compatibility only. + * Please note the $type parameter is now required and the value can not be array. * * @param string $parname the name of the page parameter we want * @param mixed $default the default value to return if nothing is found @@ -489,9 +536,8 @@ function required_param($parname, $type) { * @return mixed */ function optional_param($parname, $default, $type) { - if (!isset($type)) { - debugging('optional_param() requires $default and $type to be specified.'); - $type = PARAM_CLEAN; // for now let's use this deprecated type + if (func_num_args() != 3 or empty($parname) or empty($type)) { + throw new coding_exception('optional_param() requires $parname, $default and $type to be specified (parameter: '.$parname.')'); } if (!isset($default)) { $default = null; @@ -505,7 +551,59 @@ function optional_param($parname, $default, $type) { return $default; } - return clean_param($param, $type); + if (is_array($param)) { + debugging('Invalid array parameter detected in required_param(): '.$parname); + // TODO: switch to $default in Moodle 2.3 + //return $default; + return optional_param_array($parname, $default, $type); + } + + return clean_param($param, $type, false); +} + +/** + * Returns a particular array value for the named variable, taken from + * POST or GET, otherwise returning a given default. + * + * This function should be used to initialise all optional values + * in a script that are based on parameters. Usually it will be + * used like this: + * $ids = optional_param('id', array(), PARAM_INT); + * + * Note: arrays of arrays are not supported, only alphanumeric keys with _ and - are supported + * + * @param string $parname the name of the page parameter we want + * @param mixed $default the default value to return if nothing is found + * @param string $type expected type of parameter + * @return array + */ +function optional_param_array($parname, $default, $type) { + if (func_num_args() != 3 or empty($parname) or empty($type)) { + throw new coding_exception('optional_param_array() requires $parname, $default and $type to be specified (parameter: '.$parname.')'); + } + + if (isset($_POST[$parname])) { // POST has precedence + $param = $_POST[$parname]; + } else if (isset($_GET[$parname])) { + $param = $_GET[$parname]; + } else { + return $default; + } + if (!is_array($param)) { + debugging('optional_param_array() expects array parameters only: '.$parname); + return $default; + } + + $result = array(); + foreach($param as $key=>$value) { + if (!preg_match('/^[a-z0-9_-]+$/i', $key)) { + debugging('Invalid key name in optional_param_array() detected: '.$key.', parameter: '.$parname); + continue; + } + $result[$key] = clean_param($value, $type, false); + } + + return $result; } /** @@ -516,7 +614,7 @@ function optional_param($parname, $default, $type) { * Objects and classes are not accepted. * * @param mixed $param - * @param int $type PARAM_ constant + * @param string $type PARAM_ constant * @param bool $allownull are nulls valid value? * @param string $debuginfo optional debug information * @return mixed the $param value converted to PHP type or invalid_parameter_exception @@ -533,7 +631,7 @@ function validate_param($param, $type, $allownull=NULL_NOT_ALLOWED, $debuginfo=' throw new invalid_parameter_exception($debuginfo); } - $cleaned = clean_param($param, $type); + $cleaned = clean_param($param, $type, false); if ((string)$param !== (string)$cleaned) { // conversion to string is usually lossless throw new invalid_parameter_exception($debuginfo); @@ -542,6 +640,34 @@ function validate_param($param, $type, $allownull=NULL_NOT_ALLOWED, $debuginfo=' return $cleaned; } +/** + * Makes sure array contains only the allowed types, + * this function does not validate array key names! + * + * $options = clean_param($options, PARAM_INT); + * + * + * @param array $param the variable array we are cleaning + * @param string $type expected format of param after cleaning. + * @param bool $recursive clean recursive arrays + * @return array + */ +function clean_param_array(array $param = null, $type, $recursive = false) { + $param = (array)$param; // convert null to empty array + foreach ($param as $key => $value) { + if (is_array($value)) { + if ($recursive) { + $param[$key] = clean_param_array($value, $type, true); + } else { + throw new coding_exception('clean_param_array() can not process multidimensional arrays when $recursive is false.'); + } + } else { + $param[$key] = clean_param($value, $type); + } + } + return $param; +} + /** * Used by {@link optional_param()} and {@link required_param()} to * clean the variables and/or cast to specific types, based on @@ -552,19 +678,15 @@ function validate_param($param, $type, $allownull=NULL_NOT_ALLOWED, $debuginfo=' * * * @param mixed $param the variable we are cleaning - * @param int $type expected format of param after cleaning. + * @param string $type expected format of param after cleaning. * @return mixed */ function clean_param($param, $type) { global $CFG; - if (is_array($param)) { // Let's loop - $newparam = array(); - foreach ($param as $key => $value) { - $newparam[$key] = clean_param($value, $type); - } - return $newparam; + if (is_object($param) or is_array($param)) { + throw new coding_exception('clean_param() can not process objects or arrays, please use clean_param_array() instead.'); } switch ($type) { From 923aff64fa2519bd79ac4cc61726d0e51d8c0f85 Mon Sep 17 00:00:00 2001 From: Petr Skoda Date: Wed, 17 Aug 2011 16:26:53 +0200 Subject: [PATCH 2/5] MDL-26796 add unit tests for parameter cleanup --- lib/simpletest/testmoodlelib.php | 335 ++++++++++++++++++++++++++++++- 1 file changed, 332 insertions(+), 3 deletions(-) diff --git a/lib/simpletest/testmoodlelib.php b/lib/simpletest/testmoodlelib.php index 702ecc17e00..34c547ac02c 100644 --- a/lib/simpletest/testmoodlelib.php +++ b/lib/simpletest/testmoodlelib.php @@ -318,15 +318,344 @@ class moodlelib_test extends UnitTestCase { } function test_optional_param() { + global $CFG; + $_POST['username'] = 'post_user'; $_GET['username'] = 'get_user'; - $this->assertEqual(optional_param('username', 'default_user', PARAM_RAW), 'post_user'); + $this->assertIdentical(optional_param('username', 'default_user', PARAM_RAW), $_POST['username']); unset($_POST['username']); - $this->assertEqual(optional_param('username', 'default_user', PARAM_RAW), 'get_user'); + $this->assertIdentical(optional_param('username', 'default_user', PARAM_RAW), $_GET['username']); unset($_GET['username']); - $this->assertEqual(optional_param('username', 'default_user', PARAM_RAW), 'default_user'); + $this->assertIdentical(optional_param('username', 'default_user', PARAM_RAW), 'default_user'); + + // make sure exception is triggered when some params are missing, hide error notices here - new in 2.2 + $_POST['username'] = 'post_user'; + try { + optional_param('username', 'default_user', null); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + try { + @optional_param('username', 'default_user'); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + try { + @optional_param('username'); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + try { + optional_param('', 'default_user', PARAM_RAW); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + + // make sure warning is displayed if array submitted - TODO: throw exception in Moodle 2.3 + $debugging = isset($CFG->debug) ? $CFG->debug : null; + $debugdisplay = isset($CFG->debugdisplay) ? $CFG->debugdisplay : null; + $CFG->debug = 38911; + $CFG->debugdisplay = true; + + ob_start(); + $this->assertIdentical(optional_param('username', 'default_user', PARAM_RAW), $_POST['username']); + $d = ob_end_clean(); + $this->assertTrue($d !== ''); + + if ($debugging !== null) { + $CFG->debug = $debugging; + } else { + unset($CFG->debug); + } + if ($debugdisplay !== null) { + $CFG->debugdisplay = $debugdisplay; + } else { + unset($CFG->debugdisplay); + } + } + + function test_optional_param_array() { + global $CFG; + + $_POST['username'] = array('a'=>'post_user'); + $_GET['username'] = array('a'=>'get_user'); + $this->assertIdentical(optional_param_array('username', array('a'=>'default_user'), PARAM_RAW), $_POST['username']); + + unset($_POST['username']); + $this->assertIdentical(optional_param_array('username', array('a'=>'default_user'), PARAM_RAW), $_GET['username']); + + unset($_GET['username']); + $this->assertIdentical(optional_param_array('username', array('a'=>'default_user'), PARAM_RAW), array('a'=>'default_user')); + + // make sure exception is triggered when some params are missing, hide error notices here - new in 2.2 + $_POST['username'] = array('a'=>'post_user'); + try { + optional_param_array('username', array('a'=>'default_user'), null); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + try { + @optional_param_array('username', array('a'=>'default_user')); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + try { + @optional_param_array('username'); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + try { + optional_param_array('', array('a'=>'default_user'), PARAM_RAW); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + + // do not allow nested arrays + try { + $_POST['username'] = array('a'=>array('b'=>'post_user')); + optional_param_array('username', array('a'=>'default_user'), PARAM_RAW); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + + // do not allow non-arrays + $debugging = isset($CFG->debug) ? $CFG->debug : null; + $debugdisplay = isset($CFG->debugdisplay) ? $CFG->debugdisplay : null; + $CFG->debug = 38911; + $CFG->debugdisplay = true; + + ob_start(); + $_POST['username'] = 'post_user'; + $this->assertIdentical(optional_param_array('username', array('a'=>'default_user'), PARAM_RAW), array('a'=>'default_user')); + $d = ob_end_clean(); + $this->assertTrue($d !== ''); + + // make sure array keys are sanitised + ob_start(); + $_POST['username'] = array('abc123_;-/*-+ '=>'arrggh', 'a1_-'=>'post_user'); + $this->assertIdentical(optional_param_array('username', array(), PARAM_RAW), array('a1_-'=>'post_user')); + $d = ob_end_clean(); + $this->assertTrue($d !== ''); + + if ($debugging !== null) { + $CFG->debug = $debugging; + } else { + unset($CFG->debug); + } + if ($debugdisplay !== null) { + $CFG->debugdisplay = $debugdisplay; + } else { + unset($CFG->debugdisplay); + } + } + + function test_required_param() { + global $CFG; + + $_POST['username'] = 'post_user'; + $_GET['username'] = 'get_user'; + $this->assertIdentical(required_param('username', PARAM_RAW), 'post_user'); + + unset($_POST['username']); + $this->assertIdentical(required_param('username', PARAM_RAW), 'get_user'); + + unset($_GET['username']); + try { + $this->assertIdentical(required_param('username', PARAM_RAW), 'default_user'); + $this->fail('moodle_exception expected'); + } catch (moodle_exception $ex) { + $this->assertTrue(true); + } + + // make sure exception is triggered when some params are missing, hide error notices here - new in 2.2 + $_POST['username'] = 'post_user'; + try { + @required_param('username'); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + try { + required_param('username', ''); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + try { + required_param('', PARAM_RAW); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + + // make sure warning is displayed if array submitted - TODO: throw exception in Moodle 2.3 + $debugging = isset($CFG->debug) ? $CFG->debug : null; + $debugdisplay = isset($CFG->debugdisplay) ? $CFG->debugdisplay : null; + $CFG->debug = 38911; + $CFG->debugdisplay = true; + + ob_start(); + $this->assertIdentical(required_param('username', PARAM_RAW), $_POST['username']); + $d = ob_end_clean(); + $this->assertTrue($d !== ''); + + if ($debugging !== null) { + $CFG->debug = $debugging; + } else { + unset($CFG->debug); + } + if ($debugdisplay !== null) { + $CFG->debugdisplay = $debugdisplay; + } else { + unset($CFG->debugdisplay); + } + } + + function test_required_param_array() { + global $CFG; + + $_POST['username'] = array('a'=>'post_user'); + $_GET['username'] = array('a'=>'get_user'); + $this->assertIdentical(required_param_array('username', PARAM_RAW), $_POST['username']); + + unset($_POST['username']); + $this->assertIdentical(required_param_array('username', PARAM_RAW), $_GET['username']); + + // make sure exception is triggered when some params are missing, hide error notices here - new in 2.2 + $_POST['username'] = array('a'=>'post_user'); + try { + required_param_array('username', null); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + try { + @required_param_array('username'); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + try { + required_param_array('', PARAM_RAW); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + + // do not allow nested arrays + try { + $_POST['username'] = array('a'=>array('b'=>'post_user')); + required_param_array('username', PARAM_RAW); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + + // do not allow non-arrays + try { + $_POST['username'] = 'post_user'; + required_param_array('username', PARAM_RAW); + $this->fail('moodle_exception expected'); + } catch (moodle_exception $ex) { + $this->assertTrue(true); + } + + // do not allow non-arrays + $debugging = isset($CFG->debug) ? $CFG->debug : null; + $debugdisplay = isset($CFG->debugdisplay) ? $CFG->debugdisplay : null; + $CFG->debug = 38911; + $CFG->debugdisplay = true; + + // make sure array keys are sanitised + ob_start(); + $_POST['username'] = array('abc123_;-/*-+ '=>'arrggh', 'a1_-'=>'post_user'); + $this->assertIdentical(required_param_array('username', PARAM_RAW), array('a1_-'=>'post_user')); + $d = ob_end_clean(); + $this->assertTrue($d !== ''); + + if ($debugging !== null) { + $CFG->debug = $debugging; + } else { + unset($CFG->debug); + } + if ($debugdisplay !== null) { + $CFG->debugdisplay = $debugdisplay; + } else { + unset($CFG->debugdisplay); + } + } + + function test_clean_param() { + // forbid objects and arrays + try { + clean_param(array('x', 'y'), PARAM_RAW); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + try { + $param = new stdClass(); + $param->id = 1; + clean_param($param, PARAM_RAW); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + + // require correct type + try { + clean_param('x', 'xxxxxx'); + $this->fail('moodle_exception expected'); + } catch (moodle_exception $ex) { + $this->assertTrue(true); + } + try { + @clean_param('x'); + $this->fail('moodle_exception expected'); + } catch (moodle_exception $ex) { + $this->assertTrue(true); + } + + } + + function test_clean_param_array() { + $this->assertIdentical(clean_param_array(null, PARAM_RAW), array()); + $this->assertIdentical(clean_param_array(array('a', 'b'), PARAM_RAW), array('a', 'b')); + $this->assertIdentical(clean_param_array(array('a', array('b')), PARAM_RAW, true), array('a', array('b'))); + + // require correct type + try { + clean_param_array(array('x'), 'xxxxxx'); + $this->fail('moodle_exception expected'); + } catch (moodle_exception $ex) { + $this->assertTrue(true); + } + try { + @clean_param_array(array('x')); + $this->fail('moodle_exception expected'); + } catch (moodle_exception $ex) { + $this->assertTrue(true); + } + + try { + clean_param_array(array('x', array('y')), PARAM_RAW); + $this->fail('coding_exception expected'); + } catch (coding_exception $ex) { + $this->assertTrue(true); + } + + // test recursive } function test_clean_param_raw() { From 18bd7573d6b0837100e87f45d50ba2913075e0ff Mon Sep 17 00:00:00 2001 From: Petr Skoda Date: Wed, 17 Aug 2011 16:27:15 +0200 Subject: [PATCH 3/5] MDL-26796 convert array parameter cleaning to new functions --- admin/langimport.php | 2 +- admin/report/capability/index.php | 2 +- admin/report/customlang/edit.php | 4 ++-- admin/report/unittest/dbtest.php | 2 +- group/index.php | 2 +- lib/filelib.php | 17 +++++++++++++---- lib/formslib.php | 9 +++++++-- mod/choice/report.php | 4 ++-- mod/choice/view.php | 2 +- mod/data/field/checkbox/field.class.php | 2 +- mod/data/field/multimenu/field.class.php | 2 +- mod/quiz/report/grading/report.php | 4 ++-- mod/quiz/report/overview/report.php | 4 ++-- mod/quiz/report/responses/report.php | 2 +- mod/scorm/report/basic/report.php | 2 +- repository/lib.php | 2 +- repository/repository_ajax.php | 2 +- repository/upload/lib.php | 2 +- tag/manage.php | 6 +++--- user/addnote.php | 6 +++--- user/groupaddnote.php | 2 +- user/selector/lib.php | 2 +- 22 files changed, 48 insertions(+), 34 deletions(-) diff --git a/admin/langimport.php b/admin/langimport.php index 4aa614b2193..713f608ad0b 100644 --- a/admin/langimport.php +++ b/admin/langimport.php @@ -43,7 +43,7 @@ if (!empty($CFG->skiplangupgrade)) { } $mode = optional_param('mode', 0, PARAM_INT); // action -$pack = optional_param('pack', array(), PARAM_SAFEDIR); // pack to install +$pack = optional_param_array('pack', array(), PARAM_SAFEDIR); // pack to install $uninstalllang = optional_param('uninstalllang', '', PARAM_LANG); // installed pack to uninstall $confirm = optional_param('confirm', 0, PARAM_BOOL); // uninstallation confirmation diff --git a/admin/report/capability/index.php b/admin/report/capability/index.php index c6afaf39be2..71ba109f94e 100644 --- a/admin/report/capability/index.php +++ b/admin/report/capability/index.php @@ -18,7 +18,7 @@ require_capability('moodle/role:manage', $systemcontext); // Get URL parameters. $capability = optional_param('capability', '', PARAM_CAPABILITY); -$roleids = optional_param('roles', array('0'), PARAM_INTEGER); +$roleids = optional_param_array('roles', array('0'), PARAM_INTEGER); // Clean the passed in list of role ids. If 'All' selected as an option, or // if none were selected, do all roles. diff --git a/admin/report/customlang/edit.php b/admin/report/customlang/edit.php index 6e9a7f26a4d..99c2ed9ae6b 100644 --- a/admin/report/customlang/edit.php +++ b/admin/report/customlang/edit.php @@ -59,8 +59,8 @@ if ($filter->is_cancelled()) { } if ($translatorsubmitted) { - $strings = optional_param('cust', array(), PARAM_RAW); - $updates = optional_param('updates', array(), PARAM_INT); + $strings = optional_param_array('cust', array(), PARAM_RAW); + $updates = optional_param_array('updates', array(), PARAM_INT); $checkin = optional_param('savecheckin', false, PARAM_RAW); if ($checkin === false) { diff --git a/admin/report/unittest/dbtest.php b/admin/report/unittest/dbtest.php index 63060a16603..ae57bcfb68f 100644 --- a/admin/report/unittest/dbtest.php +++ b/admin/report/unittest/dbtest.php @@ -14,7 +14,7 @@ require_once('ex_reporter.php'); $showpasses = optional_param('showpasses', false, PARAM_BOOL); $codecoverage = optional_param('codecoverage', false, PARAM_BOOL); -$selected = optional_param('selected', array(), PARAM_INT); +$selected = optional_param_array('selected', array(), PARAM_INT); // Print the header and check access. admin_externalpage_setup('reportdbtest'); diff --git a/group/index.php b/group/index.php index 0eeaf4d2926..3d8bfb03c1d 100644 --- a/group/index.php +++ b/group/index.php @@ -19,7 +19,7 @@ $action = groups_param_action(); if ($groupid) { $groupids = array($groupid); } else { - $groupids = optional_param('groups', array(), PARAM_INT); + $groupids = optional_param_array('groups', array(), PARAM_INT); } $singlegroup = (count($groupids) == 1); diff --git a/lib/filelib.php b/lib/filelib.php index 5cc33d0e213..1e0ef669b04 100644 --- a/lib/filelib.php +++ b/lib/filelib.php @@ -588,18 +588,27 @@ function file_get_drafarea_files($draftitemid, $filepath = '/') { * @return integer the itemid, or 0 if there is not one yet. */ function file_get_submitted_draft_itemid($elname) { - $param = optional_param($elname, 0, PARAM_INT); - if ($param) { - require_sesskey(); + // this is a nasty hack, ideally all new elements should use arrays here or there should be a new parameter + if (!isset($_REQUEST[$elname])) { + return 0; } - if (is_array($param)) { + if (is_array($_REQUEST[$elname])) { + $param = optional_param_array($elname, 0, PARAM_INT); if (!empty($param['itemid'])) { $param = $param['itemid']; } else { debugging('Missing itemid, maybe caused by unset maxfiles option', DEBUG_DEVELOPER); return false; } + + } else { + $param = optional_param($elname, 0, PARAM_INT); } + + if ($param) { + require_sesskey(); + } + return $param; } diff --git a/lib/formslib.php b/lib/formslib.php index b8292051008..4b02f70f9ed 100644 --- a/lib/formslib.php +++ b/lib/formslib.php @@ -1384,9 +1384,14 @@ class MoodleQuickForm extends HTML_QuickForm_DHTMLRulesTableless { } else { foreach ($submission as $key=>$s) { if (array_key_exists($key, $this->_types)) { - $submission[$key] = clean_param($s, $this->_types[$key]); + $type = $this->_types[$key]; } else { - $submission[$key] = clean_param($s, PARAM_RAW); + $type = PARAM_RAW; + } + if (is_array($s)) { + $submission[$key] = clean_param_array($s, $type, true); + } else { + $submission[$key] = clean_param($s, $type); } } $this->_submitValues = $submission; diff --git a/mod/choice/report.php b/mod/choice/report.php index bbd673b0010..b9ef1455909 100644 --- a/mod/choice/report.php +++ b/mod/choice/report.php @@ -7,7 +7,7 @@ $format = optional_param('format', CHOICE_PUBLISH_NAMES, PARAM_INT); $download = optional_param('download', '', PARAM_ALPHA); $action = optional_param('action', '', PARAM_ALPHA); - $attemptids = optional_param('attemptid', array(), PARAM_INT); //get array of responses to delete. + $attemptids = optional_param_array('attemptid', array(), PARAM_INT); //get array of responses to delete. $url = new moodle_url('/mod/choice/report.php', array('id'=>$id)); if ($format !== CHOICE_PUBLISH_NAMES) { @@ -219,7 +219,7 @@ $results = prepare_choice_show_results($choice, $course, $cm, $users); $renderer = $PAGE->get_renderer('mod_choice'); echo $renderer->display_result($results, has_capability('mod/choice:readresponses', $context)); - + //now give links for downloading spreadsheets. if (!empty($users) && has_capability('mod/choice:downloadresponses',$context)) { $downloadoptions = array(); diff --git a/mod/choice/view.php b/mod/choice/view.php index 2e11c5e491e..b35e017c526 100644 --- a/mod/choice/view.php +++ b/mod/choice/view.php @@ -6,7 +6,7 @@ $id = required_param('id', PARAM_INT); // Course Module ID $action = optional_param('action', '', PARAM_ALPHA); - $attemptids = optional_param('attemptid', array(), PARAM_INT); // array of attempt ids for delete action + $attemptids = optional_param_array('attemptid', array(), PARAM_INT); // array of attempt ids for delete action $url = new moodle_url('/mod/choice/view.php', array('id'=>$id)); if ($action !== '') { diff --git a/mod/data/field/checkbox/field.class.php b/mod/data/field/checkbox/field.class.php index 8b5c8987459..7571d077a5a 100644 --- a/mod/data/field/checkbox/field.class.php +++ b/mod/data/field/checkbox/field.class.php @@ -96,7 +96,7 @@ class data_field_checkbox extends data_field_base { } function parse_search_field() { - $selected = optional_param('f_'.$this->field->id, array(), PARAM_NOTAGS); + $selected = optional_param_array('f_'.$this->field->id, array(), PARAM_NOTAGS); $allrequired = optional_param('f_'.$this->field->id.'_allreq', 0, PARAM_BOOL); if (empty($selected)) { // no searching diff --git a/mod/data/field/multimenu/field.class.php b/mod/data/field/multimenu/field.class.php index 170af176b0a..f3fe90b161d 100644 --- a/mod/data/field/multimenu/field.class.php +++ b/mod/data/field/multimenu/field.class.php @@ -122,7 +122,7 @@ class data_field_multimenu extends data_field_base { } function parse_search_field() { - $selected = optional_param('f_'.$this->field->id, array(), PARAM_NOTAGS); + $selected = optional_param_array('f_'.$this->field->id, array(), PARAM_NOTAGS); $allrequired = optional_param('f_'.$this->field->id.'_allreq', 0, PARAM_BOOL); if (empty($selected)) { // no searching diff --git a/mod/quiz/report/grading/report.php b/mod/quiz/report/grading/report.php index 44c420f56e1..4babb14b1f3 100644 --- a/mod/quiz/report/grading/report.php +++ b/mod/quiz/report/grading/report.php @@ -439,7 +439,7 @@ class quiz_grading_report extends quiz_default_report { if (!$qubaids) { return false; } - $qubaids = clean_param(explode(',', $qubaids), PARAM_INT); + $qubaids = clean_param_array(explode(',', $qubaids), PARAM_INT); $slots = optional_param('slots', '', PARAM_SEQUENCE); if (!$slots) { @@ -471,7 +471,7 @@ class quiz_grading_report extends quiz_default_report { return; } - $qubaids = clean_param(explode(',', $qubaids), PARAM_INT); + $qubaids = clean_param_array(explode(',', $qubaids), PARAM_INT); $attempts = $this->load_attempts_by_usage_ids($qubaids); $transaction = $DB->start_delegated_transaction(); diff --git a/mod/quiz/report/overview/report.php b/mod/quiz/report/overview/report.php index aec78095ae2..58041fb2754 100644 --- a/mod/quiz/report/overview/report.php +++ b/mod/quiz/report/overview/report.php @@ -133,14 +133,14 @@ class quiz_overview_report extends quiz_attempt_report { // Process actions. if (empty($currentgroup) || $groupstudents) { if (optional_param('delete', 0, PARAM_BOOL) && confirm_sesskey()) { - if ($attemptids = optional_param('attemptid', array(), PARAM_INT)) { + if ($attemptids = optional_param_array('attemptid', array(), PARAM_INT)) { require_capability('mod/quiz:deleteattempts', $this->context); $this->delete_selected_attempts($quiz, $cm, $attemptids, $allowed); redirect($reporturl->out(false, $displayoptions)); } } else if (optional_param('regrade', 0, PARAM_BOOL) && confirm_sesskey()) { - if ($attemptids = optional_param('attemptid', array(), PARAM_INT)) { + if ($attemptids = optional_param_array('attemptid', array(), PARAM_INT)) { require_capability('mod/quiz:regrade', $this->context); $this->regrade_attempts($quiz, false, $groupstudents, $attemptids); redirect($reporturl->out(false, $displayoptions)); diff --git a/mod/quiz/report/responses/report.php b/mod/quiz/report/responses/report.php index 5abf1e2f058..904ff587650 100644 --- a/mod/quiz/report/responses/report.php +++ b/mod/quiz/report/responses/report.php @@ -124,7 +124,7 @@ class quiz_responses_report extends quiz_attempt_report { $allowed = array(); } - if ($attemptids = optional_param('attemptid', array(), PARAM_INT) && confirm_sesskey()) { + if ($attemptids = optional_param_array('attemptid', array(), PARAM_INT) && confirm_sesskey()) { require_capability('mod/quiz:deleteattempts', $this->context); $this->delete_selected_attempts($quiz, $cm, $attemptids, $allowed); redirect($reporturl->out(false, $displayoptions)); diff --git a/mod/scorm/report/basic/report.php b/mod/scorm/report/basic/report.php index fc849a7bffe..8f7c8becaf9 100644 --- a/mod/scorm/report/basic/report.php +++ b/mod/scorm/report/basic/report.php @@ -35,7 +35,7 @@ class scorm_basic_report extends scorm_default_report { global $CFG, $DB, $OUTPUT, $PAGE; $contextmodule= get_context_instance(CONTEXT_MODULE, $cm->id); $action = optional_param('action', '', PARAM_ALPHA); - $attemptids = optional_param('attemptid', array(), PARAM_RAW); + $attemptids = optional_param_array('attemptid', array(), PARAM_RAW); if ($action == 'delete' && has_capability('mod/scorm:deleteresponses', $contextmodule) && confirm_sesskey()) { if (scorm_delete_responses($attemptids, $scorm)) { //delete responses. diff --git a/repository/lib.php b/repository/lib.php index 5216e1fb9c6..bbed2bedc64 100644 --- a/repository/lib.php +++ b/repository/lib.php @@ -1501,7 +1501,7 @@ abstract class repository { public function filter(&$value) { $pass = false; - $accepted_types = optional_param('accepted_types', '', PARAM_RAW); + $accepted_types = optional_param_array('accepted_types', '', PARAM_RAW); $ft = new filetype_parser; //$ext = $ft->get_extensions($this->supported_filetypes()); if (isset($value['children'])) { diff --git a/repository/repository_ajax.php b/repository/repository_ajax.php index 7f0e1276539..463673160aa 100644 --- a/repository/repository_ajax.php +++ b/repository/repository_ajax.php @@ -46,7 +46,7 @@ $itemid = optional_param('itemid', 0, PARAM_INT); // Itemid $page = optional_param('page', '', PARAM_RAW); // Page $maxbytes = optional_param('maxbytes', 0, PARAM_INT); // Maxbytes $req_path = optional_param('p', '', PARAM_RAW); // Path -$accepted_types = optional_param('accepted_types', '*', PARAM_RAW); +$accepted_types = optional_param_array('accepted_types', '*', PARAM_RAW); $saveas_filename = optional_param('title', '', PARAM_FILE); // save as file name $saveas_path = optional_param('savepath', '/', PARAM_PATH); // save as file path $search_text = optional_param('s', '', PARAM_CLEANHTML); diff --git a/repository/upload/lib.php b/repository/upload/lib.php index 82850423a95..bf061d3817c 100644 --- a/repository/upload/lib.php +++ b/repository/upload/lib.php @@ -44,7 +44,7 @@ class repository_upload extends repository { public function upload($saveas_filename, $maxbytes) { global $USER, $CFG; - $types = optional_param('accepted_types', '*', PARAM_RAW); + $types = optional_param_array('accepted_types', '*', PARAM_RAW); if ((is_array($types) and in_array('*', $types)) or $types == '*') { $this->mimetypes = '*'; } else { diff --git a/tag/manage.php b/tag/manage.php index c4fae4c13e0..e0edeb94bf5 100644 --- a/tag/manage.php +++ b/tag/manage.php @@ -29,9 +29,9 @@ require_once('lib.php'); define('SHOW_ALL_PAGE_SIZE', 50000); define('DEFAULT_PAGE_SIZE', 30); -$tagschecked = optional_param('tagschecked', array(), PARAM_INT); -$newnames = optional_param('newname', array(), PARAM_TAG); -$tagtypes = optional_param('tagtypes', array(), PARAM_ALPHA); +$tagschecked = optional_param_array('tagschecked', array(), PARAM_INT); +$newnames = optional_param_array('newname', array(), PARAM_TAG); +$tagtypes = optional_param_array('tagtypes', array(), PARAM_ALPHA); $action = optional_param('action', '', PARAM_ALPHA); $perpage = optional_param('perpage', DEFAULT_PAGE_SIZE, PARAM_INT); diff --git a/user/addnote.php b/user/addnote.php index 06cae6b97f2..5a0d0a8d9bb 100644 --- a/user/addnote.php +++ b/user/addnote.php @@ -27,9 +27,9 @@ require_once("../config.php"); require_once($CFG->dirroot .'/notes/lib.php'); $id = required_param('id', PARAM_INT); // course id -$users = optional_param('userid', array(), PARAM_INT); // array of user id -$contents = optional_param('contents', array(), PARAM_RAW); // array of user notes -$states = optional_param('states', array(), PARAM_ALPHA); // array of notes states +$users = optional_param_array('userid', array(), PARAM_INT); // array of user id +$contents = optional_param_array('contents', array(), PARAM_RAW); // array of user notes +$states = optional_param_array('states', array(), PARAM_ALPHA); // array of notes states $PAGE->set_url('/user/addnote.php', array('id'=>$id)); diff --git a/user/groupaddnote.php b/user/groupaddnote.php index 6d53875e9f7..90f1b3d9d6c 100644 --- a/user/groupaddnote.php +++ b/user/groupaddnote.php @@ -27,7 +27,7 @@ require_once("../config.php"); require_once($CFG->dirroot .'/notes/lib.php'); $id = required_param('id', PARAM_INT); // course id -$users = optional_param('userid', array(), PARAM_INT); // array of user id +$users = optional_param_array('userid', array(), PARAM_INT); // array of user id $content = optional_param('content', '', PARAM_RAW); // note content $state = optional_param('state', '', PARAM_ALPHA); // note publish state diff --git a/user/selector/lib.php b/user/selector/lib.php index f45686bee03..4ac03b04400 100644 --- a/user/selector/lib.php +++ b/user/selector/lib.php @@ -353,7 +353,7 @@ abstract class user_selector_base { */ protected function load_selected_users() { // See if we got anything. - $userids = optional_param($this->name, array(), PARAM_INTEGER); + $userids = optional_param_array($this->name, array(), PARAM_INTEGER); if (empty($userids)) { return array(); } From 21aa9d48e392fd397b4836d7f4c187ed62fdb3b7 Mon Sep 17 00:00:00 2001 From: Petr Skoda Date: Fri, 19 Aug 2011 17:04:04 +0200 Subject: [PATCH 4/5] MDL-26796 document array parameter changes --- mod/upgrade.txt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/mod/upgrade.txt b/mod/upgrade.txt index c7ecc4f62cf..f948872c5a8 100644 --- a/mod/upgrade.txt +++ b/mod/upgrade.txt @@ -52,3 +52,9 @@ optional - no changes needed in older code: * new ratings API (http://docs.moodle.org/dev/Ratings_2.0) + +=== 2.2 === + +required changes in code: +* fix missing parameter types in optional_param() and required_param() +* use new optional_param_array(), required_param_array() or clean_param_array() when dealing with array parameters \ No newline at end of file From ebdeccca4e0a5c9c46014b420d7043a19ec0616c Mon Sep 17 00:00:00 2001 From: Petr Skoda Date: Tue, 23 Aug 2011 08:18:03 +0200 Subject: [PATCH 5/5] MDL-26796 remove unused param --- lib/moodlelib.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/moodlelib.php b/lib/moodlelib.php index c651300bb23..231d91721d7 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -473,7 +473,7 @@ function required_param($parname, $type) { return required_param_array($parname, $type); } - return clean_param($param, $type, false); + return clean_param($param, $type); } /** @@ -513,7 +513,7 @@ function required_param_array($parname, $type) { debugging('Invalid key name in required_param_array() detected: '.$key.', parameter: '.$parname); continue; } - $result[$key] = clean_param($value, $type, false); + $result[$key] = clean_param($value, $type); } return $result; @@ -558,7 +558,7 @@ function optional_param($parname, $default, $type) { return optional_param_array($parname, $default, $type); } - return clean_param($param, $type, false); + return clean_param($param, $type); } /** @@ -600,7 +600,7 @@ function optional_param_array($parname, $default, $type) { debugging('Invalid key name in optional_param_array() detected: '.$key.', parameter: '.$parname); continue; } - $result[$key] = clean_param($value, $type, false); + $result[$key] = clean_param($value, $type); } return $result; @@ -631,7 +631,7 @@ function validate_param($param, $type, $allownull=NULL_NOT_ALLOWED, $debuginfo=' throw new invalid_parameter_exception($debuginfo); } - $cleaned = clean_param($param, $type, false); + $cleaned = clean_param($param, $type); if ((string)$param !== (string)$cleaned) { // conversion to string is usually lossless throw new invalid_parameter_exception($debuginfo);