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":""}]}'
+ ]
+ ];
+ }
}