MDL Shield

🕹️PlayerHUD Availability Condition

availability_playerhud

Print Report
Plugin Information

An availability condition (availability/condition/playerhud) that lets teachers restrict access to activities, resources, or course sections based on a student's progress in the companion PlayerHUD block — minimum Level, owned Items (with comparison operators), assigned Character class, or whether gamification is enabled. It is a required-companion extension that reads live progress data from block_playerhud and computes an access decision server-side. The plugin declares block_playerhud as a hard dependency in version.php.

Privacy API
Unit Tests
Behat Tests
Reviewed:2026-08-03
24 files·3,225 lines
Grade Justification

The plugin is well-engineered and security-conscious, with no exploitable vulnerabilities identified.

All database access uses parameterised $DB queries — there is no string concatenation of variables into SQL anywhere in the codebase. User-influenceable values from the availability tree (levelval, itemqty, itemid, classid) are consistently cast to int before use, and item/class names are passed through format_string() (which applies clean_text()/HTMLPurifier) before being embedded in the description shown to students — closing the stored-XSS vector. The block's stored configdata is read with unserialize_object(), which restricts allowed_classes to stdClass, preventing object-injection / POP-gadget instantiation from tampered configuration. Item, class, and inventory lookups are correctly scoped to the course's own PlayerHUD block instance (blockinstanceid), so a forged condition cannot leak or be satisfied by another course's data. The $not negation and the backup/restore id remapping both follow the core availability contract and are covered by regression tests.

Access is handled correctly for the plugin type: is_available() is invoked server-side by the core availability framework with a core-supplied $userid (no IDOR surface), and the frontend's item/class disclosure is only reachable through the capability-gated course/module editing forms.

The residual items are minor and non-security: the condition couples directly to the companion block's internal database schema (an acceptable but fragile consequence of the hard dependency), and get_description() calls format_string() eagerly rather than using the framework's deferred description_format_string() marker (functionally correct here because an explicit context is passed). Test coverage — PHPUnit unit tests, backup/restore tests, and Behat scenarios, including a regression test for each previously-fixed security issue — is thorough.

AI Summary

The PlayerHUD Availability Condition is a small, focused availability plugin that gates activity/section access on a student's progress in the companion block_playerhud (level, items, character class, or gamification status). The review read every shipped PHP, JS, and configuration file and verified the relevant core APIs against the Moodle 5.x source.

Security posture is strong. The plugin shows evidence of deliberate hardening (documented in CHANGES.md and docs/en/security.md), and each claimed fix was verified in the code:

  • SQL injection: none — every query uses $DB placeholders; ids are int-cast.
  • Stored XSS: mitigated — levelval/itemqty are int-cast in both save() and get_description(); item/class names pass through format_string().
  • Object injection: mitigated — configdata is read via unserialize_object() (allowed_classes => [stdClass]).
  • Cross-course data leakage: mitigated — all item/class/inventory lookups are scoped by blockinstanceid.
  • No dangerous sinks: no eval/exec, no filesystem or outbound HTTP access, no superglobals, no direct unserialize().

Two minor, non-security findings remain: direct coupling to the companion block's internal table schema (a maintainability/fragility observation, acceptable given the declared dependency), and an eager format_string() call in get_description() where core recommends the deferred description_format_string() marker (functionally safe here because an explicit context is supplied).

Testing is comprehensive: unit tests for every subtype and operator, negation regression tests, a backup/restore remap suite, an object-injection regression test with a purpose-built gadget fixture, and Behat coverage of the student-facing behaviour.

Findings

code qualityLow
get_description() formats names eagerly with format_string() instead of the deferred description_format_string() marker

The core \core_availability\condition base class documents that get_description() implementations should not call format_string() directly, because at description-gathering time modinfo/$PAGE may not be safely available. Instead it provides deferred markers — description_format_string() and description_callback() — which core resolves later in info::format_info() once the environment is ready. Core's own conditions follow this: availability_profile uses description_format_string() and availability_grade uses description_callback().

This plugin instead calls format_string() eagerly inside get_description() for the item name and the class name.

This is not a security issue and not currently a functional bug: the plugin passes an explicit ['context' => $context] to format_string(), which sidesteps the $PAGE->context notice that an earlier version hit (see CHANGES.md v1.4.1), and the output is still correctly HTML-sanitised. The finding is a deviation from the documented availability API contract — aligning with the deferred-marker pattern would make the code more robust across the contexts in which descriptions are gathered (e.g. bulk rendering, restore) and consistent with core conventions.

Risk Assessment

Low risk (code quality / API conformance). There is no exploit path: the interpolated names are teacher-controlled and are HTML-sanitised by format_string() regardless of whether the call is eager or deferred, and the explicit context prevents the historical $PAGE notice. The impact is limited to maintainability and consistency with core's documented pattern; a future change to how/when descriptions are gathered could reintroduce environment-dependent behaviour that the deferred marker is specifically designed to avoid. Blast radius is cosmetic.

Context

