From f98345b9760a9a66ddc8ba308aa6b46994a6ce27 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 13 Nov 2015 22:17:20 +0800 Subject: [PATCH 1/3] MDL-52126 forum: Correct over-escaping of text-based emails --- .../templates/forum_post_email_textemail.mustache | 12 ++++++------ .../forum_post_emaildigestbasic_textemail.mustache | 2 +- .../forum_post_emaildigestfull_textemail.mustache | 8 ++++---- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/mod/forum/templates/forum_post_email_textemail.mustache b/mod/forum/templates/forum_post_email_textemail.mustache index 6c3fed9897bae..f3f61882a8c95 100644 --- a/mod/forum/templates/forum_post_email_textemail.mustache +++ b/mod/forum/templates/forum_post_email_textemail.mustache @@ -42,17 +42,17 @@ * unsubscribediscussionlink * forumindexlink }} -{{ coursename }} -> {{# str }} forums, forum {{/ str }} -> {{ forumname }}{{# showdiscussionname }} -> {{ discussionname }} {{/ showdiscussionname }} +{{{ coursename }}} -> {{# str }} forums, forum {{/ str }} -> {{{ forumname }}}{{# showdiscussionname }} -> {{{ discussionname }}} {{/ showdiscussionname }} {{ discussionlink }} -{{ subject }} -{{# str }} bynameondate, forum, { "name": "{{ authorfullname }}", "date": "{{ postdate }}" } {{/ str }} +{{{ subject }}} +{{# str }} bynameondate, forum, { "name": "{{{ authorfullname }}}", "date": "{{ postdate }}" } {{/ str }} --------------------------------------------------------------------- -{{ message }} +{{{ message }}} -{{ attachments }} +{{{ attachments }}} --------------------------------------------------------------------- {{# canreply }} -{{# str }} postmailinfolink, forum, { "coursename": "{{ coursename }}", "replylink": "{{ replylink }}" } {{/ str }} +{{# str }} postmailinfolink, forum, { "coursename": "{{{ coursename }}}", "replylink": "{{ replylink }}" } {{/ str }} {{/ canreply }} {{# unsubscribeforumlink }} {{# str }} unsubscribelink, forum, {{{ unsubscribeforumlink }}} {{/ str }} diff --git a/mod/forum/templates/forum_post_emaildigestbasic_textemail.mustache b/mod/forum/templates/forum_post_emaildigestbasic_textemail.mustache index 4d4beb26fa4ed..7f30cd91dd5aa 100644 --- a/mod/forum/templates/forum_post_emaildigestbasic_textemail.mustache +++ b/mod/forum/templates/forum_post_emaildigestbasic_textemail.mustache @@ -33,5 +33,5 @@ }} {{ discussionlink }} -{{ subject }} {{# str }} bynameondate, forum, { "name": "{{ authorfullname }}", "date": "{{ postdate }}" } {{/ str }} +{{{ subject }}} {{# str }} bynameondate, forum, { "name": "{{{ authorfullname }}}", "date": "{{ postdate }}" } {{/ str }} --------------------------------------------------------------------- diff --git a/mod/forum/templates/forum_post_emaildigestfull_textemail.mustache b/mod/forum/templates/forum_post_emaildigestfull_textemail.mustache index 7ef386d08b1a8..deff33e490cad 100644 --- a/mod/forum/templates/forum_post_emaildigestfull_textemail.mustache +++ b/mod/forum/templates/forum_post_emaildigestfull_textemail.mustache @@ -37,11 +37,11 @@ }} {{ discussionlink }} -{{ subject }} ({{{ permalink }}}) -{{# str }} bynameondate, forum, { "name": "{{ authorfullname }}", "date": "{{ postdate }}" } {{/ str }} +{{{ subject }}} ({{{ permalink }}}) +{{# str }} bynameondate, forum, { "name": "{{{ authorfullname }}}", "date": "{{ postdate }}" } {{/ str }} --------------------------------------------------------------------- -{{ message }} +{{{ message }}} -{{ attachments }} +{{{ attachments }}} --------------------------------------------------------------------- {{# str }} digestmailpostlink, forum, {{{ forumindexlink }}} {{/ str }} From 593aaa536676c9d52b997f9bd0b5cff5baa140d6 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Sun, 15 Nov 2015 10:33:19 +0100 Subject: [PATCH 2/3] MDL-52126 forum: Correct over-escaping of html-based emails --- .../templates/forum_post_email_htmlemail.mustache | 10 +++++----- .../forum_post_emaildigestbasic_htmlemail.mustache | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/mod/forum/templates/forum_post_email_htmlemail.mustache b/mod/forum/templates/forum_post_email_htmlemail.mustache index bb32eac2f0140..f354b2333a754 100644 --- a/mod/forum/templates/forum_post_email_htmlemail.mustache +++ b/mod/forum/templates/forum_post_email_htmlemail.mustache @@ -81,14 +81,14 @@
- {{ coursename }} + {{{ coursename }}} » {{# str }} forums, forum {{/ str }} » - {{ forumname }} + {{{ forumname }}} {{# showdiscussionname }} » - {{ discussionname }} + {{{ discussionname }}} {{/ showdiscussionname }}
@@ -99,7 +99,7 @@
- {{ subject }} + {{{ subject }}}
{{# str }} bynameondate, forum, { "name": "{{ authorfullname }}", "date": "{{ postdate }}" } {{/ str }} @@ -118,7 +118,7 @@ {{# attachments }}
- {{ attachments }} + {{{ attachments }}}
{{/ attachments }} {{{ message }}} diff --git a/mod/forum/templates/forum_post_emaildigestbasic_htmlemail.mustache b/mod/forum/templates/forum_post_emaildigestbasic_htmlemail.mustache index 444d4c7c83d31..912b606677b28 100644 --- a/mod/forum/templates/forum_post_emaildigestbasic_htmlemail.mustache +++ b/mod/forum/templates/forum_post_emaildigestbasic_htmlemail.mustache @@ -42,7 +42,7 @@ } }}
- {{ subject }} + {{{ subject }}} {{# str }} bynameondate, forum, { "name": "{{ authorfullname }}", "date": "{{ postdate }}" From 9dbbdcb40af488283985bb0bcd25fa45174375f4 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Sat, 14 Nov 2015 20:06:03 +0100 Subject: [PATCH 3/3] MDL-52126 forum: Verify forum mailout contents New unit test able to verify mail contents configured by provider do match all expectations defined. --- mod/forum/tests/mail_test.php | 244 ++++++++++++++++++++++++++++++++++ 1 file changed, 244 insertions(+) diff --git a/mod/forum/tests/mail_test.php b/mod/forum/tests/mail_test.php index f7946f16fdb73..f8f25e394c196 100644 --- a/mod/forum/tests/mail_test.php +++ b/mod/forum/tests/mail_test.php @@ -833,4 +833,248 @@ public function test_long_subject() { $this->assertEquals($author->id, $message->useridfrom); $this->assertEquals($expectedsubject, $message->subject); } + + /** + * dataProvider for test_forum_post_email_templates(). + */ + public function forum_post_email_templates_provider() { + // Base information, we'll build variations based on it. + $base = array( + 'user' => array('firstname' => 'Love', 'lastname' => 'Moodle', 'mailformat' => 0, 'maildigest' => 0), + 'course' => array('shortname' => '101', 'fullname' => 'Moodle 101'), + 'forums' => array( + array( + 'name' => 'Moodle Forum', + 'forumposts' => array( + array( + 'name' => 'Hello Moodle', + 'message' => 'Welcome to Moodle', + 'messageformat' => FORMAT_MOODLE, + 'attachments' => array( + array( + 'filename' => 'example.txt', + 'filecontents' => 'Basic information about the course' + ), + ), + ), + ), + ), + ), + 'expectations' => array( + array( + 'subject' => '.*101.*Hello', + 'contents' => array( + '~{$a', + '~&(amp|lt|gt|quot|\#039);(?!course)', + 'Attachment example.txt:\n' . + 'http://www.example.com/moodle/pluginfile.php/\d*/mod_forum/attachment/\d*/example.txt\n', + 'Hello Moodle', 'Moodle Forum', 'Welcome.*Moodle', 'Love Moodle', '1\d1' + ), + ), + ), + ); + + // Build the text cases. + $textcases = array('Text mail without ampersands, quotes or lt/gt' => array('data' => $base)); + + // Single and double quotes everywhere. + $newcase = $base; + $newcase['user']['lastname'] = 'Moodle\''; + // $newcase['user']['lastname'] = 'Moodle\'"'; // TODO: This breaks badly. See MDL-52136. + $newcase['course']['shortname'] = '101\''; + // $newcase['course']['shortname'] = '101\'"'; // TODO: This breaks badly. See MDL-52136. + $newcase['forums'][0]['name'] = 'Moodle Forum\'"'; + $newcase['forums'][0]['forumposts'][0]['name'] = 'Hello Moodle\'"'; + $newcase['forums'][0]['forumposts'][0]['message'] = 'Welcome to Moodle\'"'; + $newcase['expectations'][0]['contents'] = array( + 'Attachment example.txt:', '~{\$a', '~&(quot|\#039);', 'Love Moodle\'', '101\'', 'Moodle Forum\'"', + 'Hello Moodle\'"', 'Welcome to Moodle\'"'); + $textcases['Text mail with quotes everywhere'] = array('data' => $newcase); + + // Lt and gt everywhere. This case is completely borked because format_string() + // strips tags with $CFG->formatstringstriptags and also escapes < and > (correct + // for web presentation but not for text email). See MDL-19829. + $newcase = $base; + $newcase['user']['lastname'] = 'Moodle>'; + $newcase['course']['shortname'] = '101>'; + $newcase['forums'][0]['name'] = 'Moodle Forum>'; + $newcase['forums'][0]['forumposts'][0]['name'] = 'Hello Moodle>'; + $newcase['forums'][0]['forumposts'][0]['message'] = 'Welcome to Moodle>'; + $newcase['expectations'][0]['contents'] = array( + 'Attachment example.txt:', '~{\$a', '~&gt;', 'Love Moodle>', '101>', 'Moodle Forum>', + 'Hello Moodle>', 'Welcome to Moodle>'); + $textcases['Text mail with gt and lt everywhere'] = array('data' => $newcase); + + // Ampersands everywhere. This case is completely borked because format_string() + // escapes ampersands (correct for web presentation but not for text email). See MDL-19829. + $newcase = $base; + $newcase['user']['lastname'] = 'Moodle&'; + $newcase['course']['shortname'] = '101&'; + $newcase['forums'][0]['name'] = 'Moodle Forum&'; + $newcase['forums'][0]['forumposts'][0]['name'] = 'Hello Moodle&'; + $newcase['forums'][0]['forumposts'][0]['message'] = 'Welcome to Moodle&'; + $newcase['expectations'][0]['contents'] = array( + 'Attachment example.txt:', '~{\$a', '~&amp;', 'Love Moodle&', '101&', 'Moodle Forum&', + 'Hello Moodle&', 'Welcome to Moodle&'); + $textcases['Text mail with ampersands everywhere'] = array('data' => $newcase); + + // Now the html cases. + $htmlcases = array(); + + // New base for html cases, no quotes, lts, gts or ampersands. + $htmlbase = $base; + $htmlbase['user']['mailformat'] = 1; + $htmlbase['expectations'][0]['contents'] = array( + '~{\$a', + '~&(amp|lt|gt|quot|\#039);(?!course)', + '
( *\n *)?\n.*Hello Moodle', '>Moodle Forum', '>Welcome.*Moodle', '>Love Moodle', '>1\d1'); + $htmlcases['HTML mail without ampersands, quotes or lt/gt'] = array('data' => $htmlbase); + + // Single and double quotes, lt and gt, ampersands everywhere. + $newcase = $htmlbase; + $newcase['user']['lastname'] = 'Moodle\'>&'; + // $newcase['user']['lastname'] = 'Moodle\'">&'; // TODO: This breaks badly. See MDL-52136. + $newcase['course']['shortname'] = '101\'>&'; + // $newcase['course']['shortname'] = '101\'">&'; // TODO: This breaks badly. See MDL-52136. + $newcase['forums'][0]['name'] = 'Moodle Forum\'">&'; + $newcase['forums'][0]['forumposts'][0]['name'] = 'Hello Moodle\'">&'; + $newcase['forums'][0]['forumposts'][0]['message'] = 'Welcome to Moodle\'">&'; + $newcase['expectations'][0]['contents'] = array( + '~{\$a', + '~&(amp|lt|gt|quot|\#039);', + '
( *\n *)?\n.*Hello Moodle\'">&', '>Moodle Forum\'">&', + '>Welcome.*Moodle\'">&', '>Love Moodle&\#039;>&', '>1\d1\'>&'); + $htmlcases['HTML mail with quotes, gt, lt and ampersand everywhere'] = array('data' => $newcase); + + return $textcases + $htmlcases; + } + + /** + * Verify forum emails body using templates to generate the expected results. + * + * @dataProvider forum_post_email_templates_provider + * @param array $data provider samples. + */ + public function test_forum_post_email_templates($data) { + global $DB; + + $this->resetAfterTest(); + + // Create the course, with the specified options. + $options = array(); + foreach ($data['course'] as $option => $value) { + $options[$option] = $value; + } + $course = $this->getDataGenerator()->create_course($options); + + // Create the user, with the specified options and enrol in the course. + $options = array(); + foreach ($data['user'] as $option => $value) { + $options[$option] = $value; + } + $user = $this->getDataGenerator()->create_user($options); + $this->getDataGenerator()->enrol_user($user->id, $course->id); + + // Create forums, always force susbscribed (for easy), with the specified options. + $posts = array(); + foreach ($data['forums'] as $dataforum) { + $forumposts = isset($dataforum['forumposts']) ? $dataforum['forumposts'] : array(); + unset($dataforum['forumposts']); + $options = array('course' => $course->id, 'forcesubscribe' => FORUM_FORCESUBSCRIBE); + foreach ($dataforum as $option => $value) { + $options[$option] = $value; + } + $forum = $this->getDataGenerator()->create_module('forum', $options); + + // Create posts, always for immediate delivery (for easy), with the specified options. + foreach ($forumposts as $forumpost) { + $attachments = isset($forumpost['attachments']) ? $forumpost['attachments'] : array(); + unset($forumpost['attachments']); + $postoptions = array('course' => $course->id, 'forum' => $forum->id, 'userid' => $user->id, + 'mailnow' => 1, 'attachment' => !empty($attachments)); + foreach ($forumpost as $option => $value) { + $postoptions[$option] = $value; + } + list($discussion, $post) = $this->helper_post_to_forum($forum, $user, $postoptions); + $posts[$post->subject] = $post; // Need this to verify cron output. + + // Add the attachments to the post. + if ($attachments) { + $fs = get_file_storage(); + foreach ($attachments as $attachment) { + $filerecord = array( + 'contextid' => context_module::instance($forum->cmid)->id, + 'component' => 'mod_forum', + 'filearea' => 'attachment', + 'itemid' => $post->id, + 'filepath' => '/', + 'filename' => $attachment['filename'] + ); + $fs->create_file_from_string($filerecord, $attachment['filecontents']); + } + $DB->set_field('forum_posts', 'attachment', '1', array('id' => $post->id)); + } + } + } + + // Clear the mailsink and close the messagesink. + // (surely setup should provide us this cleared but...) + $this->helper->mailsink->clear(); + $this->helper->messagesink->close(); + + // Capture and silence cron output, verifying contents. + foreach ($posts as $post) { + $this->expectOutputRegex("/1 users were sent post {$post->id}, '{$post->subject}'/"); + } + forum_cron(); // It's really annoying that we have to run cron to test this. + + // Get the mails. + $mails = $this->helper->mailsink->get_messages(); + + // Start testing the expectations. + $expectations = $data['expectations']; + + // Assert the number is the expected. + $this->assertSame(count($expectations), count($mails)); + + // Start processing mails, first localizing its expectations, then checking them. + foreach ($mails as $mail) { + // Find the corresponding expectation. + $foundexpectation = null; + foreach ($expectations as $key => $expectation) { + // All expectations must have a subject for matching. + if (!isset($expectation['subject'])) { + $this->fail('Provider expectation missing mandatory subject'); + } + if (preg_match('!' . $expectation['subject'] . '!', $mail->subject)) { + // If we already had found the expectation, there are non-unique subjects. Fail. + if (isset($foundexpectation)) { + $this->fail('Multiple expectations found (by subject matching). Please make them unique.'); + } + $foundexpectation = $expectation; + unset($expectations[$key]); + } + } + // Arrived here, we should have found the expectations. + $this->assertNotEmpty($foundexpectation, 'Expectation not found for the mail'); + + // If we have found the expectation and have contents to match, let's do it. + if (isset($foundexpectation) and isset($foundexpectation['contents'])) { + if (!is_array($foundexpectation['contents'])) { // Accept both string and array. + $foundexpectation['contents'] = array($foundexpectation['contents']); + } + foreach ($foundexpectation['contents'] as $content) { + if (strpos($content, '~') !== 0) { + $this->assertRegexp('#' . $content . '#m', $mail->body); + } else { + $this->assertNotRegexp('#' . substr($content, 1) . '#m', $mail->body); + } + } + } + } + // Finished, there should not be remaining expectations. + $this->assertCount(0, $expectations); + } }