Re: [PATCH v12 02/10] imap-send: add support for OAuth2.0 authentication

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 5, 2025, 16:28 UTC
Message-ID: <xmqq34ce9mql.fsf@gitster.g>
In-Reply-To: <20250605080002.GA2998537@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 20 quoted lines

> On Mon, Jun 02, 2025 at 04:29:33PM +0530, Aditya Garg wrote:
>
>> @@ -1405,7 +1558,11 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
>>  
>>  	server_fill_credential(srvc, cred);
>>  	curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
>> -	curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
>> +
>> +	if (!srvc->auth_method ||
>> +	    strcmp(srvc->auth_method, "XOAUTH2") ||
>> +	    strcmp(srvc->auth_method, "OAUTHBEARER"))
>> +		curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
>
> Coverity complains that this "if" will always be true, since one of the
> strcmp() calls must return non-zero (srvc->auth_method cannot match both
> strings!).
>
> I'm not sure what the logic is supposed to be here. If we are matching
> either string, it should be !strcmp() for both. If we want to match
> neither, then it should be &&, not ||.

"If XOAUTH2 or OAUTHBEARER, use the password" sounds somewhat strange (unless the bearer token is stored in .pass and passed as if it is a password).

"Unless XOAUTH2 or OAUTHBEARER, use the password" sounds even more strange. What about other methods that are not a plain simple password authentication? Will we remember extending this code when we add yet another one to exclude it like XOAUTH2 and OAUTHBEARER are excluded with this patch?