MDL Shield — Security Review

Guess It

qtype_guessit

Table of Contents

Medium(1)

#1Stored XSS: student responses rendered as raw HTML in specific_feedback

Low(7)

#2Stylesheet imports remote Google Fonts at render time
#3Plugin's supported version range omits Moodle 5.0
#4is_complete_response() and get_validation_error() return null instead of documented types
#5htmlspecialchars_decode() applied to stored gap response
#6Wordle navigation script uses document-wide selector for correct gaps
#7update_question_guessit() carries unused parameters
#8Coding-style: uppercase PHP open tag and uppercase GLOBAL keyword

Info(1)

#9No PHPUnit tests; only Behat coverage

Plugin Information

qtype_guessit is a Moodle question type that presents word- or sentence-guessing games similar to Wordle. The teacher enters a target phrase or single uppercase word; the renderer displays one input gap per word (or per letter, in Wordle mode) and provides per-attempt feedback with coloured markup for correct, partially correct, misplaced, and incorrect characters. The plugin depends on the companion qbehaviour_guessit behaviour to drive the multi-try grading flow. It ships with backup/restore, an XML import/export round-trip, a null_provider privacy class, English language strings, and AMD modules that handle gap-to-gap keyboard navigation.

moodle.org/plugins/qtype_guessit

rezeau/moodle-qtype_guessitc5de998a238452a0455bcbccb828619f727bbb99

Version:2025043000

Release:1.2

Reviewed for:5.0

Privacy API

Unit Tests

Behat Tests

Reviewed:2026-04-28

18 files·2,406 lines

Grade Justification

The plugin is structurally sound: it uses the Moodle $DB API with parameterised queries, delegates pluginfile serving to core's question_pluginfile(), ships a privacy provider (null), defines its own DB table cleanly in db/install.xml, and follows the standard question-type architecture. The main concern is the specific-feedback rendering path, which embeds previously submitted student responses into HTML without escaping. Because student gap data is collected with PARAM_RAW_TRIMMED and never sanitised before concatenation, an attacker can inject a script payload through a gap response. Practical impact is limited: the vulnerable branch only runs while the question state is todo (between Check presses, before submission), so the dominant scenario is self-XSS — a student attacking themselves — rather than cross-user reach. Remaining findings are code-quality items: a remote Google Fonts @import in the stylesheet, a $plugin->supported range that does not include Moodle 5.0, two interface methods (is_complete_response, get_validation_error) that return null instead of the documented types, an unusual htmlspecialchars_decode() of stored responses, dead parameters in update_question_guessit(), a JS scoping bug in wordlenavigation.js when multiple wordle questions render on the same page, and a couple of minor coding-style issues (uppercase <?PHP and GLOBAL).

AI Summary

qtype_guessit is a clean, focused question type with a single notable security concern and several low-severity code-quality items.

Security

Stored XSS in qtype_guessit_renderer::specific_feedback(): previously submitted gap responses are concatenated directly into HTML output. Any student can inject <script> (or other HTML) through a normal gap submission, and the payload renders the next time the same attempt page is shown. The state guard ($qa->get_state() != question_state::$todo) keeps the vulnerable branch from running once the attempt is finalised, so review-time exposure to teachers is unlikely under normal workflows; the realistic impact is self-XSS during an in-progress attempt.

Code quality / compliance

styles.css performs @import url('https://fonts.googleapis.com/...'), sending every learner's IP and Referer to a third party at page render — a privacy/data-protection concern for installations that need to control external requests.
version.php declares $plugin->supported = [401, 405], which omits Moodle 5.0 (branch 500). Since the review target is Moodle 5.0, the plugin should explicitly include it.
is_complete_response() and get_validation_error() in question.php return null (a bare return;). Both are part of the question_manually_gradable interface and core behaviours call them directly.
embedded_element() calls htmlspecialchars_decode() on the stored response before re-using it. The decoding is unnecessary (responses are stored verbatim under PARAM_RAW_TRIMMED) and obscures what data the renderer is actually working with.
update_question_guessit() accepts $options and $context parameters that are immediately overwritten or unused.
amd/src/wordlenavigation.js queries input.correct at the document level inside a per-question loop; with multiple Wordle questions on one page, the finished flag is computed against gaps from other questions.
Minor coding-style violations: <?PHP (uppercase) in version.php and GLOBAL $CFG; (uppercase) in question.php.

