Re: [PATCH/RFC] http.c: cookie file tightening

From: Piotr Szlazak <piotr.szlazak@gmail.com>
Date: Jul 10, 2024, 10:35 UTC
Message-ID: <2d76e43e-db79-4572-8f41-60fbbea10af6@gmail.com>
In-Reply-To: <20240709234941.GA1525171@coredump.intra.peff.net>

On 10.07.2024 01:49, Jeff King wrote:

Show 34 quoted lines

> On Tue, Jul 09, 2024 at 04:03:48PM -0700, Junio C Hamano wrote:
>
>> The manual pages https://curl.se/libcurl/c/CURLOPT_COOKIEFILE.html
>> and https://curl.se/libcurl/c/CURLOPT_COOKIEJAR.html talk about two
>> interesting special values.
>>
>>   * "" (an empty string) given to CURLOPT_COOKIEFILE means not to
>>     read cookies from any file upon startup.
>>
>>   * It is not specified what "" (an empty string) given to
>>     CURLOPT_COOKIEJAR does; presumably open a file whose name is an
>>     empty string and write cookies to it?  In any case, that is not
>>     what we want to see happen, ever.
>>
>>   * "-" (a dash) given to CURLOPT_COOKIEFILE makes cURL read cookies
>>     from the standard input, and given to CURLOPT_COOKIEJAR makes
>>     cURL write cookies to the standard output.  Neither of which we
>>     want ever to happen.
>>
>> So, let's make sure we avoid these nonsense cases.  Specifically,
>> when http.cookies is set to "-", ignore it with a warning, and when
>> it is set to "" and http.savecookies is set, ignore http.savecookies
>> with a warning.
>>
>> [...]
>>
>>   * I have no confidence in me doing http correctly, so I am asking
>>     from folks who have touched http.c in the past 6 months for help.
> I don't have any experience with any of the cookie options, but your
> explanation here all makes sense. It might be worth including a test,
> though the interesting part is probably how things broke _before_ this
> patch. After it, it's pretty obvious what should happen.
>
> So I'll try to comment from the general http.c perspective.

Hello! I'm able to perform some checks as I have Git repository behind HAProxy load balancer which sets HTTP cookie to record which backend should process consecutive requests.

Indeed, if http.cookieFile='-' is used, git stops and waits for input. It does *not* work even if I do: $ echo '/path/to/file' | git -c http.cookieFile='-' ...

On the other hand there is no problem if http.cookieFile='' and 
http.saveCookies=true is used together. Git operation is successful. But 
if GIT_TRACE_CURL=1 GIT_TRACE_CURL_NO_DATA=1 is enabled, I can see 
following warning it the output:
 > 12:19:56.280263 http.c:820 == Info: WARNING: failed to save cookies in
It comes from:
https://github.com/curl/curl/blob/master/lib/cookie.c#L1758
But cookies were accepted by the client and sent back to the server.

PS. I'm using Git 2.42.0.

Regards!

-- 
Piotr Szlazak

>
>> diff --git c/http.c w/http.c
>> index 13fa94bef3..86ccca49f0 100644
>> --- c/http.c
>> +++ w/http.c
>> @@ -1466,7 +1466,16 @@ struct active_request_slot *get_active_slot(void)
>>   	slot->finished = NULL;
>>   	slot->callback_data = NULL;
>>   	slot->callback_func = NULL;
>> +
>> +	if (curl_cookie_file && !strcmp(curl_cookie_file, "-")) {
>> +		warning(_("refusing to read cookies from http.cookiefile '-'"));
>> +		FREE_AND_NULL(curl_cookie_file);
>> +	}
>>   	curl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);
>> +	if (curl_save_cookies && (!curl_cookie_file || !curl_cookie_file[0])) {
>> +		curl_save_cookies = 0;
>> +		warning(_("ignoring http.savecookies for empty http.cookiefile"));
>> +	}
>>   	if (curl_save_cookies)
>>   		curl_easy_setopt(slot->curl, CURLOPT_COOKIEJAR, curl_cookie_file);
> This all looks OK to me. A few things I wondered while reading:
>
>    - is curl_cookie_file always an allocated string? The answer is yes,
>      because it comes from git_config_pathname(). Good.
>
>    - get_active_slot() will be called a lot of times, as we reuse the
>      curl handles over and over (the "slot" terminology is due to the
>      parallelism for dumb-http fetch; smart-http just reuses the one
>      handle each time). So is this the best place to put the check?
>
>      You actually unset the options when issuing the warning, so we'd
>      never see the warning multiple times, even if this code is run
>      repeatedly. Good.
>
>      I do suspect these curl_easy_setopt() calls for cookies could just
>      go into get_curl_handle(), which sets up the handle initially. But
>      it's possible there's some subtle reason why they're here, and
>      certainly moving them is orthogonal to your goal. And in the
>      meantime, putting your new checks alongside the use of the variables
>      makes sense.
>
> -Peff