Skip to content

Commit

Permalink
MDL-78505 core: stop mangling existing Mardown in text editors
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@skodak
skodak committed Aug 15, 2023
1 parent a50c2d4 commit 989636b
Show file tree
Hide file tree
Showing 4 changed files with 296 additions and 2 deletions.
10 changes: 8 additions & 2 deletions lib/filelib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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;
Expand Down
79 changes: 79 additions & 0 deletions lib/tests/filelib_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <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);
}
}

/**
Expand Down
198 changes: 198 additions & 0 deletions lib/tests/weblib_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<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
*/
Expand Down
11 changes: 11 additions & 0 deletions lib/weblib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
Expand Down

0 comments on commit 989636b

Please sign in to comment.