Architecture

File serving via qtype_guessit_pluginfile() correctly delegates to question_pluginfile().
Backup/restore, XML import/export, the install schema, and the null_provider privacy class look correct.
Behat coverage exists for create/preview/grade, backup/restore and import/export. There are no PHPUnit tests; only a helper.php that defines fixtures used by Behat.

Findings

securityMedium

Stored XSS: student responses rendered as raw HTML in specific_feedback

Exploitable by:

student

The renderer's specific_feedback() iterates over the student's previous attempt steps and concatenates each submitted gap value into HTML output without escaping.

Gap responses are collected with PARAM_RAW_TRIMMED (see qtype_guessit_question::get_expected_data() in question.php), which performs no HTML cleaning. A student can therefore submit <script>alert(1)</script> (or any HTML) as the value of any gap, and on the next render of the same question the payload is emitted verbatim inside a <div>.

The rendered string is then passed straight through html_writer::nonempty_tag('div', $this->specific_feedback($qa), ...) in the feedback() method, which wraps the HTML but does not escape it.

The vulnerable branch is gated by $qa->get_state() == question_state::$todo and "not yet fully correct". In the adaptive-style flow used by qbehaviour_guessit, that branch is exactly what runs between Check presses during an in-progress attempt — so the malicious payload renders inside the same student's session immediately after they submit it. After the attempt is submitted/graded the state is no longer todo and the branch returns early, which is what limits the practical blast radius.

Risk Assessment

Medium risk. The XSS is reachable via normal gameplay — no privileged role is required to inject the payload — but the early return in specific_feedback() ensures the rendering branch does not run once the attempt state moves past todo. In practice this means:

Self-XSS during the attempt is reliable. A student who deliberately injects a payload will execute it in their own browser between Check presses.
Teacher review of finished attempts is not affected because the question state at review time is one of the graded/final states (gradedwrong, gradedpartial, complete, gaveup, etc.), which makes $issubmitted true and the function returns empty.
Cross-user reach is limited to the unusual case where a privileged user (admin, teacher with override) opens an in-progress attempt that still has todo state for the affected question.

The code defect is unambiguous and the fix is a one-line change to wrap each concatenated response with s().

Context

get_expected_data() declares each gap field as PARAM_RAW_TRIMMED, so the engine stores responses with no HTML cleaning. get_all_responses() then walks the question_attempt step iterator and collects raw $step->get_qt_data() arrays. In the regular (non-Wordle) branch of specific_feedback() each response value is concatenated into HTML; in the Wordle branch each character is concatenated. The result is wrapped by the parent feedback() via html_writer::nonempty_tag('div', ..., ['class' => 'specificfeedback']), which preserves the inner HTML.

For comparison, embedded_element() is safe in this respect because the response value is placed into an input element's value attribute through html_writer::empty_tag(), which performs attribute escaping.

Proof of Concept

Open a quiz containing a guessit question.
In the first gap, enter:

<img src=x onerror=alert(document.cookie)>

…and fill the other gaps with arbitrary text so the attempt is partially incorrect. 3. Press Check. The adaptive-style behaviour returns the question state to todo, the renderer calls specific_feedback(), and the previously submitted answers — including the unescaped <img> tag — are emitted into the page. The onerror handler fires.

renderer.php:313

Affected Code

$formattedfeedback .= '<div class="specific-feedback input-wrapper '.$colorclass.'">'.
$studentanswer. '<span class="feedback-markup">'.$markupcode. '</span></div>';

Suggested Fix

Escape the response before concatenating into HTML. s() (alias of htmlspecialchars) is the standard helper in Moodle for plain text:

