From 6f0506fa9d9d7de2f20bb46824675a146404a0a6 Mon Sep 17 00:00:00 2001
From: Brendan Heywood <brendan@catalyst-au.net>
Date: Fri, 27 Mar 2020 23:07:28 +1100
Subject: [PATCH] MDL-68276 admin: Skip risky tables and columns in db_replace

---
 lib/adminlib.php            | 44 +++++++++++++++---
 lib/tests/adminlib_test.php | 89 +++++++++++++++++++++++++++++++++++++
 2 files changed, 127 insertions(+), 6 deletions(-)
 create mode 100644 lib/tests/adminlib_test.php

diff --git a/lib/adminlib.php b/lib/adminlib.php
index ff2678730b3..2f950ae6e36 100644
--- a/lib/adminlib.php
+++ b/lib/adminlib.php
@@ -8790,6 +8790,40 @@ function any_new_admin_settings($node) {
     return false;
 }
 
+/**
+ * Given a table and optionally a column name should replaces be done?
+ *
+ * @param string $table name
+ * @param string $column name
+ * @return bool success or fail
+ */
+function db_should_replace($table, $column = ''): bool {
+
+    // TODO: this is horrible hack, we should do whitelisting and each plugin should be responsible for proper replacing...
+    $skiptables = ['config', 'config_plugins', 'filter_config', 'sessions',
+        'events_queue', 'repository_instance_config', 'block_instances', 'files'];
+
+    // Don't process these.
+    if (in_array($table, $skiptables)) {
+        return false;
+    }
+
+    // To be safe never replace inside a table that looks related to logging.
+    if (preg_match('/(^|_)logs?($|_)/', $table)) {
+        return false;
+    }
+
+    // Do column based exclusions.
+    if (!empty($column)) {
+        // Don't touch anything that looks like a hash.
+        if (preg_match('/hash$/', $column)) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 /**
  * Moved from admin/replace.php so that we can use this in cron
  *
@@ -8800,11 +8834,6 @@ function any_new_admin_settings($node) {
 function db_replace($search, $replace) {
     global $DB, $CFG, $OUTPUT;
 
-    // TODO: this is horrible hack, we should do whitelisting and each plugin should be responsible for proper replacing...
-    $skiptables = array('config', 'config_plugins', 'config_log', 'upgrade_log', 'log',
-                        'filter_config', 'sessions', 'events_queue', 'repository_instance_config',
-                        'block_instances', '');
-
     // Turn off time limits, sometimes upgrades can be slow.
     core_php_time_limit::raise();
 
@@ -8813,13 +8842,16 @@ function db_replace($search, $replace) {
     }
     foreach ($tables as $table) {
 
-        if (in_array($table, $skiptables)) {      // Don't process these
+        if (!db_should_replace($table)) {
             continue;
         }
 
         if ($columns = $DB->get_columns($table)) {
             $DB->set_debug(true);
             foreach ($columns as $column) {
+                if (!db_should_replace($table, $column->name)) {
+                    continue;
+                }
                 $DB->replace_all_text($table, $column, $search, $replace);
             }
             $DB->set_debug(false);
diff --git a/lib/tests/adminlib_test.php b/lib/tests/adminlib_test.php
new file mode 100644
index 00000000000..3f65921e0ba
--- /dev/null
+++ b/lib/tests/adminlib_test.php
@@ -0,0 +1,89 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Unit tests for parts of adminlib.php.
+ *
+ * @package    core
+ * @subpackage admin
+ * @copyright  2020 Brendan Heywood <brendan@catalyst-au.net>
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+defined('MOODLE_INTERNAL') || die();
+
+global $CFG;
+require_once($CFG->libdir.'/adminlib.php');
+
+/**
+ * Unit tests for parts of adminlib.php.
+ *
+ * @copyright  2020 Brendan Heywood <brendan@catalyst-au.net>
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class core_adminlib_testcase extends advanced_testcase {
+
+    /**
+     * Data provider of serialized string.
+     *
+     * @return array
+     */
+    public function db_should_replace_dataprovider() {
+        return [
+            // Skipped tables.
+            ['block_instances', '', false],
+            ['config',          '', false],
+            ['config_plugins',  '', false],
+            ['config_log',      '', false],
+            ['events_queue',    '', false],
+            ['filter_config',   '', false],
+            ['log',             '', false],
+            ['repository_instance_config', '', false],
+            ['sessions',        '', false],
+            ['upgrade_log',     '', false],
+
+            // Unknown skipped tables.
+            ['foobar_log',      '', false],
+            ['foobar_logs',     '', false],
+
+            // Unknown ok tables.
+            ['foobar_logical',  '', true],
+
+            // Normal tables.
+            ['assign',          '', true],
+
+            // Normal tables with excluded columns.
+            ['message_conversations', 'convhash',   false],
+            ['user_password_history', 'hash',       false],
+            ['foo',                   'barhash',    false],
+        ];
+    }
+
+    /**
+     * Test which tables and column should be replaced.
+     *
+     * @dataProvider db_should_replace_dataprovider
+     * @param string $table name
+     * @param string $column name
+     * @param bool $expected whether it should be replaced
+     */
+    public function test_db_should_replace(string $table, string $column, bool $expected) {
+        $actual = db_should_replace($table, $column);
+        $this->assertSame($actual, $expected);
+    }
+
+}
+