L-electron-Rare/moodle
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

MDL-71241 course: Validate and sanitize sort arguments

Browse Source 
Signed-off-by: Sujith Haridasan <[email protected]>
This commit is contained in:
Sujith Haridasan
2021-07-08 23:34:39 +02:00
committed by Eloy Lafuente (stronk7)
parent b41a748a98
commit a19092e71c
3 changed files with 236 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files
course/tests/courselib_test.php
+9 -9
View File
@@ -4550,7 +4550,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' => [
@@ -4558,7 +4558,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' => [
@@ -4566,7 +4566,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' => [
@@ -4574,7 +4574,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' => [
@@ -4582,7 +4582,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' => [
@@ -4590,7 +4590,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' => [
@@ -4598,7 +4598,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' => [
@@ -4606,7 +4606,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' => [
@@ -4614,7 +4614,7 @@ class core_course_courselib_testcase extends advanced_testcase {
'totalcourses' => 9,
'limit' => 10,
'offset' => 5,
'expecteddbqueries' => 2,
'expecteddbqueries' => 3,
'expectedresult' => $buildexpectedresult(4, 5)
],
];
course/tests/externallib_test.php
+175 -4
View File
@@ -2765,7 +2765,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;
@@ -2864,6 +2864,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase {
'classification' => 'future',
'limit' => 2,
'offset' => 0,
'sort' => 'shortname ASC',
'expectedcourses' => [],
'expectednextoffset' => 0
],
@@ -2873,6 +2874,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
],
@@ -2881,6 +2883,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase {
'classification' => 'future',
'limit' => 2,
'offset' => 0,
'sort' => 'shortname ASC',
'expectedcourses' => ['afuture', 'bfuture'],
'expectednextoffset' => 4
],
@@ -2889,6 +2892,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase {
'classification' => 'future',
'limit' => 2,
'offset' => 2,
'sort' => 'shortname ASC',
'expectedcourses' => ['bfuture', 'cfuture'],
'expectednextoffset' => 7
],
@@ -2897,6 +2901,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
],
@@ -2905,6 +2910,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
],
@@ -2913,6 +2919,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
],
@@ -2921,6 +2928,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase {
'classification' => 'all',
'limit' => 0,
'offset' => 0,
'sort' => 'shortname ASC',
'expectedcourses' => [
'afuture',
'ainprogress',
@@ -2945,6 +2953,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase {
'classification' => 'all',
'limit' => 5,
'offset' => 0,
'sort' => 'shortname ASC',
'expectedcourses' => [
'afuture',
'ainprogress',
@@ -2959,6 +2968,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase {
'classification' => 'all',
'limit' => 5,
'offset' => 5,
'sort' => 'shortname ASC',
'expectedcourses' => [
'bpast',
'cfuture',
@@ -2973,9 +2983,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,
],
];
}
@@ -2987,16 +3149,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();
@@ -3013,6 +3179,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
@@ -3021,7 +3192,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(),
@@ -3032,7 +3203,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']);
}
lib/enrollib.php
+52 -12
View File
@@ -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);