$formattedfeedback .= '<div class="specific-feedback input-wrapper '.$colorclass.'">'.
        s($studentanswer).
        '<span class="feedback-markup">'.$markupcode.'</span></div>';

Also review the htmlspecialchars_decode() call in embedded_element() (line 166) — it should not be necessary if responses are escaped consistently when they reach the renderer.

renderer.php:342

Affected Code

$formattedfeedback .= '<div class="specific-feedback input-wrapper '.$colorclass.'">'.
    $studentletters[$index]. '</div>';

Suggested Fix

Although the JavaScript in Wordle mode restricts gap input to single A–Z letters, that is a client-side control only. A crafted POST can still place arbitrary characters into per-letter responses. Escape the value:

$formattedfeedback .= '<div class="specific-feedback input-wrapper '.$colorclass.'">'.
        s($studentletters[$index]).'</div>';

complianceLow

Stylesheet imports remote Google Fonts at render time

styles.css opens with an @import that pulls font CSS from fonts.googleapis.com on every page that includes the plugin's styles.

This sends the learner's IP address, User-Agent, and Referer (which discloses the Moodle URL the user is on) to Google on every page render. For installations that need to control external requests for privacy/GDPR reasons, this is a regression because Moodle core does not load fonts from fonts.googleapis.com. It also creates a runtime dependency on an external service: if the Google CDN is blocked or unreachable (corporate networks, sites behind strict CSPs, the Chinese internet, etc.) the stylesheet's downstream rules still apply but the chosen font face silently falls back.

Risk Assessment

