From 989636b0defd4c80cefcd6640ce990b0515874b2 Mon Sep 17 00:00:00 2001
From: Petr Skoda <commits@skodak.org>
Date: Mon, 7 Aug 2023 09:28:14 +0200
Subject: [PATCH] MDL-78505 core: stop mangling existing Mardown in text
 editors

The problem is that HTML Purifier is not compatible with Markdown,
that means we cannot sanitise Markdown texts before editing.

Luckily Markdown has to use plain text editor which does not have
XSS problems.

The only tiny downside is that Markdown cannot be allowed
in "trust text" areas any more.
---
 lib/filelib.php            |  10 +-
 lib/tests/filelib_test.php |  79 +++++++++++++++
 lib/tests/weblib_test.php  | 198 +++++++++++++++++++++++++++++++++++++
 lib/weblib.php             |  11 +++
 4 files changed, 296 insertions(+), 2 deletions(-)

diff --git a/lib/filelib.php b/lib/filelib.php
index a0ea6642e70..7c50999b30e 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 02c175edcf7..32dc34535c9 100644
--- a/lib/tests/filelib_test.php
+++ b/lib/tests/filelib_test.php
@@ -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);
+    }
 }
 
 /**
diff --git a/lib/tests/weblib_test.php b/lib/tests/weblib_test.php
index c390b8518d2..f34b81e5066 100644
--- a/lib/tests/weblib_test.php
+++ b/lib/tests/weblib_test.php
@@ -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
      */
diff --git a/lib/weblib.php b/lib/weblib.php
index 4c97f788820..616caf29e04 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);
     }