Skip to content

Commit

Permalink
Merge branch 'MDL-78505/401' of https://github.com/skodak/moodle into…
Browse files Browse the repository at this point in the history 
… MOODLE_401_STABLE
  • Loading branch information
@junpataleta
junpataleta committed Aug 15, 2023
2 parents baed2a1 + 3847e0b commit 152acdd
Show file tree
Hide file tree
Showing 5 changed files with 300 additions and 8 deletions.
10 changes: 8 additions & 2 deletions lib/filelib.php
View file
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
Expand Up @@ -1906,6 +1906,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
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
16 changes: 14 additions & 2 deletions lib/weblib.php
View file
Expand Up @@ -1264,6 +1264,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 @@ -1365,8 +1370,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;
Expand Down Expand Up @@ -1701,6 +1707,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
5 changes: 1 addition & 4 deletions mod/forum/post.php
View file
Expand Up @@ -343,10 +343,7 @@
$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);
Expand Down

0 comments on commit 152acdd

Please sign in to comment.