Openbook resource folder files block
block_openbook
A Moodle block that surfaces the files of an Openbook resource-folder activity (mod_openbook) directly inside a quiz. It lists teacher files, shared student files and the viewing user's own files, optionally rendering PDFs through the activity's bundled PDF.js viewer, and links back to the Openbook activity. The block is restricted to quiz pages and hard-depends on mod_openbook.
The plugin's security fundamentals are sound: all SQL is fully parameterised (dynamic clauses come from $DB->get_in_or_equal() with named placeholders), files are exposed only through moodle_url::make_pluginfile_url() so the actual download stays gated by mod_openbook's pluginfile callback, activity/owner names are emitted with format_string(), the teacher-only configuration warning is gated behind mod/quiz:manage and block/openbook:addinstance, and the Privacy API null_provider is appropriate because the block stores no personal data of its own. No SQL injection, exploitable XSS, authentication/authorisation bypass, code execution, raw DB/filesystem/HTTP access, or hardcoded secrets were found.
The grade is held back from the A range by a correctness defect and a debug leak in the recently changed group-handling code. get_openbook_cm() can return a cm_info (the common path) or null (deleted activity), but main::__construct() types the parameter as stdClass; either value raises an uncaught TypeError, and because the block manager does not wrap get_content() in a try/catch this aborts rendering of the whole page. A leftover var_dump() prints group members' user IDs into the page for students in separate-groups Openbooks. There is also a latent (currently non-exploitable) unescaped-filename path in the PDF branch, plus minor dead code and robustness gaps. These are a small set of real issues against an otherwise correctly structured plugin, and the presence of obvious debug output alongside a type defect suggests the current revision was not validated through its own CI.
block_openbook is a small, well-structured block whose job is to show the files held in an mod_openbook resource-folder activity from within a quiz. Security-sensitive operations are handled correctly throughout:
- SQL is parameterised; the only dynamically concatenated fragment (
$andgroupmembers) is built from$DB->get_in_or_equal(..., SQL_PARAMS_NAMED), and the approval clauses are static literals. - File access is delegated to
moodle_url::make_pluginfile_url(), so listing a file does not grant access —mod_openbook'spluginfilecallback still enforces download permission. - Output of activity/folder names uses
format_string()(which escapes by default), and the teacher-only "block not shown" hint is gated behindmod/quiz:manage+block/openbook:addinstance. - The Privacy API
null_provideris correct for a block that persists only its own instance configuration.
The issues found are concentrated in the freshly modified group-handling code (CHANGES.md: "v5.2-r3 — Fixed handling for groups"):
- Uncaught
TypeError(medium).get_openbook_cm()is documented and implemented to returnnull | cm_info | stdClass, butmain::__construct()requiresstdClass.cm_infois not astdClass, andnullis non-nullable, so the normal render path (a visible Openbook in the current course returns acm_info) and the deleted-activity path (null) both throw, aborting the page. - Leftover
var_dump()(low). Debug output that prints group members' user IDs into the page for students in separate-groups Openbooks. - Inconsistent filename escaping (low, not exploitable). The PDF.js branch passes the raw filename to
html_writer::link()(which does not escape) and the template renders it unescaped; the non-PDF branch correctly usesformat_string(). This is not exploitable because the File API stores filenames cleaned withPARAM_FILE, which strips< > " ' &. - Minor robustness/dead-code items (an
IGNORE_MISSINGrecord dereferenced without a guard, an unused import, unusedglobal $DB).
Overall this is a clean plugin from a security standpoint; the deductions are correctness/quality issues that should be straightforward to fix.
Findings
block_openbook::get_content() constructs the main renderable with the result of get_openbook_cm():
The method's own PHPDoc declares its return type as null|cm_info|stdClass, and its implementation honours that:
- When the configured Openbook is present in the current course's
modinfoand visible, it returns acm_infoobject (the early return from theget_fast_modinfo()branch). This is the normal case for a correctly configured block. - When the Openbook record cannot be found (e.g. the activity was deleted), it returns
null.
However, main::__construct() types that parameter as protected stdClass $openbookcm. cm_info is declared as class cm_info implements IteratorAggregate — it does not extend stdClass — and the parameter is not nullable. Passing either a cm_info or null therefore throws a TypeError.
Because Moodle's block manager calls get_content() (via formatted_contents() / get_content_for_output()) without a surrounding try/catch, the TypeError is not contained to the block — it aborts rendering of the entire page that hosts the block.
This is a correctness/availability defect rather than a security vulnerability: it is not attacker-controlled, but it breaks the block's primary function and can take down the whole quiz page for every viewer.
Medium risk. This is a functional/availability defect, not an exploitable security hole — there is no attacker and no privilege gain. However, the impact is significant: an uncaught TypeError in get_content() is not contained by the block manager, so it aborts the whole page render for every user who loads a quiz carrying a correctly configured block (or one whose Openbook was later deleted). The blast radius is limited to pages where the block is placed, and the fix is mechanical (correct the parameter type and guard the null case). The co-located leftover var_dump() suggests this revision of the group-handling code was not exercised through the project's own CI before being tagged.
The block is only rendered on quiz pages (applicable_formats()), where $PAGE->course is the real course. get_fast_modinfo($this->page->course) therefore contains the Openbook activity, so the isset($modinfo->instances['openbook'][...]) branch matches and (for a visible activity) returns a cm_info. The fallback SQL branch — which returns a minimal stdClass — is only reached when the activity is not in the current course's modinfo or is not uservisible, i.e. the less common path. As a result the stdClass-only type hint is wrong for the dominant code path.
public function __construct(
/** @var int $openbookid the openbook resource folder id. */
protected int $openbookid,
/** @var stdClass $openbookcm the openbook course module. */
protected stdClass $openbookcm,
/** @var stdClass $quiz the quiz the block is displayed in. */
protected stdClass $quiz,
/** @var string $pagetypepattern the block pagetypepattern. */
protected string $pagetypepattern,
) {
}
Widen the parameter type to match what get_openbook_cm() actually returns, and import cm_info:
use cm_info;
// ...
protected cm_info|stdClass $openbookcm,
Note that export_for_template() only ever reads $this->openbookcm->id (and re-fetches the cm via get_coursemodule_from_instance() anyway), so accepting cm_info is safe.
if (isset($this->config->openbook)) {
$renderable = new \block_openbook\output\main(
$this->config->openbook,
$this->get_openbook_cm(),
$this->get_owning_activity(),
$this->instance->pagetypepattern,
);
Guard against the null return before constructing the renderable, so a deleted Openbook degrades gracefully instead of throwing:
$openbookcm = $this->get_openbook_cm();
if (isset($this->config->openbook) && $openbookcm !== null) {
$renderable = new \block_openbook\output\main(
$this->config->openbook,
$openbookcm,
$this->get_owning_activity(),
$this->instance->pagetypepattern,
);
if (isset($modinfo->instances['openbook'][$this->config->openbook])) {
$this->openbookcm = $modinfo->instances['openbook'][$this->config->openbook];
if ($this->openbookcm->uservisible) {
// The openbook is in the same course and is already visible to the current user.
return $this->openbookcm;
}
}
This early return hands back a cm_info. Either keep it and widen the constructor type (preferred), or normalise to a stdClass here before returning.
export_for_template() contains a var_dump($allmembers) call inside the group-restriction branch. $allmembers is an array of stdClass objects (each carrying a user.id) for every member of the current user's group(s). var_dump() writes directly to the output buffer, so this raw dump is emitted into the rendered HTML of the quiz page.
This is debug code that was left in production (the change log notes the surrounding group handling was reworked in v5.2-r3). It has two effects:
- It breaks the page layout, printing a raw
array(...) { object(stdClass)#NN ... }blob. - It discloses internal user IDs of the viewer's group members into the page source.
Low risk. The disclosed data — numeric user IDs of the viewer's own group members — is low sensitivity; a student can usually see their group mates through the participants list and group views anyway. The substantive problem is that obvious debug output is shipped to end users, which both degrades the UI and signals untested code. No cross-group leakage occurs because $allmembers is scoped to the current user's groups.
The dump only runs when $grouprestricted && !$canviewallgroups is true — i.e. for a user without moodle/site:accessallgroups viewing a separate-groups Openbook. In practice that is students (and any teacher lacking the accessallgroups capability).
var_dump($allmembers);
Delete the line. It serves no purpose in production and is flagged by Moodle's code checker.
When openpdffilesinpdfjs is on and the file is a PDF, the file objects store the raw $f->filename ('filename' => $f->filename). The non-PDF branch, by contrast, stores format_string($f->filename). All three sections (teacher, shared, own files) then build links with html_writer::link($f->url, $f->filename, ...), and the template prints the resulting list through a triple-brace ({{{...}}}) Mustache variable.
Two facts combine here:
html_writer::link()delegates tohtml_writer::tag('a', $text, ...), which concatenates$textwithout escaping it.- The template emits the assembled HTML with
{{{teacherfiles}}}/{{{sharedfiles}}}/{{{ownfiles}}}, which does not auto-escape.
So the PDF branch passes an unescaped filename all the way to the browser. This is the classic shape of a stored-XSS sink. It is not exploitable in practice, because filenames in the {files} table are written by the File API, which cleans every filename with clean_param($filename, PARAM_FILE) — and PARAM_FILE strips < > " ' & \ ` and control characters. A filename therefore cannot contain HTML metacharacters. The issue is the inconsistency and the reliance on an upstream invariant rather than escaping at the point of output.
Low risk. Not exploitable with the current code base: the File API guarantees filenames are free of < > " ' &, so no HTML/script payload can survive into the {files} table. It is reported because (a) it is inconsistent with the sibling non-PDF branch, which does escape, and (b) it relies on a remote invariant instead of escaping at output — if any future code path populated these structures from a less-constrained source, this would become a genuine stored-XSS vector. Escaping at the point of output is the robust fix.
The data path is: a file uploaded to the Openbook activity → stored via the File API (filename cleaned with PARAM_FILE) → selected by direct SQL in export_for_template() → link text built with the unescaping html_writer::link() → emitted through an unescaped {{{...}}} template variable. The only thing preventing script injection is the upstream PARAM_FILE sanitisation; nothing at the output layer escapes the value in the PDF branch.
$teacherfiles[] = (object) [
'filename' => $f->filename,
'filepath' => $f->filepath,
'url' => $pdfjsurl,
];
Escape consistently in the PDF branch as the non-PDF branch already does:
$teacherfiles[] = (object) [
'filename' => format_string($f->filename),
'filepath' => $f->filepath,
'url' => $pdfjsurl,
];
The same change applies to the shared-files (around line 253) and own-files (around line 336) PDF branches. Using format_string() (or s()) at output time removes the dependence on the File API's PARAM_FILE cleaning.
$sharedfiles[] = (object) [
'filename' => $f->filename,
'filepath' => $f->filepath,
'url' => $pdfjsurl,
];
Wrap the filename in format_string() as above.
$ownfiles[] = (object) [
'filename' => $f->filename,
'filepath' => $f->filepath,
'url' => $pdfjsurl,
];
Wrap the filename in format_string() as above.
export_for_template() loads the Openbook row with IGNORE_MISSING and then immediately reads properties off the result on the next lines. If the record does not exist (get_record() returns false), reading $openbook->openpdffilesinpdfjs, $openbook->uselegacyviewer, $openbook->name, etc. produces PHP "Attempt to read property on bool" warnings and feeds null flags into the subsequent queries.
In the normal flow the activity is validated by get_openbook_cm() before this code runs, so the record almost always exists; this is a robustness gap rather than a live bug, but it should either use MUST_EXIST (and let the caller handle the exception) or guard the result.
Low risk. No security impact and unlikely to trigger in practice because the course module is validated earlier in the request. The worst observed outcome is developer-level warnings and a degraded render if the openbook row is missing while a course module still references it (an inconsistent-data edge case).
This runs at the top of export_for_template(), which is reached only after get_content() has constructed the renderable. The activity id comes from the block's stored configuration.
$openbook = $DB->get_record('openbook', ['id' => $this->openbookid], '*', IGNORE_MISSING);
$openpdffilesinpdfjs = $openbook->openpdffilesinpdfjs;
$uselegacyviewer = $openbook->uselegacyviewer;
Either fail fast with MUST_EXIST:
$openbook = $DB->get_record('openbook', ['id' => $this->openbookid], '*', MUST_EXIST);
or guard the missing case explicitly and return early before dereferencing.
Minor housekeeping items that add noise and can mislead readers:
classes/output/main.phpimportsuse core_competency\url;but never uses it (the code references\moodle_urlfully qualified). Importing a symbol from the unrelatedcore_competencysubsystem is also misleading.block_openbook::get_content()declaresglobal $DB;but never uses$DB.block_openbook::get_owning_activity()declaresglobal $DB;but never uses$DB(it usescontext::instance_by_id()andget_coursemodule_from_id()).
Informational. No functional or security impact; removing the dead code improves readability and would satisfy Moodle's code checker.
These are cosmetic findings discovered while reading the block and renderable classes.
use core_competency\url;
Remove the unused import.
global $DB;
Remove the unused global $DB; from get_content() (and likewise from get_owning_activity() at line 91).
Security fundamentals are solid. All SQL uses placeholders; the only concatenated clause ($andgroupmembers) is produced by $DB->get_in_or_equal(..., SQL_PARAMS_NAMED, 'user'), and the approval fragments are static literals — no injection surface. Files are surfaced via moodle_url::make_pluginfile_url() (with $forcedownload=true, no embedded token), so the actual download stays gated by mod_openbook's pluginfile callback; listing a file does not bypass access control. The teacher configuration hint is correctly gated behind mod/quiz:manage and block/openbook:addinstance.
Privacy API is appropriate. The block persists only its own instance configuration (title, target Openbook id), so the null_provider implementation is the correct choice; no GDPR provider work is required.
No bundled third-party code. There is no thirdpartylibs.xml, and correctly so — the plugin ships no vendored libraries (the PDF.js viewer it links to lives in the mod_openbook dependency, not in this block). No action needed.
Code duplication. The PDF-vs-non-PDF link-building logic is repeated almost verbatim three times (teacher files, shared files, own files). Extracting a private helper such as build_file_links(array $records, string $filearea): array would remove ~90 lines of duplication and make the escaping consistent in one place, which also addresses finding 3.
Architectural observation. export_for_template() re-implements mod_openbook's file-visibility rules (personal vs shared, teacher/student approval, separate-groups restriction) with direct cross-component SQL against {files}, {openbook} and {openbook_file}. The verified behaviour matches the behat expectations, and actual downloads remain protected by the activity's pluginfile callback, so this is a maintainability concern rather than an access-control flaw — but if mod_openbook ever changes its visibility logic, this query will silently drift out of sync. Exposing a shared API from mod_openbook (returning the visible files for a user/group) would be more robust than duplicating the query here.
Minor redundancy. get_content() instantiates $this->content = new stdClass() twice (once before the isset check and again inside it), and instance_config_commit() unset()s openbook/courseid immediately before persisting — an inherited pattern that works but is confusing to read. Neither affects behaviour.
Testing signal. The combination of a leftover var_dump() (which Moodle's phpcs step flags) and the cm_info/stdClass TypeError on the primary render path suggests the current v5.2-r3 revision likely does not pass the project's own moodle-plugin-ci workflow as committed; the PHPUnit test only covers can_block_be_added() and does not exercise get_content().