From 9c621d53e940c6fa524d983d64f217bfa39f5770 Mon Sep 17 00:00:00 2001 From: Sujith Haridasan Date: Mon, 10 May 2021 17:20:05 +0530 Subject: [PATCH] MDL-71241 course: Validate and sanitize sort arguments Signed-off-by: Sujith Haridasan --- course/tests/courselib_test.php | 18 +-- course/tests/externallib_test.php | 179 +++++++++++++++++++++++++++++- lib/enrollib.php | 64 +++++++++-- 3 files changed, 236 insertions(+), 25 deletions(-) diff --git a/course/tests/courselib_test.php b/course/tests/courselib_test.php index 8e2f048c083..c9975026a75 100644 --- a/course/tests/courselib_test.php +++ b/course/tests/courselib_test.php @@ -4561,7 +4561,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 0, 'limit' => 0, 'offset' => 0, - 'expecteddbqueries' => 1, + 'expecteddbqueries' => 4, 'expectedresult' => $buildexpectedresult(0, 0) ], 'less than query limit' => [ @@ -4569,7 +4569,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 2, 'limit' => 0, 'offset' => 0, - 'expecteddbqueries' => 1, + 'expecteddbqueries' => 2, 'expectedresult' => $buildexpectedresult(2, 0) ], 'more than query limit' => [ @@ -4577,7 +4577,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 7, 'limit' => 0, 'offset' => 0, - 'expecteddbqueries' => 3, + 'expecteddbqueries' => 4, 'expectedresult' => $buildexpectedresult(7, 0) ], 'limit less than query limit' => [ @@ -4585,7 +4585,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 7, 'limit' => 2, 'offset' => 0, - 'expecteddbqueries' => 1, + 'expecteddbqueries' => 2, 'expectedresult' => $buildexpectedresult(2, 0) ], 'limit less than query limit with offset' => [ @@ -4593,7 +4593,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 7, 'limit' => 2, 'offset' => 2, - 'expecteddbqueries' => 1, + 'expecteddbqueries' => 2, 'expectedresult' => $buildexpectedresult(2, 2) ], 'limit less than total' => [ @@ -4601,7 +4601,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 9, 'limit' => 6, 'offset' => 0, - 'expecteddbqueries' => 2, + 'expecteddbqueries' => 3, 'expectedresult' => $buildexpectedresult(6, 0) ], 'less results than limit' => [ @@ -4609,7 +4609,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 9, 'limit' => 20, 'offset' => 0, - 'expecteddbqueries' => 3, + 'expecteddbqueries' => 4, 'expectedresult' => $buildexpectedresult(9, 0) ], 'less results than limit exact divisible' => [ @@ -4617,7 +4617,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 9, 'limit' => 20, 'offset' => 0, - 'expecteddbqueries' => 4, + 'expecteddbqueries' => 5, 'expectedresult' => $buildexpectedresult(9, 0) ], 'less results than limit with offset' => [ @@ -4625,7 +4625,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 9, 'limit' => 10, 'offset' => 5, - 'expecteddbqueries' => 2, + 'expecteddbqueries' => 3, 'expectedresult' => $buildexpectedresult(4, 5) ], ]; diff --git a/course/tests/externallib_test.php b/course/tests/externallib_test.php index 766a0c1a6dc..c4c4d5494fd 100644 --- a/course/tests/externallib_test.php +++ b/course/tests/externallib_test.php @@ -2909,7 +2909,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { /** * Test cases for the get_enrolled_courses_by_timeline_classification test. */ - public function get_get_enrolled_courses_by_timeline_classification_test_cases() { + public function get_get_enrolled_courses_by_timeline_classification_test_cases():array { $now = time(); $day = 86400; @@ -3008,6 +3008,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 2, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => [], 'expectednextoffset' => 0 ], @@ -3017,6 +3018,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 0, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => ['afuture', 'bfuture', 'cfuture', 'dfuture', 'efuture'], 'expectednextoffset' => 15 ], @@ -3025,6 +3027,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 2, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => ['afuture', 'bfuture'], 'expectednextoffset' => 4 ], @@ -3033,6 +3036,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 2, 'offset' => 2, + 'sort' => 'shortname ASC', 'expectedcourses' => ['bfuture', 'cfuture'], 'expectednextoffset' => 7 ], @@ -3041,6 +3045,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 5, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => ['afuture', 'bfuture', 'cfuture', 'dfuture', 'efuture'], 'expectednextoffset' => 13 ], @@ -3049,6 +3054,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 10, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => ['afuture', 'bfuture', 'cfuture', 'dfuture', 'efuture'], 'expectednextoffset' => 15 ], @@ -3057,6 +3063,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 10, 'offset' => 5, + 'sort' => 'shortname ASC', 'expectedcourses' => ['cfuture', 'dfuture', 'efuture'], 'expectednextoffset' => 15 ], @@ -3065,6 +3072,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'all', 'limit' => 0, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => [ 'afuture', 'ainprogress', @@ -3089,6 +3097,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'all', 'limit' => 5, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => [ 'afuture', 'ainprogress', @@ -3103,6 +3112,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'all', 'limit' => 5, 'offset' => 5, + 'sort' => 'shortname ASC', 'expectedcourses' => [ 'bpast', 'cfuture', @@ -3117,9 +3127,161 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'all', 'limit' => 5, 'offset' => 50, + 'sort' => 'shortname ASC', 'expectedcourses' => [], 'expectednextoffset' => 50 ], + 'all limit and offset with sort ul.timeaccess desc' => [ + 'coursedata' => $coursedata, + 'classification' => 'inprogress', + 'limit' => 0, + 'offset' => 0, + 'sort' => 'ul.timeaccess desc', + 'expectedcourses' => [ + 'ainprogress', + 'binprogress', + 'cinprogress', + 'dinprogress', + 'einprogress' + ], + 'expectednextoffset' => 15 + ], + 'all limit and offset with sort sql injection for sort or 1==1' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => 'ul.timeaccess desc or 1==1', + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with sql injection of sort a custom one' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "ul.timeaccess LIMIT 1--", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong sort direction' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "ul.timeaccess abcdasc", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid sort direction in $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong sort direction' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "ul.timeaccess.foo ascd", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid sort direction in $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong sort param' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "foobar", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong field name' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "ul.foobar", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong field separator' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "ul.timeaccess.foo", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong field separator #' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "ul#timeaccess", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong field separator $' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => 'ul$timeaccess', + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong field name' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => 'timeaccess123', + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with no sort direction for ul' => [ + 'coursedata' => $coursedata, + 'classification' => 'inprogress', + 'limit' => 0, + 'offset' => 0, + 'sort' => "ul.timeaccess", + 'expectedcourses' => ['ainprogress', 'binprogress', 'cinprogress', 'dinprogress', 'einprogress'], + 'expectednextoffset' => 15, + ], + 'all limit and offset with valid field name and no prefix, test for ul' => [ + 'coursedata' => $coursedata, + 'classification' => 'inprogress', + 'limit' => 0, + 'offset' => 0, + 'sort' => "timeaccess", + 'expectedcourses' => ['ainprogress', 'binprogress', 'cinprogress', 'dinprogress', 'einprogress'], + 'expectednextoffset' => 15, + ], + 'all limit and offset with valid field name and no prefix' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "fullname", + 'expectedcourses' => ['bpast', 'cpast', 'dfuture', 'dpast', 'efuture'], + 'expectednextoffset' => 10, + ], + 'all limit and offset with valid field name and no prefix and with sort direction' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "fullname desc", + 'expectedcourses' => ['bpast', 'cpast', 'dfuture', 'dpast', 'efuture'], + 'expectednextoffset' => 10, + ], ]; } @@ -3131,16 +3293,20 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { * @param string $classification Timeline classification * @param int $limit Maximum number of results * @param int $offset Offset the unfiltered courses result set by this amount + * @param string $sort sort the courses * @param array $expectedcourses Expected courses in result * @param int $expectednextoffset Expected next offset value in result + * @param string|null $expectedexception Expected exception string */ public function test_get_enrolled_courses_by_timeline_classification( $coursedata, $classification, $limit, $offset, + $sort, $expectedcourses, - $expectednextoffset + $expectednextoffset, + $expectedexception = null ) { $this->resetAfterTest(); $generator = $this->getDataGenerator(); @@ -3157,6 +3323,11 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { $this->setUser($student); + if (isset($expectedexception)) { + $this->expectException('coding_exception'); + $this->expectExceptionMessage($expectedexception); + } + // NOTE: The offset applies to the unfiltered full set of courses before the classification // filtering is done. // E.g. In our example if an offset of 2 is given then it would mean the first @@ -3165,7 +3336,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { $classification, $limit, $offset, - 'shortname ASC' + $sort ); $result = external_api::clean_returnvalue( core_course_external::get_enrolled_courses_by_timeline_classification_returns(), @@ -3176,7 +3347,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { return $course['shortname']; }, $result['courses']); - $this->assertEquals($expectedcourses, $actual); + $this->assertEqualsCanonicalizing($expectedcourses, $actual); $this->assertEquals($expectednextoffset, $result['nextoffset']); } diff --git a/lib/enrollib.php b/lib/enrollib.php index 1873d64af26..d1c46c59fe0 100644 --- a/lib/enrollib.php +++ b/lib/enrollib.php @@ -567,6 +567,11 @@ function enrol_get_my_courses($fields = null, $sort = null, $limit = 0, $coursei $offset = 0, $excludecourses = []) { global $DB, $USER, $CFG; + // Allowed prefixes and field names. + $allowedprefixesandfields = ['c' => array_keys($DB->get_columns('course')), + 'ul' => array_keys($DB->get_columns('user_lastaccess')), + 'ue' => array_keys($DB->get_columns('user_enrolments'))]; + // Re-Arrange the course sorting according to the admin settings. $sort = enrol_get_courses_sortingsql($sort); @@ -599,28 +604,63 @@ function enrol_get_my_courses($fields = null, $sort = null, $limit = 0, $coursei $orderby = ""; $sort = trim($sort); $sorttimeaccess = false; - $allowedsortprefixes = array('c', 'ul', 'ue'); if (!empty($sort)) { $rawsorts = explode(',', $sort); $sorts = array(); foreach ($rawsorts as $rawsort) { $rawsort = trim($rawsort); - if (preg_match('/^ul\.(\S*)\s(asc|desc)/i', $rawsort, $matches)) { - if (strcasecmp($matches[2], 'asc') == 0) { - $sorts[] = 'COALESCE(ul.' . $matches[1] . ', 0) ASC'; - } else { - $sorts[] = 'COALESCE(ul.' . $matches[1] . ', 0) DESC'; + // Make sure that there are no more white spaces in sortparams after explode. + $sortparams = array_values(array_filter(explode(' ', $rawsort))); + // If more than 2 values present then throw coding_exception. + if (isset($sortparams[2])) { + throw new coding_exception('Invalid $sort parameter in enrol_get_my_courses()'); + } + // Check the sort ordering if present, at the beginning. + if (isset($sortparams[1]) && (preg_match("/^(asc|desc)$/i", $sortparams[1]) === 0)) { + throw new coding_exception('Invalid sort direction in $sort parameter in enrol_get_my_courses()'); + } + + $sortfield = $sortparams[0]; + $sortdirection = $sortparams[1] ?? 'asc'; + if (strpos($sortfield, '.') !== false) { + $sortfieldparams = explode('.', $sortfield); + // Check if more than one dots present in the prefix field. + if (isset($sortfieldparams[2])) { + throw new coding_exception('Invalid $sort parameter in enrol_get_my_courses()'); } - $sorttimeaccess = true; - } else if (strpos($rawsort, '.') !== false) { - $prefix = explode('.', $rawsort); - if (in_array($prefix[0], $allowedsortprefixes)) { - $sorts[] = trim($rawsort); + list($prefix, $fieldname) = [$sortfieldparams[0], $sortfieldparams[1]]; + // Check if the field name matches with the allowed prefix. + if (array_key_exists($prefix, $allowedprefixesandfields) && + (in_array($fieldname, $allowedprefixesandfields[$prefix]))) { + if ($prefix === 'ul') { + $sorts[] = "COALESCE({$prefix}.{$fieldname}, 0) {$sortdirection}"; + $sorttimeaccess = true; + } else { + // Check if the field name that matches with the prefix and just append to sorts. + $sorts[] = $rawsort; + } } else { throw new coding_exception('Invalid $sort parameter in enrol_get_my_courses()'); } } else { - $sorts[] = 'c.'.trim($rawsort); + // Check if the field name matches with $allowedprefixesandfields. + $found = false; + foreach (array_keys($allowedprefixesandfields) as $prefix) { + if (in_array($sortfield, $allowedprefixesandfields[$prefix])) { + if ($prefix === 'ul') { + $sorts[] = "COALESCE({$prefix}.{$sortfield}, 0) {$sortdirection}"; + $sorttimeaccess = true; + } else { + $sorts[] = "{$prefix}.{$sortfield} {$sortdirection}"; + } + $found = true; + break; + } + } + if (!$found) { + // The param is not found in $allowedprefixesandfields. + throw new coding_exception('Invalid $sort parameter in enrol_get_my_courses()'); + } } } $sort = implode(',', $sorts);