Openbook resource folder files block
block_openbook
A block for quiz pages that lists the files from a chosen Openbook resource folder (mod_openbook): teacher files, the student's own files and shared files, so students can open approved reference material during a quiz attempt.
There is no SQL injection, XSS, CSRF, code execution or unsafe file access. Queries are parameterised, the edit form's select element only accepts IDs from the course's own Openbook folders, filenames go through format_string() before rendering, and capability handling relies on core block management.
The worst problems are two medium gaps in deciding which files a student gets to see:
- Group isolation fails open. In separate-groups mode, a student who belongs to no group gets an active group of
0from core. The block then applies no filter, so that student sees every group's shared files. - The fallback lookup is too broad. It checks only
course_modules.visible. It is not limited to the current course and ignores the activity's access restrictions. After an import or restore into another course, the block keeps pointing at the original course's folder.
In both cases the leak is the listing: file names and pluginfile links of other users' shared files and teacher files. Whether those links actually download is decided by mod_openbook, which was not in the review environment. Both issues also depend on specific conditions (an ungrouped student, a restricted activity, or a teacher-run import) rather than something an attacker can choose. The remaining findings are small code-quality defects.
block_openbook is a small companion block for mod_openbook (about 800 lines of PHP plus one Mustache template). It sits on quiz pages and lists a folder's teacher files, the student's own files and shared files. There are no endpoints, AJAX calls or web services of its own. All work happens in get_content() and in the renderable's export_for_template().
The basics are sound. SQL is parameterised (get_in_or_equal() for the group filter), filenames and the folder name are escaped with format_string() before going into raw-HTML template variables, the edit form's select only accepts folders from the current course, and the Privacy API is a correct null provider.
Two medium issues deserve attention, and both concern which files a student is shown:
- Group filtering fails open. In separate-groups mode, a student who is not in any group gets active group
0from core. The block reads that as "no restriction" and lists every group's shared files. - The fallback lookup is too broad.
get_openbook_cm()checks onlycourse_modules.visible, with no course scoping and no availability check. Restore never clears a folder reference it cannot map. As a result, access restrictions on the folder are ignored, and an imported quiz can show another course's folder.
The remaining findings are low-severity code-quality issues in the file-listing code.
Findings
export_for_template() applies the group restriction only when groups_get_activity_group() returns a non-empty value. In core, _group_verify_activegroup() sets the active group to 0 for a user in separate groups mode who has no allowed groups, and core notes that module authors must handle this case themselves. Here 0 makes $grouprestricted false, $andgroupmembers stays empty, and the shared-file and teacher-file queries run with no group filter at all. An ungrouped student therefore sees every group's shared files, which the group-mode Behat scenario explicitly expects to be hidden. A related gap: groups_get_user_groups() returns the user's groups across all groupings, not just the activity's groupingid, so membership in unrelated groups widens the visible set.
Medium risk. Group-scoped shared files and teacher files (names plus pluginfile links) are disclosed across group boundaries. The student does nothing special; they just have to be ungrouped, which is common when groups are created late or enrolment sync lags. Whether the links download is up to mod_openbook's pluginfile handler, which was not available for review.
The block builds a f.userid IN (...) clause from the members of the viewer's groups and appends it to the teacher-file, shared-file and own-file queries. Users with moodle/site:accessallgroups skip the filter on purpose.
Set an Openbook folder to separate groups with shared files and automatic approvals. Groups A and B each upload a shared file. Enrol a third student in no group and open the quiz attempt page: the block lists both groups' files.
$groupwiseactivity = groups_get_activity_group($openbookcm);
$grouprestricted = !empty($groupwiseactivity);
Decide from the group mode, not the active group, and fail closed:
$groupmode = groups_get_activity_groupmode($openbookcm);
$grouprestricted = ($groupmode == SEPARATEGROUPS);
Then, if $grouprestricted && !$canviewallgroups and the user has no groups, show only the user's own files (e.g. $andgroupmembers = ' AND f.userid = :onlyme') instead of dropping the filter.
$currentusergroups = groups_get_user_groups($openbookcm->course, $USER->id);
foreach ($currentusergroups as $coursegroup) {
Iterate only $currentusergroups[$openbookcm->groupingid] (grouping 0 is the key for all groups) so the member set matches the activity's grouping.
When the configured folder is not visible to the current user in get_fast_modinfo(), get_openbook_cm() falls back to a raw query that returns cm.visible AS uservisible for any openbook instance, in any course. This pattern comes from core block_glossary_random, but there the fallback is limited to globalglossary = 1. Here it has no limit. As a result, a folder in the same course that is visible but blocked for this user by access restrictions (dates, group or grouping, completion) is still listed, because the stricter cm_info::uservisible result is overwritten. In addition, after_restore() keeps the old instance ID when no mapping exists (for example, importing only the quiz into another course). Core's glossary block clears that ID in the same situation. The block then keeps showing the source course's folder in the new course.
Medium risk. The disclosure is limited to file names, the folder name and pluginfile links. It needs either a teacher-set access restriction or a teacher-run import/restore. Teachers who restrict a folder (for example, opening it only at exam time) would reasonably expect the block to respect that restriction.
get_content() renders the listing whenever get_openbook_cm() returns a record with a truthy uservisible. In the fallback query that value is just course_modules.visible, so availability conditions and course membership are never consulted.
- In course A, add the block to a quiz pointing at folder X.
- In course B, use Import to bring in only the quiz.
- A course B student opening the quiz sees folder X's name, teacher files and shared files. The group filter is computed against course A, where the student has no groups, so it is also empty.
$cm = $DB->get_record_sql(
"SELECT cm.id, cm.visible AS uservisible
FROM {course_modules} cm
JOIN {modules} md ON md.id = cm.module
JOIN {openbook} o ON o.id = cm.instance
WHERE o.id = :instance AND md.name = :modulename",
['instance' => $this->config->openbook, 'modulename' => 'openbook']
);
Rely on cm_info for same-course folders and do not fall back:
$modinfo = get_fast_modinfo($this->page->course);
if (isset($modinfo->instances['openbook'][$this->config->openbook])) {
return $this->openbookcm = $modinfo->instances['openbook'][$this->config->openbook];
}
return null;
If a cross-course fallback is genuinely needed, add AND cm.course = :courseid.
if ($mapping = restore_dbops::get_backup_ids_record($this->get_restoreid(), 'openbook', $config->openbook)) {
$config->openbook = $mapping->newitemid;
// Encode and save the config.
$configdata = base64_encode(serialize($config));
$DB->set_field('block_instances', 'configdata', $configdata, ['id' => $blockid]);
}
Add an else branch like core's glossary block: on the same site keep the ID only if $DB->record_exists('openbook', ['id' => $config->openbook, 'course' => $this->get_courseid()]), otherwise unset($config->openbook). Save the config in both branches.
The shared-file and own-file queries select every column from both {files} and {openbook_file}. Both tables have at least an id column. Columns with the same name overwrite each other in the record object, and which value wins depends on the database driver, so properties such as $f->id (and possibly userid/filename) are ambiguous. The code reads contextid, itemid, filepath, filename, mimetype, component and filearea, so a future openbook_file column with one of those names would silently break the links.
Low risk. It does not break today because only {files} columns are read, but it is fragile and makes queries heavier than they need to be.
get_records_sql() uses the first column as the array key and builds each record from an associative row, so a later column with the same name replaces the earlier one.
$sql = "SELECT f.*, of.*
FROM {files} f
JOIN {openbook_file} of ON of.fileid = f.id
List only the columns you use:
$sql = "SELECT f.id, f.contextid, f.component, f.filearea, f.itemid,
f.filepath, f.filename, f.mimetype
FROM {files} f
JOIN {openbook_file} of ON of.fileid = f.id
$sql = "SELECT f.*, of.*
FROM {files} f
JOIN {openbook_file} of ON of.fileid = f.id
Apply the same explicit column list here.
In all three listings, the PDF.js branch builds the pluginfile URL with filepath '/', while the non-PDF branch correctly passes $f->filepath. A PDF stored in a subfolder of the file area gets a URL to a non-existent file at the root, and the viewer fails to load it. The same block of about 40 lines is also copied three times, which is how the two branches drifted apart.
Low risk. This is a functional bug only: PDFs in subfolders give broken links during a quiz. It has no security impact.
When the folder enables openpdffilesinpdfjs, PDF files are linked through mod/openbook/<viewer>/web/viewer.html?file=<pluginfile url>. Other files are linked directly.
$pluginfilerawurl = \moodle_url::make_pluginfile_url(
$f->contextid,
'mod_openbook',
'commonteacherfiles',
$f->itemid,
'/',
$f->filename,
true
);
Pass $f->filepath instead of '/', and move the URL/link building into one private helper (e.g. build_file_link(stdClass $f, bool $pdfjs, string $viewer): string) shared by all three listings.
$pluginfilerawurl = \moodle_url::make_pluginfile_url(
$f->contextid,
'mod_openbook',
'attachment',
$f->itemid,
'/',
$f->filename,
true
);
Use $f->filepath.
$pluginfilerawurl = \moodle_url::make_pluginfile_url(
$f->contextid,
'mod_openbook',
'attachment',
$f->itemid,
'/',
$f->filename,
true
);
Use $f->filepath.
Tests: tests/lib_test.php is a scaffold placeholder. It only checks that the plugin is installed and still carries the TODO change the 'covers' tag comment with @covers ::get_config. Real PHPUnit coverage of export_for_template() (group filtering, approvals, personal files) would have caught the fail-open group behaviour; the Behat suite currently covers grouped users only.
Database write during render: get_openbook_cm() calls instance_config_commit() from inside get_content() when the configured folder no longer exists. It does so twice in a row, because the else if branch and the following if both fire. Core's glossary block uses the same pattern, so this is not a defect in itself, but the duplicate write can be removed.
Untested dependency: File downloads go through mod_openbook's pluginfile handler and its bundled PDF.js viewer. Neither was in the review environment, so whether the links listed in the findings above actually serve file contents to the wrong users could not be checked.