Course ratings
tool_courserating
The tool_courserating plugin (release 4.5.1, version 2026041700) lets students rate courses with 1–5 stars and leave optional text reviews. Ratings are rendered on course pages, course listings (via an auto-created custom course field), and in a modal popup that shows per-course summaries and individual reviews. Managers can permanently delete flagged reviews; teachers/managers view a per-course report built on the Moodle 4.x Report Builder. The plugin supports Moodle 4.5 and 5.1/5.2, implements the modern hook API (before_http_headers, before_footer_html_generation, before_standard_head_html_generation) alongside legacy callbacks, ships Behat and PHPUnit test coverage, and exposes one web service (tool_courserating_course_rating_popup) that is explicitly available to unauthenticated users so public course ratings can be viewed on the front page.
The plugin demonstrates careful security practice overall: every fragment callback, web service path, form submission and file-serving entry point funnels through a central permission class that validates both capability and course visibility; all DB access uses $DB with parameter placeholders; output uses Moodle's persistent/exporter pattern with format_text / format_string; the one unauthenticated WS is explicitly designed with manual permission checks and documented intent; and dynamic forms are used for state-changing actions which provides automatic sesskey validation. Only a small number of low-severity issues remain: the Privacy API exports ratings but not the flag table (an incomplete GDPR implementation the authors flagged with a TODO), api::set_rating() accepts a third userid parameter gated only by a code comment rather than a PHPUNIT_TEST/BEHAT_SITE_RUNNING guard, the admin-controlled parentcss setting is interpolated into inline JavaScript with only HTML escaping (admin-only, not a real attack surface), and api::reindex() loads every course row into memory with get_records_sql instead of using a recordset. None of these are exploitable security vulnerabilities; they are code-quality and compliance gaps on an otherwise well-written codebase.
tool_courserating is a well-structured course-rating plugin from an experienced Moodle developer. The security model is explicit and centralised in classes/permission.php, covering view, add, delete, flag and report access. Fragment callbacks, the unauthenticated course_rating_popup WS, the pluginfile handler, the inplace-editable callback, and both dynamic forms all call into this permission class before any state change or data exposure. SQL is parameterised everywhere; there are no raw mysqli_*, exec, eval, file_get_contents on URLs, or filesystem bypasses. Templates use format_text / format_string and the stored_file API for review attachments, with a Content-Security-Policy header on in-browser image display.
The findings are entirely low-severity and non-exploitable:
- Privacy API declares
tool_courserating_flagmetadata butexport_user_data()only exports rating records — a// TODO export flagscomment acknowledges this. GDPR subject-access requests would return incomplete data for users who flagged reviews. api::set_rating($courseid, $data, $userid = 0)allows a caller to attribute a rating to any user. A code comment says "only for phpunit and behat" but there is no runtime guard. Currently only test generators pass this argument; the addrating form never does. The risk is a future caller accidentally passing untrusted input.parentcssadmin setting is interpolated into a Mustache{{#js}}block as"{{parentelement}}".split(','). Mustache's default escape iss()(HTML), which is not the correct escape for a JavaScript string context. Because only site admins can change this setting (viaadmin_setting_configtext), there is no privilege escalation — the concern is JS-syntax breakage rather than XSS.api::reindex()calls$DB->get_records_sql($sql, $params)without a limit, pulling every course on the site into a PHP array. A recordset would be more memory-safe for large installations.
The unauthenticated web service (tool_courserating_course_rating_popup, declared with 'loginrequired' => false) intentionally skips validate_context() because validate_context calls require_login and would reject guests. The plugin compensates by calling permission::require_can_view_ratings($courseid) inside the fragment callback, which in turn checks core_course_category::can_view_course_info() — the standard Moodle gate for public course listings. The design is sound and explicitly documented in comments. The tool_courserating_inplace_editable callback had a missing validate_context flagged by mdlshield in a previous release; that has been fixed in 4.5.1 (see lib.php:264).
No third-party libraries are bundled. Test coverage (PHPUnit + Behat) is good. Capability definitions correctly set RISK_DATALOSS on delete and RISK_PERSONAL on reports.
Findings
The plugin's Privacy API implementation declares the tool_courserating_flag table in get_metadata() (listing ratingid, userid, reasoncode, reason, timecreated, timemodified as user-linked fields) but the export_user_data() implementation only builds a SELECT against tool_courserating_rating. A // TODO export flags. comment acknowledges the gap.
When a user submits a data-export request via Site administration → Users → Privacy and policies, the exported archive will contain the user's ratings and reviews but will omit any reviews they have flagged as inappropriate, even though those flags are personal data recorded against their user id. This is a GDPR compliance gap, not a security vulnerability — deletion (delete_data_for_user, delete_data_for_users, delete_data_for_all_users_in_context) does correctly remove flags.
Fix: extend export_user_data() with a second query joining tool_courserating_flag and tool_courserating_rating, and call writer::with_context($context)->export_data(...) for the flag records with an appropriate subcontext.
Low risk. This is a compliance/GDPR gap rather than a security vulnerability. No unauthorized disclosure or modification is possible — the issue is only visible when a legitimate admin approves a privacy export request and the archive is missing flag records. Blast radius is limited to the specific user making the request. The authors' own TODO comment indicates awareness.
Moodle's Privacy API contract requires that any user-linked data declared in get_metadata() be exported when a data-subject request is approved. The plugin correctly declares the tool_courserating_flag table in classes/privacy/provider.php:63-75 but export_user_data() only iterates rating records. The get_contexts_for_userid() method already joins the flag table correctly to find contexts, and the delete paths (delete_data_for_user_in_courses) delete flags properly. Only the export path is incomplete.
foreach ($ratings as $rating) {
$subcontext = [
get_string('pluginname', 'tool_courserating'),
$rating->shortname,
];
$data = (object) [
'shortname' => $rating->shortname,
'fullname' => $rating->fullname,
'rating' => $rating->rating,
'review' => $rating->review,
'hasreview' => $rating->hasreview,
'userid' => transform::user($rating->userid),
'timecreated' => transform::datetime($rating->timecreated),
'timemodified' => transform::datetime($rating->timemodified),
];
$context = \context_course::instance($rating->courseid);
writer::with_context($context)->export_data($subcontext, $data);
}
// TODO export flags.
}
Add a second pass that queries tool_courserating_flag joined with tool_courserating_rating for the target user and exports each flag (with its reasoncode, reason, and the id/course of the rating it relates to) under an appropriate subcontext, mirroring the structure used for ratings.
$sql = "SELECT f.id, f.ratingid, f.reasoncode, f.reason, f.timecreated, f.timemodified,
r.courseid, c.shortname
FROM {tool_courserating_flag} f
JOIN {tool_courserating_rating} r ON r.id = f.ratingid
JOIN {course} c ON c.id = r.courseid
WHERE f.userid = :userid
AND c.id {$coursesql}";
$flags = $DB->get_records_sql($sql, $params);
foreach ($flags as $flag) {
$context = \context_course::instance($flag->courseid);
writer::with_context($context)->export_related_data(
[get_string('pluginname', 'tool_courserating'), $flag->shortname, 'flags'],
'flag_' . $flag->id,
(object)[
'ratingid' => $flag->ratingid,
'reasoncode' => $flag->reasoncode,
'reason' => $flag->reason,
'timecreated' => transform::datetime($flag->timecreated),
'timemodified' => transform::datetime($flag->timemodified),
]
);
}
api::set_rating($courseid, $data, $userid = 0) uses the third argument if non-zero, otherwise falls back to $USER->id. The docblock says the parameter is "only for phpunit and behat tests" and an inline comment repeats the same intent, but there is no runtime check (defined('PHPUNIT_TEST'), defined('BEHAT_SITE_RUNNING'), etc.) that enforces this.
Today the only callers that pass the third argument are tests/generator/lib.php::create_rating() and the Behat generator adapter. Neither is reachable from a normal HTTP request. The risk is purely future-proofing: if a future developer calls api::set_rating($courseid, $data, $someid) from a reachable code path (a new WS, a new form, an admin tool) the function will happily attribute the rating to any user id.
Also note that api::set_rating() itself performs no capability check — it assumes the caller has already invoked permission::require_can_add_rating(). The addrating dynamic form does this via check_access_for_dynamic_submission(). This is a correct pattern but combined with the unguarded $userid override it amplifies the downside of any future misuse.
Low risk. Not currently exploitable because every production call site omits $userid. Treat this as defensive hardening to prevent a future footgun. If the guard were added now, the test generators would need the same guard-check, which is trivially satisfied inside PHPUnit/Behat.
api::set_rating() is the single public entry point for writing ratings. It's called from classes/form/addrating.php::process_dynamic_submission() (without the $userid argument) and from the two test generators (tests/generator/lib.php, tests/generator/behat_tool_courserating_generator.php) which do pass it. The permission check lives in addrating::check_access_for_dynamic_submission() → permission::require_can_add_rating($courseid) which calls has_capability('tool/courserating:rate', ...).
public static function set_rating(int $courseid, \stdClass $data, int $userid = 0): rating {
global $USER;
// TODO $userid can only be used in phpunit and behat.
$userid = $userid ?: $USER->id;
Enforce the test-only contract in code rather than in a comment:
public static function set_rating(int $courseid, \stdClass $data, int $userid = 0): rating {
global $USER;
if ($userid && !(defined('PHPUNIT_TEST') && PHPUNIT_TEST)
&& !(defined('BEHAT_SITE_RUNNING') && BEHAT_SITE_RUNNING)) {
throw new \coding_exception('api::set_rating $userid override is only allowed in tests');
}
$userid = $userid ?: $USER->id;
...
}
Alternatively add a capability check (e.g. moodle/site:config) so non-test callers can only pass $userid when they really are acting as admin on behalf of another user.
templates/course_rating_block.mustache interpolates the admin-controlled parentelement value directly into a JavaScript string literal inside a {{#js}} lambda block. Moodle Mustache's default escape function is s() (HTML htmlspecialchars), which is not the correct escape for a JavaScript string context — it escapes ", ', <, >, & to HTML entities but does not escape backslashes, Unicode line separators, or closing </script> sequences.
The setting is defined in settings.php as an admin_setting_configtext with the default PARAM_RAW param type, so any text is accepted. Only users with site configuration access can change it (usually only the site admin role).
In practice, the HTML-escaping of " to " is sufficient to prevent string-break-out in a JS-double-quoted literal — the six literal characters " are not a quote in JavaScript — so this is not a real XSS vector. The concern is:
- Incorrect defensive pattern — future changes to Mustache escaping, or a switch to single quotes in the template, could turn this into an XSS vector.
- JS syntax breakage — a backslash at the end of the value (
\) would make the string literal unterminated, causing a hard JS error that breaks the page script for everyone. - Admin is trusted anyway — site admins can already run arbitrary JS via themes, language packs, and
additionalhtml*settings, so there is no privilege-escalation angle.
Fix: use the built-in quote Mustache helper (mustache_quote_helper) which emits a correctly quoted JS string, or pass parentelement as data-* HTML attribute (which is HTML-escaped correctly) and read it from JS.
Low risk. Only site admins can change this value, so privilege escalation is not possible. HTML escaping does prevent string break-out in the current template. The finding is primarily about correctness — using HTML escape in a JS context is the wrong pattern and relies on the specific surrounding JS syntax to stay safe.
The plugin supports custom themes that need the rating widget placed somewhere other than #page-header. The admin-configured CSS selector is stored in tool_courserating/parentcss and read via helper::get_setting(constants::SETTING_PARENTCSS) in classes/output/renderer.php::course_rating_block(), then pushed into Mustache as $data->parentelement.
{{#parentelement}}
{{#js}}
/* Move the widget to the header area */
const widget = document.querySelector(".tool_courserating-widget");
const parents = "{{parentelement}}".split(',');
for (let i in parents) {
const header = document.querySelector(parents[i]);
if (header) {
header.appendChild(widget);
break;
}
}
{{/js}}
{{/parentelement}}
Expose parentelement through an HTML data attribute and read it with JavaScript (HTML escaping via Mustache default {{...}} is correct for HTML attributes):
<div class="tool_courserating-widget pt-2 pt-0 {{extraclasses}}"
data-parent-selectors="{{parentelement}}">
...
</div>
{{#parentelement}}
{{#js}}
const widget = document.querySelector(".tool_courserating-widget");
const raw = widget.dataset.parentSelectors || '';
const parents = raw.split(',');
for (let i in parents) {
const header = document.querySelector(parents[i]);
if (header) { header.appendChild(widget); break; }
}
{{/js}}
{{/parentelement}}
Or use the core {{#quote}}...{{/quote}} helper to emit a properly JS-quoted literal.
api::reindex() executes a query that joins course, tool_courserating_summary, and up to two customfield_* tables across all courses on the site and retrieves the results with $DB->get_records_sql($sql, $params). Each result row is small, but the number of rows is unbounded and proportional to the course count. When api::reindex() is called without a specific $courseid (i.e. during an admin settings change via the tool_courserating\task\reindex adhoc task), every course is loaded into a PHP array in a single allocation.
For a site with tens of thousands of courses this can use hundreds of megabytes of memory and risks exceeding PHP's memory_limit. The iteration that follows only needs one record at a time (foreach ($records as $record) { self::reindex_course($record); }), so a recordset is a strictly better fit.
Low risk. This is a performance/memory-safety concern, not a security issue. It only manifests on sites with a very large number of courses; smaller sites will never notice. A memory-exhausted cron run will fail safely and can be retried.
api::reindex() runs from an adhoc task scheduled whenever a relevant plugin setting changes (setting_updatedcallback wired in settings.php) or when a course is created. It's also called directly from observer::course_created() (with a single courseid, which is fine) and from observer::course_updated() (also single courseid). The unbounded case is when site-wide reindex is triggered.
$records = $DB->get_records_sql($sql, $params);
foreach ($records as $record) {
$record->actualratingmode = helper::get_setting(constants::SETTING_RATINGMODE);
if ($percourse && $record->rateby && array_key_exists($record->rateby, constants::rated_courses_options())) {
$record->actualratingmode = $record->rateby;
}
self::reindex_course($record);
}
}
Switch to a recordset so only one row is held in memory at a time:
$rs = $DB->get_recordset_sql($sql, $params);
foreach ($rs as $record) {
$record->actualratingmode = helper::get_setting(constants::SETTING_RATINGMODE);
if ($percourse && $record->rateby
&& array_key_exists($record->rateby, constants::rated_courses_options())) {
$record->actualratingmode = $record->rateby;
}
self::reindex_course($record);
}
$rs->close();
tool_courserating_course_rating_popup is declared in db/services.php with 'loginrequired' => false so guests and unauthenticated front-page visitors can open the rating summary popup. The WS implementation in classes/external/course_rating_popup.php explicitly skips \core_external\external_api::validate_context() and manually sets $PAGE->set_context(\context_system::instance()), with an inline comment explaining the design decision. Access control is moved into tool_courserating_output_fragment_course_ratings_popup() in lib.php, which calls permission::require_can_view_ratings($courseid) → core_course_category::can_view_course_info().
This is a deliberate and documented pattern, not a bug. Moodle's validate_context() calls require_login() internally, which would reject guest sessions. Skipping it is the standard way to build an unauthenticated WS in Moodle as long as you replace the removed authorisation with an explicit capability/visibility check — which this plugin does correctly.
This finding is included as info-level because a reviewer who only grepped for missing validate_context calls might flag it as a vulnerability. In fact, the changelog for 4.5.1 notes that a different WS callback (tool_courserating_inplace_editable in lib.php) was missing validate_context and that was correctly fixed (see lib.php:264). The course_rating_popup omission is intentional and correct.
Informational. This is an intentional, documented design choice and the permission check that replaces validate_context is appropriate. Listed here only to forestall false-positive flags from automated scanners. An attacker who can call this WS cannot see anything they couldn't already see by browsing to the public course listing on the same site.
The WS is called from the AMD module amd/src/rating.js::loadCourseRatingPopupContents() when document.body.classList.contains('notloggedin') is true — i.e. the client-side code branches between core/fragment.loadFragment (logged-in) and ajax.call({methodname: 'tool_courserating_course_rating_popup'}) (unauthenticated). The authorization gate is core_course_category::can_view_course_info(), which returns true only if the course is visible and the calling user has moodle/category:viewcourselist on the course's category — the same gate Moodle uses for its public course-listing pages.
public static function execute($courseid) {
global $PAGE, $OUTPUT, $CFG;
require_once($CFG->dirroot . '/' . $CFG->admin . '/tool/courserating/lib.php');
// Basically copied from the core_get_fragment WS except for login check.
// No validate_context call here because this WS is available to non-authenticated users.
// Instead we just set the page context to system manually.
$PAGE->set_context(\context_system::instance());
No fix required — the pattern is correct. If you want extra defence-in-depth, consider calling validate_parameters() before touching $PAGE (Moodle's WS dispatcher already does this), and narrowing the skipped work: the only thing you actually need to avoid is the require_login() inside validate_context, not the parameter validation. You could also log unauthenticated invocations with a debugging() call to make monitoring easier.
Tooling maturity is very good. The plugin ships GitHub Actions running moodle-plugin-ci (phplint, phpmd, phpcs, phpdoc, validate, savepoints, mustache, grunt, phpunit, behat) across Moodle 4.5 through 5.2 and both pgsql and mariadb. PHPUnit covers api, helper, observer, permission, privacy\provider, and the Report Builder datasource. Behat covers student-rating, teacher-delete, manager-delete-via-report, unauthenticated viewing, course overrides, and Report Builder integration.
Capability risk flags are correct. tool/courserating:delete is flagged RISK_DATALOSS and tool/courserating:reports is flagged RISK_PERSONAL. tool/courserating:rate has no risk bitmap, which is appropriate for a self-only write action.
Filesystem access is correct. Review attachments go through get_file_storage(), file_postupdate_standard_editor, send_stored_file, and the tool_courserating_pluginfile callback sets a Content-Security-Policy: default-src 'none'; img-src 'self' header on in-browser display — a nice defence against HTML review attachments being used as an XSS vector.
Custom course field management is thoughtful. The plugin creates/deletes the tool_courserating, tool_courserating_mode, and tool_courserating_reviews custom course fields on demand based on admin settings, uses locked => 1 to prevent teachers editing the rating field, and hides the field from the course edit form via the hideEditField AMD entry point. db/uninstall.php correctly cleans up via helper::delete_all_custom_fields().
Event classes correctly snapshot records and use IGNORE_MISSING for course contexts in rating_deleted::create_from_rating(), which protects against races where a course is deleted concurrently with a rating deletion.
Consider deleting the dead helper::wordings() private method in classes/helper.php:36-65. It contains copyrighted-looking reference strings from Udemy wrapped in @codingStandardsIgnoreStart and has no runtime effect. Dead code of this shape in a public-facing repo is a minor housekeeping concern.