{"thread":{"id":"44415","subject":"send-email garbled header with trailing doublequote in email","startedAt":"2016-11-02T20:27:17Z","lastAt":"2016-11-04T00:11:18Z","messageCount":9,"participants":["Andrea Arcangeli","Jeff King","demerphq"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"305315","messageId":"20161102202709.GI4611@redhat.com","threadId":"44415","inReplyTo":null,"subject":"send-email garbled header with trailing doublequote in email","fromName":"Andrea Arcangeli","fromEmail":"aarcange@redhat.com","sentAt":"2016-11-02T20:27:09Z","receivedAt":"2016-11-02T20:27:17Z","isPatch":false,"sender":{"key":"aarcange@redhat.com","avatar":null},"body":"Hello,\n\nsend-email gets confused with one trailing \" at the end of the\nemail. Details and how to reproduce below, it breaks also with\nupstream git version 2.10.2.dirty.\n\nFeel free to CC me if you need any further info.\n\nThanks,\nAndrea\n\n----- Forwarded message from Andrea Arcangeli -----\n\nDate: Wed, 2 Nov 2016 21:07:02 +0100\nFrom: Andrea Arcangeli \nTo: linux-mm@kvack.org\nSubject: Re: [PATCH 00/33] userfaultfd tmpfs/hugetlbfs/non-cooperative\nUser-Agent: Mutt/1.7.1 (2016-10-04)\n\nFYI: apparently I hit a git bug in this submit... reproducible with\nthe below command:\n\ngit send-email -1 --to '\"what ever\" <--your--@--email--.com>\"'\n\nafter replacing --your--@email--.com with your own email.\n\n*snip*\nDry-OK. Log says:\nTo: \"what ever\" \" <--your--@--email--.com>\n*snip*\nX-Mailer: git-send-email 2.7.3\n\nResult: OK\n\nIt's not ok if the --dry-run outputs the above with a fine header, but\nthe actual header in the email data is different. Of course I tested\n--dry-run twice and it was fine like the above is fine as well.\n\n*snip*\n\n----- End forwarded message -----\n"},{"id":"305318","messageId":"20161102211118.sc4j3fezfqxg23i3@sigill.intra.peff.net","threadId":"44415","inReplyTo":"20161102202709.GI4611@redhat.com","subject":"Re: send-email garbled header with trailing doublequote in email","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-02T21:11:18Z","receivedAt":"2016-11-02T21:11:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 02, 2016 at 09:27:09PM +0100, Andrea Arcangeli wrote:\n\n> send-email gets confused with one trailing \" at the end of the\n> email. Details and how to reproduce below, it breaks also with\n> upstream git version 2.10.2.dirty.\n\nI'm not quite sure what happened, and what you wanted to happen. In your\nexample:\n\n> FYI: apparently I hit a git bug in this submit... reproducible with\n> the below command:\n> \n> git send-email -1 --to '\"what ever\" <--your--@--email--.com>\"'\n\nThe \"to\" address is slightly bogus here because of the extra\ndouble-quote. That gets turned into a slightly bogus rfc2822 header:\n\n> *snip*\n> Dry-OK. Log says:\n> To: \"what ever\" \" <--your--@--email--.com>\n\nWhich is funny, but matches what we do with other addresses that are\ninvalid according to the rfc. E.g., there was a discussion recently on:\n\n Stable [4.8+]\n\nwhich has historically been converted to:\n\n \"Stable [4.8+]\" \n\nIn fact, it is not even git that does this, but rather what\nMail::Address happens to output (though git has fallback routines if\nthat module isn't available that do the same thing).\n\nBut in either case, in my test, the actual email address is still\nextracted correctly and fed to the MTA, so the mail is delivered.\n\nSo I'm not sure what you wanted to happen that didn't. Did you want\n--dry-run to complain about the bogus address? Did the message not get\ndelivered for you? Something else?\n\n-Peff\n"},{"id":"305319","messageId":"20161102213805.GJ4611@redhat.com","threadId":"44415","inReplyTo":"20161102211118.sc4j3fezfqxg23i3@sigill.intra.peff.net","subject":"Re: send-email garbled header with trailing doublequote in email","fromName":"Andrea Arcangeli","fromEmail":"aarcange@redhat.com","sentAt":"2016-11-02T21:38:05Z","receivedAt":"2016-11-02T21:38:13Z","isPatch":false,"sender":{"key":"aarcange@redhat.com","avatar":null},"body":"Hello,\n\nOn Wed, Nov 02, 2016 at 05:11:18PM -0400, Jeff King wrote:\n> In fact, it is not even git that does this, but rather what\n> Mail::Address happens to output (though git has fallback routines if\n> that module isn't available that do the same thing).\n\nIf it's something in perl it should be fixed there indeed. I didn't\ndebug what exactly was wrong, so I sent it here first.\n\n> But in either case, in my test, the actual email address is still\n> extracted correctly and fed to the MTA, so the mail is delivered.\n\nThe email is delivered to the right email for me too, but to see the\nproblem you need to check the email itself, and look how the To: field\nlooks in the actual email in your mail client or web interface.\n\nDon't you see whatever@yourhostname without spaces and @yourhostname\ninstead of the email domain?\n\nIf you still can't reproduce, maybe it's a different perl\nMail::Address, I'm using dev-perl/MailTools-2.140.0\n\nFrom who receives the email it just looks garbled, but --dry-run\nshowed a correct To/Cc list so it was undetectable that it would end\nup garbled during the real email delivery.\n\n> So I'm not sure what you wanted to happen that didn't. Did you want\n> --dry-run to complain about the bogus address? Did the message not get\n> delivered for you? Something else?\n\nIf --dry-run complained and failed instead of passing and then sending\nan email with garbled To/Cc list, it'd be more than enough. Either\nthat or it should generate a mail header that matches the output of\n--dry-run so the review of --dry-run becomes meaningful again.\n\nThanks,\nAndrea\n"},{"id":"305321","messageId":"20161102220437.5ewzezxs2nasyouv@sigill.intra.peff.net","threadId":"44415","inReplyTo":"20161102213805.GJ4611@redhat.com","subject":"Re: send-email garbled header with trailing doublequote in email","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-02T22:04:37Z","receivedAt":"2016-11-02T22:04:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 02, 2016 at 10:38:05PM +0100, Andrea Arcangeli wrote:\n\n> > But in either case, in my test, the actual email address is still\n> > extracted correctly and fed to the MTA, so the mail is delivered.\n> \n> The email is delivered to the right email for me too, but to see the\n> problem you need to check the email itself, and look how the To: field\n> looks in the actual email in your mail client or web interface.\n> \n> Don't you see whatever@yourhostname without spaces and @yourhostname\n> instead of the email domain?\n\nNope, it looks exactly as --dry-run reports it.\n\nTo see exactly what is being sent out, try:\n\n-- >8 --\n\ncat >/tmp/foo <<\\EOF\n#!/bin/sh\necho \"args: $*\"\nsed 's/^/stdin: /'\nEOF\n\nchmod +x /tmp/foo\n\ngit send-email --smtp-server=/tmp/foo --to=whatever\n\n-- 8< --\n\nMine shows the same header as --dry-run. Which makes sense, because the\ncode is literally just dumping the $header variable, which is the same\nthing that gets sent to the sendmail binary (or the SMTP server if that\nis in use).\n\nSo my guess would be that either an MTA in the routing path is garbling\nit (or possibly mailing list software). or maybe even the eventual MUA,\nthough it sounds like you checked the raw bytes.\n\n> If you still can't reproduce, maybe it's a different perl\n> Mail::Address, I'm using dev-perl/MailTools-2.140.0\n\nMine is 2.13, which I would imagine is comparable.\n\n> If --dry-run complained and failed instead of passing and then sending\n> an email with garbled To/Cc list, it'd be more than enough. Either\n> that or it should generate a mail header that matches the output of\n> --dry-run so the review of --dry-run becomes meaningful again.\n\nOK. I think we get that first part right. The problem is that the\ngarbling is such that somebody in the middle is unhappy with it (which\nmakes sense; it's syntactically bogus). So the tool is there to see the\nproblem in --dry-run, but of course it's rather subtle.\n\nIn theory we should be able to detect and complain about bogus syntax\nlike this, but right now we don't re-parse the addresses at all. We rely\non Mail::Address to produce valid output, and it doesn't seem to be\ndoing so here.\n\n-Peff\n"},{"id":"305324","messageId":"20161102222901.GK4611@redhat.com","threadId":"44415","inReplyTo":"20161102220437.5ewzezxs2nasyouv@sigill.intra.peff.net","subject":"Re: send-email garbled header with trailing doublequote in email","fromName":"Andrea Arcangeli","fromEmail":"aarcange@redhat.com","sentAt":"2016-11-02T22:29:01Z","receivedAt":"2016-11-02T22:29:08Z","isPatch":false,"sender":{"key":"aarcange@redhat.com","avatar":null},"body":"On Wed, Nov 02, 2016 at 06:04:37PM -0400, Jeff King wrote:\n> Nope, it looks exactly as --dry-run reports it.\n\nMy sendmail is postfix 3.1.0.\n\n> To see exactly what is being sent out, try:\n> \n> -- >8 --\n> \n> cat >/tmp/foo <<\\EOF\n> #!/bin/sh\n> echo \"args: $*\"\n> sed 's/^/stdin: /'\n> EOF\n> \n> chmod +x /tmp/foo\n> \n> git send-email --smtp-server=/tmp/foo --to=whatever\n> \n> -- 8< --\n\nRight it's the same as --dry-run:\n\nstdin: To: \"what ever\" \" <.....>\n\nThere's not my hostname and not removed space. If I add more addresses\nthey also go in the second line with a leading space and they're not cut.\n\nSo this must be postfix then that out of the blue decided to garble it\nin a strange way while parsing the input... The removal of all\nwhitespaces s/what ever/whatever/ especially I've no idea how it\ndecided to do so.\n\nCan you reproduce with postfix as sendmail at least? If you can\nreproduce also see what happens if you add another --to.\n"},{"id":"305344","messageId":"20161103141848.42pg6iow24prign5@sigill.intra.peff.net","threadId":"44415","inReplyTo":"20161102222901.GK4611@redhat.com","subject":"Re: send-email garbled header with trailing doublequote in email","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-03T14:18:48Z","receivedAt":"2016-11-03T14:18:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 02, 2016 at 11:29:01PM +0100, Andrea Arcangeli wrote:\n\n> So this must be postfix then that out of the blue decided to garble it\n> in a strange way while parsing the input... The removal of all\n> whitespaces s/what ever/whatever/ especially I've no idea how it\n> decided to do so.\n> \n> Can you reproduce with postfix as sendmail at least? If you can\n> reproduce also see what happens if you add another --to.\n\nYes, I can easily reproduce without using git at all by installing\npostfix in local-delivery mode and running:\n\nsendmail peff@sigill.intra.peff.net <<\\EOF\nFrom: Jeff King \nTo: \"what ever\" \" \nSubject: patch\n\nThis is the body\nEOF\n\nMany MTAs do this kind of header-rewriting. I don't necessarily agree\nwith it as a general concept, but the real problem is the syntactically\nbogus header. The munging that postfix does makes things worse, but I\ncan see why it is confused and does what it does (the whole email is\ninside a double-quoted portion that is never closed, so it probably\nthinks there is no hostname portion at all).\n\nSo git is possibly at fault for passing along a bogus address. OTOH, the\nuser is perhaps at fault for providing the bogus address to git in the\nfirst place. GIGO. :)\n\nI think if any change were to be made, it would be to recognize this\nbogosity and either clean it up or abort. That ideally would happen via\nMail::Address so git does not have to add a bunch of ad-hoc \"is this\nvalid rfc822\" checks. Reading the manpage for that module, though, it\nsays:\n\n [we do not handle all of rfc2822]\n Often requests are made to the maintainers of this code improve this\n situation, but this is not a good idea, where it will break zillions\n of existing applications. If you wish for a fully RFC2822 compliant\n implementation you may take a look at Mail::Message::Field::Full, part\n of MailBox.\n\nSo it's possible that switching to a more robust module would improve\nthings.\n\n-Peff\n"},{"id":"305346","messageId":"CANgJU+X9YVcynHfx7nvedBKxDp6+2_S07_whMCSCtq6YrHMnJw@mail.gmail.com","threadId":"44415","inReplyTo":"20161103141848.42pg6iow24prign5@sigill.intra.peff.net","subject":"Re: send-email garbled header with trailing doublequote in email","fromName":"demerphq","fromEmail":"demerphq@gmail.com","sentAt":"2016-11-03T14:33:12Z","receivedAt":"2016-11-03T14:33:21Z","isPatch":false,"sender":{"key":"demerphq@gmail.com","avatar":null},"body":"On 3 November 2016 at 15:18, Jeff King wrote:\n> On Wed, Nov 02, 2016 at 11:29:01PM +0100, Andrea Arcangeli wrote:\n>\n>> So this must be postfix then that out of the blue decided to garble it\n>> in a strange way while parsing the input... The removal of all\n>> whitespaces s/what ever/whatever/ especially I've no idea how it\n>> decided to do so.\n>>\n>> Can you reproduce with postfix as sendmail at least? If you can\n>> reproduce also see what happens if you add another --to.\n>\n> Yes, I can easily reproduce without using git at all by installing\n> postfix in local-delivery mode and running:\n>\n> sendmail peff@sigill.intra.peff.net <<\\EOF\n> From: Jeff King \n> To: \"what ever\" \" \n> Subject: patch\n>\n> This is the body\n> EOF\n>\n> Many MTAs do this kind of header-rewriting. I don't necessarily agree\n> with it as a general concept, but the real problem is the syntactically\n> bogus header. The munging that postfix does makes things worse, but I\n> can see why it is confused and does what it does (the whole email is\n> inside a double-quoted portion that is never closed, so it probably\n> thinks there is no hostname portion at all).\n>\n> So git is possibly at fault for passing along a bogus address. OTOH, the\n> user is perhaps at fault for providing the bogus address to git in the\n> first place. GIGO. :)\n>\n> I think if any change were to be made, it would be to recognize this\n> bogosity and either clean it up or abort. That ideally would happen via\n> Mail::Address so git does not have to add a bunch of ad-hoc \"is this\n> valid rfc822\" checks. Reading the manpage for that module, though, it\n> says:\n>\n> [we do not handle all of rfc2822]\n> Often requests are made to the maintainers of this code improve this\n> situation, but this is not a good idea, where it will break zillions\n> of existing applications. If you wish for a fully RFC2822 compliant\n> implementation you may take a look at Mail::Message::Field::Full, part\n> of MailBox.\n>\n> So it's possible that switching to a more robust module would improve\n> things.\n\nThere is an RFC2822 compliant email address validator in the perl test\nsuite if you guys want to use it. We use it to test recursive\npatterns.\n\nhttp://perl5.git.perl.org/perl.git/blob/HEAD:/t/re/reg_email.t\n\nYves\n\n\n-- \nperl -Mre=debug -e \"/just|another|perl|hacker/\"\n"},{"id":"305388","messageId":"20161104000351.GP4611@redhat.com","threadId":"44415","inReplyTo":"20161103141848.42pg6iow24prign5@sigill.intra.peff.net","subject":"Re: send-email garbled header with trailing doublequote in email","fromName":"Andrea Arcangeli","fromEmail":"aarcange@redhat.com","sentAt":"2016-11-04T00:03:51Z","receivedAt":"2016-11-04T00:03:59Z","isPatch":false,"sender":{"key":"aarcange@redhat.com","avatar":null},"body":"On Thu, Nov 03, 2016 at 10:18:48AM -0400, Jeff King wrote:\n> bogus header. The munging that postfix does makes things worse, but I\n\nI agree it makes things worse.\n\n> can see why it is confused and does what it does (the whole email is\n> inside a double-quoted portion that is never closed, so it probably\n> thinks there is no hostname portion at all).\n\nI would see it too, if it actually sent the email to the garbled email\naddress it generated, but it actually got the right email address\n(because it sent the email to the right address), but it decided to\nshow a different email address in the mail header than the one it sent\nthe email to. But I figure this is the wrong list for such questions :).\n\n> I think if any change were to be made, it would be to recognize this\n> bogosity and either clean it up or abort. That ideally would happen via\n\nIf postfix continues to do what it does, a strict checking would be my\npreference of course, assuming it won't break anything that currently\nworks...\n\nIf not it's fine too, as nothing particularly \"unexpected\" happened in\ngit.\n\nAt this point double checking that what postfix does is legit is\nperhaps more worthwhile than adding strictness to git.\n\nThanks,\nAndrea\n"},{"id":"305389","messageId":"20161104001110.r2uzfybddbq2wyn4@sigill.intra.peff.net","threadId":"44415","inReplyTo":"20161104000351.GP4611@redhat.com","subject":"Re: send-email garbled header with trailing doublequote in email","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-04T00:11:10Z","receivedAt":"2016-11-04T00:11:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 04, 2016 at 01:03:51AM +0100, Andrea Arcangeli wrote:\n\n> > can see why it is confused and does what it does (the whole email is\n> > inside a double-quoted portion that is never closed, so it probably\n> > thinks there is no hostname portion at all).\n> \n> I would see it too, if it actually sent the email to the garbled email\n> address it generated, but it actually got the right email address\n> (because it sent the email to the right address), but it decided to\n> show a different email address in the mail header than the one it sent\n> the email to. But I figure this is the wrong list for such questions :).\n\nIt has to do with the \"envelope recipient\" versus the RFC822 header. Git\nprovides the envelope recipient on the command-line, and that is what is\nused in the SMTP conversation. In theory the MTA does not need to ever\neven look at the contents of the message itself. Some do not, but some\nhave features like rewriting the in-message headers (e.g., to turn\n\"to: user\" into \"to: user@hostname\"). You can probably disable that\nheader-rewriting feature in your config, but I don't know very much\nabout configuring postfix.\n\n-Peff\n"}]}