Merge branch 'MDL-78505/master' of https://github.com/skodak/moodle

2023-08-17 09:10:53 +02:00
parent 8ecb766d53 989636b0de
commit 2870637daa
5 changed files with 300 additions and 8 deletions
@@ -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;
@@ -1931,6 +1931,85 @@ EOF;
        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 <object>xx</object>";
+
+        $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);
+    }
 }

 /**
@@ -280,6 +280,204 @@ class weblib_test extends advanced_testcase {
        $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<script>alert('xss')</script> < > &";
+        $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 <object>xx</object>";
+
+        $CFG->enabletrusttext = 0;
+
+        $this->assertSame(s($text),
+            format_text($text, FORMAT_PLAIN, ['trusted' => true]));
+        $this->assertSame("<p>lala xx</p>\n",
+            format_text($text, FORMAT_MARKDOWN, ['trusted' => true]));
+        $this->assertSame('<div class="text_to_html">lala xx</div>',
+            format_text($text, FORMAT_MOODLE, ['trusted' => true]));
+        $this->assertSame('lala xx',
+            format_text($text, FORMAT_HTML, ['trusted' => true]));
+
+        $this->assertSame(s($text),
+            format_text($text, FORMAT_PLAIN, ['trusted' => false]));
+        $this->assertSame("<p>lala xx</p>\n",
+            format_text($text, FORMAT_MARKDOWN, ['trusted' => false]));
+        $this->assertSame('<div class="text_to_html">lala xx</div>',
+            format_text($text, FORMAT_MOODLE, ['trusted' => false]));
+        $this->assertSame('lala xx',
+            format_text($text, FORMAT_HTML, ['trusted' => false]));
+
+        $CFG->enabletrusttext = 1;
+
+        $this->assertSame(s($text),
+            format_text($text, FORMAT_PLAIN, ['trusted' => true]));
+        $this->assertSame("<p>lala xx</p>\n",
+            format_text($text, FORMAT_MARKDOWN, ['trusted' => true]));
+        $this->assertSame('<div class="text_to_html">lala <object>xx</object></div>',
+            format_text($text, FORMAT_MOODLE, ['trusted' => true]));
+        $this->assertSame('lala <object>xx</object>',
+            format_text($text, FORMAT_HTML, ['trusted' => true]));
+
+        $this->assertSame(s($text),
+            format_text($text, FORMAT_PLAIN, ['trusted' => false]));
+        $this->assertSame("<p>lala xx</p>\n",
+            format_text($text, FORMAT_MARKDOWN, ['trusted' => false]));
+        $this->assertSame('<div class="text_to_html">lala xx</div>',
+            format_text($text, FORMAT_MOODLE, ['trusted' => false]));
+        $this->assertSame('lala xx',
+            format_text($text, FORMAT_HTML, ['trusted' => false]));
+
+        $this->assertSame("<p>lala <object>xx</object></p>\n",
+            format_text($text, FORMAT_MARKDOWN, ['trusted' => true, 'noclean' => true]));
+        $this->assertSame("<p>lala <object>xx</object></p>\n",
+            format_text($text, FORMAT_MARKDOWN, ['trusted' => false, 'noclean' => true]));
+    }
+
    /**
     * @covers ::qualified_me
     */
@@ -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.
@@ -1376,8 +1381,9 @@ function format_text($text, $format = FORMAT_MOODLE, $options = null, $courseidd
            $text = markdown_to_html($text);
            $filteroptions['stage'] = 'pre_clean';
            $text = $filtermanager->filter_text($text, $context, $filteroptions);
-            // The markdown parser does not strip dangerous html so we need to clean it, even if noclean is set to true.
-            $text = clean_text($text, FORMAT_HTML, $options);
+            if (!$options['noclean']) {
+                $text = clean_text($text, FORMAT_HTML, $options);
+            }
            $filteroptions['stage'] = 'post_clean';
            $text = $filtermanager->filter_text($text, $context, $filteroptions);
            break;
@@ -1712,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);
    }
@@ -346,10 +346,7 @@ if (!empty($forum)) {
        $canreplyprivately = forum_user_can_reply_privately($modcontext, $parent);
    }

-    // If markdown is used, the parser does the job already, otherwise clean text from arbitrary code that might be dangerous.
-    if ($post->messageformat != FORMAT_MARKDOWN) {
-        $post = trusttext_pre_edit($post, 'message', $modcontext);
-    }
+    $post = trusttext_pre_edit($post, 'message', $modcontext);

    // Unsetting this will allow the correct return URL to be calculated later.
    unset($SESSION->fromdiscussion);