MBS-8300: Improve data input/output handling

mebis-lp · Oct 18, 2023 · fac1bf6 · fac1bf6
1 parent 881c13e
commit fac1bf6
Show file tree

Hide file tree

Showing 6 changed files with 96 additions and 13 deletions.
diff --git a/classes/boardmanager.php b/classes/boardmanager.php
@@ -229,7 +229,7 @@ public function create_board_from_template(int $templateid = 0, array $data = []
             $columnids = [];
             foreach ($columns as $columnname => $options) {
                 $columnids[] = $DB->insert_record('kanban_column', [
-                    'title' => $columnname,
+                    'title' => clean_param($columnname, PARAM_TEXT),
                     'sequence' => '',
                     'kanban_board' => $boardid,
                     'options' => $options,
@@ -267,6 +267,7 @@ public function create_board_from_template(int $templateid = 0, array $data = []
             $newcolumn = [];
             $newcard = [];
             foreach ($columns as $column) {
+                $column->title = clean_param($column->title, PARAM_TEXT);
                 $newcolumn[$column->id] = clone $column;
                 $newcolumn[$column->id]->kanban_board = $newboard['id'];
                 $newcolumn[$column->id]->timecreated = time();
@@ -416,6 +417,8 @@ public function add_column(int $aftercol = 0, array $data = []): int {
             ];
             $data = array_merge($defaults, $data, $defaultsfixed);
 
+            // Sanitize title to be extra safe.
+            $data['title'] = clean_param($data['title'], PARAM_TEXT);
             $data['id'] = $DB->insert_record('kanban_column', $data);
 
             $update = [
@@ -461,6 +464,8 @@ public function add_card(int $columnid, int $aftercard = 0, array $data = []): i
 
         $data['id'] = $DB->insert_record('kanban_card', $data);
         $data['assignees'] = [];
+        // Sanitize title to be extra safe.
+        $data['title'] = clean_param($data['title'], PARAM_TEXT);
 
         $column = $DB->get_record('kanban_column', ['id' => $columnid]);
 
@@ -473,7 +478,7 @@ public function add_card(int $columnid, int $aftercard = 0, array $data = []): i
 
         // Users can always edit cards they created.
         $data['canedit'] = true;
-        $data['columnname'] = $column->title;
+        $data['columnname'] = clean_param($column->title, PARAM_TEXT);
 
         $this->formatter->put('cards', $data);
         $this->formatter->put('columns', $update);
@@ -566,11 +571,11 @@ public function move_card(int $cardid, int $aftercard, int $columnid = 0): void
             $data = array_merge((array) $card, $updatecard);
             $data['username'] = fullname($USER);
             $data['boardname'] = $this->kanban->name;
-            $data['columnname'] = $targetcolumn->title;
+            $data['columnname'] = clean_param($targetcolumn->title, PARAM_TEXT);
             $assignees = $this->get_card_assignees($cardid);
             helper::send_notification($this->cminfo, 'moved', $assignees, (object) $data);
             if (!empty($options->autoclose) && $card->completed == 0) {
-                $data['title'] = $card->title;
+                $data['title'] = clean_param($card->title, PARAM_TEXT);
                 helper::send_notification($this->cminfo, 'closed', $assignees, (object) $data);
                 helper::remove_calendar_event($this->kanban, $card);
                 $this->write_history('completed', constants::MOD_KANBAN_CARD, [], $columnid, $cardid);
@@ -579,7 +584,7 @@ public function move_card(int $cardid, int $aftercard, int $columnid = 0): void
             $this->write_history(
                 'moved',
                 constants::MOD_KANBAN_CARD,
-                ['columnname' => $targetcolumn->title],
+                ['columnname' => clean_param($targetcolumn->title, PARAM_TEXT)],
                 $card->kanban_column,
                 $cardid
             );
@@ -749,7 +754,7 @@ public function add_discussion_message(int $cardid, string $message): void {
         }
 
         $update['boardname'] = $this->kanban->name;
-        $update['title'] = $card->title;
+        $update['title'] = clean_param($card->title, PARAM_TEXT);
         $assignees = $this->get_card_assignees($cardid);
         helper::send_notification($this->cminfo, 'discussion', $assignees, (object) $update);
         // Do not write username to history.
@@ -801,6 +806,16 @@ public function update_card(int $cardid, array $data): void {
             'kanban_board',
             'completed'
         ];
+        // Do some extra sanitizing.
+        if (isset($data['title'])) {
+            $data['title'] = clean_param($data['title'], PARAM_TEXT);
+        }
+        if (isset($data['description'])) {
+            $data['description'] = clean_param($data['description'], PARAM_CLEANHTML);
+        }
+        if (isset($data['options'])) {
+            $data['options'] = helper::sanitize_json_string($data['options']);
+        }
         if (!empty($data['color'])) {
             $data['options'] = json_encode(['background' => $data['color']]);
         }
@@ -900,7 +915,7 @@ public function update_card(int $cardid, array $data): void {
         $this->write_history(
             'updated',
             constants::MOD_KANBAN_CARD,
-            array_merge(['title' => $card['title']], $cardupdate),
+            array_merge(['title' => clean_param($card['title'], PARAM_TEXT)], $cardupdate),
             $card['kanban_column'],
             $card['id']
         );
@@ -921,10 +936,13 @@ public function update_column(int $columnid, array $data): void {
             'autoclose' => $data['autoclose'],
             'autohide' => $data['autohide'],
         ];
+        if (isset($data['title'])) {
+            $data['title'] = clean_param($data['title'], PARAM_TEXT);
+        }
         $columndata = [
             'id' => $columnid,
             'title' => $data['title'],
-            'options' => json_encode($options),
+            'options' => helper::sanitize_json_string(json_encode($options)),
             'timemodified' => time(),
         ];
 
@@ -1102,7 +1120,7 @@ public function write_history(string $action, int $type, array $data = [], int $
             'userid' => $USER->id,
             'kanban_column' => $columnid,
             'kanban_card' => $cardid,
-            'parameters' => json_encode($data),
+            'parameters' => helper::sanitize_json_string(json_encode($data)),
             'affected_userid' => $affecteduser,
             'timestamp' => time(),
             'type' => $type,

diff --git a/classes/external/change_kanban_content.php b/classes/external/change_kanban_content.php
@@ -104,7 +104,8 @@ public static function add_column(int $cmid, int $boardid, array $data): array {
         $boardid = $params['boardid'];
         $data = [];
         if (!empty($params['data']['title'])) {
-            $data['title'] = $params['data']['title'];
+            // Additional cleaning most likely not needed, because title is PARAM_TEXT, but let's be extra sure.
+            $data['title'] = clean_param($params['data']['title'], PARAM_TEXT);
         }
         $aftercol = $params['data']['aftercol'];
         list($course, $cminfo) = get_course_and_cm_from_cmid($cmid);
@@ -176,7 +177,8 @@ public static function add_card(int $cmid, int $boardid, array $data): array {
         $columnid = $params['data']['columnid'];
         $data = [];
         if (!empty($params['data']['title'])) {
-            $data['title'] = $params['data']['title'];
+            // Additional cleaning most likely not needed, because title is PARAM_TEXT, but let's be extra sure.
+            $data['title'] = clean_param($params['data']['title'], PARAM_TEXT);
         }
         $aftercard = $params['data']['aftercard'];
         list($course, $cminfo) = get_course_and_cm_from_cmid($cmid);
@@ -838,7 +840,8 @@ public static function add_discussion_message(int $cmid, int $boardid, array $da
         $cmid = $params['cmid'];
         $boardid = $params['boardid'];
         $cardid = $params['data']['cardid'];
-        $message = $params['data']['message'];
+        // Additional cleaning most likely not needed, because message is PARAM_TEXT, but let's be extra sure.
+        $message = clean_param($params['data']['message'], PARAM_TEXT);
 
         list($course, $cminfo) = get_course_and_cm_from_cmid($cmid);
         $context = context_module::instance($cmid);

diff --git a/classes/external/get_kanban_content.php b/classes/external/get_kanban_content.php
@@ -487,6 +487,9 @@ public static function execute(int $cmid, int $boardid, int $timestamp = 0, bool
         } else {
             $kanbancolumns = [];
         }
+        foreach ($kanbancolumns as $kanbancolumn) {
+            $kanbancolumn->title = clean_param($kanbancolumn->title, PARAM_TEXT);
+        }
 
         if (!$timestampcards || $timestamp <= $timestampcards) {
             $kanbancards = $DB->get_records_select('kanban_card', $sql, $params);
@@ -521,13 +524,14 @@ public static function execute(int $cmid, int $boardid, int $timestamp = 0, bool
                 if (empty($kanbanassignees[$card->id])) {
                     $kanbanassignees[$card->id] = [];
                 }
+                $card->title = clean_param($card->title, PARAM_TEXT);
                 $card->canedit = $capabilities['managecards'] || $card->createdby == $USER->id;
                 $card->assignees = $kanbanassignees[$card->id];
                 $card->selfassigned = in_array($USER->id, $card->assignees);
                 $card->hasdescription = !empty($card->description);
                 $card->discussions = [];
                 $card->description = file_rewrite_pluginfile_urls(
-                    $card->description,
+                    format_text($card->description),
                     'pluginfile.php',
                     $context->id,
                     'mod_kanban',
@@ -637,6 +641,7 @@ public static function get_discussion_update(int $cmid, int $boardid, int $cardi
 
         $formatter = new updateformatter();
         foreach ($discussions as $discussion) {
+            $discussion->content = format_text($discussion->content);
             $discussion->candelete = $discussion->userid == $USER->id || has_capability('mod/kanban:manageboard', $context);
             $discussion->username = fullname(\core_user::get_user($discussion->userid));
             $formatter->put('discussions', (array) $discussion);
@@ -722,6 +727,8 @@ public static function get_history_update(int $cmid, int $boardid, int $cardid,
                 }
 
                 $type = constants::MOD_KANBAN_TYPES[$item->type];
+                // One has to be careful, because $item->parameters theoretically could contain user input.
+                $item->parameters = helper::sanitize_json_string($item->parameters);
                 $item = (object) array_merge((array) $item, json_decode($item->parameters, true));
                 $historyitem = [];
                 $historyitem['id'] = $item->id;

diff --git a/classes/helper.php b/classes/helper.php
@@ -420,4 +420,20 @@ public static function get_cache_key(int $type, int $boardid): string {
     public static function get_timestamp_cache(): \cache_application {
         return \cache::make('mod_kanban', 'timestamp');
     }
+
+    /**
+     * Sanitizes a json string by stripping off all html tags.
+     *
+     * @param string $jsonstring the json encoded string
+     * @return string the same string, but HTML code will have been sanitized
+     */
+    public static function sanitize_json_string(string $jsonstring): string {
+        $json = json_decode($jsonstring, true);
+        foreach ($json as $key => $value) {
+            unset($json[$key]);
+            $key = clean_param($key, PARAM_NOTAGS);
+            $json[$key] = is_array($value) ? clean_param_array($value, PARAM_CLEANHTML, true) : clean_param($value, PARAM_CLEANHTML);
+        }
+        return json_encode($json);
+    }
 }
diff --git a/classes/updateformatter.php b/classes/updateformatter.php
@@ -51,6 +51,8 @@ class updateformatter {
      * @param array $data Fields to update, must contain 'id' field
      */
     public function put(string $name, array $data) {
+        // Sanitize the output.
+        $data = json_decode(helper::sanitize_json_string(json_encode($data)), true);
         // Find int values covered as string.
         foreach ($data as $key => $value) {
             if ($key == 'sequence') {

diff --git a/tests/boardmanager_test.php b/tests/boardmanager_test.php
@@ -37,6 +37,7 @@ class boardmanager_test extends \advanced_testcase {
      * Prepare testing environment
      */
     public function setUp(): void {
+        $this->resetAfterTest();
         global $DB;
         $this->course = $this->getDataGenerator()->create_course();
         $this->kanban = $this->getDataGenerator()->create_module('kanban', ['course' => $this->course]);
@@ -270,4 +271,40 @@ public function test_delete_column() {
         array_shift($columnids);
         $this->assertEquals(join(',', $columnids), $boardmanager->get_board()->sequence);
     }
+
+
+    /**
+     * Tests the json sanitization function.
+     *
+     * @dataProvider test_sanitize_json_string_provider
+     * @param $jsonstring string the json string to sanitize
+     * @param $sanitized string the expected sanitized json string
+     * @return void
+     */
+    public function test_sanitize_json_string(string $jsonstring, string $sanitized): void {
+        $output = helper::sanitize_json_string($jsonstring);
+        $this->assertEquals($sanitized, $output);
+    }
+
+    /**
+     * Data Provider for self::test_sanitize_json_string.
+     *
+     * @return array[] containing the keys 'json' and 'expected'
+     */
+    public function test_sanitize_json_string_provider(): array {
+        return [
+            [
+                'json' => '{"test": "<b>bad html</b><script>console.log(\"bla\")</script>"}',
+                'expected' => '{"test":"<b>bad html<\/b>"}'
+            ],
+            [
+                'json' => '{"test": "<b>good html</b>"}',
+                'expected' => '{"test":"<b>good html<\/b>"}'
+            ],
+            [
+                'json' => '{"test": "<b>good html</b>","anotherkey": [{"nestedkey":"<script>console.log(\"bla\");</script>"}]}',
+                'expected' => '{"test":"<b>good html<\/b>","anotherkey":[{"nestedkey":""}]}'
+            ]
+        ];
+    }
 }