[PATCH 0/1] [GSoC]smtp_auth_maybe: improve smtp authentication error handling logic

[PATCH 0/1] [GSoC]smtp_auth_maybe: improve smtp authentication error handling logic

[Zheng Yuting]

This patch enhances error handling in the smtp_auth_maybe() function by
distinguishing between temporary errors and permanent authentication failures.

- For temporary errors (e.g., timeout, network issues, etc.), the function
logs a warning and returns 1, allowing retries.

- For permanent errors (e.g., invalid credentials), it returns 0.

Additionally, it separates handling for SASL and plain authentication
to prevent rejecting valid credentials due to non-credential errors,
improving robustness.

Zheng Yuting (1):
  improve smtp authentication error handling logic

 git-send-email.perl | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

--
2.49.0.rc0.57.gdb91954e18

[PATCH 1/1] Improve SMTP authentication error handling logic

[Zheng Yuting]

---
 git-send-email.perl | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 798d59b84f..a012d61abb 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1419,19 +1419,19 @@ sub smtp_auth_maybe {
 		die "invalid smtp auth: '${smtp_auth}'";
 	}
 
-	# TODO: Authentication may fail not because credentials were
+	# Authentication may fail not because credentials were
 	# invalid but due to other reasons, in which we should not
 	# reject credentials.
 	$auth = Git::credential({
 		'protocol' => 'smtp',
 		'host' => smtp_host_string(),
 		'username' => $smtp_authuser,
-		# if there's no password, "git credential fill" will
-		# give us one, otherwise it'll just pass this one.
 		'password' => $smtp_authpass
+
 	}, sub {
 		my $cred = shift;
-
+		my $result;
+		my $error;
 		if ($smtp_auth) {
 			my $sasl = Authen::SASL->new(
 				mechanism => $smtp_auth,
@@ -1441,13 +1441,27 @@ sub smtp_auth_maybe {
 					authname => $cred->{'username'},
 				}
 			);
-
 			return !!$smtp->auth($sasl);
+		} else {
+			# Handle plain authentication errors
+			eval {
+				$result = $smtp->auth($cred->{'username'}, $cred->{'password'});
+				1; # Ensure true value is returned
+			} or do {
+				$error = $@ || 'Unknown error';
+			};
 		}
-
-		return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
+		# Unified error handling logic
+		if ($error) {
+			# Match temporary errors
+			if ($error =~ /timeout|temporary|greylist|throttled|quota\s+exceeded|queue|overload|try\s+again|connection\s+lost|network\s+error/i) {
+				warn "SMTP temporary error: $error";
+				return 1;
+			}
+			return 0;
+		}
+		return !!$result;
 	});
-
 	return $auth;
 }
 
-- 
2.49.0.rc0.57.gdb91954e18

Re: [PATCH 1/1] Improve SMTP authentication error handling logic

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2800 bytes --]

Zheng Yuting <05zyt30@gmail.com> writes:

I see that you've added a cover message detailing the patch, but
individual patches should include the relevant commit description and
message. This is also outlined in 'Documentation/SubmittingPatches'.

That said my perl knowledge is limited, but I'll ask some questions
anyways.

> ---
>  git-send-email.perl | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 798d59b84f..a012d61abb 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1419,19 +1419,19 @@ sub/ smtp_auth_maybe {
>  		die "invalid smtp auth: '${smtp_auth}'";
>  	}
>
> -	# TODO: Authentication may fail not because credentials were
> +	# Authentication may fail not because credentials were
>  	# invalid but due to other reasons, in which we should not
>  	# reject credentials.
>  	$auth = Git::credential({
>  		'protocol' => 'smtp',
>  		'host' => smtp_host_string(),
>  		'username' => $smtp_authuser,
> -		# if there's no password, "git credential fill" will
> -		# give us one, otherwise it'll just pass this one.

I didn't understand why these comments were removed.

>  		'password' => $smtp_authpass
> +

Nit: unnecessary newline.

>  	}, sub {
>  		my $cred = shift;
> -
> +		my $result;
> +		my $error;

Nit: It is easier to read with a newline separating the variable
declaration with the rest of the code.

>  		if ($smtp_auth) {
>  			my $sasl = Authen::SASL->new(
>  				mechanism => $smtp_auth,
> @@ -1441,13 +1441,27 @@ sub smtp_auth_maybe {
>  					authname => $cred->{'username'},
>  				}
>  			);
> -

Nit: unnecessary newline removal

>  			return !!$smtp->auth($sasl);

Shouldn't we do the same kind of changes for this `smpt->auth()` as the
other below?

> +		} else {
> +			# Handle plain authentication errors
> +			eval {
> +				$result = $smtp->auth($cred->{'username'}, $cred->{'password'});
> +				1; # Ensure true value is returned
> +			} or do {
> +				$error = $@ || 'Unknown error';
> +			};

Okay, so here we try to do the authentication and if there aren't any
exceptions we return 1.

>  		}
> -
> -		return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
> +		# Unified error handling logic
> +		if ($error) {
> +			# Match temporary errors
> +			if ($error =~ /timeout|temporary|greylist|throttled|quota\s+exceeded|queue|overload|try\s+again|connection\s+lost|network\s+error/i) {

Where do we get this list from? Regex matching errors for the type
doesn't seem like the best way to go about this.

> +				warn "SMTP temporary error: $error";
> +				return 1;
> +			}
> +			return 0;
> +		}
> +		return !!$result;
>  	});
> -

Nit: unnecessary newline removal

>  	return $auth;
>  }
>
> --
> 2.49.0.rc0.57.gdb91954e18

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 1/1] Improve SMTP authentication error handling logic

[Yuting Zheng]

