Flexible sections format
format_flexsections
Flexible sections course format for Moodle. Sections can be nested inside other sections, each section can be shown on the same page as its parent or as a link to a separate page. Hiding a section hides all nested sections and activities. Version reviewed: plugin version 2026032800 (release string 5.0.2) supporting Moodle 5.0 and 5.1.
The plugin is well-structured, uses parameterized DB queries, applies format_string() with the course context when rendering section names, uses capability checks consistently, and performs session key validation on most state-changing URL handlers. The only security finding is a missing confirm_sesskey() call for two GET-driven parameters in page_set_course() — addchildsection and movesection/moveparent — while six neighbouring handlers in the same function do validate sesskey. This is a genuine CSRF gap but is mitigated: exploitation requires a user with moodle/course:update (editing teacher or higher) to follow an attacker-crafted link, the actions (adding an empty section, re-parenting a section) are reversible and cause no data loss or privilege escalation, and the adjacent correct usage makes it clearly an oversight rather than a systemic problem. The remaining findings are code-quality items: direct $_SERVER['HTTP_REFERER'] use instead of get_local_referer(), a die('Error ...') in place of a coding_exception, and PARAM_RAW where PARAM_INT would be appropriate. A privacy null_provider is declared, which is consistent with the fact that the only user-scoped data (section preferences under the coursesectionspreferences_* name) is already handled by the core core_courseformat privacy provider.
Overview
format_flexsections is a mature course format plugin authored by Marina Glancy that implements hierarchical (nested) sections with per-section display-as-link/display-on-same-page controls, an accordion mode, an optional "back to section" widget on activity pages, and an adaptable course index. The reviewed tree targets Moodle 5.0 / 5.1 and ships a full AMD frontend (built + source), a restore plugin, behat + phpunit coverage, settings, hook callbacks, and Mustache templates that extend the core course format templates.
Security posture
Overall, the plugin follows Moodle security conventions well:
- All DB access goes through
$DBwith placeholders or Moodle-generatedget_in_or_equal()fragments. - Section name output uses
format_string()with a course context. - Capability checks (
moodle/course:update,moodle/course:movesections,moodle/course:setcurrentsection,moodle/course:sectionvisibility) gate every state action. - State-change AJAX endpoints are implemented as
core_courseformat\stateactionssubclasses (which receive automatic sesskey validation through the WS layer).
Main finding
The one real security gap is in format_flexsections::page_set_course() (lib.php). Of the eight URL-driven state mutations processed there, six call confirm_sesskey() but two do not:
addchildsection— creates a new subsection.movesection+moveparent(+ optionalmovebefore) — reparents/reorders a section.
Because page_set_course() is invoked on every $PAGE->set_course() call for a flexsections course, a teacher visiting an attacker-crafted URL (including one loaded via <img> or similar cross-origin request) will trigger these actions. The blast radius is a course the teacher can already edit; the impact is limited to adding an empty section or re-parenting a section (both reversible). The surrounding code proves the author intended sesskey protection — they just missed these two branches.
Other observations
get_caller_page_url()reads$_SERVER['HTTP_REFERER']directly intonew moodle_url()instead of usingget_local_referer(), which performsPARAM_LOCALURLcleaning.- An internal error path uses
die('Error in sections hierarchy'); // TODO.rather than throwing acoding_exception. movesectionprocessing parsesmovebeforeandsrwithPARAM_RAWinstead ofPARAM_INT; the values are later coerced to integers, so this is cosmetic.before_activitychooserbutton_exported::callbackuses PHP Reflection to mutate a protectedactionlinksproperty on the coreactivitychooserbuttonobject; this is fragile to core refactors but not a security issue.- The plugin performs direct writes to
course_sectionsandcourse_format_optionsinmove_section(),delete_section_with_children(),mergeup_section(), and the restore plugin; this is the Moodle-accepted pattern for formats that manage non-linear section hierarchies, and all queries are parameterised.
Privacy
The plugin implements \core_privacy\local\metadata\null_provider. The per-user state it writes (coursesectionspreferences_{courseid}) is the preference name already exported by the core core_courseformat\privacy\provider, so the null provider is correct for the normal case. The chunk-splitting helper in preferences.php writes additional preferences named ...#1, ...#2, which the core exporter does not enumerate — a minor edge case for courses where the preference JSON exceeds ~1.3 KB.
Findings
format_flexsections::page_set_course() in lib.php processes eight different URL-driven state mutations (add/move/merge/delete/switch-collapsed/marker/hide/show). Six of them call confirm_sesskey() before applying the change; two do not:
addchildsection— calls$this->create_new_section($addchildsection), which inserts a newcourse_sectionsrow (and acourse_format_optionsparent row) and rebuilds the course cache.movesection+moveparent(optionally withmovebefore/sr) — calls$this->move_section(...), which renumbers every section in the course and reparents one of them inside a single DB transaction.
Because page_set_course() is invoked from moodle_page::set_course() for every course page load (not only from code paths that originate in the format's own UI), an attacker can craft any URL that matches the check on line 804 (on_course_view_page()) and embed it in an off-site page (<img src=...>, auto-submitting form, cross-origin link). A user whose browser fetches that URL while authenticated as an editing teacher or higher will silently execute the action.
The rest of the function demonstrates that sesskey validation was the intended pattern — these two branches are clearly oversights, not a deliberate design choice. The capability check alone is not a substitute for sesskey verification: CSRF specifically targets users who already have the capability.
Medium risk. The vulnerability is a classic GET-based CSRF:
- Who can reach it: any user with
moodle/course:updateon the target course (editing teacher, manager, admin). A regular student or unauthenticated visitor cannot, because thehas_capability()check is still enforced. - Impact: adding an empty top-level section to a course, or moving one section to a new parent position. Both actions are reversible by the teacher, no data is destroyed, no privilege escalation occurs.
- Mitigating factors: the attacker needs to know (or guess) a course ID and a section number, the attack requires the teacher to visit an attacker-controlled page while authenticated, and the rearrangement is visible to the teacher on their next page load so any change can be undone.
- Why medium, not low: Moodle treats sesskey enforcement as mandatory for state-changing URL handlers, and the adjacent code in the same function gets it right — so this is a real deviation from project standard rather than just a stylistic gap. It also risks being worsened if the logic in
create_new_section/move_sectionever grows a more damaging side effect.
page_set_course() is called from moodle_page::set_course() (/moodle/public/lib/pagelib.php:1179). It runs on every course page load, which is why the function itself guards with if ($this->on_course_view_page()) { ... } before dispatching. Inside that block:
- Guarded by
confirm_sesskey():mergeup(line 834),deletesection(line 844),switchcollapsed(line 871),marker(line 892),hide(line 912),show(line 918). - Not guarded:
addchildsection(line 826),movesection/moveparent(line 863).
The two unguarded handlers call $this->create_new_section() and $this->move_section(). Both methods perform write transactions against course_sections and course_format_options, trigger cache rebuilds, and fire section deletion/create events.
The format's editing UI in classes/output/courseformat/content/section/controlmenu.php:64 appends sesskey to the URLs it generates for these actions, showing that the author believed the server would validate it — the server just doesn't.
Attacker hosts a page at https://evil.example/ containing:
<img src="https://target.moodle/course/view.php?id=123&addchildsection=0" style="display:none">
<img src="https://target.moodle/course/view.php?id=123&movesection=2&moveparent=1&movebefore=3" style="display:none">
When a teacher for course 123 is already logged in and visits evil.example, the browser makes authenticated GET requests to the two URLs. The first creates a new top-level section in course 123; the second reparents section 2 under section 1, inserted before section 3. No sesskey is required by the server, so the cross-origin requests succeed.
// If requested, create new section and redirect to course view page.
$addchildsection = optional_param('addchildsection', null, PARAM_INT);
if ($addchildsection !== null && has_capability('moodle/course:update', $context)) {
$sectionnum = $this->create_new_section($addchildsection);
$url = course_get_url($this->courseid, $sectionnum);
redirect($url);
}
Add confirm_sesskey() to the gate, matching the pattern used for mergeup, deletesection, switchcollapsed, marker, hide, and show in the same function:
$addchildsection = optional_param('addchildsection', null, PARAM_INT);
if ($addchildsection !== null && confirm_sesskey()
&& has_capability('moodle/course:update', $context)) {
$sectionnum = $this->create_new_section($addchildsection);
$url = course_get_url($this->courseid, $sectionnum);
redirect($url);
}
// If requested, move section.
$movesection = optional_param('movesection', null, PARAM_INT);
$moveparent = optional_param('moveparent', null, PARAM_INT);
$movebefore = optional_param('movebefore', null, PARAM_RAW);
$sr = optional_param('sr', null, PARAM_RAW);
$options = [];
if ($sr !== null) {
$options = ['sr' => $sr];
}
if ($movesection !== null && $moveparent !== null && has_capability('moodle/course:update', $context)) {
$newsectionnum = $this->move_section($movesection, $moveparent, $movebefore);
redirect(course_get_url($this->courseid, $newsectionnum, $options));
}
Add confirm_sesskey() to the gate:
if ($movesection !== null && $moveparent !== null && confirm_sesskey()
&& has_capability('moodle/course:update', $context)) {
$newsectionnum = $this->move_section($movesection, $moveparent, $movebefore);
redirect(course_get_url($this->courseid, $newsectionnum, $options));
}
get_caller_page_url() reads $_SERVER['HTTP_REFERER'] straight into new moodle_url(...). Moodle provides get_local_referer() (/moodle/public/lib/weblib.php:230) which applies clean_param($_SERVER['HTTP_REFERER'], PARAM_LOCALURL) and strips cross-site referers to an empty string.
In this plugin the resulting URL is only used to extract specific named query parameters (section, sectionid) via moodle_url::get_param(), and those values are further coerced to integers before being used in get_section_info*() lookups. So the practical risk is limited — the attacker cannot smuggle harmful data into section_info lookups. The issue is a code-quality / defense-in-depth concern: it makes the plugin's behaviour depend on unsanitised client headers, and it circumvents the standard Moodle helper.
Low risk. The value is not passed to a redirect, filesystem call, require, or raw SQL — only to moodle_url::get_param(). moodle_url is tolerant of malformed input, and the extracted params are coerced to integers before use, so a crafted referer cannot cause SQL injection or redirection. This is a hygiene finding: Moodle ships a dedicated helper for exactly this case and the plugin should use it.
get_caller_page_url() exists so that when the format is queried via the AJAX service endpoint (/lib/ajax/service.php) it can still figure out which section the user was viewing on the page that issued the AJAX call. It falls back to the HTTP referer only on that one code path. The return value is used by get_viewed_section() to read the section / sectionid query parameters of that referer URL.
protected function get_caller_page_url(): moodle_url {
global $PAGE, $FULLME;
$url = $PAGE->has_set_url() ? $PAGE->url : new moodle_url($FULLME);
if ($url->compare(new moodle_url('/lib/ajax/service.php'), URL_MATCH_BASE)) {
return !empty($_SERVER['HTTP_REFERER']) ? new moodle_url($_SERVER['HTTP_REFERER']) : $url;
}
return $url;
}
Use the Moodle helper that applies PARAM_LOCALURL cleaning and filters out cross-site referers:
protected function get_caller_page_url(): moodle_url {
global $PAGE, $FULLME;
$url = $PAGE->has_set_url() ? $PAGE->url : new moodle_url($FULLME);
if ($url->compare(new moodle_url('/lib/ajax/service.php'), URL_MATCH_BASE)) {
$referer = get_local_referer(false);
return $referer ? new moodle_url($referer) : $url;
}
return $url;
}
In move_section() the plugin checks whether the recursive reorder algorithm produced a permutation of the original section numbers. If the counts differ, it calls die('Error in sections hierarchy'); with a // TODO. comment.
Problems with this approach:
- The call halts the whole PHP request silently — no log, no stack trace, no structured error.
- The surrounding code has already opened a delegated DB transaction on line 999 by the time this is reachable only a few lines later, so abandoning via
die()leaves the transaction to be rolled back by the request shutdown rather than by explicit error handling. - The string is hard-coded English with no
get_string()key. die()is generally not allowed in Moodle plugin code — the convention is to throwcoding_exceptionormoodle_exception.
Low risk. This only affects error paths and does not introduce a user-facing security problem. The impact is diagnostic: if the invariant ever breaks, admins see a blank white page with raw text instead of a proper Moodle error page with a stack trace.
The block is the defensive fallback for an algorithmic invariant inside reorder_sections(). It is not reachable through normal user input — it would only fire if the recursion somehow dropped or duplicated a section id, which would be a bug in the plugin. Even so, the current handling is the wrong shape for that bug.
$this->reorder_sections($neworder, 0, $section->section, $parent, $before);
if (count($origorder) != count($neworder)) {
die('Error in sections hierarchy'); // TODO.
}
Throw a proper exception so the error is logged and the transaction (opened a few lines later) is rolled back cleanly:
if (count($origorder) != count($neworder)) {
throw new coding_exception('Error in sections hierarchy: reorder produced a different section count');
}
page_set_course() reads two inputs that are only ever used as section numbers:
$movebefore— passed throughmove_section()→can_move_section_to()where it is coerced with(int)$beforebefore use.$sr— placed in theoptionsarray and passed throughcourse_get_url(), which ultimately treats it as a section number.
Using PARAM_RAW bypasses Moodle's input validation at the entry point. The values happen to be made safe downstream (cast to int), so there is no injection risk in this specific code path. But accepting PARAM_RAW when PARAM_INT would work is the wrong default: it weakens audit readability (a reviewer has to trace the value through several methods before being confident it is safe), and it leaves an edge for future refactors that may consume the value without casting.
Low risk. Purely a hardening and hygiene concern — the current downstream casts close the door on injection. Mentioned so that the next refactor does not accidentally consume the raw value.
These two parameters only flow into move_section() and course_get_url(), both of which normalise the value to an integer before use. No SQL interpolation or redirect target uses the raw value.
$movebefore = optional_param('movebefore', null, PARAM_RAW);
$sr = optional_param('sr', null, PARAM_RAW);
Both parameters are section numbers (with sr being the "section return" integer). Prefer PARAM_INT:
$movebefore = optional_param('movebefore', null, PARAM_INT);
$sr = optional_param('sr', null, PARAM_INT);
If sr may legitimately be an empty string in some code paths, optional_param('sr', 0, PARAM_INT) or a dedicated normalisation step would be clearer than PARAM_RAW.
before_activitychooserbutton_exported::callback obtains the activity chooser button and uses PHP ReflectionObject to set its protected actionlinks property, removing the subsection action link.
This is a pragmatic workaround because the core class does not expose a public setter for actionlinks. The concern is not security — the data being filtered comes from core itself — but brittleness:
- If core renames
actionlinks, changes the visibility to private, wraps it in a value object, or makes the classfinal, this callback will start throwing. - Subclass refactors that re-compute
actionlinksafter the hook fires would silently re-introduce thesubsectionentry.
Consider proposing a public remove_action_link() or filter hook upstream and falling back to reflection only when the core version predates it.
Informational. No security impact — the data being removed originates in trusted core code. Called out because reflection against core is a maintenance liability and may fail quietly in a future Moodle release.
The hook runs when core exports the activity chooser button. Flexsections has its own subsection concept that conflicts with the mod_subsection activity added in Moodle 4.5+, so the format hides the latter to avoid confusion for teachers.
// Remove action link added by submodule. Use Reflections to set protected property $activitychooserbutton->actionlinks.
$refobject = new \ReflectionObject($activitychooserbutton);
$refproperty = $refobject->getProperty('actionlinks');
$refproperty->setAccessible(true);
$actionlinks = $refproperty->getValue($activitychooserbutton);
$actionlinks = array_filter($actionlinks, fn($a) => ($a->attributes['data-modname'] ?? null) !== 'subsection');
$refproperty->setValue($activitychooserbutton, array_values($actionlinks));
Longer-term, file a core MR proposing a public API (for example activitychooserbutton::remove_action_link($modname) or a filter hook) and guard the reflection fallback with a version check so newer Moodle releases use the supported API.
The plugin writes directly to two core-owned tables in multiple places:
move_section()performs the two-step negative-number renumber ($DB->set_field('course_sections', 'section', ...)in a transaction) because no core API exists for reordering arbitrary ranges of sections.delete_section_with_children()issuesDELETE FROM {course_format_options}andDELETE FROM {course_sections}in a single transaction after moving the deletion target to the end.mergeup_section()follows the same pattern.- The restore plugin (
restore_format_flexsections_plugin) re-numbers and cleans up orphan rows in both tables after a partial import.
For a course format that supports arbitrary parent/child hierarchies, there is no alternative — core's public helpers (course_update_section(), course_delete_section()) are designed for flat numbered sections and cannot express the reorderings flexsections needs. All queries are parameterised or use get_in_or_equal(), so there is no SQL injection risk.
Informational. Flagged because directly touching core tables is generally worth calling out in a review, not because anything about this implementation is unsafe. Parameters are bound, the table set is scoped to the plugin's concerns (section structure), and the logic is matched by equivalent two-step renumbering in Moodle's own move helpers.
These writes are the only way to implement flexible, nested section reordering given the current core API. All of them are wrapped in delegated transactions, all of them are followed by a rebuild_course_cache() call, and the deletion paths trigger the \core\event\course_section_deleted event and remove section summary files through the File API.
// Update all in database in one transaction.
$transaction = $DB->start_delegated_transaction();
// Update sections numbers in 2 steps to avoid breaking database uniqueness constraint.
foreach ($changes as $id => $change) {
$DB->set_field('course_sections', 'section', -$change['new'], ['id' => $id]);
}
foreach ($changes as $id => $change) {
$DB->set_field('course_sections', 'section', $change['new'], ['id' => $id]);
}
[$sectionsql, $params] = $DB->get_in_or_equal($sectionstodelete);
$sections = $DB->get_records_select('course_sections', "id $sectionsql", $params);
// Delete section records.
$transaction = $DB->start_delegated_transaction();
$DB->execute('DELETE FROM {course_format_options} WHERE sectionid ' . $sectionsql, $params);
$DB->execute('DELETE FROM {course_sections} WHERE id ' . $sectionsql, $params);
$transaction->allow_commit();
if (!empty($renumbermap)) {
// Step 1: Set to negative numbers to avoid uniqueness constraint.
foreach ($renumbermap as $old => $new) {
$DB->execute(
"UPDATE {course_sections} SET section = ? WHERE course = ? AND section = ?",
[-$new - 1, $courseid, $old]
);
}
Privacy handling is consistent with core. The plugin's null_provider is appropriate because the per-user preferences it persists use the coursesectionspreferences_{courseid} name that the core core_courseformat\privacy\provider already exports. There is a minor edge case: preferences::set_long_preference() chunks values over 1.3 KB into additional preferences named coursesectionspreferences_{courseid}#1, #2, etc. The core exporter only reads the un-suffixed name, so the overflow chunks would not appear in a subject access export. This only matters for courses with hundreds of section preferences per user and does not affect reset/deletion, which is handled by core via delete_records on the full user_preferences table.
AMD build artefacts are in sync with sources. Every file under amd/build/ is a direct minified match of the corresponding file under amd/src/; no extra third-party libraries are bundled. thirdpartylibs.xml is therefore not required.
Test coverage is solid. The plugin ships unit tests for section naming, view URL generation, inplace edit, section hide-recursion, section deletion with children, backup/restore partial-import regressions, and the long-preference helper, plus an extensive set of Behat features covering the UI flows (move, delete, duplicate, delegated sections, accordion, collapse, bulk actions, CM back-link). This made it easier to confirm that the CSRF gap in page_set_course() is not covered by existing tests — adding a test that a request without a valid sesskey is rejected would make regressions visible.
db/uninstall.php is not present. The plugin does not need one: course format data is removed by Moodle core when the plugin's own course_format_options rows and the format entries in course_sections are cleaned up via the standard format uninstall path, and there is no plugin-owned table or config_plugins data that requires bespoke cleanup.