Differentiator
local_differentiator
- #1SQL queries assembled by string concatenation of the language code and language strings
- #2Web service functions do not enforce capabilities; write endpoints declare none
- #3Broken, unreachable form/fragment code path with latent IDOR and references to non-existent method and columns
- #4No Privacy API (GDPR) provider despite storing per-user personal content
- #5Missing thirdpartylibs.xml for bundled JavaScript libraries
A local plugin that provides a Vue.js single-page tool for teachers and users to formulate, save, duplicate and delete personal "learning goals" (based on Ian Byrd's Differentiator). Learning goals are stored per user in the plugin's own tables and manipulated through AJAX web service functions; a large seed dataset of thinking-skill/content/resource/product/group vocabulary is installed via db/upgrade.php.
The plugin's core data handling is implemented correctly and defensively. Every web service function ignores the client-supplied userid and self-scopes all database operations to $USER->id, so there is no cross-user access (IDOR) on the live code paths; require_login and require_sesskey are enforced by the AJAX external-function dispatcher, and save_learninggoal performs an explicit ownership check before updating. User-supplied learning-goal text is returned as PARAM_TEXT and rendered through Vue's auto-escaping {{ }} interpolation (no v-html), so there is no XSS, and all data requests go through Moodle's core/ajax wrapper (no raw HTTP, no eval, no dynamic script in the bundle).
The deductions are a cluster of low-severity issues, none of them an exploitable vulnerability:
- Several web-service queries assemble SQL by string concatenation of the language code (
$displaylang) andget_string()output instead of using bound parameters. This is a SQL-injection-shaped pattern, but it is not exploitable because every source of the language value is constrained by core to an installed language code (PARAM_LANG/PARAM_SAFEDIR+translation_exists()), which strips SQL metacharacters. - The external functions never call
require_capability(); the declaredlocal/differentiator:editcapability is advisory only and the write endpoints declare no capability at all. Impact is limited because access is already gated by login/sesskey and every operation is self-scoped. - A broken, dead form/fragment code path (
output_fragment_mform+ theform_controllerclasses) references a non-existent method and non-existent table columns and contains a latent unscopedget_record(IDOR), but it is unreachable from the shipped frontend and would fatal-error before disclosing or writing anything. - Two compliance gaps: no Privacy (GDPR) provider despite storing per-user personal content, and no
thirdpartylibs.xmlfor the bundled Vue/Vuex/vue-router stack.
Because there are no medium, high or critical findings and the security-relevant patterns are all mitigated in context, the plugin sits just below the top tier: solid fundamentals with a handful of hygiene, correctness and compliance issues to clean up.
local_differentiator is a Vue.js single-page app (served from index.php, backed by six AJAX web service functions) that lets authenticated users create and manage personal learning goals.
The good news dominates the security picture:
- No IDOR. Every external function (
get_learninggoal,get_learninggoals,save_learninggoal,delete_learninggoal,duplicate_learninggoal) discards the client-supplieduseridand rebinds the query to$USER->id.save_learninggoaladditionally re-reads the target row and throws ifuseriddoesn't match the caller. Deletion filters on['id' => ..., 'userid' => $USER->id]. - CSRF/auth covered.
core_external\external_api::call_external_function()enforcesrequire_loginandrequire_sesskeyfor these cookie/AJAX calls before the plugin code runs. - No XSS. Learning-goal text is returned as
PARAM_TEXTand rendered via Vue text interpolation; there is nov-html. The minified bundle contains noeval,document.write,atob, or rawfetch/XMLHttpRequest. - User data queries are parameterized (
:id,:userid).
What brings the grade to B+ is a set of low-severity issues:
- SQL assembled by string concatenation of the display language and
get_string()values inhandlers.phpandlearninggoal.php. Verified non-exploitable — the language value is always sanitized to an installed language code by core (PARAM_LANG/PARAM_SAFEDIR+translation_exists()), and in the AJAX pathexternal_settings::get_lang()is empty so the value comes from the equally-sanitized$SESSION->lang/$USER->lang. Still a latent-injection anti-pattern that should use placeholders. - Capabilities are declared but not enforced. No external function calls
require_capability(); thecapabilitieskey indb/services.phpis advisory, and the three write endpoints declare no capability at all. Low impact given login+sesskey+self-scoping. - Dead, broken form/fragment code (
local_differentiator_output_fragment_mformand theform_controllerhierarchy) calls a non-existentdifferentiator::get_id(), writes to non-existent columns (name,description,differentiatorid), and reads a row without auseridfilter (latent IDOR). It is unused by the frontend and would fatal-error if invoked. - No Privacy (GDPR) provider despite storing identifiable per-user content.
- No
thirdpartylibs.xmlfor the bundled Vue 3, Vuex, vue-router, vue3-toastify and vue-input-autowidth libraries.
Overall: a competently secured plugin whose live functionality is safe, with peripheral technical debt and compliance paperwork to address.
Findings
Several web service queries build SQL by concatenating the display-language value ($displaylang) and get_string() output directly into the query text instead of using bound placeholders.
In handlers::get_handlers() the per-tab category and word queries splice $displaylang into a lang = '...' comparison, and the initial tab list splices five get_string() results into SELECT literals. In learninggoal::get_learninggoal() the default-goal branch (when learninggoalid <= 0) does the same with $displaylang and several get_string() calls.
This is a SQL-injection-shaped pattern. It is not exploitable in practice (see risk assessment) because the language value is always constrained by core to an installed language code and the strings are plugin-controlled, but it violates the requirement to parameterize queries and is fragile: a translated string containing an apostrophe would break the query, and any future change that fed a less-validated value into $displaylang would turn this into a live injection.
Low risk. I could not construct a working injection. Every avenue that feeds $displaylang is sanitized by core to an installed-language code before it reaches this code: the AJAX web-service path leaves external_settings lang empty, and both $SESSION->lang and $USER->lang pass through PARAM_SAFEDIR/PARAM_LANG plus a translation_exists() gate, which removes all SQL metacharacters and rejects any value that is not a real language directory. An attacker cannot install a language pack, so they cannot smuggle a payload through.
The residual risk is defensive/robustness rather than exploitable: the get_string() concatenation can break the query on a translation that contains an apostrophe, and the pattern would become a genuine injection if the language source were ever loosened. Blast radius if it were exploitable would be broad (these are unauthenticated-parameter-free reads returning shared vocabulary), which is why it is worth fixing even though it is currently inert.
These functions are the AJAX backend for the Vue app. $displaylang is computed as $settings->get_lang() ?: ($SESSION->lang ?? $USER->lang).
Tracing each source against core:
external_settings::get_lang()is populated (for token/REST web services) from themoodlewssettinglangrequest parameter usingPARAM_LANG(webservice/lib.php::set_web_service_call_settings). In the AJAX path used here (lib/ajax/service.php) it is never set, so it returns the empty-string default and the code falls through to the session/user language.$SESSION->langis set inlib/setup.phpfrom thelangGET parameter afterclean_param(..., PARAM_SAFEDIR)and atranslation_exists()check.$USER->langis re-cleaned withPARAM_LANGon every session load (\core\session\manager).
PARAM_LANG runs the value through PARAM_SAFEDIR (which allows only [a-zA-Z0-9_-], stripping quotes, spaces, parentheses and semicolons) and then requires it to be an installed language pack. The tab prefixes (ts/c/r/p/g) and tab ids spliced into the same queries come from a hard-coded UNION and are not attacker-influenced.
$categorysql = 'SELECT wc.id AS id,
' . $tab->id . ' AS parenttabid,
wce.' . $handlers->tabs[$tab->id]->tabprefix . 'wcetitle as cattitle,
wce.' . $handlers->tabs[$tab->id]->tabprefix . 'wcetext as cattext
FROM {local_differentiator_' . $handlers->tabs[$tab->id]->tabprefix . 'wce} wce
JOIN {local_differentiator_' . $handlers->tabs[$tab->id]->tabprefix . 'wc} wc
ON wc.id = wce.' . $handlers->tabs[$tab->id]->tabprefix . 'wcid AND wce.lang = \'' .
$displaylang . '\'
ORDER BY wc.sort ASC';
Use a bound placeholder for the language value and pass it via the $params argument of get_records_sql():
$categorysql = "SELECT wc.id AS id,
:parenttabid AS parenttabid,
wce.{$prefix}wcetitle as cattitle,
wce.{$prefix}wcetext as cattext
FROM {local_differentiator_{$prefix}wce} wce
JOIN {local_differentiator_{$prefix}wc} wc
ON wc.id = wce.{$prefix}wcid AND wce.lang = :lang
ORDER BY wc.sort ASC";
$categories = $DB->get_records_sql($categorysql, ['parenttabid' => $tab->id, 'lang' => $displaylang]);
The {$prefix} table/column fragments come from a fixed hard-coded set (ts, c, r, p, g) and are safe, but the lang value must be bound.
$sql = "SELECT 0 AS id, '" . get_string('thinkingskill', 'local_differentiator') . "' AS tabtitle, '#009' AS tabcolor,
'ts' AS tabprefix, 'thinking_skill' AS targetinput
UNION
SELECT 1 AS id, '" . get_string('content', 'local_differentiator') . "' AS tabtitle, '#600' AS tabcolor,
Build this static five-row list in PHP (e.g. an array of stdClass objects) rather than a UNION query, or bind the titles as parameters. A localized string containing a single quote (common in French/Italian translations) would otherwise produce a SQL syntax error.
$sql = "SELECT 0 AS id, '" .
get_string('clicktoedit', 'local_differentiator') . "' AS \"name\", " .
"'' AS \"description\", '" .
get_string('prethinkingskill', 'local_differentiator') . "' AS \"pre_thinking_skill\", " .
"(SELECT tswetext from {local_differentiator_tswe} WHERE tswid = 1 AND lang = '" . $displaylang . "') AS " .
Bind $displaylang as a named parameter (:lang) reused across the sub-selects, and either bind the get_string() literals or assemble this default record as a plain PHP stdClass without a database round-trip at all.
None of the six external functions call require_capability(). Access control relies entirely on validate_context(context_system::instance()) (which only enforces require_login) plus the self-scoping of every query to $USER->id.
The capabilities key in db/services.php is advisory only — Moodle's AJAX dispatcher (external_api::call_external_function) does not enforce it. The three read functions declare local/differentiator:edit, while the three write functions (save, delete, duplicate) declare no capability at all, which is both inconsistent and the opposite of what one would expect (a write-type capability is declared on reads but omitted on writes).
Consequently the plugin's own capabilities (local/differentiator:view, local/differentiator:edit) do not actually gate the API: an administrator who prohibits the capability for a role cannot prevent that role from calling any of the functions.
Low risk. There is no privilege escalation or cross-user impact: the missing capability checks are backstopped by mandatory login, sesskey validation, and strict self-scoping of every query to the authenticated user's own records. The real defect is that the declared capabilities are decorative — an administrator cannot use them to disable the tool for a role — and the read/write declarations are inconsistent. Worth fixing for correctness and defense-in-depth, but it does not expose data or allow unauthorized actions.
For AJAX/cookie calls, external_api::call_external_function() enforces isloggedin() and require_sesskey() before invoking the function, and validate_context() runs require_login(). There is no capability enforcement anywhere in that chain for the plugin's functions. Every function then constrains its DB work to $USER->id, so a caller can only ever read or modify their own learning goals regardless of capabilities. Both local/differentiator:view and :edit default to CAP_ALLOW for the user archetype, so in a default configuration all authenticated users would pass the check even if it were enforced.
A user whose role has local/differentiator:edit explicitly prohibited can still invoke local_differentiator_save_learninggoal (or delete/duplicate) via the standard AJAX endpoint with a valid sesskey, because no capability is checked. The operation still only affects that user's own rows, so the practical effect is that the capability toggle simply doesn't work.
'local_differentiator_save_learninggoal' => [
'classname' => 'local_differentiator\external\learninggoal',
'methodname' => 'save_learninggoal',
'description' => 'Save a specific learning goal.',
'type' => 'write',
'ajax' => true,
],
Declare the capability on the write functions for consistency, e.g. 'capabilities' => 'local/differentiator:edit', and enforce it inside each external function after validate_context():
require_capability('local/differentiator:edit', \context_system::instance());
Enforcing the capability in code (not just declaring it) is what actually makes an administrator's role override effective.
self::validate_context(\context_system::instance());
Add an explicit require_capability('local/differentiator:edit', \context_system::instance()); immediately after validate_context() in save_learninggoal, delete_learninggoal and duplicate_learninggoal (and :view/:edit on the read functions).
The local_differentiator_output_fragment_mform() fragment callback and the entire form_controller / learninggoal_edit / learninggoal_edit_controller hierarchy are dead, non-functional code left over from a module-style template. The live frontend edits learning goals exclusively through the external API, never through this fragment.
If this path were ever reached it would fail, and it also contains a latent access-control gap:
learninggoal_edit_controller::handle_submit()calls$this->differentiator->get_id(), but thedifferentiatorclass defines noget_id()method — this is a fatal error.- The same method writes
name,descriptionanddifferentiatoridtolocal_differentiator_lg, but none of those columns exist in the table (install.xmldefinestitle,subject,lgcontent, etc.) —insert_record/update_recordwould throw. learninggoal_edit_controller::build_customdata()loads a learning goal with$DB->get_record('local_differentiator_lg', ['id' => $this->learninggoalid])— without anyuseridfilter (a latent IDOR).differentiator::user_has_capability()references$this->context, a property the class never declares or initialises.
Because the display code only reads the non-existent name/description fields and the submit path fatals before writing, no data is actually disclosed or corrupted — but shipping broken, reachable code is a quality and maintenance risk and the unscoped get_record would become a real IDOR the moment the form were "fixed" to use real columns.
Low risk. The code is unreachable from normal use and self-destructs (fatal error / dml_exception) before it can leak or write data, so the latent IDOR in build_customdata() does not currently expose another user's fields (the display only reads columns that don't exist). The concern is code quality and future-proofing: dead, broken code that ships to production is a maintenance hazard, and the missing userid scope is exactly the kind of gap that becomes a live vulnerability during a well-intentioned refactor.
The fragment callback is invokable by any logged-in user (with sesskey) through Moodle's fragment API; the form_controller constructor gates it behind local/differentiator:view, which every user holds by default. The Vue frontend (store.js, learninggoals-edit.vue) never calls it — it uses the local_differentiator_*_learninggoal external functions instead. Static inspection shows the submit path cannot complete: differentiator::get_id() does not exist, and name/description/differentiatorid are not columns of local_differentiator_lg.
protected function handle_submit(\stdClass $data): bool {
global $DB;
if ($this->learninggoalid && $data->learninggoalid != $this->learninggoalid) {
return false;
}
$learninggoal = new \stdClass();
$learninggoal->id = $this->learninggoalid;
$learninggoal->differentiatorid = $this->differentiator->get_id();
$learninggoal->name = $data->name;
$learninggoal->description = $data->description;
if ($learninggoal->id) {
$DB->update_record('local_differentiator_lg', $learninggoal);
} else {
$DB->insert_record('local_differentiator_lg', $learninggoal);
}
return true;
}
Remove this dead form/fragment subsystem entirely (it duplicates the working external-API flow), or rewrite it to call an existing method and the real columns (title, plus the pre_*/thinking_skill/... fields) and scope the load/save to $USER->id.
$this->learninggoal = $DB->get_record('local_differentiator_lg', ['id' => $this->learninggoalid]);
If this code is retained, add ownership scoping: $DB->get_record('local_differentiator_lg', ['id' => $this->learninggoalid, 'userid' => $USER->id]);
function local_differentiator_output_fragment_mform($args) {
$differentiator = new \local_differentiator\differentiator();
Delete local_differentiator_output_fragment_mform() and the classes/form/ controllers if they are unused, to remove the broken, reachable fragment endpoint.
public function user_has_capability(string $capability): bool {
return \has_capability($capability, $this->context);
}
$this->context is never defined. Either remove this unused method or initialise a $context property (e.g. context_system::instance()) in the constructor.
The plugin stores identifiable, user-generated content — each row in local_differentiator_lg carries a userid and the user's freely-typed learning-goal text (title, subject, thinking_skill, etc.) — but ships no Privacy API implementation. There is no classes/privacy/provider.php and no reference to core_privacy anywhere in the codebase.
Under Moodle's GDPR framework a plugin that stores personal data must implement \core_privacy\local\metadata\provider and \core_privacy\local\request\core_userlist_provider (plus the request/export/delete plumbing). Because it doesn't, this user data is invisible to Moodle's data export and "delete my data" tooling, and the plugin is reported as having an unimplemented privacy provider.
Low risk. This is a compliance/GDPR gap rather than an exploitable security issue. The practical consequences are that subject-access exports omit the user's learning goals and account deletion may leave orphaned personal rows behind. It should be addressed with a full metadata + request provider (not merely a null_provider, since real personal data is stored).
db/install.xml defines local_differentiator_lg with a userid foreign key to {user} and text fields holding the learner's own wording. The external functions read and write these rows keyed by $USER->id. This is exactly the kind of per-user data the Privacy API is designed to describe and export/erase.
The shipped AMD bundle amd/build/app-lazy.min.js embeds several third-party libraries (its .LICENSE.txt sidecar and vue/package.json identify Vue 3, Vuex, vue-router, vue3-toastify and vue-input-autowidth), but the plugin contains no thirdpartylibs.xml declaring them.
Moodle requires any bundled third-party code to be declared in thirdpartylibs.xml so that library provenance, versions and licenses are tracked (and so the code is exempted from Moodle coding-style checks). The bundle carries MIT attribution comments, confirming it contains external code.
Low risk. No security impact — this is a packaging/compliance requirement. The consequence is that library versions and licenses are untracked, which complicates vulnerability management and Moodle plugin-directory validation.
The vue/ directory holds the un-built source and package.json; the artifact actually shipped and loaded by Moodle is the webpack-built amd/build/app-lazy.min.js, which inlines the runtime libraries listed above. Moodle core does not ship Vue/Vuex/vue-router as reusable AMD modules, so this is legitimately bundled third-party code that must be declared.
/*!
* vue-router v4.6.4
* (c) 2025 Eduardo San Martin Morote
* @license MIT
*/
/*!
* vuex v4.1.0
* (c) 2022 Evan You
* @license MIT
*/
Add a thirdpartylibs.xml in the plugin root declaring each bundled library, for example:
<libraries>
<library>
<location>amd/build/app-lazy.min.js</location>
<name>Vue</name>
<version>3.5.26</version>
<license>MIT</license>
</library>
<!-- Vuex 4.1.0, vue-router 4.6.4, vue3-toastify 0.2.8, vue-input-autowidth 2.2.1 -->
</libraries>
| Library | Version | License | Declared |
|---|---|---|---|
Vue Frontend framework powering the single-page learning-goal editor UI (bundled into amd/build/app-lazy.min.js). | 3.5.26 | MIT | Missing |
Vuex Client-side state store holding learning goals, handlers and strings (bundled into amd/build/app-lazy.min.js). | 4.1.0 | MIT | Missing |
vue-router Hash-based client-side routing between the overview, edit and new-goal views (bundled into amd/build/app-lazy.min.js). | 4.6.4 | MIT | Missing |
vue3-toastify Toast notifications (e.g. "Copied to clipboard") in the UI (bundled into amd/build/app-lazy.min.js). | 0.2.8 | MIT | Missing |
vue-input-autowidth Vue directive that auto-sizes the inline learning-goal text inputs (bundled into amd/build/app-lazy.min.js). | 2.2.1 | MIT | Missing |
Positive — access control is done well on the live paths. All six web service functions ignore the client-supplied userid and rebind every query to $USER->id; save_learninggoal re-reads the row and throws nopermissions if the owner differs; delete_learninggoal filters on ['id' => ..., 'userid' => $USER->id]. Combined with the require_login/require_sesskey enforced by external_api::call_external_function(), there is no IDOR or CSRF on the shipped functionality. The stale // TODO check if the learning goal really belongs to the user comment in delete_learninggoal is misleading — the userid filter already present makes the check unnecessary; the comment should be removed.
Positive — no XSS and no raw HTTP. Learning-goal fields are returned as PARAM_TEXT and rendered with Vue's escaping {{ }} interpolation (no v-html), and all network access goes through Moodle's core/ajax. A scan of the minified bundle found no eval, document.write, atob, fromCharCode, dynamic <script> injection, or raw fetch/XMLHttpRequest.
No PHPUnit tests. The plugin ships Behat coverage (add/delete/duplicate/clipboard) but no unit tests, so the external functions and their ownership checks are not exercised by phpunit. Adding unit tests for save/delete/duplicate (including the cross-user rejection case) would guard against regressions in the access-control logic.
Minor: unused $dbman in db/upgrade.php. $dbman = $DB->get_manager(); is assigned at the top of the upgrade function but never used (the upgrade steps are all insert_record/update_record seed-data operations — no schema changes, which is correct). The unused variable can be removed. All DDL correctly lives in db/install.xml.
Informational: save_learninggoal inserts new rows without setting lang. The local_differentiator_lg.lang column is NOT NULL with no XMLDB default, but Moodle's DDL generator assigns an implicit empty-string default for NOT NULL char columns ($default_for_char = ''), so new goals are stored with lang = '' and the insert succeeds on both PostgreSQL and MariaDB (confirmed by the CI matrix). This is harmless because the plugin never filters saved goals by language, but populating lang (e.g. from current_language()) would be more correct.
Informational: index.php is registered as an admin_externalpage but only calls require_login() (no admin_externalpage_setup() / capability check), which is intentional since the tool is meant for all users and every operation is self-scoped. The pre-require_login() redirect on the id parameter uses PARAM_INT and targets a fixed internal path, so it is not an open redirect.