On Tue Mar 11, 2025 at 6:14 AM CST, Karthik Nayak wrote:
> Zheng Yuting <05zyt30@gmail.com> writes:
>
> I see that you've added a cover message detailing the patch, but
> individual patches should include the relevant commit description and
> message. This is also outlined in 'Documentation/SubmittingPatches'.

Thanks for pointing out my mistakes, I'm sorry to ignore the individual
patch commits. I will add them in my next version.

>
> That said my perl knowledge is limited, but I'll ask some questions
> anyways.
>

I am grateful for your support. As a new contributor, I’m still learning
the best practices and will improve my process based on your feedback.

>> ->--->---# if there's no password, "git credential fill" will
>> ->--->---# give us one, otherwise it'll just pass this one.
>
> I didn't understand why these comments were removed.
>

I removed those comments because I intended to use the password directly
at that point. I understand the confusion this caused and will restore
or clarify the comments in my next revision.

>>  >--->---'password' => $smtp_authpass
>> +
>
> Nit: unnecessary newline.
>

I’ll remove it.

>>  >---}, sub {
>>  >--->---my $cred = shift;
>> -
>> +>--->---my $result;
>> +>--->---my $error;
>
> Nit: It is easier to read with a newline separating the variable
> declaration with the rest of the code.
>
>> @@ -1441,13 +1441,27 @@ sub smtp_auth_maybe {
>>  >--->--->--->--->---authname => $cred->{'username'},
>>  >--->--->--->---}
>>  >--->--->---);
>> -
>
> Nit: unnecessary newline removal

I will adjust these suggestions in the next version.

>
>>  >--->--->---return !!$smtp->auth($sasl);
>
> Shouldn't we do the same kind of changes for this `smpt->auth()` as the
> other below?
>

I’ll review both instances of the smtp->auth() call and update them so they
follow a consistent approach in error handling and formatting.

>> +>--->---if ($error) {
>> +>--->--->---# Match temporary errors
>> +>--->--->---if ($error =~ /timeout|temporary|greylist|throttled|quota\s+exceeded|queue|overload|try\s+again|connection\s+lost|network\s+error/i) {
>
> Where do we get this list from? Regex matching errors for the type
> doesn't seem like the best way to go about this.
>

I agree with your assessment. Initially, I aimed to interpret the SMTP
server's responses directly. But after considering your suggestion, I
realized that I should use SMTP servers return standardized status codes
for command outcomes according to RFC 5321 (4.2.2 Reply Codes by Function
Groups). I will improve it.

>> +>--->---return !!$result;
>>  >---});
>> -
>
> Nit: unnecessary newline removal
>

I will adjust these suggestions in the next version.

On Tue, Mar 11, 2025 at 6:14 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>
> Zheng Yuting <05zyt30@gmail.com> writes:
>
> I see that you've added a cover message detailing the patch, but
> individual patches should include the relevant commit description and
> message. This is also outlined in 'Documentation/SubmittingPatches'.
>
> That said my perl knowledge is limited, but I'll ask some questions
> anyways.
>
> > ---
> >  git-send-email.perl | 30 ++++++++++++++++++++++--------
> >  1 file changed, 22 insertions(+), 8 deletions(-)
> >
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index 798d59b84f..a012d61abb 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -1419,19 +1419,19 @@ sub/ smtp_auth_maybe {
> >               die "invalid smtp auth: '${smtp_auth}'";
> >       }
> >
> > -     # TODO: Authentication may fail not because credentials were
> > +     # Authentication may fail not because credentials were
> >       # invalid but due to other reasons, in which we should not
> >       # reject credentials.
> >       $auth = Git::credential({
> >               'protocol' => 'smtp',
> >               'host' => smtp_host_string(),
> >               'username' => $smtp_authuser,
> > -             # if there's no password, "git credential fill" will
> > -             # give us one, otherwise it'll just pass this one.
>
> I didn't understand why these comments were removed.
>
> >               'password' => $smtp_authpass
> > +
>
> Nit: unnecessary newline.
>
> >       }, sub {
> >               my $cred = shift;
> > -
> > +             my $result;
> > +             my $error;
>
> Nit: It is easier to read with a newline separating the variable
> declaration with the rest of the code.
>
> >               if ($smtp_auth) {
> >                       my $sasl = Authen::SASL->new(
> >                               mechanism => $smtp_auth,
> > @@ -1441,13 +1441,27 @@ sub smtp_auth_maybe {
> >                                       authname => $cred->{'username'},
> >                               }
> >                       );
> > -
>
> Nit: unnecessary newline removal
>
> >                       return !!$smtp->auth($sasl);
>
> Shouldn't we do the same kind of changes for this `smpt->auth()` as the
> other below?
>
> > +             } else {
> > +                     # Handle plain authentication errors
> > +                     eval {
> > +                             $result = $smtp->auth($cred->{'username'}, $cred->{'password'});
> > +                             1; # Ensure true value is returned
> > +                     } or do {
> > +                             $error = $@ || 'Unknown error';
> > +                     };
>
> Okay, so here we try to do the authentication and if there aren't any
> exceptions we return 1.
>
> >               }
> > -
> > -             return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
> > +             # Unified error handling logic
> > +             if ($error) {
> > +                     # Match temporary errors
> > +                     if ($error =~ /timeout|temporary|greylist|throttled|quota\s+exceeded|queue|overload|try\s+again|connection\s+lost|network\s+error/i) {
>
> Where do we get this list from? Regex matching errors for the type
> doesn't seem like the best way to go about this.
>
> > +                             warn "SMTP temporary error: $error";
> > +                             return 1;
> > +                     }
> > +                     return 0;
> > +             }
> > +             return !!$result;
> >       });
> > -
>
> Nit: unnecessary newline removal
>
> >       return $auth;
> >  }
> >
> > --
> > 2.49.0.rc0.57.gdb91954e18