From 6011085b0fcd7bdcdeb422bd73571a79584c5c8f Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 Jun 2018 09:39:00 -0700 Subject: [PATCH] Respect "metamta.email-body-limit" when building mail HTML bodies Summary: Ref T13151. See T11767. See PHI686. Although we limit outbound mail text bodies, the limit doesn't currently apply to attachments, HTML bodies, or headers. T11767 discusses improving this in the general case. In the wild, an install hit an issue (see PHI686) where edits to Phriction pages generate very large HTML bodies. Check and respect the limit when building HTML bodies. If we don't have enough room for the HTML body, we just drop it. We have the text body to fall back to, and HTML is difficult to truncate safely. Test Plan: Added unit tests and made them pass. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13151 Differential Revision: https://secure.phabricator.com/D19489 --- ...abricatorMailImplementationTestAdapter.php | 8 ++ .../storage/PhabricatorMetaMTAMail.php | 21 +++-- .../PhabricatorMetaMTAMailTestCase.php | 80 +++++++++++++++++++ 3 files changed, 103 insertions(+), 6 deletions(-) diff --git a/src/applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php b/src/applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php index 8a8d0de0c2..80807398f1 100644 --- a/src/applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php @@ -123,4 +123,12 @@ final class PhabricatorMailImplementationTestAdapter return $this; } + public function getBody() { + return idx($this->guts, 'body'); + } + + public function getHTMLBody() { + return idx($this->guts, 'html-body'); + } + } diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index d759ef6583..c2435499af 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -892,15 +892,16 @@ final class PhabricatorMetaMTAMail $body = $raw_body; } - $max = PhabricatorEnv::getEnvConfig('metamta.email-body-limit'); - if (strlen($body) > $max) { + $body_limit = PhabricatorEnv::getEnvConfig('metamta.email-body-limit'); + if (strlen($body) > $body_limit) { $body = id(new PhutilUTF8StringTruncator()) - ->setMaximumBytes($max) + ->setMaximumBytes($body_limit) ->truncateString($body); $body .= "\n"; - $body .= pht('(This email was truncated at %d bytes.)', $max); + $body .= pht('(This email was truncated at %d bytes.)', $body_limit); } $mailer->setBody($body); + $body_limit -= strlen($body); // If we sent a different message body than we were asked to, record // what we actually sent to make debugging and diagnostics easier. @@ -914,8 +915,16 @@ final class PhabricatorMetaMTAMail $send_html = $this->shouldSendHTML($preferences); } - if ($send_html && isset($params['html-body'])) { - $mailer->setHTMLBody($params['html-body']); + if ($send_html) { + $html_body = idx($params, 'html-body'); + + // NOTE: We just drop the entire HTML body if it won't fit. Safely + // truncating HTML is hard, and we already have the text body to fall + // back to. + if (strlen($html_body) <= $body_limit) { + $mailer->setHTMLBody($html_body); + $body_limit -= strlen($html_body); + } } // Pass the headers to the mailer, then save the state so we can show diff --git a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php index c0045301fd..d20a28fc15 100644 --- a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php +++ b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php @@ -331,4 +331,84 @@ final class PhabricatorMetaMTAMailTestCase extends PhabricatorTestCase { $this->assertEqual(null, $mail->getMailerKey()); } + public function testMailSizeLimits() { + $env = PhabricatorEnv::beginScopedEnv(); + $env->overrideEnvConfig('metamta.email-body-limit', 1024 * 512); + + $user = $this->generateNewTestUser(); + $phid = $user->getPHID(); + + $string_1kb = str_repeat('x', 1024); + $html_1kb = str_repeat('y', 1024); + $string_1mb = str_repeat('x', 1024 * 1024); + $html_1mb = str_repeat('y', 1024 * 1024); + + // First, send a mail with a small text body and a small HTML body to make + // sure the basics work properly. + $mail = id(new PhabricatorMetaMTAMail()) + ->addTos(array($phid)) + ->setBody($string_1kb) + ->setHTMLBody($html_1kb); + + $mailer = new PhabricatorMailImplementationTestAdapter(); + $mail->sendWithMailers(array($mailer)); + $this->assertEqual( + PhabricatorMailOutboundStatus::STATUS_SENT, + $mail->getStatus()); + + $text_body = $mailer->getBody(); + $html_body = $mailer->getHTMLBody(); + + $this->assertEqual($string_1kb, $text_body); + $this->assertEqual($html_1kb, $html_body); + + + // Now, send a mail with a large text body and a large HTML body. We expect + // the text body to be truncated and the HTML body to be dropped. + $mail = id(new PhabricatorMetaMTAMail()) + ->addTos(array($phid)) + ->setBody($string_1mb) + ->setHTMLBody($html_1mb); + + $mailer = new PhabricatorMailImplementationTestAdapter(); + $mail->sendWithMailers(array($mailer)); + $this->assertEqual( + PhabricatorMailOutboundStatus::STATUS_SENT, + $mail->getStatus()); + + $text_body = $mailer->getBody(); + $html_body = $mailer->getHTMLBody(); + + // We expect the body was truncated, because it exceeded the body limit. + $this->assertTrue( + (strlen($text_body) < strlen($string_1mb)), + pht('Text Body Truncated')); + + // We expect the HTML body was dropped completely after the text body was + // truncated. + $this->assertTrue( + !strlen($html_body), + pht('HTML Body Removed')); + + + // Next send a mail with a small text body and a large HTML body. We expect + // the text body to be intact and the HTML body to be dropped. + $mail = id(new PhabricatorMetaMTAMail()) + ->addTos(array($phid)) + ->setBody($string_1kb) + ->setHTMLBody($html_1mb); + + $mailer = new PhabricatorMailImplementationTestAdapter(); + $mail->sendWithMailers(array($mailer)); + $this->assertEqual( + PhabricatorMailOutboundStatus::STATUS_SENT, + $mail->getStatus()); + + $text_body = $mailer->getBody(); + $html_body = $mailer->getHTMLBody(); + + $this->assertEqual($string_1kb, $text_body); + $this->assertTrue(!strlen($html_body)); + } + }