diff --git a/lib/filelib.php b/lib/filelib.php index a0ea6642e7050..7c50999b30e0d 100644 --- a/lib/filelib.php +++ b/lib/filelib.php @@ -186,7 +186,9 @@ function file_prepare_standard_editor($data, $field, array $options, $context=nu $data->{$field.'format'} = editors_get_preferred_format(); } if (!$options['noclean']) { - $data->{$field} = clean_text($data->{$field}, $data->{$field.'format'}); + if ($data->{$field.'format'} != FORMAT_MARKDOWN) { + $data->{$field} = clean_text($data->{$field}, $data->{$field . 'format'}); + } } } else { @@ -198,7 +200,11 @@ function file_prepare_standard_editor($data, $field, array $options, $context=nu $data = trusttext_pre_edit($data, $field, $context); } else { if (!$options['noclean']) { - $data->{$field} = clean_text($data->{$field}, $data->{$field.'format'}); + // We do not have a way to sanitise Markdown texts, + // luckily editors for this format should not have XSS problems. + if ($data->{$field.'format'} != FORMAT_MARKDOWN) { + $data->{$field} = clean_text($data->{$field}, $data->{$field.'format'}); + } } } $contextid = $context->id; diff --git a/lib/tests/filelib_test.php b/lib/tests/filelib_test.php index 02c175edcf770..32dc34535c9fa 100644 --- a/lib/tests/filelib_test.php +++ b/lib/tests/filelib_test.php @@ -1931,6 +1931,85 @@ public function test_file_is_draft_areas_limit_reached() { sleep(ceil(1 / $leak)); $this->assertFalse(file_is_draft_areas_limit_reached($user->id)); } + + /** + * Test text cleaning when preparing text editor data. + * + * @covers ::file_prepare_standard_editor + */ + public function test_file_prepare_standard_editor_clean_text() { + $text = "lala "; + + $syscontext = \context_system::instance(); + + $object = new \stdClass(); + $object->some = $text; + $object->someformat = FORMAT_PLAIN; + + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => false]); + $this->assertSame($text, $result->some); + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => true]); + $this->assertSame($text, $result->some); + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => false, 'context' => $syscontext], $syscontext, 'core', 'some', 1); + $this->assertSame($text, $result->some); + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => true, 'context' => $syscontext], $syscontext, 'core', 'some', 1); + $this->assertSame($text, $result->some); + + $object = new \stdClass(); + $object->some = $text; + $object->someformat = FORMAT_MARKDOWN; + + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => false]); + $this->assertSame($text, $result->some); + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => true]); + $this->assertSame($text, $result->some); + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => false, 'context' => $syscontext], $syscontext, 'core', 'some', 1); + $this->assertSame($text, $result->some); + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => true, 'context' => $syscontext], $syscontext, 'core', 'some', 1); + $this->assertSame($text, $result->some); + + $object = new \stdClass(); + $object->some = $text; + $object->someformat = FORMAT_MOODLE; + + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => false]); + $this->assertSame('lala xx', $result->some); + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => true]); + $this->assertSame($text, $result->some); + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => false, 'context' => $syscontext], $syscontext, 'core', 'some', 1); + $this->assertSame('lala xx', $result->some); + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => true, 'context' => $syscontext], $syscontext, 'core', 'some', 1); + $this->assertSame($text, $result->some); + + $object = new \stdClass(); + $object->some = $text; + $object->someformat = FORMAT_HTML; + + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => false]); + $this->assertSame('lala xx', $result->some); + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => true]); + $this->assertSame($text, $result->some); + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => false, 'context' => $syscontext], $syscontext, 'core', 'some', 1); + $this->assertSame('lala xx', $result->some); + $result = file_prepare_standard_editor(clone($object), 'some', + ['noclean' => true, 'context' => $syscontext], $syscontext, 'core', 'some', 1); + $this->assertSame($text, $result->some); + } } /** diff --git a/lib/tests/weblib_test.php b/lib/tests/weblib_test.php index c390b8518d290..f34b81e5066ff 100644 --- a/lib/tests/weblib_test.php +++ b/lib/tests/weblib_test.php @@ -280,6 +280,204 @@ public function test_clean_text() { $this->assertSame('lala xx', clean_text($text, FORMAT_HTML)); } + /** + * Test trusttext enabling. + * + * @covers ::trusttext_active + */ + public function test_trusttext_active() { + global $CFG; + $this->resetAfterTest(); + + $this->assertFalse(trusttext_active()); + $CFG->enabletrusttext = '1'; + $this->assertTrue(trusttext_active()); + } + + /** + * Test trusttext detection. + * + * @covers ::trusttext_trusted + */ + public function test_trusttext_trusted() { + global $CFG; + $this->resetAfterTest(); + + $syscontext = context_system::instance(); + $course = $this->getDataGenerator()->create_course(); + $coursecontext = context_course::instance($course->id); + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($user2->id, $course->id, 'editingteacher'); + + $this->setAdminUser(); + + $CFG->enabletrusttext = '0'; + $this->assertFalse(trusttext_trusted($syscontext)); + $this->assertFalse(trusttext_trusted($coursecontext)); + + $CFG->enabletrusttext = '1'; + $this->assertTrue(trusttext_trusted($syscontext)); + $this->assertTrue(trusttext_trusted($coursecontext)); + + $this->setUser($user1); + + $CFG->enabletrusttext = '0'; + $this->assertFalse(trusttext_trusted($syscontext)); + $this->assertFalse(trusttext_trusted($coursecontext)); + + $CFG->enabletrusttext = '1'; + $this->assertFalse(trusttext_trusted($syscontext)); + $this->assertFalse(trusttext_trusted($coursecontext)); + + $this->setUser($user2); + + $CFG->enabletrusttext = '0'; + $this->assertFalse(trusttext_trusted($syscontext)); + $this->assertFalse(trusttext_trusted($coursecontext)); + + $CFG->enabletrusttext = '1'; + $this->assertFalse(trusttext_trusted($syscontext)); + $this->assertTrue(trusttext_trusted($coursecontext)); + } + + /** + * Data provider for trusttext_pre_edit() tests. + */ + public function trusttext_pre_edit_provider(): array { + return [ + [true, 0, 'editingteacher', FORMAT_HTML, 1], + [true, 0, 'editingteacher', FORMAT_MOODLE, 1], + [false, 0, 'editingteacher', FORMAT_MARKDOWN, 1], + [false, 0, 'editingteacher', FORMAT_PLAIN, 1], + + [false, 1, 'editingteacher', FORMAT_HTML, 1], + [false, 1, 'editingteacher', FORMAT_MOODLE, 1], + [false, 1, 'editingteacher', FORMAT_MARKDOWN, 1], + [false, 1, 'editingteacher', FORMAT_PLAIN, 1], + + [true, 0, 'student', FORMAT_HTML, 1], + [true, 0, 'student', FORMAT_MOODLE, 1], + [false, 0, 'student', FORMAT_MARKDOWN, 1], + [false, 0, 'student', FORMAT_PLAIN, 1], + + [true, 1, 'student', FORMAT_HTML, 1], + [true, 1, 'student', FORMAT_MOODLE, 1], + [false, 1, 'student', FORMAT_MARKDOWN, 1], + [false, 1, 'student', FORMAT_PLAIN, 1], + ]; + } + + /** + * Test text cleaning before editing. + * + * @dataProvider trusttext_pre_edit_provider + * @covers ::trusttext_pre_edit + * + * @param bool $expectedsanitised + * @param int $enabled + * @param string $rolename + * @param string $format + * @param int $trust + */ + public function test_trusttext_pre_edit(bool $expectedsanitised, int $enabled, string $rolename, + string $format, int $trust) { + global $CFG, $DB; + $this->resetAfterTest(); + + $exploit = "abc < > &"; + $sanitised = purify_html($exploit); + + $course = $this->getDataGenerator()->create_course(); + $context = context_course::instance($course->id); + + $user = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($user->id, $course->id, $rolename); + + $this->setUser($user); + + $CFG->enabletrusttext = (string)$enabled; + + $object = new stdClass(); + $object->some = $exploit; + $object->someformat = $format; + $object->sometrust = (string)$trust; + $result = trusttext_pre_edit(clone($object), 'some', $context); + + if ($expectedsanitised) { + $message = "sanitisation is expected for: $enabled, $rolename, $format, $trust"; + $this->assertSame($sanitised, $result->some, $message); + } else { + $message = "sanitisation is not expected for: $enabled, $rolename, $format, $trust"; + $this->assertSame($exploit, $result->some, $message); + } + } + + /** + * Test removal of legacy trusttext flag. + * @covers ::trusttext_strip + */ + public function test_trusttext_strip() { + $this->assertSame('abc', trusttext_strip('abc')); + $this->assertSame('abc', trusttext_strip('ab#####TRUSTTEXT#####c')); + } + + /** + * Test trust option of format_text(). + * @covers ::format_text + */ + public function test_format_text_trusted() { + global $CFG; + $this->resetAfterTest(); + + $text = "lala "; + + $CFG->enabletrusttext = 0; + + $this->assertSame(s($text), + format_text($text, FORMAT_PLAIN, ['trusted' => true])); + $this->assertSame("
lala xx
\n", + format_text($text, FORMAT_MARKDOWN, ['trusted' => true])); + $this->assertSame('lala xx
\n", + format_text($text, FORMAT_MARKDOWN, ['trusted' => false])); + $this->assertSame('lala xx
\n", + format_text($text, FORMAT_MARKDOWN, ['trusted' => true])); + $this->assertSame('lala xx
\n", + format_text($text, FORMAT_MARKDOWN, ['trusted' => false])); + $this->assertSame('lala
\n", + format_text($text, FORMAT_MARKDOWN, ['trusted' => true, 'noclean' => true])); + $this->assertSame("lala
\n", + format_text($text, FORMAT_MARKDOWN, ['trusted' => false, 'noclean' => true])); + } + /** * @covers ::qualified_me */ diff --git a/lib/weblib.php b/lib/weblib.php index 4c97f78882046..616caf29e0494 100644 --- a/lib/weblib.php +++ b/lib/weblib.php @@ -1275,6 +1275,11 @@ function format_text($text, $format = FORMAT_MOODLE, $options = null, $courseidd if (!isset($options['trusted'])) { $options['trusted'] = false; } + if ($format == FORMAT_MARKDOWN) { + // Markdown format cannot be trusted in trusttext areas, + // because we do not know how to sanitise it before editing. + $options['trusted'] = false; + } if (!isset($options['noclean'])) { if ($options['trusted'] and trusttext_active()) { // No cleaning if text trusted and noclean not specified. @@ -1713,6 +1718,12 @@ function trusttext_pre_edit($object, $field, $context) { $trustfield = $field.'trust'; $formatfield = $field.'format'; + if ($object->$formatfield == FORMAT_MARKDOWN) { + // We do not have a way to sanitise Markdown texts, + // luckily editors for this format should not have XSS problems. + return $object; + } + if (!$object->$trustfield or !trusttext_trusted($context)) { $object->$field = clean_text($object->$field, $object->$formatfield); }