Re: [BUG] git send-email: incorrectly parses email address with comma

[Heinrich Schuchardt]

On 05/12/2018 11:48 AM, Jeff King wrote:
> On Sat, May 12, 2018 at 10:21:46AM +0200, Heinrich Schuchardt wrote:
> 
>> Git send-email allows to combine multiple email addresses in one
>> parameter, e.g.
>>
>> --to="a...@example.com, b...@example.com"
>>
>> But email addresses may contain commas themselves:
>>
>> --to="LASTNAME, firstname "
>>
>> This may lead to an error:
> 
> If the name contains syntactically relevant metacharacters, it can be
> quoted. So as a workaround, you can do:
> 
>   --to='"LASTNAME, firstname" '
> 
> I think rfc822 actually requires even names with just spaces in them to
> be quoted, but git-send-email and most other mail programs are pretty
> lax about allowing just about anything outside of the <>, so people tend
> not to bother.
> 
>> If the string preceding a comma is not a valid email address do not
>> split it off.
> 
> That might work as a heuristic, though "is a valid email address" is a
> notoriously hard thing to check. Possibly looking for an "@" would catch
> most common cases, though.

A more elaborate test would be:
A string matching [\S\s]*<\S+@\S+.\S+>\s* is an email address.
A string matching \s*\S+@\S+.\S+\s* is an email address.
Both may need trimming of whitespace.
Any other string is not an email address.

Regards

Heinrich

> 
> -Peff
> 

Re: [BUG] git send-email: incorrectly parses email address with comma

[Jeff King]

On Sat, May 12, 2018 at 10:21:46AM +0200, Heinrich Schuchardt wrote:

> Git send-email allows to combine multiple email addresses in one
> parameter, e.g.
> 
> --to="a...@example.com, b...@example.com"
> 
> But email addresses may contain commas themselves:
> 
> --to="LASTNAME, firstname "
> 
> This may lead to an error:

If the name contains syntactically relevant metacharacters, it can be
quoted. So as a workaround, you can do:

  --to='"LASTNAME, firstname" '

I think rfc822 actually requires even names with just spaces in them to
be quoted, but git-send-email and most other mail programs are pretty
lax about allowing just about anything outside of the <>, so people tend
not to bother.

> If the string preceding a comma is not a valid email address do not
> split it off.

That might work as a heuristic, though "is a valid email address" is a
notoriously hard thing to check. Possibly looking for an "@" would catch
most common cases, though.

-Peff

[PATCH v2] send-email: Net::SMTP::starttls was introduced in v2.34 (Re: [BUG] git-send-email broken: Can't locate object method "starttls")

[Jonathan Nieder]

Subject: send-email: Net::SMTP::starttls was introduced in v2.34

We cannot rely on the starttls method being present in Net::SMTP until
c274b798e6881a941d941808c6d89966975cb8c8 (Merge branch 'ipv6_ssl' of
https://github.com/noxxi/perl-libnet into noxxi-ipv6_ssl, 2014-06-02),
which set the module version to 2.34.

This version was first shipped as part of perl in v5.21.5~169 (Update
libnet to CPAN version 3.01, 2014-10-10).

Noticed on an Ubuntu system with perl 5.18.2-2ubuntu1.1, which
provides Net::SMTP version 2.31. The error message is

  Can't locate object method "starttls" via package "Net::SMTP" at 
/usr/lib/git-core/git-send-email line 1410.

Reported-by: Brandon Williams 
Reported-and-tested-by: Eric Biggers 
Signed-off-by: Jonathan Nieder 
---
Jonathan Nieder wrote:

> Thanks.  Mining through https://github.com/gbarr/perl-libnet, I find
[...]
> I think 2.35 is the correct minimum version.

Nah, I'm reading wrong.  The relevant commit is c274b798 (Merge branch
'ipv6_ssl' of https://github.com/noxxi/perl-libnet into
noxxi-ipv6_ssl, 2014-06-02), which bumped VERSION to 2.34.

Here's an updated patch.

Thanks,
Jonathan

 git-send-email.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 0d90439d9..d326238c0 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1354,7 +1354,7 @@ EOF
}
 
require Net::SMTP;
-   my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
version->parse("1.28");
+   my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
version->parse("2.34");
$smtp_domain ||= maildomain();
 
