Vault - Site backup and migration

In classes/local/restoreactions/cleanup_existing_files.php the $restoreid variable is embedded directly into the SQL string for both the INSERT ... SELECT and DELETE statements rather than being bound as a placeholder.

The function is typed int $restoreid so PHP will coerce the value at the function boundary, which prevents SQL injection in practice. However, this style is inconsistent with Moodle conventions: the rest of the plugin uses $DB->execute($sql, $params) with ?/:name placeholders, and Moodle expects all dynamic values to be parameterised so that a future signature change (e.g. relaxing the int constraint, or adding a wrapper that forwards strings) cannot silently introduce a SQLi vector.

The surrounding code already builds a $params array from $DB->get_in_or_equal(...), so adding one more bound parameter is trivial.

Operation logs are constructed in operation_model::format_log_line() and wrapped in html_writer::span($message, $class). html_writer::span() (and html_writer::tag() underneath it) does not HTML-escape its first argument, and the templates that display logs use triple-brace mustache placeholders ({{{logs}}}, {{{logsshort}}}) inside <pre> blocks.

The log content includes:

• Strings the plugin itself writes (safe — internal constants).
• API error messages — already sanitised with s() in api::prepare_api_exception().
• Any output captured from PHP/Moodle during a backup/restore, via log_capture::add_line(), which is then stored verbatim with LOGLEVEL_UNKNOWN.

The last category is the concern: Moodle's debugging output and other mtrace/echo calls during a long-running backup/restore can include HTML (e.g. debugging() messages contain anchor tags). Anything rendered raw inside <pre> is still interpreted as HTML by the browser, so a debug message containing <script>...</script> would execute when an admin views the log.

Logs are admin-restricted via the main pages (moodle/site:config) but progress.php also exposes them to anyone with the accesskey (a 32-char random string typically copied from the admin UI). In practice the risk is low because the captured stream is server-controlled — but escaping at the template layer (or in format_log_line()) would close the gap and make the path defence-in-depth safe.

In templates/restore_details.mustache the backup key value is rendered with triple-brace mustache, which disables HTML escaping:

• Line 73: <td><a href="{{backupdetailsurl}}">{{{backupkey}}}</a></td>

The value originates from restore_details.php:

'backupkey' => $this->restore->backupkey,

which is the raw string stored in the tool_vault_operation.backupkey column. The DB schema allows up to 120 characters and the value is what the remote lmsvault.io API returned in response to PUT /backups. The plugin itself does not sanitise it before storing or rendering.

Other templates (logs.mustache, section_restore.mustache, dryrun.mustache) correctly use the double-brace {{backupkey}} form. This inconsistency makes it likely that the triple-brace use here is unintentional rather than a deliberate choice to allow markup.

templates/error_with_backtrace.mustache renders the error string and PHP backtrace without HTML escaping:

• Line 30: {{{ error }}}
• Line 38: <pre>{{{.}}}</pre> (backtrace)

These values come from error_with_backtrace::create_from_model() / create_from_exception(), which pull $model->get_details()['error'] (originally $t->getMessage()) and $t->getTraceAsString() directly. Exception messages can include arbitrary strings — for instance moodle_exception messages embed the $a placeholder content, and dml_exception::$debuginfo can include parts of failing SQL — which may contain <, >, or quote characters that the template will interpret as HTML.

The backtrace block is only displayed in debugging() mode, but the inline error is shown to every admin who hits a failure page (backup_details, restore_details, dryrun, check_summary, vaulttools_details, the remote-backups list, …).