{"thread":{"id":"63073","subject":"[PATCH 0/1] [GSoC]smtp_auth_maybe: improve smtp authentication error handling logic","startedAt":"2025-03-07T12:39:56Z","lastAt":"2025-03-11T08:04:02Z","messageCount":4,"participants":["Zheng Yuting","Karthik Nayak","Yuting Zheng"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"513750","messageId":"20250307123934.2064275-1-05ZYT30@gmail.com","threadId":"63073","inReplyTo":null,"subject":"[PATCH 0/1] [GSoC]smtp_auth_maybe: improve smtp authentication error handling logic","fromName":"Zheng Yuting","fromEmail":"05zyt30@gmail.com","sentAt":"2025-03-07T12:39:22Z","receivedAt":"2025-03-07T12:39:56Z","isPatch":true,"sender":{"key":"05zyt30@gmail.com","avatar":"https://avatars.githubusercontent.com/u/87643662?v=4"},"body":"This patch enhances error handling in the smtp_auth_maybe() function by\ndistinguishing between temporary errors and permanent authentication failures.\n\n- For temporary errors (e.g., timeout, network issues, etc.), the function\nlogs a warning and returns 1, allowing retries.\n\n- For permanent errors (e.g., invalid credentials), it returns 0.\n\nAdditionally, it separates handling for SASL and plain authentication\nto prevent rejecting valid credentials due to non-credential errors,\nimproving robustness.\n\nZheng Yuting (1):\n  improve smtp authentication error handling logic\n\n git-send-email.perl | 30 ++++++++++++++++++++++--------\n 1 file changed, 22 insertions(+), 8 deletions(-)\n\n--\n2.49.0.rc0.57.gdb91954e18\n"},{"id":"513751","messageId":"20250307123934.2064275-2-05ZYT30@gmail.com","threadId":"63073","inReplyTo":"20250307123934.2064275-1-05ZYT30@gmail.com","subject":"[PATCH 1/1] Improve SMTP authentication error handling logic","fromName":"Zheng Yuting","fromEmail":"05zyt30@gmail.com","sentAt":"2025-03-07T12:39:23Z","receivedAt":"2025-03-07T12:40:01Z","isPatch":true,"sender":{"key":"05zyt30@gmail.com","avatar":"https://avatars.githubusercontent.com/u/87643662?v=4"},"body":"---\n git-send-email.perl | 30 ++++++++++++++++++++++--------\n 1 file changed, 22 insertions(+), 8 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 798d59b84f..a012d61abb 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1419,19 +1419,19 @@ sub smtp_auth_maybe {\n \t\tdie \"invalid smtp auth: '${smtp_auth}'\";\n \t}\n \n-\t# TODO: Authentication may fail not because credentials were\n+\t# Authentication may fail not because credentials were\n \t# invalid but due to other reasons, in which we should not\n \t# reject credentials.\n \t$auth = Git::credential({\n \t\t'protocol' => 'smtp',\n \t\t'host' => smtp_host_string(),\n \t\t'username' => $smtp_authuser,\n-\t\t# if there's no password, \"git credential fill\" will\n-\t\t# give us one, otherwise it'll just pass this one.\n \t\t'password' => $smtp_authpass\n+\n \t}, sub {\n \t\tmy $cred = shift;\n-\n+\t\tmy $result;\n+\t\tmy $error;\n \t\tif ($smtp_auth) {\n \t\t\tmy $sasl = Authen::SASL->new(\n \t\t\t\tmechanism => $smtp_auth,\n@@ -1441,13 +1441,27 @@ sub smtp_auth_maybe {\n \t\t\t\t\tauthname => $cred->{'username'},\n \t\t\t\t}\n \t\t\t);\n-\n \t\t\treturn !!$smtp->auth($sasl);\n+\t\t} else {\n+\t\t\t# Handle plain authentication errors\n+\t\t\teval {\n+\t\t\t\t$result = $smtp->auth($cred->{'username'}, $cred->{'password'});\n+\t\t\t\t1; # Ensure true value is returned\n+\t\t\t} or do {\n+\t\t\t\t$error = $@ || 'Unknown error';\n+\t\t\t};\n \t\t}\n-\n-\t\treturn !!$smtp->auth($cred->{'username'}, $cred->{'password'});\n+\t\t# Unified error handling logic\n+\t\tif ($error) {\n+\t\t\t# Match temporary errors\n+\t\t\tif ($error =~ /timeout|temporary|greylist|throttled|quota\\s+exceeded|queue|overload|try\\s+again|connection\\s+lost|network\\s+error/i) {\n+\t\t\t\twarn \"SMTP temporary error: $error\";\n+\t\t\t\treturn 1;\n+\t\t\t}\n+\t\t\treturn 0;\n+\t\t}\n+\t\treturn !!$result;\n \t});\n-\n \treturn $auth;\n }\n \n-- \n2.49.0.rc0.57.gdb91954e18\n\n"},{"id":"513948","messageId":"CAOLa=ZThgmKkYErnxUAGOhHyFcFrYDrcQ8KvqqkMBJ3wEckVPw@mail.gmail.com","threadId":"63073","inReplyTo":"20250307123934.2064275-2-05ZYT30@gmail.com","subject":"Re: [PATCH 1/1] Improve SMTP authentication error handling logic","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-10T22:14:18Z","receivedAt":"2025-03-10T22:14:20Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Zheng Yuting <05zyt30@gmail.com> writes:\n\nI see that you've added a cover message detailing the patch, but\nindividual patches should include the relevant commit description and\nmessage. This is also outlined in 'Documentation/SubmittingPatches'.\n\nThat said my perl knowledge is limited, but I'll ask some questions\nanyways.\n\n> ---\n>  git-send-email.perl | 30 ++++++++++++++++++++++--------\n>  1 file changed, 22 insertions(+), 8 deletions(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 798d59b84f..a012d61abb 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1419,19 +1419,19 @@ sub/ smtp_auth_maybe {\n>  \t\tdie \"invalid smtp auth: '${smtp_auth}'\";\n>  \t}\n>\n> -\t# TODO: Authentication may fail not because credentials were\n> +\t# Authentication may fail not because credentials were\n>  \t# invalid but due to other reasons, in which we should not\n>  \t# reject credentials.\n>  \t$auth = Git::credential({\n>  \t\t'protocol' => 'smtp',\n>  \t\t'host' => smtp_host_string(),\n>  \t\t'username' => $smtp_authuser,\n> -\t\t# if there's no password, \"git credential fill\" will\n> -\t\t# give us one, otherwise it'll just pass this one.\n\nI didn't understand why these comments were removed.\n\n>  \t\t'password' => $smtp_authpass\n> +\n\nNit: unnecessary newline.\n\n>  \t}, sub {\n>  \t\tmy $cred = shift;\n> -\n> +\t\tmy $result;\n> +\t\tmy $error;\n\nNit: It is easier to read with a newline separating the variable\ndeclaration with the rest of the code.\n\n>  \t\tif ($smtp_auth) {\n>  \t\t\tmy $sasl = Authen::SASL->new(\n>  \t\t\t\tmechanism => $smtp_auth,\n> @@ -1441,13 +1441,27 @@ sub smtp_auth_maybe {\n>  \t\t\t\t\tauthname => $cred->{'username'},\n>  \t\t\t\t}\n>  \t\t\t);\n> -\n\nNit: unnecessary newline removal\n\n>  \t\t\treturn !!$smtp->auth($sasl);\n\nShouldn't we do the same kind of changes for this `smpt->auth()` as the\nother below?\n\n> +\t\t} else {\n> +\t\t\t# Handle plain authentication errors\n> +\t\t\teval {\n> +\t\t\t\t$result = $smtp->auth($cred->{'username'}, $cred->{'password'});\n> +\t\t\t\t1; # Ensure true value is returned\n> +\t\t\t} or do {\n> +\t\t\t\t$error = $@ || 'Unknown error';\n> +\t\t\t};\n\nOkay, so here we try to do the authentication and if there aren't any\nexceptions we return 1.\n\n>  \t\t}\n> -\n> -\t\treturn !!$smtp->auth($cred->{'username'}, $cred->{'password'});\n> +\t\t# Unified error handling logic\n> +\t\tif ($error) {\n> +\t\t\t# Match temporary errors\n> +\t\t\tif ($error =~ /timeout|temporary|greylist|throttled|quota\\s+exceeded|queue|overload|try\\s+again|connection\\s+lost|network\\s+error/i) {\n\nWhere do we get this list from? Regex matching errors for the type\ndoesn't seem like the best way to go about this.\n\n> +\t\t\t\twarn \"SMTP temporary error: $error\";\n> +\t\t\t\treturn 1;\n> +\t\t\t}\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t\treturn !!$result;\n>  \t});\n> -\n\nNit: unnecessary newline removal\n\n>  \treturn $auth;\n>  }\n>\n> --\n> 2.49.0.rc0.57.gdb91954e18\n"},{"id":"513972","messageId":"CAMvj1+on36gmtRF_yKKwxSBvhoLK3-=DM0girxwEriASsvPL3Q@mail.gmail.com","threadId":"63073","inReplyTo":"CAOLa=ZThgmKkYErnxUAGOhHyFcFrYDrcQ8KvqqkMBJ3wEckVPw@mail.gmail.com","subject":"Re: [PATCH 1/1] Improve SMTP authentication error handling logic","fromName":"Yuting Zheng","fromEmail":"05zyt30@gmail.com","sentAt":"2025-03-11T08:03:50Z","receivedAt":"2025-03-11T08:04:02Z","isPatch":true,"sender":{"key":"05zyt30@gmail.com","avatar":"https://avatars.githubusercontent.com/u/87643662?v=4"},"body":"On Tue Mar 11, 2025 at 6:14 AM CST, Karthik Nayak wrote:\n> Zheng Yuting <05zyt30@gmail.com> writes:\n>\n> I see that you've added a cover message detailing the patch, but\n> individual patches should include the relevant commit description and\n> message. This is also outlined in 'Documentation/SubmittingPatches'.\n\nThanks for pointing out my mistakes, I'm sorry to ignore the individual\npatch commits. I will add them in my next version.\n\n>\n> That said my perl knowledge is limited, but I'll ask some questions\n> anyways.\n>\n\nI am grateful for your support. As a new contributor, I’m still learning\nthe best practices and will improve my process based on your feedback.\n\n>> ->--->---# if there's no password, \"git credential fill\" will\n>> ->--->---# give us one, otherwise it'll just pass this one.\n>\n> I didn't understand why these comments were removed.\n>\n\nI removed those comments because I intended to use the password directly\nat that point. I understand the confusion this caused and will restore\nor clarify the comments in my next revision.\n\n>>  >--->---'password' => $smtp_authpass\n>> +\n>\n> Nit: unnecessary newline.\n>\n\nI’ll remove it.\n\n>>  >---}, sub {\n>>  >--->---my $cred = shift;\n>> -\n>> +>--->---my $result;\n>> +>--->---my $error;\n>\n> Nit: It is easier to read with a newline separating the variable\n> declaration with the rest of the code.\n>\n>> @@ -1441,13 +1441,27 @@ sub smtp_auth_maybe {\n>>  >--->--->--->--->---authname => $cred->{'username'},\n>>  >--->--->--->---}\n>>  >--->--->---);\n>> -\n>\n> Nit: unnecessary newline removal\n\nI will adjust these suggestions in the next version.\n\n>\n>>  >--->--->---return !!$smtp->auth($sasl);\n>\n> Shouldn't we do the same kind of changes for this `smpt->auth()` as the\n> other below?\n>\n\nI’ll review both instances of the smtp->auth() call and update them so they\nfollow a consistent approach in error handling and formatting.\n\n>> +>--->---if ($error) {\n>> +>--->--->---# Match temporary errors\n>> +>--->--->---if ($error =~ /timeout|temporary|greylist|throttled|quota\\s+exceeded|queue|overload|try\\s+again|connection\\s+lost|network\\s+error/i) {\n>\n> Where do we get this list from? Regex matching errors for the type\n> doesn't seem like the best way to go about this.\n>\n\nI agree with your assessment. Initially, I aimed to interpret the SMTP\nserver's responses directly. But after considering your suggestion, I\nrealized that I should use SMTP servers return standardized status codes\nfor command outcomes according to RFC 5321 (4.2.2 Reply Codes by Function\nGroups). I will improve it.\n\n>> +>--->---return !!$result;\n>>  >---});\n>> -\n>\n> Nit: unnecessary newline removal\n>\n\nI will adjust these suggestions in the next version.\n\nOn Tue, Mar 11, 2025 at 6:14 AM Karthik Nayak <karthik.188@gmail.com> wrote:\n>\n> Zheng Yuting <05zyt30@gmail.com> writes:\n>\n> I see that you've added a cover message detailing the patch, but\n> individual patches should include the relevant commit description and\n> message. This is also outlined in 'Documentation/SubmittingPatches'.\n>\n> That said my perl knowledge is limited, but I'll ask some questions\n> anyways.\n>\n> > ---\n> >  git-send-email.perl | 30 ++++++++++++++++++++++--------\n> >  1 file changed, 22 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/git-send-email.perl b/git-send-email.perl\n> > index 798d59b84f..a012d61abb 100755\n> > --- a/git-send-email.perl\n> > +++ b/git-send-email.perl\n> > @@ -1419,19 +1419,19 @@ sub/ smtp_auth_maybe {\n> >               die \"invalid smtp auth: '${smtp_auth}'\";\n> >       }\n> >\n> > -     # TODO: Authentication may fail not because credentials were\n> > +     # Authentication may fail not because credentials were\n> >       # invalid but due to other reasons, in which we should not\n> >       # reject credentials.\n> >       $auth = Git::credential({\n> >               'protocol' => 'smtp',\n> >               'host' => smtp_host_string(),\n> >               'username' => $smtp_authuser,\n> > -             # if there's no password, \"git credential fill\" will\n> > -             # give us one, otherwise it'll just pass this one.\n>\n> I didn't understand why these comments were removed.\n>\n> >               'password' => $smtp_authpass\n> > +\n>\n> Nit: unnecessary newline.\n>\n> >       }, sub {\n> >               my $cred = shift;\n> > -\n> > +             my $result;\n> > +             my $error;\n>\n> Nit: It is easier to read with a newline separating the variable\n> declaration with the rest of the code.\n>\n> >               if ($smtp_auth) {\n> >                       my $sasl = Authen::SASL->new(\n> >                               mechanism => $smtp_auth,\n> > @@ -1441,13 +1441,27 @@ sub smtp_auth_maybe {\n> >                                       authname => $cred->{'username'},\n> >                               }\n> >                       );\n> > -\n>\n> Nit: unnecessary newline removal\n>\n> >                       return !!$smtp->auth($sasl);\n>\n> Shouldn't we do the same kind of changes for this `smpt->auth()` as the\n> other below?\n>\n> > +             } else {\n> > +                     # Handle plain authentication errors\n> > +                     eval {\n> > +                             $result = $smtp->auth($cred->{'username'}, $cred->{'password'});\n> > +                             1; # Ensure true value is returned\n> > +                     } or do {\n> > +                             $error = $@ || 'Unknown error';\n> > +                     };\n>\n> Okay, so here we try to do the authentication and if there aren't any\n> exceptions we return 1.\n>\n> >               }\n> > -\n> > -             return !!$smtp->auth($cred->{'username'}, $cred->{'password'});\n> > +             # Unified error handling logic\n> > +             if ($error) {\n> > +                     # Match temporary errors\n> > +                     if ($error =~ /timeout|temporary|greylist|throttled|quota\\s+exceeded|queue|overload|try\\s+again|connection\\s+lost|network\\s+error/i) {\n>\n> Where do we get this list from? Regex matching errors for the type\n> doesn't seem like the best way to go about this.\n>\n> > +                             warn \"SMTP temporary error: $error\";\n> > +                             return 1;\n> > +                     }\n> > +                     return 0;\n> > +             }\n> > +             return !!$result;\n> >       });\n> > -\n>\n> Nit: unnecessary newline removal\n>\n> >       return $auth;\n> >  }\n> >\n> > --\n> > 2.49.0.rc0.57.gdb91954e18\n"}]}