From c4f51bfd31ee8092e5bd1b08869001da267a964f Mon Sep 17 00:00:00 2001 From: fujiwara Date: Mon, 4 May 2020 21:43:57 +0900 Subject: [PATCH 1/2] MDL-68415 email: Fix attachment in email_to_user() --- lib/moodlelib.php | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/lib/moodlelib.php b/lib/moodlelib.php index 6d4a2be8803..d73f9145d73 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -6276,12 +6276,31 @@ function email_to_user($user, $from, $subject, $messagetext, $messagehtml = '', // Before doing the comparison, make sure that the paths are correct (Windows uses slashes in the other direction). $attachpath = str_replace('\\', '/', $attachmentpath); - // Make sure both variables are normalised before comparing. - $temppath = str_replace('\\', '/', realpath($CFG->tempdir)); - // If the attachment is a full path to a file in the tempdir, use it as is, + // Add allowed paths to an array (also check if it's not empty). + $allowedpaths = array_filter([ + $CFG->cachedir, + $CFG->dataroot, + $CFG->dirroot, + $CFG->localcachedir, + $CFG->tempdir + ]); + // Set addpath to true. + $addpath = true; + // Check if attachment includes one of the allowed paths. + foreach ($allowedpaths as $tmpvar) { + // Make sure both variables are normalised before comparing. + $temppath = str_replace('\\', '/', realpath($tmpvar)); + // Set addpath to false if the attachment includes one of the allowed paths. + if (strpos($attachpath, $temppath) === 0) { + $addpath = false; + break; + } + } + + // If the attachment is a full path to a file in the multiple allowed paths, use it as is, // otherwise assume it is a relative path from the dataroot (for backwards compatibility reasons). - if (strpos($attachpath, $temppath) !== 0) { + if ($addpath == true) { $attachmentpath = $CFG->dataroot . '/' . $attachmentpath; } From f4488eca79f791f226b99f049ccdd46f1245faf5 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Wed, 6 May 2020 21:08:15 +0100 Subject: [PATCH 2/2] MDL-68415 core: test attachments in email_to_user from allowed paths. --- lib/moodlelib.php | 3 +- lib/tests/moodlelib_test.php | 84 ++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/lib/moodlelib.php b/lib/moodlelib.php index d73f9145d73..360ad767ab4 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -6007,7 +6007,8 @@ function generate_email_messageid($localpart = null) { * @param string $subject plain text subject line of the email * @param string $messagetext plain text version of the message * @param string $messagehtml complete html version of the message (optional) - * @param string $attachment a file on the filesystem, either relative to $CFG->dataroot or a full path to a file in $CFG->tempdir + * @param string $attachment a file on the filesystem, either relative to $CFG->dataroot or a full path to a file in one of + * the following directories: $CFG->cachedir, $CFG->dataroot, $CFG->dirroot, $CFG->localcachedir, $CFG->tempdir * @param string $attachname the name of the file (extension indicates MIME) * @param bool $usetrueaddress determines whether $from email address should * be sent out. Will be overruled by user profile setting for maildisplay diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php index 9922acba646..fbfbbb6850d 100644 --- a/lib/tests/moodlelib_test.php +++ b/lib/tests/moodlelib_test.php @@ -3385,6 +3385,90 @@ class core_moodlelib_testcase extends advanced_testcase { $sink->close(); } + /** + * Data provider for {@see test_email_to_user_attachment} + * + * @return array + */ + public function email_to_user_attachment_provider(): array { + global $CFG; + + // Return all paths that can be used to send attachments from. + return [ + 'cachedir' => [$CFG->cachedir], + 'dataroot' => [$CFG->dataroot], + 'dirroot' => [$CFG->dirroot], + 'localcachedir' => [$CFG->localcachedir], + 'tempdir' => [$CFG->tempdir], + // Pass null to indicate we want to test a path relative to $CFG->dataroot. + 'relative' => [null] + ]; + } + + /** + * Test sending attachments with email_to_user + * + * @param string|null $filedir + * + * @dataProvider email_to_user_attachment_provider + */ + public function test_email_to_user_attachment(?string $filedir): void { + global $CFG; + + // If $filedir is null, then write our test file to $CFG->dataroot. + $filepath = ($filedir ?: $CFG->dataroot) . '/hello.txt'; + file_put_contents($filepath, 'Hello'); + + $user = core_user::get_support_user(); + $message = 'Test attachment path'; + + // Create sink to catch all sent e-mails. + $sink = $this->redirectEmails(); + + // Attachment path will be that of the test file if $filedir was passed, otherwise the relative path from $CFG->dataroot. + $filename = basename($filepath); + $attachmentpath = $filedir ? $filepath : $filename; + email_to_user($user, $user, $message, $message, $message, $attachmentpath, $filename); + + $messages = $sink->get_messages(); + $sink->close(); + + $this->assertCount(1, $messages); + + // Verify attachment in message body (attachment is in MIME format, but we can detect some Content fields). + $messagebody = reset($messages)->body; + $this->assertContains('Content-Type: text/plain; name="' . $filename . '"', $messagebody); + $this->assertContains('Content-Disposition: attachment; filename=' . $filename, $messagebody); + + // Cleanup. + unlink($filepath); + } + + /** + * Test sending an attachment that doesn't exist to email_to_user + */ + public function test_email_to_user_attachment_missing(): void { + $user = core_user::get_support_user(); + $message = 'Test attachment path'; + + // Create sink to catch all sent e-mails. + $sink = $this->redirectEmails(); + + $attachmentpath = '/hola/hello.txt'; + $filename = basename($attachmentpath); + email_to_user($user, $user, $message, $message, $message, $attachmentpath, $filename); + + $messages = $sink->get_messages(); + $sink->close(); + + $this->assertCount(1, $messages); + + // Verify attachment not in message body (attachment is in MIME format, but we can detect some Content fields). + $messagebody = reset($messages)->body; + $this->assertNotContains('Content-Type: text/plain; name="' . $filename . '"', $messagebody); + $this->assertNotContains('Content-Disposition: attachment; filename=' . $filename, $messagebody); + } + /** * Test setnew_password_and_mail. */