Re: [PATCH] git-send-email: Add --no-validate-email option

[Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>]

On Sun, Jun 19 2022, Stewart Smith wrote:

> The perl Email::Valid module gets things right, but this may not always
> be what you want, as can be seen in
> https://bugzilla.redhat.com/show_bug.cgi?id=2046203
>
> So, add a --validate-email (default, current behavior) and
> the inverse --no-validate-email option to be able to skip the check
> while still having the Email::Valid perl module installed.
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2046203
> Suggested-by: Todd Zullinger <tmz@xxxxxxxxx>
> Signed-off-by: Stewart Smith <trawets@xxxxxxxxxx>
> ---
>  git-send-email.perl   | 9 +++++++++
>  t/t9902-completion.sh | 1 +
>  2 files changed, 10 insertions(+)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 5861e99a6e..c75b08f9ce 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -103,6 +103,7 @@ sub usage {
>      --quiet                        * Output one line of info per email.
>      --dry-run                      * Don't actually send the emails.
>      --[no-]validate                * Perform patch sanity checks. Default on.
> +    --[no-]validate-email          * Perform email address sanity checks. Default on.
>      --[no-]format-patch            * understand any non optional arguments as
>                                       `git format-patch` ones.
>      --force                        * Send even if safety checks would prevent it.
> @@ -281,6 +282,7 @@ sub do_edit {
>  my $chain_reply_to = 0;
>  my $use_xmailer = 1;
>  my $validate = 1;
> +my $validate_email = 1;
>  my $target_xfer_encoding = 'auto';
>  my $forbid_sendmail_variables = 1;
>  
> @@ -293,6 +295,7 @@ sub do_edit {
>      "tocover" => \$cover_to,
>      "signedoffcc" => \$signed_off_by_cc,
>      "validate" => \$validate,
> +    "validateemail" => \$validate_email,
>      "multiedit" => \$multiedit,
>      "annotate" => \$annotate,
>      "xmailer" => \$use_xmailer,
> @@ -531,6 +534,8 @@ sub config_regexp {
>  		    "no-thread" => sub {$thread = 0},
>  		    "validate!" => \$validate,
>  		    "no-validate" => sub {$validate = 0},
> +		    "validate-email!" => \$validate_email,
> +		    "no-validate-email" => sub {$validate_email = 0},
>  		    "transfer-encoding=s" => \$target_xfer_encoding,
>  		    "format-patch!" => \$format_patch,
>  		    "no-format-patch" => sub {$format_patch = 0},
> @@ -1132,6 +1137,10 @@ sub extract_valid_address {
>  	# check for a local address:
>  	return $address if ($address =~ /^($local_part_regexp)$/);
>  
> +	# Email::Valid isn't always correct, so support a way to bypass
> +	# See https://bugzilla.redhat.com/show_bug.cgi?id=2046203
> +	return 1 if not $validate_email;
> +
>  	$address =~ s/^\s*<(.*)>\s*$/$1/;
>  	my $have_email_valid = eval { require Email::Valid; 1 };
>  	if ($have_email_valid) {
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 31526e6b64..6e363c46f3 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2302,6 +2302,7 @@ test_expect_success PERL 'send-email' '
>  	EOF
>  	test_completion "git send-email --val" <<-\EOF &&
>  	--validate Z
> +	--validate-email Z
>  	EOF
>  	test_completion "git send-email ma" "main "
>  '

I don't think this patch is what we want to fix this problem: The
git-send-email script should ultimately be trying to pass an address to
a MTA. If you look into its history of Email::Valid use you'll see that
we initially used it unconditionally, then later conditionally to get
rid of the dependency.

I think a better change is to simply get rid of the Email::Valid
dependency, but I'm not 100% sure if there aren't edge cases where our
parsing there isn't something we rely on in other cases.

But in the meantime a more narrow change that I believe solves the issue
for you is:
	
	diff --git a/git-send-email.perl b/git-send-email.perl
	index 5861e99a6eb..1168da43ef2 100755
	--- a/git-send-email.perl
	+++ b/git-send-email.perl
	@@ -1135,7 +1135,11 @@ sub extract_valid_address {
	 	$address =~ s/^\s*<(.*)>\s*$/$1/;
	 	my $have_email_valid = eval { require Email::Valid; 1 };
	 	if ($have_email_valid) {
	-		return scalar Email::Valid->address($address);
	+		my $email = Email::Valid->address(
	+			-address => $address,
	+			-localpart => 0,
	+		);
	+		return $email if $email;
	 	}
	 
	 	# less robust/correct than the monster regexp in Email::Valid,

Of that we just need that "-localpart => 1" part to fix this specific
problem, but I think having the "return $email if $email" is also more
correct, and would solve this bug even without the "-localpart => 0",
i.e. we'll always fall through to trying to parse the address with our
regex.

In any case I think this could really use a corresponding update to the
t/t9001-send-email.sh script, i.e. to test an address with a long local
part.