Re: git send-email Cc with cruft not working as expected

Stefan Beller [off-list ref] writes:

+cc people from that thread

On Tue, Aug 22, 2017 at 4:30 PM, Jacob Keller [off-list ref] wrote:

quoted

On Tue, Aug 22, 2017 at 4:18 PM, Stefan Beller [off-list ref] wrote:

quoted

On Tue, Aug 22, 2017 at 4:15 PM, Jacob Keller [off-list ref] wrote:

quoted

Hi,

I recently found an issue with git-send-email where it does not
properly remove the cruft of an email address when sending using a Cc:
line.

The specific example is with a commit containing the following Cc line,

Cc: stable@vger.kernel.org # 4.10+

Please see and discuss at
https://public-inbox.org/git/20170216174924.GB2625@localhost/

I read that thread, and it addressed the problem of

Cc: <redacted> # 4.10+

but did not fix this case without the <> around the email address.

Indeed. It detects garbage as "everything after >".

I feel really sorry that we need so many iterations to get back to a
correct behavior :-(.

quoted

Additionally I just discovered that the behavior here changes pretty
drastically if you have Email::Validate installed, now it splits the
address into multiple things:

(I'm assuming you mean Email::Address, there's also Email::Valid but I
don't think it would modify the behavior)

Hmm, I think we reached the point where we should just stop using
Email::Address.

Patch series follows and should address both points.

-- 
Matthieu Moy
http://matthieu-moy.fr

This is a followup over 9d33439 (send-email: only allow one address
per body tag, 2017-02-20). The first iteration did allow writting

  Cc: [off-list ref] # garbage

but did so by matching the regex ([^>]*>?), i.e. stop after the first
instance of '>'. However, it did not properly deal with

  Cc: foo@example.com # garbage

Fix this using a new function strip_garbage_one_address, which does
essentially what the old ([^>]*>?) was doing, but dealing with more
corner-cases. Since we've allowed

  Cc: "Foo # Bar" [off-list ref]

in previous versions, it makes sense to continue allowing it (but we
still remove any garbage after it). OTOH, when an address is given
without quoting, we just take the first word and ignore everything
after.

Signed-off-by: Matthieu Moy <redacted>
---
Also available as: https://github.com/git/git/pull/398

 git-send-email.perl   | 26 ++++++++++++++++++++++++--
 t/t9001-send-email.sh |  4 ++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index fa6526986e..33a69ffe5d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -1089,6 +1089,26 @@ sub sanitize_address {
 
 }
 
+sub strip_garbage_one_address {
+	my ($addr) = @_;
+	chomp $addr;
+	if ($addr =~ /^(("[^"]*"|[^"<]*)? *<[^>]*>).*/) {
+		# "Foo Bar" <foobar@example.com> [possibly garbage here]
+		# Foo Bar <foobar@example.com> [possibly garbage here]
+		return $1;
+	}
+	if ($addr =~ /^(<[^>]*>).*/) {
+		# <foo@example.com> [possibly garbage here]
+		# if garbage contains other addresses, they are ignored.
+		return $1;
+	}
+	if ($addr =~ /^([^"#,\s]*)/) {
+		# address without quoting: remove anything after the address
+		return $1;
+	}
+	return $addr;
+}
+
 sub sanitize_address_list {
 	return (map { sanitize_address($_) } @_);
 }

@@ -1590,10 +1610,12 @@ foreach my $t (@files) {
 	# Now parse the message body
 	while(<$fh>) {
 		$message .=  $_;
-		if (/^(Signed-off-by|Cc): ([^>]*>?)/i) {
+		if (/^(Signed-off-by|Cc): (.*)/i) {
 			chomp;
 			my ($what, $c) = ($1, $2);
-			chomp $c;
+			# strip garbage for the address we'll use:
+			$c = strip_garbage_one_address($c);
+			# sanitize a bit more to decide whether to suppress the address:
 			my $sc = sanitize_address($c);
 			if ($sc eq $sender) {
 				next if ($suppress_cc{'self'});

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index d1e4e8ad19..f30980895c 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh

@@ -148,6 +148,8 @@ cat >expected-cc <<\EOF
 !two@example.com!
 !three@example.com!
 !four@example.com!
+!five@example.com!
+!six@example.com!
 EOF
 "
 

@@ -161,6 +163,8 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '
 	Cc: <two@example.com> # trailing comments are ignored
 	Cc: <three@example.com>, <not.four@example.com> one address per line
 	Cc: "Some # Body" <four@example.com> [ <also.a.comment> ]
+	Cc: five@example.com # not.six@example.com
+	Cc: six@example.com, not.seven@example.com
 	EOF
 	clean_fake_sendmail &&
 	git send-email -1 --to=recipient@example.com \

-- 
2.14.0.rc0.dirty

Using Mail::Address made sense when we didn't have a proper parser. We
now have a reasonable address parser, and using Mail::Address
_if available_ causes much more trouble than it gives benefits:

* Developers typically test one version, not both.

* Users may not be aware that installing Mail::Address will change the
  behavior. They may complain about the behavior in one case without
  knowing that Mail::Address is involved.

* Having this optional Mail::Address makes it tempting to anwser "please
  install Mail::Address" to users instead of fixing our own code. We've
  reached the stage where bugs in our parser should be fixed, not worked
  around.

Signed-off-by: Matthieu Moy <redacted>
---
 git-send-email.perl | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 33a69ffe5d..2208dcc213 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -155,7 +155,6 @@ sub format_2822_time {
 }
 
 my $have_email_valid = eval { require Email::Valid; 1 };
-my $have_mail_address = eval { require Mail::Address; 1 };
 my $smtp;
 my $auth;
 my $num_sent = 0;

@@ -490,11 +489,7 @@ my ($repoauthor, $repocommitter);
 ($repocommitter) = Git::ident_person(@repo, 'committer');
 
 sub parse_address_line {
-	if ($have_mail_address) {
-		return map { $_->format } Mail::Address->parse($_[0]);
-	} else {
-		return Git::parse_mailboxes($_[0]);
-	}
+	return Git::parse_mailboxes($_[0]);
 }
 
 sub split_addrs {

-- 
2.14.0.rc0.dirty

On Wed, Aug 23, 2017 at 3:21 AM, Matthieu Moy [off-list ref] wrote:

This is a followup over 9d33439 (send-email: only allow one address
per body tag, 2017-02-20). The first iteration did allow writting

  Cc: [off-list ref] # garbage

but did so by matching the regex ([^>]*>?), i.e. stop after the first
instance of '>'. However, it did not properly deal with

  Cc: foo@example.com # garbage

Fix this using a new function strip_garbage_one_address, which does
essentially what the old ([^>]*>?) was doing, but dealing with more
corner-cases. Since we've allowed

  Cc: "Foo # Bar" [off-list ref]

in previous versions, it makes sense to continue allowing it (but we
still remove any garbage after it). OTOH, when an address is given
without quoting, we just take the first word and ignore everything
after.

Signed-off-by: Matthieu Moy <redacted>
---

I pulled this and tested it for my issue, and it fixes the problem for
me. I think the approach in the code was solid too, extracting out the
logic helps make the code more clear.

Thanks,
Jake

On Wed, Aug 23, 2017 at 3:02 AM, Matthieu Moy [off-list ref] wrote:

quoted

On Tue, Aug 22, 2017 at 4:30 PM, Jacob Keller [off-list ref] wrote:

quoted

Additionally I just discovered that the behavior here changes pretty
drastically if you have Email::Validate installed, now it splits the
address into multiple things:

(I'm assuming you mean Email::Address, there's also Email::Valid but I
don't think it would modify the behavior)

No I actually definitely meant Email::Valid. I already had
Mail::Address installed, and I then installed Email::Valid, and it
changed behavior to split the cruft into multiple addresses.

I don't actually know why or how it did this, but I'm certain it was
presence of Email::Valid that did it.

However, your first patch addresses the issue since you remove the
cruft well before passing it into Email::Valid anyways.

Hmm, I think we reached the point where we should just stop using
Email::Address.

I do agree, I don't think we should use Mail::Address.

Patch series follows and should address both points.

--
Matthieu Moy
http://matthieu-moy.fr