get_description() builds the student-facing "Not available unless..." text. For the item and class subtypes it resolves the display name from block_playerhud_items / block_playerhud_classes (scoped to the course's own block instance) and interpolates it into a language string containing <strong> markup, so the returned string is rendered as HTML. The names originate from teacher-managed block data, and format_string()'s clean_text() step strips scripts/handlers, so the rendered output is safe either way — the concern is purely conformance with the framework's intended deferred-formatting flow, not sanitisation.

Identified Code
            $a->item = format_string($itemname, true, ['context' => $context]);
Suggested Fix

Use the deferred marker so core resolves the name at display time:

            $a->item = self::description_format_string($itemname);

The item name is already looked up from the DB (scoped to the course block) just above; passing the raw name to description_format_string() lets info::format_info() apply format_string() with the correct context later. The same applies to the class-name branch.

Identified Code
            return get_string($string, 'availability_playerhud', format_string($classname, true, ['context' => $context]));
Suggested Fix
            return get_string($string, 'availability_playerhud', self::description_format_string($classname));
best practiceInfo
Condition reads the companion block's internal tables directly rather than through a published API

The condition and frontend read five of block_playerhud's internal tables directly with raw $DB SQL:

  • block_playerhud_user (player record / currentxp / enable_gamification)
  • block_playerhud_inventory and block_playerhud_items (item ownership count)
  • block_playerhud_rpg_progress and block_playerhud_classes (class assignment)

Only the level calculation goes through a public API of the block (\block_playerhud\game::get_game_stats(), guarded by class_exists()); everything else depends on the block's table names and column layout.

This is not a security defect — all queries are parameterised, the tables belong to a plugin that is a declared hard dependency (version.php requires block_playerhud 2026051301), and reaching into a companion plugin's schema is a pragmatic and common pattern when no read API exists. It is recorded as an architectural observation: the plugin is tightly coupled to the block's internal schema, so a schema change in the block (renamed column, new instance model) would silently break these conditions. The is_available() fallbacks fail safe (missing block or missing game class ⇒ restriction is not satisfied), which limits the blast radius of such drift.

Risk Assessment

Informational. No confidentiality, integrity, or availability impact: queries are parameterised, scoped to the correct block instance, and read only the evaluated user's own data. The only practical downside is fragility — the condition depends on the block's internal schema staying stable — which is mitigated by the version-pinned dependency and the fail-safe fallbacks. Included so a maintainer is aware of the coupling when either plugin evolves.

Context

availability_playerhud is explicitly a companion to block_playerhud and cannot function without it. The queries are all correctly scoped by blockinstanceid and parameterised, and are executed either server-side during access evaluation (is_available()) or inside the capability-gated editing form (frontend). The coupling is therefore a maintainability characteristic of the two-plugin design, not a runtime risk.

Identified Code
            $player = $DB->get_record('block_playerhud_user', [
                'blockinstanceid' => $block->id,
                'userid' => $userid,
            ]);
Suggested Fix

No change required. If the block later exposes read helpers (e.g. \block_playerhud\game::get_inventory_count(), ::has_class(), ::get_player()), routing these reads through them would decouple the condition from the block's storage layout. Absent such an API, the current approach is acceptable given the declared dependency.

Identified Code
                $sql = "SELECT COUNT(inv.id)
                          FROM {block_playerhud_inventory} inv
                          JOIN {block_playerhud_items} i ON i.id = inv.itemid
                         WHERE inv.userid        = :userid
                           AND inv.itemid        = :itemid
                           AND i.blockinstanceid = :blockinstanceid";
Identified Code
                $records = $DB->get_records('block_playerhud_items', ['blockinstanceid' => $block->id], 'name ASC', 'id, name');
Additional AI Notes

Prior security hardening verified. The fixes listed in CHANGES.md v1.4.2 were each confirmed present and correct in the code: int casts on levelval/itemqty (XSS), unserialize_object() for configdata (object injection), blockinstanceid scoping on all item/class/inventory lookups (cross-course leakage), correct $not inversion in is_available()/get_description(), and update_after_restore() id remapping via restore_dbops::get_backup_ids_record().

Deserialization is safe. is_available() reads the block's configdata with unserialize_object(base64_decode(...)). Core's unserialize_object() (lib/moodlelib.php) calls unserialize($input, ['allowed_classes' => [stdClass::class]]), so a tampered configuration cannot instantiate arbitrary classes. A dedicated regression test (test_is_available_level_blocks_object_injection_in_configdata) with a __wakeup() gadget fixture proves this.

XSS rendering confirmed safe against core. format_string() (via \core\formatting::format_string) applies clean_text()/HTMLPurifier by default, stripping scripts and event handlers; combined with the int casts on numeric fields, the <strong>-wrapped description strings cannot carry injected markup even from a forged availability payload.

Access model is appropriate for the plugin type. The plugin exposes no page or endpoint of its own. is_available() is called server-side by the core availability framework with a core-supplied $userid, so there is no IDOR surface. The frontend's item/class name disclosure is only reachable via core_availability\frontend::include_all_javascript(), which core invokes solely from course/moodleform_mod.php and course/editsection_form.php — both capability-gated editing forms.

Privacy provider is correct. The plugin stores no data of its own (condition configuration lives in core's course_modules.availability field, and all progress data is owned by block_playerhud), so implementing \core_privacy\local\metadata\null_provider with a privacy:metadata reason string is the right choice. The reason key exists in both language files.

Test coverage is a strength. PHPUnit tests cover every subtype and item operator, the no-block fallback, corrupt-configdata handling, negation for all subtypes, and int-cast regressions; a separate backup/restore suite exercises real backup/restore controllers; Behat features cover the student-facing level, gamification, hidden-restriction, and allow-add behaviours. Built YUI files (-debug.js, -min.js) were checked against yui/src/form/js/form.js and match the source, with Y.Escape.html() applied to item/class names in the DOM.

Documentation and packaging hygiene. .gitattributes marks docs/ and .github/ as export-ignore, so the GitHub Pages documentation (including docs/assets/js/scrollspy.js) is excluded from the release archive and never executes in a Moodle context. No thirdpartylibs.xml is present and none is needed — the plugin bundles no third-party code (the yui/build/ output is the plugin's own compiled YUI module, not a vendored library).

This review was generated by an AI system and may contain inaccuracies. Findings should be verified by a human reviewer before acting on them.