Low risk. Not a security vulnerability per se. The concern is privacy/compliance (third-party data sharing without explicit opt-in, which is sensitive under GDPR-style regulations) and availability (external CDN is a single point of failure outside the operator's control).

Context

The stylesheet is automatically loaded by Moodle whenever the plugin is rendered (every page that displays a guessit question). The @import happens client-side, so this is not a server-side outbound HTTP call — but it does cause every learner's browser to contact Google.

styles.css:1

Identified Code

@import url('https://fonts.googleapis.com/css2?family=Roboto+Mono&family=Roboto:wght@300&display=swap');

Suggested Fix

Either bundle the font files locally inside pix/ or a dedicated fonts/ subdirectory and reference them through a local @font-face declaration, or fall back to system fonts. The existing fallback chain already lists 'Lucida Console', Courier, monospace, which would work without any external download.

complianceLow

Plugin's supported version range omits Moodle 5.0

version.php declares $plugin->supported = [401, 405], i.e. Moodle 4.1 through 4.5. Moodle 5.0 corresponds to branch 500, which falls outside this range. Because $plugin->requires is enforced (4.1+) but $plugin->supported is informational, installation does not fail — but the plugin advertises that it has not been validated against the version it is being installed on.

Risk Assessment

Low risk. Pure metadata mismatch — the plugin still installs and runs because requires is satisfied. The administrator-facing warning during install/upgrade is the only practical effect.

Context

Moodle 5.0's version.php reports $branch = '500'. Plugins maintain $plugin->supported as a hint to admins and to the plugins directory, telling them which Moodle versions the maintainer has actually tested.

version.php:36

Identified Code

$plugin->supported = [401, 405];

Suggested Fix

Update the upper bound to include the Moodle 5.x branches that have been tested, e.g. [401, 501]. Also consider extending the .github/workflows/moodle-ci.yml matrix to add MOODLE_500_STABLE so that the supported claim is backed by CI runs.

code qualityLow

is_complete_response() and get_validation_error() return null instead of documented types

Two methods that are part of the question_manually_gradable / question_with_responses contract return a bare return; (i.e. null):

is_complete_response() is documented to return bool and is consulted by behaviours (e.g. qbehaviour_adaptive::process_submit()) when deciding whether the response is complete enough to grade. Returning null is loosely falsy, so the question always looks incomplete to those callers.
get_validation_error() is documented to return a string for display when the question state is invalid.

The inline comment on is_complete_response() says it has been replaced by check_complete_answer() in the renderer. That replacement is only used inside the renderer; core behaviours still call the question class methods. The companion qbehaviour_guessit may compensate for this, but the qtype contract is still being violated.

Risk Assessment

Low risk. Functional behaviour appears to work because qbehaviour_guessit and the renderer maintain their own state, but the type-contract violation is fragile under future Moodle versions that may add stricter typing.

Context

Both methods are part of the abstract question_manually_gradable interface (see /moodle/question/type/questionbase.php) and are called by core behaviour classes during attempt processing. PHP's loose typing hides the type mismatch but the documented contract is not honoured.

question.php:155

Identified Code

public function is_complete_response(array $response) {
    return;
}

Suggested Fix

Return an explicit bool. A natural implementation is the same logic the renderer uses: every gap in $response must be a non-empty string.

public function is_complete_response(array $response): bool {
    foreach (array_keys($this->places) as $place) {
        if (!array_key_exists($this->field($place), $response)
                || trim((string) $response[$this->field($place)]) === '') {
            return false;
        }
    }
    return true;
}

question.php:165

Identified Code

public function get_validation_error(array $response) {
    return; // Not used by guessit.
}

Suggested Fix

Return an empty string (or a real validation message) so callers that concatenate the result into HTML do not silently coerce null:

public function get_validation_error(array $response): string {
    return '';
}

code qualityLow

htmlspecialchars_decode() applied to stored gap response

embedded_element() decodes HTML entities on the stored student response before using it. Responses are written to the question_attempts step data with PARAM_RAW_TRIMMED, which performs no encoding, so the decode is not undoing any earlier escape — it only affects the rare case where a response actually contains literal < etc. (e.g. a learner who typed entity references).

The practical effect is that <script> typed by a student becomes <script> before any downstream processing. In the input field's value attribute the browser still receives an attribute-encoded string thanks to html_writer, but anywhere this decoded value is concatenated into HTML without escaping it would now contain raw < and >. Combined with the unescaped concatenation in specific_feedback() (finding 1), this slightly broadens the payload surface.

Risk Assessment

Low risk. Standalone the decode is benign, but it is a code smell that masks the actual data-flow assumptions. Removing it improves clarity and reduces the post-fix surface for finding 1.

Context

get_last_qt_var() returns whatever was stored in the question_attempts step data. Since get_expected_data() declares the type as PARAM_RAW_TRIMMED, the engine does not encode the value at submit time — meaning there is nothing to decode at render time.

renderer.php:166

Identified Code

$studentanswer = $qa->get_last_qt_var($fieldname) ?? '';
$studentanswer = htmlspecialchars_decode($studentanswer);

Suggested Fix

Drop the decode. If a response did contain < it should be displayed as <, not decoded back to <:

$studentanswer = $qa->get_last_qt_var($fieldname) ?? '';

If the original intent was to handle a backup/restore artefact, narrow the call to that exact code path with a comment explaining why.

code qualityLow

Wordle navigation script uses document-wide selector for correct gaps

amd/src/wordlenavigation.js iterates each [id^="question-"] element on the page (per-question), but inside that loop it queries input.correct against the whole document rather than the current question scope. The finished flag is then computed by comparing the document-wide count of .correct inputs against the current question's gap count. With more than one Wordle question on the same page the count is incorrect: a question that is not yet finished can be classified as finished (or vice versa) depending on the state of the other questions.

Risk Assessment

Low risk. Functional/UX bug only — the worst case is that input fields are made pointer-events: none; when they should remain editable, or the cursor style is wrong. No security impact.

Context

Quizzes can render multiple questions per page (and several can use the guessit question type). The document-wide selector mixes data across them.

amd/src/wordlenavigation.js:36

Identified Code

document.querySelectorAll('[id^="question-"]').forEach(question => {
    const gaps = question.querySelectorAll('input[type="text"][name*="p"][class*="wordlegap"]');
    const correctGaps = document.querySelectorAll('input.correct');
    const finished = (correctGaps.length == gaps.length);

Suggested Fix

Scope the query to the current question:

const correctGaps = question.querySelectorAll('input.correct.wordlegap');

Also remember to rebuild amd/build/wordlenavigation.min.js (and its source map).

code qualityLow

update_question_guessit() carries unused parameters

update_question_guessit($question, $options, $context) declares three parameters but immediately overwrites $options from the database (so the passed value is ignored) and never references $context at all. The dead parameters are also passed by the caller, where the $context is computed for no reason.

Risk Assessment

Low risk. Maintenance/readability only.

Context

save_question_options() calls this method right after fetching the options from the DB, so by the time the helper runs the caller already has the current record — yet the helper re-fetches it.

questiontype.php:189

Identified Code

public function update_question_guessit($question, $options, $context) {
    global $DB;
    $options = $DB->get_record('question_guessit', ['question' => $question->id]);
    if (!$options) {
        $options = new stdClass();
        ...

Suggested Fix

Drop the unused parameters and the corresponding caller arguments:

public function update_question_guessit($question) {
    global $DB;
    $options = $DB->get_record('question_guessit', ['question' => $question->id]);
    ...
}

code qualityLow

Coding-style: uppercase PHP open tag and uppercase GLOBAL keyword

version.php uses <?PHP (uppercase open tag) and question.php::make_behaviour() uses GLOBAL $CFG; (uppercase keyword). Moodle's coding style requires <?php and global; these forms also trip Moodle Code Checker.

Risk Assessment

Low risk. Cosmetic only.

Context

Both files follow Moodle conventions otherwise; these are isolated case-sensitivity slips.

version.php:1

Identified Code

<?PHP

Suggested Fix

<?php

question.php:296

Identified Code

public function make_behaviour(question_attempt $qa, $preferredbehaviour) {
    GLOBAL $CFG;

Suggested Fix

public function make_behaviour(question_attempt $qa, $preferredbehaviour) {
    global $CFG;

best practiceInfo

No PHPUnit tests; only Behat coverage

The tests/ directory contains a helper.php (test fixtures) and three Behat features (basic flow, Wordle flow, backup/restore, import/export) but no PHPUnit unit tests. Question types typically benefit from PHPUnit coverage of grading, response handling, and import/export round-trips. The CI workflow runs moodle-plugin-ci phpunit --fail-on-warning, so PHPUnit tests would be exercised automatically if added.

Risk Assessment

Informational. Existing Behat coverage is solid; adding PHPUnit tests for grading and the markup helper would harden the plugin further.

Context

Behat covers the user-facing flow but is slower and does not catch many internal regressions (e.g. the is_complete_response() return-type issue noted above, or the markup-builder edge cases in get_markup_string()).

Additional AI Notes

File serving is correctly delegated. qtype_guessit_pluginfile() requires questionlib.php and forwards to core's question_pluginfile(), which performs the necessary access checks (login, edit-tab capability for exports, qubaid validation). No additional capability check is required in the plugin.

Database access is portable. All DB calls go through $DB with parameterised conditions; the schema in db/install.xml is well-formed; the plugin's own question_guessit table is the only one it writes to (apart from the standard question_answers core table that question types are expected to manage). No raw SQL strings are concatenated.

The Privacy provider is a null_provider with a language-string reason (privacy:null_reason). Given that the question type stores no personal data of its own — student responses are stored by the question engine, which has its own privacy provider — this is the correct choice.

make_behaviour() forces the guessit behaviour when the companion plugin is installed. The plugin's version.php declares qbehaviour_guessit as a hard dependency, so the fall-through to make_archetypal_behaviour() is effectively unreachable in normal installs. This is a deliberate design choice rather than a defect, but worth noting because the runtime feedback flow depends on qbehaviour_guessit's state-transition logic.

Form validation for Wordle mode in edit_guessit_form.php enforces uppercase A–Z and a max length of 8 server-side, which is good. Note that the byte-level strlen() is used; for ASCII-only inputs this is fine, but if the validation is ever extended to allow accented characters the check should switch to mb_strlen().

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