H5P Themer
local_h5pthemer
H5P Themer (local_h5pthemer) lets administrators, category managers, and teachers customise the visual theme (colours), density, and custom CSS of H5P content across a Moodle site, supporting both mod_hvp and core H5P (core_h5p / mod_h5pactivity). Configuration cascades through a Global → Category → Course inheritance hierarchy (themes override, custom CSS accumulates). It ships settings pages, a custom admin setting, an authenticated external (AJAX) web service that resolves the effective configuration, and a frontend AMD module that injects CSS variables and stylesheets into H5P iframes.
The plugin is well engineered and shows a strong, correctly-implemented security posture. Every state-changing entry point enforces the right controls: category_settings.php and course_settings.php call require_login() and the appropriate require_capability() (moodle/category:manage / moodle/course:update), use moodleform (automatic sesskey handling) for saves, and guard the custom GET reset action with require_sesskey(). The external service (local_h5pthemer\external\get_config) is loginrequired and calls validate_context(), which (confirmed in core) enforces course access via require_login() — an unenrolled user receives a require_login_exception, and the plugin's own PHPUnit suite asserts this.
The highest-risk feature — raw Custom CSS injected into H5P iframes — is correctly gated to site administrators (moodle/site:config) on all three levels; non-admin submissions of the read-only field are ignored server-side and existing values preserved, so there is no privilege-escalation path. Teacher/manager-settable theme colours are strictly validated both server-side (util::clean_css_color, rejecting ;{}<>"'\ and javascript/expression/url/@import/behavior/eval) and again client-side, and are written to the DOM via safe APIs (createTextNode, textContent, jQuery .css()), closing CSS/DOM-injection and stored-XSS vectors. Course/category names rendered in the inheritance panel are not passed through format_string() but are not exploitable because Moodle's Mustache engine escapes all variables with s() (ENT_QUOTES).
Database access is fully parameterized, DDL lives only in db/upgrade.php, the Privacy API (null_provider) is appropriate (no per-user data), thirdpartylibs.xml is present and correct, MOODLE_INTERNAL guards are applied exactly where required, and there is substantial PHPUnit coverage including explicit XSS/injection cases. The only findings are low-severity, non-security issues: a backup/restore routine that reads and writes a legacy config_plugins location the plugin migrated away from (silently dropping course theme settings on backup/restore), a per-page-load recomputation of the resolved config that would benefit from caching, and the cosmetic format_string() omission.
H5P Themer is a cleanly written, security-conscious local plugin. I read every PHP, JS, and Mustache file in full and verified the security-relevant core APIs it depends on (external_api::validate_context, the Mustache s() escaper, and the config_plugins → table migration) directly against the Moodle 5.1+ source tree.
Security posture — sound. The three review-critical areas all hold up:
- Custom CSS (raw, injected into iframes) is restricted to site administrators on Global, Category, and Course levels. The category/course pages only persist
custom_csswhenhas_capability('moodle/site:config', ...)is true; otherwise the previously stored value is preserved. The read-only attribute on the field is purely cosmetic — the real enforcement is server-side, so a category manager or teacher cannot inject CSS. - Theme colours (teacher/manager-settable) pass a strict allow-list validator server-side (
clean_css_color) and again in the browser before being written withcreateTextNodeinside a:root { ... }block, so values cannot break out of the CSS context. - The external web service requires login and validates the course context, which enforces enrolment; the returned data is non-sensitive theme/colour/CSS meant to be rendered to all course participants.
Engineering quality — high. Parameterized SQL throughout, DDL confined to db/upgrade.php, correct MOODLE_INTERNAL guard usage, an appropriate null_provider Privacy implementation, event observers that clean up on course/category deletion, and a comprehensive PHPUnit suite that includes CSS-injection and access-control assertions.
Findings (all low, none security):
- Backup/restore operate on the legacy
course_{id}_configentry inconfig_pluginsrather than thelocal_h5pthemer_coursetable the plugin migrated to — course theme settings are silently omitted from backups and not restored. - The effective configuration is fully resolved (with DB queries) on every eligible page load to compute a cache-key hash; this should be cached.
- Course/category names are output without
format_string()(XSS-safe thanks to Mustache escaping, but it skips filter processing).
Findings
The plugin stores course-level configuration in the local_h5pthemer_course database table (written by course_settings.php, read by util::get_resolved_config_for_course()). The upgrade step in db/upgrade.php (2026090300) explicitly migrates any old course_{id}_config entries out of config_plugins and into that table, then deletes them.
The backup and restore classes, however, still operate on the old config_plugins location:
- Backup calls
get_config('local_h5pthemer', "course_{$courseid}_config"). After the migration this key no longer exists, soget_config()returnsfalseand the backup stores an empty string. Course theme settings are therefore never included in a course backup. - Restore calls
set_config("course_{$courseid}_config", ...), writing to a key the running plugin never reads (it reads the table). The restored value has no effect, and it re-creates exactly the kind of per-courseconfig_pluginsentry the migration was written to eliminate.
The net effect is a silent data-integrity defect: course-level theme overrides are lost when a course is backed up, restored, or duplicated, and restore pollutes config_plugins with orphaned entries.
Low risk. This is a functional/data-integrity defect in a secondary feature, not a security issue. There is no attacker and no data exposure — the only consequence is that course theme overrides are silently omitted from course backups/duplicates and that restore leaves unused config_plugins entries. It is reached only through the normal backup/restore workflow (teacher/admin). Because the stray set_config writes stay within the plugin's own config namespace, they are still removed on plugin uninstall, so there is no lasting pollution beyond the install's lifetime.
backup/moodle2/* define the Moodle 2 backup/restore steps for course-level plugin data. The plugin's storage model changed in db/upgrade.php: course_%_config rows were migrated from config_plugins into the dedicated local_h5pthemer_course table, and all runtime reads/writes (course_settings.php, util::get_resolved_config_for_course()) use that table. The backup/restore code was not updated to match, so it targets a location that is empty after upgrade.
$courseid = $this->step->get_task()->get_courseid();
$configvalue = get_config('local_h5pthemer', "course_{$courseid}_config");
$coursenode->set_source_array([
['id' => $courseid, 'configvalue' => $configvalue !== false ? $configvalue : ''],
]);
Source the backup element from the local_h5pthemer_course table instead of config_plugins:
$courseid = $this->step->get_task()->get_courseid();
$record = $DB->get_record('local_h5pthemer_course', ['courseid' => $courseid], 'config');
$configvalue = ($record && $record->config !== null) ? $record->config : '';
$coursenode->set_source_array([
['id' => $courseid, 'configvalue' => $configvalue],
]);
(Inject global $DB; or use the backup step's DB access as appropriate.)
public function after_restore_course() {
$courseid = $this->task->get_courseid();
// Restore course-level configuration.
if ($this->courseconfig && isset($this->courseconfig->configvalue)) {
set_config("course_{$courseid}_config", $this->courseconfig->configvalue, 'local_h5pthemer');
}
}
Restore into the local_h5pthemer_course table (the location the plugin actually reads), inserting or updating keyed on the restored course id, and skip empty payloads:
public function after_restore_course() {
global $DB;
$courseid = $this->task->get_courseid();
if ($this->courseconfig && !empty($this->courseconfig->configvalue)) {
$existing = $DB->get_record('local_h5pthemer_course', ['courseid' => $courseid]);
if ($existing) {
$existing->config = $this->courseconfig->configvalue;
$existing->timemodified = time();
$DB->update_record('local_h5pthemer_course', $existing);
} else {
$DB->insert_record('local_h5pthemer_course', (object)[
'courseid' => $courseid,
'config' => $this->courseconfig->configvalue,
'timecreated' => time(),
'timemodified' => time(),
]);
}
}
}
local_h5pthemer_extend_navigation() runs on every page render. When util::should_load_themer() allows it (i.e. most course and activity pages), it calls util::get_config_hash_for_course(), which in turn calls util::get_resolved_config_for_course() to build the entire effective configuration purely so it can be hashed into a lightweight cache key for the frontend's sessionStorage.
For a course context this executes several database queries every page load — the category path lookup, an IN (...) fetch of category configs, and the course config fetch — even on pages that contain no H5P content at all (the frontend only fetches the real config via AJAX once it actually finds an H5P iframe). The work is duplicated: the config is resolved server-side to make the hash, and then resolved again in the web service when the AMD module calls it.
The per-page cost is small (indexed single/small-row lookups), but it is paid site-wide on essentially every content page, which is a scalability concern on busy sites.
Low risk. Purely a performance/scalability observation with no security impact. Impact scales with site traffic and category depth; on small sites it is negligible, but on high-traffic installations recomputing and hashing the full configuration on every page load is avoidable overhead. Mitigated by the fact that the global/site path short-circuits after two cached get_config() calls and the course queries hit indexed columns.
The hash is used so the frontend can decide whether its cached config in sessionStorage is stale. It is genuinely needed, but it is currently regenerated from scratch on every eligible navigation build. get_resolved_config_for_course() (util.php) performs get_config() calls plus, for real courses, a get_field_sql path lookup, a get_records_select for category configs, and a get_record for the course config.
$courseid = (!empty($COURSE->id)) ? $COURSE->id : SITEID;
$cachekey = \local_h5pthemer\util::get_config_hash_for_course($courseid);
Cache the resolved configuration (or just its hash) rather than recomputing it on every request. A Moodle MUC application cache keyed by course id, invalidated when settings are saved in course_settings.php / category_settings.php and in the existing course_deleted / course_category_deleted observers, would remove the per-page DB work:
$cache = \cache::make('local_h5pthemer', 'confighash');
$cachekey = $cache->get($courseid);
if ($cachekey === false) {
$cachekey = \local_h5pthemer\util::get_config_hash_for_course($courseid);
$cache->set($courseid, $cachekey);
}
Alternatively, defer the hash computation so it only runs when H5P content is actually detected.
In util::get_inheritance_details() the category and course display names are taken straight from the database and placed into the levels array, which is then rendered by templates/settings_layout.mustache (as {{name}} in the tree and as the $a argument of the edit_level_settings string). Moodle's convention is to pass such names through format_string() before output so that text filters (for example the multilang filter) are applied and the value is consistently cleaned.
This is not an XSS vector. I verified that Moodle's Mustache engine is configured with 'escape' => 's' (renderer_base.php), and s() uses htmlspecialchars(..., ENT_QUOTES | ENT_HTML401 | ENT_SUBSTITUTE), which escapes <, >, &, " and '. The str helper renders {{name}} through that same escaper before substituting it into get_string(), so a name containing quotes or tags cannot break out of the surrounding <strong> element or the double-quoted title / aria-label attributes. The only practical consequence is missing filter processing (e.g. a multilang course name is shown raw, and special characters appear as HTML entities).
Low risk. No security impact — confirmed that both the JSON hidden-field rendering and the Mustache output escape the value (the latter via s() with ENT_QUOTES), so injection into markup or attributes is not possible. Reported only as a Moodle output-convention deviation: names that rely on text filters (e.g. multilang) will not be processed, and special characters display as entities. Blast radius is limited to the settings pages, visible only to users who already hold moodle/course:update or moodle/category:manage.
get_inheritance_details() builds the data for the "Style Origin & Active Inheritance" panel shown on the course and category settings pages. The names flow into a hidden JSON form field (local_h5pthemer_inheritance_json, rendered with attribute escaping by the forms library) and then into the Mustache template via the settings AMD module. Both output stages escape the value, so the gap is a display-convention one, not a security one.
$catname = $catrecords[$catid]->name;
Format the name in its category context before storing it in the level array:
$catname = format_string($catrecords[$catid]->name, true, ['context' => \context_coursecat::instance($catid)]);
$coursename = $DB->get_field('course', 'fullname', ['id' => $courseid]);
Apply format_string() with the course context:
$coursename = format_string(
$DB->get_field('course', 'fullname', ['id' => $courseid]),
true,
['context' => \context_course::instance($courseid)]
);
| Library | Version | License | Declared |
|---|---|---|---|
h5p-theme-picker Web component (`<h5p-theme-picker>`) that provides the interactive theme and colour picker UI on the plugin's settings pages. Bundled minified as js/h5p-theme-picker.js and loaded as an ES module. | 0.0.13 | MIT | ✓ |
Strong defence-in-depth, correctly implemented. The security-critical controls were traced end-to-end and hold up: raw Custom CSS is genuinely admin-only (server-side moodle/site:config gate on all three levels, with the read-only field's submitted value ignored for non-admins), theme colours are validated by an allow-list both server-side (util::clean_css_color) and client-side (isValidCssValue in themer.js) and written to the DOM via createTextNode / textContent / jQuery .css(), and the external service enforces course access through validate_context(). State-changing actions are protected by moodleform sesskey handling and an explicit require_sesskey() on the GET-based inheritance reset.
Fragile prototype patch in amd/src/settings.js. createPicker() temporarily overrides HTMLElement.prototype.getAttribute globally to feed initial attributes into the web component's constructor, restoring it in a finally block. It works because construction is synchronous, but overriding a built-in prototype method (even briefly) is brittle and could interfere with other synchronously-constructed elements. Consider passing configuration through the component's documented constructor options or by setting attributes before connection instead.
No db/uninstall.php is needed. The plugin only persists data in its own tables (local_h5pthemer_course, local_h5pthemer_category, dropped automatically on uninstall) and in its own config_plugins namespace (also removed automatically). The Privacy API null_provider is appropriate because neither table stores a user identifier — only course/category ids and configuration JSON.
CI and release workflows are clean. .github/workflows/* use GitHub Actions secrets and an OIDC token-exchange publish flow rather than embedded credentials; the empty MySQL password is confined to the throwaway CI service container. These files are build/release infrastructure and not part of the plugin's runtime attack surface.