Page MenuHomePhabricator

Long list of reviewers breaks to: field when set to false
Closed, ResolvedPublic



phabricator 92d520cdc2160b95f03b89c10ebf0c9edaf7c547 (Mon, Feb 27)
arcanist 467dc3ae4286df6d42730d472b7b52d3afa33ec5 (Mon, Feb 27)
phutil 6c902414c30dfaad64f24b3bc950857cebbcf8fe (Mon, Feb 27)
libtwitter 65e64977fd93383256ed3a0292aaf9ef1502bb9c (Wed, Feb 15)

Reproduction steps:

  1. Set to false
  2. Create a revision and add hundreds of reviewers (enough such that their emails exceeds 989 characters)


To: field in revision emails contains a comma separated list of email addresses for all reviewers, with every email correctly formatted


To: field in revision emails contains comma separated list of email addresses, but adds a newline every 989 characters. So at around the 989 character mark, the to: field looks something like

...,, sba,, ...

We can privately send you a raw email example if that helps. This is consistently happening to revisions with a large number of reviewers.

Event Timeline

See also T12046 for why all open source is sketchy voodoo and the eventual pathway forward here is likely replacing it with first-party code.

I guess this technically qualifies as a scalability bug so we'll prioritize resolving it.

What is metamta.mail-adapter set to?

The default -- PhabricatorMailImplementationPHPMailerLiteAdapter

I think the actual issue is that we're not wrapping the header. When I generate a synthetic message locally with a "To:" header with a length of ~50,000 bytes, the raw output is an unbroken line ~50,00 bytes long. I suspect this is RFC-violating and that whatever the mail gets handed to next gets upset by it.

Header construction in PHPMailer is fairly ad-hoc and I think some headers just don't get wrapped.

I'm going to try to fix this very narrowly. I think we're headed toward severing this dependency, but that's a substantial amount of work.

There are also various issues here if a single address is more than 1,000 bytes in length but presumably that's not much of an issue in practice.

I think this is the controlling specification -- 2008 was a simpler time when no sequence of non-whitespace characters could ever be longer than 78 bytes, and a thousand bytes of memory was an untold wealth of riches:

Can you test if this fixes the issue in your environment?

diff --git a/externals/phpmailer/class.phpmailer-lite.php b/externals/phpmailer/class.phpmailer-lite.php
index 92601178a5..adde2ea3cf 100644
--- a/externals/phpmailer/class.phpmailer-lite.php
+++ b/externals/phpmailer/class.phpmailer-lite.php
@@ -662,6 +662,10 @@ class PHPMailerLite {
     $addr_str .= implode(', ', $addresses);
     $addr_str .= $this->LE;
+    // NOTE: This is a narrow hack to fix an issue with 1000+ characters of
+    // recipients, described in T12372.
+    $addr_str = wordwrap($addr_str, 75, "\n ");
     return $addr_str;

Internally, PHPMailer sends headers through two adjustment phases:

  • SecureHeader(), which makes headers "secure" by silently discarding newlines. I'll set this aside for now.
  • EncodeHeader(), which encodes and, if necessary, wraps headers, some of the time (but not always: see

Among many others, the narrowest issue here is that PHPMailer calls EncodeHeader() on each address individually for headers containing lists of email addresses, so the wrapping logic never applies to the header (with the full list of addresses) as a whole.

The patch above wraps those headers after they are built, approximately using the folding algorithm described in RFC5322 Section 2.2.3. wordwrap() is not correct exhaustively, but should produce correct-enough output for reasonable inputs here, I believe.

Thanks for the quick fix and the huge amount of detail under the hood, we'll give that patch a shot!

And agreed, an address exceeding 1000 bytes in length shouldn't be much of an issue in practice. I'm guessing there would be other worse symptoms if we had any of those sorts of big addresses.

Any suggestions to test this on staging without mass emailing everyone / patching prod? I am trying to test this on staging with bin/mail show-outbound --id xxxx (staging has mail disabled), but the recipients / To: list look fine with or without the patch.

Hrrm -- I can adjust bin/mail send-test to accept raw email addresses, and then you can use it to send a message to, and similar. Shouldn't be too involved.

After D17489 you should be able to do this:

echo "mail body" | ./bin/mail send-test --subject hi --to --to ...

...until you've added more than 1,000 characters of fake recipients, then maybe put a real address at the end and see if it arrives there correctly if you don't have a better way to tell.

You still probably have to patch production if staging isn't hooked up to an MTA (and can't easily be connected to one), but at least you don't have to mail a ton of people.

Confirming that the patch above works (tested using send-test --to)!

Before patch:

To:,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, ".com" <fakeemailtest+32@twitter>,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, f <>,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, fakeemai <>,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, "fakeemailtest+" <>,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, "" <fakeemailtest+163@t>,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, "" <fakeemailtest+195@twitte>,,,,

After patch:


Great, thanks for testing it! We'll upstream that and I'll file something to put a longer-term fix in place.

"" <fakeemailtest+163@t>

ah yes this is my very favorite email address of all

Great, thanks for testing it! We'll upstream that and I'll file something to put a longer-term fix in place.

"" <fakeemailtest+163@t>

ah yes this is my very favorite email address of all