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

From: Jeff King <peff@peff.net>
Date: Jul 9, 2024, 23:49 UTC
Message-ID: <20240709234941.GA1525171@coredump.intra.peff.net>
In-Reply-To: <xmqqed82cgmj.fsf@gitster.g>

On Tue, Jul 09, 2024 at 04:03:48PM -0700, Junio C Hamano wrote:

Show 26 quoted lines

> 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.

Show 20 quoted lines

> 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