QuickNote
local_quicknote
QuickNote (local_quicknote) is a local plugin that lets students capture private per-course notes while reading course material. Users can highlight text to save it as a quote, attach a personal reflection, and revisit the original passage via browser text fragments. Notes are managed through a Boost sidebar drawer and a dedicated Notes Center page (view.php) that supports course filtering, real-time search, pagination, and PDF/Markdown export. All create/read/delete operations go through three AJAX web services (also exposed to the Moodle mobile app). Teachers can enable/disable QuickNote per course or per activity, and administrators set a site-wide default, position, disabled page-type patterns, and notes-per-page.
The plugin demonstrates consistently strong security hygiene across every attacker-facing surface. The three external services (save_note, get_notes, delete_note) validate parameters, call require_login($course) and validate_context(), and scope every query to $USER->id, so no user can read, modify, or delete another user's notes — there is no IDOR, no SQL injection (all queries are parameterized, including sql_like with sql_like_escape), and no missing-capability path that damages other users.
Output handling is careful throughout: the Notes Center template escapes all note data with {{ }}, the sidebar JavaScript renders user content exclusively with textContent and scheme-checks link hrefs, PDF/Markdown export runs content through format_text(..., FORMAT_PLAIN) (which applies s()), and stored URLs are constrained by PARAM_URL (which rejects javascript:). Cross-window postMessage handling enforces an origin check, content length is capped to limit storage abuse, and the Privacy API is fully implemented including the user-list provider.
No high or critical issues were found. The remaining items are quality and robustness refinements: a broken class_exists compatibility guard that causes redundant (but harmless) config writes on Moodle 4.4+, notes not being purged when a course is deleted for the narrow case of non-enrolled authors (a data-retention gap), the per-course enable toggle being enforced only in the UI rather than in the web services, the absence of any automated tests for security-sensitive code, and a non-idiomatic synthetic config-plugin storage pattern. None of these are exploitable to harm other users.
QuickNote is a well-engineered local plugin for private per-course student notes. The review read every PHP, JavaScript, Mustache, and configuration file in full and verified the relevant Moodle core behavior (PARAM_URL scheme validation, format_text FORMAT_PLAIN escaping, and the course-deletion → unenrolment event cascade) against the core source tree.
Security posture — strong. The attacker-facing surface is the three AJAX/mobile web services and the view.php Notes Center. Both are handled correctly:
- Access control: every service call resolves the course, calls
require_login($course)andvalidate_context(), and scopes all DB access to$USER->id. Update and delete additionally verify record ownership before acting. There is no cross-user read/write/delete path. - Injection: all SQL uses placeholders; the free-text search uses
sql_like()withsql_like_escape(). - XSS: note content and quotes are stored as
PARAM_RAWbut every render path is safe — Mustache{{ }}auto-escaping in the Notes Center,textContentin the sidebar JavaScript, andformat_text(FORMAT_PLAIN)pluss(clean_param(..., PARAM_URL))in the PDF/Markdown exporters. The only triple-mustache is Moodle's own paging bar. - URL safety: page/quote URLs pass through
PARAM_URLat input and are re-cleaned at output; the sidebar additionally restricts link schemes tohttps?:/#. - Other:
postMessagehandlers enforce same-origin, content lengths are capped, and no direct DB/filesystem/shell access, superglobals, or out-of-upgrade DDL exist.
Findings (all low / info):
- A compatibility guard in the
course_updatedobserver checks\core\hook\course\after_form_submission, which is not the real class name (\core_course\hook\after_form_submission), so on Moodle 4.4+ both the hook and the observer run and write the same per-course config redundantly. - The
course_deletedobserver removes per-course config but not the notes; enrolled users' notes are cleaned by the unenrolment cascade, but notes authored by non-enrolled users (admins/managers withcourse:view) are orphaned and become unreachable by the Privacy API. - The per-course/per-activity enable toggle is enforced only in the UI — the web services never check it, so a user can still store/read/delete their own notes via the API when QuickNote is "disabled".
- No automated tests exist for the external services or privacy provider.
- Per-course settings are stored under synthetic config-plugin names (
local_quicknote_course_<id>), a non-idiomatic use ofconfig_plugins.
No third-party libraries are bundled (no thirdpartylibs.xml is required). Overall this is a security-conscious codebase whose remaining issues are refinements rather than vulnerabilities.
Findings
The course_updated event observer is intended to persist the per-course enable flag only on Moodle versions older than 4.4, where the Hooks API is unavailable. On 4.4+ the persistence is supposed to be handled exclusively by the course_edit_submission hook callback registered in db/hooks.php.
The guard that is meant to detect the Hooks API checks for the class \core\hook\course\after_form_submission, but that class does not exist. The actual hook class — the one the plugin itself registers in db/hooks.php — is \core_course\hook\after_form_submission (namespace core_course\hook, defined in course/classes/hook/after_form_submission.php).
Because class_exists() is given the wrong fully-qualified name, it always returns false, so the early return never fires. On Moodle 4.4+ both the hook callback and the observer execute for the same course-edit-form submission, each calling set_config() for the same local_quicknote_course_<courseid> key.
Low risk. This is a correctness/robustness defect, not a security vulnerability. In the normal course-edit flow both code paths read the same local_quicknote_enabled value and write it to the same synthetic config key, so the persisted result is correct; the only observable effect is a redundant set_config() call. There is no cross-course or cross-user impact: to trigger course_updated for a course you already need moodle/course:update on that course, and the value written is scoped to that same course. The finding is worth fixing because the intended "exactly one path runs" design is silently broken, which could mask subtler regressions if the two handlers ever diverge.
On Moodle 4.4+, saving a course through the edit form fires \core_course\hook\after_form_submission (handled by hooks::course_edit_submission, which reads local_quicknote_enabled and calls set_config('enabled', ...)) and also emits the \core\event\course_updated event (handled by this observer). Both read the same request parameter and write the same key, so the effect is a redundant duplicate write rather than a divergent value. In non-form course updates (for example the core_course_update_courses web service) the hook does not fire and the observer reads a missing parameter (null), so it does nothing — meaning the defect surfaces only as redundancy on the form path.
classes/observers.php:40Source link unavailable — plugin was reviewed from zip without a matching git ref// Skip if Moodle 4.4+ hook already handles persistence.
if (class_exists('\core\hook\course\after_form_submission')) {
return;
}
Reference the class the plugin actually registers (\core_course\hook\after_form_submission):
// Skip if Moodle 4.4+ hook already handles persistence.
if (class_exists(\core_course\hook\after_form_submission::class)) {
return;
}
Using the ::class constant (rather than a hand-typed string) lets static analysis and IDEs catch a wrong name at author time.
The course_deleted observer cleans up the plugin's synthetic per-course configuration but does not delete the notes stored for that course. For ordinary students this is covered indirectly: deleting a course unenrols every enrolled user (core's delete_course() → remove_course_contents() → enrol_course_delete() → delete_instance() → unenrol_user()), and each unenrolment fires \core\event\user_enrolment_deleted, which this plugin observes and uses to delete that user's notes for the course.
However, QuickNote also allows note creation by users who are not enrolled — hooks::get_top_of_body_html() renders the tool when is_enrolled($context) || has_capability('moodle/course:view', $context), and save_note accepts the request because require_login($course) succeeds for such users. Site administrators, managers, and category-level teachers who take notes in a course while relying on course:view are never "unenrolled", so no user_enrolment_deleted event fires for them when the course is removed. Their notes remain in local_quicknote_notes pointing at a now-deleted course.
Because view.php inner-joins {course}, these orphaned rows are invisible in the UI, and because their course context no longer exists, the Privacy API cannot reach them either: get_contexts_for_userid(), export_user_data(), and delete_data_for_user() all resolve notes through the course/system context, which is gone.
Low risk. The orphaned rows are not exposed to anyone — they are hidden from the UI (inner join on {course}) and are readable only via direct database access. The practical concern is data-retention/GDPR hygiene: personal data survives course deletion and escapes the Privacy subsystem, so a later right-to-erasure request would miss it. Blast radius is narrow (only non-enrolled authors: admins, managers, category-level teachers), and there is no exposure of one user's notes to another. Adding a single delete_records() call in the existing observer closes the gap.
Notes can contain personal reflections, so they are user personal data (correctly declared in the privacy metadata). The plugin already handles the two common lifecycle events (user_enrolment_deleted, user_deleted). The gap is specifically the course-deletion path for authors who were never enrolled, whose notes therefore never receive a cleanup trigger.
classes/observers.php:58Source link unavailable — plugin was reviewed from zip without a matching git refpublic static function course_deleted(\core\event\course_deleted $event) {
global $DB;
$courseid = $event->objectid;
$DB->delete_records('config_plugins', ['plugin' => 'local_quicknote_course_' . $courseid]);
}
Also remove the notes for the deleted course so no personal data is orphaned:
public static function course_deleted(\core\event\course_deleted $event) {
global $DB;
$courseid = $event->objectid;
// Remove any notes still attached to this course (covers non-enrolled authors).
$DB->delete_records('local_quicknote_notes', ['courseid' => $courseid]);
// Remove the synthetic per-course configuration.
$DB->delete_records('config_plugins', ['plugin' => 'local_quicknote_course_' . $courseid]);
}
Whether QuickNote is available in a course is decided by hooks::is_enabled_for_course() and the per-activity module_settings map, but that decision only governs whether the sidebar UI is injected in get_top_of_body_html(). The three web services — save_note, get_notes, and delete_note — never consult the enable flag. They gate on course access (require_login($course) + validate_context()) and ownership only.
As a result, a user who calls the AJAX/mobile functions directly can create, list, and delete their own notes in a course (or on an activity) for which a teacher or administrator has explicitly disabled QuickNote. The teacher-facing setting is presented as "Activate QuickNote in this course" / "QuickNote on this activity", which implies an enforced control rather than a display preference.
Low risk. Any enrolled user (or a user with course:view) can reach the services, but the only achievable effect is storing/reading/deleting notes in their own private space. No other user's data is affected and no capability is escalated. This is primarily an authorization-consistency and expectation-management gap rather than an exploitable vulnerability; it becomes relevant only if an institution assumes "disabled" prevents any note storage (for example on an exam activity).
The stored data is strictly the calling user's own private notes; there is no path to another user's data and no privilege change. The toggle is a feature/visibility control, so the security impact of bypassing it is minimal — the value is consistency between what a teacher configures and what the server enforces.
classes/external/save_note.php:93Source link unavailable — plugin was reviewed from zip without a matching git ref$course = get_course($params['courseid']);
require_login($course);
$context = context_course::instance($course->id);
self::validate_context($context);
If the toggle is meant to be authoritative, enforce it server-side in save_note (and consider get_notes) after validate_context(), mirroring the UI check, for example:
if (!\local_quicknote\hooks::is_enabled_for_course($course)) {
throw new \moodle_exception('disabledforcourse', 'local_quicknote');
}
(is_enabled_for_course() would need to be exposed as non-private, or the check duplicated.) If the toggle is intended only to control UI visibility, document that explicitly so administrators do not treat it as a data-prevention control. The same consideration applies to classes/external/get_notes.php and classes/external/delete_note.php.
The plugin ships no tests/ directory — there are no PHPUnit unit tests and no Behat features, even though the CI workflow (.github/workflows/ci.yml) invokes moodle-plugin-ci phpunit and behat (which currently pass vacuously because nothing is present to run).
The untested code includes the three external functions that perform all create/read/delete operations and enforce ownership and context checks, and the full Privacy API provider (export/delete across course and system contexts). These are exactly the areas where a regression could silently introduce a cross-user data-access or data-retention bug.
Low risk. This is a code-quality and maintainability gap, not a live vulnerability. Its value is preventing regressions in the ownership/context enforcement and privacy logic that currently keep the plugin safe; absence of tests raises the chance that a future edit re-introduces an access-control defect without detection.
Moodle's development standards expect PHPUnit coverage for external services and privacy providers. Tests here would pin down the ownership checks in save_note/delete_note, the user-scoping in get_notes, and the context-resolution logic in provider.php, all of which are correct today but unprotected against future change.
Per-course configuration (the enabled flag and the JSON module_settings map) is stored in config_plugins under a distinct, dynamically-generated plugin name for every course: local_quicknote_course_<courseid>. This is read and written across lib.php, classes/hooks.php, the backup/restore classes, and the observers.
The pattern is functional and is cleaned up correctly on course deletion and on uninstall (db/uninstall.php deletes all local_quicknote_course_% rows), but it is non-idiomatic: it creates an unbounded family of pseudo-plugin config namespaces, which is unusual for administrators inspecting configuration and slightly bloats config_plugins (one or two rows per configured course).
Informational. No security impact. The observation is about maintainability and adherence to conventional Moodle configuration storage; cleanup is handled on both course deletion and plugin uninstall, so no orphaned configuration accumulates.
The dynamic segment is always an integer course id (PARAM_INT / $course->id / $event->objectid), so there is no risk of injecting into the plugin name. The concern is purely stylistic and operational, not security-related.
classes/hooks.php:59Source link unavailable — plugin was reviewed from zip without a matching git ref$savedvalue = get_config('local_quicknote_course_' . $courseid, 'enabled');
Consider a dedicated settings table (for example local_quicknote_course_settings with courseid, enabled, and a module_settings text column) or a single serialized config value keyed by course. This keeps QuickNote's configuration under one component namespace and makes it easier to query and reason about. This is an architectural suggestion only — the current approach is safe and the path segment is a validated integer, so there is no injection concern.
lib.php:37Source link unavailable — plugin was reviewed from zip without a matching git ref$settingsjson = get_config('local_quicknote_course_' . $courseid, 'module_settings');
Overall the plugin is security-conscious. Every write/read of user-controlled note data was traced end to end: PARAM_RAW content is stored safely and rendered only through Mustache {{ }} escaping (Notes Center), DOM textContent (sidebar), and format_text(FORMAT_PLAIN)/s() (PDF & Markdown export). URLs are constrained by PARAM_URL at input and re-validated at output, and the sidebar additionally restricts link schemes to https?:/#. SQL is fully parameterized (including sql_like/sql_like_escape), and the postMessage iframe-highlight channel enforces a same-origin check. These were verified against core (param.php PARAM_URL, formatting.php FORMAT_PLAIN) rather than assumed.
CI matrix does not match the declared support range. .github/workflows/ci.yml tests only MOODLE_401_STABLE on PHP 7.4/8.0/8.1, but version.php requires Moodle 4.2 (2023042400) and the README targets 4.4/4.5+. The Hooks-API code paths (and the class-name bug in finding 1) are exercised only on 4.4+, which the pipeline never runs, and Moodle 5.x (the version under review) is not covered at all. Aligning the matrix with the supported branches would have surfaced finding 1.
Backup intentionally excludes note content. backup/moodle2/backup_local_quicknote_plugin.class.php backs up only the per-course enabled flag and the module-settings map, not the local_quicknote_notes rows. This is a reasonable choice for per-user personal data (avoiding it leaking into shared course backups) but means notes are not carried across a course copy/restore — worth documenting so the behavior is not mistaken for a bug.
No thirdpartylibs.xml is needed. The plugin bundles no external libraries; amd/build/*.min.js are the plugin's own compiled AMD modules, and no vendored code with third-party license headers is present.