From fac1bf65c185cdf01b564c0ac69b1a24539f0727 Mon Sep 17 00:00:00 2001 From: Philipp Memmel Date: Wed, 18 Oct 2023 10:31:26 +0000 Subject: [PATCH] MBS-8300: Improve data input/output handling --- classes/boardmanager.php | 36 +++++++++++++++------ classes/external/change_kanban_content.php | 9 ++++-- classes/external/get_kanban_content.php | 9 +++++- classes/helper.php | 16 ++++++++++ classes/updateformatter.php | 2 ++ tests/boardmanager_test.php | 37 ++++++++++++++++++++++ 6 files changed, 96 insertions(+), 13 deletions(-) diff --git a/classes/boardmanager.php b/classes/boardmanager.php index 3982ccf3..1bb440f6 100644 --- 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 index 44a63033..693e224d 100644 --- 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 index 403d0f9d..3bb950d3 100644 --- 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 index 8812fa6d..784833a7 100644 --- 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 index e94762da..8b5a2e40 100644 --- 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 index a450be3e..8c1816d3 100644 --- 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": "bad html"}', + 'expected' => '{"test":"bad html<\/b>"}' + ], + [ + 'json' => '{"test": "good html"}', + 'expected' => '{"test":"good html<\/b>"}' + ], + [ + 'json' => '{"test": "good html","anotherkey": [{"nestedkey":""}]}', + 'expected' => '{"test":"good html<\/b>","anotherkey":[{"nestedkey":""}]}' + ] + ]; + } }