List course files
local_listcoursefiles
- #1Bulk ZIP download and file listing bypass per-file access controls, relying only on a course-level capability
- #2Bulk license change writes directly to the core `files` table instead of using the File API
- #3Mime-type filter SQL is assembled by string concatenation
- #4Several `$DB->get_record*()` results are dereferenced without null checks
- #5Direct instantiation of `file_storage` and `stored_file` instead of the File API accessor
A local plugin that lets privileged users (editing teachers and managers by default) view a paginated list of every file in a course, see metadata (size, type, uploader, license, whether the file is embedded/used), change the license of selected files in bulk, and download selected files as a ZIP archive.
The plugin demonstrates strong security fundamentals. The single entry point (index.php) correctly calls require_login(), enforces a dedicated require_capability() check, and gates both state-changing/sensitive actions (change_license, bulk download) behind require_sesskey() and per-action capability checks. All database access uses parameterized queries via the $DB API; the one place that builds SQL by string concatenation (get_sql_mimetype()) only ever concatenates hardcoded internal constants, so it carries no injection risk. File downloads use the File API correctly, and check_files_context() ties every license change and download to the current course context, preventing cross-course access. Output is appropriately escaped: filenames are constrained by PARAM_FILE, license names are pre-formatted by core's format_string(), and URLs are emitted through moodle_url.
No critical, high, or medium issues were identified. The findings are all low or informational: the bulk ZIP download bypasses Moodle's per-file pluginfile.php access callbacks and relies solely on a course-level capability (safe in the default teacher/manager assignment, but a consideration if the capability is granted to lower-privileged roles); a bulk UPDATE on the core files table bypasses stored_file::set_license(); several $DB->get_record*() results are dereferenced without null checks (a reliability risk for orphaned file records); and there are minor best-practice items (direct file_storage/stored_file instantiation, no automated tests). A correctly implemented privacy provider and clean capability model round out a well-maintained codebase.
local_listcoursefiles is a small, mature, and generally well-engineered plugin. It exposes one page (index.php) that lists all files within a course context and offers two privileged actions (bulk license change and ZIP download).
Security posture is strong:
require_login(),require_capability('local/listcoursefiles:view', ...), andrequire_sesskey()are all present and correctly placed.- Capabilities default to
editingteacherandmanageronly, and are declared withRISK_PERSONAL. - Every DB query uses placeholders; user-supplied request parameters are read through
required_param/optional_paramwith appropriatePARAM_*types. check_files_context()confines all file operations to the current course's context subtree, blocking cross-course access and access to user-context files.- Output is safe: I verified against core that
PARAM_FILEstrips HTML-significant characters from stored filenames and that custom license names are passed throughformat_string()before reaching the unescaped{{{filelicense}}}placeholder.
Findings are limited to low and informational severity. The most security-relevant observation is that the bulk ZIP download reads file content directly through the File API rather than via pluginfile.php, so it does not re-apply the per-module/per-file access callbacks that Moodle normally enforces on download. In the default capability assignment this grants no access beyond what teachers/managers already have, but it means the capability must not be granted to untrusted roles. The remaining items are code-quality and robustness improvements.
No third-party libraries are bundled, the privacy null_provider is appropriate (the plugin stores no data in its own tables), and a CI workflow is configured.
Findings
The file listing and especially the bulk ZIP download enumerate and serve every file whose context lies within the course context subtree, gated only by the course-level capabilities local/listcoursefiles:view and local/listcoursefiles:download.
Normally, downloading a course file goes through pluginfile.php, which dispatches to the owning component's *_pluginfile() callback (for example mod_assign/assignsubmission_file), and that callback enforces fine-grained access rules (e.g. only graders or the submitting student may fetch a submission file, availability restrictions, group separation, etc.).
The plugin's download_files() bypasses pluginfile.php entirely: it loads file content directly via the File API (get_file_instance()) and zips it, performing only a context-path check (check_files_context()) plus the single course-level capability check in index.php. The listing similarly discloses metadata (filename, uploader name, size, license, used-status) for all files in the subtree.
In the default configuration the capabilities are assigned only to editingteacher and manager, who already have broad access to course content, so this does not constitute privilege escalation. The risk materialises only if an administrator grants these capabilities to a lower-privileged role (e.g. via an override to students or a custom auditor role): such a user would then be able to read every file in the course — including other students' assignment submissions — without the per-file checks that would normally apply.
Low risk. In the shipped configuration the only holders of these capabilities are editingteacher and manager, who can already reach this content through normal course tools, so there is no privilege escalation by default. check_files_context() confines operations to a single course and blocks user-context files, a meaningful mitigation. The finding is primarily a defense-in-depth and deployment-guidance concern: because the plugin centralises access to all course files and bypasses per-file pluginfile.php controls, granting these RISK_PERSONAL capabilities to students or other low-trust roles would expose private submissions and other restricted files. It is not exploitable by unauthenticated users, guests, or students under the default access rules.
index.php performs one capability check at the course context, then course_files::download_files() (classes/course_files.php:335) queries the selected file IDs, runs check_files_context() to confirm they sit under the course context path, and zips their content. There is no second authorization layer comparable to the pluginfile.php callbacks. check_files_context() does correctly prevent cross-course access and access to user-context files (drafts, private files), which limits the blast radius to the current course.
As an editing teacher in course X (default assignment):
- Visit
/local/listcoursefiles/index.php?courseid=X&component=all. - The listing now includes assignment submission files (
assignsubmission_file, etc.). - Tick any of them and submit the form with
action=download(a validsesskeyis included by the page). - The server returns a ZIP built directly from file content via the File API, without invoking the assignment's
pluginfileaccess checks.
The selected file IDs are taken from the POSTed file[] array and validated only by check_files_context(), so a client may also submit IDs of files not shown in the current filter, as long as they belong to the course context. In the default role assignment this overlaps existing teacher access; the concern is the absence of per-file checks should the capability be granted more broadly.
$checkedfiles = $this->check_files_context($res);
$fs = get_file_storage();
$filesforzipping = [];
foreach ($checkedfiles as $file) {
$fname = $this->download_get_unique_file_name($file->filename, $filesforzipping);
$filesforzipping[$fname] = $fs->get_file_instance($file);
}
Primary mitigation is operational: document clearly that local/listcoursefiles:view and :download must only be granted to roles trusted with full access to all course content (they are correctly RISK_PERSONAL and teacher/manager-only by default).
Defense in depth (optional): for higher assurance, consider validating each selected file against the owning component before zipping — for example by routing access decisions through the relevant module's access logic rather than serving any file found under the course context. At minimum, restrict the listing's default to exclude private submission areas (already done via all_wo_submissions) and avoid widening it without an access check.
require_login($courseid);
require_capability('local/listcoursefiles:view', $context);
$changelicenseallowed = has_capability('local/listcoursefiles:change_license', $context);
$downloadallowed = has_capability('local/listcoursefiles:download', $context);
set_files_license() changes the license of selected files with a raw bulk UPDATE against the core {files} table rather than going through the File API.
Moodle provides stored_file::set_license() (verified in core at lib/filestorage/stored_file.php), which updates the file record through stored_file::update(). Writing the license column directly:
- bypasses any bookkeeping
update()performs (e.g. refreshingtimemodified), - couples the plugin to the core schema, and
- skips the File API abstraction layer.
The statement itself is safe — the license value is validated against license_manager::get_active_licenses(), the file IDs are confined to the course context via check_files_context(), and the query is fully parameterized — so this is a maintainability/API-correctness issue, not a security one.
Low risk. There is no injection or authorization weakness — inputs are validated and parameterized, and the operation is scoped to the course context. The only downsides are bypassing File API side effects (such as timemodified updates) and tighter coupling to the core schema. Impact is limited to internal data hygiene; no user can exploit it for unauthorized access.
set_files_license() (classes/course_files.php:260) first validates the license shortname, caps the number of files at MAX_FILES, re-queries the selected IDs joined to their context, and filters them through check_files_context() before issuing the update inside a delegated transaction and firing a license_changed event per file. The data flow is well guarded; only the write mechanism deviates from the File API.
list($sqlin, $paramfids) = $DB->get_in_or_equal($checkedfileids, SQL_PARAMS_QM);
$transaction = $DB->start_delegated_transaction();
$sql = "UPDATE {files} SET license = ? WHERE id $sqlin";
$DB->execute($sql, array_merge([$license], $paramfids));
Load each validated file and use the File API setter, for example:
$fs = get_file_storage();
foreach ($checkedfileids as $fid) {
$storedfile = $fs->get_file_by_id($fid);
if ($storedfile) {
$storedfile->set_license($license);
}
}
If the bulk UPDATE is retained for performance over many files, add a short comment explaining the deliberate trade-off and that the File API equivalent is stored_file::set_license().
get_sql_mimetype() builds a chain of LIKE/NOT LIKE predicates by concatenating mime-type patterns directly into the SQL string rather than using bound parameters.
Crucially, this is not exploitable: the concatenated values come exclusively from the hardcoded mimetypes::$mimetypes static array, and the user-supplied filetype parameter is used only as an array-key lookup (and is validated with isset(...)) — it is never itself concatenated into the query. So no request input reaches the SQL string.
The concern is purely defensive/maintainability: building SQL via string concatenation is fragile, and if a future change ever allowed externally-influenced values into this helper, it would become an injection vector. The LIKE patterns intentionally contain % wildcards, which is why simple placeholders were likely avoided.
Low risk. As written there is no SQL injection because every concatenated token is a compile-time constant defined within the plugin and no request data is interpolated. The rating reflects defense-in-depth and maintainability rather than a present vulnerability; a maintainer who later feeds user-influenced data into this helper could inadvertently introduce an injection.
get_file_list() (classes/course_files.php:125) calls get_sql_mimetype() only after confirming the requested filter type exists as a key in the static mime-type map. The function then concatenates the values (e.g. image/%, text/%) for that key into the WHERE clause. All other dynamic values in the query (path, cid, component) are correctly passed as bound parameters.
if ($in) {
$first = "(f.mimetype LIKE '";
$glue = "' OR f.mimetype LIKE '";
} else {
$first = "(f.mimetype NOT LIKE '";
$glue = "' AND f.mimetype NOT LIKE '";
}
return $first . implode($glue, $list) . "')";
Generate one placeholder per pattern and bind the values, for example by building f.mimetype LIKE ? fragments joined with OR/AND and returning the params alongside the SQL fragment so the caller can merge them into $params. This keeps behaviour identical while removing string-built SQL. If the current approach is kept, add a comment documenting that all concatenated values are fixed internal constants.
In multiple component classes, the result of $DB->get_record() / $DB->get_record_sql() is used immediately without checking for false. These methods return false when no matching row exists, and in PHP 8 reading a property on false raises an Error.
For a file that has become orphaned (the file row still exists but the related activity instance, context, or content-bank record has been deleted — a situation that does occur after failed deletions, restores, or content-bank cleanup), rendering the listing will call these methods, get false, and fatal. Because the listing builds a course_file object for every file in the page, a single orphaned file can throw a 500 and break the entire page for that course.
Low risk. No security impact — this is a reliability/robustness defect. It requires an orphaned or inconsistent file record to trigger, which is uncommon but does happen in real installations. When it does occur, the consequence is a fatal error that disables the entire file listing for the affected course until the orphan is removed.
Each file in the page is turned into a course_file (or subclass) by course_file::create(), and the constructor eagerly calls get_displayed_filename(), get_component_url(), get_edit_url(), and is_file_used(). Note that the helper is_embedded_file_used() already defends against a false/null record, so the is_file_used() paths that route through it (book chapters, forum posts, glossary entries, page/feedback content, course sections/summary) are safe; the unguarded dereferences are concentrated in the URL builders, contentbank::get_displayed_filename(), and mod_data::is_file_used().
$cb = $DB->get_record('contentbank_content', ['id' => $this->file->itemid]);
return $cb->name;
$cb = $DB->get_record('contentbank_content', ['id' => $this->file->itemid]);
return $cb ? $cb->name : $this->file->filename;
$mod = $DB->get_record_sql($sql, [$this->file->contextid]);
return new \moodle_url('/course/modedit.php?', ['update' => $mod->id]);
Guard the result before dereferencing, e.g. if (!$mod) { return null; } then build the URL.
$mod = $DB->get_record_sql($sql, [$this->file->contextid]);
return new \moodle_url('/course/view.php', ['id' => $mod->course, 'sectionid' => $mod->section]);
Check $mod for false before accessing $mod->course/$mod->section; return null (or fall back to the course URL) when the record is missing.
$data = $DB->get_record_sql($sql, [$this->file->itemid]);
$path = '@@PLUGINFILE@@/' . rawurlencode($this->file->filename);
return $data->type !== 'textarea' || false !== strpos($data->content, $path);
Return null (unknown) when $data is false, mirroring how is_embedded_file_used() already guards its $record argument.
$ctx = $DB->get_record('context', ['id' => $this->file->contextid]);
return new \moodle_url('/mod/book/edit.php', ['cmid' => $ctx->instanceid, 'id' => $this->file->itemid]);
Guard $ctx before accessing $ctx->instanceid. The same pattern recurs in get_edit_url() of mod_assign, mod_feedback, mod_page, mod_resource, and mod_h5pactivity, and should be fixed consistently across all of them.
contentbank::is_file_used() creates the file storage and stored-file objects manually with new \file_storage() and new \stored_file($fs, $this->file), rather than using the canonical get_file_storage() singleton and loading a real stored_file via the API (e.g. get_file_by_id() / get_file_instance()).
Constructing a stored_file from $this->file is fragile because $this->file is the joined result row from the listing query (it carries extra columns such as contextlevel, instanceid, path, and user name fields), not a clean file record produced by the File API. It also depends on the file_storage/stored_file classes already being loaded. It happens to work here because the listing renders after $OUTPUT->header() (so filelib.php is loaded) and the reference count only needs the content hash, but it is non-idiomatic and brittle.
Low risk. Functionally correct in the current call context and carries no security implications. The concern is maintainability and resilience: using get_file_storage() and get_file_by_id() is the supported pattern, avoids relying on incidental class loading, and avoids passing a non-canonical record into the stored_file constructor.
This method overrides the base is_file_used() to report whether a content-bank file is referenced more than once. It is invoked from the course_file constructor while building each row of the listing.
protected function is_file_used(): ?bool {
$fs = new \file_storage();
$f = new \stored_file($fs, $this->file);
return $fs->get_references_count_by_storedfile($f) > 1;
}
protected function is_file_used(): ?bool {
$fs = get_file_storage();
$f = $fs->get_file_by_id($this->file->id);
return $f ? $fs->get_references_count_by_storedfile($f) > 1 : null;
}
This uses the singleton, obtains a properly constructed stored_file, and guards against a missing file.
The repository ships a comprehensive GitHub Actions workflow that runs phpunit and behat, but the plugin contains no tests/ directory and therefore no PHPUnit or Behat tests.
Given that the plugin performs security-sensitive operations (capability-gated bulk download, context-scoped license changes, the check_files_context() boundary), automated tests would provide valuable regression protection — particularly around the context-isolation logic, which is the main control preventing cross-course access.
Informational. Not a defect or vulnerability. Adding unit tests for check_files_context(), the mime-type/component filtering, and the license-change validation would strengthen confidence in the security-relevant code paths and guard against regressions.
The CI matrix (.github/workflows/moodle-ci.yml) exercises multiple PHP versions, Moodle branches, and both PostgreSQL and MariaDB, and includes phpunit and behat steps. Without any tests in the plugin, those steps currently validate nothing plugin-specific.
Capability assignment guidance. The three capabilities are correctly declared with RISK_PERSONAL and default to editingteacher/manager. Because the listing and ZIP download centralise access to all course files and bypass pluginfile.php per-file checks (see finding 1), administrators should avoid assigning these capabilities to students or other low-trust roles.
Context isolation is well implemented. check_files_context() confirms each targeted file's context path is a descendant of the current course context (or equals it) before any license change or download. This correctly blocks cross-course access and access to user-context files (drafts, private files), and it fails closed when a context path is missing.
Privacy provider is appropriate. The plugin stores no data in its own tables — it only displays and (for the license column) updates core file records, and emits a standard license_changed event handled by core logging. Implementing null_provider is the correct choice, and no db/install.xml/db/uninstall.php is required.
Minor front-end best practice. templates/view.mustache uses an inline href="javascript:void(0);" handler and an inline {{#js}} block that pulls in jQuery via require(['jquery'], ...). This works and is a recognised Moodle pattern, but moving the select-all/none behaviour into a dedicated AMD module and binding the handler in JS (rather than a javascript: URI) would be more CSP-friendly and maintainable.
No bundled third-party code. No thirdpartylibs.xml is present, and none is required — jQuery is consumed through Moodle's AMD loader rather than bundled, and no other external libraries ship with the plugin.