Sendgrid mailer + metamta.differential.attach-patches=true errors with "The attachment type cannot contain ';', or CRLF characters"

I updated a 2y old Phabricator instance (went really smooth on a 1TB MySQL instance, impressed!) and I was trying to reconfigure it to use the cluster.mailers setting (from phpmailer.mailer=smtp).

$ cat <<EOF  | ./bin/config set --stdin cluster.mailers 
[
  {
    "key": "my-sendgrid",
    "type": "sendgrid",
    "options": {
      "api-key": "<key>"
    }
  }
]
EOF

I also have the metamta.differential.attach-patches setting set to true. I tried to create a new revision but the SendGrid API errors with HTTP/400:
{"errors":[{"message":"The attachment type cannot contain ';', or CRLF characters.","field":"attachments.0.type","help":"http://sendgrid.com/docs/API_Reference/Web_API_v3/Mail/errors.html#message.attachments.type"}]}

Here is the dump of the json data sent to the SendGrid API

{"subject":"[Differential] D84090: Test revision5 (ignore)","personalizations":[{"to":[{"email":"<myemail>@gmail.com"}]}],"from":{"email":"<phab email>","name":"<my name>"},"reply_to":{
"email":"reviews+D84090+<....>@domain.com","name":"<my name>"},"headers":{"X-Phabricator-Sent-This-Message":"Yes","X-Mail-Transport-Agent":"MetaMTA","X-Auto-Response-Suppress":"All","X-Phabricator-Mail-Tags":"<differential-other>, <different
ial-review-request>","X-Herald-Rules":"none, <170>","X-Phabricator-To":"<PHID-USER-sm6w647xcfduguhpi7dn>","X-Phabricator-Cc":"<PHID-USER-dlhodtazs2k7ydj6hmer>","Precedence":"bulk","Thread-Topic":"PHID-DREV-uv7mkd7ssua6c5wtmccj","X-Phabricator-Mail-ID":"1017343","X-P
habricator-Send-Attempt":"7iyr24qckefg265t","In-Reply-To":"<differential-rev-PHID-DREV-uv7mkd7ssua6c5wtmccj-req@reviews.llvm.org>","References":"<differential-rev-PHID-DREV-uv7mkd7ssua6c5wtmccj-req@reviews.llvm.org>","Thread-Index":"OTY1MzA0ZGQzYjZlOTJlMjIxZjFlODg4N
zUwIF8VHaU=","X-Phabricator-Stamps":"actor(@<myusername>) application(Differential) author(@<myusername>) herald(H170) monogram(D84090) object-type(DREV) phid(PHID-DREV-uv7mkd7ssua6c5wtmccj) revision-status(needs-review) via(web)"},"content":[{"type":"text\/plain","va
lue":"<myusername> created this revision.\n<myusername> requested review of this revision.\n\n\n  https:\/\/domain.com\/D84090\n\nFiles:\n  empty\n\n\n\nIndex: empty\n===================================================================\n--- empty\n+++ empty\n@@ -
1 +1 @@\n-empty\n+not empty\n\n\n"}],"attachments":[{"content":"SW5kZXg6IGVtcHR5Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIGVtcHR5CisrKyBlbXB0eQpAQCAtMSArMSBAQAotZW1wdHkKK25vdCBlbXB0eQo=","type":"text\/x-patch; charset=utf-8","
filename":"D84090.278987.patch","disposition":"attachment"}]}

The following patch is solving the problem, but I’m not sure if it is the right fix and what are the possible side-effects:

diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php
index 98c0d92624..5a61beb1dc 100644
--- a/src/applications/audit/editor/PhabricatorAuditEditor.php
+++ b/src/applications/audit/editor/PhabricatorAuditEditor.php
@@ -636,7 +636,7 @@ final class PhabricatorAuditEditor
       new PhabricatorMailAttachment(
         $raw_patch,
         $commit_name.'.patch',
-        'text/x-patch; charset='.$encoding));
+        'text/x-patch')); //; charset='.$encoding));
   }
 
   private function inlinePatch(
diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index 3967344405..97d32c78fa 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -724,7 +724,7 @@ final class DifferentialTransactionEditor
           // major concrete problems that lackluster email size limits cause.
           if (strlen($patch) < $body_limit) {
             $name = pht('D%s.%s.patch', $object->getID(), $diff->getID());
-            $mime_type = 'text/x-patch; charset=utf-8';
+            $mime_type = 'text/x-patch'; //; charset=utf-8';
             $body->addAttachment(
               new PhabricatorMailAttachment($patch, $name, $mime_type));
           }

Version Info

Phabricator Version Information

Library Version Date Branchpoint
phabricator d32c3f8d7d65 Fri, Jul 17
arcanist 2565cc7b4d1d Sat, Jul 11

Other Version Information

Binary Version Path
php 7.2.24-0ubuntu0.18.04.6 fpm-fcgi
diff 3.6 /usr/bin/diff
git 2.17.1 /usr/bin/git
hg Not Available
pygmentize 2.2.0 /usr/bin/pygmentize
svn 1.9.7 /usr/bin/svn

https://github.com/sendgrid/sendgrid-nodejs/issues/217#issuecomment-396649268 seems to indicate your change is correct for what SendGrid wants, but a proper fix would need targeting at sendgrid specifically in the mail adaptor level. There shouldn’t be any negative side effects of your change as long as you only use the sendgrid mailer. It seems weird they don’t just handle this on their end as they do the REQUEST= one mentioned in that issue, even if just to ignore the charset.

To be sure I understand, are you suggesting patching it here instead: https://github.com/phacility/phabricator/blob/master/src/applications/metamta/adapter/PhabricatorMailSendGridAdapter.php#L108

Something like:

-          'type' => $attachment->getMimeType(),
+          'type' => explode(";", $attachment->getMimeType())[0],

Yeah, that’s the right place. Something like that looks good enough for a local patch, although something polished would probably want at least some white space handling, and probably to throw if a caller tried to provide a non-UTF-8 charset (rather than risking SendGrid interpreting it incorrectly.)