if ($smtp_encryption eq 'ssl') {
-- 
2.13.0.506.g27d5fe0cd

Re: [PATCH] send-email: Net::SMTP::starttls was introduced in v3.01 (Re: [BUG] git-send-email broken: Can't locate object method "starttls")

[Jonathan Nieder]

Eric Biggers wrote:
> On Wed, May 31, 2017 at 03:44:15PM -0700, Jonathan Nieder wrote:

>> Subject: send-email: Net::SMTP::starttls was introduced in v3.01
>>
>> We cannot rely on the starttls method being present in the copy
>> of Net::SMTP shipped with perl until v5.21.5~169 (Update libnet to
>> CPAN version 3.01, 2014-10-10).
>>
>> Reported-by: Brandon Williams 
>> Reported-by: Eric Biggers 
>> Signed-off-by: Jonathan Nieder 
>> ---
[...]
>> +++ b/git-send-email.perl
>> @@ -1354,7 +1354,7 @@ EOF
>>  }
>>  
>>  require Net::SMTP;
>> -my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
>> version->parse("1.28");
>> +my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
>> version->parse("3.01");
>>  $smtp_domain ||= maildomain();
>>  
>>  if ($smtp_encryption eq 'ssl') {
>> -- 
>
> Yes, that solves the problem for me.

Thanks.  Mining through https://github.com/gbarr/perl-libnet, I find

  $ git log -Sstarttls
[...]
  commit b4a7a274a7fe5344c154abc4b3fdd7c446d36370
  Author: Steffen Ullrich 
  Date:   Fri May 9 23:15:48 2014 +0200

  SSL and IPv6 support for Net::SMTP
  $ git show b4a7a274a7fe5344c154abc4b3fdd7c446d36370
[...]
  diff --git a/Net/SMTP.pm b/Net/SMTP.pm
  index 705b5c5..fcc124f 100644
  --- a/Net/SMTP.pm
  +++ b/Net/SMTP.pm
  @@ -18,7 +18,31 @@ use Net::Config;
 
   $VERSION = "2.33";
[...]
  $ git log -p --ancestry-path b4a7a274a7fe5344c154abc4b3fdd7c446d36370..HEAD  
-- Net/SMTP.pm
[...]
  commit 67b37d5c7118f9af50e5a5a00c242992caba3b8d
  Author: Steve Hay 
  Date:   Mon Jun 2 14:13:55 2014 +0100

  Bump $VERSION in changed modules

  diff --git a/Net/SMTP.pm b/Net/SMTP.pm
  index 4496f6f..9dfaadf 100644
  --- a/Net/SMTP.pm
  +++ b/Net/SMTP.pm
  @@ -16,7 +16,7 @@ use IO::Socket;
   use Net::Cmd;
   use Net::Config;
   
  -$VERSION = "2.34";
  +$VERSION = "2.35";
   
   # Code for detecting if we can use SSL
   my $ssl_class = eval {

I think 2.35 is the correct minimum version.

Jonathan

Re: [PATCH] send-email: Net::SMTP::starttls was introduced in v3.01 (Re: [BUG] git-send-email broken: Can't locate object method "starttls")

[Eric Biggers]

On Wed, May 31, 2017 at 03:44:15PM -0700, Jonathan Nieder wrote:
> Subject: send-email: Net::SMTP::starttls was introduced in v3.01
> 
> We cannot rely on the starttls method being present in the copy
> of Net::SMTP shipped with perl until v5.21.5~169 (Update libnet to
> CPAN version 3.01, 2014-10-10).
> 
> Reported-by: Brandon Williams 
> Reported-by: Eric Biggers 
> Signed-off-by: Jonathan Nieder 
> ---
> Hi Eric,
> 
> Eric Biggers wrote:
> 
> > There seems to be a bug in 'git send-email' caused by this commit:
> >
> > commit 0ead000c3aca13a10ae51a3c74c866981e0d33b8
> > Author: Dennis Kaarsemaker 
> > Date:   Fri Mar 24 22:37:32 2017 +0100
> >
> > send-email: Net::SMTP::SSL is obsolete, use only when necessary
> >
> > When running git send-email I get the following error:
> >
> > Can't locate object method "starttls" via package "Net::SMTP" at 
> > /usr/lib/git-core/git-send-email line 1410.
> >
> > The perl version is 5.18.2, and the Net::SMTP version is 2.31.
> 
> Thanks for reporting.  Does this patch help?
> 
> Regards,
> Jonathan
> 
>  git-send-email.perl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 0d90439d9..3441e3cf5 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1354,7 +1354,7 @@ EOF
>   }
>  
>   require Net::SMTP;
> - my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
> version->parse("1.28");
> + my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
> version->parse("3.01");
>   $smtp_domain ||= maildomain();
>  
>   if ($smtp_encryption eq 'ssl') {
> -- 

Yes, that solves the problem for me.

Eric

Re: [PATCH] send-email: Net::SMTP::starttls was introduced in v3.01 (Re: [BUG] git-send-email broken: Can't locate object method "starttls")

[Junio C Hamano]

Jonathan Nieder  writes:

> Subject: send-email: Net::SMTP::starttls was introduced in v3.01
>
> We cannot rely on the starttls method being present in the copy
> of Net::SMTP shipped with perl until v5.21.5~169 (Update libnet to
> CPAN version 3.01, 2014-10-10).
>
> Reported-by: Brandon Williams 
> Reported-by: Eric Biggers 
> Signed-off-by: Jonathan Nieder 
> ---
> Hi Eric,
>
> Eric Biggers wrote:
>
>> There seems to be a bug in 'git send-email' caused by this commit:
>>
>> commit 0ead000c3aca13a10ae51a3c74c866981e0d33b8
>> Author: Dennis Kaarsemaker 
>> Date:   Fri Mar 24 22:37:32 2017 +0100
>>
>> send-email: Net::SMTP::SSL is obsolete, use only when necessary
>>
>> When running git send-email I get the following error:
>>
>>  Can't locate object method "starttls" via package "Net::SMTP" at 
>> /usr/lib/git-core/git-send-email line 1410.
>>
>> The perl version is 5.18.2, and the Net::SMTP version is 2.31.
>
> Thanks for reporting.  Does this patch help?

Good, you beat me to it ;-)  Once we get this confirmed, let's queue
an emergency fix.

Thanks.

>
> Regards,
> Jonathan
>
>  git-send-email.perl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 0d90439d9..3441e3cf5 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1354,7 +1354,7 @@ EOF
>   }
>  
>   require Net::SMTP;
> - my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
> version->parse("1.28");
> + my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
> version->parse("3.01");
>   $smtp_domain ||= maildomain();
>  
>   if ($smtp_encryption eq 'ssl') {

[PATCH] send-email: Net::SMTP::starttls was introduced in v3.01 (Re: [BUG] git-send-email broken: Can't locate object method "starttls")

[Jonathan Nieder]

Subject: send-email: Net::SMTP::starttls was introduced in v3.01

We cannot rely on the starttls method being present in the copy
of Net::SMTP shipped with perl until v5.21.5~169 (Update libnet to
CPAN version 3.01, 2014-10-10).

Reported-by: Brandon Williams 
Reported-by: Eric Biggers 
Signed-off-by: Jonathan Nieder 
---
Hi Eric,

Eric Biggers wrote:

> There seems to be a bug in 'git send-email' caused by this commit:
>
> commit 0ead000c3aca13a10ae51a3c74c866981e0d33b8
> Author: Dennis Kaarsemaker 
> Date:   Fri Mar 24 22:37:32 2017 +0100
>
> send-email: Net::SMTP::SSL is obsolete, use only when necessary
>
> When running git send-email I get the following error:
>
>   Can't locate object method "starttls" via package "Net::SMTP" at 
> /usr/lib/git-core/git-send-email line 1410.
>
> The perl version is 5.18.2, and the Net::SMTP version is 2.31.

Thanks for reporting.  Does this patch help?

Regards,
Jonathan

 git-send-email.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 0d90439d9..3441e3cf5 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1354,7 +1354,7 @@ EOF
}
 
require Net::SMTP;
-   my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
version->parse("1.28");
+   my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
version->parse("3.01");
$smtp_domain ||= maildomain();
 
if ($smtp_encryption eq 'ssl') {
-- 
2.13.0.506.g27d5fe0cd

Re: bug (?) in send email

[Christoph Miebach]

Hello!

Removing this line
s/_/ /g;
here
https://github.com/git/git/blob/master/git-send-email.perl#L867

Solves this problem for me. But I really don't have any clue, what kind 
of side effects this modification on sub unquote_rfc2047 might have.


Regards

Christoph

On 28.07.2012 23:33, Christoph Miebach wrote:

Hello!

send-email (tested versions 1.7.9.2 and 1.7.10.4) breaks email addresses.

Steps to reproduce:


Modify file.

git commit --author=Michał Tz name_1...@some.com modified.file -m
Test

git format-patch -o patches origin

Now, the patch seems to have the address right, see [1]

git send-email  --to myown.addr...@mail.com --suppress-cc=author
patches/0001-Test.patch

But checking my inbox now shows an email starting with:
From: Michał Tz name 1...@some.com

So the address is splitted at the underscore.

Furthermore, if I don't use --suppress-cc=author, the CC field shows the
right address.

Regards

Christoph

[1]
less patches/0001-Test.patch
From: =?UTF-8?q?Micha=C5=82=20Tz?= name_1...@some.com

git show
Author: Michał Tz name_1...@some.com


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: bug (?) in send email

[Junio C Hamano]

Thomas Rast tr...@student.ethz.ch writes:

 It would prevent spaces from being decoded correctly if the encoding
 program chooses to make the '_'.  git-format-patch does not actually do
 this, see the big comment around pretty.c:304.

 I think this patch would be a better match for what RFC2047 specifies.
 On the one hand it avoids substituting _ outside of encodings, but OTOH
 it also handles more than one encoded-word.

Yeah, I think it is an improvement.

I however wonder if the captured pattern for $2 should be minimized
with ? at the end, i.e. ..\?q\?(.*?)\?=?

 It still does not handle
 the case where there are several encoded-words of *different* encodings,
 but who would do such a crazy thing?

Even if somebody did so, it wouldn't have worked, and to make it
work, the sub and its caller (there is only one caller that actually
cares what the original encoding was) needs to be rethought anyway,
so I do not think it matters.

It may deserve an in-code NEEDSWORK comment, though.

Thanks.

 diff --git i/git-send-email.perl w/git-send-email.perl
 index ef30c55..88c4758 100755
 --- i/git-send-email.perl
 +++ w/git-send-email.perl
 @@ -862,11 +862,13 @@ sub make_message_id {
  sub unquote_rfc2047 {
   local ($_) = @_;
   my $encoding;
 - if (s/=\?([^?]+)\?q\?(.*)\?=/$2/g) {
 + s{=\?([^?]+)\?q\?(.*)\?=}{
   $encoding = $1;
 - s/_/ /g;
 - s/=([0-9A-F]{2})/chr(hex($1))/eg;
 - }
 + my $e = $2;
 + $e =~ s/_/ /g;
 + $e =~ s/=([0-9A-F]{2})/chr(hex($1))/eg;
 + $e;
 + }eg;
   return wantarray ? ($_, $encoding) : $_;
  }
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: bug (?) in send email

[Jeff King]

On Mon, Jul 30, 2012 at 02:30:35PM +0200, Thomas Rast wrote:

  Removing this line
  s/_/ /g;
  here
  https://github.com/git/git/blob/master/git-send-email.perl#L867
 
  Solves this problem for me. But I really don't have any clue, what
  kind of side effects this modification on sub unquote_rfc2047 might
  have.
 
 It would prevent spaces from being decoded correctly if the encoding
 program chooses to make the '_'.  git-format-patch does not actually do
 this, see the big comment around pretty.c:304.

Yeah, I think we need to be prepared to decode arbitrary rfc2047. Even
though most of our input will come from format-patch, people can munge
the input between format-patch and send-email.

 I think this patch would be a better match for what RFC2047 specifies.
 On the one hand it avoids substituting _ outside of encodings, but OTOH
 it also handles more than one encoded-word.  It still does not handle
 the case where there are several encoded-words of *different* encodings,
 but who would do such a crazy thing?
 
 diff --git i/git-send-email.perl w/git-send-email.perl
 index ef30c55..88c4758 100755
 --- i/git-send-email.perl
 +++ w/git-send-email.perl
 @@ -862,11 +862,13 @@ sub make_message_id {
  sub unquote_rfc2047 {
   local ($_) = @_;
   my $encoding;
 - if (s/=\?([^?]+)\?q\?(.*)\?=/$2/g) {
 + s{=\?([^?]+)\?q\?(.*)\?=}{
   $encoding = $1;
 - s/_/ /g;
 - s/=([0-9A-F]{2})/chr(hex($1))/eg;
 - }
 + my $e = $2;
 + $e =~ s/_/ /g;
 + $e =~ s/=([0-9A-F]{2})/chr(hex($1))/eg;
 + $e;
 + }eg;
   return wantarray ? ($_, $encoding) : $_;

That is definitely an improvement. The existing code was just plain
wrong to assume that the whole string would be rfc2047 encoded, as that
is never the case (e.g., I don't think it is even legal to encode the
addr-spec part).

You are right that the result does not handle multiple encoded chunks
with different encodings. However, it also does not handle a single
encoded chunk with non-encoded bits. E.g., consider:

  From: =?UTF-8?q?Some=20N=C3=A1me?= y...@example.com

We will return:

  (Some Náme y...@example.com, UTF-8)

Note that the second half is unencoded ASCII, but we have simply
concatenated it with the encoded bit and claimed the whole thing at
UTF-8. This works for utf-8, of course, because it is an ASCII superset.
But what about utf-16? You now have incompatible mixed encodings in a
single string. Which of course is only the tip of the iceberg; we later
may stick the author field into the body, which does not necessarily
have the same encoding (and there is a big uh oh we should reencode
argument that does not actually do so).

Furthermore, even if we do use an ASCII superset, the callers do not use
the encoding field very well. They do things like:

  if (unquote_rfc2047($addr) eq $sender) {

So we are comparing the unquoted address, in some encoding (that we
throw away) to whatever we have in $sender, which may have come from the
author ident, or the command-line, or the expansion of a MUA alias. So
we would fail to match utf-8 and latin1 versions of the same identity.

I think the only sane thing for unquote_rfc2047 to do would be to
actually return just a string in some canonical encoding (i.e., utf8),
and iconv each of the bits separately from its appropriate
encoding. And then we assume that everything internal is in the same
encoding and we can just compare strings to get the right answer. And
outgoing, we produce encoded utf8 for the headers, or match the body for
an in-body From header.

Of course, that means we would have to start using perl's encoding
functions, which we do not use currently. I think Encode has been in the
perl base system long enough for us to rely on it, but I am not very up
to speed on encoding issues in perl. And nobody has been complaining
about those issues, which I assume means everybody is just using utf8
for outgoing messages.  So it might not be worth the trouble to fix.

I also notice that we do not handle the 'b' rfc2047 encoding at all.
We do handle this in mailinfo for incoming messages, but again, I guess
everybody just uses the more common 'q' for outgoing (since it's mostly
generated by git, anyway). So if nobody is complaining, I'd be inclined
to just leave it.

So after all that rambling, I think my response is yes, your patch is a
step in the right direction, and we should do it. There are still a ton
of unsupported setups, but really, if we are going to handle everything,
I'd rather see us make the jump to using one of the existing
mail-handling modules rather than reinventing the wheel. And I know
that some people really like the no-dependencies aspect of what we have
currently.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: bug (?) in send email

[Jeff King]

On Mon, Jul 30, 2012 at 08:38:21AM -0700, Junio C Hamano wrote:

  I think this patch would be a better match for what RFC2047 specifies.
  On the one hand it avoids substituting _ outside of encodings, but OTOH
  it also handles more than one encoded-word.
 
 Yeah, I think it is an improvement.
 
 I however wonder if the captured pattern for $2 should be minimized
 with ? at the end, i.e. ..\?q\?(.*?)\?=?

Yeah, definitely. ?= cannot appear inside (it would need to be
quoted).

  It still does not handle
  the case where there are several encoded-words of *different* encodings,
  but who would do such a crazy thing?
 
 Even if somebody did so, it wouldn't have worked, and to make it
 work, the sub and its caller (there is only one caller that actually
 cares what the original encoding was) needs to be rethought anyway,
 so I do not think it matters.
 
 It may deserve an in-code NEEDSWORK comment, though.

I rambled about this in much more detail in another reply, but the gist
of it is that yes, that is the right step for now.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html