Course ratings
tool_courserating
Admin tool that lets enrolled students leave a 1-5 star rating and an optional text review for a course. The average rating is written to an automatically created custom course field so it appears in any course listing that shows custom fields, and a rating widget is injected into each course home page. Users can flag reviews, managers can permanently delete flagged reviews, and teachers/managers get a per-course ratings report (built on the report builder). Ratings and review visibility can be configured globally or per course (disabled, teacher-only, or public), and reviews can optionally use the rich text editor with embedded files.
This is a mature, carefully written plugin (authored by a Moodle core developer) with no security vulnerabilities identified after reading every PHP, JavaScript and Mustache file.
Access control is consistent and correct. Every entry point enforces authorization: index.php calls require_course_login() plus a capability-backed require_can_view_reports(); all Fragment API callbacks in lib.php call the matching permission::require_can_view_*() helper; the dynamic_form classes implement check_access_for_dynamic_submission() (and inherit automatic sesskey handling); the pluginfile callback re-checks review visibility and confirms the file's rating belongs to the requesting course context; and the inplace_editable callback validates context and the flag capability.
The one public web service (tool_courserating_course_rating_popup, loginrequired=false) deliberately omits validate_context() and instead enforces permission::require_can_view_ratings() inside the fragment it renders — a correct, test-covered pattern for a feature intended to show public course ratings to anonymous visitors, and gated by course visibility and the forcelogin setting.
Data handling is safe. All database access uses parameterized $DB methods and core\persistent models; review HTML is sanitized on output through format_text()/core_external\util::format_text() (which clean by default, confirmed in core); colour settings injected into an inline <style> block are constrained to a #rrggbb regex; and templates escape output, with triple-brace fields limited to server-generated content. The Privacy API is fully implemented (export and deletion of ratings, flags and embedded files) with thorough unit tests, and db/uninstall.php cleans up the custom fields the plugin creates.
Only two minor, non-security issues were found: a Fragment callback discards a clean_param() result and reuses the raw argument, and a public API method accepts a caller-supplied user id that is currently used only by the test generator. Neither is exploitable.
tool_courserating 4.5.2 is a well-engineered plugin that adds student course ratings and reviews. The review surfaced no security vulnerabilities.
Strengths observed:
- Centralized, consistently applied permission layer (
classes/permission.php) used by pages, forms, fragments, the web service, file serving and the report. - Parameterized SQL everywhere — no string concatenation of user input into queries; schema changes confined to
db/upgrade.php. - Correct output encoding — reviews pass through
format_text()(cleans HTML by default), colour settings are regex-validated before entering an inline stylesheet, and Mustache escaping is respected. - Proper File API usage — embedded review files are served via
get_file_storage()+send_stored_file()with an access check and a restrictiveContent-Security-Policyheader. - Complete Privacy (GDPR) provider with export/delete of ratings, flags and files, plus
db/uninstall.phpcleanup, all backed by unit and Behat tests. - A public, unauthenticated web service that still enforces per-course authorization internally.
Findings: two low/informational code-quality items only — a Fragment callback that reuses an unsanitized argument after cleaning it, and a public api::set_rating() parameter that trusts a caller-supplied user id (used only by the test generator in practice). Neither affects security.
Findings
In the tool_courserating_output_fragment_rating_flag() callback, the rating id is sanitized with clean_param(..., PARAM_INT) into $ratingid and validated, but the subsequent rating object is constructed from the raw $args['ratingid'] value instead of the sanitized $ratingid.
The computed, sanitized value is effectively discarded. This is inconsistent with the other Fragment callbacks in the same file (for example tool_courserating_output_fragment_course_reviews() builds a fully cleaned $args array before use).
There is no security impact: the value is only ever used as a bound database parameter inside the core\persistent constructor (so SQL injection is not possible), and authorization is re-checked immediately afterwards via permission::require_can_view_ratings($rating->get('courseid')). The worst realistic outcome is a database type error if a malformed value such as 5abc is supplied (it clears the PARAM_INT truthiness guard yet reaches the query unsanitized).
Low risk. This is a code-quality/robustness defect, not a vulnerability. The sanitized variable is computed and then ignored, which could confuse future maintenance and, with a malformed input, could raise a database error on strict engines. There is no data exposure or injection vector because the value is used only as a bound parameter and the subsequent require_can_view_ratings() check gates access.
The callback is reached through core's core_get_fragment web service, which requires login, and renders the flag toggle for a single rating. Both the cleaned $ratingid and the raw $args['ratingid'] originate from the same request parameter; the only difference is that the raw value skips the PARAM_INT normalization. Because the value flows into a parameterized get_record() lookup and the course-level permission check runs on the loaded record, the control flow is safe regardless of which variable is used.
if (!$ratingid = clean_param($args['ratingid'] ?? 0, PARAM_INT)) {
throw new moodle_exception('missingparam', '', '', 'ratingid');
}
$rating = new \tool_courserating\local\models\rating($args['ratingid']);
Use the already-sanitized $ratingid when constructing the model:
if (!$ratingid = clean_param($args['ratingid'] ?? 0, PARAM_INT)) {
throw new moodle_exception('missingparam', '', '', 'ratingid');
}
$rating = new \tool_courserating\local\models\rating($ratingid);
api::set_rating() accepts an optional $userid argument that overrides $USER->id, allowing a rating to be created or updated on behalf of an arbitrary user. The author documents the intent with a // TODO $userid can only be used in phpunit and behat. comment.
In the current codebase this override is only used by the test data generator (tests/generator/lib.php). The sole production caller — the addrating form's process_dynamic_submission() — invokes the two-argument form, so the rating is always attributed to the authenticated user, and that path is protected by permission::require_can_add_rating().
This is therefore not currently exploitable. It is raised only as a hardening observation: a public static method that writes data for a caller-named user, without an internal capability check, is the kind of helper that can be misused if a future web-facing caller forwards a request parameter into the $userid argument.
Informational. No reachable code path lets an attacker influence the $userid argument, so there is no privilege or impersonation risk today. The suggestion is purely defense-in-depth to prevent a future regression from turning a test convenience into an impersonation primitive.
set_rating() is the core write path for ratings. The production entry point is the core_form\dynamic_form subclass tool_courserating\form\addrating, whose check_access_for_dynamic_submission() enforces require_can_add_rating() and which submits without a $userid. All callers that pass $userid are PHPUnit tests and the Behat/PHPUnit data generator.
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;
Consider guarding the override so it can only take effect in test runs, making misuse impossible from production code paths:
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')) {
throw new \coding_exception('Passing $userid to set_rating() is only allowed in tests');
}
$userid = $userid ?: $USER->id;
Alternatively, move the test-only behaviour into the generator and keep the public API bound to $USER.
Strong, security-relevant test coverage. tests/lib_test.php::test_pluginfile_access explicitly verifies that embedded review files are denied to students when reviews are hidden/disabled, allowed to teachers, and blocked for anonymous visitors under forcelogin; tests/permission_test.php covers forcelogin, per-course overrides, completion-gated rating, and the flag/delete capability boundaries. This materially increases confidence in the access-control model.
Public web service is correctly constrained. tool_courserating_course_rating_popup is declared with loginrequired => false but enforces permission::require_can_view_ratings() inside the rendered fragment, and the review list is only included when permission::can_view_reviews() passes. This aligns with the plugin's documented goal of showing course ratings to anonymous visitors on public courses while still honouring course visibility and the forcelogin setting.
Global report builder datasource. classes/reportbuilder/datasource/courseratings.php exposes all ratings and reviews across the whole site. Access is governed entirely by core report builder capabilities (moodle/reportbuilder:edit/editall) and report audiences, which is the standard Moodle pattern for custom-report datasources; no additional capability check is expected at the datasource level.
Minor cosmetic issue (not a finding). The language string key privacy:metadata:tool_courserating_rating:cohortid is used for the courseid metadata field; the key name is a copy-paste leftover (the displayed value is correctly "Course id"). Renaming the key would improve clarity but has no functional effect.
MOODLE_INTERNAL guards are applied correctly. Files with file-scope side effects (version.php, settings.php, db/*.php, lang/en/*) carry the guard, while lib.php (function definitions only) and the autoloaded namespaced classes correctly omit it, consistent with the moodle-cs rules.